linux-kernel - Re: [PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match SY24655(SQ52205)

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: <e45d7a0b-c79c-4762-8b78-a8e595789752@wanadoo.fr>
Date: Wed, 11 Sep 2024 19:04:32 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Wenliang <wenliang202407@....com>, linux@...ck-us.net
Cc: jdelvare@...e.com, linux-hwmon@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH linux dev-6.11 1/2] hwmon: modified ina2xx to match
 SY24655(SQ52205)

Le 11/09/2024 à 14:25, Wenliang a écrit :
> After listening to your advice, I have adapted SQ52205 by rewriting the
> ina2xx driver.At the same time, I would like to clarify that SY24655 and
> SQ52205 are different partnumber of the same chip. Therefore, you can
> refer to SY24655FBP. I have also changed the naming within the driver to
> SY24655, and I hope to receive your response.
> 
> Signed-off-by: Wenliang <wenliang202407@....com>
> ---

Hi,

...

> @@ -103,7 +115,7 @@ static struct regmap_config ina2xx_regmap_config = {
>   	.val_bits = 16,
>   };
>   
> -enum ina2xx_ids { ina219, ina226 };
> +enum ina2xx_ids { ina219, ina226, sy24655};

Nitpick: Missing space before }

>   
>   struct ina2xx_config {
>   	u16 config_default;
> @@ -117,12 +129,13 @@ struct ina2xx_config {
>   
>   struct ina2xx_data {
>   	const struct ina2xx_config *config;
> -
> +
>   	long rshunt;
>   	long current_lsb_uA;
>   	long power_lsb_uW;
>   	struct mutex config_lock;
>   	struct regmap *regmap;
> +	struct i2c_client *client;
>   
>   	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>   };

...

> +static int sy24655_read_reg48(const struct i2c_client *client, u8 reg,
> +					long *accumulator_24, long *sample_count)
> +{
> +	u8 data[6];
> +	int err;

Maybe adding a blank line here?

> +	*accumulator_24 = 0;
> +	*sample_count = 0;
> +
> +	/* 48-bit register read */
> +	err = i2c_smbus_read_i2c_block_data(client, reg, 6, data);
> +	if (err < 0)
> +		return err;
> +	if (err != 6)
> +		return -EIO;
> +	*accumulator_24 = ((data[3] << 16) |
> +				(data[4] << 8) |
> +				data[5]);
> +	*sample_count = ((data[0] << 16) |
> +				(data[1] << 8) |
> +				data[2]);
> +
> +	return 0;
> +}
> +
> +static ssize_t sy24655_avg_power_show(struct device *dev,
> +					struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	long sample_count, accumulator_24, regval;
> +	int status;
> +
> +	status = sy24655_read_reg48(data->client, SY24655_EIN,
> +						&accumulator_24, &sample_count);
> +	if (status)
> +		return status;
> +	regval = DIV_ROUND_CLOSEST(accumulator_24, sample_count);
> +	regval = regval * data->power_lsb_uW;
> +
> +

Nitpick: unneeded extra empty line

> +	return sysfs_emit(buf, "%li\n", regval);
> +}
> +
>   /* shunt voltage */
>   static SENSOR_DEVICE_ATTR_RO(in0_input, ina2xx_value, INA2XX_SHUNT_VOLTAGE);
>   /* shunt voltage over/under voltage alert setting and alarm */
> @@ -589,9 +659,13 @@ static SENSOR_DEVICE_ATTR_RO(power1_crit_alarm, ina226_alarm,
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR_RW(shunt_resistor, ina2xx_shunt, INA2XX_CALIBRATION);
>   
> -/* update interval (ina226 only) */
> +/* update interval (ina226 and sy24655) */
>   static SENSOR_DEVICE_ATTR_RW(update_interval, ina226_interval, 0);
>   
> +/* calculate_avg_power (sy24655 only) */
> +static SENSOR_DEVICE_ATTR_RO(calculate_avg_power, sy24655_avg_power, 0);
> +
> +

Nitpick: unneeded extra empty line

>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -624,6 +698,15 @@ static struct attribute *ina226_attrs[] = {
>   static const struct attribute_group ina226_group = {
>   	.attrs = ina226_attrs,
>   };


...

> @@ -691,10 +775,17 @@ static int ina2xx_probe(struct i2c_client *client)
>   		dev_err(dev, "error configuring the device: %d\n", ret);
>   		return -ENODEV;
>   	}
> -
> +	if (chip == sy24655)
> +		ret = sy24655_init(data);
> +	if (ret < 0) {
> +		dev_err(dev, "error configuring the accum_reg: %d\n", ret);
> +		return -ENODEV;

return ret;?

> +	}
>   	data->groups[group++] = &ina2xx_group;
>   	if (chip == ina226)
>   		data->groups[group++] = &ina226_group;
> +	else if (chip == sy24655)
> +		data->groups[group++] = &sy24655_group;
>   
>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>   							   data, data->groups);
> @@ -713,6 +804,7 @@ static const struct i2c_device_id ina2xx_id[] = {
>   	{ "ina226", ina226 },
>   	{ "ina230", ina226 },
>   	{ "ina231", ina226 },
> +	{ "sy24655", sy24655},

Nitpick: missing space before }

>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ina2xx_id);
> @@ -738,6 +830,10 @@ static const struct of_device_id __maybe_unused ina2xx_of_match[] = {
>   		.compatible = "ti,ina231",
>   		.data = (void *)ina226
>   	},
> +	{
> +		.compatible = "silergy,sy24655",
> +		.data = (void *)sy24655
> +	},
>   	{ },

Nitpick: Unrelated, but this comma could be removed.

CJ

>   };
>   MODULE_DEVICE_TABLE(of, ina2xx_of_match);