Re: [PATCH 1/2] hwmon: (pmbus/tps53679) Select page 0 for single-page TPS53676

[Guenter Roeck <linux@roeck-us.net>]

Hi,

On 9/16/26 13:04, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@juniper.net>
> 
> tps53676_identify() derives the number of PMBus pages but does not
> ensure that page 0 is selected for single-page configurations.
> pmbus_set_page() does not update the PAGE register when info->pages is
> 1, so if boot firmware leaves PAGE set to another value subsequent
> register accesses may target the wrong page.
> 
> For single-page devices, select page 0 explicitly and verify that the
> PAGE register was updated.
> 

Why that complexity ? We don't read back other registers. Why would it be
necessary or even make sense to do it here ? Following that logic one could
argue that every single write has to be read back to verify it.

> Fixes: cb3d37b59012 ("hwmon: (pmbus/tps53679) Add support for TI TPS53676")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sanman Pradhan <psanman@juniper.net>
> ---
>   drivers/hwmon/pmbus/tps53679.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c
> index dcd4250b679d..f38c19b8cc43 100644
> --- a/drivers/hwmon/pmbus/tps53679.c
> +++ b/drivers/hwmon/pmbus/tps53679.c
> @@ -248,6 +248,30 @@ static int tps53676_identify(struct i2c_client *client,
>   		info->phases[1] = phases_b;
>   	}
>   

A much simpler
	} else {
		/*
		 * pmbus_set_page() does not update the PAGE register on
		 * single-page devices, so select page 0 explicitly in case
		 * the boot firmware left the device on another page.
		 */
		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);
		if (ret < 0)
			return ret;
	}

should do the trick. Yes, the write may be unnecessary, but it
is cheaper than a read followed by an optional write.

Thanks,
Guenter
> +	/*
> +	 * pmbus_set_page() does not update the PAGE register on single-page
> +	 * devices, so select page 0 explicitly and verify it in case the
> +	 * boot firmware left the device on another page.
> +	 */
> +	if (info->pages == 1) {
> +		ret = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> +		if (ret < 0)
> +			return ret;
> +		if (ret != 0) {
> +			ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 0);
> +			if (ret < 0)
> +				return ret;
> +			ret = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> +			if (ret < 0)
> +				return ret;
> +			if (ret != 0) {
> +				dev_err(&client->dev,
> +					"failed to select page 0\n");
> +				return -EIO;
> +			}
> +		}
> +	}
> +
>   	return 0;
>   }
>