linux-kernel - Re: [PATCH v6 3/3] hwmon: (isl28022) support shunt voltage for ISL28022 power monitor

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6eae121b-3b0e-4938-8527-ec9a8a0b9aba@roeck-us.net>
Date: Fri, 6 Sep 2024 04:57:30 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>, patrick@...cx.xyz,
 Carsten Spieß <mail@...sten-spiess.de>,
 Jean Delvare <jdelvare@...e.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>,
 Geert Uytterhoeven <geert+renesas@...der.be>,
 Magnus Damm <magnus.damm@...il.com>, linux-hwmon@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-doc@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH v6 3/3] hwmon: (isl28022) support shunt voltage for
 ISL28022 power monitor

On 9/5/24 23:14, Delphine CC Chiu wrote:
> Added support reading shunt voltage in mV and revise code
> for Renesas ISL28022.
> 
> Signed-off-by: Delphine CC Chiu <Delphine_CC_Chiu@...ynn.com>
> ---
>   drivers/hwmon/isl28022.c | 93 ++++++++++++++++++++++++++++------------
>   1 file changed, 66 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/hwmon/isl28022.c b/drivers/hwmon/isl28022.c
> index f0494c3bd483..01220fad813d 100644
> --- a/drivers/hwmon/isl28022.c
> +++ b/drivers/hwmon/isl28022.c
> @@ -85,8 +85,6 @@ struct isl28022_data {
>   	u32			shunt;
>   	u32			gain;
>   	u32			average;
> -	u16			config;
> -	u16			calib;

I don't see the point of separating this part of the driver from the first patch
in the series. It makes me review code only to be replaced later. I am not going
to do that.

>   };
>   
>   static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
> @@ -95,20 +93,61 @@ static int isl28022_read(struct device *dev, enum hwmon_sensor_types type,
>   	struct isl28022_data *data = dev_get_drvdata(dev);
>   	unsigned int regval;
>   	int err;
> +	u16 sign_bit;
>   
>   	switch (type) {
>   	case hwmon_in:
> -		switch (attr) {
> -		case hwmon_in_input:
> -			err = regmap_read(data->regmap,
> -					  ISL28022_REG_BUS, &regval);
> -			if (err < 0)
> -				return err;
> -			/* driver supports only 60V mode (BRNG 11) */
> -			*val = (long)(((u16)regval) & 0xFFFC);
> +		switch (channel) {
> +		case 0:
> +			switch (attr) {
> +			case hwmon_in_input:
> +				err = regmap_read(data->regmap,
> +						  ISL28022_REG_BUS, &regval);
> +				if (err < 0)
> +					return err;
> +				/* driver supports only 60V mode (BRNG 11) */
> +				*val = (long)(((u16)regval) & 0xFFFC);
> +				break;
> +			default:
> +				return -EOPNOTSUPP;
> +			}
> +			break;
> +		case 1:
> +			switch (attr) {
> +			case hwmon_in_input:
> +				err = regmap_read(data->regmap,
> +						  ISL28022_REG_SHUNT, &regval);
> +				if (err < 0)
> +					return err;
> +			switch (data->gain) {
> +			case 8:
> +				sign_bit = (regval >> 15) & 0x01;
> +				*val = (long)((((u16)regval) & 0x7FFF) -
> +					   (sign_bit * 32768)) / 100;
> +				break;
> +			case 4:
> +				sign_bit = (regval >> 14) & 0x01;
> +				*val = (long)((((u16)regval) & 0x3FFF) -
> +					   (sign_bit * 16384)) / 100;
> +				break;
> +			case 2:
> +				sign_bit = (regval >> 13) & 0x01;
> +				*val = (long)((((u16)regval) & 0x1FFF) -
> +					   (sign_bit * 8192)) / 100;
> +				break;
> +			case 1:
> +				sign_bit = (regval >> 12) & 0x01;
> +				*val = (long)((((u16)regval) & 0x0FFF) -
> +					   (sign_bit * 4096)) / 100;
> +				break;
> +			}
> +			break;
> +			default:
> +				return -EOPNOTSUPP;
> +			}

Separate into its own voltage read function, and also provide separate functions
for current and power.

Guenter