drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model
PATCH 1/1 drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model
From: "Edward A. James" <[email protected]>
The MFR_ID and MFR_MODEL, which are manually read before probing the pmbus core, are only valid for the two pages that the ir35221 has available. Since we don't know the state of the device when we start probing, set the page number first before reading id and model.
--- drivers/hwmon/pmbus/ir35221.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/hwmon/pmbus/ir35221.c +6 -0
@@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, | I2C_FUNC_SMBUS_READ_BLOCK_DATA))return -ENODEV ; + ret =i2c_smbus_write_byte_data (client,PMBUS_PAGE ,0 ); +if (ret <0 ) { +dev_err (&client->dev,"Failed to set PMBUS_PAGE\n" ); +return ret; + } + ret =i2c_smbus_read_block_data (client,PMBUS_MFR_ID , buf);if (ret <0 ) {dev_err (&client->dev,"Failed to read PMBUS_MFR_ID\n" );
Signature · 1 line
1.8.3.1
Guenter Roeck
maintainerQuoted 7 lines · On Thu, Oct 19, 2017 at 03:57:08PM -0500, [email protected] wrote:
From: "Edward A. James" <[email protected]>
The
MFR_IDandMFR_MODEL, which are manually read before probing the pmbus core, are only valid for the two pages that the ir35221 has available. Since we don't know the state of the device when we start probing, set the page number first before reading id and model.
If the device only has two pages, it is highly unlikely that it is possible to select another page which is not supported; any attempts to do so should result in a command error.
Is this theory or based on an actual problem observed ? If so, it will require some additional explanation.
Thanks,
Guenter
Quoted 22 lines
Signed-off-by Edward A. James <[email protected]>--- drivers/hwmon/pmbus/ir35221.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/hwmon/pmbus/ir35221.c +6
-0@@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, | I2C_FUNC_SMBUS_READ_BLOCK_DATA))return -ENODEV ; + ret =i2c_smbus_write_byte_data (client,PMBUS_PAGE ,0 ); +if (ret <0 ) { +dev_err (&client->dev,"Failed to set PMBUS_PAGE\n" ); +return ret; + } + ret =i2c_smbus_read_block_data (client,PMBUS_MFR_ID , buf);if (ret <0 ) {dev_err (&client->dev,"Failed to read PMBUS_MFR_ID\n" );
Quoted 8 lines · Guenter Roeck
Quoted 7 lines · On Thu, Oct 19, 2017 at 03:57:08PM -0500, [email protected] wrote:
From: "Edward A. James" <[email protected]>
The
MFR_IDandMFR_MODEL, which are manually read before probing the pmbus core, are only valid for the two pages that the ir35221 has available. Since we don't know the state of the device when we start probing, set the page number first before reading id and model.
Quoted 6 lines
If the device only has two pages, it is highly unlikely that it is possible to select another page which is not supported; any attempts to do so should result in a command error.
Is this theory or based on an actual problem observed ? If so, it will require some additional explanation.
Hi,
Yes, I have observed this with a physical ir35221 device off an i2c bus. It seems that there are some places in the PMBUS core that manage to set the page number to 0xFF due to pmbus_set_page being called with page=-1. pmbus_set_page doesn't check the CML register, so if the i2c op succeeds, it will be a success. I can also set it to anything with i2cset tool without error.
I would also like to fix calling pmbus_set_page with -1, and I will send up a patch if/when I have time. But I felt it makes sense to also ensure the page is set correctly in the ir35221 driver, since it's an unknown state at probe time, and this device appears a bit "weird" in that it doesn't return an error if it's an unsupported page.
Thanks,
Eddie
Quoted 26 lines
Thanks,
GuenterQuoted 22 lines
Signed-off-by Edward A. James <[email protected]>--- drivers/hwmon/pmbus/ir35221.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/hwmon/pmbus/ir35221.c +6
-0@@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, | I2C_FUNC_SMBUS_READ_BLOCK_DATA))return -ENODEV ; + ret =i2c_smbus_write_byte_data (client,PMBUS_PAGE ,0 ); +if (ret <0 ) { +dev_err (&client->dev,"Failed to set PMBUS_PAGE\n" ); +return ret; + } + ret =i2c_smbus_read_block_data (client,PMBUS_MFR_ID , buf);if (ret <0 ) {dev_err (&client->dev,"Failed to read PMBUS_MFR_ID\n" );
Guenter Roeck
maintainerQuoted 15 lines · Eddie James
Quoted 12 lines · Guenter Roeck
Quoted 7 lines · On Thu, Oct 19, 2017 at 03:57:08PM -0500, [email protected] wrote:
From: "Edward A. James" <[email protected]>
The
MFR_IDandMFR_MODEL, which are manually read before probing the pmbus core, are only valid for the two pages that the ir35221 has available. Since we don't know the state of the device when we start probing, set the page number first before reading id and model.If the device only has two pages, it is highly unlikely that it is possible to select another page which is not supported; any attempts to do so should result in a command error.
Quoted 8 lines
Quoted 2 lines
Is this theory or based on an actual problem observed ? If so, it will require some additional explanation.
Hi,
Yes, I have observed this with a physical ir35221 device off an i2c bus. It seems that there are some places in the PMBUS core that manage to set the page number to
0xFFdue topmbus_set_pagebeing called with page=-1.
This would be bug(s) in the PMBus core which need(s) to be fixed.
Quoted 10 lines
pmbus_set_pagedoesn't check the CML register, so if the i2c op succeeds, it will be a success. I can also set it to anything with i2cset tool without error.I would also like to fix calling
pmbus_set_pagewith -1, and I will send up a patch if/when I have time. But I felt it makes sense to also ensure the page is set correctly in the ir35221 driver, since it's an unknown state at probe time, and this device appears a bit "weird" in that it doesn't return an error if it's an unsupported page.
With that logic, one could argue that we have to reprogram the entire chip because, after all, soneone could have messed it up completely using i2cset. And the same would be true for all the chips supported by any driver.
I'd rather focus on fixing the PMBus core if it really tries to set page #255.
Guenter
Quoted 29 lines
Thanks,
EddieQuoted 25 lines
Thanks,
GuenterQuoted 21 lines
Signed-off-by Edward A. James <[email protected]>--- drivers/hwmon/pmbus/ir35221.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/hwmon/pmbus/ir35221.c +6
-0@@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, | I2C_FUNC_SMBUS_READ_BLOCK_DATA))return -ENODEV ; + ret =i2c_smbus_write_byte_data (client,PMBUS_PAGE ,0 ); +if (ret <0 ) { +dev_err (&client->dev,"Failed to set PMBUS_PAGE\n" ); +return ret; + } + ret =i2c_smbus_read_block_data (client,PMBUS_MFR_ID , buf);if (ret <0 ) {dev_err (&client->dev,"Failed to read PMBUS_MFR_ID\n" );
Quoted 34 lines · Guenter Roeck
Quoted 14 lines · Eddie James
Quoted 12 lines · Guenter Roeck
Quoted 7 lines · On Thu, Oct 19, 2017 at 03:57:08PM -0500, [email protected] wrote:
From: "Edward A. James" <[email protected]>
The
MFR_IDandMFR_MODEL, which are manually read before probing the pmbus core, are only valid for the two pages that the ir35221 has available. Since we don't know the state of the device when we start probing, set the page number first before reading id and model.If the device only has two pages, it is highly unlikely that it is possible to select another page which is not supported; any attempts to do so should result in a command error.
Quoted 7 lines
Quoted 2 lines
Is this theory or based on an actual problem observed ? If so, it will require some additional explanation.
Hi,
Yes, I have observed this with a physical ir35221 device off an i2c bus. It seems that there are some places in the PMBUS core that manage to set the page number to
0xFFdue topmbus_set_pagebeing called with page=-1.This would be bug(s) in the PMBus core which need(s) to be fixed.
Quoted 10 lines
pmbus_set_pagedoesn't check the CML register, so if the i2c op succeeds, it will be a success. I can also set it to anything with i2cset tool without error.I would also like to fix calling
pmbus_set_pagewith -1, and I will send up a patch if/when I have time. But I felt it makes sense to also ensure the page is set correctly in the ir35221 driver, since it's an unknown state at probe time, and this device appears a bit "weird" in that it doesn't return an error if it's an unsupported page.
Quoted 3 lines
With that logic, one could argue that we have to reprogram the entire chip because, after all, soneone could have messed it up completely using i2cset. And the same would be true for all the chips supported by any driver.
Good point... Consider this patch abandoned. I have sent up a patch for pmbus core.
Thanks,
Eddie
Quoted 32 lines
I'd rather focus on fixing the PMBus core if it really tries to set page #255.
Guenter
Quoted 27 lines
Thanks,
EddieQuoted 24 lines
Thanks,
GuenterQuoted 21 lines
Signed-off-by Edward A. James <[email protected]>--- drivers/hwmon/pmbus/ir35221.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/hwmon/pmbus/ir35221.c +6
-0@@ -267,6 +267,12 @@ static int ir35221_probe(struct i2c_client *client, | I2C_FUNC_SMBUS_READ_BLOCK_DATA))return -ENODEV ; + ret =i2c_smbus_write_byte_data (client,PMBUS_PAGE ,0 ); +if (ret <0 ) { +dev_err (&client->dev,"Failed to set PMBUS_PAGE\n" ); +return ret; + } + ret =i2c_smbus_read_block_data (client,PMBUS_MFR_ID , buf);if (ret <0 ) {dev_err (&client->dev,"Failed to read PMBUS_MFR_ID\n" );