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

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

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