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

[Marc Titinger <mtitinger@baylibre.com>]

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);
>>
>