Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1250893 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-19 18:30 +0200
    Re: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above  160 Hz Guenter Roeck <linux@roeck-us.net> - 2015-10-20 03:40 +0200
      Re: [RFC] hwmon: ina2xx: allow for actual measurement bandwidth above  160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-20 10:00 +0200
      [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above 160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-20 10:30 +0200
        Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Guenter Roeck <linux@roeck-us.net> - 2015-10-20 15:00 +0200
          Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-20 15:20 +0200
            Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Guenter Roeck <linux@roeck-us.net> - 2015-10-20 15:40 +0200
              Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-20 15:50 +0200
                Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Guenter Roeck <linux@roeck-us.net> - 2015-10-20 19:10 +0200
                  Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth  above 160 Hz Marc Titinger <mtitinger@baylibre.com> - 2015-10-21 09:50 +0200
              Re: [PATCH v2] hwmon: ina2xx: allow for actual measurement bandwidth above  160 Hz Michael Turquette <mturquette@baylibre.com> - 2015-10-20 16:00 +0200
          [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. Marc Titinger <mtitinger@baylibre.com> - 2015-10-23 18:20 +0200
            Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. Guenter Roeck <linux@roeck-us.net> - 2015-10-23 19:00 +0200
              Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. Marc Titinger <mtitinger@baylibre.com> - 2015-10-23 22:40 +0200
                Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. Guenter Roeck <linux@roeck-us.net> - 2015-10-24 04:30 +0200
                Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. Guenter Roeck <linux@roeck-us.net> - 2015-10-24 14:50 +0200
                  [PATCH 0/2] hwmon: ina2xx: convert driver to using regmap Marc Titinger <mtitinger@baylibre.com> - 2015-10-26 17:30 +0100
                    [PATCH 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data. Marc Titinger <mtitinger@baylibre.com> - 2015-10-26 17:30 +0100
                      Re: [PATCH 2/2] hwmon: ina2xx: give precedence to DT over checking  for platform data. Guenter Roeck <linux@roeck-us.net> - 2015-10-27 02:20 +0100
                    [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap Marc Titinger <mtitinger@baylibre.com> - 2015-10-26 17:30 +0100
                      Re: [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap Guenter Roeck <linux@roeck-us.net> - 2015-10-27 02:10 +0100
                      Re: [PATCH 1/2] hwmon: ina2xx: convert driver to using regmap Guenter Roeck <linux@roeck-us.net> - 2015-10-27 02:20 +0100
                        [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap Marc Titinger <mtitinger@baylibre.com> - 2015-10-27 11:00 +0100
                          [PATCH v2 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data. Marc Titinger <mtitinger@baylibre.com> - 2015-10-27 11:00 +0100
                          Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap Guenter Roeck <linux@roeck-us.net> - 2015-10-28 03:50 +0100
                            Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap Marc Titinger <mtitinger@baylibre.com> - 2015-10-28 10:30 +0100
            Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth. kbuild test robot <lkp@intel.com> - 2015-10-23 19:00 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1256374 — 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@baylibre.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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256378 — 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.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>

Another comment: In ina2xx_calibrate, also use regmap_write().
Then you can drop the client variable in ina2xx_data.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256639 — [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

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.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

  v2:
	- rename 'rv' to 'regval' for clarity
	- fix missed smbus_xxx api change to regmap
	- rename ina2xx_do_update to ina2xx_read_reg
	- fix indentation

 drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
 1 file changed, 82 insertions(+), 129 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 4d28150..5e7ada8 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 {
@@ -97,20 +103,13 @@ struct ina2xx_config {
 };
 
 struct ina2xx_data {
-	struct i2c_client *client;
 	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 +152,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 +165,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)
@@ -178,8 +173,7 @@ static int ina2xx_calibrate(struct ina2xx_data *data)
 	u16 val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
 				    data->rshunt);
 
-	return i2c_smbus_write_word_swapped(data->client,
-					    INA2XX_CALIBRATION, val);
+	return regmap_write(data->regmap, INA2XX_CALIBRATION, val);
 }
 
 /*
@@ -187,12 +181,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,47 +193,52 @@ static int ina2xx_init(struct ina2xx_data *data)
 	return ina2xx_calibrate(data);
 }
 
-static int ina2xx_do_update(struct device *dev)
+static int ina2xx_read_reg(struct device *dev, int reg, unsigned int *regval)
 {
 	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 register %d read\n", reg);
 
 	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, regval);
+		if (ret < 0)
+			return ret;
+
+		dev_dbg(dev, "read %d, val = 0x%04x\n", reg, *regval);
 
 		/*
 		 * 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");
-
-			rv = ina2xx_init(data);
-			if (rv < 0)
-				return rv;
-
-			/*
-			 * Let's make sure the power and current registers
-			 * have been updated before trying again.
-			 */
-			msleep(INA2XX_MAX_DELAY);
-			continue;
+		if (*regval == 0) {
+			unsigned int cal;
+
+			ret = regmap_read(data->regmap, INA2XX_CALIBRATION,
+					  &cal);
+			if (ret < 0)
+				return ret;
+
+			if (cal == 0) {
+				dev_warn(dev, "chip not calibrated, reinitializing\n");
+
+				ret = ina2xx_init(data);
+				if (ret < 0)
+					return ret;
+				/*
+				 * Let's make sure the power and current
+				 * registers have been updated before trying
+				 * again.
+				 */
+				msleep(INA2XX_MAX_DELAY);
+				continue;
+			}
 		}
-
-		data->last_updated = jiffies;
-		data->valid = 1;
-
 		return 0;
 	}
 
@@ -256,51 +251,31 @@ 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 regval)
 {
 	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)regval, data->config->shunt_div);
 		break;
 	case INA2XX_BUS_VOLTAGE:
-		val = (data->regs[reg] >> data->config->bus_voltage_shift)
+		val = (regval >> 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 = regval * data->config->power_lsb;
 		break;
 	case INA2XX_CURRENT:
 		/* signed register, LSB=1mA (selected), in mA */
-		val = (s16)data->regs[reg];
+		val = (s16)regval;
 		break;
 	case INA2XX_CALIBRATION:
 		val = DIV_ROUND_CLOSEST(data->config->calibration_factor,
-					data->regs[reg]);
+					regval);
 		break;
 	default:
 		/* programmer goofed */
@@ -316,25 +291,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 regval;
+
+	int err = ina2xx_read_reg(dev, attr->index, &regval);
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	if (err < 0)
+		return err;
 
 	return snprintf(buf, PAGE_SIZE, "%d\n",
-			ina2xx_get_value(data, attr->index));
+			ina2xx_get_value(data, attr->index, regval));
 }
 
 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 +320,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 +343,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 +355,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 regval;
 
-	if (IS_ERR(data))
-		return PTR_ERR(data);
+	status = regmap_read(data->regmap, INA2XX_CONFIG, &regval);
+	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(regval));
 }
 
 /* shunt voltage */
@@ -455,7 +417,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 +424,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 +441,25 @@ 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1256643 — [PATCH v2 2/2] hwmon: ina2xx: give precedence to DT over checking for platform data.

when checking for the value of the shunt resistor.

Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
---

 v2:
	- do part type init earlier to regroup rshunt value checking
	  and assignement.

 drivers/hwmon/ina2xx.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 5e7ada8..5fe742b 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -417,7 +417,6 @@ static const struct attribute_group ina226_group = {
 static int ina2xx_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
-	struct ina2xx_platform_data *pdata;
 	struct device *dev = &client->dev;
 	struct ina2xx_data *data;
 	struct device *hwmon_dev;
@@ -428,24 +427,24 @@ static int ina2xx_probe(struct i2c_client *client,
 	if (!data)
 		return -ENOMEM;
 
-	if (dev_get_platdata(dev)) {
-		pdata = dev_get_platdata(dev);
-		data->rshunt = pdata->shunt_uohms;
-	} else if (!of_property_read_u32(dev->of_node,
-					 "shunt-resistor", &val)) {
-		data->rshunt = val;
-	} else {
-		data->rshunt = INA2XX_RSHUNT_DEFAULT;
-	}
-
 	/* set the device type */
 	data->kind = id->driver_data;
 	data->config = &ina2xx_config[data->kind];
 
-	if (data->rshunt <= 0 ||
-	    data->rshunt > data->config->calibration_factor)
+	if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
+		struct ina2xx_platform_data *pdata = dev_get_platdata(dev);
+
+		if (pdata)
+			val = pdata->shunt_uohms;
+		else
+			val = INA2XX_RSHUNT_DEFAULT;
+	}
+
+	if (val <= 0 || val > data->config->calibration_factor)
 		return -ENODEV;
 
+	data->rshunt = val;
+
 	ina2xx_regmap_config.max_register = data->config->registers;
 
 	data->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257611 — Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

On Tue, Oct 27, 2015 at 10:51:07AM +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.
> 
> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
> ---
> 
>   v2:
> 	- rename 'rv' to 'regval' for clarity
> 	- fix missed smbus_xxx api change to regmap
> 	- rename ina2xx_do_update to ina2xx_read_reg
> 	- fix indentation
> 
>  drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
>  1 file changed, 82 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 4d28150..5e7ada8 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c

[ ... ]
>  
> -	mutex_lock(&data->update_lock);
>  	data->rshunt = val;
>  	status = ina2xx_calibrate(data);
> -	mutex_unlock(&data->update_lock);

I think this can result in a race conditon if multiple processes
try to update the shunt resistor value at the same time in a
multi-core system. There is no guarantee that the value programmed
into the chip matches the value that is written into 'rshunt'.
So I think we still need the mutex, unless you have a better
idea how to avoid the race.

Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1257850 — Re: [PATCH v2 1/2] hwmon: ina2xx: convert driver to using regmap

On 28/10/2015 03:47, Guenter Roeck wrote:
> On Tue, Oct 27, 2015 at 10:51:07AM +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.
>>
>> Signed-off-by: Marc Titinger <mtitinger@baylibre.com>
>> ---
>>
>>    v2:
>> 	- rename 'rv' to 'regval' for clarity
>> 	- fix missed smbus_xxx api change to regmap
>> 	- rename ina2xx_do_update to ina2xx_read_reg
>> 	- fix indentation
>>
>>   drivers/hwmon/ina2xx.c | 211 +++++++++++++++++++------------------------------
>>   1 file changed, 82 insertions(+), 129 deletions(-)
>>
>> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
>> index 4d28150..5e7ada8 100644
>> --- a/drivers/hwmon/ina2xx.c
>> +++ b/drivers/hwmon/ina2xx.c
>
> [ ... ]
>>
>> -	mutex_lock(&data->update_lock);
>>   	data->rshunt = val;
>>   	status = ina2xx_calibrate(data);
>> -	mutex_unlock(&data->update_lock);
>
> I think this can result in a race conditon if multiple processes
> try to update the shunt resistor value at the same time in a
> multi-core system. There is no guarantee that the value programmed
> into the chip matches the value that is written into 'rshunt'.
> So I think we still need the mutex, unless you have a better
> idea how to avoid the race.
>
Right, I had that same afterthought when removing the mutex at that 
position. Fix incoming!

M.


> Thanks,
> Guenter
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1254720 — Re: [RFC] hwmon: ina2xx: port to using remap, improve bandwidth.

[Multipart message — attachments visible in raw view] — view raw

Hi Marc,

[auto build test WARNING on hwmon/hwmon-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Marc-Titinger/hwmon-ina2xx-port-to-using-remap-improve-bandwidth/20151024-001809
config: x86_64-randconfig-x011-10211707 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/hwmon/ina2xx.c: In function 'ina2xx_probe':
>> drivers/hwmon/ina2xx.c:456:16: warning: format '%li' expects argument of type 'long int', but argument 3 has type 'u32 {aka unsigned int}' [-Wformat=]
      dev_err(dev, "Invalid shunt resistor value %li", val);
                   ^

vim +456 drivers/hwmon/ina2xx.c

   440		/* Set config according to device type. */
   441		data->config = &ina2xx_config[id->driver_data];
   442		data->client = client;
   443	
   444		/* Check for shunt resistor value.
   445		 * Give precedence to device tree over must-recompile.
   446		 */
   447		if (of_property_read_u32(dev->of_node, "shunt-resistor", &val) < 0) {
   448			pdata = dev_get_platdata(dev);
   449			if (pdata)
   450				val = pdata->shunt_uohms;
   451			else
   452				val = INA2XX_RSHUNT_DEFAULT;
   453		}
   454	
   455		if (val <= 0 || val > data->config->calibration_factor) {
 > 456			dev_err(dev, "Invalid shunt resistor value %li", val);
   457			return -ENODEV;
   458		}
   459		data->rshunt = val;
   460	
   461		/* Write config to chip, and calibrate */
   462		ret = ina2xx_init(data);
   463		if (ret) {
   464			dev_err(dev, "error configuring the device.\n");

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web