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

From: Guenter Roeck

Date: Wed Sep 16 2026 - 17:45:03 EST

Hi,

On 9/16/26 13:04, Pradhan, Sanman wrote:
From: Sanman Pradhan <psanman@xxxxxxxxxxx>

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@xxxxxxxxxxxxxxx
Signed-off-by: Sanman Pradhan <psanman@xxxxxxxxxxx>
---
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;
}