Re: [PATCH v5 1/3] hwmon: ina2xx: make shunt resistance configurable at run-time

[Guenter Roeck <linux@roeck-us.net>]

On 12/10/2014 02:38 AM, Bartosz Golaszewski wrote:
> The shunt resistance can only be set via platform_data or device tree. This
> isn't suitable for devices in which the shunt resistance can change/isn't
> known at boot-time.
>
> Add a sysfs attribute that allows to read and set the shunt resistance.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>   drivers/hwmon/ina2xx.c | 74 ++++++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 65 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index e01feba..6e73add 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -51,7 +51,6 @@
>   #define INA226_ALERT_LIMIT		0x07
>   #define INA226_DIE_ID			0xFF
>
> -

While nice, this is an unrelated change.

>   /* register count */
>   #define INA219_REGISTERS		6
>   #define INA226_REGISTERS		8
> @@ -65,6 +64,9 @@
>   /* worst case is 68.10 ms (~14.6Hz, ina219) */
>   #define INA2XX_CONVERSION_RATE		15
>
> +/* default shunt resistance */
> +#define INA2XX_RSHUNT_DEFAULT		10000
> +
>   enum ina2xx_ids { ina219, ina226 };
>
>   struct ina2xx_config {
> @@ -87,6 +89,8 @@ struct ina2xx_data {
>
>   	int kind;
>   	u16 regs[INA2XX_MAX_REGISTERS];
> +
> +	long rshunt;
>   };
>
>   static const struct ina2xx_config ina2xx_config[] = {
> @@ -110,6 +114,11 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	},
>   };
>
> +static u16 ina2xx_calibration_val(const struct ina2xx_data *data)
> +{
> +	return data->config->calibration_factor / data->rshunt;
> +}
> +
>   static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -164,6 +173,13 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
>   		/* signed register, LSB=1mA (selected), in mA */
>   		val = (s16)data->regs[reg];
>   		break;
> +	case INA2XX_CALIBRATION:
> +		if (data->regs[reg] == 0)
> +			val = 0;
> +		else
> +			val = data->config->calibration_factor
> +						/ data->regs[reg];
> +		break;

This doesn't really make sense. What you want to show is rshunt, not the above.
I think it would be better to write a separate show function to display it.

>   	default:
>   		/* programmer goofed */
>   		WARN_ON_ONCE(1);
> @@ -187,6 +203,38 @@ static ssize_t ina2xx_show_value(struct device *dev,
>   			ina2xx_get_value(data, attr->index));
>   }
>
> +static ssize_t ina2xx_set_shunt(struct device *dev,
> +				struct device_attribute *da,
> +				const char *buf, size_t count)
> +{
> +	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	unsigned long val;
> +	int status;
> +
> +	if (IS_ERR(data))
> +		return PTR_ERR(data);
> +
> +	status = kstrtoul(buf, 10, &val);
> +	if (status < 0)
> +		return status;
> +
> +	if (val == 0 ||
> +	    /* Values greater than the calibration factor make no sense. */
> +	    val > data->config->calibration_factor ||
> +	    val > LONG_MAX)

data->config->calibration_factor is <= LONG_MAX, so the second check is unnecessary.
Actually, given that calibration_factor is chip dependent and not necessarily known
by the user, it would make more sense to only bail out on == 0 and then use clamp_val
to limit the range to (1, data->config->calibration_factor).

Guenter