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

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