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

[Wenliang Yan]

At 2025-01-14 01:45:48, "Guenter Roeck" <linux@xxxxxxxxxxxx> wrote:
>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@xxxxxxx>
>> ---
>...
>> @@ -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.
>

Yes, I made a mistake with the nested hierarchy. I will change
'break' to 'return 0'

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

Agree.

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

Yes, the logic here is reversed. Should exchange NULL and ina238_groups.

>Guenter

Thanks,
Wenliang Yan