* [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code
@ 2016-02-12 18:34 Andrew F. Davis
2016-02-12 18:34 ` [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments Andrew F. Davis
2016-02-13 12:55 ` [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Jonathan Cameron
0 siblings, 2 replies; 8+ messages in thread
From: Andrew F. Davis @ 2016-02-12 18:34 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel, Andrew F. Davis
Group of probably overly rigorous whitespace and code cleanups.
- Alphabetize includes
- Assign to variables in the order they are defined
- Alignment issues
- Group alike statements together
- Use helper macros
Signed-off-by: Andrew F. Davis <afd@ti.com>
---
drivers/iio/adc/ina2xx-adc.c | 164 +++++++++++++++++++++----------------------
1 file changed, 80 insertions(+), 84 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index d803e50..61e8ae9 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -19,17 +19,18 @@
*
* Configurable 7-bit I2C slave address from 0x40 to 0x4F
*/
-#include <linux/module.h>
-#include <linux/kthread.h>
+
#include <linux/delay.h>
+#include <linux/i2c.h>
#include <linux/iio/kfifo_buf.h>
#include <linux/iio/sysfs.h>
-#include <linux/i2c.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
#include <linux/regmap.h>
-#include <linux/platform_data/ina2xx.h>
-
#include <linux/util_macros.h>
+#include <linux/platform_data/ina2xx.h>
+
/* INA2XX registers definition */
#define INA2XX_CONFIG 0x00
#define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
@@ -38,7 +39,7 @@
#define INA2XX_CURRENT 0x04 /* readonly */
#define INA2XX_CALIBRATION 0x05
-#define INA226_ALERT_MASK 0x06
+#define INA226_ALERT_MASK GENMASK(2, 1)
#define INA266_CVRF BIT(3)
#define INA2XX_MAX_REGISTERS 8
@@ -113,7 +114,7 @@ struct ina2xx_chip_info {
struct mutex state_lock;
unsigned int shunt_resistor;
int avg;
- s64 prev_ns; /* track buffer capture time, check for underruns*/
+ s64 prev_ns; /* track buffer capture time, check for underruns*/
int int_time_vbus; /* Bus voltage integration time uS */
int int_time_vshunt; /* Shunt voltage integration time uS */
bool allow_async_readout;
@@ -121,21 +122,21 @@ struct ina2xx_chip_info {
static const struct ina2xx_config ina2xx_config[] = {
[ina219] = {
- .config_default = INA219_CONFIG_DEFAULT,
- .calibration_factor = 40960000,
- .shunt_div = 100,
- .bus_voltage_shift = 3,
- .bus_voltage_lsb = 4000,
- .power_lsb = 20000,
- },
+ .config_default = INA219_CONFIG_DEFAULT,
+ .calibration_factor = 40960000,
+ .shunt_div = 100,
+ .bus_voltage_shift = 3,
+ .bus_voltage_lsb = 4000,
+ .power_lsb = 20000,
+ },
[ina226] = {
- .config_default = INA226_CONFIG_DEFAULT,
- .calibration_factor = 5120000,
- .shunt_div = 400,
- .bus_voltage_shift = 0,
- .bus_voltage_lsb = 1250,
- .power_lsb = 25000,
- },
+ .config_default = INA226_CONFIG_DEFAULT,
+ .calibration_factor = 5120000,
+ .shunt_div = 400,
+ .bus_voltage_shift = 0,
+ .bus_voltage_lsb = 1250,
+ .power_lsb = 25000,
+ },
};
static int ina2xx_read_raw(struct iio_dev *indio_dev,
@@ -149,7 +150,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
ret = regmap_read(chip->regmap, chan->address, ®val);
- if (ret < 0)
+ if (ret)
return ret;
if (is_signed_reg(chan->address))
@@ -251,7 +252,7 @@ static int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
return -EINVAL;
bits = find_closest(val_us, ina226_conv_time_tab,
- ARRAY_SIZE(ina226_conv_time_tab));
+ ARRAY_SIZE(ina226_conv_time_tab));
chip->int_time_vbus = ina226_conv_time_tab[bits];
@@ -270,7 +271,7 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
return -EINVAL;
bits = find_closest(val_us, ina226_conv_time_tab,
- ARRAY_SIZE(ina226_conv_time_tab));
+ ARRAY_SIZE(ina226_conv_time_tab));
chip->int_time_vshunt = ina226_conv_time_tab[bits];
@@ -285,8 +286,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
int val, int val2, long mask)
{
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
- int ret;
unsigned int config, tmp;
+ int ret;
if (iio_buffer_enabled(indio_dev))
return -EBUSY;
@@ -294,8 +295,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
mutex_lock(&chip->state_lock);
ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
- if (ret < 0)
- goto _err;
+ if (ret)
+ goto err;
tmp = config;
@@ -310,19 +311,19 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
else
ret = ina226_set_int_time_vbus(chip, val2, &tmp);
break;
+
default:
ret = -EINVAL;
}
if (!ret && (tmp != config))
ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
-_err:
+err:
mutex_unlock(&chip->state_lock);
return ret;
}
-
static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -355,6 +356,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
return -EINVAL;
chip->shunt_resistor = val;
+
return 0;
}
@@ -408,21 +410,21 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
* Sampling Freq is a consequence of the integration times of
* the Voltage channels.
*/
-#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
- .type = IIO_VOLTAGE, \
- .address = (_address), \
- .indexed = 1, \
- .channel = (_index), \
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
- BIT(IIO_CHAN_INFO_SCALE) | \
- BIT(IIO_CHAN_INFO_INT_TIME), \
- .scan_index = (_index), \
- .scan_type = { \
- .sign = 'u', \
- .realbits = 16, \
- .storagebits = 16, \
- .endianness = IIO_LE, \
- } \
+#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
+ .type = IIO_VOLTAGE, \
+ .address = (_address), \
+ .indexed = 1, \
+ .channel = (_index), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE) | \
+ BIT(IIO_CHAN_INFO_INT_TIME), \
+ .scan_index = (_index), \
+ .scan_type = { \
+ .sign = 'u', \
+ .realbits = 16, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ } \
}
static const struct iio_chan_spec ina2xx_channels[] = {
@@ -500,7 +502,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
static int ina2xx_capture_thread(void *data)
{
- struct iio_dev *indio_dev = (struct iio_dev *)data;
+ struct iio_dev *indio_dev = data;
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
unsigned int sampling_us = SAMPLING_PERIOD(chip);
int buffer_us;
@@ -575,8 +577,7 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
}
/* Possible integration times for vshunt and vbus */
-static IIO_CONST_ATTR_INT_TIME_AVAIL \
- ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
+static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
ina2xx_allow_async_readout_show,
@@ -598,21 +599,23 @@ static const struct attribute_group ina2xx_attribute_group = {
};
static const struct iio_info ina2xx_info = {
- .debugfs_reg_access = &ina2xx_debug_reg,
- .read_raw = &ina2xx_read_raw,
- .write_raw = &ina2xx_write_raw,
- .attrs = &ina2xx_attribute_group,
.driver_module = THIS_MODULE,
+ .attrs = &ina2xx_attribute_group,
+ .read_raw = ina2xx_read_raw,
+ .write_raw = ina2xx_write_raw,
+ .debugfs_reg_access = ina2xx_debug_reg,
};
/* Initialize the configuration and calibration registers. */
static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
{
u16 regval;
- int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+ int ret;
- if (ret < 0)
+ ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
+ if (ret)
return ret;
+
/*
* Set current LSB to 1mA, shunt is in uOhms
* (equation 13 in datasheet). We hardcode a Current_LSB
@@ -621,7 +624,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
* to the user for now.
*/
regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
- chip->shunt_resistor);
+ chip->shunt_resistor);
return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
}
@@ -632,8 +635,8 @@ static int ina2xx_probe(struct i2c_client *client,
struct ina2xx_chip_info *chip;
struct iio_dev *indio_dev;
struct iio_buffer *buffer;
- int ret;
unsigned int val;
+ int ret;
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
if (!indio_dev)
@@ -641,8 +644,19 @@ static int ina2xx_probe(struct i2c_client *client,
chip = iio_priv(indio_dev);
+ /* This is only used for device removal purposes. */
+ i2c_set_clientdata(client, indio_dev);
+
+ chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
+ if (IS_ERR(chip->regmap)) {
+ dev_err(&client->dev, "failed to allocate register map\n");
+ return PTR_ERR(chip->regmap);
+ }
+
chip->config = &ina2xx_config[id->driver_data];
+ mutex_init(&chip->state_lock);
+
if (of_property_read_u32(client->dev.of_node,
"shunt-resistor", &val) < 0) {
struct ina2xx_platform_data *pdata =
@@ -658,25 +672,6 @@ static int ina2xx_probe(struct i2c_client *client,
if (ret)
return ret;
- mutex_init(&chip->state_lock);
-
- /* This is only used for device removal purposes. */
- i2c_set_clientdata(client, indio_dev);
-
- indio_dev->name = id->name;
- indio_dev->channels = ina2xx_channels;
- indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
-
- indio_dev->dev.parent = &client->dev;
- indio_dev->info = &ina2xx_info;
- indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
-
- chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
- if (IS_ERR(chip->regmap)) {
- dev_err(&client->dev, "failed to allocate register map\n");
- return PTR_ERR(chip->regmap);
- }
-
/* Patch the current config register with default. */
val = chip->config->config_default;
@@ -687,24 +682,28 @@ static int ina2xx_probe(struct i2c_client *client,
}
ret = ina2xx_init(chip, val);
- if (ret < 0) {
- dev_err(&client->dev, "error configuring the device: %d\n",
- ret);
- return -ENODEV;
+ if (ret) {
+ dev_err(&client->dev, "error configuring the device\n");
+ return ret;
}
+ indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->channels = ina2xx_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
+ indio_dev->name = id->name;
+ indio_dev->info = &ina2xx_info;
+ indio_dev->setup_ops = &ina2xx_setup_ops;
+
buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
if (!buffer)
return -ENOMEM;
- indio_dev->setup_ops = &ina2xx_setup_ops;
-
iio_device_attach_buffer(indio_dev, buffer);
return iio_device_register(indio_dev);
}
-
static int ina2xx_remove(struct i2c_client *client)
{
struct iio_dev *indio_dev = i2c_get_clientdata(client);
@@ -717,7 +716,6 @@ static int ina2xx_remove(struct i2c_client *client)
INA2XX_MODE_MASK, 0);
}
-
static const struct i2c_device_id ina2xx_id[] = {
{"ina219", ina219},
{"ina220", ina219},
@@ -726,7 +724,6 @@ static const struct i2c_device_id ina2xx_id[] = {
{"ina231", ina226},
{}
};
-
MODULE_DEVICE_TABLE(i2c, ina2xx_id);
static struct i2c_driver ina2xx_driver = {
@@ -737,7 +734,6 @@ static struct i2c_driver ina2xx_driver = {
.remove = ina2xx_remove,
.id_table = ina2xx_id,
};
-
module_i2c_driver(ina2xx_driver);
MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
--
2.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments
2016-02-12 18:34 [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Andrew F. Davis
@ 2016-02-12 18:34 ` Andrew F. Davis
2016-02-13 13:21 ` Jonathan Cameron
2016-02-13 12:55 ` [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Jonathan Cameron
1 sibling, 1 reply; 8+ messages in thread
From: Andrew F. Davis @ 2016-02-12 18:34 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel, Andrew F. Davis
These are generally for devlopment use only, remove these
from performance-critical code, convert to dev_dbg elswhere.
Signed-off-by: Andrew F. Davis <afd@ti.com>
---
drivers/iio/adc/ina2xx-adc.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 61e8ae9..ba11b2e 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -440,7 +440,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
unsigned short data[8];
int bit, ret, i = 0;
- unsigned long buffer_us, elapsed_us;
s64 time_a, time_b;
unsigned int alert;
@@ -464,8 +463,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
return ret;
alert &= INA266_CVRF;
- trace_printk("Conversion ready: %d\n", !!alert);
-
} while (!alert);
/*
@@ -490,14 +487,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
iio_push_to_buffers_with_timestamp(indio_dev,
(unsigned int *)data, time_a);
- buffer_us = (unsigned long)(time_b - time_a) / 1000;
- elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
-
- trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
-
chip->prev_ns = time_a;
- return buffer_us;
+ return (unsigned long)(time_b - time_a) / 1000;
};
static int ina2xx_capture_thread(void *data)
@@ -532,12 +524,13 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
struct ina2xx_chip_info *chip = iio_priv(indio_dev);
unsigned int sampling_us = SAMPLING_PERIOD(chip);
- trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
- (unsigned int)(*indio_dev->active_scan_mask),
- 1000000/sampling_us, chip->avg);
+ dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
+ (unsigned int)(*indio_dev->active_scan_mask),
+ 1000000 / sampling_us, chip->avg);
- trace_printk("Expected work period: %u us\n", sampling_us);
- trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
+ dev_dbg(&indio_dev->dev, "Expected work period: %u us\n", sampling_us);
+ dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
+ chip->allow_async_readout);
chip->prev_ns = iio_get_time_ns();
--
2.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code
2016-02-12 18:34 [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Andrew F. Davis
2016-02-12 18:34 ` [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments Andrew F. Davis
@ 2016-02-13 12:55 ` Jonathan Cameron
2016-02-14 20:44 ` Andrew F. Davis
1 sibling, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2016-02-13 12:55 UTC (permalink / raw)
To: Andrew F. Davis, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel
On 12/02/16 18:34, Andrew F. Davis wrote:
> Group of probably overly rigorous whitespace and code cleanups.
> - Alphabetize includes
> - Assign to variables in the order they are defined
> - Alignment issues
> - Group alike statements together
> - Use helper macros
>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
Definitely overzealous in some cases - but some good stuff in here
as well. Some of this just adds noise for no real gain. From the point
of view of bisection etc - we have to balance the possible cost of the
more minor cleanups against their benefits.
I'll run through and give my opinion on which ones are worthwhile etc.
Quite a few are marginal, but I feel fairly strongly against one of the
changes in alignment of the \ in the large macro.
Jonathan
> ---
> drivers/iio/adc/ina2xx-adc.c | 164 +++++++++++++++++++++----------------------
> 1 file changed, 80 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index d803e50..61e8ae9 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -19,17 +19,18 @@
> *
> * Configurable 7-bit I2C slave address from 0x40 to 0x4F
> */
> -#include <linux/module.h>
> -#include <linux/kthread.h>
> +
> #include <linux/delay.h>
> +#include <linux/i2c.h>
> #include <linux/iio/kfifo_buf.h>
> #include <linux/iio/sysfs.h>
> -#include <linux/i2c.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> #include <linux/regmap.h>
> -#include <linux/platform_data/ina2xx.h>
> -
> #include <linux/util_macros.h>
There is sometimes some logical grouping going on in the way the author
decides to put the headers in, here it's not that well defined that
I can see. However, there is no real benefit in alphabetical order either
though clearly some other projects disagree and do mandate this...
>
> +#include <linux/platform_data/ina2xx.h>
> +
> /* INA2XX registers definition */
> #define INA2XX_CONFIG 0x00
> #define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
> @@ -38,7 +39,7 @@
> #define INA2XX_CURRENT 0x04 /* readonly */
> #define INA2XX_CALIBRATION 0x05
>
> -#define INA226_ALERT_MASK 0x06
> +#define INA226_ALERT_MASK GENMASK(2, 1)
> #define INA266_CVRF BIT(3)
>
> #define INA2XX_MAX_REGISTERS 8
> @@ -113,7 +114,7 @@ struct ina2xx_chip_info {
> struct mutex state_lock;
> unsigned int shunt_resistor;
> int avg;
> - s64 prev_ns; /* track buffer capture time, check for underruns*/
> + s64 prev_ns; /* track buffer capture time, check for underruns*/
Fair enough, though a space before the */ would be nice!
> int int_time_vbus; /* Bus voltage integration time uS */
> int int_time_vshunt; /* Shunt voltage integration time uS */
> bool allow_async_readout;
> @@ -121,21 +122,21 @@ struct ina2xx_chip_info {
>
> static const struct ina2xx_config ina2xx_config[] = {
> [ina219] = {
I'd have left this indenting alone. It's more of a matter of taste
than any hard and fast rule. I'd do indenting myself the way
you have, but it's not worth the noise to change it.
> - .config_default = INA219_CONFIG_DEFAULT,
> - .calibration_factor = 40960000,
> - .shunt_div = 100,
> - .bus_voltage_shift = 3,
> - .bus_voltage_lsb = 4000,
> - .power_lsb = 20000,
> - },
> + .config_default = INA219_CONFIG_DEFAULT,
> + .calibration_factor = 40960000,
> + .shunt_div = 100,
> + .bus_voltage_shift = 3,
> + .bus_voltage_lsb = 4000,
> + .power_lsb = 20000,
> + },
> [ina226] = {
> - .config_default = INA226_CONFIG_DEFAULT,
> - .calibration_factor = 5120000,
> - .shunt_div = 400,
> - .bus_voltage_shift = 0,
> - .bus_voltage_lsb = 1250,
> - .power_lsb = 25000,
> - },
> + .config_default = INA226_CONFIG_DEFAULT,
> + .calibration_factor = 5120000,
> + .shunt_div = 400,
> + .bus_voltage_shift = 0,
> + .bus_voltage_lsb = 1250,
> + .power_lsb = 25000,
> + },
> };
>
> static int ina2xx_read_raw(struct iio_dev *indio_dev,
> @@ -149,7 +150,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> ret = regmap_read(chip->regmap, chan->address, ®val);
> - if (ret < 0)
> + if (ret)
> return ret;
These are good to clean up.
>
> if (is_signed_reg(chan->address))
> @@ -251,7 +252,7 @@ static int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
> return -EINVAL;
>
> bits = find_closest(val_us, ina226_conv_time_tab,
> - ARRAY_SIZE(ina226_conv_time_tab));
> + ARRAY_SIZE(ina226_conv_time_tab));
Good to get these nicely aligned as well.
>
> chip->int_time_vbus = ina226_conv_time_tab[bits];
>
> @@ -270,7 +271,7 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
> return -EINVAL;
>
> bits = find_closest(val_us, ina226_conv_time_tab,
> - ARRAY_SIZE(ina226_conv_time_tab));
> + ARRAY_SIZE(ina226_conv_time_tab));
>
> chip->int_time_vshunt = ina226_conv_time_tab[bits];
>
> @@ -285,8 +286,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
> int val, int val2, long mask)
> {
> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> - int ret;
> unsigned int config, tmp;
> + int ret;
Definitely in the doesn't matter category, but if you really want to this
one is fine.
>
> if (iio_buffer_enabled(indio_dev))
> return -EBUSY;
> @@ -294,8 +295,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
> mutex_lock(&chip->state_lock);
>
> ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
> - if (ret < 0)
> - goto _err;
> + if (ret)
> + goto err;
Good.
>
> tmp = config;
>
> @@ -310,19 +311,19 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
> else
> ret = ina226_set_int_time_vbus(chip, val2, &tmp);
> break;
> +
Maybe a slight gain in readability so fair enough.
> default:
> ret = -EINVAL;
> }
>
> if (!ret && (tmp != config))
> ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
> -_err:
> +err:
Fine.
> mutex_unlock(&chip->state_lock);
>
> return ret;
> }
>
> -
> static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -355,6 +356,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
> return -EINVAL;
>
> chip->shunt_resistor = val;
> +
good.
> return 0;
> }
>
> @@ -408,21 +410,21 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
> * Sampling Freq is a consequence of the integration times of
> * the Voltage channels.
> */
> -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> - .type = IIO_VOLTAGE, \
> - .address = (_address), \
> - .indexed = 1, \
> - .channel = (_index), \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> - BIT(IIO_CHAN_INFO_SCALE) | \
> - BIT(IIO_CHAN_INFO_INT_TIME), \
> - .scan_index = (_index), \
> - .scan_type = { \
> - .sign = 'u', \
> - .realbits = 16, \
> - .storagebits = 16, \
> - .endianness = IIO_LE, \
> - } \
This one I disagree with. It too often leads to noise when we end up with
an element with a line and have to change the spacing before the \s.
Much less churn occurs with teh version as original defined.
As someone who handles a fair number of patch reviews I feel pretty strongly
about anything that leads to more churn.
> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
> + .type = IIO_VOLTAGE, \
> + .address = (_address), \
> + .indexed = 1, \
> + .channel = (_index), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_INT_TIME), \
> + .scan_index = (_index), \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + } \
> }
>
> static const struct iio_chan_spec ina2xx_channels[] = {
> @@ -500,7 +502,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>
> static int ina2xx_capture_thread(void *data)
> {
> - struct iio_dev *indio_dev = (struct iio_dev *)data;
> + struct iio_dev *indio_dev = data;
good.
> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> unsigned int sampling_us = SAMPLING_PERIOD(chip);
> int buffer_us;
> @@ -575,8 +577,7 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
> }
>
> /* Possible integration times for vshunt and vbus */
> -static IIO_CONST_ATTR_INT_TIME_AVAIL \
> - ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>
> static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
> ina2xx_allow_async_readout_show,
> @@ -598,21 +599,23 @@ static const struct attribute_group ina2xx_attribute_group = {
> };
>
> static const struct iio_info ina2xx_info = {
> - .debugfs_reg_access = &ina2xx_debug_reg,
> - .read_raw = &ina2xx_read_raw,
> - .write_raw = &ina2xx_write_raw,
> - .attrs = &ina2xx_attribute_group,
> .driver_module = THIS_MODULE,
> + .attrs = &ina2xx_attribute_group,
> + .read_raw = ina2xx_read_raw,
> + .write_raw = ina2xx_write_raw,
> + .debugfs_reg_access = ina2xx_debug_reg,
sensible change.
> };
>
> /* Initialize the configuration and calibration registers. */
> static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> {
> u16 regval;
> - int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> + int ret;
This form is considered acceptable in kernel code - but perhaps yours is
a tiny bit more readable so if you want to keep this one.
>
> - if (ret < 0)
> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
> + if (ret)
> return ret;
good
> +
> /*
> * Set current LSB to 1mA, shunt is in uOhms
> * (equation 13 in datasheet). We hardcode a Current_LSB
> @@ -621,7 +624,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
> * to the user for now.
> */
> regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
> - chip->shunt_resistor);
> + chip->shunt_resistor);
good
>
> return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
> }
> @@ -632,8 +635,8 @@ static int ina2xx_probe(struct i2c_client *client,
> struct ina2xx_chip_info *chip;
> struct iio_dev *indio_dev;
> struct iio_buffer *buffer;
> - int ret;
> unsigned int val;
> + int ret;
Again, don't care on this one.
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> if (!indio_dev)
> @@ -641,8 +644,19 @@ static int ina2xx_probe(struct i2c_client *client,
>
> chip = iio_priv(indio_dev);
>
All this reordering needs a detailed justification. It adds a lot of churn
for limited benefit. I prefer the ordering you end up with, but is it
worth the noise? Not to my mind.
> + /* This is only used for device removal purposes. */
> + i2c_set_clientdata(client, indio_dev);
> +
> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> + if (IS_ERR(chip->regmap)) {
> + dev_err(&client->dev, "failed to allocate register map\n");
> + return PTR_ERR(chip->regmap);
> + }
> +
> chip->config = &ina2xx_config[id->driver_data];
>
> + mutex_init(&chip->state_lock);
> +
> if (of_property_read_u32(client->dev.of_node,
> "shunt-resistor", &val) < 0) {
> struct ina2xx_platform_data *pdata =
> @@ -658,25 +672,6 @@ static int ina2xx_probe(struct i2c_client *client,
> if (ret)
> return ret;
>
> - mutex_init(&chip->state_lock);
> -
> - /* This is only used for device removal purposes. */
> - i2c_set_clientdata(client, indio_dev);
> -
> - indio_dev->name = id->name;
> - indio_dev->channels = ina2xx_channels;
> - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> -
> - indio_dev->dev.parent = &client->dev;
> - indio_dev->info = &ina2xx_info;
> - indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> -
> - chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
> - if (IS_ERR(chip->regmap)) {
> - dev_err(&client->dev, "failed to allocate register map\n");
> - return PTR_ERR(chip->regmap);
> - }
> -
> /* Patch the current config register with default. */
> val = chip->config->config_default;
>
> @@ -687,24 +682,28 @@ static int ina2xx_probe(struct i2c_client *client,
> }
>
> ret = ina2xx_init(chip, val);
> - if (ret < 0) {
> - dev_err(&client->dev, "error configuring the device: %d\n",
> - ret);
> - return -ENODEV;
> + if (ret) {
This change is good however.
> + dev_err(&client->dev, "error configuring the device\n");
Dropping the return value might be 'cleaner' in some sense, but I don't think
it's worth making the change.
> + return ret;
> }
>
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->channels = ina2xx_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
> + indio_dev->name = id->name;
> + indio_dev->info = &ina2xx_info;
> + indio_dev->setup_ops = &ina2xx_setup_ops;
> +
> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> if (!buffer)
> return -ENOMEM;
>
> - indio_dev->setup_ops = &ina2xx_setup_ops;
> -
> iio_device_attach_buffer(indio_dev, buffer);
>
> return iio_device_register(indio_dev);
> }
>
> -
> static int ina2xx_remove(struct i2c_client *client)
> {
> struct iio_dev *indio_dev = i2c_get_clientdata(client);
> @@ -717,7 +716,6 @@ static int ina2xx_remove(struct i2c_client *client)
> INA2XX_MODE_MASK, 0);
> }
>
> -
good.
> static const struct i2c_device_id ina2xx_id[] = {
> {"ina219", ina219},
> {"ina220", ina219},
> @@ -726,7 +724,6 @@ static const struct i2c_device_id ina2xx_id[] = {
> {"ina231", ina226},
> {}
> };
> -
good
> MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>
> static struct i2c_driver ina2xx_driver = {
> @@ -737,7 +734,6 @@ static struct i2c_driver ina2xx_driver = {
> .remove = ina2xx_remove,
> .id_table = ina2xx_id,
> };
> -
good as well.
> module_i2c_driver(ina2xx_driver);
>
> MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments
2016-02-12 18:34 ` [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments Andrew F. Davis
@ 2016-02-13 13:21 ` Jonathan Cameron
2016-02-14 20:02 ` Andrew F. Davis
0 siblings, 1 reply; 8+ messages in thread
From: Jonathan Cameron @ 2016-02-13 13:21 UTC (permalink / raw)
To: Andrew F. Davis, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel
On 12/02/16 18:34, Andrew F. Davis wrote:
> These are generally for devlopment use only, remove these
> from performance-critical code, convert to dev_dbg elswhere.
>
> Signed-off-by: Andrew F. Davis <afd@ti.com>
Hm... Tracepoints are also somewhat considered to be ABI and
hence it is possible some tooling relies on them. Also they
are very nearly free when not enabled.
The fundamental reason they are here it to allow checking of whether
the thread is ticking along fast enough to keep up with the incoming data.
Can see this being useful on live platforms to debug sampling issues.
What do others think about this change?
Andrew, what is driving your wish to change this?
Jonathan
> ---
> drivers/iio/adc/ina2xx-adc.c | 21 +++++++--------------
> 1 file changed, 7 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
> index 61e8ae9..ba11b2e 100644
> --- a/drivers/iio/adc/ina2xx-adc.c
> +++ b/drivers/iio/adc/ina2xx-adc.c
> @@ -440,7 +440,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> unsigned short data[8];
> int bit, ret, i = 0;
> - unsigned long buffer_us, elapsed_us;
> s64 time_a, time_b;
> unsigned int alert;
>
> @@ -464,8 +463,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> return ret;
>
> alert &= INA266_CVRF;
> - trace_printk("Conversion ready: %d\n", !!alert);
> -
> } while (!alert);
>
> /*
> @@ -490,14 +487,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
> iio_push_to_buffers_with_timestamp(indio_dev,
> (unsigned int *)data, time_a);
>
> - buffer_us = (unsigned long)(time_b - time_a) / 1000;
> - elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
> -
> - trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
> -
> chip->prev_ns = time_a;
>
> - return buffer_us;
> + return (unsigned long)(time_b - time_a) / 1000;
> };
>
> static int ina2xx_capture_thread(void *data)
> @@ -532,12 +524,13 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
> unsigned int sampling_us = SAMPLING_PERIOD(chip);
>
> - trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> - (unsigned int)(*indio_dev->active_scan_mask),
> - 1000000/sampling_us, chip->avg);
> + dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
> + (unsigned int)(*indio_dev->active_scan_mask),
> + 1000000 / sampling_us, chip->avg);
>
> - trace_printk("Expected work period: %u us\n", sampling_us);
> - trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
> + dev_dbg(&indio_dev->dev, "Expected work period: %u us\n", sampling_us);
> + dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
> + chip->allow_async_readout);
>
> chip->prev_ns = iio_get_time_ns();
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments
2016-02-13 13:21 ` Jonathan Cameron
@ 2016-02-14 20:02 ` Andrew F. Davis
2016-02-15 9:08 ` Marc Titinger
0 siblings, 1 reply; 8+ messages in thread
From: Andrew F. Davis @ 2016-02-14 20:02 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel
On 02/13/2016 07:21 AM, Jonathan Cameron wrote:
> On 12/02/16 18:34, Andrew F. Davis wrote:
>> These are generally for devlopment use only, remove these
>> from performance-critical code, convert to dev_dbg elswhere.
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Hm... Tracepoints are also somewhat considered to be ABI and
> hence it is possible some tooling relies on them. Also they
> are very nearly free when not enabled.
>
> The fundamental reason they are here it to allow checking of whether
> the thread is ticking along fast enough to keep up with the incoming data.
> Can see this being useful on live platforms to debug sampling issues.
>
This looks more like development testing statements to see if the delay timer
adjustment algorithm is working well. If one is debugging sampling issues,
then they are always free to add any extra debugging lines they need.
> What do others think about this change?
>
> Andrew, what is driving your wish to change this?
>
v4.5-rc3 include/linux/kernel.h +609:
"This is intended as a debugging tool for the developer only.
Please refrain from leaving trace_printks scattered around in
your code."
> Jonathan
>> ---
>> drivers/iio/adc/ina2xx-adc.c | 21 +++++++--------------
>> 1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index 61e8ae9..ba11b2e 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -440,7 +440,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> unsigned short data[8];
>> int bit, ret, i = 0;
>> - unsigned long buffer_us, elapsed_us;
>> s64 time_a, time_b;
>> unsigned int alert;
>>
>> @@ -464,8 +463,6 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> return ret;
>>
>> alert &= INA266_CVRF;
>> - trace_printk("Conversion ready: %d\n", !!alert);
>> -
>> } while (!alert);
>>
>> /*
>> @@ -490,14 +487,9 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>> iio_push_to_buffers_with_timestamp(indio_dev,
>> (unsigned int *)data, time_a);
>>
>> - buffer_us = (unsigned long)(time_b - time_a) / 1000;
>> - elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
>> -
>> - trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us, buffer_us);
>> -
>> chip->prev_ns = time_a;
>>
>> - return buffer_us;
>> + return (unsigned long)(time_b - time_a) / 1000;
>> };
>>
>> static int ina2xx_capture_thread(void *data)
>> @@ -532,12 +524,13 @@ static int ina2xx_buffer_enable(struct iio_dev *indio_dev)
>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> unsigned int sampling_us = SAMPLING_PERIOD(chip);
>>
>> - trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
>> - (unsigned int)(*indio_dev->active_scan_mask),
>> - 1000000/sampling_us, chip->avg);
>> + dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x, freq = %d, avg =%u\n",
>> + (unsigned int)(*indio_dev->active_scan_mask),
>> + 1000000 / sampling_us, chip->avg);
>>
>> - trace_printk("Expected work period: %u us\n", sampling_us);
>> - trace_printk("Async readout mode: %d\n", chip->allow_async_readout);
>> + dev_dbg(&indio_dev->dev, "Expected work period: %u us\n", sampling_us);
>> + dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
>> + chip->allow_async_readout);
>>
>> chip->prev_ns = iio_get_time_ns();
>>
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code
2016-02-13 12:55 ` [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Jonathan Cameron
@ 2016-02-14 20:44 ` Andrew F. Davis
2016-02-17 19:29 ` Jonathan Cameron
0 siblings, 1 reply; 8+ messages in thread
From: Andrew F. Davis @ 2016-02-14 20:44 UTC (permalink / raw)
To: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel
On 02/13/2016 06:55 AM, Jonathan Cameron wrote:
> On 12/02/16 18:34, Andrew F. Davis wrote:
>> Group of probably overly rigorous whitespace and code cleanups.
>> - Alphabetize includes
>> - Assign to variables in the order they are defined
>> - Alignment issues
>> - Group alike statements together
>> - Use helper macros
>>
>> Signed-off-by: Andrew F. Davis <afd@ti.com>
> Definitely overzealous in some cases - but some good stuff in here
> as well. Some of this just adds noise for no real gain. From the point
> of view of bisection etc - we have to balance the possible cost of the
> more minor cleanups against their benefits.
>
My original plan was to push these last cycle so they would come in
with the drivers creation patches.
I still would argue this driver is new enough that not many trees
have made changes to this that would complicate rebasing back onto
mainline with these changes on top. This is the bottom of a series
with some more important changes I will not have time this window
to get in, so I pushed these early to keep the delta low if anyone
else starts working on this driver.
And at any rate, I would say code should be the #1 priority, being
afraid to change code for the sake of keeping the source control tools
happy seems a bit backwards to me.
> I'll run through and give my opinion on which ones are worthwhile etc.
> Quite a few are marginal, but I feel fairly strongly against one of the
> changes in alignment of the \ in the large macro.
>
> Jonathan
>> ---
>> drivers/iio/adc/ina2xx-adc.c | 164 +++++++++++++++++++++----------------------
>> 1 file changed, 80 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>> index d803e50..61e8ae9 100644
>> --- a/drivers/iio/adc/ina2xx-adc.c
>> +++ b/drivers/iio/adc/ina2xx-adc.c
>> @@ -19,17 +19,18 @@
>> *
>> * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>> */
>> -#include <linux/module.h>
>> -#include <linux/kthread.h>
>> +
>> #include <linux/delay.h>
>> +#include <linux/i2c.h>
>> #include <linux/iio/kfifo_buf.h>
>> #include <linux/iio/sysfs.h>
>> -#include <linux/i2c.h>
>> +#include <linux/kthread.h>
>> +#include <linux/module.h>
>> #include <linux/regmap.h>
>> -#include <linux/platform_data/ina2xx.h>
>> -
>> #include <linux/util_macros.h>
> There is sometimes some logical grouping going on in the way the author
> decides to put the headers in, here it's not that well defined that
> I can see. However, there is no real benefit in alphabetical order either
> though clearly some other projects disagree and do mandate this...
Up to you then, but you can see below I have grouped the non-kernel-common
headers out, so this would add to any logical grouping.
>>
>> +#include <linux/platform_data/ina2xx.h>
>> +
>> /* INA2XX registers definition */
>> #define INA2XX_CONFIG 0x00
>> #define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
>> @@ -38,7 +39,7 @@
>> #define INA2XX_CURRENT 0x04 /* readonly */
>> #define INA2XX_CALIBRATION 0x05
>>
>> -#define INA226_ALERT_MASK 0x06
>> +#define INA226_ALERT_MASK GENMASK(2, 1)
>> #define INA266_CVRF BIT(3)
>>
>> #define INA2XX_MAX_REGISTERS 8
>> @@ -113,7 +114,7 @@ struct ina2xx_chip_info {
>> struct mutex state_lock;
>> unsigned int shunt_resistor;
>> int avg;
>> - s64 prev_ns; /* track buffer capture time, check for underruns*/
>> + s64 prev_ns; /* track buffer capture time, check for underruns*/
> Fair enough, though a space before the */ would be nice!
>> int int_time_vbus; /* Bus voltage integration time uS */
>> int int_time_vshunt; /* Shunt voltage integration time uS */
>> bool allow_async_readout;
>> @@ -121,21 +122,21 @@ struct ina2xx_chip_info {
>>
>> static const struct ina2xx_config ina2xx_config[] = {
>> [ina219] = {
> I'd have left this indenting alone. It's more of a matter of taste
> than any hard and fast rule. I'd do indenting myself the way
> you have, but it's not worth the noise to change it.
To me this should be considered against the style guidelines on indentation.
It's almost like
while (something) {
code;
code;
}
Auto formatting tools will make this change as well, so it may get fixed someday
anyway if any checks for this are added to checkpatch.
>> - .config_default = INA219_CONFIG_DEFAULT,
>> - .calibration_factor = 40960000,
>> - .shunt_div = 100,
>> - .bus_voltage_shift = 3,
>> - .bus_voltage_lsb = 4000,
>> - .power_lsb = 20000,
>> - },
>> + .config_default = INA219_CONFIG_DEFAULT,
>> + .calibration_factor = 40960000,
>> + .shunt_div = 100,
>> + .bus_voltage_shift = 3,
>> + .bus_voltage_lsb = 4000,
>> + .power_lsb = 20000,
>> + },
>> [ina226] = {
>> - .config_default = INA226_CONFIG_DEFAULT,
>> - .calibration_factor = 5120000,
>> - .shunt_div = 400,
>> - .bus_voltage_shift = 0,
>> - .bus_voltage_lsb = 1250,
>> - .power_lsb = 25000,
>> - },
>> + .config_default = INA226_CONFIG_DEFAULT,
>> + .calibration_factor = 5120000,
>> + .shunt_div = 400,
>> + .bus_voltage_shift = 0,
>> + .bus_voltage_lsb = 1250,
>> + .power_lsb = 25000,
>> + },
>> };
>>
>> static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> @@ -149,7 +150,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>> switch (mask) {
>> case IIO_CHAN_INFO_RAW:
>> ret = regmap_read(chip->regmap, chan->address, ®val);
>> - if (ret < 0)
>> + if (ret)
>> return ret;
> These are good to clean up.
>>
>> if (is_signed_reg(chan->address))
>> @@ -251,7 +252,7 @@ static int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
>> return -EINVAL;
>>
>> bits = find_closest(val_us, ina226_conv_time_tab,
>> - ARRAY_SIZE(ina226_conv_time_tab));
>> + ARRAY_SIZE(ina226_conv_time_tab));
> Good to get these nicely aligned as well.
>>
>> chip->int_time_vbus = ina226_conv_time_tab[bits];
>>
>> @@ -270,7 +271,7 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>> return -EINVAL;
>>
>> bits = find_closest(val_us, ina226_conv_time_tab,
>> - ARRAY_SIZE(ina226_conv_time_tab));
>> + ARRAY_SIZE(ina226_conv_time_tab));
>>
>> chip->int_time_vshunt = ina226_conv_time_tab[bits];
>>
>> @@ -285,8 +286,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>> int val, int val2, long mask)
>> {
>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> - int ret;
>> unsigned int config, tmp;
>> + int ret;
> Definitely in the doesn't matter category, but if you really want to this
> one is fine.
>>
>> if (iio_buffer_enabled(indio_dev))
>> return -EBUSY;
>> @@ -294,8 +295,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>> mutex_lock(&chip->state_lock);
>>
>> ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>> - if (ret < 0)
>> - goto _err;
>> + if (ret)
>> + goto err;
> Good.
>>
>> tmp = config;
>>
>> @@ -310,19 +311,19 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>> else
>> ret = ina226_set_int_time_vbus(chip, val2, &tmp);
>> break;
>> +
> Maybe a slight gain in readability so fair enough.
>> default:
>> ret = -EINVAL;
>> }
>>
>> if (!ret && (tmp != config))
>> ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>> -_err:
>> +err:
> Fine.
>> mutex_unlock(&chip->state_lock);
>>
>> return ret;
>> }
>>
>> -
>> static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
>> struct device_attribute *attr,
>> char *buf)
>> @@ -355,6 +356,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>> return -EINVAL;
>>
>> chip->shunt_resistor = val;
>> +
> good.
>> return 0;
>> }
>>
>> @@ -408,21 +410,21 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>> * Sampling Freq is a consequence of the integration times of
>> * the Voltage channels.
>> */
>> -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> - .type = IIO_VOLTAGE, \
>> - .address = (_address), \
>> - .indexed = 1, \
>> - .channel = (_index), \
>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> - BIT(IIO_CHAN_INFO_SCALE) | \
>> - BIT(IIO_CHAN_INFO_INT_TIME), \
>> - .scan_index = (_index), \
>> - .scan_type = { \
>> - .sign = 'u', \
>> - .realbits = 16, \
>> - .storagebits = 16, \
>> - .endianness = IIO_LE, \
>> - } \
> This one I disagree with. It too often leads to noise when we end up with
> an element with a line and have to change the spacing before the \s.
> Much less churn occurs with teh version as original defined.
> As someone who handles a fair number of patch reviews I feel pretty strongly
> about anything that leads to more churn.
>
That's fine, this one I changed to I could see what was going on
in didn't really care how it is done, I think I even left the
other macro like this unchanged, so this change can be dropped.
>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>> + .type = IIO_VOLTAGE, \
>> + .address = (_address), \
>> + .indexed = 1, \
>> + .channel = (_index), \
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | \
>> + BIT(IIO_CHAN_INFO_INT_TIME), \
>> + .scan_index = (_index), \
>> + .scan_type = { \
>> + .sign = 'u', \
>> + .realbits = 16, \
>> + .storagebits = 16, \
>> + .endianness = IIO_LE, \
>> + } \
>> }
>>
>> static const struct iio_chan_spec ina2xx_channels[] = {
>> @@ -500,7 +502,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>>
>> static int ina2xx_capture_thread(void *data)
>> {
>> - struct iio_dev *indio_dev = (struct iio_dev *)data;
>> + struct iio_dev *indio_dev = data;
> good.
>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>> unsigned int sampling_us = SAMPLING_PERIOD(chip);
>> int buffer_us;
>> @@ -575,8 +577,7 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>> }
>>
>> /* Possible integration times for vshunt and vbus */
>> -static IIO_CONST_ATTR_INT_TIME_AVAIL \
>> - ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>>
>> static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>> ina2xx_allow_async_readout_show,
>> @@ -598,21 +599,23 @@ static const struct attribute_group ina2xx_attribute_group = {
>> };
>>
>> static const struct iio_info ina2xx_info = {
>> - .debugfs_reg_access = &ina2xx_debug_reg,
>> - .read_raw = &ina2xx_read_raw,
>> - .write_raw = &ina2xx_write_raw,
>> - .attrs = &ina2xx_attribute_group,
>> .driver_module = THIS_MODULE,
>> + .attrs = &ina2xx_attribute_group,
>> + .read_raw = ina2xx_read_raw,
>> + .write_raw = ina2xx_write_raw,
>> + .debugfs_reg_access = ina2xx_debug_reg,
> sensible change.
>> };
>>
>> /* Initialize the configuration and calibration registers. */
>> static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>> {
>> u16 regval;
>> - int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>> + int ret;
> This form is considered acceptable in kernel code - but perhaps yours is
> a tiny bit more readable so if you want to keep this one.
>>
>> - if (ret < 0)
>> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>> + if (ret)
>> return ret;
> good
>> +
>> /*
>> * Set current LSB to 1mA, shunt is in uOhms
>> * (equation 13 in datasheet). We hardcode a Current_LSB
>> @@ -621,7 +624,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>> * to the user for now.
>> */
>> regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>> - chip->shunt_resistor);
>> + chip->shunt_resistor);
> good
>>
>> return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>> }
>> @@ -632,8 +635,8 @@ static int ina2xx_probe(struct i2c_client *client,
>> struct ina2xx_chip_info *chip;
>> struct iio_dev *indio_dev;
>> struct iio_buffer *buffer;
>> - int ret;
>> unsigned int val;
>> + int ret;
> Again, don't care on this one.
>>
>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>> if (!indio_dev)
>> @@ -641,8 +644,19 @@ static int ina2xx_probe(struct i2c_client *client,
>>
>> chip = iio_priv(indio_dev);
>>
> All this reordering needs a detailed justification. It adds a lot of churn
> for limited benefit. I prefer the ordering you end up with, but is it
> worth the noise? Not to my mind.
I would say it is, keeping the initialization steps logically ordered and
grouped, as opposed to randomly mixed, is very important for bug finding
and for easier bug spotting during future development, it's also easier to
read and follow/comprehend.
>> + /* This is only used for device removal purposes. */
>> + i2c_set_clientdata(client, indio_dev);
>> +
>> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>> + if (IS_ERR(chip->regmap)) {
>> + dev_err(&client->dev, "failed to allocate register map\n");
>> + return PTR_ERR(chip->regmap);
>> + }
>> +
>> chip->config = &ina2xx_config[id->driver_data];
>>
>> + mutex_init(&chip->state_lock);
>> +
>> if (of_property_read_u32(client->dev.of_node,
>> "shunt-resistor", &val) < 0) {
>> struct ina2xx_platform_data *pdata =
>> @@ -658,25 +672,6 @@ static int ina2xx_probe(struct i2c_client *client,
>> if (ret)
>> return ret;
>>
>> - mutex_init(&chip->state_lock);
>> -
>> - /* This is only used for device removal purposes. */
>> - i2c_set_clientdata(client, indio_dev);
>> -
>> - indio_dev->name = id->name;
>> - indio_dev->channels = ina2xx_channels;
>> - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>> -
>> - indio_dev->dev.parent = &client->dev;
>> - indio_dev->info = &ina2xx_info;
>> - indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> -
>> - chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>> - if (IS_ERR(chip->regmap)) {
>> - dev_err(&client->dev, "failed to allocate register map\n");
>> - return PTR_ERR(chip->regmap);
>> - }
>> -
>> /* Patch the current config register with default. */
>> val = chip->config->config_default;
>>
>> @@ -687,24 +682,28 @@ static int ina2xx_probe(struct i2c_client *client,
>> }
>>
>> ret = ina2xx_init(chip, val);
>> - if (ret < 0) {
>> - dev_err(&client->dev, "error configuring the device: %d\n",
>> - ret);
>> - return -ENODEV;
>> + if (ret) {
> This change is good however.
>
>> + dev_err(&client->dev, "error configuring the device\n");
> Dropping the return value might be 'cleaner' in some sense, but I don't think
> it's worth making the change.
We are returning the error code, so informing the user of its number every
step in the error path leads to a lot of noise, we often end up with the
number printed three or more times. I think it should only get printed by
the last step in the chain that eats the number by not returning it though
to another step.
>> + return ret;
>> }
>>
>> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>> + indio_dev->dev.parent = &client->dev;
>> + indio_dev->channels = ina2xx_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>> + indio_dev->name = id->name;
>> + indio_dev->info = &ina2xx_info;
>> + indio_dev->setup_ops = &ina2xx_setup_ops;
>> +
>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>> if (!buffer)
>> return -ENOMEM;
>>
>> - indio_dev->setup_ops = &ina2xx_setup_ops;
>> -
>> iio_device_attach_buffer(indio_dev, buffer);
>>
>> return iio_device_register(indio_dev);
>> }
>>
>> -
>> static int ina2xx_remove(struct i2c_client *client)
>> {
>> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> @@ -717,7 +716,6 @@ static int ina2xx_remove(struct i2c_client *client)
>> INA2XX_MODE_MASK, 0);
>> }
>>
>> -
> good.
>> static const struct i2c_device_id ina2xx_id[] = {
>> {"ina219", ina219},
>> {"ina220", ina219},
>> @@ -726,7 +724,6 @@ static const struct i2c_device_id ina2xx_id[] = {
>> {"ina231", ina226},
>> {}
>> };
>> -
> good
>> MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>>
>> static struct i2c_driver ina2xx_driver = {
>> @@ -737,7 +734,6 @@ static struct i2c_driver ina2xx_driver = {
>> .remove = ina2xx_remove,
>> .id_table = ina2xx_id,
>> };
>> -
> good as well.
>> module_i2c_driver(ina2xx_driver);
>>
>> MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments
2016-02-14 20:02 ` Andrew F. Davis
@ 2016-02-15 9:08 ` Marc Titinger
0 siblings, 0 replies; 8+ messages in thread
From: Marc Titinger @ 2016-02-15 9:08 UTC (permalink / raw)
To: Andrew F. Davis, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald
Cc: linux-iio, linux-kernel
On 14/02/2016 21:02, Andrew F. Davis wrote:
> On 02/13/2016 07:21 AM, Jonathan Cameron wrote:
>> On 12/02/16 18:34, Andrew F. Davis wrote:
>>> These are generally for devlopment use only, remove these
>>> from performance-critical code, convert to dev_dbg elswhere.
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Hm... Tracepoints are also somewhat considered to be ABI and
>> hence it is possible some tooling relies on them. Also they
>> are very nearly free when not enabled.
>>
>> The fundamental reason they are here it to allow checking of whether
>> the thread is ticking along fast enough to keep up with the incoming
>> data.
>> Can see this being useful on live platforms to debug sampling issues.
>>
>
> This looks more like development testing statements to see if the
> delay timer
> adjustment algorithm is working well. If one is debugging sampling
> issues,
> then they are always free to add any extra debugging lines they need.
Hi Andrew,
I've left it in in case I would get feedback with different i2c
controllers than the one of Omap
I've tested against, and would need to rework the mechanics of the data
capture (for instance if the i2c
message completion code did not use a threaded irq or stuff like that).
I guess the trace stuff this can be remove for now.
BR,
Marc.
>
>> What do others think about this change?
>>
>> Andrew, what is driving your wish to change this?
>>
>
> v4.5-rc3 include/linux/kernel.h +609:
>
> "This is intended as a debugging tool for the developer only.
> Please refrain from leaving trace_printks scattered around in
> your code."
>
>> Jonathan
>>> ---
>>> drivers/iio/adc/ina2xx-adc.c | 21 +++++++--------------
>>> 1 file changed, 7 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ina2xx-adc.c
>>> b/drivers/iio/adc/ina2xx-adc.c
>>> index 61e8ae9..ba11b2e 100644
>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>> @@ -440,7 +440,6 @@ static int ina2xx_work_buffer(struct iio_dev
>>> *indio_dev)
>>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> unsigned short data[8];
>>> int bit, ret, i = 0;
>>> - unsigned long buffer_us, elapsed_us;
>>> s64 time_a, time_b;
>>> unsigned int alert;
>>>
>>> @@ -464,8 +463,6 @@ static int ina2xx_work_buffer(struct iio_dev
>>> *indio_dev)
>>> return ret;
>>>
>>> alert &= INA266_CVRF;
>>> - trace_printk("Conversion ready: %d\n", !!alert);
>>> -
>>> } while (!alert);
>>>
>>> /*
>>> @@ -490,14 +487,9 @@ static int ina2xx_work_buffer(struct iio_dev
>>> *indio_dev)
>>> iio_push_to_buffers_with_timestamp(indio_dev,
>>> (unsigned int *)data, time_a);
>>>
>>> - buffer_us = (unsigned long)(time_b - time_a) / 1000;
>>> - elapsed_us = (unsigned long)(time_a - chip->prev_ns) / 1000;
>>> -
>>> - trace_printk("uS: elapsed: %lu, buf: %lu\n", elapsed_us,
>>> buffer_us);
>>> -
>>> chip->prev_ns = time_a;
>>>
>>> - return buffer_us;
>>> + return (unsigned long)(time_b - time_a) / 1000;
>>> };
>>>
>>> static int ina2xx_capture_thread(void *data)
>>> @@ -532,12 +524,13 @@ static int ina2xx_buffer_enable(struct iio_dev
>>> *indio_dev)
>>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> unsigned int sampling_us = SAMPLING_PERIOD(chip);
>>>
>>> - trace_printk("Enabling buffer w/ scan_mask %02x, freq = %d, avg
>>> =%u\n",
>>> - (unsigned int)(*indio_dev->active_scan_mask),
>>> - 1000000/sampling_us, chip->avg);
>>> + dev_dbg(&indio_dev->dev, "Enabling buffer w/ scan_mask %02x,
>>> freq = %d, avg =%u\n",
>>> + (unsigned int)(*indio_dev->active_scan_mask),
>>> + 1000000 / sampling_us, chip->avg);
>>>
>>> - trace_printk("Expected work period: %u us\n", sampling_us);
>>> - trace_printk("Async readout mode: %d\n",
>>> chip->allow_async_readout);
>>> + dev_dbg(&indio_dev->dev, "Expected work period: %u us\n",
>>> sampling_us);
>>> + dev_dbg(&indio_dev->dev, "Async readout mode: %d\n",
>>> + chip->allow_async_readout);
>>>
>>> chip->prev_ns = iio_get_time_ns();
>>>
>>>
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code
2016-02-14 20:44 ` Andrew F. Davis
@ 2016-02-17 19:29 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2016-02-17 19:29 UTC (permalink / raw)
To: Andrew F. Davis, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Marc Titinger
Cc: linux-iio, linux-kernel
On 14/02/16 20:44, Andrew F. Davis wrote:
> On 02/13/2016 06:55 AM, Jonathan Cameron wrote:
>> On 12/02/16 18:34, Andrew F. Davis wrote:
>>> Group of probably overly rigorous whitespace and code cleanups.
>>> - Alphabetize includes
>>> - Assign to variables in the order they are defined
>>> - Alignment issues
>>> - Group alike statements together
>>> - Use helper macros
>>>
>>> Signed-off-by: Andrew F. Davis <afd@ti.com>
>> Definitely overzealous in some cases - but some good stuff in here
>> as well. Some of this just adds noise for no real gain. From the point
>> of view of bisection etc - we have to balance the possible cost of the
>> more minor cleanups against their benefits.
>>
>
> My original plan was to push these last cycle so they would come in
> with the drivers creation patches.
Things would have been a little easier then, but still churn is a problem
from a maintenance point of view.
>
> I still would argue this driver is new enough that not many trees
> have made changes to this that would complicate rebasing back onto
> mainline with these changes on top. This is the bottom of a series
> with some more important changes I will not have time this window
> to get in, so I pushed these early to keep the delta low if anyone
> else starts working on this driver.
>
> And at any rate, I would say code should be the #1 priority, being
> afraid to change code for the sake of keeping the source control tools
> happy seems a bit backwards to me.
It's not about keeping the source controls happy, it's about keeping them
useful. Churn adds a high level of noise - it is far from costless.
It breaks the ability to workout where a bug was originally introduced
and it also makes back porting bug fixes effectively not happen. As far
as I know, no one has ever submitted a non trivial backport of an IIO bug
fix to stable. The ones that go in are the ones that the stable maintainers
can apply in a few seconds.
Some of the changes you make in here do not significantly improve the
code - that is the barrier that any patch must cross if it is to be applied.
Others are definitely worth the cost.
Jonathan
>
>> I'll run through and give my opinion on which ones are worthwhile etc.
>> Quite a few are marginal, but I feel fairly strongly against one of the
>> changes in alignment of the \ in the large macro.
>>
>> Jonathan
>>> ---
>>> drivers/iio/adc/ina2xx-adc.c | 164 +++++++++++++++++++++----------------------
>>> 1 file changed, 80 insertions(+), 84 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
>>> index d803e50..61e8ae9 100644
>>> --- a/drivers/iio/adc/ina2xx-adc.c
>>> +++ b/drivers/iio/adc/ina2xx-adc.c
>>> @@ -19,17 +19,18 @@
>>> *
>>> * Configurable 7-bit I2C slave address from 0x40 to 0x4F
>>> */
>>> -#include <linux/module.h>
>>> -#include <linux/kthread.h>
>>> +
>>> #include <linux/delay.h>
>>> +#include <linux/i2c.h>
>>> #include <linux/iio/kfifo_buf.h>
>>> #include <linux/iio/sysfs.h>
>>> -#include <linux/i2c.h>
>>> +#include <linux/kthread.h>
>>> +#include <linux/module.h>
>>> #include <linux/regmap.h>
>>> -#include <linux/platform_data/ina2xx.h>
>>> -
>>> #include <linux/util_macros.h>
>> There is sometimes some logical grouping going on in the way the author
>> decides to put the headers in, here it's not that well defined that
>> I can see. However, there is no real benefit in alphabetical order either
>> though clearly some other projects disagree and do mandate this...
>
> Up to you then, but you can see below I have grouped the non-kernel-common
> headers out, so this would add to any logical grouping.
This one is unlikely to cause maintenance issues so feel free.
>
>>>
>>> +#include <linux/platform_data/ina2xx.h>
>>> +
>>> /* INA2XX registers definition */
>>> #define INA2XX_CONFIG 0x00
>>> #define INA2XX_SHUNT_VOLTAGE 0x01 /* readonly */
>>> @@ -38,7 +39,7 @@
>>> #define INA2XX_CURRENT 0x04 /* readonly */
>>> #define INA2XX_CALIBRATION 0x05
>>>
>>> -#define INA226_ALERT_MASK 0x06
>>> +#define INA226_ALERT_MASK GENMASK(2, 1)
>>> #define INA266_CVRF BIT(3)
>>>
>>> #define INA2XX_MAX_REGISTERS 8
>>> @@ -113,7 +114,7 @@ struct ina2xx_chip_info {
>>> struct mutex state_lock;
>>> unsigned int shunt_resistor;
>>> int avg;
>>> - s64 prev_ns; /* track buffer capture time, check for underruns*/
>>> + s64 prev_ns; /* track buffer capture time, check for underruns*/
>> Fair enough, though a space before the */ would be nice!
>>> int int_time_vbus; /* Bus voltage integration time uS */
>>> int int_time_vshunt; /* Shunt voltage integration time uS */
>>> bool allow_async_readout;
>>> @@ -121,21 +122,21 @@ struct ina2xx_chip_info {
>>>
>>> static const struct ina2xx_config ina2xx_config[] = {
>>> [ina219] = {
>> I'd have left this indenting alone. It's more of a matter of taste
>> than any hard and fast rule. I'd do indenting myself the way
>> you have, but it's not worth the noise to change it.
>
> To me this should be considered against the style guidelines on indentation.
> It's almost like
>
> while (something) {
> code;
> code;
> }
>
> Auto formatting tools will make this change as well, so it may get fixed someday
> anyway if any checks for this are added to checkpatch.
That would first of all require the style guidelines to cover this case.
Right now they don't that I can identify.
Fine keep this one in.
>
>>> - .config_default = INA219_CONFIG_DEFAULT,
>>> - .calibration_factor = 40960000,
>>> - .shunt_div = 100,
>>> - .bus_voltage_shift = 3,
>>> - .bus_voltage_lsb = 4000,
>>> - .power_lsb = 20000,
>>> - },
>>> + .config_default = INA219_CONFIG_DEFAULT,
>>> + .calibration_factor = 40960000,
>>> + .shunt_div = 100,
>>> + .bus_voltage_shift = 3,
>>> + .bus_voltage_lsb = 4000,
>>> + .power_lsb = 20000,
>>> + },
>>> [ina226] = {
>>> - .config_default = INA226_CONFIG_DEFAULT,
>>> - .calibration_factor = 5120000,
>>> - .shunt_div = 400,
>>> - .bus_voltage_shift = 0,
>>> - .bus_voltage_lsb = 1250,
>>> - .power_lsb = 25000,
>>> - },
>>> + .config_default = INA226_CONFIG_DEFAULT,
>>> + .calibration_factor = 5120000,
>>> + .shunt_div = 400,
>>> + .bus_voltage_shift = 0,
>>> + .bus_voltage_lsb = 1250,
>>> + .power_lsb = 25000,
>>> + },
>>> };
>>>
>>> static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>> @@ -149,7 +150,7 @@ static int ina2xx_read_raw(struct iio_dev *indio_dev,
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> ret = regmap_read(chip->regmap, chan->address, ®val);
>>> - if (ret < 0)
>>> + if (ret)
>>> return ret;
>> These are good to clean up.
>>>
>>> if (is_signed_reg(chan->address))
>>> @@ -251,7 +252,7 @@ static int ina226_set_int_time_vbus(struct ina2xx_chip_info *chip,
>>> return -EINVAL;
>>>
>>> bits = find_closest(val_us, ina226_conv_time_tab,
>>> - ARRAY_SIZE(ina226_conv_time_tab));
>>> + ARRAY_SIZE(ina226_conv_time_tab));
>> Good to get these nicely aligned as well.
>>>
>>> chip->int_time_vbus = ina226_conv_time_tab[bits];
>>>
>>> @@ -270,7 +271,7 @@ static int ina226_set_int_time_vshunt(struct ina2xx_chip_info *chip,
>>> return -EINVAL;
>>>
>>> bits = find_closest(val_us, ina226_conv_time_tab,
>>> - ARRAY_SIZE(ina226_conv_time_tab));
>>> + ARRAY_SIZE(ina226_conv_time_tab));
>>>
>>> chip->int_time_vshunt = ina226_conv_time_tab[bits];
>>>
>>> @@ -285,8 +286,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>> int val, int val2, long mask)
>>> {
>>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> - int ret;
>>> unsigned int config, tmp;
>>> + int ret;
>> Definitely in the doesn't matter category, but if you really want to this
>> one is fine.
>>>
>>> if (iio_buffer_enabled(indio_dev))
>>> return -EBUSY;
>>> @@ -294,8 +295,8 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>> mutex_lock(&chip->state_lock);
>>>
>>> ret = regmap_read(chip->regmap, INA2XX_CONFIG, &config);
>>> - if (ret < 0)
>>> - goto _err;
>>> + if (ret)
>>> + goto err;
>> Good.
>>>
>>> tmp = config;
>>>
>>> @@ -310,19 +311,19 @@ static int ina2xx_write_raw(struct iio_dev *indio_dev,
>>> else
>>> ret = ina226_set_int_time_vbus(chip, val2, &tmp);
>>> break;
>>> +
>> Maybe a slight gain in readability so fair enough.
>>> default:
>>> ret = -EINVAL;
>>> }
>>>
>>> if (!ret && (tmp != config))
>>> ret = regmap_write(chip->regmap, INA2XX_CONFIG, tmp);
>>> -_err:
>>> +err:
>> Fine.
>>> mutex_unlock(&chip->state_lock);
>>>
>>> return ret;
>>> }
>>>
>>> -
>>> static ssize_t ina2xx_allow_async_readout_show(struct device *dev,
>>> struct device_attribute *attr,
>>> char *buf)
>>> @@ -355,6 +356,7 @@ static int set_shunt_resistor(struct ina2xx_chip_info *chip, unsigned int val)
>>> return -EINVAL;
>>>
>>> chip->shunt_resistor = val;
>>> +
>> good.
>>> return 0;
>>> }
>>>
>>> @@ -408,21 +410,21 @@ static ssize_t ina2xx_shunt_resistor_store(struct device *dev,
>>> * Sampling Freq is a consequence of the integration times of
>>> * the Voltage channels.
>>> */
>>> -#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> - .type = IIO_VOLTAGE, \
>>> - .address = (_address), \
>>> - .indexed = 1, \
>>> - .channel = (_index), \
>>> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> - BIT(IIO_CHAN_INFO_SCALE) | \
>>> - BIT(IIO_CHAN_INFO_INT_TIME), \
>>> - .scan_index = (_index), \
>>> - .scan_type = { \
>>> - .sign = 'u', \
>>> - .realbits = 16, \
>>> - .storagebits = 16, \
>>> - .endianness = IIO_LE, \
>>> - } \
>> This one I disagree with. It too often leads to noise when we end up with
>> an element with a line and have to change the spacing before the \s.
>> Much less churn occurs with teh version as original defined.
>> As someone who handles a fair number of patch reviews I feel pretty strongly
>> about anything that leads to more churn.
>>
>
> That's fine, this one I changed to I could see what was going on
> in didn't really care how it is done, I think I even left the
> other macro like this unchanged, so this change can be dropped.
>
>>> +#define INA2XX_CHAN_VOLTAGE(_index, _address) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .address = (_address), \
>>> + .indexed = 1, \
>>> + .channel = (_index), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
>>> + BIT(IIO_CHAN_INFO_SCALE) | \
>>> + BIT(IIO_CHAN_INFO_INT_TIME), \
>>> + .scan_index = (_index), \
>>> + .scan_type = { \
>>> + .sign = 'u', \
>>> + .realbits = 16, \
>>> + .storagebits = 16, \
>>> + .endianness = IIO_LE, \
>>> + } \
>>> }
>>>
>>> static const struct iio_chan_spec ina2xx_channels[] = {
>>> @@ -500,7 +502,7 @@ static int ina2xx_work_buffer(struct iio_dev *indio_dev)
>>>
>>> static int ina2xx_capture_thread(void *data)
>>> {
>>> - struct iio_dev *indio_dev = (struct iio_dev *)data;
>>> + struct iio_dev *indio_dev = data;
>> good.
>>> struct ina2xx_chip_info *chip = iio_priv(indio_dev);
>>> unsigned int sampling_us = SAMPLING_PERIOD(chip);
>>> int buffer_us;
>>> @@ -575,8 +577,7 @@ static int ina2xx_debug_reg(struct iio_dev *indio_dev,
>>> }
>>>
>>> /* Possible integration times for vshunt and vbus */
>>> -static IIO_CONST_ATTR_INT_TIME_AVAIL \
>>> - ("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>>> +static IIO_CONST_ATTR_INT_TIME_AVAIL("0.000140 0.000204 0.000332 0.000588 0.001100 0.002116 0.004156 0.008244");
>>>
>>> static IIO_DEVICE_ATTR(in_allow_async_readout, S_IRUGO | S_IWUSR,
>>> ina2xx_allow_async_readout_show,
>>> @@ -598,21 +599,23 @@ static const struct attribute_group ina2xx_attribute_group = {
>>> };
>>>
>>> static const struct iio_info ina2xx_info = {
>>> - .debugfs_reg_access = &ina2xx_debug_reg,
>>> - .read_raw = &ina2xx_read_raw,
>>> - .write_raw = &ina2xx_write_raw,
>>> - .attrs = &ina2xx_attribute_group,
>>> .driver_module = THIS_MODULE,
>>> + .attrs = &ina2xx_attribute_group,
>>> + .read_raw = ina2xx_read_raw,
>>> + .write_raw = ina2xx_write_raw,
>>> + .debugfs_reg_access = ina2xx_debug_reg,
>> sensible change.
>>> };
>>>
>>> /* Initialize the configuration and calibration registers. */
>>> static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>>> {
>>> u16 regval;
>>> - int ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>>> + int ret;
>> This form is considered acceptable in kernel code - but perhaps yours is
>> a tiny bit more readable so if you want to keep this one.
>>>
>>> - if (ret < 0)
>>> + ret = regmap_write(chip->regmap, INA2XX_CONFIG, config);
>>> + if (ret)
>>> return ret;
>> good
>>> +
>>> /*
>>> * Set current LSB to 1mA, shunt is in uOhms
>>> * (equation 13 in datasheet). We hardcode a Current_LSB
>>> @@ -621,7 +624,7 @@ static int ina2xx_init(struct ina2xx_chip_info *chip, unsigned int config)
>>> * to the user for now.
>>> */
>>> regval = DIV_ROUND_CLOSEST(chip->config->calibration_factor,
>>> - chip->shunt_resistor);
>>> + chip->shunt_resistor);
>> good
>>>
>>> return regmap_write(chip->regmap, INA2XX_CALIBRATION, regval);
>>> }
>>> @@ -632,8 +635,8 @@ static int ina2xx_probe(struct i2c_client *client,
>>> struct ina2xx_chip_info *chip;
>>> struct iio_dev *indio_dev;
>>> struct iio_buffer *buffer;
>>> - int ret;
>>> unsigned int val;
>>> + int ret;
>> Again, don't care on this one.
>>>
>>> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
>>> if (!indio_dev)
>>> @@ -641,8 +644,19 @@ static int ina2xx_probe(struct i2c_client *client,
>>>
>>> chip = iio_priv(indio_dev);
>>>
>> All this reordering needs a detailed justification. It adds a lot of churn
>> for limited benefit. I prefer the ordering you end up with, but is it
>> worth the noise? Not to my mind.
>
> I would say it is, keeping the initialization steps logically ordered and
> grouped, as opposed to randomly mixed, is very important for bug finding
> and for easier bug spotting during future development, it's also easier to
> read and follow/comprehend.
While the ordering is different from yours, I'm not convinced that it
is particularly less logical. The level of churn in here, might make bug
finding ever so slightly easier, but at the cost of pretty much ensuring
any bug fixes never make it as back ports.
>
>>> + /* This is only used for device removal purposes. */
>>> + i2c_set_clientdata(client, indio_dev);
>>> +
>>> + chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>>> + if (IS_ERR(chip->regmap)) {
>>> + dev_err(&client->dev, "failed to allocate register map\n");
>>> + return PTR_ERR(chip->regmap);
>>> + }
>>> +
>>> chip->config = &ina2xx_config[id->driver_data];
>>>
>>> + mutex_init(&chip->state_lock);
>>> +
>>> if (of_property_read_u32(client->dev.of_node,
>>> "shunt-resistor", &val) < 0) {
>>> struct ina2xx_platform_data *pdata =
>>> @@ -658,25 +672,6 @@ static int ina2xx_probe(struct i2c_client *client,
>>> if (ret)
>>> return ret;
>>>
>>> - mutex_init(&chip->state_lock);
>>> -
>>> - /* This is only used for device removal purposes. */
>>> - i2c_set_clientdata(client, indio_dev);
>>> -
>>> - indio_dev->name = id->name;
>>> - indio_dev->channels = ina2xx_channels;
>>> - indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>>> -
>>> - indio_dev->dev.parent = &client->dev;
>>> - indio_dev->info = &ina2xx_info;
>>> - indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>> -
>>> - chip->regmap = devm_regmap_init_i2c(client, &ina2xx_regmap_config);
>>> - if (IS_ERR(chip->regmap)) {
>>> - dev_err(&client->dev, "failed to allocate register map\n");
>>> - return PTR_ERR(chip->regmap);
>>> - }
>>> -
>>> /* Patch the current config register with default. */
>>> val = chip->config->config_default;
>>>
>>> @@ -687,24 +682,28 @@ static int ina2xx_probe(struct i2c_client *client,
>>> }
>>>
>>> ret = ina2xx_init(chip, val);
>>> - if (ret < 0) {
>>> - dev_err(&client->dev, "error configuring the device: %d\n",
>>> - ret);
>>> - return -ENODEV;
>>> + if (ret) {
>> This change is good however.
>>
>>> + dev_err(&client->dev, "error configuring the device\n");
>> Dropping the return value might be 'cleaner' in some sense, but I don't think
>> it's worth making the change.
>
> We are returning the error code, so informing the user of its number every
> step in the error path leads to a lot of noise, we often end up with the
> number printed three or more times. I think it should only get printed by
> the last step in the chain that eats the number by not returning it though
> to another step.
>
>>> + return ret;
>>> }
>>>
>>> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->channels = ina2xx_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(ina2xx_channels);
>>> + indio_dev->name = id->name;
>>> + indio_dev->info = &ina2xx_info;
>>> + indio_dev->setup_ops = &ina2xx_setup_ops;
>>> +
>>> buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
>>> if (!buffer)
>>> return -ENOMEM;
>>>
>>> - indio_dev->setup_ops = &ina2xx_setup_ops;
>>> -
>>> iio_device_attach_buffer(indio_dev, buffer);
>>>
>>> return iio_device_register(indio_dev);
>>> }
>>>
>>> -
>>> static int ina2xx_remove(struct i2c_client *client)
>>> {
>>> struct iio_dev *indio_dev = i2c_get_clientdata(client);
>>> @@ -717,7 +716,6 @@ static int ina2xx_remove(struct i2c_client *client)
>>> INA2XX_MODE_MASK, 0);
>>> }
>>>
>>> -
>> good.
>>> static const struct i2c_device_id ina2xx_id[] = {
>>> {"ina219", ina219},
>>> {"ina220", ina219},
>>> @@ -726,7 +724,6 @@ static const struct i2c_device_id ina2xx_id[] = {
>>> {"ina231", ina226},
>>> {}
>>> };
>>> -
>> good
>>> MODULE_DEVICE_TABLE(i2c, ina2xx_id);
>>>
>>> static struct i2c_driver ina2xx_driver = {
>>> @@ -737,7 +734,6 @@ static struct i2c_driver ina2xx_driver = {
>>> .remove = ina2xx_remove,
>>> .id_table = ina2xx_id,
>>> };
>>> -
>> good as well.
>>> module_i2c_driver(ina2xx_driver);
>>>
>>> MODULE_AUTHOR("Marc Titinger <marc.titinger@baylibre.com>");
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-02-17 19:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 18:34 [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Andrew F. Davis
2016-02-12 18:34 ` [PATCH 2/2] iio: ina2xx: Remove trace_printk debug statments Andrew F. Davis
2016-02-13 13:21 ` Jonathan Cameron
2016-02-14 20:02 ` Andrew F. Davis
2016-02-15 9:08 ` Marc Titinger
2016-02-13 12:55 ` [PATCH 1/2] iio: ina2xx: Fix whitespace and re-order code Jonathan Cameron
2016-02-14 20:44 ` Andrew F. Davis
2016-02-17 19:29 ` Jonathan Cameron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).