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: <d49d4b4c-e7ee-e0a1-56e6-7f193e0d1340@roeck-us.net>
Date:   Mon, 9 Jan 2023 16:04:45 -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:     Jonathan Cormier <jcormier@criticallink.com>,
        linux-hwmon@vger.kernel.org
Cc:     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>
References: <20221214220727.1350784-3-jcormier@criticallink.com>
 <20230109233534.1932370-1-jcormier@criticallink.com>
 <20230109233534.1932370-4-jcormier@criticallink.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20230109233534.1932370-4-jcormier@criticallink.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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;