Re: [PATCH 01/10] spi: spi-axi-spi-engine: remove usage of delay_usecs

[Lars-Peter Clausen]

On 3/10/21 8:16 AM, Alexandru Ardelean wrote:

On Mon, 8 Mar 2021 at 18:42, Lars-Peter Clausen  wrote:

On 3/8/21 3:54 PM, Alexandru Ardelean wrote:

The 'delay_usecs' field was handled for backwards compatibility in case
there were some users that still configured SPI delay transfers with
this field.

They should all be removed by now.

Signed-off-by: Alexandru Ardelean 
---
   drivers/spi/spi-axi-spi-engine.c | 12 
   1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index af86e6d6e16b..80c3e38f5c1b 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct 
spi_engine_program *p, bool dry,
   unsigned int t;
   int delay;

- if (xfer->delay_usecs) {
- delay = xfer->delay_usecs;
- } else {
- delay = spi_delay_to_ns(>delay, xfer);
- if (delay < 0)
- return;
- delay /= 1000;
- }
+ delay = spi_delay_to_ns(>delay, xfer);
+ if (delay < 0)
+ return;

Bit of a nit, but this could be `delay <= 0` and then drop the check for
`delay == 0` below.

hmm, that's a bit debatable, because the `delay == 0` check comes
after `delay /= 1000` ;
to do what you're suggesting, it would probably need a
DIV_ROUND_UP(delay, 1000) to make sure that even sub-microsecond
delays don't become zero;


Ah, true. Lets keep the code as it is.

On the other hand you could argue that we should round up to ensure the 
delay is at least as long as requested. But that is something that 
should be changed independently from this series.



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 01/10] spi: spi-axi-spi-engine: remove usage of delay_usecs

[Lars-Peter Clausen]

On 3/8/21 3:54 PM, Alexandru Ardelean wrote:

The 'delay_usecs' field was handled for backwards compatibility in case
there were some users that still configured SPI delay transfers with
this field.

They should all be removed by now.

Signed-off-by: Alexandru Ardelean 
---
  drivers/spi/spi-axi-spi-engine.c | 12 
  1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-axi-spi-engine.c b/drivers/spi/spi-axi-spi-engine.c
index af86e6d6e16b..80c3e38f5c1b 100644
--- a/drivers/spi/spi-axi-spi-engine.c
+++ b/drivers/spi/spi-axi-spi-engine.c
@@ -170,14 +170,10 @@ static void spi_engine_gen_sleep(struct 
spi_engine_program *p, bool dry,
unsigned int t;
int delay;
  
-	if (xfer->delay_usecs) {

-   delay = xfer->delay_usecs;
-   } else {
-   delay = spi_delay_to_ns(>delay, xfer);
-   if (delay < 0)
-   return;
-   delay /= 1000;
-   }
+   delay = spi_delay_to_ns(>delay, xfer);
+   if (delay < 0)
+   return;


Bit of a nit, but this could be `delay <= 0` and then drop the check for 
`delay == 0` below.



+   delay /= 1000;
  
  	if (delay == 0)

return;



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: static analysis bug report in drivers/staging/iio/dac/ad5380.c

[Lars-Peter Clausen]

On 8/15/19 12:21 PM, Colin Ian King wrote:
> Hi,
> 
> Static analysis with Coverity Scan has detected a potential assignment
> bug in ad5380.c:
> 
> 217case IIO_CHAN_INFO_CALIBBIAS:
> 218ret = regmap_read(st->regmap,
> AD5380_REG_OFFSET(chan->address),
> 219val);
> 220if (ret)
> 221return ret;
> 222*val >>= chan->scan_type.shift;
> 
> CID 43178 (#1 of 1): Unused value (UNUSED_VALUE)assigned_pointer:
> Assigning value from val - (1 << chan->scan_type.realbits) / 2 to val
> here, but that stored value is not used.
> 
> 223val -= (1 << chan->scan_type.realbits) / 2;
> 224return IIO_VAL_INT;
> 
> val is a pointer and so updating it before a return is probably not the
> intention.  I suspect the intention was probably:
> 
>  *val -= (1 << chan->scan_type.realbits) / 2;
> 
> However, I'm not confident about this as the following case has:
> 
> 225case IIO_CHAN_INFO_SCALE:
> 226*val = 2 * st->vref;
> 227*val2 = chan->scan_type.realbits;
> 228return IIO_VAL_FRACTIONAL_LOG2;
> 
> which may imply the update maybe to *val2 instead, e.g.:
> 
>   *val2 -= (1 << chan->scan_type.realbits) / 2;
> 
> Any ideas?

Updating changing val to *val is the right fix in this case.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7780: update voltage on read

[Lars-Peter Clausen]

On 10/25/2018 04:55 PM, Himanshu Jha wrote:
> On Thu, Oct 25, 2018 at 11:26:36AM -0300, Renato Lui Geh wrote:
>> Hi,
>>
>> Thanks for the quick review. :)
>>
>>> But please create one patch per issue and do not put unrelated changes into
>>> the same patch.
>>
>> Should I resend this patch as a patchset containing the two changes?
> 
> Yes! "One patch per change policy"
> 
>>> Also your mail client seems to have replaced tabs in the patch with spaces,
>>> this means the patch will not apply cleanly. Check the
>>> Documentation/email-clients.txt file for some hints how to configure your
>>> mail client so it will not break patches.
>>
>> From my end my original email patch appears to have tabs instead of
>> spaces. I redownloaded my email and vim shows that the indentation has
>> the ^I tab characters. But when downloading your reply it was converted
>> to spaces. Am I missing something?
> 
> Your patch applies fine.
> 
> I think the problem is on Lars end due to Thunderbird.

Yeah, looks like I need to read email-clients.txt...

I think it is mis-rendered on my side due to the "Content-Type: ...
format=flowed" in the header.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7780: update voltage on read

[Lars-Peter Clausen]

On 10/25/2018 03:32 PM, Renato Lui Geh wrote:
> The ad7780 driver previously did not read the correct device output.
> This patch fixes two issues.
> 
> - The driver read an outdated value set at initialization. It now
> updates its voltage on read.
> - Variable val subtracted an uninitialized value on
> IIO_CHAN_INFO_OFFSET. This was fixed by assiging the correct value
> instead.
> 
> Signed-off-by: Renato Lui Geh 

Hi,

Thanks for the patch, this looks good.

But please create one patch per issue and do not put unrelated changes into
the same patch.

Also your mail client seems to have replaced tabs in the patch with spaces,
this means the patch will not apply cleanly. Check the
Documentation/email-clients.txt file for some hints how to configure your
mail client so it will not break patches.

- Lars

> ---
> drivers/staging/iio/adc/ad7780.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index b67412db0318..06700fe554a2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -87,16 +87,20 @@ static int ad7780_read_raw(struct iio_dev *indio_dev,
>    long m)
> {
> struct ad7780_state *st = iio_priv(indio_dev);
> +    int voltage_uv = 0;
> 
> switch (m) {
> case IIO_CHAN_INFO_RAW:
>     return ad_sigma_delta_single_conversion(indio_dev, chan, val);
> case IIO_CHAN_INFO_SCALE:
> +    voltage_uv = regulator_get_voltage(st->reg);
> +    if (voltage_uv)
> +    st->int_vref_mv = voltage_uv/1000;
>     *val = st->int_vref_mv * st->gain;
>     *val2 = chan->scan_type.realbits - 1;
>     return IIO_VAL_FRACTIONAL_LOG2;
> case IIO_CHAN_INFO_OFFSET:
> -    *val -= (1 << (chan->scan_type.realbits - 1));
> +    *val = -(1 << (chan->scan_type.realbits - 1));
>     return IIO_VAL_INT;
> }
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging: iio: ad7606: Move out of staging

[Lars-Peter Clausen]

On 10/18/2018 02:55 PM, Dan Carpenter wrote:
> On Thu, Oct 18, 2018 at 12:10:32PM +0300, Stefan Popa wrote:
>> +static int ad7606_read_samples(struct ad7606_state *st)
>> +{
>> +unsigned int num = st->chip_info->num_channels;
>> +u16 *data = st->data;
>> +int ret;
>> +
>> +/*
>> + * The frstdata signal is set to high while and after reading the sample
>> + * of the first channel and low for all other channels. This can be used
>> + * to check that the incoming data is correctly aligned. During normal
>> + * operation the data should never become unaligned, but some glitch or
>> + * electrostatic discharge might cause an extra read or clock cycle.
>> + * Monitoring the frstdata signal allows to recover from such failure
>> + * situations.
>> + */
>> +
>> +if (st->gpio_frstdata) {
>> +ret = st->bops->read_block(st->dev, 1, data);
>> +if (ret)
>> +return ret;
>> +
>> +if (!gpiod_get_value(st->gpio_frstdata)) {
> 
> This check should maybe be:
> 
>   if (gpiod_get_value(st->gpio_frstdata) <= 0) {
> 
> (Or possibly not, I don't know the code very well).

gpiod_get_value() is only allowed to return either 0 or 1.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

[Lars-Peter Clausen]

On 10/18/2018 09:28 AM, Phil Reid wrote:
[...]
>> +    chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
>> +    if (IS_ERR(chip->rdwr_pin)) {
>> +    ret = PTR_ERR(chip->rdwr_pin);
>> +    dev_err(_dev->dev, "Failed to request rdwr GPIO: %d\n",
>> +    ret);
>>   return ret;
>>   }
>> -    gpio_direction_input(chip->rdwr_pin);
> 
> The RD/WR pin is an input to the AD78xx. So this doesn't make sense being
> GPIOD_IN.

One thing at a time. This patch is a straight forward conversion to the GPIO
descriptor interface. It keeps the existing semantics of the driver as they are.

Now these semantics are obviously wrong and should be fixed but that should
be a separate patch from changing the interface.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: ad7816: Switch to the gpio descriptor interface

[Lars-Peter Clausen]

On 10/17/2018 04:47 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Acked-by: Lars-Peter Clausen 

Thanks.

> ---
> Changes in v2:
>  - Correct the error messages as pin number being showed
>has now been replaced by error code.
> ---
>  drivers/staging/iio/adc/ad7816.c | 80 ++--
>  1 file changed, 34 insertions(+), 46 deletions(-)
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7816: Switch to the gpio descriptor interface

[Lars-Peter Clausen]

On 10/16/2018 04:46 PM, Nishad Kamdar wrote:
> Use the gpiod interface for rdwr_pin, convert_pin and busy_pin
> instead of the deprecated old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar 

Hi,

Thanks for the patch, this looks good.

One thing about the error messages though.

> + chip->rdwr_pin = devm_gpiod_get(_dev->dev, "rdwr", GPIOD_IN);
> + if (IS_ERR(chip->rdwr_pin)) {
> + ret = PTR_ERR(chip->rdwr_pin);
>   dev_err(_dev->dev, "Fail to request rdwr gpio PIN %d.\n",
> - chip->rdwr_pin);
> + ret);

This previously showed the pin number which has now been replaced with the
error code. The message doesn't make that much sense semantically anymore.
Maybe replace it with something like

"Failed to request rdwr GPIO: %d\n", ret

>   return ret;
___
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

[Lars-Peter Clausen]

On 10/01/2018 09:33 PM, Slawomir Stepien wrote:
> 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 

Hi,

Thanks for the patch. This looks good. But maybe a small suggestion further
improvement.

> ---
>  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 = >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.
___
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

[Lars-Peter Clausen]

On 10/01/2018 02:52 PM, Dan Carpenter wrote:
> On Mon, Oct 01, 2018 at 02:28:34PM +0200, Slawomir Stepien wrote:
>> 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);
> 
> 
> This doesn't work.  That's not arithmatic, it's a string which is passed
> to a macro.  It's an ugly macro, but it would take a lot of work to
> change it at this point.

>From a technical perspective it is easy to change this. We already had
patches for that. The difficulty is convincing Greg that this would be a
sensible change to pass the string as a string and not a preprocessor token.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio:adc:ad7280a: Use GFP_ATOMIC in interrupt handler

[Lars-Peter Clausen]

On 06/29/2018 01:55 AM, Karim Eshapa wrote:
> Use GFP_ATOMIC rather GFP_KERNEL in interrupt handler,
> as GFP_KERNEL may sleep according to slab allocator.

This is a threaded interrupt. Sleeping is OK.

> 
> Signed-off-by: Karim Eshapa 
> ---
>  drivers/staging/iio/adc/ad7280a.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7280a.c 
> b/drivers/staging/iio/adc/ad7280a.c
> index b736275c10f5..25c4fbc303f7 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -692,7 +692,7 @@ static irqreturn_t ad7280_event_handler(int irq, void 
> *private)
>   unsigned int *channels;
>   int i, ret;
>  
> - channels = kcalloc(st->scan_cnt, sizeof(*channels), GFP_KERNEL);
> + channels = kcalloc(st->scan_cnt, sizeof(*channels), GFP_ATOMIC);
>   if (!channels)
>   return IRQ_HANDLED;
>  
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio:adc:ad7816: Backward resources cleanups in probe

[Lars-Peter Clausen]

On 06/19/2018 01:38 AM, Karim Eshapa wrote:
> Backward cleanups for all resources allocated in probing
> in case of failure at any regestering or allocation step.

Hi,

Thanks for the patch.

Resources that are allocated with devm_ are freed automatically in case of
an error, so this patch should not be necessary.

> 
> Signed-off-by: Karim Eshapa 
> ---
>  drivers/staging/iio/adc/ad7816.c | 22 +-
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c 
> b/drivers/staging/iio/adc/ad7816.c
> index bf76a8620bdb..5ff14c830451 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -373,7 +373,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   if (ret) {
>   dev_err(_dev->dev, "Fail to request rdwr gpio PIN %d.\n",
>   chip->rdwr_pin);
> - return ret;
> + goto device_free;
>   }
>   gpio_direction_input(chip->rdwr_pin);
>   ret = devm_gpio_request(_dev->dev, chip->convert_pin,
> @@ -381,7 +381,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   if (ret) {
>   dev_err(_dev->dev, "Fail to request convert gpio PIN %d.\n",
>   chip->convert_pin);
> - return ret;
> + goto free_rdwr_pin;
>   }
>   gpio_direction_input(chip->convert_pin);
>   ret = devm_gpio_request(_dev->dev, chip->busy_pin,
> @@ -389,7 +389,7 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   if (ret) {
>   dev_err(_dev->dev, "Fail to request busy gpio PIN %d.\n",
>   chip->busy_pin);
> - return ret;
> + goto free_convert_pin;
>   }
>   gpio_direction_input(chip->busy_pin);
>  
> @@ -407,17 +407,29 @@ static int ad7816_probe(struct spi_device *spi_dev)
>   indio_dev->name,
>   indio_dev);
>   if (ret)
> - return ret;
> + goto free_busy_pin;
>   }
>  
>   ret = devm_iio_device_register(_dev->dev, indio_dev);
>   if (ret)
> - return ret;
> + goto free_dev_irq;
>  
>   dev_info(_dev->dev, "%s temperature sensor and ADC registered.\n",
>indio_dev->name);
>  
>   return 0;
> +free_dev_irq:
> + devm_free_irq(_dev->dev, spi_dev->irq, indio_dev);
> +free_busy_pin:
> + devm_gpio_free(_dev->dev, chip->busy_pin);
> +free_convert_pin:
> + devm_gpio_free(_dev->dev, chip->convert_pin);
> +free_rdwr_pin:
> + devm_gpio_free(_dev->dev, chip->rdwr_pin);
> +device_free:
> + devm_iio_device_free(_dev->dev, indio_dev);
> +
> + return ret;
>  }
>  
>  static const struct spi_device_id ad7816_id[] = {
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier

[Lars-Peter Clausen]

On 02/18/2018 01:02 PM, Jonathan Cameron wrote:
> On Sun, 18 Feb 2018 17:07:57 +0530
> Shreeya Patel  wrote:
> 
>> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
>>> Hi Shreeya,
>>>   
>> Hi Himanshu,
>>
>>> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:  

 Use SPDX identifier format instead of GPLv2. Also rearrange the
 headers in alphabetical order.

 Signed-off-by: Shreeya Patel 
 ---
  drivers/staging/iio/accel/adis16209.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)

 diff --git a/drivers/staging/iio/accel/adis16209.c
 b/drivers/staging/iio/accel/adis16209.c
 index 7fcef9a..e3d9f80 100644
 --- a/drivers/staging/iio/accel/adis16209.c
 +++ b/drivers/staging/iio/accel/adis16209.c
 @@ -1,19 +1,18 @@
 +// SPDX-License-Identifier: GPL-2.0+
  /*
   * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
   *
   * Copyright 2010 Analog Devices Inc.
 - *
 - * Licensed under the GPL-2 or later.  
>>> I see that you too are doing similar cleanup which I did a while ago
>>> https://lkml.org/lkml/2018/2/12/255  
>>
>> Yes, Jonathan suggested me to work on adis16209.
>> Your patches were quite useful for me :)
>>
>>> where I got some update suggestions for the patch series. It would be
>>> great if you could update this patch series consistent with the
>>> reviewers.
>>>
>>> For eg: in this patch you changed 
>>>
>>> +// SPDX-License-Identifier: GPL-2.0+
>>>
>>> and therefore
>>>
>>> MODULE_LICENSE("GPL v2");
>>>
>>> should also be changed to 
>>>
>>> MODULE_LICENSE("GPL"); 
>>>
>>> as explained by Philippe Ombredanne to me in my patch series.  
>>
> I'm not sure that was exactly what Philippe was suggesting.
> He was making the point that the licensing was inconsistent without
> saying which option should be chosen.
> 
> We will need to seek clarification from Analog Devices
> on what their opinion on this is.
> 
> Lars / Michael, any clarification on the right way to resolve this
> inconsistency?

I can't speak for the intended license for code I wasn't involved in.

But I'd in general if there are conflicting licensing information and you
want to be on the safe side choose the more restrictive license. E.g. GPL2+
is compatible with GPL2, but GPL2 is not compatible with GPL2+. So to be
compatible with both choose GPL2.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier

[Lars-Peter Clausen]

On 02/18/2018 01:08 PM, Lars-Peter Clausen wrote:
> On 02/18/2018 01:02 PM, Jonathan Cameron wrote:
>> On Sun, 18 Feb 2018 17:07:57 +0530
>> Shreeya Patel <shreeya.patel23...@gmail.com> wrote:
>>
>>> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
>>>> Hi Shreeya,
>>>>   
>>> Hi Himanshu,
>>>
>>>> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:  
>>>>>
>>>>> Use SPDX identifier format instead of GPLv2. Also rearrange the
>>>>> headers in alphabetical order.
>>>>>
>>>>> Signed-off-by: Shreeya Patel <shreeya.patel23...@gmail.com>
>>>>> ---
>>>>>  drivers/staging/iio/accel/adis16209.c | 7 +++
>>>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/iio/accel/adis16209.c
>>>>> b/drivers/staging/iio/accel/adis16209.c
>>>>> index 7fcef9a..e3d9f80 100644
>>>>> --- a/drivers/staging/iio/accel/adis16209.c
>>>>> +++ b/drivers/staging/iio/accel/adis16209.c
>>>>> @@ -1,19 +1,18 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>>  /*
>>>>>   * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
>>>>>   *
>>>>>   * Copyright 2010 Analog Devices Inc.
>>>>> - *
>>>>> - * Licensed under the GPL-2 or later.  
>>>> I see that you too are doing similar cleanup which I did a while ago
>>>> https://lkml.org/lkml/2018/2/12/255  
>>>
>>> Yes, Jonathan suggested me to work on adis16209.
>>> Your patches were quite useful for me :)
>>>
>>>> where I got some update suggestions for the patch series. It would be
>>>> great if you could update this patch series consistent with the
>>>> reviewers.
>>>>
>>>> For eg: in this patch you changed 
>>>>
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>
>>>> and therefore
>>>>
>>>> MODULE_LICENSE("GPL v2");
>>>>
>>>> should also be changed to 
>>>>
>>>> MODULE_LICENSE("GPL"); 
>>>>
>>>> as explained byPhilippe Ombredanne to me in my patch series.  
>>>
>> I'm not sure that was exactly what Philippe was suggesting.
>> He was making the point that the licensing was inconsistent without
>> saying which option should be chosen.
>>
>> We will need to seek clarification from Analog Devices
>> on what their opinion on this is.
>>
>> Lars / Michael, any clarification on the right way to resolve this
>> inconsistency?
> 
> I can't speak for the intended license for code I wasn't involved in.
> 
> But I'd in general if there are conflicting licensing information and you
> want to be on the safe side choose the more restrictive license. E.g. GPL2+
> is compatible with GPL2, but GPL2 is not compatible with GPL2+. So to be
> compatible with both choose GPL2.

This is not legal advice btw.

I personally would stay away from messing with the licenses of code I do not
own. Not everybody seems to agree yet that a SPDX tag is equivalent to a
explicit licensing statement.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

[Lars-Peter Clausen]

On 09/13/2017 11:29 PM, Greg KH wrote:
> On Wed, Sep 13, 2017 at 09:23:31PM +0200, Lars-Peter Clausen wrote:
>> On 09/13/2017 08:58 PM, Greg KH wrote:
>>> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
>>>> On Wed, 13 Sep 2017 14:14:07 +0530
>>>> Himanshi Jain <himshijain...@gmail.com> wrote:
>>>>
>>>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
>>>>> string instead of implicit conversion of argument to string using
>>>>> the macro _stringify(_name).
>>>>>
>>>>> Signed-off-by: Himanshi Jain <himshijain...@gmail.com>
>>>>> ---
>>>>>  include/linux/sysfs.h | 7 +++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>>>> index aa02c32..20321cf 100644
>>>>> --- a/include/linux/sysfs.h
>>>>> +++ b/include/linux/sysfs.h
>>>>> @@ -104,6 +104,13 @@ struct attribute_group {
>>>>>   .store  = _store,   \
>>>>>  }
>>>>>  
>>>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) {  
>>>>> \
>>>>
>>>> I'm not sure about the naming here.  The normal __ATTR macro is also
>>>> 'named'.  Maybe something as awful as
>>>>
>>>> __ATTR_STRING_NAME ? 
>>>>
>>>> Greg what do you think?
>>>
>>> ick ick ick.
>>>
>>>> This is all to allow us to have names with operators in them without
>>>> checkpatch complaining about them... A worthwhile aim just to stop
>>>> more people wasting time trying to 'fix' those cases by adding spaces.
>>>
>>> Yeah, but this really seems "heavy" for just a crazy sysfs name in a
>>> macro.  Adding a whole new "core" define for that is a hard sell...
>>>
>>> I also want to get rid of the "generic" __ATTR type macros, and force
>>> people to use the proper _RW and friends instead.  I don't want to add
>>> another new one that people will start to use that I later have to
>>> change...
>>>
>>> So no, I don't like this, how about just changing your macros instead?
>>> No one else has this problem :)
>>
>> Nobody else realized they have this problem yet. E.g. there are a few users
>> of __ATTR in block/genhd.c that have the same issue and are likely to
>> generate the same false positives from static checkers.
> 
> Then fix the broken static checkers :)

The static checkers aren't broken, the macro is. It takes a string
parameter, but instead of a string it is passed as an expression and then
transformed to string using preprocessor magic in the macro. That's not very
good semantics. And hence this gets detected as a false positive, because
nobody expects such strange behavior.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7192: Use the dedicated reset function

[Lars-Peter Clausen]

On 09/14/2017 03:50 PM, Stefan Popa wrote:
> SPI host drivers can use DMA to transfer data, so the buffer should be 
> properly allocated.
> Keeping it on the stack could cause an undefined behavior.
> 
> The dedicated reset function solves this issue.
> 
> Signed-off-by: Stefan Popa <stefan.p...@analog.com>

Acked-by: Lars-Peter Clausen <l...@metafoo.de>

Thanks.

> ---
>  drivers/staging/iio/adc/ad7192.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7192.c 
> b/drivers/staging/iio/adc/ad7192.c
> index d11c6de..6150d27 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -223,11 +223,9 @@ static int ad7192_setup(struct ad7192_state *st,
>   struct iio_dev *indio_dev = spi_get_drvdata(st->sd.spi);
>   unsigned long long scale_uv;
>   int i, ret, id;
> - u8 ones[6];
>  
>   /* reset the serial interface */
> - memset(, 0xFF, 6);
> - ret = spi_write(st->sd.spi, , 6);
> + ret = ad_sd_reset(>sd, 48);
>   if (ret < 0)
>   goto out;
>   usleep_range(500, 1000); /* Wait for at least 500us */
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/2] include: linux: sysfs: Add __ATTR_NAMED macro

[Lars-Peter Clausen]

On 09/13/2017 08:58 PM, Greg KH wrote:
> On Wed, Sep 13, 2017 at 06:03:10PM +0100, Jonathan Cameron wrote:
>> On Wed, 13 Sep 2017 14:14:07 +0530
>> Himanshi Jain  wrote:
>>
>>> Add __ATTR_NAMED macro similar to __ATTR but taking name as a
>>> string instead of implicit conversion of argument to string using
>>> the macro _stringify(_name).
>>>
>>> Signed-off-by: Himanshi Jain 
>>> ---
>>>  include/linux/sysfs.h | 7 +++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
>>> index aa02c32..20321cf 100644
>>> --- a/include/linux/sysfs.h
>>> +++ b/include/linux/sysfs.h
>>> @@ -104,6 +104,13 @@ struct attribute_group {
>>> .store  = _store,   \
>>>  }
>>>  
>>> +#define __ATTR_NAMED(_name, _mode, _show, _store) {
>>> \
>>
>> I'm not sure about the naming here.  The normal __ATTR macro is also
>> 'named'.  Maybe something as awful as
>>
>> __ATTR_STRING_NAME ? 
>>
>> Greg what do you think?
> 
> ick ick ick.
> 
>> This is all to allow us to have names with operators in them without
>> checkpatch complaining about them... A worthwhile aim just to stop
>> more people wasting time trying to 'fix' those cases by adding spaces.
> 
> Yeah, but this really seems "heavy" for just a crazy sysfs name in a
> macro.  Adding a whole new "core" define for that is a hard sell...
> 
> I also want to get rid of the "generic" __ATTR type macros, and force
> people to use the proper _RW and friends instead.  I don't want to add
> another new one that people will start to use that I later have to
> change...
> 
> So no, I don't like this, how about just changing your macros instead?
> No one else has this problem :)

Nobody else realized they have this problem yet. E.g. there are a few users
of __ATTR in block/genhd.c that have the same issue and are likely to
generate the same false positives from static checkers.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names

[Lars-Peter Clausen]

On 09/12/2017 08:06 PM, Julia Lawall wrote:
> 
> 
> On Tue, 12 Sep 2017, himanshi wrote:
> 
>> Thanks for the review Daniel! I will change the imperative mood for the 
>> commit
>> message once the other changes are finalised too and as suggested by Julia,
>> would try to make the description specific than general.
>>
>> I tried to think of adding subsystem to the commit subject but could not
>> conclude any because of the files involved.
>> I like the idea of splitting the patch into 2 as you suggested but I
>> have a doubt that adding the new MACROS to different sysfs files can be put 
>> into 1
>> patch with the subsystem you mentioned but changing the existing
>> IIO_DEVICE_ATTR_NAMED to use IIO_ATTR_NAMED (sysfs file again) would be 
>> included
>> in the second patch if I am not wrong. So would it be fine to keep the
>> subsystem as iio for the second patch?
> 
> Indeed, the kernel has to compile after every commit.  Unless you change
> the name of the macro, to allow the old and new versions to co-exist, it
> seems hard to break up such a patch.

We can still split things into two parts. One patch introducing __ATTR_NAMED
in the device driver core and then another patch making use of that macro in
the IIO subsystem.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] [PATCH] Fixed IIO_DEVICE_ATTR_NAMED API to take name as a string and added "" around names

[Lars-Peter Clausen]

On 09/12/2017 09:53 AM, Daniel Baluta wrote:
> Hi Himanshi,
> 
> On Tue, Sep 12, 2017 at 1:43 AM, Himanshi Jain  
> wrote:
>> Fixed IIO_DEVICE_ATTR_NAMED API to take name as a
>> string instead of implicit conversion to string using
>> preprocessors. Added double quotes around names in
>> existing usage of IIO_DEVICE_ATTR_NAMED.
> 
> Always use imperative mood in commit subject (Fix instead of Fixed).
> 
> The subject should contain a tag, which describes the subsytem/files affected.
> 
> I would split this patch into:
> 
> 1) sysfs: iio: Introduce *_ATTR_NAMED
> 
> and explain here why do we need  __ATTR_NAMED and IIO_ATTR_NAMED
> 
> 2) iio: Use new IIO_DEVICE_ATTR_NAMED API
> 
> But of course, lets wait to see Lars and Jonathan's opinions.

Fully agreed with what you wrote.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:59 AM, Julia Lawall wrote:
> 
> 
> On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> 
>> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
>>> On Fri, 8 Sep 2017 07:29:06 +0100
>>> Jonathan Cameron <ji...@jic23.retrosnub.co.uk> wrote:
>>>
>>>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain...@gmail.com> 
>>>> wrote:
>>>>> Added space around(one on each side of) binary
>>>>> operator(-) as preferred according to kernel
>>>>> coding style.
>>>>>
>>>>> Signed-off-by: Himanshi Jain <himshijain...@gmail.com>
>>>>
>>>> Take a closer look at that macro. It isn't doing what you think...
>>>> To give a hint, changing this breaks userspace.
>>>
>>> Ok, I'm bored of this particular one coming up. When you have
>>> worked out what is going on Himanshi, would you mind putting
>>> together a patch adding a comment describing why it is a bad
>>> idea to 'fix' this?  That would be a very useful patch as
>>> far as I'm concerned :)
>>>
>>> There aren't that many cases of this in IIO so adding a comment
>>> on each of them is probably reasonable just to avoid wasting
>>> people's time on fixing them! (I think we have had more than
>>> 5 such goes this year so far...)
>>>
>>
>> I'd much rather fix this API so that you have to put "" around the name so
>> that it is clear that it is a string, rather than doing the implicit
>> conversion to string using preprocessor magic. Better to have a
>> self-documenting API then having to add a comment to each user of the API.
> 
> All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
> eg DEVICE_ATTR_RW, also use the provided name to construct the names of
> the show and store functions, so I don't think a string would be
> appropriate.

I'm only suggesting to use a string for the _NAMED macros where the name is
not used to construct the identifiers.

In the case where the name is used to construct the identifiers we don't
have the issue with false positives since the name must be a valid
identifier on its own.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> On Fri, 8 Sep 2017 07:29:06 +0100
> Jonathan Cameron  wrote:
> 
>> On 8 September 2017 05:47:52 BST, Himanshi Jain  
>> wrote:
>>> Added space around(one on each side of) binary
>>> operator(-) as preferred according to kernel
>>> coding style.
>>>
>>> Signed-off-by: Himanshi Jain   
>>
>> Take a closer look at that macro. It isn't doing what you think...  
>> To give a hint, changing this breaks userspace.
> 
> Ok, I'm bored of this particular one coming up. When you have
> worked out what is going on Himanshi, would you mind putting
> together a patch adding a comment describing why it is a bad
> idea to 'fix' this?  That would be a very useful patch as
> far as I'm concerned :)
> 
> There aren't that many cases of this in IIO so adding a comment
> on each of them is probably reasonable just to avoid wasting
> people's time on fixing them! (I think we have had more than
> 5 such goes this year so far...)
> 

I'd much rather fix this API so that you have to put "" around the name so
that it is clear that it is a string, rather than doing the implicit
conversion to string using preprocessor magic. Better to have a
self-documenting API then having to add a comment to each user of the API.

> Jonathan
> 
>>
>> Jonathan
>>
>>
>>> ---
>>> drivers/staging/iio/adc/ad7192.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/ad7192.c
>>> b/drivers/staging/iio/adc/ad7192.c
>>> index d11c6de..1aee662 100644
>>> --- a/drivers/staging/iio/adc/ad7192.c
>>> +++ b/drivers/staging/iio/adc/ad7192.c
>>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
>>> }
>>>
>>> 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,  
>>
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCHv2] staging: iio: adc: add missing of_node references to iio_dev

[Lars-Peter Clausen]

Hi,

The patch seems to be reverted?

Also should this part in the IIO core take care of automatically setting the
of_node of the IIO device? As far as I can see we don't have to initialize
it manually.

/* If the calling driver did not initialize of_node, do it here */
if (!indio_dev->dev.of_node && indio_dev->dev.parent)
indio_dev->dev.of_node = indio_dev->dev.parent->of_node;

- Lars

On 07/17/2017 02:34 PM, Hari Prasath wrote:
> Adding missing indio_dev->dev.of_node references to allow iio consumers
> to access the device channels.
> 
> Signed-off-by: Hari Prasath 
> ---
>   v2: Wrong from mail ID in first version of patch
> ---
>  drivers/staging/iio/adc/ad7192.c | 1 -
>  drivers/staging/iio/adc/ad7780.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7192.c 
> b/drivers/staging/iio/adc/ad7192.c
> index 78308a5..d11c6de 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -668,7 +668,6 @@ static int ad7192_probe(struct spi_device *spi)
>   spi_set_drvdata(spi, indio_dev);
>   st->devid = spi_get_device_id(spi)->driver_data;
>   indio_dev->dev.parent = >dev;
> - indio_dev->dev.of_node = spi->dev.of_node;
>   indio_dev->name = spi_get_device_id(spi)->name;
>   indio_dev->modes = INDIO_DIRECT_MODE;
>  
> diff --git a/drivers/staging/iio/adc/ad7780.c 
> b/drivers/staging/iio/adc/ad7780.c
> index e38d2ab9..dec3ba6 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -195,7 +195,6 @@ static int ad7780_probe(struct spi_device *spi)
>   spi_set_drvdata(spi, indio_dev);
>  
>   indio_dev->dev.parent = >dev;
> - indio_dev->dev.of_node = spi->dev.of_node;
>   indio_dev->name = spi_get_device_id(spi)->name;
>   indio_dev->modes = INDIO_DIRECT_MODE;
>   indio_dev->channels = >chip_info->channel;
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: New Driver for electrical energy measurement

[Lars-Peter Clausen]

On 07/14/2017 11:00 AM, Guenter Roeck wrote:
> On 07/14/2017 01:07 AM, Jonathan Cameron wrote:
>> On Wed, 12 Jul 2017 15:19:40 +0200
>> Christian Gromm  wrote:
>>
>>> On Wed, 12 Jul 2017 14:51:01 +0200
>>> Greg KH  wrote:
>>>
 On Wed, Jul 12, 2017 at 02:18:54PM +0200, Christian Gromm wrote:
>
> Hi,
>
> Microchip is planning to introduce a driver for a new companion
> chip series capable of electical energy measurement. However, we
> are not sure which location in the source tree is the most
> suitable. First thought was to put it into /drivers/powercap. But
> now we are in discussions whether it would make even more sense to
> introduce a new driver for electrical energy measuring or
> monitoring.

 Why not just have it be an IIO device, I think they have energy
 sensors already (adding the iio mailing list to find out...)
>>>
>>> We thought about hwmon, but not IIO.
>>>
   
>
> Following is a rough overview of the part we want the driver for.
> Any input on this will be appreciated.
>
> thanks,
> Chris
>
> === Introduction
>
> Following the recent focus on low-power systems, Microchip has
> designed a companion chip series capable of measuring the
> electrical energy flow of an electrical system.  The series in
> question, EM Chip  is capable of measuring the energy flow (in or
> out) of an electrical system by monitoring one or more power
> rails.  The 1st chip of the series, EM Chip is able to measure the
> net energy flow over 4 different power rails.  The chip itself is a
> small QFN16 (4mm x 4mm x 0.5mm) or WLCSP16 (2.25mm x 2.2mm) package.
>
> === Theory of Operation
>
> In order to measure the amount of energy entering/exiting a system,
> a small shunt resistor is interleaved on a power rail. By measuring
> the small voltage drop across the shunt resistor (a known value),
> the electrical current is measured.
>
> I = U/R
>
> Depending on whether the system is consuming or producing energy,
> the measured current value can be either a positive or negative
> number.
>
> To get the instantaneous power figure we will have to measure the
> power rail’s voltage and multiply it with the measured value for
> the current
>
> P = V * I = V * U_shunt/R
>
> Knowing the instantaneous power and by making a high enough sample
> rate (for measuring the current and the power rail voltage), we can
> assume the measured value for power is equal to the average power
> figure for the amount of time between 2 sampling moments.
>
> Now that we also know the average power for a given time interval
> (dt_x = time between 2 sampling moments; sampling speed is known),
> we can measure the energy amount entering or exiting the system
> between 2 sampling moments
>
> E_partial_x = P * dt_x
>
> Having the energy information available, we can continue to add the
> subsequent energy values for as long as the system is active. The
> amount of energy is the sum of all the partial energy values
> measured for each time interval E = Sum (E_partial_x), where x can
> take values from 0 till infinite.
>
>
> === Chip Operation
>
> The EM Chip chip uses the same principles of operation as presented
> in the “Theory of Operation” and it does so for a number of 4
> channels. 4 independent power rails can be energy monitored in the
> same time.  The chip is controlled over I2C/SMBus by an I2C/SMBus
> master.
>
> In order to reduce the amount of information traffic between the EM
> Chip and the entity asking for its information, the chip features
> large accumulator energy registers for the accumulated energy
> values. It can accumulate up to 2^24 power values. Depending on the
> selected chip’s sampling speed, such a number of samples are
> produced in about 4.5 hours (fastest sampling speed) up to 582.5
> hours or more than 24 days (lowest sampling speed).
>
> EM Chip measures the power as a 28-bits number. The 28-bits number
> is the multiplication result of the 16-bits number used to measure
> the power rail voltage and the 12-bits number used for measuring
> the voltage drop across the power rail shunt resistor.
>
> If bidirectional energy flow is required, the power is measured as a
> 27-bits number and 1 bit for the sign. When no bidirectional flow is
> needed, the power value is measured as an unsigned 28-bits number.
>
> Due to the relatively large size of the accumulated energy
> registers inside the chip (48 bits), in the worst case scenario
> (power values are full scale numbers), a number of 2^20 full-scale
> power values can be measured before energy register’s 

Re: [PATCH] staging: iio: ad7152: Fix deadlock in ad7152_write_raw_samp_freq()

[Lars-Peter Clausen]

On 05/27/2017 12:53 AM, Alexey Khoroshilov wrote:
> ad7152_write_raw_samp_freq() is called by ad7152_write_raw() with
> chip->state_lock held. So, there is unavoidable deadlock when
> ad7152_write_raw_samp_freq() locks the mutex itself.
> 
> The patch removes unneeded locking.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Signed-off-by: Alexey Khoroshilov <khoroshi...@ispras.ru>

Looks good, thanks.

Fixes: 6572389bcc11 ("staging: iio: cdc: ad7152: Implement
IIO_CHAN_INFO_SAMP_FREQ attribute")
Acked-by: Lars-Peter Clausen <l...@metafoo.de>


> ---
>  drivers/staging/iio/cdc/ad7152.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7152.c 
> b/drivers/staging/iio/cdc/ad7152.c
> index dc6ecd824365..ff10d1f0a7e4 100644
> --- a/drivers/staging/iio/cdc/ad7152.c
> +++ b/drivers/staging/iio/cdc/ad7152.c
> @@ -231,16 +231,12 @@ static int ad7152_write_raw_samp_freq(struct device 
> *dev, int val)
>   if (i >= ARRAY_SIZE(ad7152_filter_rate_table))
>   i = ARRAY_SIZE(ad7152_filter_rate_table) - 1;
>  
> - mutex_lock(>state_lock);
>   ret = i2c_smbus_write_byte_data(chip->client,
>   AD7152_REG_CFG2, AD7152_CFG2_OSR(i));
> - if (ret < 0) {
> - mutex_unlock(>state_lock);
> + if (ret < 0)
>   return ret;
> - }
>  
>   chip->filter_rate_setup = i;
> - mutex_unlock(>state_lock);
>  
>   return ret;
>  }
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: tsl2x7x: Moved contents of the header to the source file.

[Lars-Peter Clausen]

On 03/23/2017 12:53 PM, Arushi Singhal wrote:
> Moved the contents of the header(tsl2x7x.h) into the source file
> tsl2x7x_core.c with the exception of the platform data struct which is
> supposed to be used from somewhere else other than the driver.

The platform_data struct uses the other structs though.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: ad7746: Moved contents of the header to the source file.

[Lars-Peter Clausen]

On 03/23/2017 12:36 PM, Arushi Singhal wrote:
> Moved the contents of the header(ad7746.h) into the source file
> ad7746.c with the exception of the platform data struct which is
> supposed to be used from somewhere else other than the driver.
> 
> Signed-off-by: Arushi Singhal 
> ---
>  drivers/staging/iio/cdc/ad7746.c | 5 +
>  drivers/staging/iio/cdc/ad7746.h | 5 -
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c 
> b/drivers/staging/iio/cdc/ad7746.c
> index 2d8397b11b19..033a41fd9bee 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -21,6 +21,11 @@
>  
>  #include "ad7746.h"
>  
> +#define AD7466_EXCLVL_0 0 /* +-VDD/8 */
> +#define AD7466_EXCLVL_1 1 /* +-VDD/4 */
> +#define AD7466_EXCLVL_2 2 /* +-VDD * 3/8 */
> +#define AD7466_EXCLVL_3 3 /* +-VDD/2 */
> +
>  /*
>   * AD7746 Register Definition
>   */
> diff --git a/drivers/staging/iio/cdc/ad7746.h 
> b/drivers/staging/iio/cdc/ad7746.h
> index ea8572d1df02..2fbcee88fda6 100644
> --- a/drivers/staging/iio/cdc/ad7746.h
> +++ b/drivers/staging/iio/cdc/ad7746.h
> @@ -13,11 +13,6 @@
>   * TODO: struct ad7746_platform_data needs to go into include/linux/iio
>   */
>  
> -#define AD7466_EXCLVL_0  0 /* +-VDD/8 */
> -#define AD7466_EXCLVL_1  1 /* +-VDD/4 */
> -#define AD7466_EXCLVL_2  2 /* +-VDD * 3/8 */
> -#define AD7466_EXCLVL_3  3 /* +-VDD/2 */
> -

I believe those are settings for the exclvl field in the platform data
struct. Hence this has to stay here.

>  struct ad7746_platform_data {
>   unsigned char exclvl;   /*Excitation Voltage Level */
>   bool exca_en;   /* enables EXCA pin as the excitation output */
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: Replace a bit shift by a use of BIT.

[Lars-Peter Clausen]

On 03/22/2017 09:38 AM, Arushi Singhal wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
> @@
> constant c;
> @@
> 
> -1 << c
> +BIT(c)

When it comes to doing this type of conversion semantics, i.e. the meaning
of the value, are important. The BIT() macro should only be used when the
value is really a 1 bit value. Otherwise it will be confusing to the reader
of the code.

Also one patch per driver please (Unless it is an absolutely identical
change to multiple driver, not just the same class of change).

> 
> Signed-off-by: Arushi Singhal 
> ---
> changes in v2
>  -change the cc list
> 
>  drivers/staging/iio/adc/ad7816.c |  2 +-
>  drivers/staging/iio/cdc/ad7150.c |  2 +-
>  drivers/staging/iio/cdc/ad7746.c | 22 +++---
>  3 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c 
> b/drivers/staging/iio/adc/ad7816.c
> index 17d280581e24..42f637ca0251 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -222,7 +222,7 @@ static ssize_t ad7816_show_value(struct device *dev,
>   value = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
>   data &= AD7816_TEMP_FLOAT_MASK;
>   if (value < 0)
> - data = (1 << AD7816_TEMP_FLOAT_OFFSET) - data;
> + data = (BIT(AD7816_TEMP_FLOAT_OFFSET)) - data;

This for example is just a really poor implementation of unsigned binary to
signed conversion (The formula I think is straight from the datasheet).
Semantically talking about a 1-bit value here makes no sense. What the code
does is to construct a value that is equal to 2**AD7816_TEMP_FLOAT_OFFSET.

>   return sprintf(buf, "%d.%.2d\n", value, data * 25);
>   }
>   return sprintf(buf, "%u\n", data);
> diff --git a/drivers/staging/iio/cdc/ad7150.c 
> b/drivers/staging/iio/cdc/ad7150.c
> index ca72af3e9d4b..c5574b3ee939 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -232,7 +232,7 @@ static int ad7150_write_event_config(struct iio_dev 
> *indio_dev,
>   if (ret < 0)
>   goto error_ret;
>  
> - cfg = ret & ~((0x03 << 5) | (0x1 << 7));
> + cfg = ret & ~((0x03 << 5) | (BIT(7)));

This is OK. It's a one bit field that is set here.

>  
>   switch (type) {
>   case IIO_EV_TYPE_MAG_ADAPTIVE:
> diff --git a/drivers/staging/iio/cdc/ad7746.c 
> b/drivers/staging/iio/cdc/ad7746.c
> index 81f8b9ee1120..8c573817826f 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -45,20 +45,20 @@
>  #define AD7746_STATUS_RDYCAP BIT(0)
>  
>  /* Capacitive Channel Setup Register Bit Designations (AD7746_REG_CAP_SETUP) 
> */
> -#define AD7746_CAPSETUP_CAPEN(1 << 7)
> -#define AD7746_CAPSETUP_CIN2 (1 << 6) /* AD7746 only */
> -#define AD7746_CAPSETUP_CAPDIFF  (1 << 5)
> -#define AD7746_CAPSETUP_CACHOP   (1 << 0)
> +#define AD7746_CAPSETUP_CAPEN(BIT(7))
> +#define AD7746_CAPSETUP_CIN2 (BIT(6)) /* AD7746 only */
> +#define AD7746_CAPSETUP_CAPDIFF  (BIT(5))
> +#define AD7746_CAPSETUP_CACHOP   (BIT(0))

These are all OK, they are single bit values or flags.

>  
>  /* Voltage/Temperature Setup Register Bit Designations (AD7746_REG_VT_SETUP) 
> */
> -#define AD7746_VTSETUP_VTEN  (1 << 7)
> +#define AD7746_VTSETUP_VTEN  (BIT(7))
>  #define AD7746_VTSETUP_VTMD_INT_TEMP (0 << 5)
> -#define AD7746_VTSETUP_VTMD_EXT_TEMP (1 << 5)
> +#define AD7746_VTSETUP_VTMD_EXT_TEMP (BIT(5))

This is not OK. This is a multi-bit field. b01 just happens to be one of the
valid values.

>  #define AD7746_VTSETUP_VTMD_VDD_MON  (2 << 5)
>  #define AD7746_VTSETUP_VTMD_EXT_VIN  (3 << 5)
> -#define AD7746_VTSETUP_EXTREF(1 << 4)
> -#define AD7746_VTSETUP_VTSHORT   (1 << 1)
> -#define AD7746_VTSETUP_VTCHOP(1 << 0)
> +#define AD7746_VTSETUP_EXTREF(BIT(4))
> +#define AD7746_VTSETUP_VTSHORT   (BIT(1))
> +#define AD7746_VTSETUP_VTCHOP(BIT(0))

These are OK. All single bit values.

>  
>  /* Excitation Setup Register Bit Designations (AD7746_REG_EXC_SETUP) */
>  #define AD7746_EXCSETUP_CLKCTRL  BIT(7)
> @@ -75,14 +75,14 @@
>  #define AD7746_CONF_VTFS_MASKGENMASK(7, 6)
>  #define AD7746_CONF_CAPFS_MASK   GENMASK(5, 3)
>  #define AD7746_CONF_MODE_IDLE(0 << 0)
> -#define AD7746_CONF_MODE_CONT_CONV   (1 << 0)
> +#define AD7746_CONF_MODE_CONT_CONV   (BIT(0))

This again is a multi-bit field where 1 is one of the possible values. So
not OK.

>  #define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
>  #define AD7746_CONF_MODE_PWRDN   (3 << 0)
>  #define AD7746_CONF_MODE_OFFS_CAL(5 << 0)
>  #define AD7746_CONF_MODE_GAIN_CAL(6 << 0)
>  

Re: [PATCH v3] staging: ad7606: Replace mlock with driver private lock

[Lars-Peter Clausen]

On 03/20/2017 07:56 PM, Arushi Singhal wrote:
[...]
> @@ -413,6 +413,7 @@ int ad7606_probe(struct device *dev, int irq, void 
> __iomem *base_address,
>   st = iio_priv(indio_dev);
>  
>   st->dev = dev;
> + mutex_init(>lock);

This is nitpicking, but putting this in the middle of the assignments has a
bit of a weired flow it. Either put it above or below the assignments.

>   st->bops = bops;
>   st->base_address = base_address;
>   /* tied to logic low, analog input range is +/- 5V */
> diff --git a/drivers/staging/iio/adc/ad7606.h 
> b/drivers/staging/iio/adc/ad7606.h
> index 746f9553d2ba..5d59bdd78727 100644
> --- a/drivers/staging/iio/adc/ad7606.h
> +++ b/drivers/staging/iio/adc/ad7606.h
> @@ -14,6 +14,7 @@
>   * @name:identification string for chip
>   * @channels:channel specification
>   * @num_channels:number of channels
> + * @lock protect sensor state

This documentation is for struct ad7607_chip_info, but you are adding the
field to struct ad7606_state.

>   */
>  
>  struct ad7606_chip_info {
> @@ -37,6 +38,7 @@ struct ad7606_state {
>   booldone;
>   void __iomem*base_address;
>  
> + struct mutexlock; /* protect sensor state */
>   struct gpio_desc*gpio_convst;
>   struct gpio_desc*gpio_reset;
>   struct gpio_desc*gpio_range;
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RESEND PATCH] staging: ad7606: Replace mlock with driver private lock

[Lars-Peter Clausen]

On 03/20/2017 04:22 PM, Arushi Singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> In this driver, mlock was being used to protect hardware state
> changes.  Replace it with a lock in the devices global data.
> 
> Signed-off-by: Arushi Singhal 

Hi,

Looks good, but mutex_init() is missing.

- Lars

> ---
>  drivers/staging/iio/adc/ad7606.c | 8 
>  drivers/staging/iio/adc/ad7606.h | 2 ++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7606.c 
> b/drivers/staging/iio/adc/ad7606.c
> index 9dbfa64b1e53..9f529b34e018 100644
> --- a/drivers/staging/iio/adc/ad7606.c
> +++ b/drivers/staging/iio/adc/ad7606.c
> @@ -208,7 +208,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>   switch (mask) {
>   case IIO_CHAN_INFO_SCALE:
>   ret = -EINVAL;
> - mutex_lock(_dev->mlock);
> + mutex_lock(>lock);
>   for (i = 0; i < ARRAY_SIZE(scale_avail); i++)
>   if (val2 == scale_avail[i][1]) {
>   gpiod_set_value(st->gpio_range, i);
> @@ -217,7 +217,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>   ret = 0;
>   break;
>   }
> - mutex_unlock(_dev->mlock);
> + mutex_unlock(>lock);
>  
>   return ret;
>   case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> @@ -231,11 +231,11 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>   values[1] = (ret >> 1) & 1;
>   values[2] = (ret >> 2) & 1;
>  
> - mutex_lock(_dev->mlock);
> + mutex_lock(>lock);
>   gpiod_set_array_value(ARRAY_SIZE(values), st->gpio_os->desc,
> values);
>   st->oversampling = val;
> - mutex_unlock(_dev->mlock);
> + mutex_unlock(>lock);
>  
>   return 0;
>   default:
> diff --git a/drivers/staging/iio/adc/ad7606.h 
> b/drivers/staging/iio/adc/ad7606.h
> index 746f9553d2ba..5d59bdd78727 100644
> --- a/drivers/staging/iio/adc/ad7606.h
> +++ b/drivers/staging/iio/adc/ad7606.h
> @@ -14,6 +14,7 @@
>   * @name:identification string for chip
>   * @channels:channel specification
>   * @num_channels:number of channels
> + * @lock protect sensor state
>   */
>  
>  struct ad7606_chip_info {
> @@ -37,6 +38,7 @@ struct ad7606_state {
>   booldone;
>   void __iomem*base_address;
>  
> + struct mutexlock; /* protect sensor state */
>   struct gpio_desc*gpio_convst;
>   struct gpio_desc*gpio_reset;
>   struct gpio_desc*gpio_range;
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Lars-Peter Clausen]

On 03/13/2017 01:33 PM, SIMRAN SINGHAL wrote:
> On Mon, Mar 13, 2017 at 5:30 PM, Lars-Peter Clausen <l...@metafoo.de> wrote:
>> On 03/12/2017 02:32 PM, simran singhal wrote:
>>> The IIO subsystem is redefining iio_dev->mlock to be used by
>>> the IIO core only for protecting device operating mode changes.
>>> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
>>>
>>> In this driver, mlock was being used to protect hardware state
>>> changes.  Replace it with a lock in the devices global data.
>>>
>>> Fix some coding style issues related to white space also.
>>>
>>> Signed-off-by: simran singhal <singhalsimr...@gmail.com>
>>> ---
>>>  drivers/staging/iio/meter/ade7753.c | 14 --
>>>  1 file changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/meter/ade7753.c 
>>> b/drivers/staging/iio/meter/ade7753.c
>>> index dfd8b71..ca99d82 100644
>>> --- a/drivers/staging/iio/meter/ade7753.c
>>> +++ b/drivers/staging/iio/meter/ade7753.c
>>> @@ -81,12 +81,14 @@
>>>   * @tx: transmit buffer
>>>   * @rx: receive buffer
>>>   * @buf_lock:   mutex to protect tx and rx
>>> + * @lock:protect sensor state
>>
>> It might make sense to reuse the existing lock which currently protects the
>> read/write functions. You can do this by introducing a variant of
>> ade7753_spi_{read,write}_reg_16() that does not take a lock and use these to
>> implement the read-modify-write cycle in a protected section.
>>
>> Looking through the driver there seem to be other places as well that do
>> read-modify-write that should be protected by a lock, but currently are not.
>> This might be a good task.
>>
> 
> Are you trying to say that their is no need of introducing "lock",
> I can using "buf_lock" only.

Yes, there should be no need for two locks. But you need to slightly
refactor the code to avoid taking the same lock nested.

> 
> Thanks!
> 
>>>   **/
>>>  struct ade7753_state {
>>> - struct spi_device   *us;
>>> - struct mutexbuf_lock;
>>> - u8  tx[ADE7753_MAX_TX] 
>>> cacheline_aligned;
>>> - u8  rx[ADE7753_MAX_RX];
>>> + struct spi_device   *us;
>>> + struct mutexbuf_lock;
>>> + struct mutexlock;   /* protect sensor state */
>>> + u8  tx[ADE7753_MAX_TX] cacheline_aligned;
>>> + u8  rx[ADE7753_MAX_RX];
>>>  };
>>>
>>>  static int ade7753_spi_write_reg_8(struct device *dev,
>>> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device 
>>> *dev,
>>>   if (!val)
>>>   return -EINVAL;
>>>
>>> - mutex_lock(_dev->mlock);
>>> + mutex_lock(>lock);
>>>
>>>   t = 27900 / val;
>>>   if (t > 0)
>>> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device 
>>> *dev,
>>>   ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>>>
>>>  out:
>>> - mutex_unlock(_dev->mlock);
>>> + mutex_unlock(>lock);
>>>
>>>   return ret ? ret : len;
>>>  }
>>>
>>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: adis16060_core: Use private driver lock instead of mlock

[Lars-Peter Clausen]

On 03/12/2017 02:10 PM, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> In this driver, mlock was being used to protect hardware state
> changes.  Replace it with a lock in the devices global data.
> 
> Signed-off-by: simran singhal 
> ---
>  drivers/staging/iio/gyro/adis16060_core.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/gyro/adis16060_core.c 
> b/drivers/staging/iio/gyro/adis16060_core.c
> index c9d46e7..90a3a18 100644
> --- a/drivers/staging/iio/gyro/adis16060_core.c
> +++ b/drivers/staging/iio/gyro/adis16060_core.c
> @@ -29,11 +29,13 @@
>   * @us_r:actual spi_device to read back data
>   * @buf: transmit or receive buffer
>   * @buf_lock:mutex to protect tx and rx
> + * @lock:protect sensor state
>   **/
>  struct adis16060_state {
>   struct spi_device   *us_w;
>   struct spi_device   *us_r;
>   struct mutexbuf_lock;
> + struct mutexlock;/* protect sensor state */

There should be no need to have two locks here. One should be enough. The
buf_lock protects both the adis16060_spi_write() and adis16060_spi_read()
functions. But both are always called in a pair. First write, then read. You
can refactor the code to have one single function
adis16060_spi_write_than_read() which is protected by the existing buf_lock.


>  
>   u8 buf[3] cacheline_aligned;
>  };
> @@ -87,7 +89,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>   switch (mask) {
>   case IIO_CHAN_INFO_RAW:
>   /* Take the iio_dev status lock */
> - mutex_lock(_dev->mlock);
> + mutex_lock(>lock);
>   ret = adis16060_spi_write(indio_dev, chan->address);
>   if (ret < 0)
>   goto out_unlock;
> @@ -96,7 +98,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>   if (ret < 0)
>   goto out_unlock;
>  
> - mutex_unlock(_dev->mlock);
> + mutex_unlock(>lock);
>   *val = tval;
>   return IIO_VAL_INT;
>   case IIO_CHAN_INFO_OFFSET:
> @@ -112,7 +114,7 @@ static int adis16060_read_raw(struct iio_dev *indio_dev,
>   return -EINVAL;
>  
>  out_unlock:
> - mutex_unlock(_dev->mlock);
> + mutex_unlock(>lock);
>   return ret;
>  }
>  
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ade7753: replace mlock with driver private lock

[Lars-Peter Clausen]

On 03/12/2017 02:32 PM, simran singhal wrote:
> The IIO subsystem is redefining iio_dev->mlock to be used by
> the IIO core only for protecting device operating mode changes.
> ie. Changes between INDIO_DIRECT_MODE, INDIO_BUFFER_* modes.
> 
> In this driver, mlock was being used to protect hardware state
> changes.  Replace it with a lock in the devices global data.
> 
> Fix some coding style issues related to white space also.
> 
> Signed-off-by: simran singhal 
> ---
>  drivers/staging/iio/meter/ade7753.c | 14 --
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c 
> b/drivers/staging/iio/meter/ade7753.c
> index dfd8b71..ca99d82 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -81,12 +81,14 @@
>   * @tx: transmit buffer
>   * @rx: receive buffer
>   * @buf_lock:   mutex to protect tx and rx
> + * @lock:protect sensor state

It might make sense to reuse the existing lock which currently protects the
read/write functions. You can do this by introducing a variant of
ade7753_spi_{read,write}_reg_16() that does not take a lock and use these to
implement the read-modify-write cycle in a protected section.

Looking through the driver there seem to be other places as well that do
read-modify-write that should be protected by a lock, but currently are not.
This might be a good task.

>   **/
>  struct ade7753_state {
> - struct spi_device   *us;
> - struct mutexbuf_lock;
> - u8  tx[ADE7753_MAX_TX] 
> cacheline_aligned;
> - u8  rx[ADE7753_MAX_RX];
> + struct spi_device   *us;
> + struct mutexbuf_lock;
> + struct mutexlock;   /* protect sensor state */
> + u8  tx[ADE7753_MAX_TX] cacheline_aligned;
> + u8  rx[ADE7753_MAX_RX];
>  };
>  
>  static int ade7753_spi_write_reg_8(struct device *dev,
> @@ -484,7 +486,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>   if (!val)
>   return -EINVAL;
>  
> - mutex_lock(_dev->mlock);
> + mutex_lock(>lock);
>  
>   t = 27900 / val;
>   if (t > 0)
> @@ -505,7 +507,7 @@ static ssize_t ade7753_write_frequency(struct device *dev,
>   ret = ade7753_spi_write_reg_16(dev, ADE7753_MODE, reg);
>  
>  out:
> - mutex_unlock(_dev->mlock);
> + mutex_unlock(>lock);
>  
>   return ret ? ret : len;
>  }
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: accel: Move header file content to source file

[Lars-Peter Clausen]

On 02/28/2017 08:32 PM, SIMRAN SINGHAL wrote:
> On Wed, Mar 1, 2017 at 12:53 AM, Jonathan Cameron  wrote:
>> On 28/02/17 19:17, Jonathan Cameron wrote:
>>> On 28/02/17 18:51, simran singhal wrote:
 The contents of the header file are used only by this single
 source file. Move content into .c and remove .h.

 Signed-off-by: simran singhal 
>>> Applied to the togreg branch of iio.git and pushed out as testing
>>> for the autobuilders to play with it.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>
>> One quick note:
>>
>> The title should include the name of the driver being modified
>> as this is probably the bit interested people are most likely
>> to pick up on in a long list of patches.
>>
>> I fixed this up.  Thanks,
>>
>> Jonathan
> 
> I have included the name of driver being modified.
> Due to want me to include the name of file i.e adis16201_core.c

Not the full filename, but the partnumber. E.g.

'staging: iio: adis16201: ...'

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] iio: trigger: free trigger resource correctly

[Lars-Peter Clausen]

On 01/22/2017 03:25 PM, Jonathan Cameron wrote:
> On 20/01/17 03:47, Alison Schofield wrote:
>> These stand-alone trigger drivers were using iio_trigger_put()
>> where they should have been using iio_trigger_free().  The
>> iio_trigger_put() adds a module_put which is bad since they
>> never did a module_get.
>>
>> In the sysfs driver, module_get/put's are used as triggers are
>> added & removed. This extra module_put() occurs on an error path
>> in the probe routine (probably rare).
>>
>> In the bfin-timer & interrupt trigger drivers, the module resources
>> are not explicitly managed, so it's doing a put on something that
>> was never get'd.  It occurs on the probe error path and on the
>> remove path (not so rare).
>>
>> Tested with the sysfs trigger driver.
>> The bfin & interrupt drivers were build tested & inspected only.
>>
>> Signed-off-by: Alison Schofield 
> This is certainly more consistent. 
> Lars, could you sanity check as well given the bfin timer is
> in here.

looks good.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4] staging: iio: ad7606: replace range/range_available with corresponding scale

[Lars-Peter Clausen]

On 01/07/2017 12:10 PM, Eva Rachel Retuya wrote:
> Eliminate the non-standard attributes in_voltage_range and
> in_voltage_range_available. Implement in_voltage_scale_available in place
> of these attributes and update the SCALE accordingly. The array
> scale_avail is introduced to hold the available scale values.
> 
> Signed-off-by: Eva Rachel Retuya <eraret...@gmail.com>

Looks good, thanks.

Acked-by: Lars-Peter Clausen <l...@metafoo.de>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/2] staging: iio: ad7606: move out of staging

[Lars-Peter Clausen]

On 12/30/2016 09:16 PM, Jonathan Cameron wrote:
> On 11/12/16 02:47, Eva Rachel Retuya wrote:
>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>> corresponding Makefile and Kconfig associated with the change.
>>
>> Signed-off-by: Eva Rachel Retuya 
> Personally (and this is much debated ;) I prefer for the odd case
> of staging graduations, that git isn't run with the -M parameter so we
> have the whole driver to comment on.
> 
> Anyhow, just casting my eyes over the code so here are some notes:
> 1. The whole computing scale available values on the fly seems rather over the
> top as they can be easily precomputed and stored in a static const array.
> There are only 2 of them!
> 
> 2. There are some single line comments still using multiline syntax (now
> I'm really nitpicking ;)
> 
> 3. A slight oddity has crept in with the cleanup of attributes.  We now
> prevent the appearance of the _scale_available / oversampling_ratio_available
> attributes if the gpios are attached, but not _scale or _oversampling.
> (oops).  If we are going to do this dance, then we should also have an
> alternative set of iio_chan_spec array to reflect this.
> 
> 4. I'd drop the 'More devices added in future' comment. It's now the future
> and they haven't been yet ;)
> 

I actually have a patch adding more devices, that just waits for the driver
to be moved out of staging. But still the comment can be removed it is not
really meaningful.

> 5. We can write oversampling ratio, but queries to the write_get_fmt will
> return an error for that. Probably want to fix that unless I'm missing 
> something.
> 
> 6. There are some ancient references to 'ring' buffers in the comments and
> function naming.  We switched over from my hand rolled ring buffer to the
> kfifo buffer we now use ages ago.  Would be nice to clear those out.
> 
> None of these are really blockers to it moving out of staging
> (except the bugs we introduced in the cleanup), but might be nice
> to do one last series pinning those down then get a final review done to
> see if we missed anything else.
> 
> Been a long time since I looked at this one in any depth and it's certainly
> heading in the right direction!
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

[Lars-Peter Clausen]

On 12/30/2016 08:59 PM, Jonathan Cameron wrote:
> On 11/12/16 02:47, Eva Rachel Retuya wrote:
>> Eliminate the non-standard attributes in_voltage_range and
>> in_voltage_range_available. Implement in_voltage_scale_available in place
>> of these attributes and update the SCALE accordingly. The array
>> scale_avail is introduced to hold the available scale values.
>>
>> Signed-off-by: Eva Rachel Retuya <eraret...@gmail.com>
> Looks good to me.
> 
> Lars, if you have a minute to look over this as well that would
> be great.

I think this is OK as is, since it is an improvement over the current
situation. But there is still the open issue that we don't know the scale
setting if pin is hardwired. I'd like to see that addressed before the
driver is moved out of staging. Similar for the oversampling ratio.

Consider this Acked-by: Lars-Peter Clausen <l...@metafoo.de>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/6] staging: iio: set proper supply name to devm_regulator_get()

[Lars-Peter Clausen]

On 11/01/2016 05:03 AM, Matt Ranostay wrote:
> On Mon, Oct 31, 2016 at 10:04 AM, Eva Rachel Retuya  
> wrote:
>> The name passed to devm_regulator_get() should match the name of the
>> supply as specified in the device datasheet. This makes it clear what
>> power supply is being referred to in case of presence of other
>> regulators.
>>
>> Currently, the supply name specified on the affected devices is 'vcc'.
>> Use lowercase version of the datasheet name to specify the supply
>> voltage.
>>
> 
> Aren't we possibly breaking current device tree definitions that
> people may have? We should still check the old name after the new
> datasheet name in my opinion.

None of those drivers have DT bindings, so I think we are OK. And they are
in staging anyway.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3] staging: iio: cdc: ad7746: add additional config defines

[Lars-Peter Clausen]

On 10/30/2016 06:41 PM, Jonathan Cameron wrote:
> On 28/10/16 09:26, Eva Rachel Retuya wrote:
>> Introduce defines for shifting and mask under the config register for
>> better readability. Also, introduce helper variables for index
>> calculation.
>>
>> Signed-off-by: Eva Rachel Retuya <eraret...@gmail.com>
> Looks good to me.
> 
> Lars could you sanity check this one as well?

Acked-by: Lars-Peter Clausen <l...@metafoo.de>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: cdc: ad7746: set ret on IIO_VOLTAGE case

[Lars-Peter Clausen]

On 10/25/2016 08:44 PM, Colin King wrote:
> From: Colin Ian King 
> 
> For a IIO_VOLTAGE case, ret is not being set causing an
> uninitialized value being returned by ad7746_read_raw. Fix
> this by setting ret to IIO_VAL_INT for this specific case.
> 
> Signed-off-by: Colin Ian King 

Arnd did beat you by a few hours, https://lkml.org/lkml/2016/10/25/485
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] [v2] staging: iio: ad5933: avoid uninitialized variable in error case

[Lars-Peter Clausen]

On 10/25/2016 06:57 PM, Jonathan Cameron wrote:
> On 24/10/16 16:22, Arnd Bergmann wrote:
>> The ad5933_i2c_read function returns an error code to indicate
>> whether it could read data or not. However ad5933_work() ignores
>> this return code and just accesses the data unconditionally,
>> which gets detected by gcc as a possible bug:
>>
>> drivers/staging/iio/impedance-analyzer/ad5933.c: In function 'ad5933_work':
>> drivers/staging/iio/impedance-analyzer/ad5933.c:649:16: warning: 'status' 
>> may be used uninitialized in this function [-Wmaybe-uninitialized]
>>
>> This adds minimal error handling so we only evaluate the
>> data if it was correctly read.
>>
>> Link: https://patchwork.kernel.org/patch/8110281/
>> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> Looks good to me.
> 
> Lars?
> 

Looks good, thanks.

Acked-by: Lars-Peter Clausen <l...@metafoo.de>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] stagging:iio:ad9834: add devicetree property support

[Lars-Peter Clausen]

On 09/18/2016 10:52 AM, Gwenhael Goavec-Merou wrote:
[...]
> +#if defined(CONFIG_OF)
> +static struct ad9834_platform_data *ad9834_parse_dt(struct spi_device *spi)
> +{
> + struct ad9834_platform_data *pdata;
> + struct device_node *np = spi->dev.of_node;
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + pdata->freq0 = 134000;
> + of_property_read_u32(np, "freq0", >freq0);
> +
> + pdata->freq1 = 134000;
> + of_property_read_u32(np, "freq1", >freq1);
> +
> + pdata->phase0 = 0;
> + of_property_read_u16(np, "phase0", >phase0);
> +
> + pdata->phase1 = 0;
> + of_property_read_u16(np, "phase1", >phase1);
> +
> + pdata->en_div2 = of_property_read_bool(np, "en_div2");
> + pdata->en_signbit_msb_out = of_property_read_bool(np,
> + "en_signbit_msb_out");

Sorry, I should have been more clear what I meant in the previous mail.

The devicetree describes the hardware, which components are present and how
they are connected to each other and sometimes system level operating
constraints.

The parameters above in my opinion are application specific configuration
parameters though. At least phase and frequency. I'm not quite sure what
exactly decides how the SIGN_OUT pin should be used, they might be hardware
setup dependent.

But I'm not that familiar with the typical usecase of these types of
devices, so if you disagree please say so. Maybe you can explain your setup
a bit and what you need from the chip and the driver.

> +
> + return pdata;
> +}
> +#else
> +static struct ad9834_platform_data *ad9834_parse_dt(struct spi_device *spi)
> +{
> + return NULL;
> +}
> +#endif
> +
>  static int ad9834_probe(struct spi_device *spi)
>  {
>   struct ad9834_platform_data *pdata = dev_get_platdata(>dev);
>   struct ad9834_state *st;
>   struct iio_dev *indio_dev;
>   struct regulator *reg;
> + struct clk *clk = NULL;
>   int ret;
>  
> + if (!pdata && spi->dev.of_node) {
> + pdata = ad9834_parse_dt(spi);
> + if (IS_ERR(pdata))
> + return PTR_ERR(pdata);
> + }
> +
>   if (!pdata) {
>   dev_dbg(>dev, "no platform data?\n");
>   return -ENODEV;
>   }
>  
> + if (!pdata->mclk) {
> + clk = devm_clk_get(>dev, NULL);

I'd make the clock name explicit clk_get(..., "mclk");

> + if (IS_ERR(clk))

Should be 'return PTR_ERR(clk);'. clk_get() will return EPROBE_DEFER if the
clock has been specified but is not yet available, but it will return an
error, if e.g. there is an error in the specification. We should propagate
this error.

> + return -EPROBE_DEFER;
> +
> + ret = clk_prepare_enable(clk);
> + if (ret < 0)
> + return ret;
> + }
> +
[...]
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] stagging:iio:ad9834: add devicetree property support

[Lars-Peter Clausen]

Hi,

Thanks for the patch.

On 09/11/2016 12:52 PM, Gwenhael Goavec-Merou wrote:
> +static struct ad9834_platform_data *ad9834_parse_dt(struct spi_device *spi)
> +{
> + struct ad9834_platform_data *pdata;
> + struct device_node *np = spi->dev.of_node;
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return ERR_PTR(-ENOMEM);
> +
> + if (of_property_read_u32(np, "mclk", >mclk))
> + return ERR_PTR(-ENODEV);

The input clock should be using the standard clock bindings.

> + if (of_property_read_u32(np, "freq0", >freq0))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u32(np, "freq1", >freq1))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u16(np, "phase0", >phase0))
> + return ERR_PTR(-ENODEV);
> + if (of_property_read_u16(np, "phase1", >phase1))
> + return ERR_PTR(-ENODEV);
> + pdata->en_div2 = of_property_read_bool(np, "en_div2");
> + pdata->en_signbit_msb_out = of_property_read_bool(np,
> + "en_signbit_msb_out");

The other attributes seem to be more runtime configuration data, rather than
hardware description. Maybe just choose a fixed default.

- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: ad5933: fix order of cycle conditions

[Lars-Peter Clausen]

On 06/11/2016 06:50 PM, Jonathan Cameron wrote:
> On 01/06/16 20:25, Luis de Bethencourt wrote:
>> Correctly handle the settling time cycles value. The else branch is an
>> impossible condition, > 1022 in the else branch of > 511. Flipping the order.
>>
>> Based on the Table 13 at the bottom of Page 25 of the Data Sheet:
>> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5933.pdf
>>
>> Signed-off-by: Luis de Bethencourt <lui...@osg.samsung.com>
>> Reviewed-by: Lars-Peter Clausen <l...@metafoo.de>
> While Lars did review, it I'm not seeing an email where he
> gave this tag (my inbox may well have eaten it).
> 
> Lars, could you confirm that you are happy with the
> patch as it now stands.
> 

All good, thanks.

> Certainly looks right to me!
> 
> Thanks, 
> Jonathan
>> ---
>>  drivers/staging/iio/impedance-analyzer/ad5933.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c 
>> b/drivers/staging/iio/impedance-analyzer/ad5933.c
>> index 9f43976..170ac98 100644
>> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
>> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
>> @@ -444,10 +444,10 @@ static ssize_t ad5933_store(struct device *dev,
>>  st->settling_cycles = val;
>>  
>>  /* 2x, 4x handling, see datasheet */
>> -if (val > 511)
>> -val = (val >> 1) | (1 << 9);
>> -else if (val > 1022)
>> +if (val > 1022)
>>  val = (val >> 2) | (3 << 9);
>> +else if (val > 511)
>> +val = (val >> 1) | (1 << 9);
>>  
>>  dat = cpu_to_be16(val);
>>  ret = ad5933_i2c_write(st->client,
>>
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad5933: fix handling of settling time cycles

[Lars-Peter Clausen]

On 06/01/2016 05:55 PM, Luis de Bethencourt wrote:
> Correctly handle the settling time cycles value. The else branch was an
> impossible condition (> 1022 in the else branch of > 511) and the handling
> of the values was dividing by 2 and 4, with a left shift, instead of
> multiplying.
> 
> Based on the Table 13 at the bottom of Page 25 of the Data Sheet:
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD5933.pdf
> 
> Signed-off-by: Luis de Bethencourt 
> ---
> 
> Hi,
> 
> I decided to use the hexadecimal values instead of (1 << 10) and (1 << 9), for
> briefness, I could resend using those instead if it is prefered.
> 
> I also decided to use multiplications instead of right-shifts for readability.
> I could use change that as well.
> 
> Thanks,
> Luis
> 
>  drivers/staging/iio/impedance-analyzer/ad5933.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c 
> b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 9f43976..3a2cf8f3 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -444,10 +444,13 @@ static ssize_t ad5933_store(struct device *dev,
>   st->settling_cycles = val;
>  
>   /* 2x, 4x handling, see datasheet */
> - if (val > 511)
> - val = (val >> 1) | (1 << 9);
> - else if (val > 1022)
> - val = (val >> 2) | (3 << 9);
> + if (val & 0x400 && val & 0x200) {
> + val &= 0x1ff;
> + val *= 4;
> + } else if (val & 0x200) {
> + val &= 0x1ff;
> + val *= 2;
> + }

This does not look correct. D10 and D9 select an additional multiplier of
either 1, 2 or 4. So dividing the value before writing it to the register is
the right approach in that case. Just flipping the order in which the
conditions are evaluated should be sufficient.

>  
>   dat = cpu_to_be16(val);
>   ret = ad5933_i2c_write(st->client,
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()

[Lars-Peter Clausen]

On 04/06/2016 06:11 AM, Alison Schofield wrote:
> Replace the code that guarantees the device stays in direct mode with
> iio_device_{claim|release}_direct_mode() which does same.
> 
> Signed-off-by: Alison Schofield <amsfiel...@gmail.com>

Looks good, thanks.

Acked-by: Lars-Peter Clausen <l...@metafoo.de>

> ---
> Changed in v2:
>  - removed improper application of claim/release from intr handler
>  - updated changelog
> 
>  drivers/staging/iio/adc/ad7606_core.c | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7606_core.c 
> b/drivers/staging/iio/adc/ad7606_core.c
> index 6dbc811..f79ee61 100644
> --- a/drivers/staging/iio/adc/ad7606_core.c
> +++ b/drivers/staging/iio/adc/ad7606_core.c
> @@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  
>   switch (m) {
>   case IIO_CHAN_INFO_RAW:
> - mutex_lock(_dev->mlock);
> - if (iio_buffer_enabled(indio_dev))
> - ret = -EBUSY;
> - else
> - ret = ad7606_scan_direct(indio_dev, chan->address);
> - mutex_unlock(_dev->mlock);
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = ad7606_scan_direct(indio_dev, chan->address);
> + iio_device_release_direct_mode(indio_dev);
>  
>   if (ret < 0)
>   return ret;
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7606: use iio_device_{claim|release}_direct_mode()

[Lars-Peter Clausen]

On 04/03/2016 11:09 AM, Jonathan Cameron wrote:
> On 01/04/16 17:53, Alison Schofield wrote:
>> Two instances are moved to the new claim/release API:
>>
>> In the first instance, the driver was using mlock followed by
>> iio_buffer_enabled(). Replace that code with the new API to guarantee
>> the device stays in direct mode. There is no change in driver behavior.
>>
>> In the second instance, the driver was not using mlock to hold the
>> device in direct mode, but should have been.  Here we introduce the
>> new API to guarantee direct mode. This is a change in driver behavior.
>>
>> Signed-off-by: Alison Schofield 
>> ---
>>  drivers/staging/iio/adc/ad7606_core.c | 15 ---
>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7606_core.c
b/drivers/staging/iio/adc/ad7606_core.c
>> index 6dbc811..f914b8d 100644
>> --- a/drivers/staging/iio/adc/ad7606_core.c
>> +++ b/drivers/staging/iio/adc/ad7606_core.c
>> @@ -88,12 +88,12 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>>
>>  switch (m) {
>>  case IIO_CHAN_INFO_RAW:
>> -mutex_lock(_dev->mlock);
>> -if (iio_buffer_enabled(indio_dev))
>> -ret = -EBUSY;
>> -else
>> -ret = ad7606_scan_direct(indio_dev, chan->address);
>> -mutex_unlock(_dev->mlock);
>> +ret = iio_device_claim_direct_mode(indio_dev);
>> +if (ret)
>> +return ret;
>> +
>> +ret = ad7606_scan_direct(indio_dev, chan->address);
>> +iio_device_release_direct_mode(indio_dev);
>>
>>  if (ret < 0)
>>  return ret;
>> @@ -411,8 +411,9 @@ static irqreturn_t ad7606_interrupt(int irq, void
*dev_id)
>>  struct iio_dev *indio_dev = dev_id;
>>  struct ad7606_state *st = iio_priv(indio_dev);
>>
>> -if (iio_buffer_enabled(indio_dev)) {
>> +if (!iio_device_claim_direct_mode(indio_dev))  {
>>  schedule_work(>poll_work);
>> +iio_device_release_direct_mode(indio_dev);
> Unfortunately this won't work.  That interrupt is still in traditional non
> threaded form.  This will take a mutex in a top half interrupt handler
> where a sleep cannot occur.
>
> I'm just wondering how expensive it would be to fix this by moving that over
> to a threaded handler.  In the poll_work case (buffer) it would be cleaner
to do
> so. I'm really confused what the intended interrupt handler
> is in here.  I 'think' the sequence is:
>
> Trigger fires the convst pin whether in top half or the bottom half of
> a threaded interrupt, but not both - I guess this works, if it is rather
> 'unusual'.
>
> We then get a interrupt to indicate that it has finished conversion and that
> filters through to actually fill the buffer via a traditional top half /
> bottom half interrupt handler.
>
> So if we were to convert that to a threaded interrupt (with no top half / non
> threaded part), we could drop the schedule_work and just call
> ad7606_poll_bh_to_ring from the thread handler.
>
> In the direct read case I doubt we care about the delay in dropping to a
> thread prior to signalling the data is ready.
>
> Can't think why this driver is still in staging :)

Yeah, we should leave this driver out from the conversion for now. The whole
convst pin handling need a major rework. It shouldn't really be in the
driver and usually you wouldn't want to use to use a GPIO and software timer
since that gives you way to much jitter for good results. You'd probably use
something like a PWM.

>
> Lars, any interest from Analog in getting this one cleaned up?  Also
> do you have any test hardware, if we mess around with this interrupt handling?

I have the hardware somewhere in some storage bay, but just converting this
over to threaded interrupt handling is not really a solution. So, if you
want to get rid of the iio_buffer_enabled() in the interrupt handler a
simple solution is to register preenable and postdisable callbacks where you
set a flag in the driver struct to indicate that it is in buffered mode or not.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v5 21/50] mtd: nand: jz4740: switch to mtd_ooblayout_ops

[Lars-Peter Clausen]

On 03/30/2016 06:14 PM, Boris Brezillon wrote:
> Implementing the mtd_ooblayout_ops interface is the new way of exposing
> ECC/OOB layout to MTD users.
>
> Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com>
Acked-by: Lars-Peter Clausen <l...@metafoo.de>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 1/2] iio: core: implement iio_{claim|release}_direct_mode()

[Lars-Peter Clausen]

On 03/01/2016 08:02 PM, Alison Schofield wrote:
> It is often the case that the driver wants to be sure a device stays
> in direct mode while it is executing a task or series of tasks.  To
> accomplish this today, the driver performs this sequence: 1) take the
> device state lock, 2)verify it is not in a buffered mode, 3) execute
> some tasks, and 4) release that lock.
> 
> This patch introduces a pair of helper functions that simplify these
> steps and make it more semantically expressive.
> 
> iio_claim_direct_mode()
> If the device is not in any buffered mode it is guaranteed
> to stay that way until iio_release_direct_mode() is called.
> 
> iio_release_direct_mode()
> Release the claim. Device is no longer guaranteed to stay
> in direct mode.
> 
> Signed-off-by: Alison Schofield 

Looks basically good.

> ---
>  drivers/iio/industrialio-core.c | 39 +++
>  include/linux/iio/iio.h |  2 ++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index 70cb7eb..f6f0c89 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -25,6 +25,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include "iio_core.h"
>  #include "iio_core_trigger.h"
> @@ -1375,6 +1376,44 @@ void devm_iio_device_unregister(struct device *dev, 
> struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL_GPL(devm_iio_device_unregister);
>  
> +/**
> + * iio_claim_direct_mode - Keep device in direct mode
> + * @indio_dev:   the iio_dev associated with the device
> + *
> + * If the device is in direct mode it is guaranteed to
> + * stay that way until iio_release_direct_mode() is called.
> + *
> + * Use with iio_release_direct_mode()
> + *
> + * Returns: 0 on success, -EINVAL on failure
> + */
> +int iio_claim_direct_mode(struct iio_dev *indio_dev)

To be consistent with the reset of the API I'd use the iio_device_... prefix
here, same for the release function.

> +{
> + mutex_lock(_dev->mlock);
> +
> + if (iio_buffer_enabled(indio_dev)) {
> + mutex_unlock(_dev->mlock);
> + return -EINVAL;

-EINVAL doesn't make much sense here, -EBUSY is better.

> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(iio_claim_direct_mode);
> 
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] Staging:iio:Prefer using BIT macro

[Lars-Peter Clausen]

On 02/08/2016 07:48 AM, Bhumika Goyal wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> This was done with coccinelle:
>  @@ int g; @@
> -(1 << g)
> +BIT(g)
> 
> Signed-off-by: Bhumika Goyal 

Hi,

Thanks for the patch.

> ---
>  drivers/staging/iio/adc/ad7816.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7816.c 
> b/drivers/staging/iio/adc/ad7816.c
> index 2226051..f631012 100644
> --- a/drivers/staging/iio/adc/ad7816.c
> +++ b/drivers/staging/iio/adc/ad7816.c
> @@ -222,7 +222,7 @@ static ssize_t ad7816_show_value(struct device *dev,
>   value = (s8)((data >> AD7816_TEMP_FLOAT_OFFSET) - 103);
>   data &= AD7816_TEMP_FLOAT_MASK;
>   if (value < 0)
> - data = (1 << AD7816_TEMP_FLOAT_OFFSET) - data;
> + data = BIT(AD7816_TEMP_FLOAT_OFFSET) - data;


But in this case this is a false positive. The intended semantic meaning
here is 2**... not BIT(...). Using BIT() here in my opinion only causes
confusion.

>   return sprintf(buf, "%d.%.2d\n", value, data * 25);
>   }
>   return sprintf(buf, "%u\n", data);
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] Staging: iio: adc: Prefer using the BIT macro

[Lars-Peter Clausen]

On 02/08/2016 07:48 AM, Bhumika Goyal wrote:
> This patch replaces bit shifting on 1 with the BIT(x) macro.
> 
> This was done with coccinelle:
> 
> @@ int g; @@
> 
> -(1 << g)
> +BIT(g)
> 
> Signed-off-by: Bhumika Goyal 

Hi,

Thanks for the patch. This looks good.

> ---
>  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 0c73bce..ccf3157 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -117,7 +117,7 @@
>   */
>  #define POLYNOM  0x2F
>  #define POLYNOM_ORDER8
> -#define HIGHBIT  (1 << (POLYNOM_ORDER - 1))
> +#define HIGHBIT  BIT((POLYNOM_ORDER - 1))

But please drop the extra brackets and resend the patch.

>  
>  struct ad7280_state {
>   struct spi_device   *spi;
> @@ -388,7 +388,7 @@ static ssize_t ad7280_show_balance_sw(struct device *dev,
>  
>   return sprintf(buf, "%d\n",
>  !!(st->cb_mask[this_attr->address >> 8] &
> -(1 << ((this_attr->address & 0xFF) + 2;
> +BIT(((this_attr->address & 0xFF) + 2;

Same here.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [RFC PATCH 1/2] iio: hmc5843 Add channel attribute for bias configuration

[Lars-Peter Clausen]

On 02/04/2016 03:50 PM, Cristina Moraru wrote:
> Replace non standard meas_conf attribute with the standard IIO
> calibbias attribute.
> 
> API for setting bias measurement configuration:
> 
> 0 - Normal measurement configuration (default): In normal measurement
> configuration the device follows normal measurement flow. Pins BP
> and BN are left floating and high impedance.
> 
> 1 - Positive bias configuration: In positive bias configuration, a
> positive current is forced across the resistive load on pins BP
> and BN.
> 
> 2 - Negative bias configuration. In negative bias configuration, a
> negative current is forced across the resistive load on pins BP
> and BN.
> 
> 3 - Only available on HMC5983. Magnetic sensor is disabled.
> Temperature sensor is enabled.
> 
> With this in place, we can think of moving this driver out of staging.

Using a standard attribute, but overloading it with custom semantics doesn't
do any good either. calibbias is supposed to be a integer that gets added to
measurements internally in the device (unit is device specific though).

This attribute seems to do something else. In that case it might be better
to stay with a custom attribute (as long as it is documented) or come up
with a better way to map the device configuration onto standard attributes.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH resend] staging: rts5208: Remove unnecessary synchronize_irq() before free_irq()

[Lars-Peter Clausen]

Calling synchronize_irq() right before free_irq() is quite useless. On one
hand the IRQ can easily fire again before free_irq() is entered, on the
other hand free_irq() itself calls synchronize_irq() internally (in a race
condition free way), before any state associated with the IRQ is freed.

Patch was generated using the following semantic patch:
// 
@@
expression irq;
@@
-synchronize_irq(irq);
 free_irq(irq, ...);
// 

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/staging/rts5208/rtsx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 1fe8e3e..5dfcdfb 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -320,7 +320,6 @@ static int rtsx_suspend(struct pci_dev *pci, pm_message_t 
state)
rtsx_do_before_power_down(chip, PM_S3);
 
if (dev->irq >= 0) {
-   synchronize_irq(dev->irq);
free_irq(dev->irq, (void *)dev);
dev->irq = -1;
}
@@ -398,7 +397,6 @@ static void rtsx_shutdown(struct pci_dev *pci)
rtsx_do_before_power_down(chip, PM_S1);
 
if (dev->irq >= 0) {
-   synchronize_irq(dev->irq);
free_irq(dev->irq, (void *)dev);
dev->irq = -1;
}
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad5933: avoid uninitialized variable in error case

[Lars-Peter Clausen]

On 01/30/2016 03:18 PM, Jonathan Cameron wrote:
> On 25/01/16 15:50, Arnd Bergmann wrote:
>> The ad5933_i2c_read function returns an error code to indicate
>> whether it could read data or not. However ad5933_work() ignores
>> this return code and just accesses the data unconditionally,
>> which gets detected by gcc as a possible bug:
>>
>> drivers/staging/iio/impedance-analyzer/ad5933.c: In function 'ad5933_work':
>> drivers/staging/iio/impedance-analyzer/ad5933.c:649:16: warning: 'status' 
>> may be used uninitialized in this function [-Wmaybe-uninitialized]
>>
>> This adds minimal error handling so we only evaluate the
>> data if it was correctly read.
>>
>> Signed-off-by: Arnd Bergmann 
> Hi Arnd,
> 
> Thanks for the patch.   The handling in here is a little fiddly
> by the look of things. Lars can you take a look at this when
> you have a minute?
> 
> At a very high level, it doesn't make sense to fix this instance and
> not the one in the context of the patch below.
> See below...
>> ---
>>  drivers/staging/iio/impedance-analyzer/ad5933.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c 
>> b/drivers/staging/iio/impedance-analyzer/ad5933.c
>> index 10c43dda0f5a..304bb464e478 100644
>> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
>> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
>> @@ -647,6 +647,7 @@ static void ad5933_work(struct work_struct *work)
>>  __be16 buf[2];
>>  int val[2];
>>  unsigned char status;
>> +int ret;
>>  
>>  mutex_lock(_dev->mlock);
>>  if (st->state == AD5933_CTRL_INIT_START_FREQ) {
>> @@ -658,9 +659,9 @@ static void ad5933_work(struct work_struct *work)
>>  return;
>>  }
>>  
>> -ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, );
>> +ret = ad5933_i2c_read(st->client, AD5933_REG_STATUS, 1, );
>>  
>> -if (status & AD5933_STAT_DATA_VALID) {
>> +if (!ret && (status & AD5933_STAT_DATA_VALID)) {
> The else is non trivial here as it assumes we will get the data later. If we
> get such a failure, we probably want to drop out completely rather than paper
> over the gaps..

I agree. Although we could argue that Arnd's approach allows to recover from
temporary failure. But then again we don't want to keep polling forever if
it's a permanent failure. I'd say the best thing for a quick fix is to just
error out and assume the error is permanent.

>>  int scan_count = bitmap_weight(indio_dev->active_scan_mask,
>> indio_dev->masklength);
> Same issue on the next line - this results in known garbage data being spooled
> out.

Also agreed.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/30/2016 04:12 PM, Dan Carpenter wrote:
> We could make checkpatch.pl not complain if the line says checkpatch: on
> it.  It would look like this.
> 
> -   in_voltage-voltage_thresh_low_value,
> +   in_voltage-voltage_thresh_low_value, /* checkpatch: not math 
> */
> 
> I suppose I could have made the explanation longer since the it won't
> complain about the 80 character limit...  What do you guys think?

We could add it as a temporary way to silence the checker. But it feels a
bit ugly, there is really no reason why this shouldn't be a string, other
than that the current device attr macros don't support that.

- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/24/2016 05:36 PM, Jonathan Cameron wrote:
> On 20/01/16 14:21, Dan Carpenter wrote:
>> On Fri, Jan 15, 2016 at 09:15:52PM +0100, Lars-Peter Clausen wrote:
>>> On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
>>>> This patch adds apace around '-' operator.Found using checkpatch.pl
>>>>
>>>> Signed-off-by: Bhumika Goyal <bhumi...@gmail.com>
>>>> ---
>>>>  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 f45ebed..0c73bce 100644
>>>> --- a/drivers/staging/iio/adc/ad7280a.c
>>>> +++ b/drivers/staging/iio/adc/ad7280a.c
>>>> @@ -744,14 +744,14 @@ out:
>>>>  }
>>>>  
>>>>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
>>>> -  in_voltage-voltage_thresh_low_value,
>>>> +  in_voltage - voltage_thresh_low_value,
>>>
>>> Hi,
>>>
>>> Thanks for patch. But when sending cleanup patches like this please make
>>> sure that you a) understand what the code does and how your change affects
>>> it and b) as a bare minimum of testing perform a compile test, if possible
>>> also do functional testing.
>>>
>>> The patch as it is, is neither semantically nor syntactically correct. As an
>>> exercise please make sure you understand why.
>>
>> Ugh!
>>
>> It took me a long time to figure out the bug in this patch...  Why does
>> that filename have a mix of dashes and underscores?  Too late to fix it
>> now...  :/
>>
> Very deliberately.  The - is indicating it is a differential channel!
> Literally A minus B.
> 
> It's an awfully compact representation for maths ;)
> This is obscured partly in this case as it's specifying an attribute
> shared by a set of differential channels so it's the generalization
> of
> in_voltage0-voltage1_thresh_low_value
> which does begin to slightly stretch the argument that it is nice and
> clear ;(

One thing we should maybe take a look at is making it explicit that this is
a string so it does not get picked up by checkpatch.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] staging:iio:adc:added space around '-'

[Lars-Peter Clausen]

On 01/15/2016 08:42 PM, Bhumika Goyal wrote:
> This patch adds apace around '-' operator.Found using checkpatch.pl
> 
> Signed-off-by: Bhumika Goyal 
> ---
>  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 f45ebed..0c73bce 100644
> --- a/drivers/staging/iio/adc/ad7280a.c
> +++ b/drivers/staging/iio/adc/ad7280a.c
> @@ -744,14 +744,14 @@ out:
>  }
>  
>  static IIO_DEVICE_ATTR_NAMED(in_thresh_low_value,
> - in_voltage-voltage_thresh_low_value,
> + in_voltage - voltage_thresh_low_value,

Hi,

Thanks for patch. But when sending cleanup patches like this please make
sure that you a) understand what the code does and how your change affects
it and b) as a bare minimum of testing perform a compile test, if possible
also do functional testing.

The patch as it is, is neither semantically nor syntactically correct. As an
exercise please make sure you understand why.

Same for the second patch.

- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: rts5208: Remove unnecessary synchronize_irq() before free_irq()

[Lars-Peter Clausen]

Calling synchronize_irq() right before free_irq() is quite useless. On one
hand the IRQ can easily fire again before free_irq() is entered, on the
other hand free_irq() itself calls synchronize_irq() internally (in a race
condition free way), before any state associated with the IRQ is freed.

Patch was generated using the following semantic patch:
// 
@@
expression irq;
@@
-synchronize_irq(irq);
 free_irq(irq, ...);
// 

Signed-off-by: Lars-Peter Clausen <l...@metafoo.de>
---
 drivers/staging/rts5208/rtsx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c b/drivers/staging/rts5208/rtsx.c
index 1fe8e3e..5dfcdfb 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -320,7 +320,6 @@ static int rtsx_suspend(struct pci_dev *pci, pm_message_t 
state)
rtsx_do_before_power_down(chip, PM_S3);
 
if (dev->irq >= 0) {
-   synchronize_irq(dev->irq);
free_irq(dev->irq, (void *)dev);
dev->irq = -1;
}
@@ -398,7 +397,6 @@ static void rtsx_shutdown(struct pci_dev *pci)
rtsx_do_before_power_down(chip, PM_S1);
 
if (dev->irq >= 0) {
-   synchronize_irq(dev->irq);
free_irq(dev->irq, (void *)dev);
dev->irq = -1;
}
-- 
2.1.4

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] Staging: iio: iio_simple_dummy_buffer: Typo in comments area

[Lars-Peter Clausen]

On 11/14/2015 03:44 AM, Nizam Haider wrote:
> Fix simple typo in comments
> 
> Signed-off-by: Nizam Haider 

Thanks for the patch.

> ---
>  drivers/staging/iio/iio_simple_dummy_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/iio_simple_dummy_buffer.c 
> b/drivers/staging/iio/iio_simple_dummy_buffer.c
> index cf44a6f..c8f889b 100644
> --- a/drivers/staging/iio/iio_simple_dummy_buffer.c
> +++ b/drivers/staging/iio/iio_simple_dummy_buffer.c
> @@ -64,7 +64,7 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void 
> *p)
>* software scans: can be considered to be random access
>*   so efficient reading is just a case of minimal bus
>*   transactions.
> -  * software culled hardware scans:
> +  * software called hardware scans:

I don't think that's a typo. The non-patched version makes a lot more sense
then the patched vesion.

>*   occasionally a driver may process the nearest hardware
>*   scan to avoid storing elements that are not desired. This
>*   is the fiddliest option by far.
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: adc: fix comment block coding style issue

[Lars-Peter Clausen]

On 10/03/2015 08:32 PM, Hugo Camboulive wrote:
> This patch to ad7746.c makes the comment block end with a */
> on a separate line.
> 
> Signed-off-by: Hugo Camboulive 
> ---
>  drivers/staging/iio/cdc/ad7746.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/cdc/ad7746.c 
> b/drivers/staging/iio/cdc/ad7746.c
> index 10fa372..8445ddd 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -531,7 +531,7 @@ static int ad7746_write_raw(struct iio_dev *indio_dev,
>   /* CAPDAC Scale = 21pF_typ / 127
>* CIN Scale = 8.192pF / 2^24
>* Offset Scale = CAPDAC Scale / CIN Scale = 338646
> -  * */
> +  */

Looks good, but while you are at it please also fix the beginning of the
comment. Kernel style multi-line comments look like

/*
 * text
 * more text
 */

So the line with the /* should have any text.

Bonus points if you can also fix all other occurrences of this style error
in the same file.

- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] Staging: iio: cdc: Remove unnecessary dev_info

[Lars-Peter Clausen]

On 09/25/2015 07:23 PM, Shraddha Barke wrote:
> Remove dev_info as the information can be obtained by other means
> 
> Signed-off-by: Shraddha Barke <shraddha.6...@gmail.com>

Acked-by: Lars-Peter Clausen <l...@metafoo.de>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] Staging: iio: cdc: Prefer using the BIT macro

[Lars-Peter Clausen]

On 09/25/2015 07:23 PM, Shraddha Barke wrote:
> Replace bit shifting on 1 with the BIT(x) macro
> 
> Signed-off-by: Shraddha Barke <shraddha.6...@gmail.com>

Acked-by: Lars-Peter Clausen <l...@metafoo.de>

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/19] staging: iio: Remove unnecessary externs

[Lars-Peter Clausen]

On 08/15/2015 09:57 PM, Jonathan Cameron wrote:
 On 11/08/15 19:43, Lars-Peter Clausen wrote:
 On 08/10/2015 11:51 PM, Joe Perches wrote:
 Using 'extern' is not necessary for function prototypes.

 Signed-off-by: Joe Perches j...@perches.com

 Acked-by: Lars-Peter Clausen l...@metafoo.de

 Applied to the togreg branch of iio.git.  4.4 material now
 probably.

Greg already picked the patch up earlier today on his own.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] staging: iio: hmc5843: Set iio name dynamically

[Lars-Peter Clausen]

On 08/12/2015 03:25 PM, sdliy...@gmail.com wrote:
 From: Yong Li sdliy...@gmail.com
 
 Load the driver using the below command:
 echo hmc5983 0x1e  /sys/bus/i2c/devices/i2c-?/new_device
 
 In sysfs, the iio name is hmc5843, however the i2c name is hmc5983,
 they are inconsistent.
 
 With this patch, the iio name will be the same as the i2c device name
 
 Signed-off-by: Yong Li sdliy...@gmail.com

Looks good.

Reviewed-by: Lars-Peter Clausen l...@metafoo.de
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v1] staging: iio: hmc5843: Set iio name dynamically based on i2c name

[Lars-Peter Clausen]

On 08/12/2015 10:20 AM, sdliy...@gmail.com wrote:
 From: Yong Li sdliy...@gmail.com
 
 Load the driver using the below command:
 echo hmc5983 0x1e  /sys/bus/i2c/devices/i2c-?/new_device
 
 In sysfs, the iio name is hmc5843, however the i2c name is hmc5983,
 they are inconsistent.
 
 With this patch, the iio name will be the same as the i2c device name
 
 Signed-off-by: Yong Li sdliy...@gmail.com
 ---
  drivers/staging/iio/magnetometer/hmc5843_i2c.c | 1 +
  1 file changed, 1 insertion(+)
 
 diff --git a/drivers/staging/iio/magnetometer/hmc5843_i2c.c 
 b/drivers/staging/iio/magnetometer/hmc5843_i2c.c
 index ff08667..3b03644 100644
 --- a/drivers/staging/iio/magnetometer/hmc5843_i2c.c
 +++ b/drivers/staging/iio/magnetometer/hmc5843_i2c.c
 @@ -59,6 +59,7 @@ static const struct regmap_config hmc5843_i2c_regmap_config 
 = {
  static int hmc5843_i2c_probe(struct i2c_client *cli,
const struct i2c_device_id *id)
  {
 + cli-dev.driver-name = id-name;

You are overwriting a the name of the driver, which is a struct that is
shared between all instances of the device which specific data from one
device. That is most certainly not correct.

Update hmc5843_common_probe() and add a parameter that takes the name for
the device and then pass the id-name when the function is called.

   return hmc5843_common_probe(cli-dev,
   devm_regmap_init_i2c(cli, hmc5843_i2c_regmap_config),
   id-driver_data);
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 05/19] staging: iio: Remove unnecessary externs

[Lars-Peter Clausen]

On 08/10/2015 11:51 PM, Joe Perches wrote:
 Using 'extern' is not necessary for function prototypes.
 
 Signed-off-by: Joe Perches j...@perches.com

Acked-by: Lars-Peter Clausen l...@metafoo.de

Thanks.

 ---
  drivers/staging/iio/meter/ade7854.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/staging/iio/meter/ade7854.h 
 b/drivers/staging/iio/meter/ade7854.h
 index 52ca541..52f4195 100644
 --- a/drivers/staging/iio/meter/ade7854.h
 +++ b/drivers/staging/iio/meter/ade7854.h
 @@ -168,7 +168,7 @@ struct ade7854_state {
  
  };
  
 -extern int ade7854_probe(struct iio_dev *indio_dev, struct device *dev);
 -extern int ade7854_remove(struct iio_dev *indio_dev);
 +int ade7854_probe(struct iio_dev *indio_dev, struct device *dev);
 +int ade7854_remove(struct iio_dev *indio_dev);
  
  #endif
 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad7606_par: Constify platform_device_id

[Lars-Peter Clausen]

On 05/01/2015 05:43 PM, Krzysztof Kozlowski wrote:

The platform_device_id is not modified by the driver and core uses it as
const.

Signed-off-by: Krzysztof Kozlowski k.kozlowsk...@gmail.com


Acked-by: Lars-Peter Clausen l...@metafoo.de

Thanks.


---
  drivers/staging/iio/adc/ad7606_par.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7606_par.c 
b/drivers/staging/iio/adc/ad7606_par.c
index 9e24b4d4455f..1d48ae381d16 100644
--- a/drivers/staging/iio/adc/ad7606_par.c
+++ b/drivers/staging/iio/adc/ad7606_par.c
@@ -119,7 +119,7 @@ static const struct dev_pm_ops ad7606_pm_ops = {
  #define AD7606_PAR_PM_OPS NULL
  #endif  /* CONFIG_PM */

-static struct platform_device_id ad7606_driver_ids[] = {
+static const struct platform_device_id ad7606_driver_ids[] = {
{
.name   = ad7606-8,
.driver_data= ID_AD7606_8,



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad2s1200: Fix sign extension

[Lars-Peter Clausen]

On 01/23/2015 12:09 AM, Rasmus Villemoes wrote:

The line above makes vel a 12-bit quantity (st-rx[] is u8). The
intention is to sign-extend vel using bit 11 as the sign bit. But
because of C's promotion rules vel = (vel  4)  4; is actually a
no-op, since vel is promoted to int before the inner
shift. sign_extend32 works equally well for 8 and 16 bits types, so
use that.

Signed-off-by: Rasmus Villemoes li...@rasmusvillemoes.dk


Acked-by: Lars-Peter Clausen l...@metafoo.de
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: ad5933: fix format string warnings

[Lars-Peter Clausen]

On 01/20/2015 08:25 AM, Asaf Vertz wrote:

Fixed the following warnings (reported by cppcheck):
[drivers/staging/iio/impedance-analyzer/ad5933.c:363]: (warning) %d in format 
string (no. 1)
requires 'int' but the argument type is 'unsigned int'.
[drivers/staging/iio/impedance-analyzer/ad5933.c:367]: (warning) %d in format 
string (no. 1)
requires 'int' but the argument type is 'unsigned int'.
[drivers/staging/iio/impedance-analyzer/ad5933.c:367]: (warning) %d in format 
string (no. 2)
requires 'int' but the argument type is 'unsigned int'.
[drivers/staging/iio/impedance-analyzer/ad5933.c:367]: (warning) %d in format 
string (no. 3)
requires 'int' but the argument type is 'unsigned int'.
[drivers/staging/iio/impedance-analyzer/ad5933.c:367]: (warning) %d in format 
string (no. 4)
requires 'int' but the argument type is 'unsigned int'.

Signed-off-by: Asaf Vertz asaf.ve...@tandemg.com


Acked-by: Lars-Peter Clausen l...@metafoo.de

Thanks.


---
  drivers/staging/iio/impedance-analyzer/ad5933.c |4 ++--
  1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c 
b/drivers/staging/iio/impedance-analyzer/ad5933.c
index b6bd609..4230a43 100644
--- a/drivers/staging/iio/impedance-analyzer/ad5933.c
+++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
@@ -360,11 +360,11 @@ static ssize_t ad5933_show(struct device *dev,
mutex_lock(indio_dev-mlock);
switch ((u32) this_attr-address) {
case AD5933_OUT_RANGE:
-   len = sprintf(buf, %d\n,
+   len = sprintf(buf, %u\n,
  st-range_avail[(st-ctrl_hb  1)  0x3]);
break;
case AD5933_OUT_RANGE_AVAIL:
-   len = sprintf(buf, %d %d %d %d\n, st-range_avail[0],
+   len = sprintf(buf, %u %u %u %u\n, st-range_avail[0],
  st-range_avail[3], st-range_avail[2],
  st-range_avail[1]);
break;



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: virtpci: Remove no-op suspend/resume functions

[Lars-Peter Clausen]

The virtpci bus driver still uses the legacy suspend/resume callbacks. In
their current implementation these callbacks only contain a macro that
always expands to 'do { } while(0)'. So instead of converting them to dev PM
ops just remove them.

Signed-off-by: Lars-Peter Clausen l...@metafoo.de
---
 drivers/staging/unisys/virtpci/virtpci.c | 16 
 1 file changed, 16 deletions(-)

diff --git a/drivers/staging/unisys/virtpci/virtpci.c 
b/drivers/staging/unisys/virtpci/virtpci.c
index 39b828d..2d901ab 100644
--- a/drivers/staging/unisys/virtpci/virtpci.c
+++ b/drivers/staging/unisys/virtpci/virtpci.c
@@ -104,8 +104,6 @@ static ssize_t virtpci_driver_attr_store(struct kobject 
*kobj,
 const char *buf, size_t count);
 static int virtpci_bus_match(struct device *dev, struct device_driver *drv);
 static int virtpci_uevent(struct device *dev, struct kobj_uevent_env *env);
-static int virtpci_device_suspend(struct device *dev, pm_message_t state);
-static int virtpci_device_resume(struct device *dev);
 static int virtpci_device_probe(struct device *dev);
 static int virtpci_device_remove(struct device *dev);
 
@@ -128,8 +126,6 @@ static struct bus_type virtpci_bus_type = {
.name = uisvirtpci,
.match = virtpci_bus_match,
.uevent = virtpci_uevent,
-   .suspend = virtpci_device_suspend,
-   .resume = virtpci_device_resume,
 };
 
 static struct device virtpci_rootbus_device = {
@@ -757,18 +753,6 @@ static int virtpci_uevent(struct device *dev, struct 
kobj_uevent_env *env)
return 0;
 }
 
-static int virtpci_device_suspend(struct device *dev, pm_message_t state)
-{
-   DBGINF(In virtpci_device_suspend -NYI \n);
-   return 0;
-}
-
-static int virtpci_device_resume(struct device *dev)
-{
-   DBGINF(In virtpci_device_resume -NYI \n);
-   return 0;
-}
-
 /* For a child device just created on a client bus, fill in
  * information about the driver that is controlling this device into
  * the appropriate slot within the vbus channel of the bus
-- 
1.8.0

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] iio: Add support for waveform output

[Lars-Peter Clausen]

On 12/01/2014 03:57 PM, George McCollister wrote:

On Wed, Nov 26, 2014 at 4:06 PM, Lars-Peter Clausen l...@metafoo.de wrote:

On 11/26/2014 10:45 PM, George McCollister wrote:


Output can be held high or low for a specified period of time.
Support for waveform capture could be added in the future.



That's a PWM device?


The device I'm adding only generates a single pulse per write, but my
intention was to leave the door open for other types of devices.



Can you try to elaborate more on how exactly this interface is supposed to 
be used, what kind of the devices it is used for, the use case of the device 
and so on. This patch is adding userspace ABI which we have to maintain 
forever so we should try to give this proper review, which is not easy 
without knowing what it actually is.


- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 1/2] iio: Add support for waveform output

[Lars-Peter Clausen]

On 11/26/2014 10:45 PM, George McCollister wrote:

Output can be held high or low for a specified period of time.
Support for waveform capture could be added in the future.



That's a PWM device?
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: adc: mxs-lradc.c: sparse warning of incorrect type

[Lars-Peter Clausen]

On 09/04/2014 06:45 PM, Sudip Mukherjee wrote:

fixed sparse warning : incorrect type in argument 1
   (different address spaces)

Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
  drivers/staging/iio/adc/mxs-lradc.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c 
b/drivers/staging/iio/adc/mxs-lradc.c
index 468327f..d8d1fe1 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -1545,9 +1545,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
/* Grab the memory area */
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lradc-dev = pdev-dev;
-   lradc-base = devm_ioremap_resource(dev, iores);
-   if (IS_ERR(lradc-base))
-   return PTR_ERR(lradc-base);
+   void *tempptr;
+
+   tempptr = devm_ioremap_resource(dev, iores);
+   if (IS_ERR(tempptr))
+   return PTR_ERR(tempptr);
+   lradc-base = tempptr;


This makes no sense at all... devm_ioremap_resource() returns a iomem 
pointer lrdac-base is a iomem pointer.


Please make sure that you have the latest version of sparse installed on 
your system.




lradc-clk = devm_clk_get(pdev-dev, NULL);
if (IS_ERR(lradc-clk)) {



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: adc: mxs-lradc.c: sparse warning of incorrect type

[Lars-Peter Clausen]

On 09/04/2014 07:12 PM, Sudip Mukherjee wrote:

On Thu, Sep 04, 2014 at 06:50:05PM +0200, Lars-Peter Clausen wrote:

On 09/04/2014 06:45 PM, Sudip Mukherjee wrote:

fixed sparse warning : incorrect type in argument 1
   (different address spaces)

Signed-off-by: Sudip Mukherjee su...@vectorindia.org
---
  drivers/staging/iio/adc/mxs-lradc.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/adc/mxs-lradc.c 
b/drivers/staging/iio/adc/mxs-lradc.c
index 468327f..d8d1fe1 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -1545,9 +1545,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
/* Grab the memory area */
iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
lradc-dev = pdev-dev;
-   lradc-base = devm_ioremap_resource(dev, iores);
-   if (IS_ERR(lradc-base))
-   return PTR_ERR(lradc-base);
+   void *tempptr;
+
+   tempptr = devm_ioremap_resource(dev, iores);
+   if (IS_ERR(tempptr))
+   return PTR_ERR(tempptr);
+   lradc-base = tempptr;


This makes no sense at all... devm_ioremap_resource() returns a
iomem pointer lrdac-base is a iomem pointer.


sparse is giving warning for argument 1 of IS_ERR.


Not if you update to the latest version of sparse.





Please make sure that you have the latest version of sparse
installed on your system.



lradc-clk = devm_clk_get(pdev-dev, NULL);
if (IS_ERR(lradc-clk)) {





thanks
sudip



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio: moved platform_data into include/linux/iio

[Lars-Peter Clausen]

On 08/25/2014 09:17 AM, Sharma, Sanjeev wrote:

Hello Lars,

As per your suggestion Can I move complete Driver out of staging specially SPI 
ADC Driver.


Only if they are cleaned up first. All of the drivers that are still in 
staging do have issues, otherwise we'd already had moved them. A few of them 
are OK codestyle wise, but do have ABI issues which need to be resolved 
before they can be moved.


- Lars



Regards
Sanjeev Sharma

-Original Message-
From: Lars-Peter Clausen [mailto:l...@metafoo.de]
Sent: Wednesday, August 20, 2014 12:20 PM
To: sanjeev sharma
Cc: Sharma, Sanjeev; ji...@kernel.org; gregkh; linux-...@vger.kernel.org; 
devel; linux-kernel
Subject: Re: [PATCH] staging:iio: moved platform_data into include/linux/iio

On 08/20/2014 08:44 AM, sanjeev sharma wrote:

Hi,

This was the action item(TO-DO). IMO, it make sense to move into
include/linux/iio because IIO complete subsystem may take some time.


The code that is in staging is not supposed to 'leak' outside of staging. So 
either you move the driver as a whole out of staging or leave it there, but do 
not move individual files of the driver out of staging. The action item is for 
when the driver is moved out of staging.



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio: moved platform_data into include/linux/iio

[Lars-Peter Clausen]

On 08/25/2014 01:49 PM, Sharma, Sanjeev wrote:

-Original Message-
From: Lars-Peter Clausen [mailto:l...@metafoo.de]
Sent: Monday, August 25, 2014 1:39 PM
To: Sharma, Sanjeev; sanjeev sharma
Cc: ji...@kernel.org; gregkh; linux-...@vger.kernel.org; devel; linux-kernel
Subject: Re: [PATCH] staging:iio: moved platform_data into include/linux/iio

On 08/25/2014 09:17 AM, Sharma, Sanjeev wrote:

Hello Lars,

As per your suggestion Can I move complete Driver out of staging specially SPI 
ADC Driver.


Only if they are cleaned up first. All of the drivers that are still in staging 
do have issues, otherwise we'd already had moved them. A few of them are OK 
codestyle wise, but do have ABI issues which need to be resolved before they 
can be moved.

- Lars

Where I can find ABI issues which need to be resolved so that these can be 
looked upon.


Compare the sysfs files and their behavior that the driver registers with 
what is documented in the IIO ABI spec[1]. If you can't find it in the 
documentation it either needs to be documented or updated to use the 
existing ABI correctly. Note that this is not necessarily trivial and may 
require in-depth knowledge and understanding of the IIO ABI.


Thanks,
- Lars

[1] 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-bus-iio




-Sanjeev


Regards
Sanjeev Sharma

-Original Message-
From: Lars-Peter Clausen [mailto:l...@metafoo.de]
Sent: Wednesday, August 20, 2014 12:20 PM
To: sanjeev sharma
Cc: Sharma, Sanjeev; ji...@kernel.org; gregkh;
linux-...@vger.kernel.org; devel; linux-kernel
Subject: Re: [PATCH] staging:iio: moved platform_data into
include/linux/iio

On 08/20/2014 08:44 AM, sanjeev sharma wrote:

Hi,

This was the action item(TO-DO). IMO, it make sense to move into
include/linux/iio because IIO complete subsystem may take some time.


The code that is in staging is not supposed to 'leak' outside of staging. So 
either you move the driver as a whole out of staging or leave it there, but do 
not move individual files of the driver out of staging. The action item is for 
when the driver is moved out of staging.



N�r��y���b�X��ǧv�^�)޺{.n�+{��*��^n�r���z���h���G���h�(�階�ݢj���m�z�ޖ���f���h���~�mml==



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging:iio: moved platform_data into include/linux/iio

[Lars-Peter Clausen]

On 08/20/2014 08:41 AM, Sanjeev Sharma wrote:

This is a patch to the iio which will move all platform_data
into include/linux/iio.


It should be moved together with the driver.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Lars-Peter Clausen]

On 07/17/2014 11:20 AM, Arnd Bergmann wrote:

On Thursday 17 July 2014 09:27:58 Chen Gang wrote:

  gfp_t gfp_mask, unsigned int order);
  extern void devm_free_pages(struct device *dev, unsigned long addr);

+#ifdef CONFIG_HAS_IOMEM
  void __iomem *devm_ioremap_resource(struct device *dev, struct resource *res);
+#elif defined(CONFIG_COMPILE_TEST)
+static inline void __iomem *devm_ioremap_resource(struct device *dev,
+   struct resource *res)
+{
+   pr_warn(no hardware io memory, only for COMPILE_TEST\n);
+   return (__force void __iomem *)ERR_PTR(-ENXIO);
+}
+#endif /* CONFIG_HAS_IOMEM || CONFIG_COMPILE_TEST */

  /* allows to add/remove a custom action to devres stack */


To be honest, I think it's a bad idea to introduce wrappers functions
that are only available when CONFIG_COMPILE_TEST is set.

COMPILE_TEST is a great tool in general, but it has its limits.
In particular, the case for !CONFIG_IOMEM is completely obscure
and we won't find any bugs by allowing more drivers to be built
in those configurations, but attempting to do it would cause
endless churn by changing each instance of 'depends on HAS_IOMEM'
to 'depends on HAS_IOMEM || COMPILE_TEST'.


The point of this exercise is that we do not have to replace a good chunk of 
'depends on COMPILE_TEST' with 'depends on COMPILE_TEST  HAS_IOMEM'


E.g. the typical Kconfig entry for your random SoC peripheral driver looks like

config ARCH_FOOBAR_DRIVER
depends on ARCH_FOOBAR || COMPILE_TEST
...

Now when COMPILE_TEST is not set there is a implicit dependency on HAS_IOMEM 
since the architecture will provide it. If COMPILE_TEST is selected the 
driver will also be build-able on architectures that do no have HAS_IOMEM 
and hence linking the driver fails. One way to fix this is of course to 
replace the COMPILE_TEST with (COMPILE_TEST  HAS_IOMEM). But this is very 
often overlooked and only noticed later on when somebody actually builds a 
allyesconfig on an architecture that does not provide HAS_IOMEM. To avoid 
these kinds of build errors and tedious fixup patches the idea is to provide 
a stub function when HAS_IOMEM is not enabled, but COMPILE_TEST is enabled.


___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: adis16203: Use devm_iio_device_register

[Lars-Peter Clausen]

On 07/17/2014 10:40 PM, Himangi Saraogi wrote:

This patch introduces the use of iio_device_register and does away with
the call to the corressponding unregister function in the probe and
remove functions of the driver respectively.

Signed-off-by: Himangi Saraogi himangi...@gmail.com
Acked-by: Julia Lawall julia.law...@lip6.fr


No, you changed the relative order in which iio_device_unregister() and 
adis_cleanup_buffer_and_trigger() are called, this opens up the way for race 
conditions. Rule of thumb: Don't convert drivers to managed functions if 
this will change the order in which functions will called on device removal.


- Lars


---
  drivers/staging/iio/accel/adis16203_core.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16203_core.c 
b/drivers/staging/iio/accel/adis16203_core.c
index 409a28e..509 100644
--- a/drivers/staging/iio/accel/adis16203_core.c
+++ b/drivers/staging/iio/accel/adis16203_core.c
@@ -175,7 +175,7 @@ static int adis16203_probe(struct spi_device *spi)
if (ret)
goto error_cleanup_buffer_trigger;

-   ret = iio_device_register(indio_dev);
+   ret = devm_iio_device_register(spi-dev, indio_dev);
if (ret)
goto error_cleanup_buffer_trigger;

@@ -191,7 +191,6 @@ static int adis16203_remove(struct spi_device *spi)
struct iio_dev *indio_dev = spi_get_drvdata(spi);
struct adis *st = iio_priv(indio_dev);

-   iio_device_unregister(indio_dev);
adis_cleanup_buffer_and_trigger(st, indio_dev);

return 0;



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: question regarding drivers/staging/iio/adc/ad7280a.c

[Lars-Peter Clausen]

On 07/15/2014 07:31 PM, Jonathan Cameron wrote:

On 14/07/14 21:31, Himangi Saraogi wrote:

Hi,

The code seems to have a memory leak. The function ad7280_attr_init
calls kasprintf a number of times, which calls kmalloc (or more
precisely kmalloc_track_caller), but this data does not ever seem to
be freed. I propose to introduce a devm_  version of kasprintf, which
will be useful for other files also. I am not very sure that will it
be useful to introduce a bunch of kfrees, just to remove the memory
leaks immediately, but I think it would be safer just to devm
everything, so then one is sure that everything is freed as it should
be, in the right order.


The question here is whether such a memory leak squashing would be
worth applying to stable.  Personally I'd go with no. In which case
feel free to fix it via the introduction of a devm version.


devm is probably fine here.

The long term fix should be to switch the driver to use 
iio_chan_spec_ext_info infrastructure which will handle the allocation and 
freeing internally in the IIO framework.


- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Lars-Peter Clausen]

On 07/14/2014 10:31 AM, Richard Weinberger wrote:

Am 13.07.2014 22:17, schrieb Greg Kroah-Hartman:

On Sun, Jul 13, 2014 at 09:33:38PM +0200, Richard Weinberger wrote:

Maybe we could add COMPILE_TEST to the version string too?
Just to detect such kernels fast in user bug reports...


What kind of bug report are you going to get?


User manages to enable CONFIG_FOO by selecting COMPILE_TEST and
complains that it does not work. :)


These drivers are typically drivers for some SoC peripheral and the device 
will simply physically not exist on a platform that does not provide 
HAS_IOMEM. This is not really any different from making the driver 
selectable via COMPILE_TEST for any other platform. To hit the issue you'd 
have to instantiate a device driver instance for a device that physically 
does not exist. This will always result in a failure.


- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v3 2/5] staging:iio:hmc5843: Split hmc5843.c to multiple files

[Lars-Peter Clausen]

On 07/08/2014 03:39 PM, Josef Gajdusek wrote:
[...]

diff --git a/drivers/staging/iio/magnetometer/Kconfig 
b/drivers/staging/iio/magnetometer/Kconfig
index ad88d61..28c2612 100644
--- a/drivers/staging/iio/magnetometer/Kconfig
+++ b/drivers/staging/iio/magnetometer/Kconfig
@@ -5,15 +5,23 @@ menu Magnetometer sensors

  config SENSORS_HMC5843
tristate Honeywell HMC5843/5883/5883L 3-Axis Magnetometer
-   depends on I2C
+   depends on (I2C || SPI_MASTER)
select IIO_BUFFER
select IIO_TRIGGERED_BUFFER
-   select REGMAP_I2C
+   select SENSORS_HMC5843_I2C if (I2C)


This approach doesn't work to well and will cause randconfig build failures. 
If SPI_MASTER is 'y' and I2C is 'm' you'll be able to select this driver as 
built-in, which in turn will also select SENSORS_HMC5843_I2C as built-in, 
which means you'll get unresolved symbols during linking since core I2C 
support is built as a module. A better approach is to have a user-selectable 
symbol per bus and a non-user-selectable symbol for the core infrastructure 
of the driver. e.g.


config SENSORS_HMC5843
tristate
select IIO_BUFFER
...

config SENSORS_HMC5843_I2C
tristate Honeywell HMC5843/5883/5883L 3-Axis Magnetometer (I2C)
select SENSORS_HMC5843
select REGMAP_I2C

config SENSORS_HMC5843_SPI
tristate Honeywell HMC5843/5883/5883L 3-Axis Magnetometer (SPI)
select SENSORS_HMC5843
select REGMAP_SPI



 +struct regmap_config hmc5843_i2c_regmap_config = {

static

 +  .reg_bits = 8,
 +  .val_bits = 8,
 +
 +  .rd_table = hmc5843_readable_table,
 +  .wr_table = hmc5843_writable_table,
 +  .volatile_table = hmc5843_volatile_table,
 +
 +  .cache_type = REGCACHE_RBTREE,
 +};


 +static int hmc5843_i2c_probe(struct i2c_client *client,
 +   const struct i2c_device_id *id)
 +{
 +  struct hmc5843_data *data;
 +  struct iio_dev *indio_dev;
 +
 +  indio_dev = devm_iio_device_alloc(client-dev, sizeof(*data));
 +  if (indio_dev == NULL)
 +  return -ENOMEM;
 +
 +  i2c_set_clientdata(client, indio_dev);
 +
 +  data = iio_priv(indio_dev);
 +  data-dev = client-dev;
 +  data-regmap = devm_regmap_init_i2c(client, hmc5843_i2c_regmap_config);
 +
 +  indio_dev-name = id-name;
 +  indio_dev-dev.parent = client-dev;
 +
 +  return hmc5843_common_probe(indio_dev, id-driver_data);
 +}

If you do the allocation of the IIO device in the common function this can 
be simplified a bit. E.g.


static int hmc5843_i2c_probe(struct i2c_client *client,
 const struct i2c_device_id *id)
{
return hmc5853_common_probe(client-dev,
devm_regmap_init_i2c(client, mc5843_i2c_regmap_config),
id-driver_data);
}


 +#ifdef CONFIG_PM_SLEEP
 +static int hmc5843_i2c_suspend(struct device *dev)
 +{
 +  return hmc5843_common_suspend(i2c_get_clientdata(to_i2c_client(dev)));
 +}
 +
 +static int hmc5843_i2c_resume(struct device *dev)
 +{
 +  return hmc5843_common_resume(i2c_get_clientdata(to_i2c_client(dev)));
 +}
 +
 +static SIMPLE_DEV_PM_OPS(hmc5843_pm_ops,
 +  hmc5843_i2c_suspend, hmc5843_i2c_resume);
 +#define HMC5843_PM_OPS (hmc5843_pm_ops)
 +#else
 +#define HMC5843_PM_OPS NULL
 +#endif

Those ops will be the same for both SPI and I2C. 
i2c_get_clientdata(to_i2c_client(dev)) is the same as dev_get_drvdata(dev), 
so this can go into the core driver.



Also as a hint for future patches, if you rename a file use 
'git-format-patch -M', this will make the patch a bit more legible.


- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Lars-Peter Clausen]

On 07/13/2014 11:45 AM, Richard Weinberger wrote:

Am 13.07.2014 11:27, schrieb Lennox Wu:

As I said before, some configurations don't make sense.


If such a configuration can be achieved using allmod/yesconfig it has to be 
fixed.
Chen's fixes seem reasonable as not all architectures support iomem.


Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to 
avoid these linker errors. That's in my opinion better than turning most of the 
'depends on COMPILE_TEST' into 'depends on COMPILE_TEST  HAS_IOMEM'. The 
issue comes up quite a lot and it is often overlooked when adding a driver that 
can be build when COMPILE_TEST is enabled.


- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] drivers: Let several drivers depends on HAS_IOMEM for 'devm_ioremap_resource'

[Lars-Peter Clausen]

On 07/13/2014 03:40 PM, Richard Weinberger wrote:

Am 13.07.2014 15:26, schrieb Lars-Peter Clausen:

On 07/13/2014 11:45 AM, Richard Weinberger wrote:

Am 13.07.2014 11:27, schrieb Lennox Wu:

As I said before, some configurations don't make sense.


If such a configuration can be achieved using allmod/yesconfig it has to be 
fixed.
Chen's fixes seem reasonable as not all architectures support iomem.


Maybe we should stub out ioremap() and friends when COMPILE_TEST is enabled to 
avoid these linker errors. That's in my opinion better than turning most of the 
'depends on
COMPILE_TEST' into 'depends on COMPILE_TEST  HAS_IOMEM'. The issue comes up 
quite a lot and it is often overlooked when adding a driver that can be build when 
COMPILE_TEST is
enabled.


And what should this stub do?
Except calling BUG()...


return NULL;

It's for compile testing, it's not meant to work at runtime.

- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: iio: Introduce the use of devm_ioremap_resource

[Lars-Peter Clausen]

On 07/07/2014 02:36 PM, Jonathan Cameron wrote:

On 01/07/14 19:44, Himangi Saraogi wrote:

This patch introduces the use of devm_ioremap_resource. It does away
with call to request_mem_region and the error checking on
platform_get_resource. Also, the calls to free the allocated resources
like release_mem_region and iounmap are done away with. The ret variable
in the probe function is also eliminated. Also, a bug is fixed as the
goto in the error handling of request_mem_region should not have
called release_mem_region which releases a resource that has not been
allocated.

Signed-off-by: Himangi Saraogi himangi...@gmail.com
Acked-by: Julia Lawall julia.law...@lip6.fr

Again, looks superficially fine, but I'd like an ack from someone
at Analog ideally.
Cc'd Lars


Acked-by: Lars-Peter Clausen l...@metafoo.de

Thanks.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] iio:trigger: Introduce the use of devm_kzalloc

[Lars-Peter Clausen]

On 07/07/2014 02:52 PM, Julia Lawall wrote:

On Mon, 7 Jul 2014, Jonathan Cameron wrote:


On 01/07/14 22:19, Himangi Saraogi wrote:

This patch introduces the use of the managed version of kzalloc and
removes the kfrees in the probe and remove functions. More return
paths are added and the labels are renamed to order them.

Signed-off-by: Himangi Saraogi himangi...@gmail.com

Looks sane, but I'd like an ack or reviewed-by from someone as Analog for this
one.
cc'd Lars and Michael.


Should MAINTAINERS be updated somehow?

julia


yea, it's been on my todo list for a while.

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void

[Lars-Peter Clausen]

On 06/09/2014 01:18 AM, Ben Dooks wrote:

On Fri, May 30, 2014 at 08:16:59PM +0200, Lars-Peter Clausen wrote:

On 05/30/2014 07:33 PM, David Daney wrote:

On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:

On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe berthe...@gmail.com
wrote:

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
gpio_chip *gpiochip);
   *
   * A gpio_chip with any GPIOs still requested may not be removed.
   */
-int gpiochip_remove(struct gpio_chip *chip)
+void gpiochip_remove(struct gpio_chip *chip)
  {
 unsigned long   flags;
-   int status = 0;
 unsignedid;

 acpi_gpiochip_remove(chip);
@@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
 of_gpiochip_remove(chip);

 for (id = 0; id  chip-ngpio; id++) {
-   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags)) {
-   status = -EBUSY;
-   break;
-   }
-   }
-   if (status == 0) {
-   for (id = 0; id  chip-ngpio; id++)
-   chip-desc[id].chip = NULL;
-
-   list_del(chip-list);
+   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags))
+   panic(gpio: removing gpiochip with gpios still
requested\n);


panic?


NACK to the patch for this reason.  The strongest thing you should do here
is WARN.

That said, I am not sure why we need this whole patch set in the first place.


Well, what currently happens when you remove a device that is a
provider of a gpio_chip which is still in use, is that the kernel
crashes. Probably with a rather cryptic error message. So this patch
doesn't really change the behavior, but makes it more explicit what
is actually wrong. And even if you replace the panic() by a WARN()
it will again just crash slightly later.

This is a design flaw in the GPIO subsystem that needs to be fixed.


Surely then the best way is to error out to the module unload and
stop the driver being unloaded?



You can't error out on module unload, although that's not really relevant 
here. gpiochip_remove() is typically called when the device that registered 
the GPIO chip is unbound. And despite some remove() callbacks having a 
return type of int you can not abort the removal of a device.


- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void

[Lars-Peter Clausen]

On 05/31/2014 01:29 AM, Greg KH wrote:

On Fri, May 30, 2014 at 08:16:59PM +0200, Lars-Peter Clausen wrote:

On 05/30/2014 07:33 PM, David Daney wrote:

On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:

On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe berthe...@gmail.com
wrote:

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
gpio_chip *gpiochip);
   *
   * A gpio_chip with any GPIOs still requested may not be removed.
   */
-int gpiochip_remove(struct gpio_chip *chip)
+void gpiochip_remove(struct gpio_chip *chip)
  {
 unsigned long   flags;
-   int status = 0;
 unsignedid;

 acpi_gpiochip_remove(chip);
@@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
 of_gpiochip_remove(chip);

 for (id = 0; id  chip-ngpio; id++) {
-   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags)) {
-   status = -EBUSY;
-   break;
-   }
-   }
-   if (status == 0) {
-   for (id = 0; id  chip-ngpio; id++)
-   chip-desc[id].chip = NULL;
-
-   list_del(chip-list);
+   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags))
+   panic(gpio: removing gpiochip with gpios still
requested\n);


panic?


NACK to the patch for this reason.  The strongest thing you should do here
is WARN.

That said, I am not sure why we need this whole patch set in the first place.


Well, what currently happens when you remove a device that is a provider of
a gpio_chip which is still in use, is that the kernel crashes. Probably with
a rather cryptic error message. So this patch doesn't really change the
behavior, but makes it more explicit what is actually wrong. And even if you
replace the panic() by a WARN() it will again just crash slightly later.

This is a design flaw in the GPIO subsystem that needs to be fixed.


Then fix the GPIO code properly, don't add a new panic() to the kernel.


Until somebody comes up with a patch that fixes this for good I think that 
patch is still an improvement over the current situation. Rather than 
keeping the kernel running in a inconsistent state, which might cause all 
kinds of undefined behavior and which will lead to a crash eventually, we 
might as well just crash the kernel at the cause of the inconsistent state. 
This will make it obvious why it crashed (compared to a random stacktrace) 
and will also prevent the kernel from running in a undefined state.


- Lars

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 2/2] gpio: gpiolib: set gpiochip_remove retval to void

[Lars-Peter Clausen]

On 05/30/2014 07:33 PM, David Daney wrote:

On 05/30/2014 04:39 AM, Geert Uytterhoeven wrote:

On Fri, May 30, 2014 at 1:30 PM, abdoulaye berthe berthe...@gmail.com
wrote:

--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1263,10 +1263,9 @@ static void gpiochip_irqchip_remove(struct
gpio_chip *gpiochip);
   *
   * A gpio_chip with any GPIOs still requested may not be removed.
   */
-int gpiochip_remove(struct gpio_chip *chip)
+void gpiochip_remove(struct gpio_chip *chip)
  {
 unsigned long   flags;
-   int status = 0;
 unsignedid;

 acpi_gpiochip_remove(chip);
@@ -1278,24 +1277,15 @@ int gpiochip_remove(struct gpio_chip *chip)
 of_gpiochip_remove(chip);

 for (id = 0; id  chip-ngpio; id++) {
-   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags)) {
-   status = -EBUSY;
-   break;
-   }
-   }
-   if (status == 0) {
-   for (id = 0; id  chip-ngpio; id++)
-   chip-desc[id].chip = NULL;
-
-   list_del(chip-list);
+   if (test_bit(FLAG_REQUESTED, chip-desc[id].flags))
+   panic(gpio: removing gpiochip with gpios still
requested\n);


panic?


NACK to the patch for this reason.  The strongest thing you should do here
is WARN.

That said, I am not sure why we need this whole patch set in the first place.


Well, what currently happens when you remove a device that is a provider of 
a gpio_chip which is still in use, is that the kernel crashes. Probably with 
a rather cryptic error message. So this patch doesn't really change the 
behavior, but makes it more explicit what is actually wrong. And even if you 
replace the panic() by a WARN() it will again just crash slightly later.


This is a design flaw in the GPIO subsystem that needs to be fixed.

- Lars
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] fix some coding style in drivers/staging/iio

[Lars-Peter Clausen]

[...]

diff --git a/drivers/staging/iio/accel/adis16220_core.c 
b/drivers/staging/iio/accel/adis16220_core.c
index 6f38ca9..31c7a9d 100644
--- a/drivers/staging/iio/accel/adis16220_core.c
+++ b/drivers/staging/iio/accel/adis16220_core.c
@@ -392,7 +392,8 @@ static const struct iio_info adis16220_info = {
  };

  static const char * const adis16220_status_error_msgs[] = {
-   [ADIS16220_DIAG_STAT_VIOLATION_BIT] = Capture period 
violation/interruption,
+   [ADIS16220_DIAG_STAT_VIOLATION_BIT] =
+   Capture period violation/interruption,


This does not improve legibility. The 80 chars per line rule is to improve 
legibility, if it doesn't it's better to ignore it.



[ADIS16220_DIAG_STAT_SPI_FAIL_BIT] = SPI failure,
[ADIS16220_DIAG_STAT_FLASH_UPT_BIT] = Flash update failed,
[ADIS16220_DIAG_STAT_POWER_HIGH_BIT] = Power supply above 3.625V,

[...]

diff --git a/drivers/staging/iio/resolver/ad2s1200.c 
b/drivers/staging/iio/resolver/ad2s1200.c
index 36eedd8..d38df2e 100644
--- a/drivers/staging/iio/resolver/ad2s1200.c
+++ b/drivers/staging/iio/resolver/ad2s1200.c
@@ -70,6 +70,7 @@ static int ad2s1200_read_raw(struct iio_dev *indio_dev,
vel = (((s16)(st-rx[0]))  4) | ((st-rx[1]  0xF0)  4);
vel = (vel  4)  4;
*val = vel;
+   /* fall through */


This not a coding style issue, here is actually a break missing.


default:
mutex_unlock(st-lock);
return -EINVAL;



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel