Re: [PATCH v2 4/4] hwmon: (kb9002) Add documentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The driver implements a custom private mutex to serialize access instead of using the mandated hwmon_lock() API.
- [Low] The i2c_device_id array is initialized without named initializers, violating I2C subsystem guidelines.
--

--- Patch [4]: [PATCH v2 4/4] hwmon: (kb9002) Add documentation ---
commit 9ba497f3aeb63905e9272367f2a7abaf03805f1c
Author: Andy Chung <Andy.Chung@amd.com>

hwmon: (kb9002) Add documentation
    
Document the sysfs and debugfs interfaces of the Kandou KB9002 hwmon
driver.

> 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 */
> +};

[Severity: Medium]
Does this custom mutex violate the hwmon locking guidelines?

The hwmon subsystem core already serializes operations for registered
attributes. For attributes registered by other means (such as the debugfs
entries added by this driver), the hwmon guidelines state that drivers
should use hwmon_lock() and hwmon_unlock() rather than implementing a
private lock.

Using a custom lock here results in unnecessary double-locking on the sysfs
read paths.

[ ... ]

> +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]
Can this lock be replaced with hwmon_lock()?

The private lock is taken here during firmware reads instead of relying on
hwmon core serialization or hwmon_lock().

[ ... ]

> +static int kb9002_smbus_hw_read(struct kb9002_data *data, u32 reg, u32 *val)
> +{
> +	struct i2c_client *client = data->client;
> +	u8 addr[4];
> +	u8 rbuf[I2C_SMBUS_BLOCK_MAX];
> +	int ret;
> +
> +	put_unaligned_le32(reg, addr);
> +
> +	mutex_lock(&data->lock);

[Severity: Medium]
Similarly, can this hardware read use hwmon_lock() instead of the custom
lock?

[ ... ]

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

[Severity: Low]
Does this array initialization violate I2C subsystem guidelines?

The I2C subsystem explicitly mandates that initialized arrays of type
struct i2c_device_id must use named initializers. Should this be updated
to use .name = KB9002_DEV_NAME?

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