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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing compatible strings for MAX20830C and MAX20840C in the of_match_table.
- [Medium] I2C block read path truncates the device ID for 9-character names, leading to potential false positive matches.
- [Low] struct i2c_device_id array uses positional instead of named initializers.
--

--- Patch [4]: [PATCH v3 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c ---
commit dca1bbdb54e3f769bf22d26e97ead67754787a95
Author: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>

hwmon: (pmbus/max20830): add support for max20830c and max20840c
    
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.

> diff --git a/drivers/hwmon/pmbus/max20830.c b/drivers/hwmon/pmbus/max20830.c
> index 7a5e67bd33c0a..73fe59685cf15 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c
> @@ -98,13 +98,12 @@ static int max20830_probe(struct i2c_client *client)

[ ... ]

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

[Severity: Medium]
Since MAX20830C and MAX20840C have 9-character names, does this hardcoded
MAX20830_IC_DEVICE_ID_LENGTH + 1 truncate the newly added device IDs?

In max20830_probe(), if the I2C controller lacks SMBUS_READ_BLOCK_DATA support,
the fallback i2c_smbus_read_i2c_block_data() call uses
MAX20830_IC_DEVICE_ID_LENGTH. Because this length was initially sized for the
8-character "MAX20830" identifier, could it under-read the new longer device
IDs and cause false positive matches in the I2C fallback path?

[ ... ]

> @@ -120,26 +119,28 @@ static int max20830_probe(struct i2c_client *client)

[ ... ]

>  static const struct i2c_device_id max20830_id[] = {
> -	{"max20830"},
> +	{ "max20830" },
> +	{ "max20830c" },
> +	{ "max20840c" },

[Severity: High]
Are the compatible strings for max20830c and max20840c missing from the
max20830_of_match[] table?

Without adding them alongside the newly added i2c_device_id entries, could
this break module autoloading on systems using Device Tree when the OS attempts
to autoload the driver via the udev modalias?

[Severity: Low]
This isn't a bug, but do these entries violate the I2C subsystem coding style
by using positional initializers?

The I2C client guidelines require initialized arrays of type
struct i2c_device_id to use named initializers (e.g., { .name = "max20830c" }).

>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, max20830_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dev-max20830c-v3-0-9d06d27d6da3@analog.com?part=4