Re: [PATCH V2 2/3] hwmon: (ina2xx) Make it easier to add more devices

[Guenter Roeck]

On 2/19/26 05:01, Ian Ray wrote:
> * Make sysfs entries documentation easier to maintain.
> * Use multi-line enum.
> * Correct "has_power_average" comment.
>
> Create a new "has_update_interval" member for chips which support
> averaging.
>
> Signed-off-by: Ian Ray <ian.ray@xxxxxxxxxxxxxxxx>
> ---
>   Documentation/hwmon/ina2xx.rst | 12 ++++++++++--
>   drivers/hwmon/ina2xx.c         | 21 +++++++++++++++++----
>   2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/hwmon/ina2xx.rst b/Documentation/hwmon/ina2xx.rst
> index a3860aae444c..a4ddf4bd2b08 100644
> --- a/Documentation/hwmon/ina2xx.rst
> +++ b/Documentation/hwmon/ina2xx.rst
> @@ -124,8 +124,16 @@ power1_input		Power(uW) measurement channel
>   shunt_resistor		Shunt resistance(uOhm) channel (not for ina260)
>   ======================= ===============================================
>   
> -Additional sysfs entries for ina226, ina230, ina231, ina260, and sy24655
> -------------------------------------------------------------------------
> +Additional sysfs entries
> +------------------------
> +
> +Additional entries are available for the following chips:
> +
> +  * ina226
> +  * ina230
> +  * ina231
> +  * ina260
> +  * sy24655
>   
>   ======================= ====================================================
>   curr1_lcrit		Critical low current
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 69ac0468dee4..4bf609e25f8a 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -46,9 +46,11 @@
>   #define INA2XX_CURRENT			0x04 /* readonly */
>   #define INA2XX_CALIBRATION		0x05
>   
> -/* INA226 register definitions */
> +/* INA2xx register definitions */

There was a reason for this. INA219 does not support those registers
or, more generically, they are only supported on chips supporting
alert limits.

>   #define INA226_MASK_ENABLE		0x06
>   #define INA226_ALERT_LIMIT		0x07
> +
> +/* INA226 register definitions */
>   #define INA226_DIE_ID			0xFF

That isn't even used, and the comment is wrong (at least INA230 and INA260
also support it). Might as well drop it.

Either case, is that bike shedding really necessary ? The only really valuable
change in this patch is the introduction of has_update_interval. Please keep that
and drop the rest.

Thanks,
Guenter

>   
>   /* SY24655 register definitions */
> @@ -135,13 +137,19 @@ static const struct regmap_config ina2xx_regmap_config = {
>   	.writeable_reg = ina2xx_writeable_reg,
>   };
>   
> -enum ina2xx_ids { ina219, ina226, ina260, sy24655 };
> +enum ina2xx_ids {
> +	ina219,
> +	ina226,
> +	ina260,
> +	sy24655
> +};
>   
>   struct ina2xx_config {
>   	u16 config_default;
>   	bool has_alerts;	/* chip supports alerts and limits */
>   	bool has_ishunt;	/* chip has internal shunt resistor */
> -	bool has_power_average;	/* chip has internal shunt resistor */
> +	bool has_power_average;	/* chip supports average power */
> +	bool has_update_interval;
>   	int calibration_value;
>   	int shunt_div;
>   	int bus_voltage_shift;
> @@ -171,6 +179,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.has_alerts = false,
>   		.has_ishunt = false,
>   		.has_power_average = false,
> +		.has_update_interval = false,
>   	},
>   	[ina226] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
> @@ -182,6 +191,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.has_alerts = true,
>   		.has_ishunt = false,
>   		.has_power_average = false,
> +		.has_update_interval = true,
>   	},
>   	[ina260] = {
>   		.config_default = INA260_CONFIG_DEFAULT,
> @@ -192,6 +202,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.has_alerts = true,
>   		.has_ishunt = true,
>   		.has_power_average = false,
> +		.has_update_interval = true,
>   	},
>   	[sy24655] = {
>   		.config_default = SY24655_CONFIG_DEFAULT,
> @@ -203,6 +214,7 @@ static const struct ina2xx_config ina2xx_config[] = {
>   		.has_alerts = true,
>   		.has_ishunt = false,
>   		.has_power_average = true,
> +		.has_update_interval = false,
>   	},
>   };
>   
> @@ -706,6 +718,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   	const struct ina2xx_data *data = _data;
>   	bool has_alerts = data->config->has_alerts;
>   	bool has_power_average = data->config->has_power_average;
> +	bool has_update_interval = data->config->has_update_interval;
>   	enum ina2xx_ids chip = data->chip;
>   
>   	switch (type) {
> @@ -768,7 +781,7 @@ static umode_t ina2xx_is_visible(const void *_data, enum hwmon_sensor_types type
>   	case hwmon_chip:
>   		switch (attr) {
>   		case hwmon_chip_update_interval:
> -			if (chip == ina226 || chip == ina260)
> +			if (has_update_interval)
>   				return 0644;
>   			break;
>   		default: