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

[Torreno, Alexis Czezar]

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

I may not know enough how important variants are when supporting them, the
C variant seems to be a package difference and the ID string.

> >>> Signed-off-by: Alexis Czezar Torreno
> >>> <alexisczezar.torreno@xxxxxxxxxx>
> >>> ---
> >>>    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 <alexisczezar.torreno@xxxxxxxxxx> @@
> >>> -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".
> 

Ok will double check this.