On Wed, 2017-08-02 at 16:45 +0930, Andrew Jeffery 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 I expect I'll be dropping this patch. At this point it looks like we have another chip on the bus interfering with transactions to the MAX31785. Checking the behaviour with a scope showed that SCL was being held down by something that wasn't the master, but according to Maxim the SCL pin on the MAX31785 is input-only and therefore it cannot interfere in the manner we observed. Further testing has narrowed down the list of candidates for the interference, but our investigation is ongoing. Andrew > --- >  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 c2b693badcea..509b1a5a49b9 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 > + * > + * 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) > +{ > > + 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_i2c_smbus_write_word_data(struct i2c_client *client, > > +       int command, u16 data) > +{ > > + int ret; > + > > + ret = i2c_smbus_write_word_data(client, command, data); > > + if (ret == -EIO) > > + ret = i2c_smbus_write_word_data(client, command, data); > + > > + return ret; > +} > + > +static int max31785_pmbus_write_word_data(struct i2c_client *client, int page, > > +   int command, u16 data) > +{ > > + int ret; > + > > + ret = pmbus_write_word_data(client, page, command, data); > > + if (ret == -EIO) > > + ret = pmbus_write_word_data(client, page, command, data); > + > > + return ret; > +} > + >  static int max31785_read_byte_data(struct i2c_client *client, int page, > >      int reg) >  { > @@ -210,6 +259,31 @@ static int max31785_read_word_data(struct i2c_client *client, int page, > >   return rv; >  } >   > +static int max31785_update_fan(struct i2c_client *client, int page, > > +        u8 config, u8 mask, u16 command) > +{ > > + int from, rv; > > + u8 to; > + > > + from = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12); > > + if (from < 0) > > + return from; > + > > + to = (from & ~mask) | (config & mask); > + > > + if (to != from) { > > + rv = pmbus_write_byte_data(client, page, PMBUS_FAN_CONFIG_12, > > +    to); > > + if (rv < 0) > > + return rv; > > + } > + > > + rv = max31785_pmbus_write_word_data(client, page, PMBUS_FAN_COMMAND_1, > > +     command); > + > > + return rv; > +} > + >  static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0xffff }; >   >  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); > > + } > >   case PMBUS_VIRT_PWM_ENABLE_1: > >   if (word >= ARRAY_SIZE(max31785_pwm_modes)) > >   return -EINVAL; >   > > - return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM, > > - max31785_pwm_modes[word]); > > + return max31785_update_fan(client, page, 0, PB_FAN_1_RPM, > > +    max31785_pwm_modes[word]); > >   default: > >   break; > >   } > @@ -262,7 +348,7 @@ static int max31785_of_fan_config(struct i2c_client *client, > >   return -ENXIO; > >   } >   > > - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > > + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > >   if (ret < 0) > >   return ret; >   > @@ -419,7 +505,8 @@ static int max31785_of_fan_config(struct i2c_client *client, > >   if (ret < 0) > >   return ret; >   > > - ret = i2c_smbus_write_word_data(client, MFR_FAN_CONFIG, mfr_cfg); > > + ret = max31785_i2c_smbus_write_word_data(client, MFR_FAN_CONFIG, > > +  mfr_cfg); > >   if (ret < 0) > >   return ret; >   > @@ -473,7 +560,7 @@ static int max31785_of_tmp_config(struct i2c_client *client, > >   return -ENXIO; > >   } >   > > - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > > + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page); > >   if (ret < 0) > >   return ret; >   > @@ -631,7 +718,8 @@ static int max31785_probe(struct i2c_client *client, > >   if (!have_fan || fan_configured) > >   continue; >   > > - ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i); > > + ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, > > +  i); > >   if (ret < 0) > >   return ret; >   > @@ -640,8 +728,9 @@ static int max31785_probe(struct i2c_client *client, > >   return ret; >   > >   ret &= ~PB_FAN_1_INSTALLED; > > - ret = i2c_smbus_write_word_data(client, PMBUS_FAN_CONFIG_12, > > - ret); > > + ret = max31785_i2c_smbus_write_word_data(client, > > +  PMBUS_FAN_CONFIG_12, > > +  ret); > >   if (ret < 0) > >   return ret; > >   }