From: Guenter Roeck <linux@roeck-us.net>
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>
Subject: Re: [PATCH v3 3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store
Date: Tue, 10 Jan 2023 10:22:54 -0800 [thread overview]
Message-ID: <dd80ee77-3b71-ee47-2744-36b09e0ec372@roeck-us.net> (raw)
In-Reply-To: <CADL8D3YEkZaOjUY3mRLGT0M+b7MwN5zQZrbsw5W8Mn=PJ7PtcA@mail.gmail.com>
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, ®val);
>>> + 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
next prev parent reply other threads:[~2023-01-10 18:28 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-14 22:07 [PATCH 0/2] hwmon: ltc2945: Add binding and shunt resistor support Cormier, Jonathan
2022-12-14 22:07 ` [PATCH 1/2] dt-bindings: hwmon: adi,ltc2945: Add binding Cormier, Jonathan
2022-12-15 9:42 ` Krzysztof Kozlowski
[not found] ` <CADL8D3ZUE5WbV0oS6hEVUNh9asrhTKQeGR4McR6Kh6qykSFw=Q@mail.gmail.com>
2022-12-15 14:37 ` Jon Cormier
2022-12-14 22:07 ` [PATCH 2/2] hwmon: ltc2945: Allow setting shunt resistor Cormier, Jonathan
2022-12-15 15:11 ` Guenter Roeck
2022-12-15 19:42 ` Jon Cormier
2023-01-09 23:35 ` [PATCH v3 0/2] hwmon: ltc2945: Add binding and shunt resistor support Jonathan Cormier
2023-01-09 23:35 ` [PATCH v3 1/5] dt-bindings: hwmon: adi,ltc2945: Add binding Jonathan Cormier
2023-01-10 9:07 ` Krzysztof Kozlowski
2023-01-09 23:35 ` [PATCH v3 2/5] hwmon: ltc2945: Add devicetree match table Jonathan Cormier
2023-01-09 23:35 ` [PATCH v3 3/5] hwmon: ltc2945: Handle error case in ltc2945_value_store Jonathan Cormier
2023-01-10 0:04 ` Guenter Roeck
2023-01-10 18:19 ` Jon Cormier
2023-01-10 18:22 ` Guenter Roeck [this message]
2023-01-10 19:25 ` Jon Cormier
2023-01-12 0:44 ` Guenter Roeck
2023-01-18 18:32 ` Jon Cormier
2023-01-09 23:35 ` [PATCH v3 4/5] hwmon: ltc2945: Allow setting shunt resistor Jonathan Cormier
2023-01-09 23:35 ` [PATCH v3 5/5] hwmon: ltc2945: Convert division to DIV_ROUND_CLOSEST_ULL Jonathan Cormier
2022-12-20 0:04 ` [PATCH v2 0/2] hwmon: ltc2945: Add binding and shunt resistor support Cormier, Jonathan
2022-12-20 0:04 ` [PATCH v2 1/4] dt-bindings: hwmon: adi,ltc2945: Add binding Cormier, Jonathan
2022-12-20 10:15 ` Krzysztof Kozlowski
2022-12-20 14:35 ` Jon Cormier
2022-12-20 14:46 ` Guenter Roeck
2022-12-20 21:47 ` Jon Cormier
2022-12-20 0:04 ` [PATCH v2 2/4] hwmon: ltc2945: Add devicetree match table Cormier, Jonathan
2022-12-28 16:43 ` Guenter Roeck
2022-12-20 0:04 ` [PATCH v2 3/4] hwmon: ltc2945: Allow setting shunt resistor Cormier, Jonathan
2022-12-28 16:49 ` Guenter Roeck
2022-12-20 0:04 ` [PATCH v2 4/4] hwmon: ltc2945: Fix possible overflows Cormier, Jonathan
2022-12-28 17:01 ` Guenter Roeck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dd80ee77-3b71-ee47-2744-36b09e0ec372@roeck-us.net \
--to=linux@roeck-us.net \
--cc=bduke@criticallink.com \
--cc=devicetree@vger.kernel.org \
--cc=dvincelette@criticallink.com \
--cc=jcormier@criticallink.com \
--cc=jdelvare@suse.com \
--cc=jpruitt@criticallink.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).