[PATCH linux dev-4.10 6/6] pmbus: max31785: Work around back-to-back writes with FAN_CONFIG_1_2

[Andrew Jeffery]

On Mon, 2017-07-31 at 20:57 +0930, Joel Stanley wrote:
> > On Sat, Jul 29, 2017 at 1:52 AM, Andrew Jeffery <andrew at aj.id.au> wrote:
> > Testing of the pmbus max31785 driver implementation revealed occasional
> > NACKs from the device. Attempting the same transaction immediately after
> > the failure appears to always succeed. The NACK has consistently been
> > observed to happen on the second write of back-to-back writes to the
> > device, where the first write is to FAN_CONFIG_1_2. The NACK occurs
> > against the first data byte transmitted by the master on the second
> > write. The behaviour has the hallmarks of a PMBus Device Busy response,
> > but the busy bit is not set in the status byte.
> > 
> > Work around this undocumented behaviour by retrying any back-to-back
> > writes that occur after first writing FAN_CONFIG_1_2.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew at aj.id.au>
> > ---
> >  drivers/hwmon/pmbus/max31785.c | 105 +++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 97 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > index a86919bc7294..1911658ffcf6 100644
> > --- a/drivers/hwmon/pmbus/max31785.c
> > +++ b/drivers/hwmon/pmbus/max31785.c
> > @@ -48,6 +48,55 @@ enum max31785_regs {
> > 
> >  #define MAX31785_NR_PAGES              23
> > 
> > +/*
> > + * MAX31785 dragons ahead
> 
>                 \||/
>                 |  @___oo
>       /\  /\   / (__,,,,|
>      ) /^\) ^\/ _)
>      )   /^\/   _)
>      )   _ /  / _)
>  /\  )/\/ ||  | )_)
> <  >      |(,,) )__)
>  ||      /    \)___)\
>  | \____(      )___) )___
>   \______(_______;;; __;;;


It's tempting to stick that in the comment.

> 
> 
> 
> > + *
> > + * It seems there's an undocumented timing constraint when performing
> > + * back-to-back writes where the first write is to FAN_CONFIG_1_2. The device
> > + * provides no indication of this besides NACK'ing master Txs; no bits are set
> > + * in STATUS_BYTE to suggest anything has gone wrong.
> > + *
> > + * Given that, do a one-shot retry of the write.
> > + *
> > + * The __max31785_*_write_*_data() functions should be used at any point where
> > + * the prior write is to FAN_CONFIG_1_2.
> > + */
> > +static int __max31785_i2c_smbus_write_byte_data(struct i2c_client *client,
> > +                                               int command, u16 data)
> 
> Drop the __ prefix for these functions.

Aww. I liked how dangerous they looked.

> 
> > +{
> > +       int ret;
> > +
> > +       ret = i2c_smbus_write_byte_data(client, command, data);
> > +       if (ret == -EIO)
> > +               ret = i2c_smbus_write_byte_data(client, command, data);
> > +
> > +       return ret;
> > +}
> > +
> >  static int max31785_write_word_data(struct i2c_client *client, int page,
> > @@ -219,12 +293,24 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
> >                 return -ENXIO;
> > 
> >         switch (reg) {
> > +       case PMBUS_VIRT_FAN_TARGET_1:
> > +               return __max31785_update_fan(client, page, PB_FAN_1_RPM,
> > +                                            PB_FAN_1_RPM, word);
> > +       case PMBUS_VIRT_PWM_1:
> > +       {
> > +               u32 command = word;
> > +
> > +               command *= 100;
> > +               command /= 255;
> > +               return __max31785_update_fan(client, page, 0, PB_FAN_1_RPM,
> > +                                            command);
> 
> This looks like code from the pmbus core. Does this override the code
> from the core so we can use the magic try-twice update?

Yes.

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170801/ba709df6/attachment.sig>