* [PATCH] drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model @ 2017-10-19 20:57 Eddie James 2017-10-21 16:10 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Eddie James @ 2017-10-19 20:57 UTC (permalink / raw) To: linux; +Cc: jdelvare, linux-hwmon, linux-kernel, andrew, eajames, Edward A. James 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. 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"); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model 2017-10-19 20:57 [PATCH] drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model Eddie James @ 2017-10-21 16:10 ` Guenter Roeck 2017-10-23 15:23 ` Eddie James 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2017-10-21 16:10 UTC (permalink / raw) To: eajames; +Cc: jdelvare, linux-hwmon, linux-kernel, andrew, Edward A. James 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"); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model 2017-10-21 16:10 ` Guenter Roeck @ 2017-10-23 15:23 ` Eddie James 2017-10-23 22:12 ` Guenter Roeck 0 siblings, 1 reply; 5+ messages in thread From: Eddie James @ 2017-10-23 15:23 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, linux-hwmon, linux-kernel, andrew, Edward A. James 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"); ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model 2017-10-23 15:23 ` Eddie James @ 2017-10-23 22:12 ` Guenter Roeck 2017-10-25 19:54 ` Eddie James 0 siblings, 1 reply; 5+ messages in thread From: Guenter Roeck @ 2017-10-23 22:12 UTC (permalink / raw) To: Eddie James; +Cc: jdelvare, linux-hwmon, linux-kernel, andrew, Edward A. James 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"); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model 2017-10-23 22:12 ` Guenter Roeck @ 2017-10-25 19:54 ` Eddie James 0 siblings, 0 replies; 5+ messages in thread From: Eddie James @ 2017-10-25 19:54 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, linux-hwmon, linux-kernel, andrew, Edward A. James 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"); ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-10-25 19:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-19 20:57 [PATCH] drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model Eddie James 2017-10-21 16:10 ` Guenter Roeck 2017-10-23 15:23 ` Eddie James 2017-10-23 22:12 ` Guenter Roeck 2017-10-25 19:54 ` Eddie James
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox