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

[Guenter Roeck]

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

Thanks,
Guenter