Re: drivers (pmbus): ir35221: Set PMBUS_PAGE before reading id and model

[Eddie 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@xxxxxxxxxxxxxxxxxx wrote:
> > > > From: "Edward A. James" <eajames@xxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxx>
> > > > ---
> > > >   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");