[RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

With the current implementation, the driver will prevent a readout at a
pace faster than the default conversion time (2ms) times the averaging
setting, min AVG being 1:1.

Any sysfs "show" read access from the client app faster than 500 Hz will be
'cached' by the driver, but actually since do_update reads all 8 registers,
the best achievable measurement rate is roughly 8*800 us (for the time
spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.

This change set uses a register mask to allow for the readout of a single
i2c register at a time. Furthermore, performing subsequent reads on the
same register will make use of the ability of the i2c chip to retain the
last reg offset, hence use a shorter i2c message (roughly 400us instead of
800us spent in i2c-core.c).

The best readout rate for a single measurement is now around 2kHz. And for
four measurements around (1/(4*800us) = 312 Hz. Since for any readout rate
faster than 160 Hz the interval is set by the i2c transactions completion,
the 'last-update' anti-flooding code will not have a limiting effect in
practice. Hence I also remove the elapsed time checking in the hwmon driver
for ina2xx.

To summarize, the patch provides a max bandwidth improvement with hwmon
client apps from ~160 Hz to ~320 Hz, and better in single-channel
polling mode.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 90 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 71 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..ce3a2ee 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -48,6 +48,8 @@
 #define INA2XX_CURRENT			0x04 /* readonly */
 #define INA2XX_CALIBRATION		0x05
 
+#define BITPOS_TO_MASK(x) (1L << x)
+
 /* INA226 register definitions */
 #define INA226_MASK_ENABLE		0x06
 #define INA226_ALERT_LIMIT		0x07
@@ -105,9 +107,14 @@ struct ina2xx_data {
 
 	struct mutex update_lock;
 	bool valid;
-	unsigned long last_updated;
 	int update_interval; /* in jiffies */
 
+	/* Last read register (slave address already set
+	 * reading out from this same register repeatedly will
+	 * be significantly faster!
+	 */
+	int last_reg;
+
 	int kind;
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 	u16 regs[INA2XX_MAX_REGISTERS];
@@ -203,21 +210,63 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+/*
+ * Most I2c chips will allow reading from the current register pointer
+ * w/o setting the register offset again.
+ */
+static inline s32 __i2c_read_same_word(const struct i2c_client *client)
+{
+	unsigned char msgbuf[2];
+
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = client->flags | I2C_M_RD,
+		.len = 2,
+		.buf = msgbuf,
+		};
+
+	int status = i2c_transfer(client->adapter, &msg, 1);
+
+	return (status < 0) ? status : (msgbuf[1]|(msgbuf[0]<<8));
+}
+
+static int ina2xx_do_update(struct device *dev, int reg_mask)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int i, rv, retry;
+	int i = 0, rv, retry;
 
 	dev_dbg(&client->dev, "Starting ina2xx update\n");
 
 	for (retry = 5; retry; retry--) {
-		/* Read all registers */
-		for (i = 0; i < data->config->registers; i++) {
-			rv = i2c_smbus_read_word_swapped(client, i);
+
+		/* Try to issue a shorter i2c message */
+		if (reg_mask & (1 << data->last_reg)) {
+			rv = __i2c_read_same_word(client);
 			if (rv < 0)
 				return rv;
-			data->regs[i] = rv;
+
+			reg_mask &= ~(1 << data->last_reg);
+			data->regs[data->last_reg] = rv;
+
+			dev_dbg(&client->dev, "%d, rv = %x, (last_reg)\n",
+						data->last_reg,
+						data->regs[data->last_reg]);
+		}
+
+		/* Check for remaining registers in mask. */
+		while (reg_mask && i < data->config->registers) {
+			if (reg_mask & (1L << i)) {
+				rv = i2c_smbus_read_word_swapped(client, i);
+				if (rv < 0)
+					return rv;
+				data->regs[i] = rv;
+				data->last_reg = i;
+
+				dev_dbg(&client->dev, "%d, rv = %x\n", i,
+							data->regs[i]);
+			}
+			i++;
 		}
 
 		/*
@@ -240,8 +289,6 @@ static int ina2xx_do_update(struct device *dev)
 			msleep(INA2XX_MAX_DELAY);
 			continue;
 		}
-
-		data->last_updated = jiffies;
 		data->valid = 1;
 
 		return 0;
@@ -256,22 +303,24 @@ static int ina2xx_do_update(struct device *dev)
 	return -ENODEV;
 }
 
-static struct ina2xx_data *ina2xx_update_device(struct device *dev)
+static struct ina2xx_data *ina2xx_update_device(struct device *dev,
+						int reg_mask)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	struct ina2xx_data *ret = data;
-	unsigned long after;
 	int rv;
 
 	mutex_lock(&data->update_lock);
 
-	after = data->last_updated + data->update_interval;
-	if (time_after(jiffies, after) || !data->valid) {
-		rv = ina2xx_do_update(dev);
-		if (rv < 0)
-			ret = ERR_PTR(rv);
+	if (!data->valid) {
+		reg_mask = 0xff; /* do all regs */
+		data->last_reg = 0xff;
 	}
 
+	rv = ina2xx_do_update(dev, reg_mask);
+	if (rv < 0)
+		ret = ERR_PTR(rv);
+
 	mutex_unlock(&data->update_lock);
 	return ret;
 }
@@ -316,7 +365,8 @@ static ssize_t ina2xx_show_value(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+						BITPOS_TO_MASK(attr->index));
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -329,7 +379,8 @@ 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);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+					BITPOS_TO_MASK(INA2XX_CONFIG));
 	unsigned long val;
 	int status;
 
@@ -390,7 +441,8 @@ static ssize_t ina226_set_interval(struct device *dev,
 static ssize_t ina226_show_interval(struct device *dev,
 				    struct device_attribute *da, char *buf)
 {
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+						BITPOS_TO_MASK(INA2XX_CONFIG));
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
-- 
1.9.1

Re: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Guenter Roeck]

On 10/19/2015 09:21 AM, Marc Titinger wrote:
> With the current implementation, the driver will prevent a readout at a
> pace faster than the default conversion time (2ms) times the averaging
> setting, min AVG being 1:1.
>
> Any sysfs "show" read access from the client app faster than 500 Hz will be
> 'cached' by the driver, but actually since do_update reads all 8 registers,
> the best achievable measurement rate is roughly 8*800 us (for the time
> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>
> This change set uses a register mask to allow for the readout of a single
> i2c register at a time. Furthermore, performing subsequent reads on the
> same register will make use of the ability of the i2c chip to retain the
> last reg offset, hence use a shorter i2c message (roughly 400us instead of
> 800us spent in i2c-core.c).
>
> The best readout rate for a single measurement is now around 2kHz. And for
> four measurements around (1/(4*800us) = 312 Hz. Since for any readout rate
> faster than 160 Hz the interval is set by the i2c transactions completion,
> the 'last-update' anti-flooding code will not have a limiting effect in
> practice. Hence I also remove the elapsed time checking in the hwmon driver
> for ina2xx.
>
> To summarize, the patch provides a max bandwidth improvement with hwmon
> client apps from ~160 Hz to ~320 Hz, and better in single-channel
> polling mode.
>

I really dislike that complexity. Maybe we should drop caching entirely
in this driver ? Maybe even convert it to use regmap ?

Guenter

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>   drivers/hwmon/ina2xx.c | 90 +++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 71 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 4d28150..ce3a2ee 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -48,6 +48,8 @@
>   #define INA2XX_CURRENT			0x04 /* readonly */
>   #define INA2XX_CALIBRATION		0x05
>
> +#define BITPOS_TO_MASK(x) (1L << x)
> +
>   /* INA226 register definitions */
>   #define INA226_MASK_ENABLE		0x06
>   #define INA226_ALERT_LIMIT		0x07
> @@ -105,9 +107,14 @@ struct ina2xx_data {
>
>   	struct mutex update_lock;
>   	bool valid;
> -	unsigned long last_updated;
>   	int update_interval; /* in jiffies */
>
> +	/* Last read register (slave address already set
> +	 * reading out from this same register repeatedly will
> +	 * be significantly faster!
> +	 */
> +	int last_reg;
> +
>   	int kind;
>   	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>   	u16 regs[INA2XX_MAX_REGISTERS];
> @@ -203,21 +210,63 @@ static int ina2xx_init(struct ina2xx_data *data)
>   	return ina2xx_calibrate(data);
>   }
>
> -static int ina2xx_do_update(struct device *dev)
> +/*
> + * Most I2c chips will allow reading from the current register pointer
> + * w/o setting the register offset again.
> + */
> +static inline s32 __i2c_read_same_word(const struct i2c_client *client)
> +{
> +	unsigned char msgbuf[2];
> +
> +	struct i2c_msg msg = {
> +		.addr = client->addr,
> +		.flags = client->flags | I2C_M_RD,
> +		.len = 2,
> +		.buf = msgbuf,
> +		};
> +
> +	int status = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return (status < 0) ? status : (msgbuf[1]|(msgbuf[0]<<8));
> +}
> +
> +static int ina2xx_do_update(struct device *dev, int reg_mask)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
>   	struct i2c_client *client = data->client;
> -	int i, rv, retry;
> +	int i = 0, rv, retry;
>
>   	dev_dbg(&client->dev, "Starting ina2xx update\n");
>
>   	for (retry = 5; retry; retry--) {
> -		/* Read all registers */
> -		for (i = 0; i < data->config->registers; i++) {
> -			rv = i2c_smbus_read_word_swapped(client, i);
> +
> +		/* Try to issue a shorter i2c message */
> +		if (reg_mask & (1 << data->last_reg)) {
> +			rv = __i2c_read_same_word(client);
>   			if (rv < 0)
>   				return rv;
> -			data->regs[i] = rv;
> +
> +			reg_mask &= ~(1 << data->last_reg);
> +			data->regs[data->last_reg] = rv;
> +
> +			dev_dbg(&client->dev, "%d, rv = %x, (last_reg)\n",
> +						data->last_reg,
> +						data->regs[data->last_reg]);
> +		}
> +
> +		/* Check for remaining registers in mask. */
> +		while (reg_mask && i < data->config->registers) {
> +			if (reg_mask & (1L << i)) {
> +				rv = i2c_smbus_read_word_swapped(client, i);
> +				if (rv < 0)
> +					return rv;
> +				data->regs[i] = rv;
> +				data->last_reg = i;
> +
> +				dev_dbg(&client->dev, "%d, rv = %x\n", i,
> +							data->regs[i]);
> +			}
> +			i++;
>   		}
>
>   		/*
> @@ -240,8 +289,6 @@ static int ina2xx_do_update(struct device *dev)
>   			msleep(INA2XX_MAX_DELAY);
>   			continue;
>   		}
> -
> -		data->last_updated = jiffies;
>   		data->valid = 1;
>
>   		return 0;
> @@ -256,22 +303,24 @@ static int ina2xx_do_update(struct device *dev)
>   	return -ENODEV;
>   }
>
> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
> +static struct ina2xx_data *ina2xx_update_device(struct device *dev,
> +						int reg_mask)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
>   	struct ina2xx_data *ret = data;
> -	unsigned long after;
>   	int rv;
>
>   	mutex_lock(&data->update_lock);
>
> -	after = data->last_updated + data->update_interval;
> -	if (time_after(jiffies, after) || !data->valid) {
> -		rv = ina2xx_do_update(dev);
> -		if (rv < 0)
> -			ret = ERR_PTR(rv);
> +	if (!data->valid) {
> +		reg_mask = 0xff; /* do all regs */
> +		data->last_reg = 0xff;
>   	}
>
> +	rv = ina2xx_do_update(dev, reg_mask);
> +	if (rv < 0)
> +		ret = ERR_PTR(rv);
> +
>   	mutex_unlock(&data->update_lock);
>   	return ret;
>   }
> @@ -316,7 +365,8 @@ static ssize_t ina2xx_show_value(struct device *dev,
>   				 struct device_attribute *da, char *buf)
>   {
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	struct ina2xx_data *data = ina2xx_update_device(dev,
> +						BITPOS_TO_MASK(attr->index));
>
>   	if (IS_ERR(data))
>   		return PTR_ERR(data);
> @@ -329,7 +379,8 @@ 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);
> +	struct ina2xx_data *data = ina2xx_update_device(dev,
> +					BITPOS_TO_MASK(INA2XX_CONFIG));
>   	unsigned long val;
>   	int status;
>
> @@ -390,7 +441,8 @@ static ssize_t ina226_set_interval(struct device *dev,
>   static ssize_t ina226_show_interval(struct device *dev,
>   				    struct device_attribute *da, char *buf)
>   {
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	struct ina2xx_data *data = ina2xx_update_device(dev,
> +						BITPOS_TO_MASK(INA2XX_CONFIG));
>
>   	if (IS_ERR(data))
>   		return PTR_ERR(data);
>

Re: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

On 20/10/2015 03:32, Guenter Roeck wrote:
> On 10/19/2015 09:21 AM, Marc Titinger wrote:
>> With the current implementation, the driver will prevent a readout at a
>> pace faster than the default conversion time (2ms) times the averaging
>> setting, min AVG being 1:1.
>>
>> Any sysfs "show" read access from the client app faster than 500 Hz
>> will be
>> 'cached' by the driver, but actually since do_update reads all 8
>> registers,
>> the best achievable measurement rate is roughly 8*800 us (for the time
>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>
>> This change set uses a register mask to allow for the readout of a single
>> i2c register at a time. Furthermore, performing subsequent reads on the
>> same register will make use of the ability of the i2c chip to retain the
>> last reg offset, hence use a shorter i2c message (roughly 400us
>> instead of
>> 800us spent in i2c-core.c).
>>
>> The best readout rate for a single measurement is now around 2kHz. And
>> for
>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>> rate
>> faster than 160 Hz the interval is set by the i2c transactions
>> completion,
>> the 'last-update' anti-flooding code will not have a limiting effect in
>> practice. Hence I also remove the elapsed time checking in the hwmon
>> driver
>> for ina2xx.
>>
>> To summarize, the patch provides a max bandwidth improvement with hwmon
>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>> polling mode.
>>
>
Hi Guenter,

> I really dislike that complexity. Maybe we should drop caching entirely
> in this driver ? Maybe even convert it to use regmap ?

I dropped caching completely with this patch. The only (mild!) 
complexity added is to keep track of the current register pointer
set in the chip, to issue a shorter i2c message.

Thanks for the hint re. regmap, is there a driver file you recommend I 
look into (because the i2c caps were close to in2xx) ?

Many thanks,
Marc.


>
> Guenter
>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>   drivers/hwmon/ina2xx.c | 90
>> +++++++++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 71 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 4d28150..ce3a2ee 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>> @@ -48,6 +48,8 @@
>>   #define INA2XX_CURRENT            0x04 /* readonly */
>>   #define INA2XX_CALIBRATION        0x05
>>
>> +#define BITPOS_TO_MASK(x) (1L << x)
>> +
>>   /* INA226 register definitions */
>>   #define INA226_MASK_ENABLE        0x06
>>   #define INA226_ALERT_LIMIT        0x07
>> @@ -105,9 +107,14 @@ struct ina2xx_data {
>>
>>       struct mutex update_lock;
>>       bool valid;
>> -    unsigned long last_updated;
>>       int update_interval; /* in jiffies */
>>
>> +    /* Last read register (slave address already set
>> +     * reading out from this same register repeatedly will
>> +     * be significantly faster!
>> +     */
>> +    int last_reg;
>> +
>>       int kind;
>>       const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>>       u16 regs[INA2XX_MAX_REGISTERS];
>> @@ -203,21 +210,63 @@ static int ina2xx_init(struct ina2xx_data *data)
>>       return ina2xx_calibrate(data);
>>   }
>>
>> -static int ina2xx_do_update(struct device *dev)
>> +/*
>> + * Most I2c chips will allow reading from the current register pointer
>> + * w/o setting the register offset again.
>> + */
>> +static inline s32 __i2c_read_same_word(const struct i2c_client *client)
>> +{
>> +    unsigned char msgbuf[2];
>> +
>> +    struct i2c_msg msg = {
>> +        .addr = client->addr,
>> +        .flags = client->flags | I2C_M_RD,
>> +        .len = 2,
>> +        .buf = msgbuf,
>> +        };
>> +
>> +    int status = i2c_transfer(client->adapter, &msg, 1);
>> +
>> +    return (status < 0) ? status : (msgbuf[1]|(msgbuf[0]<<8));
>> +}
>> +
>> +static int ina2xx_do_update(struct device *dev, int reg_mask)
>>   {
>>       struct ina2xx_data *data = dev_get_drvdata(dev);
>>       struct i2c_client *client = data->client;
>> -    int i, rv, retry;
>> +    int i = 0, rv, retry;
>>
>>       dev_dbg(&client->dev, "Starting ina2xx update\n");
>>
>>       for (retry = 5; retry; retry--) {
>> -        /* Read all registers */
>> -        for (i = 0; i < data->config->registers; i++) {
>> -            rv = i2c_smbus_read_word_swapped(client, i);
>> +
>> +        /* Try to issue a shorter i2c message */
>> +        if (reg_mask & (1 << data->last_reg)) {
>> +            rv = __i2c_read_same_word(client);
>>               if (rv < 0)
>>                   return rv;
>> -            data->regs[i] = rv;
>> +
>> +            reg_mask &= ~(1 << data->last_reg);
>> +            data->regs[data->last_reg] = rv;
>> +
>> +            dev_dbg(&client->dev, "%d, rv = %x, (last_reg)\n",
>> +                        data->last_reg,
>> +                        data->regs[data->last_reg]);
>> +        }
>> +
>> +        /* Check for remaining registers in mask. */
>> +        while (reg_mask && i < data->config->registers) {
>> +            if (reg_mask & (1L << i)) {
>> +                rv = i2c_smbus_read_word_swapped(client, i);
>> +                if (rv < 0)
>> +                    return rv;
>> +                data->regs[i] = rv;
>> +                data->last_reg = i;
>> +
>> +                dev_dbg(&client->dev, "%d, rv = %x\n", i,
>> +                            data->regs[i]);
>> +            }
>> +            i++;
>>           }
>>
>>           /*
>> @@ -240,8 +289,6 @@ static int ina2xx_do_update(struct device *dev)
>>               msleep(INA2XX_MAX_DELAY);
>>               continue;
>>           }
>> -
>> -        data->last_updated = jiffies;
>>           data->valid = 1;
>>
>>           return 0;
>> @@ -256,22 +303,24 @@ static int ina2xx_do_update(struct device *dev)
>>       return -ENODEV;
>>   }
>>
>> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>> +static struct ina2xx_data *ina2xx_update_device(struct device *dev,
>> +                        int reg_mask)
>>   {
>>       struct ina2xx_data *data = dev_get_drvdata(dev);
>>       struct ina2xx_data *ret = data;
>> -    unsigned long after;
>>       int rv;
>>
>>       mutex_lock(&data->update_lock);
>>
>> -    after = data->last_updated + data->update_interval;
>> -    if (time_after(jiffies, after) || !data->valid) {
>> -        rv = ina2xx_do_update(dev);
>> -        if (rv < 0)
>> -            ret = ERR_PTR(rv);
>> +    if (!data->valid) {
>> +        reg_mask = 0xff; /* do all regs */
>> +        data->last_reg = 0xff;
>>       }
>>
>> +    rv = ina2xx_do_update(dev, reg_mask);
>> +    if (rv < 0)
>> +        ret = ERR_PTR(rv);
>> +
>>       mutex_unlock(&data->update_lock);
>>       return ret;
>>   }
>> @@ -316,7 +365,8 @@ static ssize_t ina2xx_show_value(struct device *dev,
>>                    struct device_attribute *da, char *buf)
>>   {
>>       struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> -    struct ina2xx_data *data = ina2xx_update_device(dev);
>> +    struct ina2xx_data *data = ina2xx_update_device(dev,
>> +                        BITPOS_TO_MASK(attr->index));
>>
>>       if (IS_ERR(data))
>>           return PTR_ERR(data);
>> @@ -329,7 +379,8 @@ 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);
>> +    struct ina2xx_data *data = ina2xx_update_device(dev,
>> +                    BITPOS_TO_MASK(INA2XX_CONFIG));
>>       unsigned long val;
>>       int status;
>>
>> @@ -390,7 +441,8 @@ static ssize_t ina226_set_interval(struct device
>> *dev,
>>   static ssize_t ina226_show_interval(struct device *dev,
>>                       struct device_attribute *da, char *buf)
>>   {
>> -    struct ina2xx_data *data = ina2xx_update_device(dev);
>> +    struct ina2xx_data *data = ina2xx_update_device(dev,
>> +                        BITPOS_TO_MASK(INA2XX_CONFIG));
>>
>>       if (IS_ERR(data))
>>           return PTR_ERR(data);
>>
>

[PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

With the current implementation, the driver will prevent a readout at a
pace faster than the default conversion time (2ms) times the averaging
setting, min AVG being 1:1.

Any sysfs "show" read access from the client app faster than 500 Hz will be
'cached' by the driver, but actually since do_update reads all 8 registers,
the best achievable measurement rate is roughly 8*800 us (for the time
spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.

This change set uses a register mask to allow for the readout of a single
i2c register at a time. Furthermore, performing subsequent reads on the
same register will make use of the ability of the i2c chip to retain the
last reg offset, hence use a shorter i2c message (roughly 400us instead of
800us spent in i2c-core.c).

The best readout rate for a single measurement is now around 2kHz. And for
four measurements around (1/(4*800us) = 312 Hz. Since for any readout rate
faster than 160 Hz the interval is set by the i2c transactions completion,
the 'last-update' anti-flooding code will not have a limiting effect in
practice. Hence I also remove the elapsed time checking in the hwmon driver
for ina2xx.

To summarize, the patch provides a max bandwidth improvement with hwmon
client apps from ~160 Hz to ~320 Hz, and better in single-channel
polling mode.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

	v2: remove local macro for BIT_MASK, use the bitops.h one.

 drivers/hwmon/ina2xx.c | 88 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 19 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..2f82a9b 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -105,9 +105,14 @@ struct ina2xx_data {
 
 	struct mutex update_lock;
 	bool valid;
-	unsigned long last_updated;
 	int update_interval; /* in jiffies */
 
+	/* Last read register (slave address already set
+	 * reading out from this same register repeatedly will
+	 * be significantly faster!
+	 */
+	int last_reg;
+
 	int kind;
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
 	u16 regs[INA2XX_MAX_REGISTERS];
@@ -203,21 +208,63 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+/*
+ * Most I2c chips will allow reading from the current register pointer
+ * w/o setting the register offset again.
+ */
+static inline s32 __i2c_read_same_word(const struct i2c_client *client)
+{
+	unsigned char msgbuf[2];
+
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = client->flags | I2C_M_RD,
+		.len = 2,
+		.buf = msgbuf,
+		};
+
+	int status = i2c_transfer(client->adapter, &msg, 1);
+
+	return (status < 0) ? status : (msgbuf[1]|(msgbuf[0]<<8));
+}
+
+static int ina2xx_do_update(struct device *dev, int reg_mask)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	struct i2c_client *client = data->client;
-	int i, rv, retry;
+	int i = 0, rv, retry;
 
 	dev_dbg(&client->dev, "Starting ina2xx update\n");
 
 	for (retry = 5; retry; retry--) {
-		/* Read all registers */
-		for (i = 0; i < data->config->registers; i++) {
-			rv = i2c_smbus_read_word_swapped(client, i);
+
+		/* Try to issue a shorter i2c message */
+		if (reg_mask & (1 << data->last_reg)) {
+			rv = __i2c_read_same_word(client);
 			if (rv < 0)
 				return rv;
-			data->regs[i] = rv;
+
+			reg_mask &= ~(1 << data->last_reg);
+			data->regs[data->last_reg] = rv;
+
+			dev_dbg(&client->dev, "%d, rv = %x, (last_reg)\n",
+						data->last_reg,
+						data->regs[data->last_reg]);
+		}
+
+		/* Check for remaining registers in mask. */
+		while (reg_mask && i < data->config->registers) {
+			if (reg_mask & (1L << i)) {
+				rv = i2c_smbus_read_word_swapped(client, i);
+				if (rv < 0)
+					return rv;
+				data->regs[i] = rv;
+				data->last_reg = i;
+
+				dev_dbg(&client->dev, "%d, rv = %x\n", i,
+							data->regs[i]);
+			}
+			i++;
 		}
 
 		/*
@@ -240,8 +287,6 @@ static int ina2xx_do_update(struct device *dev)
 			msleep(INA2XX_MAX_DELAY);
 			continue;
 		}
-
-		data->last_updated = jiffies;
 		data->valid = 1;
 
 		return 0;
@@ -256,22 +301,24 @@ static int ina2xx_do_update(struct device *dev)
 	return -ENODEV;
 }
 
-static struct ina2xx_data *ina2xx_update_device(struct device *dev)
+static struct ina2xx_data *ina2xx_update_device(struct device *dev,
+						int reg_mask)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
 	struct ina2xx_data *ret = data;
-	unsigned long after;
 	int rv;
 
 	mutex_lock(&data->update_lock);
 
-	after = data->last_updated + data->update_interval;
-	if (time_after(jiffies, after) || !data->valid) {
-		rv = ina2xx_do_update(dev);
-		if (rv < 0)
-			ret = ERR_PTR(rv);
+	if (!data->valid) {
+		reg_mask = 0xff; /* do all regs */
+		data->last_reg = 0xff;
 	}
 
+	rv = ina2xx_do_update(dev, reg_mask);
+	if (rv < 0)
+		ret = ERR_PTR(rv);
+
 	mutex_unlock(&data->update_lock);
 	return ret;
 }
@@ -316,7 +363,8 @@ static ssize_t ina2xx_show_value(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+						BIT_MASK(attr->index));
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
@@ -329,7 +377,8 @@ 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);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+					BIT_MASK(INA2XX_CONFIG));
 	unsigned long val;
 	int status;
 
@@ -390,7 +439,8 @@ static ssize_t ina226_set_interval(struct device *dev,
 static ssize_t ina226_show_interval(struct device *dev,
 				    struct device_attribute *da, char *buf)
 {
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = ina2xx_update_device(dev,
+						BIT_MASK(INA2XX_CONFIG));
 
 	if (IS_ERR(data))
 		return PTR_ERR(data);
-- 
1.9.1

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Guenter Roeck]

On 10/20/2015 01:20 AM, Marc Titinger wrote:
> With the current implementation, the driver will prevent a readout at a
> pace faster than the default conversion time (2ms) times the averaging
> setting, min AVG being 1:1.
>
> Any sysfs "show" read access from the client app faster than 500 Hz will be
> 'cached' by the driver, but actually since do_update reads all 8 registers,
> the best achievable measurement rate is roughly 8*800 us (for the time
> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>
> This change set uses a register mask to allow for the readout of a single
> i2c register at a time. Furthermore, performing subsequent reads on the
> same register will make use of the ability of the i2c chip to retain the
> last reg offset, hence use a shorter i2c message (roughly 400us instead of
> 800us spent in i2c-core.c).
>
That doesn't work. There could be accesses from other sources (such as through
i2c-dev, or in multi-master systems) between two reads.

> The best readout rate for a single measurement is now around 2kHz. And for
> four measurements around (1/(4*800us) = 312 Hz. Since for any readout rate
> faster than 160 Hz the interval is set by the i2c transactions completion,
> the 'last-update' anti-flooding code will not have a limiting effect in
> practice. Hence I also remove the elapsed time checking in the hwmon driver
> for ina2xx.
>
> To summarize, the patch provides a max bandwidth improvement with hwmon
> client apps from ~160 Hz to ~320 Hz, and better in single-channel
> polling mode.
>
Overall your patch pretty much re-implements regmap. Since you drop caching,
it is also unnecessary to read all registers at a time, so you can just use
a function to read _one_ register and returns its value (with retries).
Or use regmap. Either case, do_update() and ina2xx_update_device() are no
longer needed.

If you want to convert the driver to regmap, just look for 'regmap' in
drivers/hwmon for examples.

Guenter

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

On 20/10/2015 14:54, Guenter Roeck wrote:
> On 10/20/2015 01:20 AM, Marc Titinger wrote:
>> With the current implementation, the driver will prevent a readout at a
>> pace faster than the default conversion time (2ms) times the averaging
>> setting, min AVG being 1:1.
>>
>> Any sysfs "show" read access from the client app faster than 500 Hz
>> will be
>> 'cached' by the driver, but actually since do_update reads all 8
>> registers,
>> the best achievable measurement rate is roughly 8*800 us (for the time
>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>
>> This change set uses a register mask to allow for the readout of a single
>> i2c register at a time. Furthermore, performing subsequent reads on the
>> same register will make use of the ability of the i2c chip to retain the
>> last reg offset, hence use a shorter i2c message (roughly 400us
>> instead of
>> 800us spent in i2c-core.c).
>>
> That doesn't work. There could be accesses from other sources (such as
> through
> i2c-dev, or in multi-master systems) between two reads.

Re-setting the register address with each read transaction will not 
prevent another master to change the configuration in your back, in this 
case. That sounds like a general issue of concurrent clients for one 
device, this is beyond just reading one register IMO.

>
>> The best readout rate for a single measurement is now around 2kHz. And
>> for
>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>> rate
>> faster than 160 Hz the interval is set by the i2c transactions
>> completion,
>> the 'last-update' anti-flooding code will not have a limiting effect in
>> practice. Hence I also remove the elapsed time checking in the hwmon
>> driver
>> for ina2xx.
>>
>> To summarize, the patch provides a max bandwidth improvement with hwmon
>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>> polling mode.
>>
> Overall your patch pretty much re-implements regmap. Since you drop
> caching,
> it is also unnecessary to read all registers at a time, so you can just use
> a function to read _one_ register and returns its value (with retries).
> Or use regmap. Either case, do_update() and ina2xx_update_device() are no
> longer needed.
Agreed.

>
> If you want to convert the driver to regmap, just look for 'regmap' in
> drivers/hwmon for examples.

Fair enough, but based on your comments, I may look into an iio driver 
instead for this device, given our application, rather than 'twisting' 
the hwmon interface.

Cheers,
Marc.


>
> Guenter
>

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Guenter Roeck]

On 10/20/2015 06:17 AM, Marc Titinger wrote:
> On 20/10/2015 14:54, Guenter Roeck wrote:
>> On 10/20/2015 01:20 AM, Marc Titinger wrote:
>>> With the current implementation, the driver will prevent a readout at a
>>> pace faster than the default conversion time (2ms) times the averaging
>>> setting, min AVG being 1:1.
>>>
>>> Any sysfs "show" read access from the client app faster than 500 Hz
>>> will be
>>> 'cached' by the driver, but actually since do_update reads all 8
>>> registers,
>>> the best achievable measurement rate is roughly 8*800 us (for the time
>>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>>
>>> This change set uses a register mask to allow for the readout of a single
>>> i2c register at a time. Furthermore, performing subsequent reads on the
>>> same register will make use of the ability of the i2c chip to retain the
>>> last reg offset, hence use a shorter i2c message (roughly 400us
>>> instead of
>>> 800us spent in i2c-core.c).
>>>
>> That doesn't work. There could be accesses from other sources (such as
>> through
>> i2c-dev, or in multi-master systems) between two reads.
>
> Re-setting the register address with each read transaction will not prevent another master to change the configuration in your back, in this case. That sounds like a general issue of concurrent clients for one device, this is beyond just reading one register IMO.
>
That is an invasive change, though, not just a simple read. Sure, it is
a risk as well. But it is a different level of risk than someone using
i2cget or i2cdump while the driver is running.

>>
>>> The best readout rate for a single measurement is now around 2kHz. And
>>> for
>>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>>> rate
>>> faster than 160 Hz the interval is set by the i2c transactions
>>> completion,
>>> the 'last-update' anti-flooding code will not have a limiting effect in
>>> practice. Hence I also remove the elapsed time checking in the hwmon
>>> driver
>>> for ina2xx.
>>>
>>> To summarize, the patch provides a max bandwidth improvement with hwmon
>>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>>> polling mode.
>>>
>> Overall your patch pretty much re-implements regmap. Since you drop
>> caching,
>> it is also unnecessary to read all registers at a time, so you can just use
>> a function to read _one_ register and returns its value (with retries).
>> Or use regmap. Either case, do_update() and ina2xx_update_device() are no
>> longer needed.
> Agreed.
>
>>
>> If you want to convert the driver to regmap, just look for 'regmap' in
>> drivers/hwmon for examples.
>
> Fair enough, but based on your comments, I may look into an iio driver instead for this device, given our application, rather than 'twisting' the hwmon interface.
>

Sorry, you lost me there. How are you twisting the hwmon interface ?
Because I am concerned about multiple accesses from multiple sources ?
How is iio going to solve that problem ? By ignoring it ?

Not that I mind if you want to convert the driver to iio. One less driver
to take care of. Just asking.

Thanks,
Guenter

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

On 20/10/2015 15:30, Guenter Roeck wrote:
> On 10/20/2015 06:17 AM, Marc Titinger wrote:
>> On 20/10/2015 14:54, Guenter Roeck wrote:
>>> On 10/20/2015 01:20 AM, Marc Titinger wrote:
>>>> With the current implementation, the driver will prevent a readout at a
>>>> pace faster than the default conversion time (2ms) times the averaging
>>>> setting, min AVG being 1:1.
>>>>
>>>> Any sysfs "show" read access from the client app faster than 500 Hz
>>>> will be
>>>> 'cached' by the driver, but actually since do_update reads all 8
>>>> registers,
>>>> the best achievable measurement rate is roughly 8*800 us (for the time
>>>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>>>
>>>> This change set uses a register mask to allow for the readout of a
>>>> single
>>>> i2c register at a time. Furthermore, performing subsequent reads on the
>>>> same register will make use of the ability of the i2c chip to retain
>>>> the
>>>> last reg offset, hence use a shorter i2c message (roughly 400us
>>>> instead of
>>>> 800us spent in i2c-core.c).
>>>>
>>> That doesn't work. There could be accesses from other sources (such as
>>> through
>>> i2c-dev, or in multi-master systems) between two reads.
>>
>> Re-setting the register address with each read transaction will not
>> prevent another master to change the configuration in your back, in
>> this case. That sounds like a general issue of concurrent clients for
>> one device, this is beyond just reading one register IMO.
>>
> That is an invasive change, though, not just a simple read. Sure, it is
> a risk as well. But it is a different level of risk than someone using
> i2cget or i2cdump while the driver is running.
>
Yes, I get your point.

>>>
>>>> The best readout rate for a single measurement is now around 2kHz. And
>>>> for
>>>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>>>> rate
>>>> faster than 160 Hz the interval is set by the i2c transactions
>>>> completion,
>>>> the 'last-update' anti-flooding code will not have a limiting effect in
>>>> practice. Hence I also remove the elapsed time checking in the hwmon
>>>> driver
>>>> for ina2xx.
>>>>
>>>> To summarize, the patch provides a max bandwidth improvement with hwmon
>>>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>>>> polling mode.
>>>>
>>> Overall your patch pretty much re-implements regmap. Since you drop
>>> caching,
>>> it is also unnecessary to read all registers at a time, so you can
>>> just use
>>> a function to read _one_ register and returns its value (with retries).
>>> Or use regmap. Either case, do_update() and ina2xx_update_device()
>>> are no
>>> longer needed.
>> Agreed.
>>
>>>
>>> If you want to convert the driver to regmap, just look for 'regmap' in
>>> drivers/hwmon for examples.
>>
>> Fair enough, but based on your comments, I may look into an iio driver
>> instead for this device, given our application, rather than 'twisting'
>> the hwmon interface.
>>
>
> Sorry, you lost me there. How are you twisting the hwmon interface ?
> Because I am concerned about multiple accesses from multiple sources ?
> How is iio going to solve that problem ? By ignoring it ?

Sure someone can still use i2c diag tools as you said, you have a point 
here. But similarly, someone can use /dev/mem to remap stuff and 
peek/poke mm registers, and to my knowledge we do not generally design 
drivers or subsystems with retries and feature limitations to cope with 
potential use of diag and debug facilities.

M.

>
> Not that I mind if you want to convert the driver to iio. One less driver
> to take care of. Just asking.

>
> Thanks,
> Guenter
>

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Michael Turquette]

Hi Guenter,

Quoting Guenter Roeck (2015-10-20 06:30:03)
> On 10/20/2015 06:17 AM, Marc Titinger wrote:
> > On 20/10/2015 14:54, Guenter Roeck wrote:
> >> On 10/20/2015 01:20 AM, Marc Titinger wrote:
> >>> With the current implementation, the driver will prevent a readout at a
> >>> pace faster than the default conversion time (2ms) times the averaging
> >>> setting, min AVG being 1:1.
> >>>
> >>> Any sysfs "show" read access from the client app faster than 500 Hz
> >>> will be
> >>> 'cached' by the driver, but actually since do_update reads all 8
> >>> registers,
> >>> the best achievable measurement rate is roughly 8*800 us (for the time
> >>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
> >>>
> >>> This change set uses a register mask to allow for the readout of a single
> >>> i2c register at a time. Furthermore, performing subsequent reads on the
> >>> same register will make use of the ability of the i2c chip to retain the
> >>> last reg offset, hence use a shorter i2c message (roughly 400us
> >>> instead of
> >>> 800us spent in i2c-core.c).
> >>>
> >> That doesn't work. There could be accesses from other sources (such as
> >> through
> >> i2c-dev, or in multi-master systems) between two reads.
> >
> > Re-setting the register address with each read transaction will not prevent another master to change the configuration in your back, in this case. That sounds like a general issue of concurrent clients for one device, this is beyond just reading one register IMO.
> >
> That is an invasive change, though, not just a simple read. Sure, it is
> a risk as well. But it is a different level of risk than someone using
> i2cget or i2cdump while the driver is running.
> 
> >>
> >>> The best readout rate for a single measurement is now around 2kHz. And
> >>> for
> >>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
> >>> rate
> >>> faster than 160 Hz the interval is set by the i2c transactions
> >>> completion,
> >>> the 'last-update' anti-flooding code will not have a limiting effect in
> >>> practice. Hence I also remove the elapsed time checking in the hwmon
> >>> driver
> >>> for ina2xx.
> >>>
> >>> To summarize, the patch provides a max bandwidth improvement with hwmon
> >>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
> >>> polling mode.
> >>>
> >> Overall your patch pretty much re-implements regmap. Since you drop
> >> caching,
> >> it is also unnecessary to read all registers at a time, so you can just use
> >> a function to read _one_ register and returns its value (with retries).
> >> Or use regmap. Either case, do_update() and ina2xx_update_device() are no
> >> longer needed.
> > Agreed.
> >
> >>
> >> If you want to convert the driver to regmap, just look for 'regmap' in
> >> drivers/hwmon for examples.
> >
> > Fair enough, but based on your comments, I may look into an iio driver instead for this device, given our application, rather than 'twisting' the hwmon interface.
> >
> 
> Sorry, you lost me there. How are you twisting the hwmon interface ?
> Because I am concerned about multiple accesses from multiple sources ?
> How is iio going to solve that problem ? By ignoring it ?

There is no twisting going on here :-)

We're going to investigate IIO with or without the multi-client/regmap
issues raised in this thread. The purpose is to investigate if we can
get better performance/higher sample rate for our application (the ACME
power measurement device).

> 
> Not that I mind if you want to convert the driver to iio. One less driver
> to take care of. Just asking.

Yup, it's orthogonal to the issues raised here.

Regards,
Mike

> 
> Thanks,
> Guenter
> 

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Guenter Roeck]

On 10/20/2015 06:46 AM, Marc Titinger wrote:
> On 20/10/2015 15:30, Guenter Roeck wrote:
>> On 10/20/2015 06:17 AM, Marc Titinger wrote:
>>> On 20/10/2015 14:54, Guenter Roeck wrote:
>>>> On 10/20/2015 01:20 AM, Marc Titinger wrote:
>>>>> With the current implementation, the driver will prevent a readout at a
>>>>> pace faster than the default conversion time (2ms) times the averaging
>>>>> setting, min AVG being 1:1.
>>>>>
>>>>> Any sysfs "show" read access from the client app faster than 500 Hz
>>>>> will be
>>>>> 'cached' by the driver, but actually since do_update reads all 8
>>>>> registers,
>>>>> the best achievable measurement rate is roughly 8*800 us (for the time
>>>>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>>>>
>>>>> This change set uses a register mask to allow for the readout of a
>>>>> single
>>>>> i2c register at a time. Furthermore, performing subsequent reads on the
>>>>> same register will make use of the ability of the i2c chip to retain
>>>>> the
>>>>> last reg offset, hence use a shorter i2c message (roughly 400us
>>>>> instead of
>>>>> 800us spent in i2c-core.c).
>>>>>
>>>> That doesn't work. There could be accesses from other sources (such as
>>>> through
>>>> i2c-dev, or in multi-master systems) between two reads.
>>>
>>> Re-setting the register address with each read transaction will not
>>> prevent another master to change the configuration in your back, in
>>> this case. That sounds like a general issue of concurrent clients for
>>> one device, this is beyond just reading one register IMO.
>>>
>> That is an invasive change, though, not just a simple read. Sure, it is
>> a risk as well. But it is a different level of risk than someone using
>> i2cget or i2cdump while the driver is running.
>>
> Yes, I get your point.
>
>>>>
>>>>> The best readout rate for a single measurement is now around 2kHz. And
>>>>> for
>>>>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>>>>> rate
>>>>> faster than 160 Hz the interval is set by the i2c transactions
>>>>> completion,
>>>>> the 'last-update' anti-flooding code will not have a limiting effect in
>>>>> practice. Hence I also remove the elapsed time checking in the hwmon
>>>>> driver
>>>>> for ina2xx.
>>>>>
>>>>> To summarize, the patch provides a max bandwidth improvement with hwmon
>>>>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>>>>> polling mode.
>>>>>
>>>> Overall your patch pretty much re-implements regmap. Since you drop
>>>> caching,
>>>> it is also unnecessary to read all registers at a time, so you can
>>>> just use
>>>> a function to read _one_ register and returns its value (with retries).
>>>> Or use regmap. Either case, do_update() and ina2xx_update_device()
>>>> are no
>>>> longer needed.
>>> Agreed.
>>>
>>>>
>>>> If you want to convert the driver to regmap, just look for 'regmap' in
>>>> drivers/hwmon for examples.
>>>
>>> Fair enough, but based on your comments, I may look into an iio driver
>>> instead for this device, given our application, rather than 'twisting'
>>> the hwmon interface.
>>>
>>
>> Sorry, you lost me there. How are you twisting the hwmon interface ?
>> Because I am concerned about multiple accesses from multiple sources ?
>> How is iio going to solve that problem ? By ignoring it ?
>
> Sure someone can still use i2c diag tools as you said, you have a point here. But similarly, someone can use /dev/mem to remap stuff and peek/poke mm registers, and to my knowledge we do not generally design drivers or subsystems with retries and feature limitations to cope with potential use of diag and debug facilities.
>

Depends on the system, and on the use case. Many systems I deal with
are multi-master, and user space does end up accessing chips using i2c-dev.

Drivers are (supposed to be) designed with as much multi-master access
safety as possible. I had patches rejected because they broke multi-master
support by sending two commands depending on each other in a sequence.

Using /dev/mem and peek/poke as counter-examples isn't really appropriate
here, so I won't comment further on it.

Thanks,
Guenter

Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz

[Marc Titinger]

On 20/10/2015 19:00, Guenter Roeck wrote:
> On 10/20/2015 06:46 AM, Marc Titinger wrote:
>> On 20/10/2015 15:30, Guenter Roeck wrote:
>>> On 10/20/2015 06:17 AM, Marc Titinger wrote:
>>>> On 20/10/2015 14:54, Guenter Roeck wrote:
>>>>> On 10/20/2015 01:20 AM, Marc Titinger wrote:
>>>>>> With the current implementation, the driver will prevent a readout
>>>>>> at a
>>>>>> pace faster than the default conversion time (2ms) times the
>>>>>> averaging
>>>>>> setting, min AVG being 1:1.
>>>>>>
>>>>>> Any sysfs "show" read access from the client app faster than 500 Hz
>>>>>> will be
>>>>>> 'cached' by the driver, but actually since do_update reads all 8
>>>>>> registers,
>>>>>> the best achievable measurement rate is roughly 8*800 us (for the
>>>>>> time
>>>>>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>>>>>
>>>>>> This change set uses a register mask to allow for the readout of a
>>>>>> single
>>>>>> i2c register at a time. Furthermore, performing subsequent reads
>>>>>> on the
>>>>>> same register will make use of the ability of the i2c chip to retain
>>>>>> the
>>>>>> last reg offset, hence use a shorter i2c message (roughly 400us
>>>>>> instead of
>>>>>> 800us spent in i2c-core.c).
>>>>>>
>>>>> That doesn't work. There could be accesses from other sources (such as
>>>>> through
>>>>> i2c-dev, or in multi-master systems) between two reads.
>>>>
>>>> Re-setting the register address with each read transaction will not
>>>> prevent another master to change the configuration in your back, in
>>>> this case. That sounds like a general issue of concurrent clients for
>>>> one device, this is beyond just reading one register IMO.
>>>>
>>> That is an invasive change, though, not just a simple read. Sure, it is
>>> a risk as well. But it is a different level of risk than someone using
>>> i2cget or i2cdump while the driver is running.
>>>
>> Yes, I get your point.
>>
>>>>>
>>>>>> The best readout rate for a single measurement is now around 2kHz.
>>>>>> And
>>>>>> for
>>>>>> four measurements around (1/(4*800us) = 312 Hz. Since for any readout
>>>>>> rate
>>>>>> faster than 160 Hz the interval is set by the i2c transactions
>>>>>> completion,
>>>>>> the 'last-update' anti-flooding code will not have a limiting
>>>>>> effect in
>>>>>> practice. Hence I also remove the elapsed time checking in the hwmon
>>>>>> driver
>>>>>> for ina2xx.
>>>>>>
>>>>>> To summarize, the patch provides a max bandwidth improvement with
>>>>>> hwmon
>>>>>> client apps from ~160 Hz to ~320 Hz, and better in single-channel
>>>>>> polling mode.
>>>>>>
>>>>> Overall your patch pretty much re-implements regmap. Since you drop
>>>>> caching,
>>>>> it is also unnecessary to read all registers at a time, so you can
>>>>> just use
>>>>> a function to read _one_ register and returns its value (with
>>>>> retries).
>>>>> Or use regmap. Either case, do_update() and ina2xx_update_device()
>>>>> are no
>>>>> longer needed.
>>>> Agreed.
>>>>
>>>>>
>>>>> If you want to convert the driver to regmap, just look for 'regmap' in
>>>>> drivers/hwmon for examples.
>>>>
>>>> Fair enough, but based on your comments, I may look into an iio driver
>>>> instead for this device, given our application, rather than 'twisting'
>>>> the hwmon interface.
>>>>
>>>
>>> Sorry, you lost me there. How are you twisting the hwmon interface ?
>>> Because I am concerned about multiple accesses from multiple sources ?
>>> How is iio going to solve that problem ? By ignoring it ?
>>
>> Sure someone can still use i2c diag tools as you said, you have a
>> point here. But similarly, someone can use /dev/mem to remap stuff and
>> peek/poke mm registers, and to my knowledge we do not generally design
>> drivers or subsystems with retries and feature limitations to cope
>> with potential use of diag and debug facilities.
>>
>
> Depends on the system, and on the use case. Many systems I deal with
> are multi-master, and user space does end up accessing chips using i2c-dev.
>
> Drivers are (supposed to be) designed with as much multi-master access
> safety as possible. I had patches rejected because they broke multi-master
> support by sending two commands depending on each other in a sequence.
>
I Guenter,

I will rework this patch for using regmap, I'll leave the short i2c read 
out for now. The iio story is OOT, my bad, it's related to baylibre's 
product and is unrelated to this thread really, sorry for that.


> Using /dev/mem and peek/poke as counter-examples isn't really appropriate
> here, so I won't comment further on it.
>
Agreed. Further comments on that part would require a bar table and two 
beers ;)

Thanks,
Marc.


> Thanks,
> Guenter
>

[RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Marc Titinger]

With the current implementation, the driver will prevent a readout at a
pace faster than the default conversion time (2ms) times the averaging
setting, min AVG being 1:1.

Any sysfs "show" read access from the client app faster than 500 Hz will be
'cached' by the driver, but actually since do_update reads all 8 registers,
the best achievable measurement rate is roughly 8*800 us (for the time
spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.

Registers are now accessed individually through the regmap facility.
For four measurements the readout rate is now around
 (1/(4*800us) = 312 Hz.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 359 ++++++++++++++++++++++---------------------------
 1 file changed, 159 insertions(+), 200 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..e7c1aaa 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/util_macros.h>
+#include <linux/regmap.h>
 
 #include <linux/platform_data/ina2xx.h>
 
@@ -48,32 +49,25 @@
 #define INA2XX_CURRENT			0x04 /* readonly */
 #define INA2XX_CALIBRATION		0x05
 
-/* INA226 register definitions */
-#define INA226_MASK_ENABLE		0x06
-#define INA226_ALERT_LIMIT		0x07
-#define INA226_DIE_ID			0xFF
-
-/* register count */
-#define INA219_REGISTERS		6
-#define INA226_REGISTERS		8
-
-#define INA2XX_MAX_REGISTERS		8
+/* CONFIG register fields */
+#define INA2XX_AVG_MASK			0x0E00
+#define INA2XX_AVG_SHFT			9
 
 /* settings - depend on use case */
 #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
 #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
 
 /* worst case is 68.10 ms (~14.6Hz, ina219) */
-#define INA2XX_CONVERSION_RATE		15
 #define INA2XX_MAX_DELAY		69 /* worst case delay in ms */
 
 #define INA2XX_RSHUNT_DEFAULT		10000
 
-/* bit mask for reading the averaging setting in the configuration register */
-#define INA226_AVG_RD_MASK		0x0E00
-
-#define INA226_READ_AVG(reg)		(((reg) & INA226_AVG_RD_MASK) >> 9)
-#define INA226_SHIFT_AVG(val)		((val) << 9)
+/* Currently only handling common register set */
+static const struct regmap_config INA2XX_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = INA2XX_CALIBRATION,
+};
 
 /* common attrs, ina226 attrs and NULL */
 #define INA2XX_MAX_ATTRIBUTE_GROUPS	3
@@ -89,7 +83,6 @@ enum ina2xx_ids { ina219, ina226 };
 struct ina2xx_config {
 	u16 config_default;
 	int calibration_factor;
-	int registers;
 	int shunt_div;
 	int bus_voltage_shift;
 	int bus_voltage_lsb;	/* uV */
@@ -99,25 +92,16 @@ struct ina2xx_config {
 struct ina2xx_data {
 	struct i2c_client *client;
 	const struct ina2xx_config *config;
-
+	struct regmap *regmap;
 	long rshunt;
-	u16 curr_config;
-
-	struct mutex update_lock;
-	bool valid;
-	unsigned long last_updated;
-	int update_interval; /* in jiffies */
-
-	int kind;
+	int valid;
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
-	u16 regs[INA2XX_MAX_REGISTERS];
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
 	[ina219] = {
 		.config_default = INA219_CONFIG_DEFAULT,
 		.calibration_factor = 40960000,
-		.registers = INA219_REGISTERS,
 		.shunt_div = 100,
 		.bus_voltage_shift = 3,
 		.bus_voltage_lsb = 4000,
@@ -126,7 +110,6 @@ static const struct ina2xx_config ina2xx_config[] = {
 	[ina226] = {
 		.config_default = INA226_CONFIG_DEFAULT,
 		.calibration_factor = 5120000,
-		.registers = INA226_REGISTERS,
 		.shunt_div = 400,
 		.bus_voltage_shift = 0,
 		.bus_voltage_lsb = 1250,
@@ -142,9 +125,9 @@ static const struct ina2xx_config ina2xx_config[] = {
  */
 static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
 
-static int ina226_reg_to_interval(u16 config)
+static int ina226_field_to_interval(int field)
 {
-	int avg = ina226_avg_tab[INA226_READ_AVG(config)];
+	int avg = ina226_avg_tab[field];
 
 	/*
 	 * Multiply the total conversion time by the number of averages.
@@ -153,24 +136,11 @@ static int ina226_reg_to_interval(u16 config)
 	return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 1000);
 }
 
-static u16 ina226_interval_to_reg(int interval, u16 config)
-{
-	int avg, avg_bits;
-
-	avg = DIV_ROUND_CLOSEST(interval * 1000,
-				INA226_TOTAL_CONV_TIME_DEFAULT);
-	avg_bits = find_closest(avg, ina226_avg_tab,
-				ARRAY_SIZE(ina226_avg_tab));
-
-	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
-}
-
-static void ina226_set_update_interval(struct ina2xx_data *data)
+static int ina226_interval_to_field(int interval)
 {
-	int ms;
-
-	ms = ina226_reg_to_interval(data->curr_config);
-	data->update_interval = msecs_to_jiffies(ms);
+	int avg = DIV_ROUND_CLOSEST(interval * 1000,
+				    INA226_TOTAL_CONV_TIME_DEFAULT);
+	return find_closest(avg, ina226_avg_tab, ARRAY_SIZE(ina226_avg_tab));
 }
 
 static int ina2xx_calibrate(struct ina2xx_data *data)
@@ -178,8 +148,7 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
 	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
 				    data->rshunt);
 
-	return i2c_smbus_write_word_swapped(data->client,
-					    INA2XX_CALIBRATION, val);
+	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
 }
 
 /*
@@ -187,15 +156,10 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
  */
 static int ina2xx_init(struct ina2xx_data *data)
 {
-	struct i2c_client *client = data->client;
-	int ret;
-
-	/* device configuration */
-	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-					   data->curr_config);
-	if (ret < 0)
+	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
+			       data->config->config_default);
+	if (ret)
 		return ret;
-
 	/*
 	 * Set current LSB to 1mA, shunt is in uOhms
 	 * (equation 13 in datasheet).
@@ -203,22 +167,22 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+static int ina2xx_show_common(struct device *dev, int index, int *val)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int i, rv, retry;
+	int err, retry;
 
-	dev_dbg(&client->dev, "Starting ina2xx update\n");
+	if (unlikely(!val))
+		return -EINVAL;
 
 	for (retry = 5; retry; retry--) {
-		/* Read all registers */
-		for (i = 0; i < data->config->registers; i++) {
-			rv = i2c_smbus_read_word_swapped(client, i);
-			if (rv < 0)
-				return rv;
-			data->regs[i] = rv;
-		}
+
+		/* Check for remaining registers in mask. */
+		err = regmap_read(data->regmap, index, val);
+		if (err)
+			return err;
+
+		dev_dbg(dev, "read %d, val = 0x%04x\n", index, *val);
 
 		/*
 		 * If the current value in the calibration register is 0, the
@@ -226,24 +190,24 @@ static int ina2xx_do_update(struct device *dev)
 		 * the chip has been reset let's check the calibration
 		 * register and reinitialize if needed.
 		 */
-		if (data->regs[INA2XX_CALIBRATION] == 0) {
-			dev_warn(dev, "chip not calibrated, reinitializing\n");
-
-			rv = ina2xx_init(data);
-			if (rv < 0)
-				return rv;
+		if (!data->valid) {
+			dev_warn(dev,
+				 "chip needs calibration, reinitializing\n");
 
+			err = ina2xx_calibrate(data);
+			if (err)
+				return err;
 			/*
 			 * Let's make sure the power and current registers
 			 * have been updated before trying again.
 			 */
 			msleep(INA2XX_MAX_DELAY);
+
+			/* data valid once we have a cal value. */
+			regmap_read(data->regmap, INA2XX_CALIBRATION,
+				    &data->valid);
 			continue;
 		}
-
-		data->last_updated = jiffies;
-		data->valid = 1;
-
 		return 0;
 	}
 
@@ -256,86 +220,89 @@ static int ina2xx_do_update(struct device *dev)
 	return -ENODEV;
 }
 
-static struct ina2xx_data *ina2xx_update_device(struct device *dev)
+static ssize_t ina2xx_show_shunt(struct device *dev,
+				 struct device_attribute *da, char *buf)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct ina2xx_data *ret = data;
-	unsigned long after;
-	int rv;
+	int val, err;
 
-	mutex_lock(&data->update_lock);
+	err = ina2xx_show_common(dev, INA2XX_SHUNT_VOLTAGE, &val);
+	if (err)
+		return err;
 
-	after = data->last_updated + data->update_interval;
-	if (time_after(jiffies, after) || !data->valid) {
-		rv = ina2xx_do_update(dev);
-		if (rv < 0)
-			ret = ERR_PTR(rv);
-	}
+	val = DIV_ROUND_CLOSEST((s16) val, data->config->shunt_div);
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t ina2xx_show_bus(struct device *dev,
+			       struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int val, err;
+
+	err = ina2xx_show_common(dev, INA2XX_BUS_VOLTAGE, &val);
+	if (err)
+		return err;
 
-	mutex_unlock(&data->update_lock);
-	return ret;
+	val = (val >> data->config->bus_voltage_shift)
+	    * data->config->bus_voltage_lsb;
+	val = DIV_ROUND_CLOSEST(val, 1000);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
-static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
+static ssize_t ina2xx_show_pow(struct device *dev,
+			       struct device_attribute *da, char *buf)
 {
-	int val;
-
-	switch (reg) {
-	case INA2XX_SHUNT_VOLTAGE:
-		/* signed register */
-		val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
-					data->config->shunt_div);
-		break;
-	case INA2XX_BUS_VOLTAGE:
-		val = (data->regs[reg] >> data->config->bus_voltage_shift)
-		  * data->config->bus_voltage_lsb;
-		val = DIV_ROUND_CLOSEST(val, 1000);
-		break;
-	case INA2XX_POWER:
-		val = data->regs[reg] * data->config->power_lsb;
-		break;
-	case INA2XX_CURRENT:
-		/* signed register, LSB=1mA (selected), in mA */
-		val = (s16)data->regs[reg];
-		break;
-	case INA2XX_CALIBRATION:
-		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
-					data->regs[reg]);
-		break;
-	default:
-		/* programmer goofed */
-		WARN_ON_ONCE(1);
-		val = 0;
-		break;
-	}
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int val, err;
+
+	err = ina2xx_show_common(dev, INA2XX_POWER, &val);
+	if (err)
+		return err;
+
+	val *= data->config->power_lsb;
 
-	return val;
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
-static ssize_t ina2xx_show_value(struct device *dev,
-				 struct device_attribute *da, char *buf)
+static ssize_t ina2xx_show_curr(struct device *dev,
+				struct device_attribute *da, char *buf)
 {
-	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	int val, err;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	err = ina2xx_show_common(dev, INA2XX_CURRENT, &val);
+	if (err)
+		return err;
 
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina2xx_get_value(data, attr->index));
+	/* signed register, LSB=1mA (selected), in mA */
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t ina2xx_show_cal(struct device *dev,
+			       struct device_attribute *da, char *buf)
+{
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int val, err;
+
+	err = ina2xx_show_common(dev, INA2XX_CALIBRATION, &val);
+	if (err)
+		return err;
+
+	val = DIV_ROUND_CLOSEST(data->config->calibration_factor, val);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
 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);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+
 	unsigned long val;
 	int status;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
-
 	status = kstrtoul(buf, 10, &val);
 	if (status < 0)
 		return status;
@@ -345,12 +312,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	    val > data->config->calibration_factor)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
 	data->rshunt = val;
-	status = ina2xx_calibrate(data);
-	mutex_unlock(&data->update_lock);
-	if (status < 0)
-		return status;
+	data->valid = 0;
 
 	return count;
 }
@@ -370,65 +333,58 @@ static ssize_t ina226_set_interval(struct device *dev,
 	if (val > INT_MAX || val == 0)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->curr_config = ina226_interval_to_reg(val,
-						   data->regs[INA2XX_CONFIG]);
-	status = i2c_smbus_write_word_swapped(data->client,
-					      INA2XX_CONFIG,
-					      data->curr_config);
+	val = ina226_interval_to_field(val);
 
-	ina226_set_update_interval(data);
-	/* Make sure the next access re-reads all registers. */
-	data->valid = 0;
-	mutex_unlock(&data->update_lock);
+	status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
+				    INA2XX_AVG_MASK, (val << INA2XX_AVG_SHFT));
 	if (status < 0)
 		return status;
 
+	data->valid = 0;
+
 	return count;
 }
 
 static ssize_t ina226_show_interval(struct device *dev,
 				    struct device_attribute *da, char *buf)
 {
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int status, val;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	status = regmap_read(data->regmap, INA2XX_CONFIG, &val);
+	if (status)
+		return status;
+
+	val = (val & INA2XX_AVG_MASK) >> INA2XX_AVG_SHFT;
+	val = ina226_field_to_interval(val);
 
 	/*
-	 * We don't use data->update_interval here as we want to display
-	 * the actual interval used by the chip and jiffies_to_msecs()
-	 * doesn't seem to be accurate enough.
+	 * We want to display the actual interval used by the chip.
 	 */
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
+	return snprintf(buf, PAGE_SIZE, "%d\n", val);
 }
 
 /* shunt voltage */
-static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_shunt, NULL,
 			  INA2XX_SHUNT_VOLTAGE);
 
 /* bus voltage */
-static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_value, NULL,
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_bus, NULL,
 			  INA2XX_BUS_VOLTAGE);
 
 /* calculated current */
-static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_value, NULL,
+static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_curr, NULL,
 			  INA2XX_CURRENT);
 
 /* calculated power */
-static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
+static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_pow, NULL,
 			  INA2XX_POWER);
 
 /* shunt resistance */
 static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
-			  ina2xx_show_value, ina2xx_set_shunt,
+			  ina2xx_show_cal, ina2xx_set_shunt,
 			  INA2XX_CALIBRATION);
 
-/* update interval (ina226 only) */
-static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
-			  ina226_show_interval, ina226_set_interval, 0);
-
 /* pointers to created device attributes */
 static struct attribute *ina2xx_attrs[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
@@ -443,6 +399,10 @@ static const struct attribute_group ina2xx_group = {
 	.attrs = ina2xx_attrs,
 };
 
+/* update interval (ina226 only) */
+static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
+			  ina226_show_interval, ina226_set_interval, 0);
+
 static struct attribute *ina226_attrs[] = {
 	&sensor_dev_attr_update_interval.dev_attr.attr,
 	NULL,
@@ -452,65 +412,65 @@ static const struct attribute_group ina226_group = {
 	.attrs = ina226_attrs,
 };
 
+
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct i2c_adapter *adapter = client->adapter;
 	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
+	struct regmap *regmap;
 	u32 val;
 	int ret, group = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
-		return -ENODEV;
+	/* Register regmap */
+	regmap = devm_regmap_init_i2c(client, &INA2XX_regmap_config);
+	if (IS_ERR(regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(regmap);
+	}
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	if (dev_get_platdata(dev)) {
-		pdata = dev_get_platdata(dev);
-		data->rshunt = pdata->shunt_uohms;
-	} else if (!of_property_read_u32(dev->of_node,
-					 "shunt-resistor", &val)) {
-		data->rshunt = val;
-	} else {
-		data->rshunt = INA2XX_RSHUNT_DEFAULT;
-	}
+	data->regmap = regmap;
 
-	/* set the device type */
-	data->kind = id->driver_data;
-	data->config = &ina2xx_config[data->kind];
-	data->curr_config = data->config->config_default;
+	/* Set config according to device type. */
+	data->config = &ina2xx_config[id->driver_data];
 	data->client = client;
 
-	/*
-	 * Ina226 has a variable update_interval. For ina219 we
-	 * use a constant value.
+	/* Check for shunt resistor value.
+	 * Give precedence to device tree over must-recompile.
 	 */
-	if (data->kind == ina226)
-		ina226_set_update_interval(data);
-	else
-		data->update_interval = HZ / INA2XX_CONVERSION_RATE;
+	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
+		pdata = dev_get_platdata(dev);
+		if (pdata)
+			val = pdata->shunt_uohms;
+		else
+			val = INA2XX_RSHUNT_DEFAULT;
+	}
 
-	if (data->rshunt <= 0 ||
-	    data->rshunt > data->config->calibration_factor)
+	if (val <= 0 || val > data->config->calibration_factor) {
+		dev_err(dev, "Invalid shunt resistor value %li", val);
 		return -ENODEV;
+	}
+	data->rshunt = val;
 
+	/* Write config to chip, and calibrate */
 	ret = ina2xx_init(data);
-	if (ret < 0) {
-		dev_err(dev, "error configuring the device: %d\n", ret);
-		return -ENODEV;
+	if (ret) {
+		dev_err(dev, "error configuring the device.\n");
+		return ret;
 	}
 
-	mutex_init(&data->update_lock);
-
+	/* Set sensor group according to device type. */
 	data->groups[group++] = &ina2xx_group;
-	if (data->kind == ina226)
+	if (ina226 == id->driver_data)
 		data->groups[group++] = &ina226_group;
 
+	/* register to hwmon */
 	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
 							   data, data->groups);
 	if (IS_ERR(hwmon_dev))
@@ -541,7 +501,6 @@ static struct i2c_driver ina2xx_driver = {
 };
 
 module_i2c_driver(ina2xx_driver);
-
 MODULE_AUTHOR("Lothar Felten <l-felten@ti.com>");
 MODULE_DESCRIPTION("ina2xx driver");
 MODULE_LICENSE("GPL");
-- 
1.9.1

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

Hi Marc,

[auto build test WARNING on hwmon/hwmon-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Marc-Titinger/hwmon-ina2xx-port-to-using-remap-improve-bandwidth/20151024-001809
config: x86_64-randconfig-x011-10211707 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/hwmon/ina2xx.c: In function 'ina2xx_probe':
>> drivers/hwmon/ina2xx.c:456:16: warning: format '%li' expects argument of type 'long int', but argument 3 has type 'u32 {aka unsigned int}' [-Wformat=]
      dev_err(dev, "Invalid shunt resistor value %li", val);
                   ^

vim +456 drivers/hwmon/ina2xx.c

   440		/* Set config according to device type. */
   441		data->config = &ina2xx_config[id->driver_data];
   442		data->client = client;
   443	
   444		/* Check for shunt resistor value.
   445		 * Give precedence to device tree over must-recompile.
   446		 */
   447		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
   448			pdata = dev_get_platdata(dev);
   449			if (pdata)
   450				val = pdata->shunt_uohms;
   451			else
   452				val = INA2XX_RSHUNT_DEFAULT;
   453		}
   454	
   455		if (val <= 0 || val > data->config->calibration_factor) {
 > 456			dev_err(dev, "Invalid shunt resistor value %li", val);
   457			return -ENODEV;
   458		}
   459		data->rshunt = val;
   460	
   461		/* Write config to chip, and calibrate */
   462		ret = ina2xx_init(data);
   463		if (ret) {
   464			dev_err(dev, "error configuring the device.\n");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26613 bytes --]

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Guenter Roeck]

On 10/23/2015 09:13 AM, Marc Titinger wrote:
> With the current implementation, the driver will prevent a readout at a
> pace faster than the default conversion time (2ms) times the averaging
> setting, min AVG being 1:1.
>
> Any sysfs "show" read access from the client app faster than 500 Hz will be

500 Hz ?

> 'cached' by the driver, but actually since do_update reads all 8 registers,
> the best achievable measurement rate is roughly 8*800 us (for the time
> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>
> Registers are now accessed individually through the regmap facility.
> For four measurements the readout rate is now around
>   (1/(4*800us) = 312 Hz.

I'll need some time to go through the patch in detail Some quick comments below.

>
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>   drivers/hwmon/ina2xx.c | 359 ++++++++++++++++++++++---------------------------
>   1 file changed, 159 insertions(+), 200 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 4d28150..e7c1aaa 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -37,6 +37,7 @@
>   #include <linux/of.h>
>   #include <linux/delay.h>
>   #include <linux/util_macros.h>
> +#include <linux/regmap.h>
>
>   #include <linux/platform_data/ina2xx.h>
>
> @@ -48,32 +49,25 @@
>   #define INA2XX_CURRENT			0x04 /* readonly */
>   #define INA2XX_CALIBRATION		0x05
>
> -/* INA226 register definitions */
> -#define INA226_MASK_ENABLE		0x06
> -#define INA226_ALERT_LIMIT		0x07
> -#define INA226_DIE_ID			0xFF
> -
> -/* register count */
> -#define INA219_REGISTERS		6
> -#define INA226_REGISTERS		8
> -
> -#define INA2XX_MAX_REGISTERS		8
> +/* CONFIG register fields */
> +#define INA2XX_AVG_MASK			0x0E00
> +#define INA2XX_AVG_SHFT			9
>
>   /* settings - depend on use case */
>   #define INA219_CONFIG_DEFAULT		0x399F	/* PGA=8 */
>   #define INA226_CONFIG_DEFAULT		0x4527	/* averages=16 */
>
>   /* worst case is 68.10 ms (~14.6Hz, ina219) */
> -#define INA2XX_CONVERSION_RATE		15
>   #define INA2XX_MAX_DELAY		69 /* worst case delay in ms */
>
>   #define INA2XX_RSHUNT_DEFAULT		10000
>
> -/* bit mask for reading the averaging setting in the configuration register */
> -#define INA226_AVG_RD_MASK		0x0E00
> -
> -#define INA226_READ_AVG(reg)		(((reg) & INA226_AVG_RD_MASK) >> 9)
> -#define INA226_SHIFT_AVG(val)		((val) << 9)
> +/* Currently only handling common register set */
> +static const struct regmap_config INA2XX_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = INA2XX_CALIBRATION,
> +};
>
>   /* common attrs, ina226 attrs and NULL */
>   #define INA2XX_MAX_ATTRIBUTE_GROUPS	3
> @@ -89,7 +83,6 @@ enum ina2xx_ids { ina219, ina226 };
>   struct ina2xx_config {
>   	u16 config_default;
>   	int calibration_factor;
> -	int registers;
>   	int shunt_div;
>   	int bus_voltage_shift;
>   	int bus_voltage_lsb;	/* uV */
> @@ -99,25 +92,16 @@ struct ina2xx_config {
>   struct ina2xx_data {
>   	struct i2c_client *client;
>   	const struct ina2xx_config *config;
> -
> +	struct regmap *regmap;
>   	long rshunt;
> -	u16 curr_config;
> -
> -	struct mutex update_lock;
> -	bool valid;
> -	unsigned long last_updated;
> -	int update_interval; /* in jiffies */
> -
> -	int kind;
> +	int valid;

bool, please. But then valid was supposed to be used to indicate
if the cache is valid, and should no longer be needed since there
is no more cache. I'll have to spend more time to understand if and
why it is still needed and how it is used.

>   	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
> -	u16 regs[INA2XX_MAX_REGISTERS];
>   };
>
>   static const struct ina2xx_config ina2xx_config[] = {
>   	[ina219] = {
>   		.config_default = INA219_CONFIG_DEFAULT,
>   		.calibration_factor = 40960000,
> -		.registers = INA219_REGISTERS,
>   		.shunt_div = 100,
>   		.bus_voltage_shift = 3,
>   		.bus_voltage_lsb = 4000,
> @@ -126,7 +110,6 @@ static const struct ina2xx_config ina2xx_config[] = {
>   	[ina226] = {
>   		.config_default = INA226_CONFIG_DEFAULT,
>   		.calibration_factor = 5120000,
> -		.registers = INA226_REGISTERS,
>   		.shunt_div = 400,
>   		.bus_voltage_shift = 0,
>   		.bus_voltage_lsb = 1250,
> @@ -142,9 +125,9 @@ static const struct ina2xx_config ina2xx_config[] = {
>    */
>   static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
>
> -static int ina226_reg_to_interval(u16 config)
> +static int ina226_field_to_interval(int field)
>   {
> -	int avg = ina226_avg_tab[INA226_READ_AVG(config)];
> +	int avg = ina226_avg_tab[field];
>
>   	/*
>   	 * Multiply the total conversion time by the number of averages.
> @@ -153,24 +136,11 @@ static int ina226_reg_to_interval(u16 config)
>   	return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 1000);
>   }
>
> -static u16 ina226_interval_to_reg(int interval, u16 config)
> -{
> -	int avg, avg_bits;
> -
> -	avg = DIV_ROUND_CLOSEST(interval * 1000,
> -				INA226_TOTAL_CONV_TIME_DEFAULT);
> -	avg_bits = find_closest(avg, ina226_avg_tab,
> -				ARRAY_SIZE(ina226_avg_tab));
> -
> -	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
> -}
> -
> -static void ina226_set_update_interval(struct ina2xx_data *data)
> +static int ina226_interval_to_field(int interval)
>   {
> -	int ms;
> -
> -	ms = ina226_reg_to_interval(data->curr_config);
> -	data->update_interval = msecs_to_jiffies(ms);
> +	int avg = DIV_ROUND_CLOSEST(interval * 1000,
> +				    INA226_TOTAL_CONV_TIME_DEFAULT);
> +	return find_closest(avg, ina226_avg_tab, ARRAY_SIZE(ina226_avg_tab));
>   }
>
>   static int ina2xx_calibrate(struct ina2xx_data *data)
> @@ -178,8 +148,7 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
>   	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
>   				    data->rshunt);
>
> -	return i2c_smbus_write_word_swapped(data->client,
> -					    INA2XX_CALIBRATION, val);
> +	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
>   }
>
>   /*
> @@ -187,15 +156,10 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
>    */
>   static int ina2xx_init(struct ina2xx_data *data)
>   {
> -	struct i2c_client *client = data->client;
> -	int ret;
> -
> -	/* device configuration */
> -	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
> -					   data->curr_config);
> -	if (ret < 0)
> +	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
> +			       data->config->config_default);
> +	if (ret)
>   		return ret;
> -
>   	/*
>   	 * Set current LSB to 1mA, shunt is in uOhms
>   	 * (equation 13 in datasheet).
> @@ -203,22 +167,22 @@ static int ina2xx_init(struct ina2xx_data *data)
>   	return ina2xx_calibrate(data);
>   }
>
> -static int ina2xx_do_update(struct device *dev)
> +static int ina2xx_show_common(struct device *dev, int index, int *val)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int i, rv, retry;
> +	int err, retry;
>
> -	dev_dbg(&client->dev, "Starting ina2xx update\n");
> +	if (unlikely(!val))
> +		return -EINVAL;

Please no unnecessary parameter checks. If val is NULL here,
something is wrong with the implementation and needs to get fixed.

Besides, the return value is only negative on errors. There is no need
to pass an extra pointer to the value. Furthermore, regmap_read expects
a pointer to an unsigned int, not a pointer to an int.

Just return an error or the value returned by regmap. The calling code
can determine if there is an error by checking if the returned value is < 0.

>
>   	for (retry = 5; retry; retry--) {
> -		/* Read all registers */
> -		for (i = 0; i < data->config->registers; i++) {
> -			rv = i2c_smbus_read_word_swapped(client, i);
> -			if (rv < 0)
> -				return rv;
> -			data->regs[i] = rv;
> -		}
> +
> +		/* Check for remaining registers in mask. */
> +		err = regmap_read(data->regmap, index, val);
> +		if (err)
> +			return err;
> +
> +		dev_dbg(dev, "read %d, val = 0x%04x\n", index, *val);
>
>   		/*
>   		 * If the current value in the calibration register is 0, the
> @@ -226,24 +190,24 @@ static int ina2xx_do_update(struct device *dev)
>   		 * the chip has been reset let's check the calibration
>   		 * register and reinitialize if needed.
>   		 */
> -		if (data->regs[INA2XX_CALIBRATION] == 0) {
> -			dev_warn(dev, "chip not calibrated, reinitializing\n");
> -
> -			rv = ina2xx_init(data);
> -			if (rv < 0)
> -				return rv;
> +		if (!data->valid) {
> +			dev_warn(dev,
> +				 "chip needs calibration, reinitializing\n");
>
> +			err = ina2xx_calibrate(data);
> +			if (err)
> +				return err;

Ah, that is the context of "valid". I don't immediately see why moving its scope
out of this function, and setting it to false in the calling code, is beneficial here.

This also changes the dynamic of the driver. Previously it was specifically able
to detect if the chip was power cycled (for whatever reason) and it would re-initialize
itself. Now you use 'valid' and you defer (re-)initialization until chip registers
are read. I don't immediately see if and how this is beneficial.

>   			/*
>   			 * Let's make sure the power and current registers
>   			 * have been updated before trying again.
>   			 */
>   			msleep(INA2XX_MAX_DELAY);
> +
> +			/* data valid once we have a cal value. */
> +			regmap_read(data->regmap, INA2XX_CALIBRATION,
> +				    &data->valid);
>   			continue;
>   		}
> -
> -		data->last_updated = jiffies;
> -		data->valid = 1;
> -
>   		return 0;
>   	}
>
> @@ -256,86 +220,89 @@ static int ina2xx_do_update(struct device *dev)
>   	return -ENODEV;
>   }
>
> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
> +static ssize_t ina2xx_show_shunt(struct device *dev,
> +				 struct device_attribute *da, char *buf)
>   {
>   	struct ina2xx_data *data = dev_get_drvdata(dev);
> -	struct ina2xx_data *ret = data;
> -	unsigned long after;
> -	int rv;
> +	int val, err;
>
> -	mutex_lock(&data->update_lock);
> +	err = ina2xx_show_common(dev, INA2XX_SHUNT_VOLTAGE, &val);
> +	if (err)
> +		return err;
>
> -	after = data->last_updated + data->update_interval;
> -	if (time_after(jiffies, after) || !data->valid) {
> -		rv = ina2xx_do_update(dev);
> -		if (rv < 0)
> -			ret = ERR_PTR(rv);
> -	}
> +	val = DIV_ROUND_CLOSEST((s16) val, data->config->shunt_div);
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t ina2xx_show_bus(struct device *dev,
> +			       struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	int val, err;
> +
> +	err = ina2xx_show_common(dev, INA2XX_BUS_VOLTAGE, &val);
> +	if (err)
> +		return err;
>
> -	mutex_unlock(&data->update_lock);
> -	return ret;
> +	val = (val >> data->config->bus_voltage_shift)
> +	    * data->config->bus_voltage_lsb;
> +	val = DIV_ROUND_CLOSEST(val, 1000);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
>   }
>
> -static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
> +static ssize_t ina2xx_show_pow(struct device *dev,
> +			       struct device_attribute *da, char *buf)
>   {
> -	int val;
> -
> -	switch (reg) {
> -	case INA2XX_SHUNT_VOLTAGE:
> -		/* signed register */
> -		val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
> -					data->config->shunt_div);
> -		break;
> -	case INA2XX_BUS_VOLTAGE:
> -		val = (data->regs[reg] >> data->config->bus_voltage_shift)
> -		  * data->config->bus_voltage_lsb;
> -		val = DIV_ROUND_CLOSEST(val, 1000);
> -		break;
> -	case INA2XX_POWER:
> -		val = data->regs[reg] * data->config->power_lsb;
> -		break;
> -	case INA2XX_CURRENT:
> -		/* signed register, LSB=1mA (selected), in mA */
> -		val = (s16)data->regs[reg];
> -		break;
> -	case INA2XX_CALIBRATION:
> -		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
> -					data->regs[reg]);
> -		break;
> -	default:
> -		/* programmer goofed */
> -		WARN_ON_ONCE(1);
> -		val = 0;
> -		break;
> -	}
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	int val, err;
> +
> +	err = ina2xx_show_common(dev, INA2XX_POWER, &val);
> +	if (err)
> +		return err;
> +
> +	val *= data->config->power_lsb;
>
> -	return val;
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
>   }
>
> -static ssize_t ina2xx_show_value(struct device *dev,
> -				 struct device_attribute *da, char *buf)
> +static ssize_t ina2xx_show_curr(struct device *dev,
> +				struct device_attribute *da, char *buf)
>   {
> -	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	int val, err;
>
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	err = ina2xx_show_common(dev, INA2XX_CURRENT, &val);
> +	if (err)
> +		return err;
>
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			ina2xx_get_value(data, attr->index));
> +	/* signed register, LSB=1mA (selected), in mA */
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t ina2xx_show_cal(struct device *dev,
> +			       struct device_attribute *da, char *buf)
> +{
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	int val, err;
> +
> +	err = ina2xx_show_common(dev, INA2XX_CALIBRATION, &val);
> +	if (err)
> +		return err;
> +
> +	val = DIV_ROUND_CLOSEST(data->config->calibration_factor, val);
> +
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
>   }
>
>   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);
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +
>   	unsigned long val;
>   	int status;
>
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> -
>   	status = kstrtoul(buf, 10, &val);
>   	if (status < 0)
>   		return status;
> @@ -345,12 +312,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>   	    val > data->config->calibration_factor)
>   		return -EINVAL;
>
> -	mutex_lock(&data->update_lock);
>   	data->rshunt = val;
> -	status = ina2xx_calibrate(data);
> -	mutex_unlock(&data->update_lock);
> -	if (status < 0)
> -		return status;
> +	data->valid = 0;
>
>   	return count;
>   }
> @@ -370,65 +333,58 @@ static ssize_t ina226_set_interval(struct device *dev,
>   	if (val > INT_MAX || val == 0)
>   		return -EINVAL;
>
> -	mutex_lock(&data->update_lock);
> -	data->curr_config = ina226_interval_to_reg(val,
> -						   data->regs[INA2XX_CONFIG]);
> -	status = i2c_smbus_write_word_swapped(data->client,
> -					      INA2XX_CONFIG,
> -					      data->curr_config);
> +	val = ina226_interval_to_field(val);
>
> -	ina226_set_update_interval(data);
> -	/* Make sure the next access re-reads all registers. */
> -	data->valid = 0;
> -	mutex_unlock(&data->update_lock);
> +	status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
> +				    INA2XX_AVG_MASK, (val << INA2XX_AVG_SHFT));
>   	if (status < 0)
>   		return status;
>
> +	data->valid = 0;
> +

This now causes re-calibration. Why ?

>   	return count;
>   }
>
>   static ssize_t ina226_show_interval(struct device *dev,
>   				    struct device_attribute *da, char *buf)
>   {
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	int status, val;
>
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	status = regmap_read(data->regmap, INA2XX_CONFIG, &val);
> +	if (status)
> +		return status;
> +
> +	val = (val & INA2XX_AVG_MASK) >> INA2XX_AVG_SHFT;
> +	val = ina226_field_to_interval(val);
>
>   	/*
> -	 * We don't use data->update_interval here as we want to display
> -	 * the actual interval used by the chip and jiffies_to_msecs()
> -	 * doesn't seem to be accurate enough.
> +	 * We want to display the actual interval used by the chip.
>   	 */
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
> +	return snprintf(buf, PAGE_SIZE, "%d\n", val);
>   }
>
>   /* shunt voltage */
> -static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_shunt, NULL,
>   			  INA2XX_SHUNT_VOLTAGE);

If you no longer use the register parameter, there is no need to provide it.
But then I'll have to spend some time trying to understand _why_ you don't
use it anymore and why you introduced separate show functions.
Some explanation might help.

>
>   /* bus voltage */
> -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_value, NULL,
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_bus, NULL,
>   			  INA2XX_BUS_VOLTAGE);
>
>   /* calculated current */
> -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_value, NULL,
> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_curr, NULL,
>   			  INA2XX_CURRENT);
>
>   /* calculated power */
> -static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
> +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_pow, NULL,
>   			  INA2XX_POWER);
>
>   /* shunt resistance */
>   static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
> -			  ina2xx_show_value, ina2xx_set_shunt,
> +			  ina2xx_show_cal, ina2xx_set_shunt,
>   			  INA2XX_CALIBRATION);
>
> -/* update interval (ina226 only) */
> -static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
> -			  ina226_show_interval, ina226_set_interval, 0);
> -
>   /* pointers to created device attributes */
>   static struct attribute *ina2xx_attrs[] = {
>   	&sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -443,6 +399,10 @@ static const struct attribute_group ina2xx_group = {
>   	.attrs = ina2xx_attrs,
>   };
>
> +/* update interval (ina226 only) */
> +static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
> +			  ina226_show_interval, ina226_set_interval, 0);
> +

I don't really see the value of moving this code around.
Please no unnecessary changes.

>   static struct attribute *ina226_attrs[] = {
>   	&sensor_dev_attr_update_interval.dev_attr.attr,
>   	NULL,
> @@ -452,65 +412,65 @@ static const struct attribute_group ina226_group = {
>   	.attrs = ina226_attrs,
>   };
>
> +
>   static int ina2xx_probe(struct i2c_client *client,
>   			const struct i2c_device_id *id)
>   {
> -	struct i2c_adapter *adapter = client->adapter;
>   	struct ina2xx_platform_data *pdata;
>   	struct device *dev = &client->dev;
>   	struct ina2xx_data *data;
>   	struct device *hwmon_dev;
> +	struct regmap *regmap;
>   	u32 val;
>   	int ret, group = 0;
>
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
> -		return -ENODEV;
> +	/* Register regmap */
> +	regmap = devm_regmap_init_i2c(client, &INA2XX_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(regmap);
> +	}
>
>   	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>   	if (!data)
>   		return -ENOMEM;
>
> -	if (dev_get_platdata(dev)) {
> -		pdata = dev_get_platdata(dev);
> -		data->rshunt = pdata->shunt_uohms;
> -	} else if (!of_property_read_u32(dev->of_node,
> -					 "shunt-resistor", &val)) {
> -		data->rshunt = val;
> -	} else {
> -		data->rshunt = INA2XX_RSHUNT_DEFAULT;
> -	}
> +	data->regmap = regmap;
>
> -	/* set the device type */
> -	data->kind = id->driver_data;
> -	data->config = &ina2xx_config[data->kind];
> -	data->curr_config = data->config->config_default;
> +	/* Set config according to device type. */
> +	data->config = &ina2xx_config[id->driver_data];
>   	data->client = client;
>
> -	/*
> -	 * Ina226 has a variable update_interval. For ina219 we
> -	 * use a constant value.
> +	/* Check for shunt resistor value.
> +	 * Give precedence to device tree over must-recompile.
>   	 */
> -	if (data->kind == ina226)
> -		ina226_set_update_interval(data);
> -	else
> -		data->update_interval = HZ / INA2XX_CONVERSION_RATE;
> +	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
> +		pdata = dev_get_platdata(dev);
> +		if (pdata)
> +			val = pdata->shunt_uohms;
> +		else
> +			val = INA2XX_RSHUNT_DEFAULT;
> +	}

This changes priority from platform data first to devicetree configuration first.
As such, it is an unrelated change. If needed, split into a separate patch, and
explain the reasoning, please.

>
> -	if (data->rshunt <= 0 ||
> -	    data->rshunt > data->config->calibration_factor)
> +	if (val <= 0 || val > data->config->calibration_factor) {
> +		dev_err(dev, "Invalid shunt resistor value %li", val);
>   		return -ENODEV;
> +	}
> +	data->rshunt = val;
>
> +	/* Write config to chip, and calibrate */

That comment, if needed, would be more appropriate at the top of the called function.

>   	ret = ina2xx_init(data);
> -	if (ret < 0) {
> -		dev_err(dev, "error configuring the device: %d\n", ret);
> -		return -ENODEV;
> +	if (ret) {
> +		dev_err(dev, "error configuring the device.\n");
> +		return ret;

Please explain whuy this code is better than before. To me it looks like
personal preference, and I don't see the improvement.

>   	}
>
> -	mutex_init(&data->update_lock);
> -
> +	/* Set sensor group according to device type. */

Seems to be obvious to me.

>   	data->groups[group++] = &ina2xx_group;
> -	if (data->kind == ina226)
> +	if (ina226 == id->driver_data)

Not likely to accept Yoda programming I am.

>   		data->groups[group++] = &ina226_group;
>
> +	/* register to hwmon */

Isn't that obvious ? I don't see the value of this comment.

>   	hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
>   							   data, data->groups);
>   	if (IS_ERR(hwmon_dev))
> @@ -541,7 +501,6 @@ static struct i2c_driver ina2xx_driver = {
>   };
>
>   module_i2c_driver(ina2xx_driver);
> -

Does this change fix a checkpatch problem ? If so, please submit as separate patch.
If not, please no unnecessary whitespace changes.

>   MODULE_AUTHOR("Lothar Felten <l-felten@ti.com>");
>   MODULE_DESCRIPTION("ina2xx driver");
>   MODULE_LICENSE("GPL");
>

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

Hi Marc,

[auto build test WARNING on hwmon/hwmon-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Marc-Titinger/hwmon-ina2xx-port-to-using-remap-improve-bandwidth/20151024-001809
config: xtensa-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All warnings (new ones prefixed by >>):

   drivers/hwmon/ina2xx.c: In function 'ina2xx_probe':
>> drivers/hwmon/ina2xx.c:456:3: warning: format '%li' expects argument of type 'long int', but argument 3 has type 'u32' [-Wformat=]
      dev_err(dev, "Invalid shunt resistor value %li", val);
      ^

vim +456 drivers/hwmon/ina2xx.c

   440		/* Set config according to device type. */
   441		data->config = &ina2xx_config[id->driver_data];
   442		data->client = client;
   443	
   444		/* Check for shunt resistor value.
   445		 * Give precedence to device tree over must-recompile.
   446		 */
   447		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
   448			pdata = dev_get_platdata(dev);
   449			if (pdata)
   450				val = pdata->shunt_uohms;
   451			else
   452				val = INA2XX_RSHUNT_DEFAULT;
   453		}
   454	
   455		if (val <= 0 || val > data->config->calibration_factor) {
 > 456			dev_err(dev, "Invalid shunt resistor value %li", val);
   457			return -ENODEV;
   458		}
   459		data->rshunt = val;
   460	
   461		/* Write config to chip, and calibrate */
   462		ret = ina2xx_init(data);
   463		if (ret) {
   464			dev_err(dev, "error configuring the device.\n");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 42415 bytes --]

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]

Hi Marc,

[auto build test WARNING on hwmon/hwmon-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Marc-Titinger/hwmon-ina2xx-port-to-using-remap-improve-bandwidth/20151024-001809
config: openrisc-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=openrisc 

All warnings (new ones prefixed by >>):

   drivers/hwmon/ina2xx.c: In function 'ina2xx_probe':
>> drivers/hwmon/ina2xx.c:456:3: warning: format '%li' expects type 'long int', but argument 3 has type 'u32'

vim +456 drivers/hwmon/ina2xx.c

   440		/* Set config according to device type. */
   441		data->config = &ina2xx_config[id->driver_data];
   442		data->client = client;
   443	
   444		/* Check for shunt resistor value.
   445		 * Give precedence to device tree over must-recompile.
   446		 */
   447		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
   448			pdata = dev_get_platdata(dev);
   449			if (pdata)
   450				val = pdata->shunt_uohms;
   451			else
   452				val = INA2XX_RSHUNT_DEFAULT;
   453		}
   454	
   455		if (val <= 0 || val > data->config->calibration_factor) {
 > 456			dev_err(dev, "Invalid shunt resistor value %li", val);
   457			return -ENODEV;
   458		}
   459		data->rshunt = val;
   460	
   461		/* Write config to chip, and calibrate */
   462		ret = ina2xx_init(data);
   463		if (ret) {
   464			dev_err(dev, "error configuring the device.\n");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 35395 bytes --]

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Marc Titinger]

Hi Guenter

thanks for the review, answers bellow.

Marc.

Le 23/10/2015 18:52, Guenter Roeck a écrit :
> On 10/23/2015 09:13 AM, Marc Titinger wrote:
>> With the current implementation, the driver will prevent a readout at a
>> pace faster than the default conversion time (2ms) times the averaging
>> setting, min AVG being 1:1.
>>
>> Any sysfs "show" read access from the client app faster than 500 Hz 
>> will be
>
> 500 Hz ?
The current code checks for the elapsed time since the last read. If 
this time is less than the default Conversion Time (CT = 2.2ms) times 
the averaging setting, then the driver will not actually access the 
registers.
Hence the user can at best read fresh values every 2.2 x 1 ms, roughly 
500Hz.

Note that since the driver does bulk-read of all 8 registers (on 
ina226), it takes about 0.8 x 8 = 6.4 ms to update all/any values on 
BeagleBoneBlack, hence a client app cannot read fresh values faster than 
160Hz per seconds.
This is the problem that motivates this rework.

>
>
>> 'cached' by the driver, but actually since do_update reads all 8 
>> registers,
>> the best achievable measurement rate is roughly 8*800 us (for the time
>> spent in i2c-core) i.e. <= 156Hz with Beagle Bone Black.
>>
>> Registers are now accessed individually through the regmap facility.
>> For four measurements the readout rate is now around
>>   (1/(4*800us) = 312 Hz.
>
> I'll need some time to go through the patch in detail Some quick 
> comments below.
>
ok
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>   drivers/hwmon/ina2xx.c | 359 
>> ++++++++++++++++++++++---------------------------
>>   1 file changed, 159 insertions(+), 200 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 4d28150..e7c1aaa 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>> @@ -37,6 +37,7 @@
>>   #include <linux/of.h>
>>   #include <linux/delay.h>
>>   #include <linux/util_macros.h>
>> +#include <linux/regmap.h>
>>
>>   #include <linux/platform_data/ina2xx.h>
>>
>> @@ -48,32 +49,25 @@
>>   #define INA2XX_CURRENT            0x04 /* readonly */
>>   #define INA2XX_CALIBRATION        0x05
>>
>> -/* INA226 register definitions */
>> -#define INA226_MASK_ENABLE        0x06
>> -#define INA226_ALERT_LIMIT        0x07
>> -#define INA226_DIE_ID            0xFF
>> -
>> -/* register count */
>> -#define INA219_REGISTERS        6
>> -#define INA226_REGISTERS        8
>> -
>> -#define INA2XX_MAX_REGISTERS        8
>> +/* CONFIG register fields */
>> +#define INA2XX_AVG_MASK            0x0E00
>> +#define INA2XX_AVG_SHFT            9
>>
>>   /* settings - depend on use case */
>>   #define INA219_CONFIG_DEFAULT        0x399F    /* PGA=8 */
>>   #define INA226_CONFIG_DEFAULT        0x4527    /* averages=16 */
>>
>>   /* worst case is 68.10 ms (~14.6Hz, ina219) */
>> -#define INA2XX_CONVERSION_RATE        15
>>   #define INA2XX_MAX_DELAY        69 /* worst case delay in ms */
>>
>>   #define INA2XX_RSHUNT_DEFAULT        10000
>>
>> -/* bit mask for reading the averaging setting in the configuration 
>> register */
>> -#define INA226_AVG_RD_MASK        0x0E00
>> -
>> -#define INA226_READ_AVG(reg)        (((reg) & INA226_AVG_RD_MASK) >> 9)
>> -#define INA226_SHIFT_AVG(val)        ((val) << 9)
>> +/* Currently only handling common register set */
>> +static const struct regmap_config INA2XX_regmap_config = {
>> +    .reg_bits = 8,
>> +    .val_bits = 16,
>> +    .max_register = INA2XX_CALIBRATION,
>> +};
>>
>>   /* common attrs, ina226 attrs and NULL */
>>   #define INA2XX_MAX_ATTRIBUTE_GROUPS    3
>> @@ -89,7 +83,6 @@ enum ina2xx_ids { ina219, ina226 };
>>   struct ina2xx_config {
>>       u16 config_default;
>>       int calibration_factor;
>> -    int registers;
>>       int shunt_div;
>>       int bus_voltage_shift;
>>       int bus_voltage_lsb;    /* uV */
>> @@ -99,25 +92,16 @@ struct ina2xx_config {
>>   struct ina2xx_data {
>>       struct i2c_client *client;
>>       const struct ina2xx_config *config;
>> -
>> +    struct regmap *regmap;
>>       long rshunt;
>> -    u16 curr_config;
>> -
>> -    struct mutex update_lock;
>> -    bool valid;
>> -    unsigned long last_updated;
>> -    int update_interval; /* in jiffies */
>> -
>> -    int kind;
>> +    int valid;
>
> bool, please. But then valid was supposed to be used to indicate
> if the cache is valid, and should no longer be needed since there
> is no more cache. I'll have to spend more time to understand if and
> why it is still needed and how it is used.

Ah yes thanks, re-reading the initial code, I figure I 'invented' 
another need for this by mistake...
I'm happy to remove it.  Good catch!

>
>>       const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
>> -    u16 regs[INA2XX_MAX_REGISTERS];
>>   };
>>
>>   static const struct ina2xx_config ina2xx_config[] = {
>>       [ina219] = {
>>           .config_default = INA219_CONFIG_DEFAULT,
>>           .calibration_factor = 40960000,
>> -        .registers = INA219_REGISTERS,
>>           .shunt_div = 100,
>>           .bus_voltage_shift = 3,
>>           .bus_voltage_lsb = 4000,
>> @@ -126,7 +110,6 @@ static const struct ina2xx_config ina2xx_config[] 
>> = {
>>       [ina226] = {
>>           .config_default = INA226_CONFIG_DEFAULT,
>>           .calibration_factor = 5120000,
>> -        .registers = INA226_REGISTERS,
>>           .shunt_div = 400,
>>           .bus_voltage_shift = 0,
>>           .bus_voltage_lsb = 1250,
>> @@ -142,9 +125,9 @@ static const struct ina2xx_config ina2xx_config[] 
>> = {
>>    */
>>   static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 
>> 1024 };
>>
>> -static int ina226_reg_to_interval(u16 config)
>> +static int ina226_field_to_interval(int field)
>>   {
>> -    int avg = ina226_avg_tab[INA226_READ_AVG(config)];
>> +    int avg = ina226_avg_tab[field];
>>
>>       /*
>>        * Multiply the total conversion time by the number of averages.
>> @@ -153,24 +136,11 @@ static int ina226_reg_to_interval(u16 config)
>>       return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 
>> 1000);
>>   }
>>
>> -static u16 ina226_interval_to_reg(int interval, u16 config)
>> -{
>> -    int avg, avg_bits;
>> -
>> -    avg = DIV_ROUND_CLOSEST(interval * 1000,
>> -                INA226_TOTAL_CONV_TIME_DEFAULT);
>> -    avg_bits = find_closest(avg, ina226_avg_tab,
>> -                ARRAY_SIZE(ina226_avg_tab));
>> -
>> -    return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
>> -}
>> -
>> -static void ina226_set_update_interval(struct ina2xx_data *data)
>> +static int ina226_interval_to_field(int interval)
>>   {
>> -    int ms;
>> -
>> -    ms = ina226_reg_to_interval(data->curr_config);
>> -    data->update_interval = msecs_to_jiffies(ms);
>> +    int avg = DIV_ROUND_CLOSEST(interval * 1000,
>> +                    INA226_TOTAL_CONV_TIME_DEFAULT);
>> +    return find_closest(avg, ina226_avg_tab, 
>> ARRAY_SIZE(ina226_avg_tab));
>>   }
>>
>>   static int ina2xx_calibrate(struct ina2xx_data *data)
>> @@ -178,8 +148,7 @@ static int ina2xx_calibrate(struct ina2xx_data 
>> *data)
>>       u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
>>                       data->rshunt);
>>
>> -    return i2c_smbus_write_word_swapped(data->client,
>> -                        INA2XX_CALIBRATION, val);
>> +    return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
>>   }
>>
>>   /*
>> @@ -187,15 +156,10 @@ static int ina2xx_calibrate(struct ina2xx_data 
>> *data)
>>    */
>>   static int ina2xx_init(struct ina2xx_data *data)
>>   {
>> -    struct i2c_client *client = data->client;
>> -    int ret;
>> -
>> -    /* device configuration */
>> -    ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
>> -                       data->curr_config);
>> -    if (ret < 0)
>> +    int ret = regmap_write(data->regmap, INA2XX_CONFIG,
>> +                   data->config->config_default);
>> +    if (ret)
>>           return ret;
>> -
>>       /*
>>        * Set current LSB to 1mA, shunt is in uOhms
>>        * (equation 13 in datasheet).
>> @@ -203,22 +167,22 @@ static int ina2xx_init(struct ina2xx_data *data)
>>       return ina2xx_calibrate(data);
>>   }
>>
>> -static int ina2xx_do_update(struct device *dev)
>> +static int ina2xx_show_common(struct device *dev, int index, int *val)
>>   {
>>       struct ina2xx_data *data = dev_get_drvdata(dev);
>> -    struct i2c_client *client = data->client;
>> -    int i, rv, retry;
>> +    int err, retry;
>>
>> -    dev_dbg(&client->dev, "Starting ina2xx update\n");
>> +    if (unlikely(!val))
>> +        return -EINVAL;
>
> Please no unnecessary parameter checks. If val is NULL here,
> something is wrong with the implementation and needs to get fixed.
>
> Besides, the return value is only negative on errors. There is no need
> to pass an extra pointer to the value. Furthermore, regmap_read expects
> a pointer to an unsigned int, not a pointer to an int.
>
> Just return an error or the value returned by regmap. The calling code
> can determine if there is an error by checking if the returned value 
> is < 0.
>
>>
>>       for (retry = 5; retry; retry--) {
>> -        /* Read all registers */
>> -        for (i = 0; i < data->config->registers; i++) {
>> -            rv = i2c_smbus_read_word_swapped(client, i);
>> -            if (rv < 0)
>> -                return rv;
>> -            data->regs[i] = rv;
>> -        }
>> +
>> +        /* Check for remaining registers in mask. */
>> +        err = regmap_read(data->regmap, index, val);
>> +        if (err)
>> +            return err;
>> +
>> +        dev_dbg(dev, "read %d, val = 0x%04x\n", index, *val);
>>
>>           /*
>>            * If the current value in the calibration register is 0, the
>> @@ -226,24 +190,24 @@ static int ina2xx_do_update(struct device *dev)
>>            * the chip has been reset let's check the calibration
>>            * register and reinitialize if needed.
>>            */
>> -        if (data->regs[INA2XX_CALIBRATION] == 0) {
>> -            dev_warn(dev, "chip not calibrated, reinitializing\n");
>> -
>> -            rv = ina2xx_init(data);
>> -            if (rv < 0)
>> -                return rv;
>> +        if (!data->valid) {
>> +            dev_warn(dev,
>> +                 "chip needs calibration, reinitializing\n");
>>
>> +            err = ina2xx_calibrate(data);
>> +            if (err)
>> +                return err;
>
> Ah, that is the context of "valid". I don't immediately see why moving 
> its scope
> out of this function, and setting it to false in the calling code, is 
> beneficial here.
>
> This also changes the dynamic of the driver. Previously it was 
> specifically able
> to detect if the chip was power cycled (for whatever reason) and it 
> would re-initialize
> itself. Now you use 'valid' and you defer (re-)initialization until 
> chip registers
> are read. I don't immediately see if and how this is beneficial.
Agreed, as I said, it "morphed" the use of valid because of my earlier 
patch...

>>               /*
>>                * Let's make sure the power and current registers
>>                * have been updated before trying again.
>>                */
>>               msleep(INA2XX_MAX_DELAY);
>> +
>> +            /* data valid once we have a cal value. */
>> +            regmap_read(data->regmap, INA2XX_CALIBRATION,
>> +                    &data->valid);
>>               continue;
>>           }
>> -
>> -        data->last_updated = jiffies;
>> -        data->valid = 1;
>> -
>>           return 0;
>>       }
>>
>> @@ -256,86 +220,89 @@ static int ina2xx_do_update(struct device *dev)
>>       return -ENODEV;
>>   }
>>
>> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
>> +static ssize_t ina2xx_show_shunt(struct device *dev,
>> +                 struct device_attribute *da, char *buf)
>>   {
>>       struct ina2xx_data *data = dev_get_drvdata(dev);
>> -    struct ina2xx_data *ret = data;
>> -    unsigned long after;
>> -    int rv;
>> +    int val, err;
>>
>> -    mutex_lock(&data->update_lock);
>> +    err = ina2xx_show_common(dev, INA2XX_SHUNT_VOLTAGE, &val);
>> +    if (err)
>> +        return err;
>>
>> -    after = data->last_updated + data->update_interval;
>> -    if (time_after(jiffies, after) || !data->valid) {
>> -        rv = ina2xx_do_update(dev);
>> -        if (rv < 0)
>> -            ret = ERR_PTR(rv);
>> -    }
>> +    val = DIV_ROUND_CLOSEST((s16) val, data->config->shunt_div);
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> +}
>> +
>> +static ssize_t ina2xx_show_bus(struct device *dev,
>> +                   struct device_attribute *da, char *buf)
>> +{
>> +    struct ina2xx_data *data = dev_get_drvdata(dev);
>> +    int val, err;
>> +
>> +    err = ina2xx_show_common(dev, INA2XX_BUS_VOLTAGE, &val);
>> +    if (err)
>> +        return err;
>>
>> -    mutex_unlock(&data->update_lock);
>> -    return ret;
>> +    val = (val >> data->config->bus_voltage_shift)
>> +        * data->config->bus_voltage_lsb;
>> +    val = DIV_ROUND_CLOSEST(val, 1000);
>> +
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>   }
>>
>> -static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
>> +static ssize_t ina2xx_show_pow(struct device *dev,
>> +                   struct device_attribute *da, char *buf)
>>   {
>> -    int val;
>> -
>> -    switch (reg) {
>> -    case INA2XX_SHUNT_VOLTAGE:
>> -        /* signed register */
>> -        val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
>> -                    data->config->shunt_div);
>> -        break;
>> -    case INA2XX_BUS_VOLTAGE:
>> -        val = (data->regs[reg] >> data->config->bus_voltage_shift)
>> -          * data->config->bus_voltage_lsb;
>> -        val = DIV_ROUND_CLOSEST(val, 1000);
>> -        break;
>> -    case INA2XX_POWER:
>> -        val = data->regs[reg] * data->config->power_lsb;
>> -        break;
>> -    case INA2XX_CURRENT:
>> -        /* signed register, LSB=1mA (selected), in mA */
>> -        val = (s16)data->regs[reg];
>> -        break;
>> -    case INA2XX_CALIBRATION:
>> -        val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
>> -                    data->regs[reg]);
>> -        break;
>> -    default:
>> -        /* programmer goofed */
>> -        WARN_ON_ONCE(1);
>> -        val = 0;
>> -        break;
>> -    }
>> +    struct ina2xx_data *data = dev_get_drvdata(dev);
>> +    int val, err;
>> +
>> +    err = ina2xx_show_common(dev, INA2XX_POWER, &val);
>> +    if (err)
>> +        return err;
>> +
>> +    val *= data->config->power_lsb;
>>
>> -    return val;
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>   }
>>
>> -static ssize_t ina2xx_show_value(struct device *dev,
>> -                 struct device_attribute *da, char *buf)
>> +static ssize_t ina2xx_show_curr(struct device *dev,
>> +                struct device_attribute *da, char *buf)
>>   {
>> -    struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> -    struct ina2xx_data *data = ina2xx_update_device(dev);
>> +    int val, err;
>>
>> -    if (IS_ERR(data))
>> -        return PTR_ERR(data);
>> +    err = ina2xx_show_common(dev, INA2XX_CURRENT, &val);
>> +    if (err)
>> +        return err;
>>
>> -    return snprintf(buf, PAGE_SIZE, "%d\n",
>> -            ina2xx_get_value(data, attr->index));
>> +    /* signed register, LSB=1mA (selected), in mA */
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>> +}
>> +
>> +static ssize_t ina2xx_show_cal(struct device *dev,
>> +                   struct device_attribute *da, char *buf)
>> +{
>> +    struct ina2xx_data *data = dev_get_drvdata(dev);
>> +    int val, err;
>> +
>> +    err = ina2xx_show_common(dev, INA2XX_CALIBRATION, &val);
>> +    if (err)
>> +        return err;
>> +
>> +    val = DIV_ROUND_CLOSEST(data->config->calibration_factor, val);
>> +
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>   }
>>
>>   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);
>> +    struct ina2xx_data *data = dev_get_drvdata(dev);
>> +
>>       unsigned long val;
>>       int status;
>>
>> -    if (IS_ERR(data))
>> -        return PTR_ERR(data);
>> -
>>       status = kstrtoul(buf, 10, &val);
>>       if (status < 0)
>>           return status;
>> @@ -345,12 +312,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>>           val > data->config->calibration_factor)
>>           return -EINVAL;
>>
>> -    mutex_lock(&data->update_lock);
>>       data->rshunt = val;
>> -    status = ina2xx_calibrate(data);
>> -    mutex_unlock(&data->update_lock);
>> -    if (status < 0)
>> -        return status;
>> +    data->valid = 0;
>>
>>       return count;
>>   }
>> @@ -370,65 +333,58 @@ static ssize_t ina226_set_interval(struct 
>> device *dev,
>>       if (val > INT_MAX || val == 0)
>>           return -EINVAL;
>>
>> -    mutex_lock(&data->update_lock);
>> -    data->curr_config = ina226_interval_to_reg(val,
>> -                           data->regs[INA2XX_CONFIG]);
>> -    status = i2c_smbus_write_word_swapped(data->client,
>> -                          INA2XX_CONFIG,
>> -                          data->curr_config);
>> +    val = ina226_interval_to_field(val);
>>
>> -    ina226_set_update_interval(data);
>> -    /* Make sure the next access re-reads all registers. */
>> -    data->valid = 0;
>> -    mutex_unlock(&data->update_lock);
>> +    status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
>> +                    INA2XX_AVG_MASK, (val << INA2XX_AVG_SHFT));
>>       if (status < 0)
>>           return status;
>>
>> +    data->valid = 0;
>> +
>
> This now causes re-calibration. Why ?
Mistake.
>>       return count;
>>   }
>>
>>   static ssize_t ina226_show_interval(struct device *dev,
>>                       struct device_attribute *da, char *buf)
>>   {
>> -    struct ina2xx_data *data = ina2xx_update_device(dev);
>> +    struct ina2xx_data *data = dev_get_drvdata(dev);
>> +    int status, val;
>>
>> -    if (IS_ERR(data))
>> -        return PTR_ERR(data);
>> +    status = regmap_read(data->regmap, INA2XX_CONFIG, &val);
>> +    if (status)
>> +        return status;
>> +
>> +    val = (val & INA2XX_AVG_MASK) >> INA2XX_AVG_SHFT;
>> +    val = ina226_field_to_interval(val);
>>
>>       /*
>> -     * We don't use data->update_interval here as we want to display
>> -     * the actual interval used by the chip and jiffies_to_msecs()
>> -     * doesn't seem to be accurate enough.
>> +     * We want to display the actual interval used by the chip.
>>        */
>> -    return snprintf(buf, PAGE_SIZE, "%d\n",
>> - ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
>> +    return snprintf(buf, PAGE_SIZE, "%d\n", val);
>>   }
>>
>>   /* shunt voltage */
>> -static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_shunt, NULL,
>>                 INA2XX_SHUNT_VOLTAGE);
>
> If you no longer use the register parameter, there is no need to 
> provide it.
> But then I'll have to spend some time trying to understand _why_ you 
> don't
> use it anymore and why you introduced separate show functions.
> Some explanation might help.
>
The interval value is no longer needed to compute a read "gard" delay, 
but the client Application may still need to set a different averaging 
value through this interval setting, based on the DUT characteristics.
As of using separate functions, it fits better in the new logic of 
"per-register" accesses. It does not bloat the code either.

>>
>>   /* bus voltage */
>> -static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_value, NULL,
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ina2xx_show_bus, NULL,
>>                 INA2XX_BUS_VOLTAGE);
>>
>>   /* calculated current */
>> -static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_value, 
>> NULL,
>> +static SENSOR_DEVICE_ATTR(curr1_input, S_IRUGO, ina2xx_show_curr, NULL,
>>                 INA2XX_CURRENT);
>>
>>   /* calculated power */
>> -static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, 
>> NULL,
>> +static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_pow, NULL,
>>                 INA2XX_POWER);
>>
>>   /* shunt resistance */
>>   static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR,
>> -              ina2xx_show_value, ina2xx_set_shunt,
>> +              ina2xx_show_cal, ina2xx_set_shunt,
>>                 INA2XX_CALIBRATION);
>>
>> -/* update interval (ina226 only) */
>> -static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
>> -              ina226_show_interval, ina226_set_interval, 0);
>> -
>>   /* pointers to created device attributes */
>>   static struct attribute *ina2xx_attrs[] = {
>>       &sensor_dev_attr_in0_input.dev_attr.attr,
>> @@ -443,6 +399,10 @@ static const struct attribute_group ina2xx_group 
>> = {
>>       .attrs = ina2xx_attrs,
>>   };
>>
>> +/* update interval (ina226 only) */
>> +static SENSOR_DEVICE_ATTR(update_interval, S_IRUGO | S_IWUSR,
>> +              ina226_show_interval, ina226_set_interval, 0);
>> +
>
> I don't really see the value of moving this code around.
> Please no unnecessary changes.
>
>>   static struct attribute *ina226_attrs[] = {
>>       &sensor_dev_attr_update_interval.dev_attr.attr,
>>       NULL,
>> @@ -452,65 +412,65 @@ static const struct attribute_group 
>> ina226_group = {
>>       .attrs = ina226_attrs,
>>   };
>>
>> +
>>   static int ina2xx_probe(struct i2c_client *client,
>>               const struct i2c_device_id *id)
>>   {
>> -    struct i2c_adapter *adapter = client->adapter;
>>       struct ina2xx_platform_data *pdata;
>>       struct device *dev = &client->dev;
>>       struct ina2xx_data *data;
>>       struct device *hwmon_dev;
>> +    struct regmap *regmap;
>>       u32 val;
>>       int ret, group = 0;
>>
>> -    if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
>> -        return -ENODEV;
>> +    /* Register regmap */
>> +    regmap = devm_regmap_init_i2c(client, &INA2XX_regmap_config);
>> +    if (IS_ERR(regmap)) {
>> +        dev_err(dev, "failed to allocate register map\n");
>> +        return PTR_ERR(regmap);
>> +    }
>>
>>       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>       if (!data)
>>           return -ENOMEM;
>>
>> -    if (dev_get_platdata(dev)) {
>> -        pdata = dev_get_platdata(dev);
>> -        data->rshunt = pdata->shunt_uohms;
>> -    } else if (!of_property_read_u32(dev->of_node,
>> -                     "shunt-resistor", &val)) {
>> -        data->rshunt = val;
>> -    } else {
>> -        data->rshunt = INA2XX_RSHUNT_DEFAULT;
>> -    }
>> +    data->regmap = regmap;
>>
>> -    /* set the device type */
>> -    data->kind = id->driver_data;
>> -    data->config = &ina2xx_config[data->kind];
>> -    data->curr_config = data->config->config_default;
>> +    /* Set config according to device type. */
>> +    data->config = &ina2xx_config[id->driver_data];
>>       data->client = client;
>>
>> -    /*
>> -     * Ina226 has a variable update_interval. For ina219 we
>> -     * use a constant value.
>> +    /* Check for shunt resistor value.
>> +     * Give precedence to device tree over must-recompile.
>>        */
>> -    if (data->kind == ina226)
>> -        ina226_set_update_interval(data);
>> -    else
>> -        data->update_interval = HZ / INA2XX_CONVERSION_RATE;
>> +    if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 
>> 0) {
>> +        pdata = dev_get_platdata(dev);
>> +        if (pdata)
>> +            val = pdata->shunt_uohms;
>> +        else
>> +            val = INA2XX_RSHUNT_DEFAULT;
>> +    }
>
> This changes priority from platform data first to devicetree 
> configuration first.
> As such, it is an unrelated change. If needed, split into a separate 
> patch, and
Yes I would do a separate patch normaly, agreed.

> explain the reasoning, please.
Changing the platform data requires changes in the kernel code, and 
hence recompilation. It seems a bit unexpected that setting a new value 
in the dtb will be ignored because there is a compiled-in platform data. 
Should'nt the dtb allow to override platform data ?
>
>>
>> -    if (data->rshunt <= 0 ||
>> -        data->rshunt > data->config->calibration_factor)
>> +    if (val <= 0 || val > data->config->calibration_factor) {
>> +        dev_err(dev, "Invalid shunt resistor value %li", val);
>>           return -ENODEV;
>> +    }
>> +    data->rshunt = val;
>>
>> +    /* Write config to chip, and calibrate */
>
> That comment, if needed, would be more appropriate at the top of the 
> called function.
>
>>       ret = ina2xx_init(data);
>> -    if (ret < 0) {
>> -        dev_err(dev, "error configuring the device: %d\n", ret);
>> -        return -ENODEV;
>> +    if (ret) {
>> +        dev_err(dev, "error configuring the device.\n");
>> +        return ret;
>
> Please explain whuy this code is better than before. To me it looks like
> personal preference, and I don't see the improvement.
>
It's not, it came from successive changes, will revert...
>>       }
>>
>> -    mutex_init(&data->update_lock);
>> -
>> +    /* Set sensor group according to device type. */
>
> Seems to be obvious to me.
>
>>       data->groups[group++] = &ina2xx_group;
>> -    if (data->kind == ina226)
>> +    if (ina226 == id->driver_data)
>
> Not likely to accept Yoda programming I am.
>
Appropriate it is, In some instances. Usually I dislike it too, question 
of style and opinion really I'd say.

>>           data->groups[group++] = &ina226_group;
>>
>> +    /* register to hwmon */
>
> Isn't that obvious ? I don't see the value of this comment.
>
>>       hwmon_dev = devm_hwmon_device_register_with_groups(dev, 
>> client->name,
>>                                  data, data->groups);
>>       if (IS_ERR(hwmon_dev))
>> @@ -541,7 +501,6 @@ static struct i2c_driver ina2xx_driver = {
>>   };
>>
>>   module_i2c_driver(ina2xx_driver);
>> -
>
> Does this change fix a checkpatch problem ? If so, please submit as 
> separate patch.
> If not, please no unnecessary whitespace changes.
Ok.
>
>>   MODULE_AUTHOR("Lothar Felten <l-felten@ti.com>");
>>   MODULE_DESCRIPTION("ina2xx driver");
>>   MODULE_LICENSE("GPL");
>>
>

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Guenter Roeck]

On 10/23/2015 01:35 PM, Marc Titinger wrote:
> Hi Guenter
>
> thanks for the review, answers bellow.
>
> Marc.
>

[ ... ]

>>> -    /*
>>> -     * Ina226 has a variable update_interval. For ina219 we
>>> -     * use a constant value.
>>> +    /* Check for shunt resistor value.

Another comment: Standard multi-line comments, please.

>>> +     * Give precedence to device tree over must-recompile.
>>>        */
>>> -    if (data->kind == ina226)
>>> -        ina226_set_update_interval(data);
>>> -    else
>>> -        data->update_interval = HZ / INA2XX_CONVERSION_RATE;
>>> +    if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
>>> +        pdata = dev_get_platdata(dev);
>>> +        if (pdata)
>>> +            val = pdata->shunt_uohms;
>>> +        else
>>> +            val = INA2XX_RSHUNT_DEFAULT;
>>> +    }
>>
>> This changes priority from platform data first to devicetree configuration first.
>> As such, it is an unrelated change. If needed, split into a separate patch, and
> Yes I would do a separate patch normaly, agreed.
>
>> explain the reasoning, please.
> Changing the platform data requires changes in the kernel code, and hence recompilation. It seems a bit unexpected that setting a new value in the dtb will be ignored because there is a compiled-in platform data. Should'nt the dtb allow to override platform data ?

Normally you would not _have_ platform data in a system which is dtb enabled.
I don't really mind changing priorities (you are right, it makes sense to
check for devicetree data first), but as mentioned as separate patch, please.

Thanks,
Guenter

Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Guenter Roeck]

On 10/23/2015 01:35 PM, Marc Titinger wrote:
> Hi Guenter
>
> thanks for the review, answers bellow.
>
[ ... ]

>>>   /* shunt voltage */
>>> -static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_value, NULL,
>>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ina2xx_show_shunt, NULL,
>>>                 INA2XX_SHUNT_VOLTAGE);
>>
>> If you no longer use the register parameter, there is no need to provide it.
>> But then I'll have to spend some time trying to understand _why_ you don't
>> use it anymore and why you introduced separate show functions.
>> Some explanation might help.
>>
> The interval value is no longer needed to compute a read "gard" delay, but the client Application may still need to set a different averaging value through this interval setting, based on the DUT characteristics.

I don't really understand what you are saying here, sorry, and what it has
to do with the switch from a single to multiple show functions.

> As of using separate functions, it fits better in the new logic of "per-register" accesses. It does not bloat the code either.
>
"It does not bloat the code" is not a valid reason for making such a change,
nor "it fits better" (because that is an opinion). Reducing code size,
or simplifying the code, could be valid reasons.
The change is also technically unrelated to the switch to regmap.
Also, while it may not bloat the code, it definitely bloats the patch size
and makes it much more difficult to review the patch.

Please make only one logical change per patch. So far, we have identified
three independent sets of changes:
- switch to regmap
- switch to separate show functions
- switch to prioritize devicetree data over platform data

and I am not sure if there are more, given the complexity of the patch.

I would expect separate patches to address those changes. Please split
the patch accordingly.

Thanks,
Guenter

[PATCH 0/2] hwmon: ina2xx: convert driver to using regmap

[Marc Titinger]

Hi Guenter,

as requested, here's the regmap version as a separate patch. I've left out
the idea of separate show functions as it doesn't bring much indeed.

If you're interested in a before/after comparison, here are the plots of
of a usb key (DUT) power-cycle captured using either driver version.
As expected, the measurement is better resolved with single reads.

"github dot com"/mtitinger/ACME/blob/master/docs/measurements/bulk-700pts.png
"github dot com"/mtitinger/ACME/blob/master/docs/measurements/regmap-700pts.png

Many thanks.

Marc Titinger (2):
  hwmon: ina2xx: convert driver to using regmap
  hwmon: ina2xx: give precedence to DT over checking for platform data.

 drivers/hwmon/ina2xx.c | 208 +++++++++++++++++++------------------------------
 1 file changed, 79 insertions(+), 129 deletions(-)

-- 
1.9.1

[PATCH 1/2] hwmon: ina2xx: convert driver to using regmap

[Marc Titinger]

Any sysfs "show" read access from the client app will result in reading
all registers (8 with ina226). Depending on the host this can limit the
best achievable read rate.

This changeset allows for individual register accesses through regmap.

Tested with BeagleBone Black (Baylibre-ACME) and ina226.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 187 ++++++++++++++++++-------------------------------
 1 file changed, 69 insertions(+), 118 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..3edd163 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/util_macros.h>
+#include <linux/regmap.h>
 
 #include <linux/platform_data/ina2xx.h>
 
@@ -84,6 +85,11 @@
  */
 #define INA226_TOTAL_CONV_TIME_DEFAULT	2200
 
+static struct regmap_config ina2xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -101,16 +107,10 @@ struct ina2xx_data {
 	const struct ina2xx_config *config;
 
 	long rshunt;
-	u16 curr_config;
-
-	struct mutex update_lock;
-	bool valid;
-	unsigned long last_updated;
-	int update_interval; /* in jiffies */
+	struct regmap *regmap;
 
 	int kind;
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
-	u16 regs[INA2XX_MAX_REGISTERS];
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -153,7 +153,11 @@ static int ina226_reg_to_interval(u16 config)
 	return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 1000);
 }
 
-static u16 ina226_interval_to_reg(int interval, u16 config)
+/*
+ * Return the new, shifted AVG field value of CONFIG register,
+ * to use with regmap_update_bits
+ */
+static u16 ina226_interval_to_reg(int interval)
 {
 	int avg, avg_bits;
 
@@ -162,15 +166,7 @@ static u16 ina226_interval_to_reg(int interval, u16 config)
 	avg_bits = find_closest(avg, ina226_avg_tab,
 				ARRAY_SIZE(ina226_avg_tab));
 
-	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
-}
-
-static void ina226_set_update_interval(struct ina2xx_data *data)
-{
-	int ms;
-
-	ms = ina226_reg_to_interval(data->curr_config);
-	data->update_interval = msecs_to_jiffies(ms);
+	return INA226_SHIFT_AVG(avg_bits);
 }
 
 static int ina2xx_calibrate(struct ina2xx_data *data)
@@ -187,12 +183,8 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
  */
 static int ina2xx_init(struct ina2xx_data *data)
 {
-	struct i2c_client *client = data->client;
-	int ret;
-
-	/* device configuration */
-	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-					   data->curr_config);
+	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
+			       data->config->config_default);
 	if (ret < 0)
 		return ret;
 
@@ -203,35 +195,40 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+static int ina2xx_do_update(struct device *dev, int reg, unsigned int *rv)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int i, rv, retry;
+	int ret, retry;
 
-	dev_dbg(&client->dev, "Starting ina2xx update\n");
+	dev_dbg(dev, "Starting ina2xx update\n");
 
 	for (retry = 5; retry; retry--) {
-		/* Read all registers */
-		for (i = 0; i < data->config->registers; i++) {
-			rv = i2c_smbus_read_word_swapped(client, i);
-			if (rv < 0)
-				return rv;
-			data->regs[i] = rv;
-		}
+
+		ret = regmap_read(data->regmap, reg, rv);
+		if (ret < 0)
+			return ret;
+
+		dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *rv);
 
 		/*
 		 * If the current value in the calibration register is 0, the
 		 * power and current registers will also remain at 0. In case
 		 * the chip has been reset let's check the calibration
 		 * register and reinitialize if needed.
+		 * We do that extra read of the calibration register if there
+		 * is some hint of a chip reset.
 		 */
-		if (data->regs[INA2XX_CALIBRATION] == 0) {
-			dev_warn(dev, "chip not calibrated, reinitializing\n");
+		if (*rv == 0) {
+			unsigned int cal;
+
+			regmap_read(data->regmap, INA2XX_CALIBRATION, &cal);
+
+			if (cal == 0) {
+				dev_warn(dev, "chip not calibrated, reinitializing\n");
 
-			rv = ina2xx_init(data);
-			if (rv < 0)
-				return rv;
+				ret = ina2xx_init(data);
+				if (ret < 0)
+					return ret;
 
 			/*
 			 * Let's make sure the power and current registers
@@ -239,11 +236,8 @@ static int ina2xx_do_update(struct device *dev)
 			 */
 			msleep(INA2XX_MAX_DELAY);
 			continue;
+			}
 		}
-
-		data->last_updated = jiffies;
-		data->valid = 1;
-
 		return 0;
 	}
 
@@ -256,51 +250,29 @@ static int ina2xx_do_update(struct device *dev)
 	return -ENODEV;
 }
 
-static struct ina2xx_data *ina2xx_update_device(struct device *dev)
-{
-	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct ina2xx_data *ret = data;
-	unsigned long after;
-	int rv;
-
-	mutex_lock(&data->update_lock);
-
-	after = data->last_updated + data->update_interval;
-	if (time_after(jiffies, after) || !data->valid) {
-		rv = ina2xx_do_update(dev);
-		if (rv < 0)
-			ret = ERR_PTR(rv);
-	}
-
-	mutex_unlock(&data->update_lock);
-	return ret;
-}
-
-static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
+static int ina2xx_get_value(struct ina2xx_data *data, u8 reg, unsigned int rv)
 {
 	int val;
 
 	switch (reg) {
 	case INA2XX_SHUNT_VOLTAGE:
 		/* signed register */
-		val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
-					data->config->shunt_div);
+		val = DIV_ROUND_CLOSEST((s16)rv, data->config->shunt_div);
 		break;
 	case INA2XX_BUS_VOLTAGE:
-		val = (data->regs[reg] >> data->config->bus_voltage_shift)
+		val = (rv >> data->config->bus_voltage_shift)
 		  * data->config->bus_voltage_lsb;
 		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_POWER:
-		val = data->regs[reg] * data->config->power_lsb;
+		val = rv * data->config->power_lsb;
 		break;
 	case INA2XX_CURRENT:
 		/* signed register, LSB=1mA (selected), in mA */
-		val = (s16)data->regs[reg];
+		val = (s16)rv;
 		break;
 	case INA2XX_CALIBRATION:
-		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
-					data->regs[reg]);
+		val = DIV_ROUND_CLOSEST(data->config->calibration_factor, rv);
 		break;
 	default:
 		/* programmer goofed */
@@ -316,25 +288,25 @@ static ssize_t ina2xx_show_value(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	unsigned int rv;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	int err = ina2xx_do_update(dev, attr->index, &rv);
+
+	if (err < 0)
+		return err;
 
 	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina2xx_get_value(data, attr->index));
+			ina2xx_get_value(data, attr->index, rv));
 }
 
 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);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
 
 	status = kstrtoul(buf, 10, &val);
 	if (status < 0)
@@ -345,10 +317,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	    val > data->config->calibration_factor)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
 	data->rshunt = val;
 	status = ina2xx_calibrate(data);
-	mutex_unlock(&data->update_lock);
 	if (status < 0)
 		return status;
 
@@ -370,17 +340,9 @@ static ssize_t ina226_set_interval(struct device *dev,
 	if (val > INT_MAX || val == 0)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->curr_config = ina226_interval_to_reg(val,
-						   data->regs[INA2XX_CONFIG]);
-	status = i2c_smbus_write_word_swapped(data->client,
-					      INA2XX_CONFIG,
-					      data->curr_config);
-
-	ina226_set_update_interval(data);
-	/* Make sure the next access re-reads all registers. */
-	data->valid = 0;
-	mutex_unlock(&data->update_lock);
+	status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
+				    INA226_AVG_RD_MASK,
+				    ina226_interval_to_reg(val));
 	if (status < 0)
 		return status;
 
@@ -390,18 +352,15 @@ static ssize_t ina226_set_interval(struct device *dev,
 static ssize_t ina226_show_interval(struct device *dev,
 				    struct device_attribute *da, char *buf)
 {
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int status;
+	unsigned int rv;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	status = regmap_read(data->regmap, INA2XX_CONFIG, &rv);
+	if (status)
+		return status;
 
-	/*
-	 * We don't use data->update_interval here as we want to display
-	 * the actual interval used by the chip and jiffies_to_msecs()
-	 * doesn't seem to be accurate enough.
-	 */
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
+	return snprintf(buf, PAGE_SIZE, "%d\n", ina226_reg_to_interval(rv));
 }
 
 /* shunt voltage */
@@ -455,7 +414,6 @@ static const struct attribute_group ina226_group = {
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct i2c_adapter *adapter = client->adapter;
 	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
@@ -463,9 +421,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	u32 val;
 	int ret, group = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
-		return -ENODEV;
-
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -483,30 +438,26 @@ static int ina2xx_probe(struct i2c_client *client,
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
-	data->curr_config = data->config->config_default;
 	data->client = client;
 
-	/*
-	 * Ina226 has a variable update_interval. For ina219 we
-	 * use a constant value.
-	 */
-	if (data->kind == ina226)
-		ina226_set_update_interval(data);
-	else
-		data->update_interval = HZ / INA2XX_CONVERSION_RATE;
-
 	if (data->rshunt <= 0 ||
 	    data->rshunt > data->config->calibration_factor)
 		return -ENODEV;
 
+	ina2xx_regmap_config.max_register = data->config->registers;
+
+	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
 	ret = ina2xx_init(data);
 	if (ret < 0) {
 		dev_err(dev, "error configuring the device: %d\n", ret);
 		return -ENODEV;
 	}
 
-	mutex_init(&data->update_lock);
-
 	data->groups[group++] = &ina2xx_group;
 	if (data->kind == ina226)
 		data->groups[group++] = &ina226_group;
-- 
1.9.1

[PATCH 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data.

[Marc Titinger]

when checking for the value of the shunt resistor.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---
 drivers/hwmon/ina2xx.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 3edd163..1946433 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -414,7 +414,6 @@ static const struct attribute_group ina226_group = {
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
@@ -425,14 +424,13 @@ static int ina2xx_probe(struct i2c_client *client,
 	if (!data)
 		return -ENOMEM;
 
-	if (dev_get_platdata(dev)) {
-		pdata = dev_get_platdata(dev);
-		data->rshunt = pdata->shunt_uohms;
-	} else if (!of_property_read_u32(dev->of_node,
-					 "shunt-resistor", &val)) {
-		data->rshunt = val;
-	} else {
-		data->rshunt = INA2XX_RSHUNT_DEFAULT;
+	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
+		struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
+
+		if (pdata)
+			val = pdata->shunt_uohms;
+		else
+			val = INA2XX_RSHUNT_DEFAULT;
 	}
 
 	/* set the device type */
@@ -440,10 +438,11 @@ static int ina2xx_probe(struct i2c_client *client,
 	data->config = &ina2xx_config[data->kind];
 	data->client = client;
 
-	if (data->rshunt <= 0 ||
-	    data->rshunt > data->config->calibration_factor)
+	if (val <= 0 || val > data->config->calibration_factor)
 		return -ENODEV;
 
+	data->rshunt = val;
+
 	ina2xx_regmap_config.max_register = data->config->registers;
 
 	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
-- 
1.9.1

Re: [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap

[Guenter Roeck]

On Mon, Oct 26, 2015 at 05:24:32PM +0100, Marc Titinger wrote:
> Any sysfs "show" read access from the client app will result in reading
> all registers (8 with ina226). Depending on the host this can limit the
> best achievable read rate.
> 
> This changeset allows for individual register accesses through regmap.
> 
> Tested with BeagleBone Black (Baylibre-ACME) and ina226.
> 
Pretty good. Couple of comments inline.

Thanks,
Guenter

> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/hwmon/ina2xx.c | 187 ++++++++++++++++++-------------------------------
>  1 file changed, 69 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 4d28150..3edd163 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -37,6 +37,7 @@
>  #include <linux/of.h>
>  #include <linux/delay.h>
>  #include <linux/util_macros.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/platform_data/ina2xx.h>
>  
> @@ -84,6 +85,11 @@
>   */
>  #define INA226_TOTAL_CONV_TIME_DEFAULT	2200
>  
> +static struct regmap_config ina2xx_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +};
> +
>  enum ina2xx_ids { ina219, ina226 };
>  
>  struct ina2xx_config {
> @@ -101,16 +107,10 @@ struct ina2xx_data {
>  	const struct ina2xx_config *config;
>  
>  	long rshunt;
> -	u16 curr_config;
> -
> -	struct mutex update_lock;
> -	bool valid;
> -	unsigned long last_updated;
> -	int update_interval; /* in jiffies */
> +	struct regmap *regmap;
>  
>  	int kind;
>  	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
> -	u16 regs[INA2XX_MAX_REGISTERS];
>  };
>  
>  static const struct ina2xx_config ina2xx_config[] = {
> @@ -153,7 +153,11 @@ static int ina226_reg_to_interval(u16 config)
>  	return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 1000);
>  }
>  
> -static u16 ina226_interval_to_reg(int interval, u16 config)
> +/*
> + * Return the new, shifted AVG field value of CONFIG register,
> + * to use with regmap_update_bits
> + */
> +static u16 ina226_interval_to_reg(int interval)
>  {
>  	int avg, avg_bits;
>  
> @@ -162,15 +166,7 @@ static u16 ina226_interval_to_reg(int interval, u16 config)
>  	avg_bits = find_closest(avg, ina226_avg_tab,
>  				ARRAY_SIZE(ina226_avg_tab));
>  
> -	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
> -}
> -
> -static void ina226_set_update_interval(struct ina2xx_data *data)
> -{
> -	int ms;
> -
> -	ms = ina226_reg_to_interval(data->curr_config);
> -	data->update_interval = msecs_to_jiffies(ms);
> +	return INA226_SHIFT_AVG(avg_bits);
>  }
>  
>  static int ina2xx_calibrate(struct ina2xx_data *data)
> @@ -187,12 +183,8 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
>   */
>  static int ina2xx_init(struct ina2xx_data *data)
>  {
> -	struct i2c_client *client = data->client;
> -	int ret;
> -
> -	/* device configuration */
> -	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
> -					   data->curr_config);
> +	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
> +			       data->config->config_default);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -203,35 +195,40 @@ static int ina2xx_init(struct ina2xx_data *data)
>  	return ina2xx_calibrate(data);
>  }
>  
> -static int ina2xx_do_update(struct device *dev)
> +static int ina2xx_do_update(struct device *dev, int reg, unsigned int *rv)

How about ina2xx_read_reg() ? 
do_update() doesn't really describe the function anymore.

>  {
>  	struct ina2xx_data *data = dev_get_drvdata(dev);
> -	struct i2c_client *client = data->client;
> -	int i, rv, retry;
> +	int ret, retry;
>  
> -	dev_dbg(&client->dev, "Starting ina2xx update\n");
> +	dev_dbg(dev, "Starting ina2xx update\n");

	"Starting register 0x%x read\n" ?
>  
>  	for (retry = 5; retry; retry--) {
> -		/* Read all registers */
> -		for (i = 0; i < data->config->registers; i++) {
> -			rv = i2c_smbus_read_word_swapped(client, i);
> -			if (rv < 0)
> -				return rv;
> -			data->regs[i] = rv;
> -		}
> +
> +		ret = regmap_read(data->regmap, reg, rv);
> +		if (ret < 0)
> +			return ret;
> +
> +		dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *rv);
>  
>  		/*
>  		 * If the current value in the calibration register is 0, the
>  		 * power and current registers will also remain at 0. In case
>  		 * the chip has been reset let's check the calibration
>  		 * register and reinitialize if needed.
> +		 * We do that extra read of the calibration register if there
> +		 * is some hint of a chip reset.
>  		 */
> -		if (data->regs[INA2XX_CALIBRATION] == 0) {
> -			dev_warn(dev, "chip not calibrated, reinitializing\n");
> +		if (*rv == 0) {
> +			unsigned int cal;
> +
> +			regmap_read(data->regmap, INA2XX_CALIBRATION, &cal);

This needs an error check.

> +
> +			if (cal == 0) {
> +				dev_warn(dev, "chip not calibrated, reinitializing\n");
>  
> -			rv = ina2xx_init(data);
> -			if (rv < 0)
> -				return rv;
> +				ret = ina2xx_init(data);
> +				if (ret < 0)
> +					return ret;
>  
>  			/*
>  			 * Let's make sure the power and current registers
> @@ -239,11 +236,8 @@ static int ina2xx_do_update(struct device *dev)
>  			 */
>  			msleep(INA2XX_MAX_DELAY);
>  			continue;
> +			}

Indentation is messed up here.

>  		}
> -
> -		data->last_updated = jiffies;
> -		data->valid = 1;
> -
>  		return 0;
>  	}
>  
> @@ -256,51 +250,29 @@ static int ina2xx_do_update(struct device *dev)
>  	return -ENODEV;
>  }
>  
> -static struct ina2xx_data *ina2xx_update_device(struct device *dev)
> -{
> -	struct ina2xx_data *data = dev_get_drvdata(dev);
> -	struct ina2xx_data *ret = data;
> -	unsigned long after;
> -	int rv;
> -
> -	mutex_lock(&data->update_lock);
> -
> -	after = data->last_updated + data->update_interval;
> -	if (time_after(jiffies, after) || !data->valid) {
> -		rv = ina2xx_do_update(dev);
> -		if (rv < 0)
> -			ret = ERR_PTR(rv);
> -	}
> -
> -	mutex_unlock(&data->update_lock);
> -	return ret;
> -}
> -
> -static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
> +static int ina2xx_get_value(struct ina2xx_data *data, u8 reg, unsigned int rv)

rv -> regval

even though this requires you to split the line, it is a much better
variable name.

>  {
>  	int val;
>  
>  	switch (reg) {
>  	case INA2XX_SHUNT_VOLTAGE:
>  		/* signed register */
> -		val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
> -					data->config->shunt_div);
> +		val = DIV_ROUND_CLOSEST((s16)rv, data->config->shunt_div);
>  		break;
>  	case INA2XX_BUS_VOLTAGE:
> -		val = (data->regs[reg] >> data->config->bus_voltage_shift)
> +		val = (rv >> data->config->bus_voltage_shift)
>  		  * data->config->bus_voltage_lsb;
>  		val = DIV_ROUND_CLOSEST(val, 1000);
>  		break;
>  	case INA2XX_POWER:
> -		val = data->regs[reg] * data->config->power_lsb;
> +		val = rv * data->config->power_lsb;
>  		break;
>  	case INA2XX_CURRENT:
>  		/* signed register, LSB=1mA (selected), in mA */
> -		val = (s16)data->regs[reg];
> +		val = (s16)rv;
>  		break;
>  	case INA2XX_CALIBRATION:
> -		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
> -					data->regs[reg]);
> +		val = DIV_ROUND_CLOSEST(data->config->calibration_factor, rv);
>  		break;
>  	default:
>  		/* programmer goofed */
> @@ -316,25 +288,25 @@ static ssize_t ina2xx_show_value(struct device *dev,
>  				 struct device_attribute *da, char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	unsigned int rv;

"regval" would be a better variable name. "rv" is confusing because it commonly
means "return value".

>  
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	int err = ina2xx_do_update(dev, attr->index, &rv);
> +
> +	if (err < 0)
> +		return err;
>  
>  	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			ina2xx_get_value(data, attr->index));
> +			ina2xx_get_value(data, attr->index, rv));
>  }
>  
>  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);
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
>  
>  	status = kstrtoul(buf, 10, &val);
>  	if (status < 0)
> @@ -345,10 +317,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
>  	    val > data->config->calibration_factor)
>  		return -EINVAL;
>  
> -	mutex_lock(&data->update_lock);
>  	data->rshunt = val;
>  	status = ina2xx_calibrate(data);
> -	mutex_unlock(&data->update_lock);
>  	if (status < 0)
>  		return status;
>  
> @@ -370,17 +340,9 @@ static ssize_t ina226_set_interval(struct device *dev,
>  	if (val > INT_MAX || val == 0)
>  		return -EINVAL;
>  
> -	mutex_lock(&data->update_lock);
> -	data->curr_config = ina226_interval_to_reg(val,
> -						   data->regs[INA2XX_CONFIG]);
> -	status = i2c_smbus_write_word_swapped(data->client,
> -					      INA2XX_CONFIG,
> -					      data->curr_config);
> -
> -	ina226_set_update_interval(data);
> -	/* Make sure the next access re-reads all registers. */
> -	data->valid = 0;
> -	mutex_unlock(&data->update_lock);
> +	status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
> +				    INA226_AVG_RD_MASK,
> +				    ina226_interval_to_reg(val));
>  	if (status < 0)
>  		return status;
>  
> @@ -390,18 +352,15 @@ static ssize_t ina226_set_interval(struct device *dev,
>  static ssize_t ina226_show_interval(struct device *dev,
>  				    struct device_attribute *da, char *buf)
>  {
> -	struct ina2xx_data *data = ina2xx_update_device(dev);
> +	struct ina2xx_data *data = dev_get_drvdata(dev);
> +	int status;
> +	unsigned int rv;
>  
> -	if (IS_ERR(data))
> -		return PTR_ERR(data);
> +	status = regmap_read(data->regmap, INA2XX_CONFIG, &rv);
> +	if (status)
> +		return status;
>  
> -	/*
> -	 * We don't use data->update_interval here as we want to display
> -	 * the actual interval used by the chip and jiffies_to_msecs()
> -	 * doesn't seem to be accurate enough.
> -	 */
> -	return snprintf(buf, PAGE_SIZE, "%d\n",
> -			ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
> +	return snprintf(buf, PAGE_SIZE, "%d\n", ina226_reg_to_interval(rv));
>  }
>  
>  /* shunt voltage */
> @@ -455,7 +414,6 @@ static const struct attribute_group ina226_group = {
>  static int ina2xx_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	struct i2c_adapter *adapter = client->adapter;
>  	struct ina2xx_platform_data *pdata;
>  	struct device *dev = &client->dev;
>  	struct ina2xx_data *data;
> @@ -463,9 +421,6 @@ static int ina2xx_probe(struct i2c_client *client,
>  	u32 val;
>  	int ret, group = 0;
>  
> -	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
> -		return -ENODEV;
> -
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
> @@ -483,30 +438,26 @@ static int ina2xx_probe(struct i2c_client *client,
>  	/* set the device type */
>  	data->kind = id->driver_data;
>  	data->config = &ina2xx_config[data->kind];
> -	data->curr_config = data->config->config_default;
>  	data->client = client;
>  
> -	/*
> -	 * Ina226 has a variable update_interval. For ina219 we
> -	 * use a constant value.
> -	 */
> -	if (data->kind == ina226)
> -		ina226_set_update_interval(data);
> -	else
> -		data->update_interval = HZ / INA2XX_CONVERSION_RATE;
> -
>  	if (data->rshunt <= 0 ||
>  	    data->rshunt > data->config->calibration_factor)
>  		return -ENODEV;
>  
> +	ina2xx_regmap_config.max_register = data->config->registers;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(dev, "failed to allocate register map\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
>  	ret = ina2xx_init(data);
>  	if (ret < 0) {
>  		dev_err(dev, "error configuring the device: %d\n", ret);
>  		return -ENODEV;
>  	}
>  
> -	mutex_init(&data->update_lock);
> -
>  	data->groups[group++] = &ina2xx_group;
>  	if (data->kind == ina226)
>  		data->groups[group++] = &ina226_group;
> -- 
> 1.9.1
> 

Re: [PATCH 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data.

[Guenter Roeck]

On Mon, Oct 26, 2015 at 05:24:33PM +0100, Marc Titinger wrote:
> when checking for the value of the shunt resistor.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
>  drivers/hwmon/ina2xx.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 3edd163..1946433 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -414,7 +414,6 @@ static const struct attribute_group ina226_group = {
>  static int ina2xx_probe(struct i2c_client *client,
>  			const struct i2c_device_id *id)
>  {
> -	struct ina2xx_platform_data *pdata;
>  	struct device *dev = &client->dev;
>  	struct ina2xx_data *data;
>  	struct device *hwmon_dev;
> @@ -425,14 +424,13 @@ static int ina2xx_probe(struct i2c_client *client,
>  	if (!data)
>  		return -ENOMEM;
>  
> -	if (dev_get_platdata(dev)) {
> -		pdata = dev_get_platdata(dev);
> -		data->rshunt = pdata->shunt_uohms;
> -	} else if (!of_property_read_u32(dev->of_node,
> -					 "shunt-resistor", &val)) {
> -		data->rshunt = val;
> -	} else {
> -		data->rshunt = INA2XX_RSHUNT_DEFAULT;
> +	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
> +		struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
> +
> +		if (pdata)
> +			val = pdata->shunt_uohms;
> +		else
> +			val = INA2XX_RSHUNT_DEFAULT;
>  	}
>  
>  	/* set the device type */
> @@ -440,10 +438,11 @@ static int ina2xx_probe(struct i2c_client *client,
>  	data->config = &ina2xx_config[data->kind];
>  	data->client = client;
>  
> -	if (data->rshunt <= 0 ||
> -	    data->rshunt > data->config->calibration_factor)
> +	if (val <= 0 || val > data->config->calibration_factor)
>  		return -ENODEV;
>  
> +	data->rshunt = val;
> +

I think it would be better to rearrange the code a bit. 
The code now looks odd, with value being assigned, then the assignment
of data->kind etc, followed by checking val.

It might be better to do the assignments first, then calculate val,
and check its range immediately.

>  	ina2xx_regmap_config.max_register = data->config->registers;
>  
>  	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> -- 
> 1.9.1
> 

Re: [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap

[Guenter Roeck]

On Mon, Oct 26, 2015 at 05:24:32PM +0100, Marc Titinger wrote:
> Any sysfs "show" read access from the client app will result in reading
> all registers (8 with ina226). Depending on the host this can limit the
> best achievable read rate.
> 
> This changeset allows for individual register accesses through regmap.
> 
> Tested with BeagleBone Black (Baylibre-ACME) and ina226.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

Another comment: In ina2xx_calibrate, also use regmap_write().
Then you can drop the client variable in ina2xx_data.

Thanks,
Guenter

[PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

[Marc Titinger]

Any sysfs "show" read access from the client app will result in reading
all registers (8 with ina226). Depending on the host this can limit the
best achievable read rate.

This changeset allows for individual register accesses through regmap.

Tested with BeagleBone Black (Baylibre-ACME) and ina226.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

  v2:
	- rename 'rv' to 'regval' for clarity
	- fix missed smbus_xxx api change to regmap
	- rename ina2xx_do_update to ina2xx_read_reg
	- fix indentation

 drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
 1 file changed, 82 insertions(+), 129 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..5e7ada8 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -37,6 +37,7 @@
 #include <linux/of.h>
 #include <linux/delay.h>
 #include <linux/util_macros.h>
+#include <linux/regmap.h>
 
 #include <linux/platform_data/ina2xx.h>
 
@@ -84,6 +85,11 @@
  */
 #define INA226_TOTAL_CONV_TIME_DEFAULT	2200
 
+static struct regmap_config ina2xx_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+};
+
 enum ina2xx_ids { ina219, ina226 };
 
 struct ina2xx_config {
@@ -97,20 +103,13 @@ struct ina2xx_config {
 };
 
 struct ina2xx_data {
-	struct i2c_client *client;
 	const struct ina2xx_config *config;
 
 	long rshunt;
-	u16 curr_config;
-
-	struct mutex update_lock;
-	bool valid;
-	unsigned long last_updated;
-	int update_interval; /* in jiffies */
+	struct regmap *regmap;
 
 	int kind;
 	const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
-	u16 regs[INA2XX_MAX_REGISTERS];
 };
 
 static const struct ina2xx_config ina2xx_config[] = {
@@ -153,7 +152,11 @@ static int ina226_reg_to_interval(u16 config)
 	return DIV_ROUND_CLOSEST(avg * INA226_TOTAL_CONV_TIME_DEFAULT, 1000);
 }
 
-static u16 ina226_interval_to_reg(int interval, u16 config)
+/*
+ * Return the new, shifted AVG field value of CONFIG register,
+ * to use with regmap_update_bits
+ */
+static u16 ina226_interval_to_reg(int interval)
 {
 	int avg, avg_bits;
 
@@ -162,15 +165,7 @@ static u16 ina226_interval_to_reg(int interval, u16 config)
 	avg_bits = find_closest(avg, ina226_avg_tab,
 				ARRAY_SIZE(ina226_avg_tab));
 
-	return (config & ~INA226_AVG_RD_MASK) | INA226_SHIFT_AVG(avg_bits);
-}
-
-static void ina226_set_update_interval(struct ina2xx_data *data)
-{
-	int ms;
-
-	ms = ina226_reg_to_interval(data->curr_config);
-	data->update_interval = msecs_to_jiffies(ms);
+	return INA226_SHIFT_AVG(avg_bits);
 }
 
 static int ina2xx_calibrate(struct ina2xx_data *data)
@@ -178,8 +173,7 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
 	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
 				    data->rshunt);
 
-	return i2c_smbus_write_word_swapped(data->client,
-					    INA2XX_CALIBRATION, val);
+	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
 }
 
 /*
@@ -187,12 +181,8 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
  */
 static int ina2xx_init(struct ina2xx_data *data)
 {
-	struct i2c_client *client = data->client;
-	int ret;
-
-	/* device configuration */
-	ret = i2c_smbus_write_word_swapped(client, INA2XX_CONFIG,
-					   data->curr_config);
+	int ret = regmap_write(data->regmap, INA2XX_CONFIG,
+			       data->config->config_default);
 	if (ret < 0)
 		return ret;
 
@@ -203,47 +193,52 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
 {
 	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct i2c_client *client = data->client;
-	int i, rv, retry;
+	int ret, retry;
 
-	dev_dbg(&client->dev, "Starting ina2xx update\n");
+	dev_dbg(dev, "Starting register %d read\n", reg);
 
 	for (retry = 5; retry; retry--) {
-		/* Read all registers */
-		for (i = 0; i < data->config->registers; i++) {
-			rv = i2c_smbus_read_word_swapped(client, i);
-			if (rv < 0)
-				return rv;
-			data->regs[i] = rv;
-		}
+
+		ret = regmap_read(data->regmap, reg, regval);
+		if (ret < 0)
+			return ret;
+
+		dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *regval);
 
 		/*
 		 * If the current value in the calibration register is 0, the
 		 * power and current registers will also remain at 0. In case
 		 * the chip has been reset let's check the calibration
 		 * register and reinitialize if needed.
+		 * We do that extra read of the calibration register if there
+		 * is some hint of a chip reset.
 		 */
-		if (data->regs[INA2XX_CALIBRATION] == 0) {
-			dev_warn(dev, "chip not calibrated, reinitializing\n");
-
-			rv = ina2xx_init(data);
-			if (rv < 0)
-				return rv;
-
-			/*
-			 * Let's make sure the power and current registers
-			 * have been updated before trying again.
-			 */
-			msleep(INA2XX_MAX_DELAY);
-			continue;
+		if (*regval == 0) {
+			unsigned int cal;
+
+			ret = regmap_read(data->regmap, INA2XX_CALIBRATION,
+					  &cal);
+			if (ret < 0)
+				return ret;
+
+			if (cal == 0) {
+				dev_warn(dev, "chip not calibrated, reinitializing\n");
+
+				ret = ina2xx_init(data);
+				if (ret < 0)
+					return ret;
+				/*
+				 * Let's make sure the power and current
+				 * registers have been updated before trying
+				 * again.
+				 */
+				msleep(INA2XX_MAX_DELAY);
+				continue;
+			}
 		}
-
-		data->last_updated = jiffies;
-		data->valid = 1;
-
 		return 0;
 	}
 
@@ -256,51 +251,31 @@ static int ina2xx_do_update(struct device *dev)
 	return -ENODEV;
 }
 
-static struct ina2xx_data *ina2xx_update_device(struct device *dev)
-{
-	struct ina2xx_data *data = dev_get_drvdata(dev);
-	struct ina2xx_data *ret = data;
-	unsigned long after;
-	int rv;
-
-	mutex_lock(&data->update_lock);
-
-	after = data->last_updated + data->update_interval;
-	if (time_after(jiffies, after) || !data->valid) {
-		rv = ina2xx_do_update(dev);
-		if (rv < 0)
-			ret = ERR_PTR(rv);
-	}
-
-	mutex_unlock(&data->update_lock);
-	return ret;
-}
-
-static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
+static int ina2xx_get_value(struct ina2xx_data *data, u8 reg,
+			    unsigned int regval)
 {
 	int val;
 
 	switch (reg) {
 	case INA2XX_SHUNT_VOLTAGE:
 		/* signed register */
-		val = DIV_ROUND_CLOSEST((s16)data->regs[reg],
-					data->config->shunt_div);
+		val = DIV_ROUND_CLOSEST((s16)regval, data->config->shunt_div);
 		break;
 	case INA2XX_BUS_VOLTAGE:
-		val = (data->regs[reg] >> data->config->bus_voltage_shift)
+		val = (regval >> data->config->bus_voltage_shift)
 		  * data->config->bus_voltage_lsb;
 		val = DIV_ROUND_CLOSEST(val, 1000);
 		break;
 	case INA2XX_POWER:
-		val = data->regs[reg] * data->config->power_lsb;
+		val = regval * data->config->power_lsb;
 		break;
 	case INA2XX_CURRENT:
 		/* signed register, LSB=1mA (selected), in mA */
-		val = (s16)data->regs[reg];
+		val = (s16)regval;
 		break;
 	case INA2XX_CALIBRATION:
 		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
-					data->regs[reg]);
+					regval);
 		break;
 	default:
 		/* programmer goofed */
@@ -316,25 +291,25 @@ static ssize_t ina2xx_show_value(struct device *dev,
 				 struct device_attribute *da, char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	unsigned int regval;
+
+	int err = ina2xx_read_reg(dev, attr->index, &regval);
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	if (err < 0)
+		return err;
 
 	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina2xx_get_value(data, attr->index));
+			ina2xx_get_value(data, attr->index, regval));
 }
 
 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);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
 
 	status = kstrtoul(buf, 10, &val);
 	if (status < 0)
@@ -345,10 +320,8 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
 	    val > data->config->calibration_factor)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
 	data->rshunt = val;
 	status = ina2xx_calibrate(data);
-	mutex_unlock(&data->update_lock);
 	if (status < 0)
 		return status;
 
@@ -370,17 +343,9 @@ static ssize_t ina226_set_interval(struct device *dev,
 	if (val > INT_MAX || val == 0)
 		return -EINVAL;
 
-	mutex_lock(&data->update_lock);
-	data->curr_config = ina226_interval_to_reg(val,
-						   data->regs[INA2XX_CONFIG]);
-	status = i2c_smbus_write_word_swapped(data->client,
-					      INA2XX_CONFIG,
-					      data->curr_config);
-
-	ina226_set_update_interval(data);
-	/* Make sure the next access re-reads all registers. */
-	data->valid = 0;
-	mutex_unlock(&data->update_lock);
+	status = regmap_update_bits(data->regmap, INA2XX_CONFIG,
+				    INA226_AVG_RD_MASK,
+				    ina226_interval_to_reg(val));
 	if (status < 0)
 		return status;
 
@@ -390,18 +355,15 @@ static ssize_t ina226_set_interval(struct device *dev,
 static ssize_t ina226_show_interval(struct device *dev,
 				    struct device_attribute *da, char *buf)
 {
-	struct ina2xx_data *data = ina2xx_update_device(dev);
+	struct ina2xx_data *data = dev_get_drvdata(dev);
+	int status;
+	unsigned int regval;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	status = regmap_read(data->regmap, INA2XX_CONFIG, &regval);
+	if (status)
+		return status;
 
-	/*
-	 * We don't use data->update_interval here as we want to display
-	 * the actual interval used by the chip and jiffies_to_msecs()
-	 * doesn't seem to be accurate enough.
-	 */
-	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina226_reg_to_interval(data->regs[INA2XX_CONFIG]));
+	return snprintf(buf, PAGE_SIZE, "%d\n", ina226_reg_to_interval(regval));
 }
 
 /* shunt voltage */
@@ -455,7 +417,6 @@ static const struct attribute_group ina226_group = {
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct i2c_adapter *adapter = client->adapter;
 	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
@@ -463,9 +424,6 @@ static int ina2xx_probe(struct i2c_client *client,
 	u32 val;
 	int ret, group = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
-		return -ENODEV;
-
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
@@ -483,30 +441,25 @@ static int ina2xx_probe(struct i2c_client *client,
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
-	data->curr_config = data->config->config_default;
-	data->client = client;
-
-	/*
-	 * Ina226 has a variable update_interval. For ina219 we
-	 * use a constant value.
-	 */
-	if (data->kind == ina226)
-		ina226_set_update_interval(data);
-	else
-		data->update_interval = HZ / INA2XX_CONVERSION_RATE;
 
 	if (data->rshunt <= 0 ||
 	    data->rshunt > data->config->calibration_factor)
 		return -ENODEV;
 
+	ina2xx_regmap_config.max_register = data->config->registers;
+
+	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(dev, "failed to allocate register map\n");
+		return PTR_ERR(data->regmap);
+	}
+
 	ret = ina2xx_init(data);
 	if (ret < 0) {
 		dev_err(dev, "error configuring the device: %d\n", ret);
 		return -ENODEV;
 	}
 
-	mutex_init(&data->update_lock);
-
 	data->groups[group++] = &ina2xx_group;
 	if (data->kind == ina226)
 		data->groups[group++] = &ina226_group;
-- 
1.9.1

[PATCH v2 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data.

[Marc Titinger]

when checking for the value of the shunt resistor.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

 v2:
	- do part type init earlier to regroup rshunt value checking
	  and assignement.

 drivers/hwmon/ina2xx.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 5e7ada8..5fe742b 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -417,7 +417,6 @@ static const struct attribute_group ina226_group = {
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
@@ -428,24 +427,24 @@ static int ina2xx_probe(struct i2c_client *client,
 	if (!data)
 		return -ENOMEM;
 
-	if (dev_get_platdata(dev)) {
-		pdata = dev_get_platdata(dev);
-		data->rshunt = pdata->shunt_uohms;
-	} else if (!of_property_read_u32(dev->of_node,
-					 "shunt-resistor", &val)) {
-		data->rshunt = val;
-	} else {
-		data->rshunt = INA2XX_RSHUNT_DEFAULT;
-	}
-
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
 
-	if (data->rshunt <= 0 ||
-	    data->rshunt > data->config->calibration_factor)
+	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
+		struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
+
+		if (pdata)
+			val = pdata->shunt_uohms;
+		else
+			val = INA2XX_RSHUNT_DEFAULT;
+	}
+
+	if (val <= 0 || val > data->config->calibration_factor)
 		return -ENODEV;
 
+	data->rshunt = val;
+
 	ina2xx_regmap_config.max_register = data->config->registers;
 
 	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
-- 
1.9.1

Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

[Guenter Roeck]

On Tue, Oct 27, 2015 at 10:51:07AM +0100, Marc Titinger wrote:
> Any sysfs "show" read access from the client app will result in reading
> all registers (8 with ina226). Depending on the host this can limit the
> best achievable read rate.
> 
> This changeset allows for individual register accesses through regmap.
> 
> Tested with BeagleBone Black (Baylibre-ACME) and ina226.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
> 
>   v2:
> 	- rename 'rv' to 'regval' for clarity
> 	- fix missed smbus_xxx api change to regmap
> 	- rename ina2xx_do_update to ina2xx_read_reg
> 	- fix indentation
> 
>  drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
>  1 file changed, 82 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 4d28150..5e7ada8 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c

[ ... ]
>  
> -	mutex_lock(&data->update_lock);
>  	data->rshunt = val;
>  	status = ina2xx_calibrate(data);
> -	mutex_unlock(&data->update_lock);

I think this can result in a race conditon if multiple processes
try to update the shunt resistor value at the same time in a
multi-core system. There is no guarantee that the value programmed
into the chip matches the value that is written into 'rshunt'.
So I think we still need the mutex, unless you have a better
idea how to avoid the race.

Thanks,
Guenter

Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

[Marc Titinger]

On 28/10/2015 03:47, Guenter Roeck wrote:
> On Tue, Oct 27, 2015 at 10:51:07AM +0100, Marc Titinger wrote:
>> Any sysfs "show" read access from the client app will result in reading
>> all registers (8 with ina226). Depending on the host this can limit the
>> best achievable read rate.
>>
>> This changeset allows for individual register accesses through regmap.
>>
>> Tested with BeagleBone Black (Baylibre-ACME) and ina226.
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>
>>    v2:
>> 	- rename 'rv' to 'regval' for clarity
>> 	- fix missed smbus_xxx api change to regmap
>> 	- rename ina2xx_do_update to ina2xx_read_reg
>> 	- fix indentation
>>
>>   drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
>>   1 file changed, 82 insertions(+), 129 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 4d28150..5e7ada8 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>
> [ ... ]
>>
>> -	mutex_lock(&data->update_lock);
>>   	data->rshunt = val;
>>   	status = ina2xx_calibrate(data);
>> -	mutex_unlock(&data->update_lock);
>
> I think this can result in a race conditon if multiple processes
> try to update the shunt resistor value at the same time in a
> multi-core system. There is no guarantee that the value programmed
> into the chip matches the value that is written into 'rshunt'.
> So I think we still need the mutex, unless you have a better
> idea how to avoid the race.
>
Right, I had that same afterthought when removing the mutex at that 
position. Fix incoming!

M.


> Thanks,
> Guenter
>