Messages in this thread | | | Date | Mon, 9 Jan 2023 16:04:45 -0800 | Subject | Re: [PATCH v3 3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store | From | Guenter Roeck <> |
| |
On 1/9/23 15:35, Jonathan Cormier wrote: > ltc2945_val_to_reg errors were not being handled > which would have resulted in register being set to > 0 (clamped) instead of being left alone. > > Change reg_to_val and val_to_reg to return values > via parameters to make it more obvious when an > error case isn't handled. Also to allow > the regval type to be the correct sign in prep for > next commits. >
Sorry, I don't see that as reason or argument for such invasive changes. As far as I can see, a two-liner to check the return value of val_to_reg() should have been sufficient. Most of the rest, such as splitting the return value into two elements, is POV and just adds additional code and complexity for zero gain.
Guenter
> Fixes: 6700ce035f83 ("hwmon: Driver for Linear Technologies LTC2945") > > Signed-off-by: Jonathan Cormier <jcormier@criticallink.com> > --- > drivers/hwmon/ltc2945.c | 30 ++++++++++++++++++------------ > 1 file changed, 18 insertions(+), 12 deletions(-) > > diff --git a/drivers/hwmon/ltc2945.c b/drivers/hwmon/ltc2945.c > index 9af3e3821152..c66acf8d2124 100644 > --- a/drivers/hwmon/ltc2945.c > +++ b/drivers/hwmon/ltc2945.c > @@ -70,12 +70,12 @@ static inline bool is_power_reg(u8 reg) > } > > /* Return the value from the given register in uW, mV, or mA */ > -static long long ltc2945_reg_to_val(struct device *dev, u8 reg) > +static int ltc2945_reg_to_val(struct device *dev, u8 reg, u64 *regval) > { > struct regmap *regmap = dev_get_drvdata(dev); > unsigned int control; > u8 buf[3]; > - long long val; > + u64 val; > int ret; > > ret = regmap_bulk_read(regmap, reg, buf, > @@ -148,11 +148,12 @@ static long long ltc2945_reg_to_val(struct device *dev, u8 reg) > default: > return -EINVAL; > } > - return val; > + *regval = val; > + return 0; > } > > static int ltc2945_val_to_reg(struct device *dev, u8 reg, > - unsigned long val) > + unsigned long val, unsigned long *regval) > { > struct regmap *regmap = dev_get_drvdata(dev); > unsigned int control; > @@ -220,19 +221,21 @@ static int ltc2945_val_to_reg(struct device *dev, u8 reg, > default: > return -EINVAL; > } > - return val; > + *regval = val; > + return 0; > } > > static ssize_t ltc2945_value_show(struct device *dev, > struct device_attribute *da, char *buf) > { > struct sensor_device_attribute *attr = to_sensor_dev_attr(da); > - long long value; > + int ret; > + u64 value; > > - value = ltc2945_reg_to_val(dev, attr->index); > - if (value < 0) > - return value; > - return sysfs_emit(buf, "%lld\n", value); > + ret = ltc2945_reg_to_val(dev, attr->index, &value); > + if (ret < 0) > + return ret; > + return sysfs_emit(buf, "%llu\n", value); > } > > static ssize_t ltc2945_value_store(struct device *dev, > @@ -245,7 +248,7 @@ static ssize_t ltc2945_value_store(struct device *dev, > unsigned long val; > u8 regbuf[3]; > int num_regs; > - int regval; > + unsigned long regval; > int ret; > > ret = kstrtoul(buf, 10, &val); > @@ -253,7 +256,10 @@ static ssize_t ltc2945_value_store(struct device *dev, > return ret; > > /* convert to register value, then clamp and write result */ > - regval = ltc2945_val_to_reg(dev, reg, val); > + ret = ltc2945_val_to_reg(dev, reg, val, ®val); > + if (ret < 0) > + return ret; > + > if (is_power_reg(reg)) { > regval = clamp_val(regval, 0, 0xffffff); > regbuf[0] = regval >> 16;
| |