Hi Guenter-san, Yes you are correct. At first I thought to change the read and write functions to repeat for the error case. Then I confirmed the lm93 implementation and followed it basically since I thought it is better. But as you mentioned this is not an error handling so I will change to add the original retry functions.
Regards, Ikegami > -----Original Message----- > From: Guenter Roeck [mailto:[email protected]] On Behalf Of Guenter > Roeck > Sent: Thursday, July 12, 2018 11:00 AM > To: IKEGAMI Tokunori; Jean Delvare > Cc: PACKHAM Chris; [email protected] > Subject: Re: [PATCH 1/5] hwmon: adt7475: Split device update function > to measure and limits > > On 07/11/2018 06:04 PM, Tokunori Ikegami wrote: > > The function has the measure update part and limits and settings part. > > And those parts can be split so split them for a maintainability. > > > > Signed-off-by: Tokunori Ikegami <[email protected]> > > Cc: Chris Packham <[email protected]> > > Cc: [email protected] > > --- > > drivers/hwmon/adt7475.c | 214 > +++++++++++++++++++++++++++--------------------- > > 1 file changed, 122 insertions(+), 92 deletions(-) > > > > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c > > index 9ef84998c7f3..234f725213f9 100644 > > --- a/drivers/hwmon/adt7475.c > > +++ b/drivers/hwmon/adt7475.c > > @@ -1658,119 +1658,149 @@ static void adt7475_read_pwm(struct > i2c_client *client, int index) > > } > > } > > > > -static struct adt7475_data *adt7475_update_device(struct device > *dev) > > +static int adt7475_update_measure(struct device *dev) > > { > > struct i2c_client *client = to_i2c_client(dev); > > struct adt7475_data *data = i2c_get_clientdata(client); > > u16 ext; > > int i; > > > > - mutex_lock(&data->lock); > > + data->alarms = adt7475_read(REG_STATUS2) << 8; > > + data->alarms |= adt7475_read(REG_STATUS1); > > + > > + ext = (adt7475_read(REG_EXTEND2) << 8) | > > + adt7475_read(REG_EXTEND1); > > + for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) { > > + if (!(data->has_voltage & (1 << i))) > > + continue; > > + data->voltage[INPUT][i] = > > + (adt7475_read(VOLTAGE_REG(i)) << 2) | > > + ((ext >> (i * 2)) & 3); > > + } > > > > - /* Measurement values update every 2 seconds */ > > - if (time_after(jiffies, data->measure_updated + HZ * 2) || > > - !data->valid) { > > - data->alarms = adt7475_read(REG_STATUS2) << 8; > > - data->alarms |= adt7475_read(REG_STATUS1); > > - > > - ext = (adt7475_read(REG_EXTEND2) << 8) | > > - adt7475_read(REG_EXTEND1); > > - for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) { > > - if (!(data->has_voltage & (1 << i))) > > - continue; > > - data->voltage[INPUT][i] = > > - (adt7475_read(VOLTAGE_REG(i)) << 2) | > > - ((ext >> (i * 2)) & 3); > > - } > > + for (i = 0; i < ADT7475_TEMP_COUNT; i++) > > + data->temp[INPUT][i] = > > + (adt7475_read(TEMP_REG(i)) << 2) | > > + ((ext >> ((i + 5) * 2)) & 3); > > + > > + if (data->has_voltage & (1 << 5)) { > > + data->alarms |= adt7475_read(REG_STATUS4) << 24; > > + ext = adt7475_read(REG_EXTEND3); > > + data->voltage[INPUT][5] = adt7475_read(REG_VTT) << 2 | > > + ((ext >> 4) & 3); > > + } > > > > - for (i = 0; i < ADT7475_TEMP_COUNT; i++) > > - data->temp[INPUT][i] = > > - (adt7475_read(TEMP_REG(i)) << 2) | > > - ((ext >> ((i + 5) * 2)) & 3); > > + for (i = 0; i < ADT7475_TACH_COUNT; i++) { > > + if (i == 3 && !data->has_fan4) > > + continue; > > + data->tach[INPUT][i] = > > + adt7475_read_word(client, TACH_REG(i)); > > + } > > > > - if (data->has_voltage & (1 << 5)) { > > - data->alarms |= adt7475_read(REG_STATUS4) << > 24; > > - ext = adt7475_read(REG_EXTEND3); > > - data->voltage[INPUT][5] = > adt7475_read(REG_VTT) << 2 | > > - ((ext >> 4) & 3); > > - } > > + /* Updated by hw when in auto mode */ > > + for (i = 0; i < ADT7475_PWM_COUNT; i++) { > > + if (i == 1 && !data->has_pwm2) > > + continue; > > + data->pwm[INPUT][i] = adt7475_read(PWM_REG(i)); > > + } > > > > - for (i = 0; i < ADT7475_TACH_COUNT; i++) { > > - if (i == 3 && !data->has_fan4) > > - continue; > > - data->tach[INPUT][i] = > > - adt7475_read_word(client, > TACH_REG(i)); > > - } > > + if (data->has_vid) > > + data->vid = adt7475_read(REG_VID) & 0x3f; > > > > - /* Updated by hw when in auto mode */ > > - for (i = 0; i < ADT7475_PWM_COUNT; i++) { > > - if (i == 1 && !data->has_pwm2) > > - continue; > > - data->pwm[INPUT][i] = > adt7475_read(PWM_REG(i)); > > - } > > + return 0; > > +} > > > > - if (data->has_vid) > > - data->vid = adt7475_read(REG_VID) & 0x3f; > > +static int adt7475_update_limits(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct adt7475_data *data = i2c_get_clientdata(client); > > + int i; > > > > - data->measure_updated = jiffies; > > + data->config4 = adt7475_read(REG_CONFIG4); > > + data->config5 = adt7475_read(REG_CONFIG5); > > + > > + for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) { > > + if (!(data->has_voltage & (1 << i))) > > + continue; > > + /* Adjust values so they match the input precision */ > > + data->voltage[MIN][i] = > > + adt7475_read(VOLTAGE_MIN_REG(i)) << 2; > > + data->voltage[MAX][i] = > > + adt7475_read(VOLTAGE_MAX_REG(i)) << 2; > > } > > > > - /* Limits and settings, should never change update every 60 > seconds */ > > - if (time_after(jiffies, data->limits_updated + HZ * 60) || > > - !data->valid) { > > - data->config4 = adt7475_read(REG_CONFIG4); > > - data->config5 = adt7475_read(REG_CONFIG5); > > - > > - for (i = 0; i < ADT7475_VOLTAGE_COUNT; i++) { > > - if (!(data->has_voltage & (1 << i))) > > - continue; > > - /* Adjust values so they match the input > precision */ > > - data->voltage[MIN][i] = > > - adt7475_read(VOLTAGE_MIN_REG(i)) << 2; > > - data->voltage[MAX][i] = > > - adt7475_read(VOLTAGE_MAX_REG(i)) << 2; > > - } > > + if (data->has_voltage & (1 << 5)) { > > + data->voltage[MIN][5] = adt7475_read(REG_VTT_MIN) << > 2; > > + data->voltage[MAX][5] = adt7475_read(REG_VTT_MAX) << > 2; > > + } > > > > - if (data->has_voltage & (1 << 5)) { > > - data->voltage[MIN][5] = > adt7475_read(REG_VTT_MIN) << 2; > > - data->voltage[MAX][5] = > adt7475_read(REG_VTT_MAX) << 2; > > - } > > + for (i = 0; i < ADT7475_TEMP_COUNT; i++) { > > + /* Adjust values so they match the input precision */ > > + data->temp[MIN][i] = > > + adt7475_read(TEMP_MIN_REG(i)) << 2; > > + data->temp[MAX][i] = > > + adt7475_read(TEMP_MAX_REG(i)) << 2; > > + data->temp[AUTOMIN][i] = > > + adt7475_read(TEMP_TMIN_REG(i)) << 2; > > + data->temp[THERM][i] = > > + adt7475_read(TEMP_THERM_REG(i)) << 2; > > + data->temp[OFFSET][i] = > > + adt7475_read(TEMP_OFFSET_REG(i)); > > + } > > + adt7475_read_hystersis(client); > > > > - for (i = 0; i < ADT7475_TEMP_COUNT; i++) { > > - /* Adjust values so they match the input > precision */ > > - data->temp[MIN][i] = > > - adt7475_read(TEMP_MIN_REG(i)) << 2; > > - data->temp[MAX][i] = > > - adt7475_read(TEMP_MAX_REG(i)) << 2; > > - data->temp[AUTOMIN][i] = > > - adt7475_read(TEMP_TMIN_REG(i)) << 2; > > - data->temp[THERM][i] = > > - adt7475_read(TEMP_THERM_REG(i)) << 2; > > - data->temp[OFFSET][i] = > > - adt7475_read(TEMP_OFFSET_REG(i)); > > - } > > - adt7475_read_hystersis(client); > > + for (i = 0; i < ADT7475_TACH_COUNT; i++) { > > + if (i == 3 && !data->has_fan4) > > + continue; > > + data->tach[MIN][i] = > > + adt7475_read_word(client, TACH_MIN_REG(i)); > > + } > > > > - for (i = 0; i < ADT7475_TACH_COUNT; i++) { > > - if (i == 3 && !data->has_fan4) > > - continue; > > - data->tach[MIN][i] = > > - adt7475_read_word(client, > TACH_MIN_REG(i)); > > - } > > + for (i = 0; i < ADT7475_PWM_COUNT; i++) { > > + if (i == 1 && !data->has_pwm2) > > + continue; > > + data->pwm[MAX][i] = adt7475_read(PWM_MAX_REG(i)); > > + data->pwm[MIN][i] = adt7475_read(PWM_MIN_REG(i)); > > + /* Set the channel and control information */ > > + adt7475_read_pwm(client, i); > > + } > > > > - for (i = 0; i < ADT7475_PWM_COUNT; i++) { > > - if (i == 1 && !data->has_pwm2) > > - continue; > > - data->pwm[MAX][i] = > adt7475_read(PWM_MAX_REG(i)); > > - data->pwm[MIN][i] = > adt7475_read(PWM_MIN_REG(i)); > > - /* Set the channel and control information */ > > - adt7475_read_pwm(client, i); > > - } > > + data->range[0] = adt7475_read(TEMP_TRANGE_REG(0)); > > + data->range[1] = adt7475_read(TEMP_TRANGE_REG(1)); > > + data->range[2] = adt7475_read(TEMP_TRANGE_REG(2)); > > + > > + return 0; > > +} > > > > - data->range[0] = adt7475_read(TEMP_TRANGE_REG(0)); > > - data->range[1] = adt7475_read(TEMP_TRANGE_REG(1)); > > - data->range[2] = adt7475_read(TEMP_TRANGE_REG(2)); > > +static struct adt7475_data *adt7475_update_device(struct device > *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct adt7475_data *data = i2c_get_clientdata(client); > > + int ret; > > > > + mutex_lock(&data->lock); > > + > > + /* Measurement values update every 2 seconds */ > > + if (time_after(jiffies, data->measure_updated + HZ * 2) || > > + !data->valid) { > > + ret = adt7475_update_measure(dev); > > + if (ret < 0) { > > + data->valid = 0; > > + mutex_unlock(&data->lock); > > + return data; > > Maybe I am missing something, but I don't see the point of this code. > The errors still won't be returned to the user. The only effect is that > old values will be returned, and the next time around the read operation > will hopefully succeed. That isn't error handling, it is error > suppression. > > > + } > > + data->measure_updated = jiffies; > > + } > > + > > + /* Limits and settings, should never change update every 60 > seconds */ > > + if (time_after(jiffies, data->limits_updated + HZ * 60) || > > + !data->valid) { > > Is there any reason to believe that the limits have to be updated more > than once ? Does the chip have the habit of loosing its configuration ? > If it does, wouldn't it be better to _restore_ the limits to the ones > previously cached instead of silently discarding the configuration ? > > > + ret = adt7475_update_limits(dev); > > + if (ret < 0) { > > + data->valid = 0; > > + mutex_unlock(&data->lock); > > + return data; > > + } > > data->limits_updated = jiffies; > > data->valid = 1; > > } > >
