[PATCH] staging: iio: adc: add missing spaces around minus sign
The checkpatch.pl tool detected coding style problem: CHECK: spaces preferred around that '-' (ctx:VxV) in two files inside the adc directory. This patch will remove this problem. Signed-off-by: Slawomir Stepien --- drivers/staging/iio/adc/ad7192.c | 2 +- drivers/staging/iio/adc/ad7280a.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c index acdbc07fd259..7c632cf1932b 100644 --- a/drivers/staging/iio/adc/ad7192.c +++ b/drivers/staging/iio/adc/ad7192.c @@ -355,7 +355,7 @@ ad7192_show_scale_available(struct device *dev, } static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, -in_voltage-voltage_scale_available, +in_voltage - voltage_scale_available, 0444, ad7192_show_scale_available, NULL, 0); static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444, diff --git a/drivers/staging/iio/adc/ad7280a.c b/drivers/staging/iio/adc/ad7280a.c index 58420dcb406d..a4b4f8678c56 100644 --- a/drivers/staging/iio/adc/ad7280a.c +++ b/drivers/staging/iio/adc/ad7280a.c @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void *private) } static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, - in_voltage-voltage_thresh_low_value, + in_voltage - voltage_thresh_low_value, 0644, ad7280_read_channel_config, ad7280_write_channel_config, AD7280A_CELL_UNDERVOLTAGE); static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, - in_voltage-voltage_thresh_high_value, + in_voltage - voltage_thresh_high_value, 0644, ad7280_read_channel_config, ad7280_write_channel_config, -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adc: add missing spaces around minus sign
On paź 01, 2018 14:51, Peter Meerwald-Stadler wrote: > > > The checkpatch.pl tool detected coding style problem: > > > > CHECK: spaces preferred around that '-' (ctx:VxV) > > oh no, not again > please see e.g. https://lore.kernel.org/patchwork/patch/635994/ > for some discussion why this patch doesn't make sense Oh...my mistake. Thank you for clarification. I should be more careful. Sorry! > > in two files inside the adc directory. This patch will remove this > > problem. > > > > Signed-off-by: Slawomir Stepien > > --- > > drivers/staging/iio/adc/ad7192.c | 2 +- > > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/iio/adc/ad7192.c > > b/drivers/staging/iio/adc/ad7192.c > > index acdbc07fd259..7c632cf1932b 100644 > > --- a/drivers/staging/iio/adc/ad7192.c > > +++ b/drivers/staging/iio/adc/ad7192.c > > @@ -355,7 +355,7 @@ ad7192_show_scale_available(struct device *dev, > > } > > > > > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available, > > -in_voltage-voltage_scale_available, > > +in_voltage - voltage_scale_available, > > 0444, ad7192_show_scale_available, NULL, 0); > > > > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444, > > diff --git a/drivers/staging/iio/adc/ad7280a.c > > b/drivers/staging/iio/adc/ad7280a.c > > index 58420dcb406d..a4b4f8678c56 100644 > > --- a/drivers/staging/iio/adc/ad7280a.c > > +++ b/drivers/staging/iio/adc/ad7280a.c > > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void > > *private) > > } > > > > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, > > - in_voltage-voltage_thresh_low_value, > > + in_voltage - voltage_thresh_low_value, > > 0644, > > ad7280_read_channel_config, > > ad7280_write_channel_config, > > AD7280A_CELL_UNDERVOLTAGE); > > > > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, > > - in_voltage-voltage_thresh_high_value, > > + in_voltage - voltage_thresh_high_value, > > 0644, > > ad7280_read_channel_config, > > ad7280_write_channel_config, > > > -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 0/2] Fix style problems in ad7150 driver
This series will fix simple style problem inside the ad7150 driver. Slawomir Stepien (2): staging: iio: cdc: ad7150: use pointer to shorten the line length staging: iio: cdc: ad7150: fix misaligned lines drivers/staging/iio/cdc/ad7150.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 1/2] staging: iio: cdc: ad7150: use pointer to shorten the line length
By using the pointer to channel attribute, we can now make the lines short enough to eliminate the checkpatch.pl problem: CHECK: Alignment should match open parenthesis Signed-off-by: Slawomir Stepien --- drivers/staging/iio/cdc/ad7150.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c index d16084d7068c..5b5b766a0405 100644 --- a/drivers/staging/iio/cdc/ad7150.c +++ b/drivers/staging/iio/cdc/ad7150.c @@ -102,18 +102,19 @@ static int ad7150_read_raw(struct iio_dev *indio_dev, { int ret; struct ad7150_chip_info *chip = iio_priv(indio_dev); + const int *channel = &chan->channel; switch (mask) { case IIO_CHAN_INFO_RAW: ret = i2c_smbus_read_word_data(chip->client, - ad7150_addresses[chan->channel][0]); + ad7150_addresses[*channel][0]); if (ret < 0) return ret; *val = swab16(ret); return IIO_VAL_INT; case IIO_CHAN_INFO_AVERAGE_RAW: ret = i2c_smbus_read_word_data(chip->client, - ad7150_addresses[chan->channel][1]); + ad7150_addresses[*channel][1]); if (ret < 0) return ret; *val = swab16(ret); -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH 2/2] staging: iio: cdc: ad7150: fix misaligned lines
These lines were misaligned, but the checkpatch.pl didn't indicate them as such. Signed-off-by: Slawomir Stepien --- drivers/staging/iio/cdc/ad7150.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c index 5b5b766a0405..bbbf05ca2624 100644 --- a/drivers/staging/iio/cdc/ad7150.c +++ b/drivers/staging/iio/cdc/ad7150.c @@ -183,8 +183,8 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev, case IIO_EV_TYPE_THRESH: value = chip->threshold[rising][chan]; return i2c_smbus_write_word_data(chip->client, - ad7150_addresses[chan][3], - swab16(value)); +ad7150_addresses[chan][3], +swab16(value)); case IIO_EV_TYPE_MAG_ADAPTIVE: sens = chip->mag_sensitivity[rising][chan]; timeout = chip->mag_timeout[rising][chan]; -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] staging: iio: cdc: ad7150: use pointer to shorten the line length
On paź 04, 2018 17:21, Lars-Peter Clausen wrote: > On 10/01/2018 09:33 PM, Slawomir Stepien wrote: > > drivers/staging/iio/cdc/ad7150.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/staging/iio/cdc/ad7150.c > > b/drivers/staging/iio/cdc/ad7150.c > > index d16084d7068c..5b5b766a0405 100644 > > --- a/drivers/staging/iio/cdc/ad7150.c > > +++ b/drivers/staging/iio/cdc/ad7150.c > > @@ -102,18 +102,19 @@ static int ad7150_read_raw(struct iio_dev *indio_dev, > > { > > int ret; > > struct ad7150_chip_info *chip = iio_priv(indio_dev); > > + const int *channel = &chan->channel; > > Instead of taking a pointer, just take the value of the channel index and > use that. The generated code should be very similar but its a bit nicer to > read. OK. I've been thinking about that for a moment, but in the end decided to go with the pointer. I don't mind the value. I'll prepare v2. Thank you. -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 0/2] Fix style problems in ad7150 driver
This series will fix simple style problems inside the ad7150 driver. --- Changes since v1: - Use channel value copy rather than pointer - for better readability - Change "problem" -> "problems" in cover letter description --- Slawomir Stepien (2): staging: iio: cdc: ad7150: use value copy to shorten the line length staging: iio: cdc: ad7150: fix misaligned lines drivers/staging/iio/cdc/ad7150.c | 9 + 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 1/2] staging: iio: cdc: ad7150: use value copy to shorten the line length
By using the copy of channel attribute, we can now make the lines short enough to eliminate the checkpatch.pl problem: CHECK: Alignment should match open parenthesis Signed-off-by: Slawomir Stepien --- drivers/staging/iio/cdc/ad7150.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c index d16084d7068c..e2c70063fa0f 100644 --- a/drivers/staging/iio/cdc/ad7150.c +++ b/drivers/staging/iio/cdc/ad7150.c @@ -102,18 +102,19 @@ static int ad7150_read_raw(struct iio_dev *indio_dev, { int ret; struct ad7150_chip_info *chip = iio_priv(indio_dev); + int channel = chan->channel; switch (mask) { case IIO_CHAN_INFO_RAW: ret = i2c_smbus_read_word_data(chip->client, - ad7150_addresses[chan->channel][0]); + ad7150_addresses[channel][0]); if (ret < 0) return ret; *val = swab16(ret); return IIO_VAL_INT; case IIO_CHAN_INFO_AVERAGE_RAW: ret = i2c_smbus_read_word_data(chip->client, - ad7150_addresses[chan->channel][1]); + ad7150_addresses[channel][1]); if (ret < 0) return ret; *val = swab16(ret); -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v2 2/2] staging: iio: cdc: ad7150: fix misaligned lines
These lines were misaligned, but the checkpatch.pl didn't indicate them as such. Signed-off-by: Slawomir Stepien --- drivers/staging/iio/cdc/ad7150.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c index e2c70063fa0f..24f74ce60f80 100644 --- a/drivers/staging/iio/cdc/ad7150.c +++ b/drivers/staging/iio/cdc/ad7150.c @@ -183,8 +183,8 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev, case IIO_EV_TYPE_THRESH: value = chip->threshold[rising][chan]; return i2c_smbus_write_word_data(chip->client, - ad7150_addresses[chan][3], - swab16(value)); +ad7150_addresses[chan][3], +swab16(value)); case IIO_EV_TYPE_MAG_ADAPTIVE: sens = chip->mag_sensitivity[rising][chan]; timeout = chip->mag_timeout[rising][chan]; -- 2.19.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 06, 2018 13:27, Gabriel Capella wrote: > This patch does not change the logic, it only > corrects the checkpatch checks. > > The patch fixes 2 checks of type: > "CHECK: spaces preferred around that '-'" I've made the same mistake few days ago. This change is incorrect. Please see: https://lore.kernel.org/patchwork/patch/635994/. > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7280a.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/iio/adc/ad7280a.c > b/drivers/staging/iio/adc/ad7280a.c > index 58420dcb406d..a4b4f8678c56 100644 > --- a/drivers/staging/iio/adc/ad7280a.c > +++ b/drivers/staging/iio/adc/ad7280a.c > @@ -750,14 +750,14 @@ static irqreturn_t ad7280_event_handler(int irq, void > *private) > } > > static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value, > - in_voltage-voltage_thresh_low_value, > + in_voltage - voltage_thresh_low_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, > AD7280A_CELL_UNDERVOLTAGE); > > static IIO_DEVICE_ATTR_NAMED(in_thresh_high_value, > - in_voltage-voltage_thresh_high_value, > + in_voltage - voltage_thresh_high_value, > 0644, > ad7280_read_channel_config, > ad7280_write_channel_config, -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adc: ad7280a: fix 2 checks
On paź 11, 2018 10:32, Dan Carpenter wrote: > On Sat, Oct 06, 2018 at 10:25:42PM +0200, Slawomir Stepien wrote: > > On paź 06, 2018 13:27, Gabriel Capella wrote: > > > This patch does not change the logic, it only > > > corrects the checkpatch checks. > > > > > > The patch fixes 2 checks of type: > > > "CHECK: spaces preferred around that '-'" > > > > I've made the same mistake few days ago. This change is incorrect. > > Please see: https://lore.kernel.org/patchwork/patch/635994/. > > > > You could add a comment. /* do not put spaces around the hyphen. > #checkpatch.pl */ > > These are the only places which use this style and a lot of people > bump into it. Gabriel go ahead if you want to. If not then I would be happy to prepare this patch. -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: adc: Add comments to prevent checks corrections
On paź 15, 2018 18:25, Gabriel Capella wrote: > This patch adds 3 comments in 2 different files. > These comments warn to don't correct the checks of type: > "CHECK: spaces preferred around that '-'" > > Signed-off-by: Gabriel Capella > --- > drivers/staging/iio/adc/ad7192.c | 1 + > drivers/staging/iio/adc/ad7280a.c | 2 ++ > 2 files changed, 3 insertions(+) I have this simpler solution...let's focus our efforts on moving the two drivers out of staging. This will prevent the CHK to appear: Cut from checkpatch.pl: if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) { $check = 1; Existing driver out of staging, with this "problem": $ ./scripts/checkpatch.pl --types SPACING --file drivers/iio/adc/ad7793.c total: 0 errors, 0 warnings, 827 lines checked drivers/iio/adc/ad7793.c has no obvious style problems and is ready for submission. NOTE: Used message types: SPACING In my opinion it would stop this incorrect patches. I have also this changes to checkpatch.pl: https://github.com/s-stepien/linux-1/commit/c976a31b392393fb417f2d9e2ec9639bc226ad0b and usage: https://github.com/s-stepien/linux-1/commit/1adc0428b496f44f6a931637084bb619ddd9992d but I'm not that sure it is the best way to go. What do you all think? -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
> { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + int ret = 0; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + st->sample = devm_gpiod_get(&spi->dev, "sample", GPIOD_IN); > + if (IS_ERR(st->sample)) { > + ret = PTR_ERR(st->sample); > + dev_err(&spi->dev, "Failed to request sample GPIO: %d\n", > + ret); > + return ret; > + } > + st->a0 = devm_gpiod_get(&spi->dev, "a0", flags); > + if (IS_ERR(st->a0)) { > + ret = PTR_ERR(st->a0); > + dev_err(&spi->dev, "Failed to request a0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->a1 = devm_gpiod_get(&spi->dev, "a1", flags); > + if (IS_ERR(st->a1)) { > + ret = PTR_ERR(st->a1); > + dev_err(&spi->dev, "Failed to request a1 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res0 = devm_gpiod_get(&spi->dev, "res0", flags); > + if (IS_ERR(st->res0)) { > + ret = PTR_ERR(st->res0); > + dev_err(&spi->dev, "Failed to request res0 GPIO: %d\n", > + ret); > + return ret; > + } > + st->res1 = devm_gpiod_get(&spi->dev, "res1", flags); > + if (IS_ERR(st->res1)) { > + ret = PTR_ERR(st->res1); > + dev_err(&spi->dev, "Failed to request res1 GPIO: %d\n", > + ret); > + return ret; > + } To reduce the redundant code here, create an array of structs. The struct will have a pointer to gpio_desc, name of the gpio and the gpios' flags. Then you can use for loop to get all the needed gpios: for (l = 0; l < ARRAY_SIZE(str); l++) { str[l].ptr = devm_gpiod_get(&spi->dev, str[l].name, str[l].flags); if (IS_ERR(str[l].res1)) { ... } } > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > +static void ad2s1210_free_gpios(struct spi_device *spi, > + struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > - > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + devm_gpiod_put(&spi->dev, st->sample); > + devm_gpiod_put(&spi->dev, st->a0); > + devm_gpiod_put(&spi->dev, st->a1); > + devm_gpiod_put(&spi->dev, st->res0); > + devm_gpiod_put(&spi->dev, st->res1); > } This whole function ad2s1210_free_gpios is not needed anymore because you are using the devm API (it's called from probe and remove, so you are safe). > static int ad2s1210_probe(struct spi_device *spi) > @@ -670,7 +702,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return -ENOMEM; > st = iio_priv(indio_dev); > st->pdata = spi->dev.platform_data; > - ret = ad2s1210_setup_gpios(st); > + ret = ad2s1210_setup_gpios(spi, st); > if (ret < 0) > return ret; > > @@ -702,7 +734,7 @@ static int ad2s1210_probe(struct spi_device *spi) > return 0; > > error_free_gpios: > - ad2s1210_free_gpios(st); > + ad2s1210_free_gpios(spi, st); > return ret; > } > > @@ -711,7 +743,7 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > + ad2s1210_free_gpios(spi, iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 21, 2018 10:31, Slawomir Stepien wrote: > On paź 21, 2018 11:49, Nishad Kamdar wrote: > > -static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > > +static int ad2s1210_setup_gpios(struct spi_device *spi, > > + struct ad2s1210_state *st) > > This change is not needed. The st has the spi_device inside. Use > container_of() > macro here. Wait...what? I mean use direct access using st->sdev. -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2] staging: iio: ad2s1210: Switch to the gpio descriptor interface
-512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(&st->lock); > @@ -630,30 +641,30 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret = 0, i = 0; You do not need to init that i with 0. Also the ret do not have to be 0. If you return just 0 at the on this function. > + struct spi_device *spi = st->sdev; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {st->sample, "sample", GPIOD_IN}, > + {st->a0, "a0", flags}, > + {st->a1, "a1", flags}, > + {st->res0, "res0", flags}, > + {st->res1, "res1", flags}, To change a pointer, you need to pass an address of that pointer. I think you should you . notation when initializing the struct. > }; > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { I do not know if that is a common practise, but you can set a pointer to gpio[i] as the first instruction in this for, so you would not have to write this whole gpio[i]: pin = &gpios[i] and then: pin->ptr, etc. > + gpios[i].ptr = devm_gpiod_get(&spi->dev, gpios[i].name, > + gpios[i].flags); > + if (IS_ERR(gpios[i].ptr)) { > + ret = PTR_ERR(gpios[i].ptr); > + dev_err(&spi->dev, "Failed to request %s GPIO: %d\n", > + gpios[i].name, ret); > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return ret; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,18 +703,13 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > - Removing a empty line here, that is here to make it more readable. > + return ret; > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > spi_setup(spi); > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +717,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] staging: iio: ad2s1210: Switch to the gpio descriptor interface
gt;sample, 0); > /* delay (6 * tck + 20) nano seconds */ > udelay(1); > > @@ -512,7 +523,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > > error_ret: > - gpio_set_value(st->pdata->sample, 1); > + gpiod_set_value(st->sample, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(&st->lock); > @@ -630,30 +641,32 @@ static const struct iio_info ad2s1210_info = { > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > + int ret, i; > + struct spi_device *spi = st->sdev; > + struct ad2s1210_gpio *pin; > + unsigned long flags = st->pdata->gpioin ? GPIOD_IN : GPIOD_OUT_LOW; > + > + struct ad2s1210_gpio gpios[] = { > + {.ptr = &st->sample, .name = "sample", .flags = GPIOD_IN}, > + {.ptr = &st->a0, .name = "a0", .flags = flags}, > + {.ptr = &st->a1, .name = "a1", .flags = flags}, > + {.ptr = &st->res0, .name = "res0", .flags = flags}, > + {.ptr = &st->res1, .name = "res1", .flags = flags}, > }; I think that you should add spaces after { and before }. It's the file style from what I see. > > - return gpio_request_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > -} > - > -static void ad2s1210_free_gpios(struct ad2s1210_state *st) > -{ > - unsigned long flags = st->pdata->gpioin ? GPIOF_DIR_IN : GPIOF_DIR_OUT; > - struct gpio ad2s1210_gpios[] = { > - { st->pdata->sample, GPIOF_DIR_IN, "sample" }, > - { st->pdata->a[0], flags, "a0" }, > - { st->pdata->a[1], flags, "a1" }, > - { st->pdata->res[0], flags, "res0" }, > - { st->pdata->res[0], flags, "res1" }, > - }; > + for (i = 0; i < ARRAY_SIZE(gpios); i++) { > + pin = &gpios[i]; > + *pin->ptr = devm_gpiod_get(&spi->dev, pin->name, > +pin->flags); This can now fit into one line. > + if (IS_ERR(pin->ptr)) { Here you are checking the wrong pointer. Also notice the line below. > + ret = PTR_ERR(pin->ptr); > + dev_err(&spi->dev, "Failed to request %s GPIO: %d\n", > + pin->name, ret); Please notice how other dev_err looks like in this code. Align the message to the existing format. > + return ret; > + } > + } > > - gpio_free_array(ad2s1210_gpios, ARRAY_SIZE(ad2s1210_gpios)); > + return 0; > } > > static int ad2s1210_probe(struct spi_device *spi) > @@ -692,7 +705,7 @@ static int ad2s1210_probe(struct spi_device *spi) > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_free_gpios; > + return ret; > > st->fclkin = spi->max_speed_hz; > spi->mode = SPI_MODE_3; > @@ -700,10 +713,6 @@ static int ad2s1210_probe(struct spi_device *spi) > ad2s1210_initial(st); > > return 0; > - > -error_free_gpios: > - ad2s1210_free_gpios(st); > - return ret; > } > > static int ad2s1210_remove(struct spi_device *spi) > @@ -711,7 +720,6 @@ static int ad2s1210_remove(struct spi_device *spi) > struct iio_dev *indio_dev = spi_get_drvdata(spi); > > iio_device_unregister(indio_dev); > - ad2s1210_free_gpios(iio_priv(indio_dev)); > > return 0; > } > diff --git a/drivers/staging/iio/resolver/ad2s1210.h > b/drivers/staging/iio/resolver/ad2s1210.h > index e9b2147701fc..63d479b20a6c 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.h > +++ b/drivers/staging/iio/resolver/ad2s1210.h > @@ -12,9 +12,6 @@ > #define _AD2S1210_H > > struct ad2s1210_platform_data { > - unsigned intsample; > - unsigned inta[2]; > - unsigned intres[2]; > boolgpioin; > }; > #endif /* _AD2S1210_H */ -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] staging: iio: ad2s1210: Switch to the gpio descriptor interface
On paź 24, 2018 20:20, Nishad Kamdar wrote: > Use the gpiod interface instead of the deprecated old non-descriptor > interface. > > Signed-off-by: Nishad Kamdar > --- > Changes in v4: > - Add spaces after { and before } in gpios[] >initialization. > - Check the correct pointer for error. > - Align the dev_err msg to existing format in the code. > Changes in v3: > - Use a pointer to pointer for gpio_desc in >struct ad2s1210_gpio as it will be used to >modify a pointer. > - Use dot notation to initialize the structure. > - Use a pointer variable to avoid writing gpios[i]. > Changes in v2: > - Use the spi_device struct embedded in st instead >of passing it as an argument to ad2s1210_setup_gpios(). > - Use an array of structs to reduce redundant code in >in ad2s1210_setup_gpios(). > - Remove ad2s1210_free_gpios() as devm API is being used. > --- > drivers/staging/iio/resolver/ad2s1210.c | 92 ++--- > drivers/staging/iio/resolver/ad2s1210.h | 3 - > 2 files changed, 50 insertions(+), 45 deletions(-) Looks good to me. Reviewed-by: Slawomir Stepien -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v5 2/2] staging: iio: ad2s1210: Add device tree support.
Hi On paź 26, 2018 18:55, Nishad Kamdar wrote: > Add device tree table for matching vendor ID > and support for retrieving platform data > from device tree. So maybe you should make 2 commits? > Signed-off-by: Nishad Kamdar > --- > drivers/staging/iio/resolver/ad2s1210.c | 43 - > 1 file changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c > b/drivers/staging/iio/resolver/ad2s1210.c > index 93c3c70ce62e..9fd5461c4ed0 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -17,6 +17,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -669,6 +670,27 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state > *st) > return 0; > } > > +#ifdef CONFIG_OF > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct ad2s1210_platform_data *pdata; > + > + pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return NULL; > + > + pdata->gpioin = of_property_read_bool(np, "adi,gpioin"); Why here you are adding "adi", but you are not adding it to the gpios in prev commit? I've also seen this: https://patchwork.kernel.org/patch/10355839/. However I do not understand why adding vendor id to props is needed... > + > + return pdata; > +} > +#else > +static struct ad2s1210_platform_data *ad2s1210_parse_dt(struct device *dev) > +{ > + return NULL; > +} > +#endif > + > static int ad2s1210_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > @@ -682,7 +704,19 @@ static int ad2s1210_probe(struct spi_device *spi) > if (!indio_dev) > return -ENOMEM; > st = iio_priv(indio_dev); > - st->pdata = spi->dev.platform_data; > + if (spi->dev.of_node) { > + st->pdata = ad2s1210_parse_dt(&spi->dev); > + if (!st->pdata) > + return -EINVAL; > + } else { > + st->pdata = spi->dev.platform_data; > + } > + > + if (!st->pdata) { > + dev_err(&spi->dev, "ad2s1210: no platform data supplied\n"); > + return -EINVAL; > + } > + Why not just use only device-tree here? The ad2s1210_platform_data has now only one entry... In that case remember to add "depends on OF" in Kconfig. > ret = ad2s1210_setup_gpios(st); > if (ret < 0) > return ret; > @@ -724,6 +758,12 @@ static int ad2s1210_remove(struct spi_device *spi) > return 0; > } > > +static const struct of_device_id ad2s1210_of_match[] = { > + { .compatible = "adi,ad2s1210", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ad2s1210_of_match); > + > static const struct spi_device_id ad2s1210_id[] = { > { "ad2s1210" }, > {} > @@ -733,6 +773,7 @@ MODULE_DEVICE_TABLE(spi, ad2s1210_id); > static struct spi_driver ad2s1210_driver = { > .driver = { > .name = DRV_NAME, > + .of_match_table = of_match_ptr(ad2s1210_of_match), > }, > .probe = ad2s1210_probe, > .remove = ad2s1210_remove, -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: iio: ad5933: finalized device-tree support
On lis 23, 2018 21:51, Marcelo Schmitt wrote: > Added a of_device_id struct variable and subsequent call to > MODULE_DEVICE_TABLE macro to complete device-tree support for this > driver. > > Signed-off-by: Marcelo Schmitt > --- > drivers/staging/iio/impedance-analyzer/ad5933.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c > b/drivers/staging/iio/impedance-analyzer/ad5933.c > index edb8b540bbf1..19e8b6d2865c 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -771,9 +771,18 @@ static const struct i2c_device_id ad5933_id[] = { > > MODULE_DEVICE_TABLE(i2c, ad5933_id); > > +static const struct of_device_id ad5933_of_match[] = { > + { .compatible = "analog-devices,ad5933" }, > + { .compatible = "analog-devices,ad5934" }, Why "analog-devices", rather than "adi"? My understanding is that analog devices have "adi" as a manufacture part in compatible. > + { }, > +}; > + > +MODULE_DEVICE_TABLE(of, ad5933_of_match); > + > static struct i2c_driver ad5933_driver = { > .driver = { > .name = "ad5933", > + .of_match_table = ad5933_of_match, > }, > .probe = ad5933_probe, > .remove = ad5933_remove, -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[RFC] Resolving "false positive" error message from checkpatch.pl
Hi all There is a "false positive" error reported by checkpatch.pl: ERROR: Use 4 digit octal (0777) not decimal permissions #272: FILE: drivers/staging/iio/meter/ade7759.c:272: +static IIO_DEV_ATTR_CH_OFF(1, S_IWUSR | S_IRUGO, + ade7759_read_8bit, + ade7759_write_8bit, + ADE7759_CH1OS); ERROR: Use 4 digit octal (0777) not decimal permissions #276: FILE: drivers/staging/iio/meter/ade7759.c:276: +static IIO_DEV_ATTR_CH_OFF(2, S_IWUSR | S_IRUGO, + ade7759_read_8bit, + ade7759_write_8bit, + ADE7759_CH2OS); The same for the file drivers/staging/iio/meter/ade7753.c. We can see that this macro is matched by the pattern "IIO_DEV_ATTR_[A-Z_]+" from @mode_permission_funcs in checkpatch.pl. My question is: how this should be fixed? >From what I can say, it's a false positive so the correct way to fix it is to change the matching pattern: "IIO_DEV_ATTR_(?!CH_OFF)[A-Z_]+". Or maybe the fix should be done in iio code - argument position inside this macro? -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] Resolving "false positive" error message from checkpatch.pl
On Feb 03, 2017 11:27, Greg KH wrote: > On Fri, Feb 03, 2017 at 11:11:35AM +0100, Slawomir Stepien wrote: > > Hi all > > > > There is a "false positive" error reported by checkpatch.pl: > > > > ERROR: Use 4 digit octal (0777) not decimal permissions > > #272: FILE: drivers/staging/iio/meter/ade7759.c:272: > > +static IIO_DEV_ATTR_CH_OFF(1, S_IWUSR | S_IRUGO, > > + ade7759_read_8bit, > > + ade7759_write_8bit, > > + ADE7759_CH1OS); > > > > ERROR: Use 4 digit octal (0777) not decimal permissions > > #276: FILE: drivers/staging/iio/meter/ade7759.c:276: > > +static IIO_DEV_ATTR_CH_OFF(2, S_IWUSR | S_IRUGO, > > + ade7759_read_8bit, > > + ade7759_write_8bit, > > + ADE7759_CH2OS); > > > > The same for the file drivers/staging/iio/meter/ade7753.c. > > > > We can see that this macro is matched by the pattern "IIO_DEV_ATTR_[A-Z_]+" > > from > > @mode_permission_funcs in checkpatch.pl. > > > > My question is: how this should be fixed? > > Why do you think this is a false-positive? Because the 1st arg of that macro is not supposed to be a permissions flags. > > From what I can say, it's a false positive so the correct way to fix it is > > to > > change the matching pattern: "IIO_DEV_ATTR_(?!CH_OFF)[A-Z_]+". > > Huh? > > Provide a patch to show what you are suggesting please. diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8e96af53611c..0a1c6ec86caa 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -525,7 +525,7 @@ our @mode_permission_funcs = ( ["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", 2], ["proc_create(?:_data|)", 2], ["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2], - ["IIO_DEV_ATTR_[A-Z_]+", 1], + ["IIO_DEV_ATTR_(?!CH_OFF)[A-Z_]+", 1], ["SENSOR_(?:DEVICE_|)ATTR_2", 2], ["SENSOR_TEMPLATE(?:_2|)", 3], ["__ATTR", 2], -- 2.11.0 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [RFC] Resolving "false positive" error message from checkpatch.pl
On Feb 03, 2017 04:39, Joe Perches wrote: > On Fri, 2017-02-03 at 13:10 +0100, Slawomir Stepien wrote: > > On Feb 03, 2017 11:27, Greg KH wrote: > > > On Fri, Feb 03, 2017 at 11:11:35AM +0100, Slawomir Stepien wrote: > > > > Hi all > > > > > > > > There is a "false positive" error reported by checkpatch.pl: > > > > > > > > ERROR: Use 4 digit octal (0777) not decimal permissions > > > > #272: FILE: drivers/staging/iio/meter/ade7759.c:272: > > > > +static IIO_DEV_ATTR_CH_OFF(1, S_IWUSR | S_IRUGO, > > > > + ade7759_read_8bit, > > > > + ade7759_write_8bit, > > > > + ADE7759_CH1OS); > > > > > > > > ERROR: Use 4 digit octal (0777) not decimal permissions > > > > #276: FILE: drivers/staging/iio/meter/ade7759.c:276: > > > > +static IIO_DEV_ATTR_CH_OFF(2, S_IWUSR | S_IRUGO, > > > > + ade7759_read_8bit, > > > > + ade7759_write_8bit, > > > > + ADE7759_CH2OS); > > > > > > > > The same for the file drivers/staging/iio/meter/ade7753.c. > > > > > > > > We can see that this macro is matched by the pattern > > > > "IIO_DEV_ATTR_[A-Z_]+" from > > > > @mode_permission_funcs in checkpatch.pl. > > > > > > > > My question is: how this should be fixed? > > > > > > Why do you think this is a false-positive? > > > > Because the 1st arg of that macro is not supposed to be a permissions flags. > > > > > > From what I can say, it's a false positive so the correct way to fix it > > > > is to > > > > change the matching pattern: "IIO_DEV_ATTR_(?!CH_OFF)[A-Z_]+". > > > > > > Huh? > > > > > > Provide a patch to show what you are suggesting please. > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 8e96af53611c..0a1c6ec86caa 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -525,7 +525,7 @@ our @mode_permission_funcs = ( > > > > ["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", > > 2], > > ["proc_create(?:_data|)", 2], > > ["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2], > > - ["IIO_DEV_ATTR_[A-Z_]+", 1], > > + ["IIO_DEV_ATTR_(?!CH_OFF)[A-Z_]+", 1], > > ["SENSOR_(?:DEVICE_|)ATTR_2", 2], > > ["SENSOR_TEMPLATE(?:_2|)", 3], > > ["__ATTR", 2], > > There are many local MODE_DEV_ATTR_ functions > that don't have permission in as the first argument. Yeah...I just found the ones in the drivers/staging/iio/frequency that you pointed out. > $ grep -rPoh --include=*.[ch] "\bIIO_DEV_ATTR_[A-Z_]+\b" *|sort|uniq|wc -l > 123 > > Maybe it'd be better to list the ones that do > or to change the argument order of the defines > and uses so mode _is_ the first argument. That is my original question. What would be better here (especially that no we can see that there are few more places where the error will pop)? > $ grep -rPoh --include=*.[ch] "\bIIO_DEV_ATTR_[A-Z_]+\b" *|sort|uniq| \ > while read line ; do git grep -w $line | grep -w define ; done|grep _mode | > grep -v "(_mode" > drivers/staging/iio/meter/meter.h:#define IIO_DEV_ATTR_CH_OFF(_num, _mode, > _show, _store, _addr) \ > drivers/staging/iio/frequency/dds.h:#define IIO_DEV_ATTR_FREQ(_channel, _num, > _mode, _show, _store, _addr)\ > drivers/staging/iio/frequency/dds.h:#define IIO_DEV_ATTR_FREQSYMBOL(_channel, > _mode, _show, _store, _addr)\ > drivers/staging/iio/frequency/dds.h:#define IIO_DEV_ATTR_OUT_ENABLE(_channel, > _mode, _show, _store, _addr)\ > drivers/staging/iio/frequency/dds.h:#define IIO_DEV_ATTR_PHASE(_channel, > _num, _mode, _show, _store, _addr) \ > drivers/staging/iio/frequency/dds.h:#define > IIO_DEV_ATTR_PHASESYMBOL(_channel, _mode, _show, _store, _addr) \ > drivers/staging/iio/frequency/dds.h:#define > IIO_DEV_ATTR_PINCONTROL_EN(_channel, _mode, _show, _store, _addr)\ > drivers/staging/iio/frequency/dds.h:#define > IIO_DEV_ATTR_PINCONTROL_FREQ_EN(_channel, _mode, _show, _store, _addr)\ > drivers/staging/iio/frequency/dds.h:#define > IIO_DEV_ATTR_PINCONTROL_PHASE_EN(_channel, _mode, _show, _store, _addr)\ -- Slawomir Stepien ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel