Re: [PATCH v0 1/2] hwmon: pmbus: Fix vout_mode parsing

[Guenter Roeck]

On 9/3/26 17:53, Aryan Srivastava wrote:
> Currently this parsing assumes the top-most bit of the register is
> unset, allowing it to match against all 2bit numbers. In the case where
> the top bit is set, the parsing fails as the extracted value is always a
> 3 bit number.
>
> AND the read value after the bit shift to ensure only 2 bits are being
> parsed out.

This isn't that easy. Bit 7 is for "relative mode" starting with
PMBus v1.3. Relative mode is not currently supported by the driver,
and ignoring the bit could potentially be fatal.

A patch adding support for it was submitted a while ago.
https://patchwork.kernel.org/project/linux-hwmon/patch/20240315151855.377627-2-lars.petter.mostad@xxxxxxxxxx/
Unfortunately I never got to test it. This or something similar will
be needed. Again, we can not just ignore the "relative mode" bit.

Thanks,
Guenter
> Signed-off-by: Aryan Srivastava <aryan.srivastava@xxxxxxxxxxxxxxxxxxx>
> ---
>   drivers/hwmon/pmbus/pmbus.c      | 2 +-
>   drivers/hwmon/pmbus/pmbus_core.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7a7eac90d62e..b629acbffbfb 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c
> @@ -121,7 +121,7 @@ static int pmbus_identify(struct i2c_client *client,
>   
>   		vout_mode = pmbus_read_byte_data(client, 0, PMBUS_VOUT_MODE);
>   		if (vout_mode >= 0 && vout_mode != 0xff) {
> -			switch (vout_mode >> 5) {
> +			switch ((vout_mode >> 5) & 0x3) {
>   			case 0:
>   				break;
>   			case 1:
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 5f69c1420b4e..55391db5b414 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -2753,7 +2753,7 @@ static int pmbus_identify_common(struct i2c_client *client,
>   		 * Not all chips support the VOUT_MODE command,
>   		 * so a failure to read it is not an error.
>   		 */
> -		switch (vout_mode >> 5) {
> +		switch ((vout_mode >> 5) & 0x3) {
>   		case 0:	/* linear mode      */
>   			if (data->info->format[PSC_VOLTAGE_OUT] != linear)
>   				return -ENODEV;