Re: [PATCH 6/6] hwmon: (pmbus/max20830): add support for max20830c and max20840c

[Guenter Roeck]

On 7/1/26 19:50, Torreno, Alexis Czezar wrote:
> > > > >     static const struct of_device_id max20830_of_match[] = {
> > > > > -	{ .compatible = "adi,max20830" },
> > > > > +	{ .compatible = "adi,max20830", .data = &max20830_chip },
> > > > > +	{ .compatible = "adi,max20830c", .data = &max20830c_chip },
> > > > > +	{ .compatible = "adi,max20840c", .data = &max20840c_chip },
> > > >
> > > > "adi,max20830" is a fallback for the other two chips, but that is not
> > > > how the code is implemented.
> > >
> > > I may be inclined to just not use fallback as it seems to be more
> > > complicated and a bit unnecessary. There's also other devices that may
> > > be added on top of this so it lessens the complexity. Will edit the bindings
> > regarding this.
> > >
> >
> > You are missing the point. The fallback is perfectly valid. Technically the driver
> > does not even need to support adi,max20830c and adi,max20840c.
> > That is only used to validate that the chip is supported. That does not need a
> > separate devicetree entry.
>
> Checking if I understand this. I need to add a check that if DT chip->id = max20830,
> and if the HW returns string of either nmax20830c or max20840c, device still probes
>
> if ((buf == "max20830c" || buf  == "max20840c) && chip->id != "max20830""
>          // then it's an error...
> }

Technically, you don't even have to check the device ID in the first place.
You could drop all the device ID checking code.

Checking the device ID is just a safety feature that ensure that the
physical chip is supported by the driver. You could do something similar
to the max31785 driver, i.e., read the device ID and do a length check and
hard-coded string comparison. Separate compatible entries are not needed
at all.

Something like

	buf[ret] = '\0';
	if (ret != MAX20830_IC_DEVICE_ID_LENGTH ||
	    (strcmp(buf, "MAX20830") &&
	     strcmp(buf, "MAX20830C") &&
	     strcmp(buf, "MAX20840C"))
		return dev_err_probe(&client->dev, -ENODEV,
                                     "Unsupported device: '%*pE'\n", buf);

should do (using %*pE because the unknown device might not necessarily
have printable characters).

Guenter