From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751585AbbJTBcL (ORCPT ); Mon, 19 Oct 2015 21:32:11 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:53039 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750784AbbJTBcJ (ORCPT ); Mon, 19 Oct 2015 21:32:09 -0400 Subject: Re: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz To: Marc Titinger , jdelvare@suse.com References: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> Cc: lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org From: Guenter Roeck Message-ID: <56259997.6080706@roeck-us.net> Date: Mon, 19 Oct 2015 18:32:07 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: linux@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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); > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Date: Tue, 20 Oct 2015 01:32:07 +0000 Subject: Re: [lm-sensors] [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Message-Id: <56259997.6080706@roeck-us.net> List-Id: References: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> In-Reply-To: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Marc Titinger , jdelvare@suse.com Cc: lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org 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 > --- > 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); > _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors