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

[Joel Stanley]

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 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.

> +{
> +       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?

Cheers,

Joel