* [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 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 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-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 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 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).