lkml.org 
[lkml]   [2023]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store
From
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, &regval);
> + if (ret < 0)
> + return ret;
> +
> if (is_power_reg(reg)) {
> regval = clamp_val(regval, 0, 0xffffff);
> regbuf[0] = regval >> 16;

\
 
 \ /
  Last update: 2023-03-26 23:34    [W:0.082 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site