Re: [PATCH v2 3/4] hwmon: (kb9002) Add driver for Kandou KB9002 retimer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver implements its own internal mutex for locking hardware accesses instead of using the hwmon subsystem's `hwmon_lock()` API.
- [Low] The `i2c_device_id` array is initialized using a positional instead of a named initializer.
--

--- Patch [3]: [PATCH v2 3/4] hwmon: (kb9002) Add driver for Kandou KB9002 retimer ---
commit 59c6448090144292eb629f4257436694b1eaaa0d
Author: Andy Chung <Andy.Chung@amd.com>

hwmon: (kb9002) Add driver for Kandou KB9002 retimer
    
The Kandou KB9002 is an 8-lane PCIe 5.0 retimer that exposes an SMBus
target with mandatory PEC. Add a hwmon driver reporting the firmware
aggregated maximum die temperature as temp1_input, with the firmware
version and boot status under debugfs.

> diff --git a/drivers/hwmon/kb9002.c b/drivers/hwmon/kb9002.c
> new file mode 100644
> index 0000000000000..94c7d95b3fff5
> --- /dev/null
> +++ b/drivers/hwmon/kb9002.c
[ ... ]
> +struct kb9002_data {
> +	struct i2c_client *client;
> +	struct mutex lock;	/* serialises register accesses */
> +};
[ ... ]
> +static int kb9002_fw_read(struct kb9002_data *data, u16 reg, u32 *val)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 addr[2];
> +	u8 rbuf[I2C_SMBUS_BLOCK_MAX];
> +	int ret;
> +
> +	put_unaligned_le16(reg, addr);
> +
> +	mutex_lock(&data->lock);

[Severity: Medium]
Is this custom mutex necessary? The hwmon subsystem guidelines state that
drivers should use hwmon_lock() and hwmon_unlock() to serialize operations
for attributes registered by other means.

By using a custom internal mutex, does the driver bypass the standard
subsystem synchronization architecture and introduce inefficient double-locking
on sysfs reads, which are already protected by the hwmon core?

[ ... ]
> +static const struct i2c_device_id kb9002_id[] = {
> +	{ KB9002_DEV_NAME },
> +	{ }
> +};

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

The I2C client initialization rules request that initialized arrays of type
struct i2c_device_id use named initializers rather than positional
initializers.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715-kb9002-upstream-v2-0-2fd390383da5@amd.com?part=3