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

> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: Wednesday, July 29, 2026 9:36 AM
> To: Torreno, Alexis Czezar <[email protected]>; Rob Herring
> <[email protected]>; Krzysztof Kozlowski <[email protected]>; Conor Dooley
> <[email protected]>; Jonathan Corbet <[email protected]>; Shuah Khan
> <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for
> max20830c and max20840c
> 
> [External]
> 
> On 7/28/26 17:25, Torreno, Alexis Czezar wrote:
> >> On 7/27/26 23:58, Torreno, Alexis Czezar wrote:
> >>>>
> >>>>>>
> >>>>>> Are those chips still not published ? I find MAX20840T, but no "C"
> variants.
> >>>>>> And MAX20840T presumably has an I2C device ID of "MAX20840", not
> >>>>>> "MAX20840C".
> >>>>>>
> >>>>>> I also noticed that MAX20810 and MAX20815 seem to be register
> >>>> compatible.
> >>>>>>
> >>>>>
> >>>>> I believe so yes, they aren't yet.
> >>>>>
> >>>>
> >>>> I just hope they are really compatible, and that the device ID
> >>>> strings really include the "C". The "T" variants seem to have no T
> >>>> in the device ID string, making it a bit odd that it was (or will
> >>>> be) added for the
> >> C variants.
> >>>>
> >>>
> >>> Yeah the T is weird, but I did test the C variants and they do reply the 'c'
> char.
> >>>
> >>>>> Actually MAX20810/815 and a few more are next in line after this.
> >>>>> A different person is handling it, but they're waiting on how this patches
> go.
> >>>>>
> >>>>
> >>>> You are making yourself more work than necessary. Knowing that
> >>>> there are more chips coming, the sequence of strcmp() is not really
> >>>> that desirable anymore.
> >>>> It might make sense to create an array with all chips supported by
> >>>> the driver instead of adding up strcmp sequences. That would make
> >>>> it much easier to add support for new variants.
> >>>>
> >>>> There also seems to be a MAX20830T. Does it actually make sense to
> >>>> list the variants (C/T) in the first place ?
> >>>>
> >>>
> >>> Will think about how to make to scale the code when adding newer
> variants.
> >>>
> >>
> >> Something like the following should do.
> >>
> >> const char *supported_chips[] = {
> >> 	"MAX20830",
> >> 	"MAX20830C",
> >> 	"MAX20840",
> >> 	"MAX20840C"
> >> };
> >>
> >> ...
> >> 	for (i = 0, supported = false; i < ARRAY_SIZE(supported_chips) &&
> >> !supported; i++)
> >> 		supported = !strcmp(buf, supported_chips[i]);
> >>
> >> 	if (!supported)
> >> 		return dev_err_probe(&client->dev, -ENODEV,
> >> 				     "Unsupported device: '%*pE'\n", ret, buf);
> >>
> >> Then all you'd need to do to add more chips would be to add the
> >> strings to the supported_chips[] array.
> >>
> >
> > For some reason they didn't ask me to add MAX20840, and I did double check
> with them.
> >
> 
> Odd. Well, guess we can add it later if/when someone asks for it.
> 
> > Code wise, I ended up with this. (I guess am trying to avoid mailing a
> > v6)
> >
> > ...
> > static const char * const supported_chip_ids[] = {
> > 	"MAX20830",
> > 	"MAX20830C",
> > 	"MAX20840C",
> > };
> > ...
> > 	for (i = 0; i < ARRAY_SIZE(supported_chip_ids); i++) {
> > 		if (!strcmp(buf, supported_chip_ids[i]))
> > 			goto id_ok;
> > 	}
> >
> > 	return dev_err_probe(&client->dev, -ENODEV,
> > 			     "Unsupported device: '%*pE'\n", ret, buf);
> >
> > id_ok:
> > ... (probe after)
> >
> 
> If you don't like the extra variable, you could avoid the goto with
> 
>   	for (i = 0; i < ARRAY_SIZE(supported_chip_ids); i++) {
>   		if (!strcmp(buf, supported_chip_ids[i]))
>   			break;
>   	}
> 
> 	if (i == ARRAY_SIZE(supported_chip_ids)
> 	 	return dev_err_probe(&client->dev, -ENODEV,
>   				     "Unsupported device: '%*pE'\n", ret, buf);
> 

Looks nice! will send v5 within the week. Thank you!