linux-kernel - Re: [PATCH v3 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: <5f4095b0-8105-4001-81ac-e4f7053e45ae@wanadoo.fr>
Date: Fri, 24 Jan 2025 09:09:14 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Wenliang Yan <wenliang202407@....com>, linux@...ck-us.net,
 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>,
 linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] hwmon:(ina238)Add support for SQ52206

Le 17/01/2025 à 09:20, Wenliang Yan a écrit :
> 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>
> ---

Hi,

a few nitpick below, should a v4 be needed.

...

> +static ssize_t energy1_input_show(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	struct ina238_data *data = dev_get_drvdata(dev);
> +	int ret;
> +	u64 val;
> +
> +	ret = ina238_read_reg40(data->client, SQ52206_ENERGY, &val);
> +	if (ret)
> +		return ret;
> +
> +	/* result in microJoule */
> +	val = div_u64(val * 96 * INA238_FIXED_SHUNT * data->gain,
> +			       data->rshunt * 100);
> +
> +	return sprintf(buf, "%llu\n", val);

Maybe sysfs_emit()?

> +}
> +
>   static int ina238_read(struct device *dev, enum hwmon_sensor_types type,
>   		       u32 attr, int channel, long *val)
>   {

...

> @@ -530,6 +640,15 @@ static const struct hwmon_chip_info ina238_chip_info = {
>   	.info = ina238_info,
>   };
>   
> +/* energy attributes are 5bytes wide so we need u64 */

Missing space or done intentionally? (5 bytes)

> +static DEVICE_ATTR_RO(energy1_input);
> +
> +static struct attribute *ina238_attrs[] = {
> +	&dev_attr_energy1_input.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ina238);
> +
>   static int ina238_probe(struct i2c_client *client)
>   {
>   	struct ina2xx_platform_data *pdata = dev_get_platdata(&client->dev);
> @@ -537,13 +656,19 @@ static int ina238_probe(struct i2c_client *client)
>   	struct device *hwmon_dev;
>   	struct ina238_data *data;
>   	int config;
> +	enum ina238_ids chip;

Maybe move 1 line up, to keep RCT style?

>   	int ret;
>   
> +	chip = (uintptr_t)i2c_get_match_data(client);
> +
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;

...

> @@ -616,14 +747,26 @@ static int ina238_probe(struct i2c_client *client)
>   }
>   
>   static const struct i2c_device_id ina238_id[] = {
> -	{ "ina238" },
> +	{ "ina237", ina237 },
> +	{ "ina238", ina238 },
> +	{ "sq52206", sq52206 },
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(i2c, ina238_id);
>   
>   static const struct of_device_id __maybe_unused ina238_of_match[] = {
> -	{ .compatible = "ti,ina237" },
> -	{ .compatible = "ti,ina238" },
> +	{
> +		.compatible = "silergy,sq52206",
> +		.data = (void *)sq52206
> +	},
> +	{
> +		.compatible = "ti,ina237",
> +		.data = (void *)ina237
> +	},
> +	{
> +		.compatible = "ti,ina238",
> +		.data = (void *)ina238
> +	},
>   	{ },

While touching things here, this ending comma could be removed ro be 
consistent with the other struct just above.

>   };
>   MODULE_DEVICE_TABLE(of, ina238_of_match);
> @@ -642,3 +785,4 @@ module_i2c_driver(ina238_driver);
>   MODULE_AUTHOR("Nathan Rossi <nathan.rossi@...i.com>");
>   MODULE_DESCRIPTION("ina238 driver");
>   MODULE_LICENSE("GPL");
> +