Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <dd80ee77-3b71-ee47-2744-36b09e0ec372@roeck-us.net>
Date:   Tue, 10 Jan 2023 10:22:54 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v3 3/5] hwmon: ltc2945: Handle error case in
 ltc2945_value_store
Content-Language: en-US
To:     Jon Cormier <jcormier@criticallink.com>
Cc:     linux-hwmon@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Bob Duke <bduke@criticallink.com>,
        John Pruitt <jpruitt@criticallink.com>,
        Dan Vincelette <dvincelette@criticallink.com>
References: <20221214220727.1350784-3-jcormier@criticallink.com>
 <20230109233534.1932370-1-jcormier@criticallink.com>
 <20230109233534.1932370-4-jcormier@criticallink.com>
 <d49d4b4c-e7ee-e0a1-56e6-7f193e0d1340@roeck-us.net>
 <CADL8D3YEkZaOjUY3mRLGT0M+b7MwN5zQZrbsw5W8Mn=PJ7PtcA@mail.gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <CADL8D3YEkZaOjUY3mRLGT0M+b7MwN5zQZrbsw5W8Mn=PJ7PtcA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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