linux-kernel - Re: [PATCH v2 2/2] hwmon:(ina238)Add support for SQ52206

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <c7c3503f-526e-41f3-8b82-ba7c7c68e89e@roeck-us.net>
Date: Mon, 13 Jan 2025 09:45:48 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Wenliang Yan <wenliang202407@....com>
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] hwmon:(ina238)Add support for SQ52206

On Mon, Jan 13, 2025 at 11:50:23AM +0800, Wenliang Yan wrote:
> Add support for SQ52206 to the Ina238 driver. Add registers,
> add calculation formulas, increase compatibility, add
> compatibility programs for multiple chips.
> 
> Signed-off-by: Wenliang Yan <wenliang202407@....com>
> ---
...
> @@ -452,6 +554,9 @@ static umode_t ina238_is_visible(const void *drvdata,
>  				 enum hwmon_sensor_types type,
>  				 u32 attr, int channel)
>  {
> +	const struct ina238_data *data = drvdata;
> +	bool has_power_highest = data->config->has_power_highest;
> +
>  	switch (type) {
>  	case hwmon_in:
>  		switch (attr) {
> @@ -479,6 +584,10 @@ static umode_t ina238_is_visible(const void *drvdata,
>  			return 0444;
>  		case hwmon_power_max:
>  			return 0644;
> +		case hwmon_power_input_highest:
> +			if (has_power_highest)
> +				return 0444;
> +			break;

This doesn't work as intended. The break; results in the code
entering the hwmon_temp: case. It has to be "return 0;" or
the entire function needs to be rewritten to use "break;"
to exit the switch statements, and "return 0;" at the end
of the function.

>  static int ina238_probe(struct i2c_client *client)
>  {
...
> -	if (data->gain == 1)
> +	config = data->config->config_default;
> +	if (chip == sq52206)
> +		if (data->gain == 1)
> +			config |= SQ52206_CONFIG_ADCRANGE_HIGH; /* ADCRANGE = 10/11 is /1 */
> +		else if (data->gain == 2)
> +			config |= SQ52206_CONFIG_ADCRANGE_LOW; /* ADCRANGE = 01 is /2 */
> +	else if (data->gain == 1)

The "else" here is the else from "if (data->gain == 2)" which is wrong.
The entire if/else block from "if (chip == sq52206)" needs to be in {}
to avoid that.

>  		config |= INA238_CONFIG_ADCRANGE; /* ADCRANGE = 1 is /1 */
>  	ret = regmap_write(data->regmap, INA238_CONFIG, config);
>  	if (ret < 0) {
> @@ -605,7 +735,8 @@ static int ina238_probe(struct i2c_client *client)
>  
>  	hwmon_dev = devm_hwmon_device_register_with_info(dev, client->name, data,
>  							 &ina238_chip_info,
> -							 NULL);
> +							 data->config->has_energy ?
> +								NULL : ina238_groups);

This seems to be wrong check. I would assume that ina238_groups is passed
if data->config->has_energy is true, not if it is false.

Guenter