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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing OF match table entries for new devices breaks module auto-loading.
- [Low] The newly added entries in the i2c_device_id array use positional initializers instead of named initializers, violating I2C subsystem guidelines.
--

--- Patch [4]: [PATCH v4 4/4] hwmon: (pmbus/max20830): add support for max20830c and max20840c ---
commit 4a124678c88f942f9f2f9403e79e6632f0000f9f
Author: Alexis Czezar Torreno <[email protected]>

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 7c1e94c43ac4e..029733a377123 100644
> --- a/drivers/hwmon/pmbus/max20830.c
> +++ b/drivers/hwmon/pmbus/max20830.c

[ ... ]

> @@ -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" },

[Severity: Low]
This isn't a bug, but does the initialization of this array violate the I2C
subsystem rules?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must be declared const and use named initializers
instead of positional ones:

	{ .name = "max20830" },
	{ .name = "max20830c" },
	{ .name = "max20840c" },

[Severity: Medium]
Will this driver fail to auto-load via udev for the new devices when
instantiated via Device Tree?

It seems the new compatibles (e.g., "adi,max20830c" and "adi,max20840c")
were not added to max20830_of_match[] lower down in this file:

static const struct of_device_id max20830_of_match[] = {
	{ .compatible = "adi,max20830" },
	{ }
};

Without updating this array to include the new devices, the kernel won't
emit the correct MODALIAS for the new chips, which breaks module
auto-loading.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4