diff mbox series

[v3,3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store

Message ID 20230109233534.1932370-4-jcormier@criticallink.com
State New
Headers
Series hwmon: ltc2945: Add binding and shunt resistor support |

Commit Message

Jonathan Cormier Jan. 9, 2023, 11:35 p.m. UTC 
  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.

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(-)

Comments

Guenter Roeck Jan. 10, 2023, 12:04 a.m. UTC | #1 
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;
Jonathan Cormier Jan. 10, 2023, 6:19 p.m. UTC | #2 
On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> 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.
I can do that. However, you had also mentioned changing the return
type to match what the calling function was expecting, an unsigned
long. But I can't do that since error codes are negative so it would
be a signed long which would lose precision and seemingly defeat the
point of matching the variable type the caller wants.  I could make it
a signed long long but that still doesn't match.  So it seemed saner
to just return the error and the value separately, that way the
function declaration was explicit about the types it wanted/returned,
and less room for error.  Would love to know your preferred solution.

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


--
Jonathan Cormier
Software Engineer

Voice:  315.425.4045 x222



http://www.CriticalLink.com
6712 Brooklawn Parkway, Syracuse, NY 13211
Guenter Roeck Jan. 10, 2023, 6:22 p.m. UTC | #3 
On 1/10/23 10:19, Jon Cormier wrote:
> On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> 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.
> I can do that. However, you had also mentioned changing the return
> type to match what the calling function was expecting, an unsigned
> long. But I can't do that since error codes are negative so it would
> be a signed long which would lose precision and seemingly defeat the
> point of matching the variable type the caller wants.  I could make it
> a signed long long but that still doesn't match.  So it seemed saner
> to just return the error and the value separately, that way the
> function declaration was explicit about the types it wanted/returned,
> and less room for error.  Would love to know your preferred solution.
> 

That is only true if the upper bit is actually ever set in that signed long.
Which means I'll have to verify if "would lose precision" is actually
a correct statement.

Guenter

>>
>> 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;
>>
> 
> 
> --
> Jonathan Cormier
> Software Engineer
> 
> Voice:  315.425.4045 x222
> 
> 
> 
> http://www.CriticalLink.com
> 6712 Brooklawn Parkway, Syracuse, NY 13211
Jonathan Cormier Jan. 10, 2023, 7:25 p.m. UTC | #4 
On Tue, Jan 10, 2023 at 1:22 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/10/23 10:19, Jon Cormier wrote:
> > On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> 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.
> > I can do that. However, you had also mentioned changing the return
> > type to match what the calling function was expecting, an unsigned
> > long. But I can't do that since error codes are negative so it would
> > be a signed long which would lose precision and seemingly defeat the
> > point of matching the variable type the caller wants.  I could make it
> > a signed long long but that still doesn't match.  So it seemed saner
> > to just return the error and the value separately, that way the
> > function declaration was explicit about the types it wanted/returned,
> > and less room for error.  Would love to know your preferred solution.
> >
>
> That is only true if the upper bit is actually ever set in that signed long.
> Which means I'll have to verify if "would lose precision" is actually
> a correct statement.
I'd like to argue that is another reason to go with this change
instead of working out the math of just how many bits are needed in
the worst case and having to document it. And potentially getting that
calculation wrong.  But I can if you'd like me to.
>
> Guenter
>
> >>
> >> 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;
> >>
> >
> >
> > --
> > Jonathan Cormier
> > Software Engineer
> >
> > Voice:  315.425.4045 x222
> >
> >
> >
> > http://www.CriticalLink.com
> > 6712 Brooklawn Parkway, Syracuse, NY 13211
>
Guenter Roeck Jan. 12, 2023, 12:44 a.m. UTC | #5 
On Tue, Jan 10, 2023 at 02:25:37PM -0500, Jon Cormier wrote:
> On Tue, Jan 10, 2023 at 1:22 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 1/10/23 10:19, Jon Cormier wrote:
> > > On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >>
> > >> 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.
> > > I can do that. However, you had also mentioned changing the return
> > > type to match what the calling function was expecting, an unsigned
> > > long. But I can't do that since error codes are negative so it would
> > > be a signed long which would lose precision and seemingly defeat the
> > > point of matching the variable type the caller wants.  I could make it
> > > a signed long long but that still doesn't match.  So it seemed saner
> > > to just return the error and the value separately, that way the
> > > function declaration was explicit about the types it wanted/returned,
> > > and less room for error.  Would love to know your preferred solution.
> > >
> >
> > That is only true if the upper bit is actually ever set in that signed long.
> > Which means I'll have to verify if "would lose precision" is actually
> > a correct statement.
> I'd like to argue that is another reason to go with this change
> instead of working out the math of just how many bits are needed in
> the worst case and having to document it. And potentially getting that
> calculation wrong.  But I can if you'd like me to.

You are turning things on its head. We don't make changes like that
because of maybe. It is you who has to show that the change is
necessary, and that there is indeed a loss of precision otherwise.

Guenter
Jonathan Cormier Jan. 18, 2023, 6:32 p.m. UTC | #6 
Alright, I'll take another pass at it.

On Wed, Jan 11, 2023 at 7:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Jan 10, 2023 at 02:25:37PM -0500, Jon Cormier wrote:
> > On Tue, Jan 10, 2023 at 1:22 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On 1/10/23 10:19, Jon Cormier wrote:
> > > > On Mon, Jan 9, 2023 at 7:04 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > >>
> > > >> 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.
> > > > I can do that. However, you had also mentioned changing the return
> > > > type to match what the calling function was expecting, an unsigned
> > > > long. But I can't do that since error codes are negative so it would
> > > > be a signed long which would lose precision and seemingly defeat the
> > > > point of matching the variable type the caller wants.  I could make it
> > > > a signed long long but that still doesn't match.  So it seemed saner
> > > > to just return the error and the value separately, that way the
> > > > function declaration was explicit about the types it wanted/returned,
> > > > and less room for error.  Would love to know your preferred solution.
> > > >
> > >
> > > That is only true if the upper bit is actually ever set in that signed long.
> > > Which means I'll have to verify if "would lose precision" is actually
> > > a correct statement.
> > I'd like to argue that is another reason to go with this change
> > instead of working out the math of just how many bits are needed in
> > the worst case and having to document it. And potentially getting that
> > calculation wrong.  But I can if you'd like me to.
>
> You are turning things on its head. We don't make changes like that
> because of maybe. It is you who has to show that the change is
> necessary, and that there is indeed a loss of precision otherwise.
>
> Guenter
diff mbox series

Patch

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;