| Message ID | 1508446628-13112-1-git-send-email-eajames@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Oct 19, 2017 at 03:57:08PM -0500, eajames@linux.vnet.ibm.com wrote: > From: "Edward A. James" <eajames@us.ibm.com> > > 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. > 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 > Signed-off-by: Edward A. James <eajames@us.ibm.com> > --- > drivers/hwmon/pmbus/ir35221.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c > index 8b906b4..9a53930 100644 > --- a/drivers/hwmon/pmbus/ir35221.c > +++ b/drivers/hwmon/pmbus/ir35221.c > @@ -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"); -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/21/2017 11:10 AM, Guenter Roeck wrote: > On Thu, Oct 19, 2017 at 03:57:08PM -0500, eajames@linux.vnet.ibm.com wrote: >> From: "Edward A. James" <eajames@us.ibm.com> >> >> 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. >> > 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 > > Thanks, > Guenter > >> Signed-off-by: Edward A. James <eajames@us.ibm.com> >> --- >> drivers/hwmon/pmbus/ir35221.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c >> index 8b906b4..9a53930 100644 >> --- a/drivers/hwmon/pmbus/ir35221.c >> +++ b/drivers/hwmon/pmbus/ir35221.c >> @@ -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"); -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 23, 2017 at 10:23:02AM -0500, Eddie James wrote: > > > On 10/21/2017 11:10 AM, Guenter Roeck wrote: > >On Thu, Oct 19, 2017 at 03:57:08PM -0500, eajames@linux.vnet.ibm.com wrote: > >>From: "Edward A. James" <eajames@us.ibm.com> > >> > >>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. > >> > >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. This would be bug(s) in the PMBus core which need(s) to be fixed. > 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. > 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 > Thanks, > Eddie > > > > >Thanks, > >Guenter > > > >>Signed-off-by: Edward A. James <eajames@us.ibm.com> > >>--- > >> drivers/hwmon/pmbus/ir35221.c | 6 ++++++ > >> 1 file changed, 6 insertions(+) > >> > >>diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c > >>index 8b906b4..9a53930 100644 > >>--- a/drivers/hwmon/pmbus/ir35221.c > >>+++ b/drivers/hwmon/pmbus/ir35221.c > >>@@ -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"); > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/23/2017 05:12 PM, Guenter Roeck wrote: > On Mon, Oct 23, 2017 at 10:23:02AM -0500, Eddie James wrote: >> >> On 10/21/2017 11:10 AM, Guenter Roeck wrote: >>> On Thu, Oct 19, 2017 at 03:57:08PM -0500, eajames@linux.vnet.ibm.com wrote: >>>> From: "Edward A. James" <eajames@us.ibm.com> >>>> >>>> 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. >>>> >>> 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. > This would be bug(s) in the PMBus core which need(s) to be fixed. > >> 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. >> > 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 > > I'd rather focus on fixing the PMBus core if it really tries to set page #255. > > Guenter > >> Thanks, >> Eddie >> >>> Thanks, >>> Guenter >>> >>>> Signed-off-by: Edward A. James <eajames@us.ibm.com> >>>> --- >>>> drivers/hwmon/pmbus/ir35221.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c >>>> index 8b906b4..9a53930 100644 >>>> --- a/drivers/hwmon/pmbus/ir35221.c >>>> +++ b/drivers/hwmon/pmbus/ir35221.c >>>> @@ -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"); -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c index 8b906b4..9a53930 100644 --- a/drivers/hwmon/pmbus/ir35221.c +++ b/drivers/hwmon/pmbus/ir35221.c @@ -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");