Re: [PATCH 2/2] hwmon: (pmbus/max20860a) Add driver for Analog Devices MAX20860A

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Writing 0x20 to PMBUS_WRITE_PROTECT causes the PMBus core to lock sysfs configuration attributes as read-only.
--

commit 6a162d6f1757daa1f8dd5d77f833404928b847be
Author: Sanman Pradhan <psanman@juniper.net>

hwmon: (pmbus/max20860a) Add driver for Analog Devices MAX20860A

This commit adds a PMBus client driver for the Analog Devices MAX20860A
step-down DC-DC switching regulator. It registers the device with the PMBus
core and configures write protection during probe to enable configuration
access.

> diff --git a/drivers/hwmon/pmbus/max20860a.c b/drivers/hwmon/pmbus/max20860a.c
> new file mode 100644
> index 0000000000000..9af6888ed07e1
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max20860a.c
[ ... ]
> +/*
> + * Writing 0x20 to WRITE_PROTECT disables all write protection except
> + * for the WRITE_PROTECT command itself, enabling configuration access.
> + */
> +#define MAX20860A_WRITE_ENABLE	0x20
[ ... ]
> +static int max20860a_probe(struct i2c_client *client)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(client, PMBUS_WRITE_PROTECT,
> +					MAX20860A_WRITE_ENABLE);
> +	if (ret < 0)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to configure write protection\n");
> +
> +	return pmbus_do_probe(client, &max20860a_info);

[Severity: High]
Does this write to PMBUS_WRITE_PROTECT actually cause the PMBus core to lock
the sysfs configuration attributes as read-only?

The driver intentionally writes 0x20 to PMBUS_WRITE_PROTECT to allow runtime
configuration access. However, per the PMBus specification, 0x20 corresponds
to PB_WP_VOUT (BIT 5).

When pmbus_do_probe() is subsequently called, the PMBus core (in
pmbus_init_wp()) reads this register back. Seeing the 0x20 (PB_WP_VOUT) value,
the core sets the PMBUS_WRITE_PROTECTED flag in the driver's state. This
causes the PMBus core to unconditionally export all hardware monitoring limit
attributes as read-only (0444) in sysfs, which seems to defeat the intent of
enabling runtime configuration.

If 0x20 is indeed a device-specific value that enables writes, should the
driver either set PMBUS_NO_WRITE_PROTECT via platform data before calling
pmbus_do_probe(), or implement a read_byte_data hook to mask the read? Or if
the device complies with the standard PMBus specification, should 0x00 be
written instead to enable all writes?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260527045409.9092-1-sanman.pradhan@hpe.com?part=2