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

[Torreno, Alexis Czezar]

> >>>>>
> >>>>>     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).
> 


I see, Ill base it on the max31785 and simplify it similar to code given. Thanks!

Regards,
Alexis