[PATCH 0/5] Adding scale support to the lpc32xx ADC driver

[PATCH 0/5] Adding scale support to the lpc32xx ADC driver

[Gregory CLEMENT]

Hello,

This series adds the support of the scale feature to the lpc32xx ADC
driver. In order to use it we need to have the voltage reference as a
device tree property, however in order to be backward compatible, this
property is optional and do not prevent to use the driver if missing.

I updated the binding documentation accordingly.

While I was on this driver, I made also some clean-up.

Gregory

Gregory CLEMENT (5):
  dt-bindings: iio: adc: move lpc32xx-adc out of staging
  dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply
  iio:adc:lpc32xx use SPDX-License-Identifier
  iio:adc:lpc32xx Cleanup headers
  iio:adc:lpc32xx Add scale feature

 .../{staging => }/iio/adc/lpc32xx-adc.txt     |  5 ++
 drivers/iio/adc/lpc32xx_adc.c                 | 57 +++++++++----------
 2 files changed, 33 insertions(+), 29 deletions(-)
 rename Documentation/devicetree/bindings/{staging => }/iio/adc/lpc32xx-adc.txt (67%)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] dt-bindings: iio: adc: move lpc32xx-adc out of staging

[Gregory CLEMENT]

The drivers has been moved out since
0097e20e7771 ("staging:iio:adc:lpc32xx Move out of staging.") in v4.10,
so let's align the binding documentation.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 .../devicetree/bindings/{staging => }/iio/adc/lpc32xx-adc.txt     | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/{staging => }/iio/adc/lpc32xx-adc.txt (100%)

diff --git a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
similarity index 100%
rename from Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
rename to Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Gregory CLEMENT]

As most of the other ADC the lpc32xx one use a vref-supply property:
document it.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
index b3629d3a9adf..3a1bc669bd51 100644
--- a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
@@ -6,6 +6,10 @@ Required properties:
   region.
 - interrupts: The ADC interrupt
 
+Optional:
+ - vref-supply: The regulator supply ADC reference voltage, optional
+   for legacy reason, but highly encouraging to us in new device tree
+
 Example:
 
 	adc@40048000 {
@@ -13,4 +17,5 @@ Example:
 		reg = <0x40048000 0x1000>;
 		interrupt-parent = <&mic>;
 		interrupts = <39 0>;
+		vref-supply = <&vcc>;
 	};
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] iio:adc:lpc32xx use SPDX-License-Identifier

[Gregory CLEMENT]

Convert the driver to SPDX license description which allow removing
several lines in the file.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/iio/adc/lpc32xx_adc.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
index 20b36690fa4f..e361c1532a75 100644
--- a/drivers/iio/adc/lpc32xx_adc.c
+++ b/drivers/iio/adc/lpc32xx_adc.c
@@ -1,23 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  *  lpc32xx_adc.c - Support for ADC in LPC32XX
  *
  *  3-channel, 10-bit ADC
  *
  *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
- *
- *  This program is free software; you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation; either version 2 of the License, or
- *  (at your option) any later version.
- *
- *  This program is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with this program; if not, write to the Free Software
- *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
 #include <linux/module.h>
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] iio:adc:lpc32xx Cleanup headers

[Gregory CLEMENT]

A few headers was useless: remove them, and also sort them in alphabetic
order.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/iio/adc/lpc32xx_adc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
index e361c1532a75..f391c1e10136 100644
--- a/drivers/iio/adc/lpc32xx_adc.c
+++ b/drivers/iio/adc/lpc32xx_adc.c
@@ -7,20 +7,13 @@
  *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
  */
 
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/interrupt.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/slab.h>
-#include <linux/io.h>
 #include <linux/clk.h>
-#include <linux/err.h>
 #include <linux/completion.h>
-#include <linux/of.h>
-
 #include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
 
 /*
  * LPC32XX registers definitions
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] iio:adc:lpc32xx Add scale feature

[Gregory CLEMENT]

Until now this driver only exposed the raw value of the channels. With
this patch, the scale value is also exposed.

It depends of a regulator supply, and unlike most of the other driver, do
not having this regulator won't prevent to use the driver. The reason for
it is to allow to continue to use this driver with an old device tree. If
there is no regulator supply then the scale won't be exposed.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/iio/adc/lpc32xx_adc.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
index f391c1e10136..e36ca307f065 100644
--- a/drivers/iio/adc/lpc32xx_adc.c
+++ b/drivers/iio/adc/lpc32xx_adc.c
@@ -14,6 +14,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
 
 /*
  * LPC32XX registers definitions
@@ -45,6 +46,7 @@ struct lpc32xx_adc_state {
 	void __iomem *adc_base;
 	struct clk *clk;
 	struct completion completion;
+	struct regulator *vref;
 
 	u32 value;
 };
@@ -57,7 +59,9 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
 {
 	struct lpc32xx_adc_state *st = iio_priv(indio_dev);
 	int ret;
-	if (mask == IIO_CHAN_INFO_RAW) {
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
 		mutex_lock(&indio_dev->mlock);
 		ret = clk_prepare_enable(st->clk);
 		if (ret) {
@@ -77,6 +81,12 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
 		mutex_unlock(&indio_dev->mlock);
 
 		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = regulator_get_voltage(st->vref) / 1000;
+		*val2 =  chan->scan_type.realbits;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
 	}
 
 	return -EINVAL;
@@ -93,9 +103,10 @@ static const struct iio_info lpc32xx_adc_iio_info = {
 	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
 	.address = LPC32XXAD_IN * _index,		\
 	.scan_index = _index,				\
+	.scan_type.realbits = 10			\
 }
 
-static const struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
+static struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
 	LPC32XX_ADC_CHANNEL(0),
 	LPC32XX_ADC_CHANNEL(1),
 	LPC32XX_ADC_CHANNEL(2),
@@ -119,7 +130,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
 	struct resource *res;
 	int retval = -ENODEV;
 	struct iio_dev *iodev = NULL;
-	int irq;
+	int irq, i;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -159,6 +170,15 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
 		return retval;
 	}
 
+	st->vref = devm_regulator_get(&pdev->dev, "vref");
+	if (IS_ERR(st->vref))
+		dev_err(&pdev->dev,
+			"Missing vref regulator: No scaling available\n");
+	else
+		for (i = 0; i <  ARRAY_SIZE(lpc32xx_adc_iio_channels); i++)
+			lpc32xx_adc_iio_channels[i].info_mask_shared_by_type =
+				BIT(IIO_CHAN_INFO_SCALE);
+
 	platform_set_drvdata(pdev, iodev);
 
 	init_completion(&st->completion);
@@ -175,7 +195,6 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
 		return retval;
 
 	dev_info(&pdev->dev, "LPC32XX ADC driver loaded, IRQ %d\n", irq);
-
 	return 0;
 }
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] dt-bindings: iio: adc: move lpc32xx-adc out of staging

[Jonathan Cameron]

On Fri,  8 Feb 2019 17:09:40 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> The drivers has been moved out since
> 0097e20e7771 ("staging:iio:adc:lpc32xx Move out of staging.") in v4.10,
> so let's align the binding documentation.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Oops. My fault.

You forgot the iio list on these so +cc

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it.

Thanks,

Jonathan

> ---
>  .../devicetree/bindings/{staging => }/iio/adc/lpc32xx-adc.txt     | 0
>  1 file changed, 0 insertions(+), 0 deletions(-)
>  rename Documentation/devicetree/bindings/{staging => }/iio/adc/lpc32xx-adc.txt (100%)
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> similarity index 100%
> rename from Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
> rename to Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Jonathan Cameron]

On Fri,  8 Feb 2019 17:09:41 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> As most of the other ADC the lpc32xx one use a vref-supply property:
> document it.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Hmm. This is indeed an oddity as you document. Normally we would
insist on it, but we can't because of legacy and as it is actually
queries, we can't even use the fact a stub regulator will be provided
to get around it.

I'll have some comments on the patch implementing it anyway, but
good to let this sit for a while given it's slightly unusual nature.

Thanks,

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> index b3629d3a9adf..3a1bc669bd51 100644
> --- a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> @@ -6,6 +6,10 @@ Required properties:
>    region.
>  - interrupts: The ADC interrupt
>  
> +Optional:
> + - vref-supply: The regulator supply ADC reference voltage, optional
> +   for legacy reason, but highly encouraging to us in new device tree
> +
>  Example:
>  
>  	adc@40048000 {
> @@ -13,4 +17,5 @@ Example:
>  		reg = <0x40048000 0x1000>;
>  		interrupt-parent = <&mic>;
>  		interrupts = <39 0>;
> +		vref-supply = <&vcc>;
>  	};


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] iio:adc:lpc32xx use SPDX-License-Identifier

[Jonathan Cameron]

On Fri,  8 Feb 2019 17:09:42 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Convert the driver to SPDX license description which allow removing
> several lines in the file.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/lpc32xx_adc.c | 15 +--------------
>  1 file changed, 1 insertion(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index 20b36690fa4f..e361c1532a75 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -1,23 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0+
>  /*
>   *  lpc32xx_adc.c - Support for ADC in LPC32XX
>   *
>   *  3-channel, 10-bit ADC
>   *
>   *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
> - *
> - *  This program is free software; you can redistribute it and/or modify
> - *  it under the terms of the GNU General Public License as published by
> - *  the Free Software Foundation; either version 2 of the License, or
> - *  (at your option) any later version.
> - *
> - *  This program is distributed in the hope that it will be useful,
> - *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> - *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> - *  GNU General Public License for more details.
> - *
> - *  You should have received a copy of the GNU General Public License
> - *  along with this program; if not, write to the Free Software
> - *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
>  #include <linux/module.h>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iio:adc:lpc32xx Cleanup headers

[Jonathan Cameron]

On Fri,  8 Feb 2019 17:09:43 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> A few headers was useless: remove them, and also sort them in alphabetic
> order.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Hmm. Given the headers in question are mostly only useless (I think)
in the sense they are always included by something else, I'm
not sure this patch is worth the churn.

It's also tricky to see which ones were actually removed
given the combination with sorting.
I think it's just kernel.h, device.h, err.h all of which
are used in various ways and often directly included.

The other one is the iio/sysfs.h file.  That one we could
do to eventually kill off entirely, so happy to see that
one alone go.

Jonathan

> ---
>  drivers/iio/adc/lpc32xx_adc.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index e361c1532a75..f391c1e10136 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -7,20 +7,13 @@
>   *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
>   */
>  
> -#include <linux/module.h>
> -#include <linux/platform_device.h>
> -#include <linux/interrupt.h>
> -#include <linux/device.h>
> -#include <linux/kernel.h>
> -#include <linux/slab.h>
> -#include <linux/io.h>
>  #include <linux/clk.h>
> -#include <linux/err.h>
>  #include <linux/completion.h>
> -#include <linux/of.h>
> -
>  #include <linux/iio/iio.h>
> -#include <linux/iio/sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
>  
>  /*
>   * LPC32XX registers definitions


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iio:adc:lpc32xx Add scale feature

[Jonathan Cameron]

On Fri,  8 Feb 2019 17:09:44 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Until now this driver only exposed the raw value of the channels. With
> this patch, the scale value is also exposed.
> 
> It depends of a regulator supply, and unlike most of the other driver, do
> not having this regulator won't prevent to use the driver. The reason for
> it is to allow to continue to use this driver with an old device tree. If
> there is no regulator supply then the scale won't be exposed.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Hi Gregory,

A few minor comments inline.

I'll admit to being surprised to see any patches at all for this driver given
how long it has been since we had any known users.  We nearly dropped it as
unused years ago IIRC!  Good thing we didn't it seems.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/lpc32xx_adc.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> index f391c1e10136..e36ca307f065 100644
> --- a/drivers/iio/adc/lpc32xx_adc.c
> +++ b/drivers/iio/adc/lpc32xx_adc.c
> @@ -14,6 +14,7 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
>  
>  /*
>   * LPC32XX registers definitions
> @@ -45,6 +46,7 @@ struct lpc32xx_adc_state {
>  	void __iomem *adc_base;
>  	struct clk *clk;
>  	struct completion completion;
> +	struct regulator *vref;
>  
>  	u32 value;
>  };
> @@ -57,7 +59,9 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct lpc32xx_adc_state *st = iio_priv(indio_dev);
>  	int ret;
> -	if (mask == IIO_CHAN_INFO_RAW) {
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
>  		mutex_lock(&indio_dev->mlock);
>  		ret = clk_prepare_enable(st->clk);
>  		if (ret) {
> @@ -77,6 +81,12 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>  		mutex_unlock(&indio_dev->mlock);
>  
>  		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = regulator_get_voltage(st->vref) / 1000;
> +		*val2 =  chan->scan_type.realbits;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;

Please add a default, otherwise we are going to get some compiler
warnings that are irritating as it will think we 'forgot' to handle
the other cases.

>  	}
>  
>  	return -EINVAL;
> @@ -93,9 +103,10 @@ static const struct iio_info lpc32xx_adc_iio_info = {
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>  	.address = LPC32XXAD_IN * _index,		\
>  	.scan_index = _index,				\
> +	.scan_type.realbits = 10			\

Given scan_type is only used in the core in buffered modes
that this driver doesn't support this is a little 'unusual'.

Also, only one value, so why bother rather than just putting it
in the one place it is used?

>  }
>  
> -static const struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
> +static struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
Please provide two const versions and pick between them rather than
modifying the one.

Clearly we might 'know' there is only ever one such ADC on these devices
but it's not obvious to a reviewer who isn't familiar with the hardware,
making this look like a bug.

>  	LPC32XX_ADC_CHANNEL(0),
>  	LPC32XX_ADC_CHANNEL(1),
>  	LPC32XX_ADC_CHANNEL(2),
> @@ -119,7 +130,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	int retval = -ENODEV;
>  	struct iio_dev *iodev = NULL;
> -	int irq;
> +	int irq, i;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!res) {
> @@ -159,6 +170,15 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>  		return retval;
>  	}
>  
> +	st->vref = devm_regulator_get(&pdev->dev, "vref");
> +	if (IS_ERR(st->vref))
> +		dev_err(&pdev->dev,
> +			"Missing vref regulator: No scaling available\n");
> +	else
> +		for (i = 0; i <  ARRAY_SIZE(lpc32xx_adc_iio_channels); i++)
> +			lpc32xx_adc_iio_channels[i].info_mask_shared_by_type =
> +				BIT(IIO_CHAN_INFO_SCALE);
> +
>  	platform_set_drvdata(pdev, iodev);
>  
>  	init_completion(&st->completion);
> @@ -175,7 +195,6 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>  		return retval;
>  
>  	dev_info(&pdev->dev, "LPC32XX ADC driver loaded, IRQ %d\n", irq);
> -
Unrelated (and in my view) bad whitespace change.
>  	return 0;
>  }
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Vladimir Zapolskiy]

Hi Gregory,

On 02/08/2019 06:09 PM, Gregory CLEMENT wrote:
> As most of the other ADC the lpc32xx one use a vref-supply property:
> document it.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> index b3629d3a9adf..3a1bc669bd51 100644
> --- a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
> @@ -6,6 +6,10 @@ Required properties:
>    region.
>  - interrupts: The ADC interrupt
>  
> +Optional:
> + - vref-supply: The regulator supply ADC reference voltage, optional
> +   for legacy reason, but highly encouraging to us in new device tree
> +

here vref-supply voltage is 3.3v only, I think that might be a reason why
the property is omitted.

My concern is that the documentation shall not contain sections with
description of properties found in the past for 'legacy reasons', if you
like to support the backward compatibility in the driver, please update
the documentation to the expected state and add a stub to care about
legacy DTBs in the driver only.

FWIW I personally don't mind about breaking the backward compatibility
for NXP LPC32xx, it might be a concern when CONFIG_ARM_ATAG_DTB_COMPAT=y
is removed from the lpc32xx_defconfig though.

>  Example:
>  
>  	adc@40048000 {
> @@ -13,4 +17,5 @@ Example:
>  		reg = <0x40048000 0x1000>;
>  		interrupt-parent = <&mic>;
>  		interrupts = <39 0>;
> +		vref-supply = <&vcc>;
>  	};
> 

I kindly ask you to include me to To: list, if you send another changes for
NXP LPC32xx platform.

Thank you.

--
Best wishes,
Vladimir

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] iio:adc:lpc32xx Add scale feature

[Gregory CLEMENT]

Hi Jonathan,
 
 On sam., févr. 09 2019, Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri,  8 Feb 2019 17:09:44 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
>
>> Until now this driver only exposed the raw value of the channels. With
>> this patch, the scale value is also exposed.
>> 
>> It depends of a regulator supply, and unlike most of the other driver, do
>> not having this regulator won't prevent to use the driver. The reason for
>> it is to allow to continue to use this driver with an old device tree. If
>> there is no regulator supply then the scale won't be exposed.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Hi Gregory,
>
> A few minor comments inline.
>
> I'll admit to being surprised to see any patches at all for this driver given
> how long it has been since we had any known users.  We nearly dropped it as
> unused years ago IIRC!  Good thing we didn't it seems.
>
> Thanks,
>
> Jonathan
>
>> ---
>>  drivers/iio/adc/lpc32xx_adc.c | 27 +++++++++++++++++++++++----
>>  1 file changed, 23 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
>> index f391c1e10136..e36ca307f065 100644
>> --- a/drivers/iio/adc/lpc32xx_adc.c
>> +++ b/drivers/iio/adc/lpc32xx_adc.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>>  
>>  /*
>>   * LPC32XX registers definitions
>> @@ -45,6 +46,7 @@ struct lpc32xx_adc_state {
>>  	void __iomem *adc_base;
>>  	struct clk *clk;
>>  	struct completion completion;
>> +	struct regulator *vref;
>>  
>>  	u32 value;
>>  };
>> @@ -57,7 +59,9 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>>  {
>>  	struct lpc32xx_adc_state *st = iio_priv(indio_dev);
>>  	int ret;
>> -	if (mask == IIO_CHAN_INFO_RAW) {
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>>  		mutex_lock(&indio_dev->mlock);
>>  		ret = clk_prepare_enable(st->clk);
>>  		if (ret) {
>> @@ -77,6 +81,12 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>>  		mutex_unlock(&indio_dev->mlock);
>>  
>>  		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = regulator_get_voltage(st->vref) / 1000;
>> +		*val2 =  chan->scan_type.realbits;
>> +
>> +		return IIO_VAL_FRACTIONAL_LOG2;
>
> Please add a default, otherwise we are going to get some compiler
> warnings that are irritating as it will think we 'forgot' to handle
> the other cases.

Sure, I will do it.

>
>>  	}
>>  
>>  	return -EINVAL;
>> @@ -93,9 +103,10 @@ static const struct iio_info lpc32xx_adc_iio_info = {
>>  	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
>>  	.address = LPC32XXAD_IN * _index,		\
>>  	.scan_index = _index,				\
>> +	.scan_type.realbits = 10			\
>
> Given scan_type is only used in the core in buffered modes
> that this driver doesn't support this is a little 'unusual'.

I found it in other drivers and thought it was expected to store the bit
resolution here.

>
> Also, only one value, so why bother rather than just putting it
> in the one place it is used?

OK I will just use it in the lpc32xx_read_raw function then.

>
>>  }
>>  
>> -static const struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
>> +static struct iio_chan_spec lpc32xx_adc_iio_channels[] = {
> Please provide two const versions and pick between them rather than
> modifying the one.
>
> Clearly we might 'know' there is only ever one such ADC on these devices
> but it's not obvious to a reviewer who isn't familiar with the hardware,
> making this look like a bug.

OK

>
>>  	LPC32XX_ADC_CHANNEL(0),
>>  	LPC32XX_ADC_CHANNEL(1),
>>  	LPC32XX_ADC_CHANNEL(2),
>> @@ -119,7 +130,7 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>>  	struct resource *res;
>>  	int retval = -ENODEV;
>>  	struct iio_dev *iodev = NULL;
>> -	int irq;
>> +	int irq, i;
>>  
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	if (!res) {
>> @@ -159,6 +170,15 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>>  		return retval;
>>  	}
>>  
>> +	st->vref = devm_regulator_get(&pdev->dev, "vref");
>> +	if (IS_ERR(st->vref))
>> +		dev_err(&pdev->dev,
>> +			"Missing vref regulator: No scaling available\n");
>> +	else
>> +		for (i = 0; i <  ARRAY_SIZE(lpc32xx_adc_iio_channels); i++)
>> +			lpc32xx_adc_iio_channels[i].info_mask_shared_by_type =
>> +				BIT(IIO_CHAN_INFO_SCALE);
>> +
>>  	platform_set_drvdata(pdev, iodev);
>>  
>>  	init_completion(&st->completion);
>> @@ -175,7 +195,6 @@ static int lpc32xx_adc_probe(struct platform_device *pdev)
>>  		return retval;
>>  
>>  	dev_info(&pdev->dev, "LPC32XX ADC driver loaded, IRQ %d\n", irq);
>> -
> Unrelated (and in my view) bad whitespace change.

Indeed, it was a mistake.

Thanks,

Gregory

>>  	return 0;
>>  }
>>  
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iio:adc:lpc32xx Cleanup headers

[Gregory CLEMENT]

Hi Jonathan,
 
 On sam., févr. 09 2019, Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri,  8 Feb 2019 17:09:43 +0100
> Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
>
>> A few headers was useless: remove them, and also sort them in alphabetic
>> order.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Hmm. Given the headers in question are mostly only useless (I think)
> in the sense they are always included by something else, I'm
> not sure this patch is worth the churn.
>
> It's also tricky to see which ones were actually removed
> given the combination with sorting.
> I think it's just kernel.h, device.h, err.h all of which
> are used in various ways and often directly included.

Actually, as I needed to add a header, I wanted to sort the list to put
it in the right place and then some of the header looked superfluous
that why I remove some of them. For example there was no reason to use
slab.h or of.h.

>
> The other one is the iio/sysfs.h file.  That one we could
> do to eventually kill off entirely, so happy to see that
> one alone go.

I am nit sure to know what do you want with this patch.

Gregory


>
> Jonathan
>
>> ---
>>  drivers/iio/adc/lpc32xx_adc.c | 15 ++++-----------
>>  1 file changed, 4 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
>> index e361c1532a75..f391c1e10136 100644
>> --- a/drivers/iio/adc/lpc32xx_adc.c
>> +++ b/drivers/iio/adc/lpc32xx_adc.c
>> @@ -7,20 +7,13 @@
>>   *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
>>   */
>>  
>> -#include <linux/module.h>
>> -#include <linux/platform_device.h>
>> -#include <linux/interrupt.h>
>> -#include <linux/device.h>
>> -#include <linux/kernel.h>
>> -#include <linux/slab.h>
>> -#include <linux/io.h>
>>  #include <linux/clk.h>
>> -#include <linux/err.h>
>>  #include <linux/completion.h>
>> -#include <linux/of.h>
>> -
>>  #include <linux/iio/iio.h>
>> -#include <linux/iio/sysfs.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>>  
>>  /*
>>   * LPC32XX registers definitions
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Gregory CLEMENT]

Hi Vladimir,
 
 On sam., févr. 09 2019, Vladimir Zapolskiy <vz@mleia.com> wrote:

> Hi Gregory,
>
> On 02/08/2019 06:09 PM, Gregory CLEMENT wrote:
>> As most of the other ADC the lpc32xx one use a vref-supply property:
>> document it.
>> 
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>  Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
>>  1 file changed, 5 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
>> index b3629d3a9adf..3a1bc669bd51 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
>> +++ b/Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt
>> @@ -6,6 +6,10 @@ Required properties:
>>    region.
>>  - interrupts: The ADC interrupt
>>  
>> +Optional:
>> + - vref-supply: The regulator supply ADC reference voltage, optional
>> +   for legacy reason, but highly encouraging to us in new device tree
>> +
>
> here vref-supply voltage is 3.3v only, I think that might be a reason why
> the property is omitted.

VDD_AD is an external pin so in theroy it would be possible to use a
different voltage, but indeed the datasheet describe this pin as the
3.3V supply for ADC.

>
> My concern is that the documentation shall not contain sections with
> description of properties found in the past for 'legacy reasons', if you
> like to support the backward compatibility in the driver, please update
> the documentation to the expected state and add a stub to care about
> legacy DTBs in the driver only.

I saw a lot of reference about legacy properties in the binding, and I
found it quite usefull when I read old device trees.

>
> FWIW I personally don't mind about breaking the backward compatibility
> for NXP LPC32xx, it might be a concern when CONFIG_ARM_ATAG_DTB_COMPAT=y
> is removed from the lpc32xx_defconfig though.
>
>>  Example:
>>  
>>  	adc@40048000 {
>> @@ -13,4 +17,5 @@ Example:
>>  		reg = <0x40048000 0x1000>;
>>  		interrupt-parent = <&mic>;
>>  		interrupts = <39 0>;
>> +		vref-supply = <&vcc>;
>>  	};
>> 
>
> I kindly ask you to include me to To: list, if you send another changes for
> NXP LPC32xx platform.

Yes sure! I wrongly only use the first lines provided by
./scripts/get_maintainer.pl, I will include you in the next version.


Thanks,

Gregory



>
> Thank you.
>
> --
> Best wishes,
> Vladimir
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] iio:adc:lpc32xx Cleanup headers

[Jonathan Cameron]

On Fri, 15 Feb 2019 16:42:55 +0100
Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hi Jonathan,
>  
>  On sam., févr. 09 2019, Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri,  8 Feb 2019 17:09:43 +0100
> > Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> >  
> >> A few headers was useless: remove them, and also sort them in alphabetic
> >> order.
> >> 
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>  
> > Hmm. Given the headers in question are mostly only useless (I think)
> > in the sense they are always included by something else, I'm
> > not sure this patch is worth the churn.
> >
> > It's also tricky to see which ones were actually removed
> > given the combination with sorting.
> > I think it's just kernel.h, device.h, err.h all of which
> > are used in various ways and often directly included.  
> 
> Actually, as I needed to add a header, I wanted to sort the list to put
> it in the right place and then some of the header looked superfluous
> that why I remove some of them. For example there was no reason to use
> slab.h or of.h.
Agreed on those two. I missed them probably because of the re organization
being combined with them.  No specific allocations except
via interfaces from elsewhere and no direct use of the devicetree
stuff, so fine to drop them.

> 
> >
> > The other one is the iio/sysfs.h file.  That one we could
> > do to eventually kill off entirely, so happy to see that
> > one alone go.  
> 
> I am nit sure to know what do you want with this patch.

Break it in two.  Drop the unwanted ones first, then reorganize.
That way the diff is easy to read.  Clearly I missed some
changes on my first read as it stands!

Jonathan

> 
> Gregory
> 
> 
> >
> > Jonathan
> >  
> >> ---
> >>  drivers/iio/adc/lpc32xx_adc.c | 15 ++++-----------
> >>  1 file changed, 4 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/iio/adc/lpc32xx_adc.c b/drivers/iio/adc/lpc32xx_adc.c
> >> index e361c1532a75..f391c1e10136 100644
> >> --- a/drivers/iio/adc/lpc32xx_adc.c
> >> +++ b/drivers/iio/adc/lpc32xx_adc.c
> >> @@ -7,20 +7,13 @@
> >>   *  Copyright (C) 2011, 2012 Roland Stigge <stigge@antcom.de>
> >>   */
> >>  
> >> -#include <linux/module.h>
> >> -#include <linux/platform_device.h>
> >> -#include <linux/interrupt.h>
> >> -#include <linux/device.h>
> >> -#include <linux/kernel.h>
> >> -#include <linux/slab.h>
> >> -#include <linux/io.h>
> >>  #include <linux/clk.h>
> >> -#include <linux/err.h>
> >>  #include <linux/completion.h>
> >> -#include <linux/of.h>
> >> -
> >>  #include <linux/iio/iio.h>
> >> -#include <linux/iio/sysfs.h>
> >> +#include <linux/interrupt.h>
> >> +#include <linux/io.h>
> >> +#include <linux/module.h>
> >> +#include <linux/platform_device.h>
> >>  
> >>  /*
> >>   * LPC32XX registers definitions  
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Sylvain Lemieux]

Hi Gregory,

On Fri, Feb 15, 2019 at 11:07 AM Gregory CLEMENT
<gregory.clement@bootlin.com> wrote:
>
> Hi Vladimir,
>
>  On sam., févr. 09 2019, Vladimir Zapolskiy <vz@mleia.com> wrote:
>
> > Hi Gregory,
> >
> > On 02/08/2019 06:09 PM, Gregory CLEMENT wrote:
> >> As most of the other ADC the lpc32xx one use a vref-supply property:
> >> document it.
> >>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> >> ---
> >>  Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
[...]
> >>
> >
> > I kindly ask you to include me to To: list, if you send another changes for
> > NXP LPC32xx platform.
>
> Yes sure! I wrongly only use the first lines provided by
> ./scripts/get_maintainer.pl, I will include you in the next version.
>

Please also include myself in the next LPC32xx related patches.

Regards,
Sylvain
>
> Thanks,
>
> Gregory
>
>
>
> >
> > Thank you.
> >
> > --
> > Best wishes,
> > Vladimir
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> --
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] dt-bindings: iio: adc: lpc32xx-adc: Document vref-supply

[Rob Herring]

On Fri,  8 Feb 2019 17:09:41 +0100, Gregory CLEMENT wrote:
> As most of the other ADC the lpc32xx one use a vref-supply property:
> document it.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  Documentation/devicetree/bindings/iio/adc/lpc32xx-adc.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel