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

[Guenter Roeck]

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.

>  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