From: Marc Titinger <mtitinger@baylibre.com> To: linux@roeck-us.net, jdelvare@suse.com Cc: lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org, Marc Titinger <mtitinger@baylibre.com> Subject: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Date: Mon, 19 Oct 2015 18:21:56 +0200 [thread overview] Message-ID: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> (raw) 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
WARNING: multiple messages have this Message-ID (diff)
From: Marc Titinger <mtitinger@baylibre.com> To: linux@roeck-us.net, jdelvare@suse.com Cc: lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org, Marc Titinger <mtitinger@baylibre.com> Subject: [lm-sensors] [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Date: Mon, 19 Oct 2015 16:21:56 +0000 [thread overview] Message-ID: <1445271716-15434-1-git-send-email-mtitinger@baylibre.com> (raw) 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 _______________________________________________ lm-sensors mailing list lm-sensors@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next reply other threads:[~2015-10-19 16:22 UTC|newest] Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-10-19 16:21 Marc Titinger [this message] 2015-10-19 16:21 ` [lm-sensors] [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Marc Titinger 2015-10-20 1:32 ` Guenter Roeck 2015-10-20 1:32 ` [lm-sensors] " Guenter Roeck 2015-10-20 7:58 ` Marc Titinger 2015-10-20 7:58 ` [lm-sensors] " Marc Titinger 2015-10-20 8:20 ` [PATCH v2] " Marc Titinger 2015-10-20 8:20 ` [lm-sensors] " Marc Titinger 2015-10-20 12:54 ` Guenter Roeck 2015-10-20 12:54 ` [lm-sensors] " Guenter Roeck 2015-10-20 13:17 ` Marc Titinger 2015-10-20 13:17 ` [lm-sensors] " Marc Titinger 2015-10-20 13:30 ` Guenter Roeck 2015-10-20 13:30 ` [lm-sensors] " Guenter Roeck 2015-10-20 13:46 ` Marc Titinger 2015-10-20 13:46 ` [lm-sensors] " Marc Titinger 2015-10-20 17:00 ` Guenter Roeck 2015-10-20 17:00 ` [lm-sensors] " Guenter Roeck 2015-10-21 7:46 ` Marc Titinger 2015-10-21 7:46 ` [lm-sensors] " Marc Titinger 2015-10-20 13:52 ` Michael Turquette 2015-10-20 13:52 ` [lm-sensors] " Michael Turquette 2015-10-23 16:13 ` [RFC] hwmon: ina2xx: port to using remap, improve bandwidth Marc Titinger 2015-10-23 16:13 ` [lm-sensors] " Marc Titinger 2015-10-23 16:49 ` kbuild test robot 2015-10-23 16:49 ` [lm-sensors] " kbuild test robot 2015-10-23 16:52 ` Guenter Roeck 2015-10-23 16:52 ` [lm-sensors] " Guenter Roeck 2015-10-23 20:35 ` Marc Titinger 2015-10-23 20:35 ` [lm-sensors] " Marc Titinger 2015-10-24 2:21 ` Guenter Roeck 2015-10-24 2:21 ` [lm-sensors] " Guenter Roeck 2015-10-24 12:45 ` Guenter Roeck 2015-10-24 12:45 ` [lm-sensors] " Guenter Roeck 2015-10-26 16:24 ` [PATCH 0/2] hwmon: ina2xx: convert driver to using regmap Marc Titinger 2015-10-26 16:24 ` [lm-sensors] " Marc Titinger 2015-10-26 16:24 ` [PATCH 1/2] " Marc Titinger 2015-10-26 16:24 ` [lm-sensors] " Marc Titinger 2015-10-27 1:02 ` Guenter Roeck 2015-10-27 1:02 ` [lm-sensors] " Guenter Roeck 2015-10-27 1:12 ` Guenter Roeck 2015-10-27 1:12 ` [lm-sensors] " Guenter Roeck 2015-10-27 9:51 ` [PATCH v2 " Marc Titinger 2015-10-27 9:51 ` [lm-sensors] " Marc Titinger 2015-10-28 2:47 ` Guenter Roeck 2015-10-28 2:47 ` [lm-sensors] " Guenter Roeck 2015-10-28 9:23 ` Marc Titinger 2015-10-28 9:23 ` [lm-sensors] " Marc Titinger 2015-10-26 16:24 ` [PATCH 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data Marc Titinger 2015-10-26 16:24 ` [lm-sensors] " Marc Titinger 2015-10-27 1:10 ` Guenter Roeck 2015-10-27 1:10 ` [lm-sensors] " Guenter Roeck 2015-10-27 9:51 ` [PATCH v2 " Marc Titinger 2015-10-27 9:51 ` [lm-sensors] " Marc Titinger 2015-10-23 16:55 ` [RFC] hwmon: ina2xx: port to using remap, improve bandwidth kbuild test robot 2015-10-23 16:55 ` [lm-sensors] " kbuild test robot 2015-10-23 20:10 ` kbuild test robot 2015-10-23 20:10 ` [lm-sensors] " kbuild test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=1445271716-15434-1-git-send-email-mtitinger@baylibre.com \ --to=mtitinger@baylibre.com \ --cc=jdelvare@suse.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=lm-sensors@lm-sensors.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.