Re: [PATCH v3 2/2] hwmon:(ina238)Add support for SQ52206

[Wenliang Yan]

At 2025-01-24 16:09:14, "Christophe JAILLET" <christophe.jaillet@xxxxxxxxxx> wrote:
>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@xxxxxxx>
>> ---
>
>Hi,
>
>a few nitpick below, should a v4 be needed.

Hello,

Thank you for your review, I will update and submit V4.

>...
>
>> +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()?

Agree, I will make modifications.

>> +}
>> +
>>   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)

Agree, I will make modifications. (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?

Agree, I will make modifications.

>>   	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.

Okay, I will follow the same structure as above. (remove comma)

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


Thanks,
Wenliang Yan