lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20151027010235.GA31838@roeck-us.net> Date: Mon, 26 Oct 2015 18:02:35 -0700 From: Guenter Roeck <linux@...ck-us.net> To: Marc Titinger <mtitinger@...libre.com> Cc: jdelvare@...e.com, lm-sensors@...sensors.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap 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@...libre.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 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists