| 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: <547B7707.5060708@roeck-us.net>
Date: Sun, 30 Nov 2014 11:59:03 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Bartosz Golaszewski <bgolaszewski@...libre.com>
CC: LKML <linux-kernel@...r.kernel.org>,
Benoit Cousson <bcousson@...libre.com>,
Patrick Titiano <ptitiano@...libre.com>,
LM Sensors <lm-sensors@...sensors.org>,
Jean Delvare <jdelvare@...e.de>
Subject: Re: [PATCH v2 3/5] hwmon: ina2xx: allow to change the averaging rate
at run-time
On 11/27/2014 01:59 AM, Bartosz Golaszewski wrote:
> The averaging rate of ina226 is hardcoded to 16 in the driver.
>
> Make it modifiable at run-time via a new sysfs attribute.
>
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> ---
> drivers/hwmon/ina2xx.c | 120 ++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 109 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
> index 341da67..7fdb586 100644
> --- a/drivers/hwmon/ina2xx.c
> +++ b/drivers/hwmon/ina2xx.c
> @@ -54,6 +54,9 @@
> /* shunt resistor sysfs attribute index */
> #define INA2XX_RSHUNT 0x8
>
> +/* INA226 averaging sysfs index */
> +#define INA226_AVG 0x9
> +
> /* register count */
> #define INA219_REGISTERS 6
> #define INA226_REGISTERS 8
> @@ -70,6 +73,12 @@
> /* default shunt resistance */
> #define INA2XX_RSHUNT_DEFAULT 10000
>
> +/* bit masks for the averaging setting in the configuration register */
> +#define INA226_AVG_RD_MASK 0x0E00
> +#define INA226_AVG_WR_MASK 0xF1FF
> +
> +#define INA226_READ_AVG(reg) ((reg & INA226_AVG_RD_MASK) >> 9)
> +
> enum ina2xx_ids { ina219, ina226 };
>
> struct ina2xx_config {
> @@ -117,11 +126,57 @@ static const struct ina2xx_config ina2xx_config[] = {
> },
> };
>
> +/* Available averaging rates for ina226. The indices correspond with
We use standard multi-line comment style in hwmon.
> + * the bit values expected by the chip (according to the ina226 datasheet,
> + * table 3 AVG bit settings, found at
> + * http://www.ti.com/lit/ds/symlink/ina226.pdf.
> + */
> +static const int ina226_avg_tab[] = { 1, 4, 16, 64, 128, 256, 512, 1024 };
> +
> static u16 ina2xx_calibration_val(const struct ina2xx_data *data)
> {
> return data->config->calibration_factor / data->rshunt;
> }
>
> +static int ina226_avg_bits(int avg)
> +{
> + int i;
> +
> + for (i = 0; i <= ARRAY_SIZE(ina226_avg_tab); i++) {
> + if (avg == ina226_avg_tab[i])
> + return i;
Reads beyond the end of the table.
> + }
> +
> + return -EINVAL;
You are expecting the user to know the valid averages.
> +}
> +
> +static int ina226_avg_val(int bits)
> +{
> + /* Value read from the config register should be correct, but do check
> + * the boundary just in case.
> + */
> + if (bits >= ARRAY_SIZE(ina226_avg_tab)) {
> + WARN_ON_ONCE(1);
Really ? Traceback if you read an unexpected value from the chip ?
> + return -1;
Please use a defined error code.
> + }
> +
> + return ina226_avg_tab[bits];
> +}
> +
> +static inline int ina226_update_avg(struct ina2xx_data *data, int avg)
> +{
> + int status;
> + u16 conf;
> +
> + mutex_lock(&data->update_lock);
> + conf = (data->regs[INA2XX_CONFIG] & INA226_AVG_WR_MASK) | (avg << 9);
> + status = i2c_smbus_write_word_swapped(data->client,
> + INA2XX_CONFIG, conf);
> + mutex_unlock(&data->update_lock);
> +
> + return status;
> +}
> +
> static struct ina2xx_data *ina2xx_update_device(struct device *dev)
> {
> struct ina2xx_data *data = dev_get_drvdata(dev);
> @@ -179,6 +234,10 @@ static int ina2xx_get_value(struct ina2xx_data *data, u8 reg)
> case INA2XX_RSHUNT:
> val = data->rshunt;
> break;
> + case INA226_AVG:
> + val = ina226_avg_val(INA226_READ_AVG(
> + data->regs[INA2XX_CONFIG]));
> + break;
> default:
> /* programmer goofed */
> WARN_ON_ONCE(1);
> @@ -202,10 +261,22 @@ static ssize_t ina2xx_show_value(struct device *dev,
> ina2xx_get_value(data, attr->index));
> }
>
> -static ssize_t ina2xx_set_shunt(struct device *dev,
Use separate functions. There is no common code.
> +static ssize_t ina226_show_avg(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + struct ina2xx_data *data = dev_get_drvdata(dev);
> +
> + if (data->kind != ina226)
> + return -ENXIO;
> +
No.
> + return ina2xx_show_value(dev, da, buf);
> +}
> +
> +static ssize_t ina2xx_set_value(struct device *dev,
> struct device_attribute *da,
> const char *buf, size_t count)
> {
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> struct ina2xx_data *data = ina2xx_update_device(dev);
> long val;
> int status;
> @@ -217,16 +288,38 @@ static ssize_t ina2xx_set_shunt(struct device *dev,
> if (status < 0)
> return status;
>
> - if (val == 0)
> - return -EINVAL;
> + switch (attr->index) {
> + case INA2XX_RSHUNT:
> + if (val == 0)
> + return -EINVAL;
>
> - mutex_lock(&data->update_lock);
> - data->rshunt = val;
> - status = i2c_smbus_write_word_swapped(data->client, INA2XX_CALIBRATION,
> - ina2xx_calibration_val(data));
> - mutex_unlock(&data->update_lock);
> - if (status < 0)
> - return status;
> + mutex_lock(&data->update_lock);
> + data->rshunt = val;
> + status = i2c_smbus_write_word_swapped(data->client,
> + INA2XX_CALIBRATION, ina2xx_calibration_val(data));
> + mutex_unlock(&data->update_lock);
> + if (status < 0)
> + return status;
> +
> + break;
> + case INA226_AVG:
> + if (data->kind != ina226)
> + return -ENXIO;
> +
For other chips the attribute should not exist or be handled correctly.
ENXIO is inappropriate.
> + status = ina226_avg_bits(val);
> + if (status < 0)
> + return status;
> +
> + status = ina226_update_avg(data, status);
> + if (status < 0)
> + return status;
> + break;
> + default:
> + /* programmer goofed */
> + WARN_ON_ONCE(1);
> + val = 0;
> + break;
> + }
>
> return count;
> }
> @@ -249,7 +342,11 @@ static SENSOR_DEVICE_ATTR(power1_input, S_IRUGO, ina2xx_show_value, NULL,
>
> /* shunt resistance */
> static SENSOR_DEVICE_ATTR(shunt_resistor, S_IRUGO | S_IWUSR | S_IWGRP,
> - ina2xx_show_value, ina2xx_set_shunt, INA2XX_RSHUNT);
> + ina2xx_show_value, ina2xx_set_value, INA2XX_RSHUNT);
> +
> +/* averaging rate */
> +static SENSOR_DEVICE_ATTR(avg_rate, S_IRUGO | S_IWUSR | S_IWGRP,
> + ina226_show_avg, ina2xx_set_value, INA226_AVG);
>
> /* pointers to created device attributes */
> static struct attribute *ina2xx_attrs[] = {
> @@ -258,6 +355,7 @@ static struct attribute *ina2xx_attrs[] = {
> &sensor_dev_attr_curr1_input.dev_attr.attr,
> &sensor_dev_attr_power1_input.dev_attr.attr,
> &sensor_dev_attr_shunt_resistor.dev_attr.attr,
> + &sensor_dev_attr_avg_rate.dev_attr.attr,
> NULL,
> };
> ATTRIBUTE_GROUPS(ina2xx);
>
--
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