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

On 7/27/26 22:05, Torreno, Alexis Czezar wrote:
>>
>> [External]
>>
>> On 7/27/26 20:32, Alexis Czezar Torreno wrote:
>>> Add support for MAX20830C and MAX20840 step-down DC-DC switching
>>> regulator with PMBus interface. MAX20830C is a different packaging for
>>> MAX20830, and MAX20840C supports 40A regulation compared to
>>> MAX20830 that is only 30A.
>>>
>>
>> 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.

> 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 ?

>>> Signed-off-by: Alexis Czezar Torreno <[email protected]>
>>> ---
>>>    Documentation/hwmon/max20830.rst | 27 ++++++++++++++++++++++-----
>>>    drivers/hwmon/pmbus/max20830.c   | 27 ++++++++++++++-------------
>>>    2 files changed, 36 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/hwmon/max20830.rst
>>> b/Documentation/hwmon/max20830.rst
>>> index
>>>
>> 936e409dcc5c0898dde27d782308d4a7e1357e73..b850f3b6e40d1f1d0cec944be
>> 40a
>>> f02265aced59 100644
>>> --- a/Documentation/hwmon/max20830.rst
>>> +++ b/Documentation/hwmon/max20830.rst
>>> @@ -13,6 +13,22 @@ Supported chips:
>>>
>>>        Datasheet:
>>> https://www.analog.com/media/en/technical-documentation/data-sheets/ma
>>> x20830.pdf
>>>
>>> +  * Analog Devices MAX20830C
>>> +
>>> +    Prefix: 'max20830c'
>>> +
>>> +    Addresses scanned: -
>>> +
>>> +    Datasheet:
>>> +
>>> +  * Analog Devices MAX20840C
>>> +
>>> +    Prefix: 'max20840c'
>>> +
>>> +    Addresses scanned: -
>>> +
>>> +    Datasheet:
>>> +
>>>    Author:
>>>
>>>      - Alexis Czezar Torreno <[email protected]> @@
>>> -21,12 +37,13 @@ Author:
>>>    Description
>>>    -----------
>>>
>>> -This driver supports hardware monitoring for Analog Devices MAX20830
>>> -Step-Down Switching Regulator with PMBus Interface.
>>> +This driver supports hardware monitoring for Analog Devices MAX20830,
>>> +MAX20830C and MAX20840C. These are Step-Down Switching Regulator
>> with PMBus Interface.
>>>
>>> -The MAX20830 is a 2.7V to 16V, 30A fully integrated step-down DC-DC
>>> switching -regulator. Through the PMBus interface, the device can
>>> monitor input/output -voltages, output current and temperature.
>>> +MAX20830, and MAX20830C are 2.7V to 16V, 30A fully integrated
>>> +step-down DC-DC switching regulators. MAX20840C is similar but can
>>> +reach 40A. Through the PMBus interface, these devices can monitor
>>> +input/output voltages, output current and temperature.
>>>
>>>    The driver is a client driver to the core PMBus driver. Please see
>>>    Documentation/hwmon/pmbus.rst for details on PMBus client drivers.
>>> diff --git a/drivers/hwmon/pmbus/max20830.c
>>> b/drivers/hwmon/pmbus/max20830.c index
>>>
>> 7c1e94c43ac4e5d1b4f69dc77b02e424a674e908..029733a377123ae6a861841d
>> b4a4
>>> 0dd540f1149e 100644
>>> --- a/drivers/hwmon/pmbus/max20830.c
>>> +++ b/drivers/hwmon/pmbus/max20830.c
>>> @@ -99,13 +99,12 @@ static int max20830_probe(struct i2c_client *client)
>>>    	 * which do not support SMBus block reads.
>>>    	 */
>>>    	if (i2c_check_functionality(client->adapter,
>> I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
>>> -		/* Reads 9 Data bytes from MAX20830 */
>>>    		ret = i2c_smbus_read_block_data(client,
>> PMBUS_IC_DEVICE_ID, buf);
>>>    		if (ret < 0)
>>>    			return dev_err_probe(&client->dev, ret,
>>>    					     "Failed to read IC_DEVICE_ID\n");
>>>    	} else {
>>> -		/* Reads 1 length byte + 9 Data bytes from MAX20830 */
>>> +		/* Reads 1 length byte + data bytes */
>>>    		ret = i2c_smbus_read_i2c_block_data(client,
>> PMBUS_IC_DEVICE_ID,
>>>
>> MAX20830_IC_DEVICE_ID_LENGTH + 1,
>>>    						    buf);
>>> @@ -121,26 +120,28 @@ static int max20830_probe(struct i2c_client
>> *client)
>>>    		ret = ret - 1;
>>>    	}
>>>
>>> -	/*
>>> -	 * MAX20830 IC_DEVICE_ID sends string data "MAX20830\0".
>>> -	 * Return value should at least be 9 bytes of data.
>>> -	 */
>>> +	/* Verify we read the expected number of bytes */
>>>    	if (ret < MAX20830_IC_DEVICE_ID_LENGTH)
>>>    		return dev_err_probe(&client->dev, -ENODEV,
>>> -				     "IC_DEVICE_ID too short: expected at least
>> 9 bytes, got %d\n",
>>> -				     ret);
>>> +				     "IC_DEVICE_ID too short: expected %d
>> bytes, got %d\n",
>>> +				     MAX20830_IC_DEVICE_ID_LENGTH, ret);
>>> +
>>> +	/* Null-terminate the string */
>>> +	buf[ret] = '\0';
>>>
>>> -	/* 9 bytes of data, buf[0]-buf[7] = "MAX20830", buf[8] = '\0' */
>>> -	buf[MAX20830_IC_DEVICE_ID_LENGTH - 1] = '\0';
>>> -	if (strncmp(buf, "MAX20830", MAX20830_IC_DEVICE_ID_LENGTH - 1))
>>> +	/* Verify the device ID matches what we expect */
>>> +	if ((strcmp(buf, "MAX20830") && strcmp(buf, "MAX20830C") &&
>>> +	    strcmp(buf, "MAX20840C")))
>>>    		return dev_err_probe(&client->dev, -ENODEV,
>>> -				     "Unsupported device: '%s'\n", buf);
>>> +				     "Unsupported device: '%*pE'\n", ret, buf);
>>>
>>>    	return pmbus_do_probe(client, &data->info);
>>>    }
>>>
>>>    static const struct i2c_device_id max20830_id[] = {
>>> -	{"max20830"},
>>> +	{ "max20830" },
>>> +	{ "max20830c" },
>>> +	{ "max20840c" },
>>
>> I don't think that is needed or warranted, given that there is no matching
>> devicetree node.
>>
> 
> I see, will remove.

Same for the prefix string in the documentation. You might want to mention the
supported chips, though, in both the configuration and Kconfig. In Kconfig,
that could be a generic such as "MAX20830 and compatible chips", or list the
base variants, such as "MAX20830, MAX20840 and variants".

Thanks,
Guenter