Re: [PATCH 1/2] hwmon: (pmbus) Add a NO_PEC flag to probe chips with faulty CAPABILITY

[Eddie James <eajames@linux.ibm.com>]

On Mon, 2020-12-21 at 08:54 -0800, Guenter Roeck wrote:
> On 12/21/20 8:30 AM, Eddie James wrote:
> > Some PMBus chips don't respond with valid data when reading the
> > CAPABILITY register. For instance the register may report that the
> > chip supports PEC when in reality it does not. For such chips, PEC
> > must not be enabled while probing the chip, so add a flag so that
> > device drivers can force PEC off.
> > 
> 
> I think the flag should indicate that the capability register
> shall not be read/used. That the capability register is currently
> only used to check for PEC is secondary. We might,for example,
> start using it to check for alert support or to check the numeric
> format.

OK, that makes sense. I'll rename the flag in v2, how does
PMBUS_NO_CAPABILITY sound?

Thanks for the quick reply,
Eddie

> 
> Thanks,
> Guenter
> 
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> > ---
> >  drivers/hwmon/pmbus/pmbus_core.c |  8 +++++---
> >  include/linux/pmbus.h            | 10 ++++++++++
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c
> > b/drivers/hwmon/pmbus/pmbus_core.c
> > index 192442b3b7a2..3de1657dde35 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -2204,9 +2204,11 @@ static int pmbus_init_common(struct
> > i2c_client *client, struct pmbus_data *data,
> >  	}
> >  
> >  	/* Enable PEC if the controller supports it */
> > -	ret = i2c_smbus_read_byte_data(client, PMBUS_CAPABILITY);
> > -	if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
> > -		client->flags |= I2C_CLIENT_PEC;
> > +	if (!(data->flags & PMBUS_NO_PEC)) {
> > +		ret = i2c_smbus_read_byte_data(client,
> > PMBUS_CAPABILITY);
> > +		if (ret >= 0 && (ret & PB_CAPABILITY_ERROR_CHECK))
> > +			client->flags |= I2C_CLIENT_PEC;
> > +	}
> >  
> >  	/*
> >  	 * Check if the chip is write protected. If it is, we can not
> > clear
> > diff --git a/include/linux/pmbus.h b/include/linux/pmbus.h
> > index 1ea5bae708a1..9bdc8a581b03 100644
> > --- a/include/linux/pmbus.h
> > +++ b/include/linux/pmbus.h
> > @@ -34,6 +34,16 @@
> >   */
> >  #define PMBUS_WRITE_PROTECTED	BIT(1)
> >  
> > +/*
> > + * PMBUS_NO_PEC
> > + *
> > + * Some PMBus chips don't respond with valid data when reading the
> > CAPABILITY
> > + * register. In this case, the register may report that the chip
> > supports PEC
> > + * with bit 7 (PB_CAPABILITY_ERROR_CHECK) when in reality it's not
> > supported.
> > + * For such chips, PEC must not be enabled before probing the
> > chip.
> > + */
> > +#define PMBUS_NO_PEC			BIT(2)
> > +
> >  struct pmbus_platform_data {
> >  	u32 flags;		/* Device specific flags */
> >  
> >