[PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Artur Rojek]

Hi all,

v8 of this patchset introduces some structural changes, which I deemed
worthy highlighting here:

 - adc-joystick related changes have been dropped from this patchset and
   will be upstreamed separately. Their only connection to this patchset
   was that they used INGENIC_ADC_TOUCH_* defines in the DTS example,
   causing trouble to Rob's scripts.

 - Integrated Paul's changes, which introduce an ADCMD low-level command
   feature. These changes affect patches 5/6 and 6/6, with the former
   requiring Rob to re-ack.

Cheers,
Artur

Artur Rojek (5):
  dt-bindings: iio/adc: Convert ingenic-adc docs to YAML.
  IIO: Ingenic JZ47xx: Error check clk_enable calls.
  IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx
  dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC
  IIO: Ingenic JZ47xx: Add touchscreen mode.

Paul Cercueil (1):
  iio/adc: ingenic: Retrieve channels list from soc data struct

 .../bindings/iio/adc/ingenic,adc.txt          |  49 ---
 .../bindings/iio/adc/ingenic,adc.yaml         |  71 ++++
 drivers/iio/adc/Kconfig                       |   1 +
 drivers/iio/adc/ingenic-adc.c                 | 386 ++++++++++++++++--
 include/dt-bindings/iio/adc/ingenic,adc.h     |   6 +
 5 files changed, 426 insertions(+), 87 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml

-- 
2.27.0

[PATCH v8 1/6] dt-bindings: iio/adc: Convert ingenic-adc docs to YAML.

[Artur Rojek]

Convert the textual documentation of Device Tree bindings for the
Ingenic JZ47xx SoCs ADC controller to YAML.

The `interrupts` property is now explicitly listed and marked as
required. While missing from the previous textual documentation, this
property has been used with all the boards which probe this driver.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Reviewed-by: Rob Herring <robh@kernel.org>
---

 Changes:

 v6: new patch

 v7: - specify `maxItems: 1` for single entry properties
     - get rid of redundant descriptions of said properties

 v8: no change

 .../bindings/iio/adc/ingenic,adc.txt          | 49 -------------
 .../bindings/iio/adc/ingenic,adc.yaml         | 71 +++++++++++++++++++
 2 files changed, 71 insertions(+), 49 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
deleted file mode 100644
index cd9048cf9dcf..000000000000
--- a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
+++ /dev/null
@@ -1,49 +0,0 @@
-* Ingenic JZ47xx ADC controller IIO bindings
-
-Required properties:
-
-- compatible: Should be one of:
-  * ingenic,jz4725b-adc
-  * ingenic,jz4740-adc
-  * ingenic,jz4770-adc
-- reg: ADC controller registers location and length.
-- clocks: phandle to the SoC's ADC clock.
-- clock-names: Must be set to "adc".
-- #io-channel-cells: Must be set to <1> to indicate channels are selected
-  by index.
-
-ADC clients must use the format described in iio-bindings.txt, giving
-a phandle and IIO specifier pair ("io-channels") to the ADC controller.
-
-Example:
-
-#include <dt-bindings/iio/adc/ingenic,adc.h>
-
-adc: adc@10070000 {
-	compatible = "ingenic,jz4740-adc";
-	#io-channel-cells = <1>;
-
-	reg = <0x10070000 0x30>;
-
-	clocks = <&cgu JZ4740_CLK_ADC>;
-	clock-names = "adc";
-
-	interrupt-parent = <&intc>;
-	interrupts = <18>;
-};
-
-adc-keys {
-	...
-	compatible = "adc-keys";
-	io-channels = <&adc INGENIC_ADC_AUX>;
-	io-channel-names = "buttons";
-	...
-};
-
-battery {
-	...
-	compatible = "ingenic,jz4740-battery";
-	io-channels = <&adc INGENIC_ADC_BATTERY>;
-	io-channel-names = "battery";
-	...
-};
diff --git a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
new file mode 100644
index 000000000000..9f414dbdae86
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019-2020 Artur Rojek
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/iio/adc/ingenic,adc.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: Ingenic JZ47xx ADC controller IIO bindings
+
+maintainers:
+  - Artur Rojek <contact@artur-rojek.eu>
+
+description: >
+  Industrial I/O subsystem bindings for ADC controller found in
+  Ingenic JZ47xx SoCs.
+
+  ADC clients must use the format described in iio-bindings.txt, giving
+  a phandle and IIO specifier pair ("io-channels") to the ADC controller.
+
+properties:
+  compatible:
+    enum:
+      - ingenic,jz4725b-adc
+      - ingenic,jz4740-adc
+      - ingenic,jz4770-adc
+
+  '#io-channel-cells':
+    const: 1
+    description:
+      Must be set to <1> to indicate channels are selected by index.
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: adc
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#io-channel-cells'
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/jz4740-cgu.h>
+    #include <dt-bindings/iio/adc/ingenic,adc.h>
+
+    adc@10070000 {
+            compatible = "ingenic,jz4740-adc";
+            #io-channel-cells = <1>;
+
+            reg = <0x10070000 0x30>;
+
+            clocks = <&cgu JZ4740_CLK_ADC>;
+            clock-names = "adc";
+
+            interrupt-parent = <&intc>;
+            interrupts = <18>;
+    };
-- 
2.27.0

Re: [PATCH v8 1/6] dt-bindings: iio/adc: Convert ingenic-adc docs to YAML.

[Jonathan Cameron]

On Thu,  9 Jul 2020 17:21:55 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> Convert the textual documentation of Device Tree bindings for the
> Ingenic JZ47xx SoCs ADC controller to YAML.
> 
> The `interrupts` property is now explicitly listed and marked as
> required. While missing from the previous textual documentation, this
> property has been used with all the boards which probe this driver.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> Reviewed-by: Rob Herring <robh@kernel.org>

Given this is a good change on it's own. I'll apply it before even
reading the rest of the series.  If we do need to go to a v9 at least
it'll be shorter ;) 

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

Thanks,

Jonathan

> ---
> 
>  Changes:
> 
>  v6: new patch
> 
>  v7: - specify `maxItems: 1` for single entry properties
>      - get rid of redundant descriptions of said properties
> 
>  v8: no change
> 
>  .../bindings/iio/adc/ingenic,adc.txt          | 49 -------------
>  .../bindings/iio/adc/ingenic,adc.yaml         | 71 +++++++++++++++++++
>  2 files changed, 71 insertions(+), 49 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
> deleted file mode 100644
> index cd9048cf9dcf..000000000000
> --- a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -* Ingenic JZ47xx ADC controller IIO bindings
> -
> -Required properties:
> -
> -- compatible: Should be one of:
> -  * ingenic,jz4725b-adc
> -  * ingenic,jz4740-adc
> -  * ingenic,jz4770-adc
> -- reg: ADC controller registers location and length.
> -- clocks: phandle to the SoC's ADC clock.
> -- clock-names: Must be set to "adc".
> -- #io-channel-cells: Must be set to <1> to indicate channels are selected
> -  by index.
> -
> -ADC clients must use the format described in iio-bindings.txt, giving
> -a phandle and IIO specifier pair ("io-channels") to the ADC controller.
> -
> -Example:
> -
> -#include <dt-bindings/iio/adc/ingenic,adc.h>
> -
> -adc: adc@10070000 {
> -	compatible = "ingenic,jz4740-adc";
> -	#io-channel-cells = <1>;
> -
> -	reg = <0x10070000 0x30>;
> -
> -	clocks = <&cgu JZ4740_CLK_ADC>;
> -	clock-names = "adc";
> -
> -	interrupt-parent = <&intc>;
> -	interrupts = <18>;
> -};
> -
> -adc-keys {
> -	...
> -	compatible = "adc-keys";
> -	io-channels = <&adc INGENIC_ADC_AUX>;
> -	io-channel-names = "buttons";
> -	...
> -};
> -
> -battery {
> -	...
> -	compatible = "ingenic,jz4740-battery";
> -	io-channels = <&adc INGENIC_ADC_BATTERY>;
> -	io-channel-names = "battery";
> -	...
> -};
> diff --git a/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
> new file mode 100644
> index 000000000000..9f414dbdae86
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2019-2020 Artur Rojek
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/iio/adc/ingenic,adc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Ingenic JZ47xx ADC controller IIO bindings
> +
> +maintainers:
> +  - Artur Rojek <contact@artur-rojek.eu>
> +
> +description: >
> +  Industrial I/O subsystem bindings for ADC controller found in
> +  Ingenic JZ47xx SoCs.
> +
> +  ADC clients must use the format described in iio-bindings.txt, giving
> +  a phandle and IIO specifier pair ("io-channels") to the ADC controller.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ingenic,jz4725b-adc
> +      - ingenic,jz4740-adc
> +      - ingenic,jz4770-adc
> +
> +  '#io-channel-cells':
> +    const: 1
> +    description:
> +      Must be set to <1> to indicate channels are selected by index.
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +  clock-names:
> +    items:
> +      - const: adc
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - '#io-channel-cells'
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/jz4740-cgu.h>
> +    #include <dt-bindings/iio/adc/ingenic,adc.h>
> +
> +    adc@10070000 {
> +            compatible = "ingenic,jz4740-adc";
> +            #io-channel-cells = <1>;
> +
> +            reg = <0x10070000 0x30>;
> +
> +            clocks = <&cgu JZ4740_CLK_ADC>;
> +            clock-names = "adc";
> +
> +            interrupt-parent = <&intc>;
> +            interrupts = <18>;
> +    };

[PATCH v8 2/6] IIO: Ingenic JZ47xx: Error check clk_enable calls.

[Artur Rojek]

Introduce error checks for the clk_enable calls used in this driver.
As part of the changes, move clk_enable/clk_disable calls out of
ingenic_adc_set_config and into respective logic of its callers.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---

 Changes:

 v6: new patch

 v7: no change

 v8: move `clk_disable` outside the lock

 drivers/iio/adc/ingenic-adc.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 39c0a609fc94..c1946a9f1cca 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -73,7 +73,6 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
 {
 	uint32_t cfg;
 
-	clk_enable(adc->clk);
 	mutex_lock(&adc->lock);
 
 	cfg = readl(adc->base + JZ_ADC_REG_CFG) & ~mask;
@@ -81,7 +80,6 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
 	writel(cfg, adc->base + JZ_ADC_REG_CFG);
 
 	mutex_unlock(&adc->lock);
-	clk_disable(adc->clk);
 }
 
 static void ingenic_adc_enable(struct ingenic_adc *adc,
@@ -124,6 +122,8 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
 				 long m)
 {
 	struct ingenic_adc *adc = iio_priv(iio_dev);
+	struct device *dev = iio_dev->dev.parent;
+	int ret;
 
 	switch (m) {
 	case IIO_CHAN_INFO_SCALE:
@@ -131,6 +131,14 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
 		case INGENIC_ADC_BATTERY:
 			if (!adc->soc_data->battery_vref_mode)
 				return -EINVAL;
+
+			ret = clk_enable(adc->clk);
+			if (ret) {
+				dev_err(dev, "Failed to enable clock: %d\n",
+					ret);
+				return ret;
+			}
+
 			if (val > JZ_ADC_BATTERY_LOW_VREF) {
 				ingenic_adc_set_config(adc,
 						       JZ_ADC_REG_CFG_BAT_MD,
@@ -142,6 +150,9 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
 						       JZ_ADC_REG_CFG_BAT_MD);
 				adc->low_vref_mode = true;
 			}
+
+			clk_disable(adc->clk);
+
 			return 0;
 		default:
 			return -EINVAL;
@@ -317,6 +328,13 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
 					  int *val)
 {
 	int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
+	struct device *dev = iio_priv_to_dev(adc)->dev.parent;
+
+	ret = clk_enable(adc->clk);
+	if (ret) {
+		dev_err(dev, "Failed to enable clock: %d\n", ret);
+		return ret;
+	}
 
 	/* We cannot sample AUX/AUX2 in parallel. */
 	mutex_lock(&adc->aux_lock);
@@ -325,7 +343,6 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
 		ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_AUX_MD, bit);
 	}
 
-	clk_enable(adc->clk);
 	ret = ingenic_adc_capture(adc, engine);
 	if (ret)
 		goto out;
@@ -342,8 +359,8 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
 
 	ret = IIO_VAL_INT;
 out:
-	clk_disable(adc->clk);
 	mutex_unlock(&adc->aux_lock);
+	clk_disable(adc->clk);
 
 	return ret;
 }
-- 
2.27.0

Re: [PATCH v8 2/6] IIO: Ingenic JZ47xx: Error check clk_enable calls.

[Jonathan Cameron]

On Thu,  9 Jul 2020 17:21:56 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> Introduce error checks for the clk_enable calls used in this driver.
> As part of the changes, move clk_enable/clk_disable calls out of
> ingenic_adc_set_config and into respective logic of its callers.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
Applied.

Thanks,

Jonathan

> ---
> 
>  Changes:
> 
>  v6: new patch
> 
>  v7: no change
> 
>  v8: move `clk_disable` outside the lock
> 
>  drivers/iio/adc/ingenic-adc.c | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index 39c0a609fc94..c1946a9f1cca 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -73,7 +73,6 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
>  {
>  	uint32_t cfg;
>  
> -	clk_enable(adc->clk);
>  	mutex_lock(&adc->lock);
>  
>  	cfg = readl(adc->base + JZ_ADC_REG_CFG) & ~mask;
> @@ -81,7 +80,6 @@ static void ingenic_adc_set_config(struct ingenic_adc *adc,
>  	writel(cfg, adc->base + JZ_ADC_REG_CFG);
>  
>  	mutex_unlock(&adc->lock);
> -	clk_disable(adc->clk);
>  }
>  
>  static void ingenic_adc_enable(struct ingenic_adc *adc,
> @@ -124,6 +122,8 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
>  				 long m)
>  {
>  	struct ingenic_adc *adc = iio_priv(iio_dev);
> +	struct device *dev = iio_dev->dev.parent;
> +	int ret;
>  
>  	switch (m) {
>  	case IIO_CHAN_INFO_SCALE:
> @@ -131,6 +131,14 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
>  		case INGENIC_ADC_BATTERY:
>  			if (!adc->soc_data->battery_vref_mode)
>  				return -EINVAL;
> +
> +			ret = clk_enable(adc->clk);
> +			if (ret) {
> +				dev_err(dev, "Failed to enable clock: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
>  			if (val > JZ_ADC_BATTERY_LOW_VREF) {
>  				ingenic_adc_set_config(adc,
>  						       JZ_ADC_REG_CFG_BAT_MD,
> @@ -142,6 +150,9 @@ static int ingenic_adc_write_raw(struct iio_dev *iio_dev,
>  						       JZ_ADC_REG_CFG_BAT_MD);
>  				adc->low_vref_mode = true;
>  			}
> +
> +			clk_disable(adc->clk);
> +
>  			return 0;
>  		default:
>  			return -EINVAL;
> @@ -317,6 +328,13 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
>  					  int *val)
>  {
>  	int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
> +	struct device *dev = iio_priv_to_dev(adc)->dev.parent;
> +
> +	ret = clk_enable(adc->clk);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable clock: %d\n", ret);
> +		return ret;
> +	}
>  
>  	/* We cannot sample AUX/AUX2 in parallel. */
>  	mutex_lock(&adc->aux_lock);
> @@ -325,7 +343,6 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
>  		ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_AUX_MD, bit);
>  	}
>  
> -	clk_enable(adc->clk);
>  	ret = ingenic_adc_capture(adc, engine);
>  	if (ret)
>  		goto out;
> @@ -342,8 +359,8 @@ static int ingenic_adc_read_chan_info_raw(struct ingenic_adc *adc,
>  
>  	ret = IIO_VAL_INT;
>  out:
> -	clk_disable(adc->clk);
>  	mutex_unlock(&adc->aux_lock);
> +	clk_disable(adc->clk);
>  
>  	return ret;
>  }

Re: [PATCH v8 2/6] IIO: Ingenic JZ47xx: Error check clk_enable calls.

[Ardelean, Alexandru]

On Sun, 2020-07-12 at 13:02 +0100, Jonathan Cameron wrote:
> On Thu,  9 Jul 2020 17:21:56 +0200
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
> > Introduce error checks for the clk_enable calls used in this driver.
> > As part of the changes, move clk_enable/clk_disable calls out of
> > ingenic_adc_set_config and into respective logic of its callers.
> > 
> > Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> > Tested-by: Paul Cercueil <paul@crapouillou.net>
> Applied.
> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > 
> >  Changes:
> > 
> >  v6: new patch
> > 
> >  v7: no change
> > 
> >  v8: move `clk_disable` outside the lock
> > 
> >  drivers/iio/adc/ingenic-adc.c | 25 +++++++++++++++++++++----
> >  1 file changed, 21 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-
> > adc.c
> > index 39c0a609fc94..c1946a9f1cca 100644
> > --- a/drivers/iio/adc/ingenic-adc.c
> > +++ b/drivers/iio/adc/ingenic-adc.c
> > @@ -73,7 +73,6 @@ static void ingenic_adc_set_config(struct ingenic_adc
> > *adc,
> >  {
> >  	uint32_t cfg;
> >  
> > -	clk_enable(adc->clk);
> >  	mutex_lock(&adc->lock);
> >  
> >  	cfg = readl(adc->base + JZ_ADC_REG_CFG) & ~mask;
> > @@ -81,7 +80,6 @@ static void ingenic_adc_set_config(struct ingenic_adc
> > *adc,
> >  	writel(cfg, adc->base + JZ_ADC_REG_CFG);
> >  
> >  	mutex_unlock(&adc->lock);
> > -	clk_disable(adc->clk);
> >  }
> >  
> >  static void ingenic_adc_enable(struct ingenic_adc *adc,
> > @@ -124,6 +122,8 @@ static int ingenic_adc_write_raw(struct iio_dev
> > *iio_dev,
> >  				 long m)
> >  {
> >  	struct ingenic_adc *adc = iio_priv(iio_dev);
> > +	struct device *dev = iio_dev->dev.parent;
> > +	int ret;
> >  
> >  	switch (m) {
> >  	case IIO_CHAN_INFO_SCALE:
> > @@ -131,6 +131,14 @@ static int ingenic_adc_write_raw(struct iio_dev
> > *iio_dev,
> >  		case INGENIC_ADC_BATTERY:
> >  			if (!adc->soc_data->battery_vref_mode)
> >  				return -EINVAL;
> > +
> > +			ret = clk_enable(adc->clk);
> > +			if (ret) {
> > +				dev_err(dev, "Failed to enable clock:
> > %d\n",
> > +					ret);
> > +				return ret;
> > +			}
> > +
> >  			if (val > JZ_ADC_BATTERY_LOW_VREF) {
> >  				ingenic_adc_set_config(adc,
> >  						       JZ_ADC_REG_CFG_BAT_M
> > D,
> > @@ -142,6 +150,9 @@ static int ingenic_adc_write_raw(struct iio_dev
> > *iio_dev,
> >  						       JZ_ADC_REG_CFG_BAT_M
> > D);
> >  				adc->low_vref_mode = true;
> >  			}
> > +
> > +			clk_disable(adc->clk);
> > +
> >  			return 0;
> >  		default:
> >  			return -EINVAL;
> > @@ -317,6 +328,13 @@ static int ingenic_adc_read_chan_info_raw(struct
> > ingenic_adc *adc,
> >  					  int *val)
> >  {
> >  	int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
> > +	struct device *dev = iio_priv_to_dev(adc)->dev.parent;

This uses iio_priv_to_dev(), which should be removed [if it hasn't been
already].
Also, with the iio_dev_opaque struct/type, using the iio_priv_to_dev()
helper shouldn't work, or should give undefined behavior, as the offset to
the private data should be matched against iio_dev_opaque (and not
iio_dev).

Looking at this code, it looks like ingenic_adc_read_chan_info_raw() can
take an 'indio_dev' reference and obtain the private information via
iio_priv()

Maybe [for some] there's a question as to why iio_priv_to_dev() is being
removed.
The answer is: because the iio_dev_opaque struct/type was introduced to
hide some iio_dev [INTERN] fields; the main reason for hiding them [aside
from good coding practice, and because they belong to the IIO framework] is
to not have to review new drivers being copied/adapted from old drivers.

Naturally, the iio_priv_to_dev() could have been kept around [in a reworked
form], but it didn't make much sense, since most users of iio_priv_to_dev()
could access a reference to indio_dev without much/any hassle.

> > +
> > +	ret = clk_enable(adc->clk);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to enable clock: %d\n", ret);
> > +		return ret;
> > +	}
> >  
> >  	/* We cannot sample AUX/AUX2 in parallel. */
> >  	mutex_lock(&adc->aux_lock);
> > @@ -325,7 +343,6 @@ static int ingenic_adc_read_chan_info_raw(struct
> > ingenic_adc *adc,
> >  		ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_AUX_MD, bit);
> >  	}
> >  
> > -	clk_enable(adc->clk);
> >  	ret = ingenic_adc_capture(adc, engine);
> >  	if (ret)
> >  		goto out;
> > @@ -342,8 +359,8 @@ static int ingenic_adc_read_chan_info_raw(struct
> > ingenic_adc *adc,
> >  
> >  	ret = IIO_VAL_INT;
> >  out:
> > -	clk_disable(adc->clk);
> >  	mutex_unlock(&adc->aux_lock);
> > +	clk_disable(adc->clk);
> >  
> >  	return ret;
> >  }

Re: [PATCH v8 2/6] IIO: Ingenic JZ47xx: Error check clk_enable calls.

[Jonathan Cameron]

On Mon, 13 Jul 2020 05:07:44 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-07-12 at 13:02 +0100, Jonathan Cameron wrote:
> > On Thu,  9 Jul 2020 17:21:56 +0200
> > Artur Rojek <contact@artur-rojek.eu> wrote:
> >   
> > > Introduce error checks for the clk_enable calls used in this driver.
> > > As part of the changes, move clk_enable/clk_disable calls out of
> > > ingenic_adc_set_config and into respective logic of its callers.
> > > 
> > > Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> > > Tested-by: Paul Cercueil <paul@crapouillou.net>  
> > Applied.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > ---
> > > 
> > >  Changes:
> > > 
> > >  v6: new patch
> > > 
> > >  v7: no change
> > > 
> > >  v8: move `clk_disable` outside the lock
> > > 
> > >  drivers/iio/adc/ingenic-adc.c | 25 +++++++++++++++++++++----
> > >  1 file changed, 21 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-
> > > adc.c
> > > index 39c0a609fc94..c1946a9f1cca 100644
> > > --- a/drivers/iio/adc/ingenic-adc.c
> > > +++ b/drivers/iio/adc/ingenic-adc.c
> > > @@ -73,7 +73,6 @@ static void ingenic_adc_set_config(struct ingenic_adc
> > > *adc,
> > >  {
> > >  	uint32_t cfg;
> > >  
> > > -	clk_enable(adc->clk);
> > >  	mutex_lock(&adc->lock);
> > >  
> > >  	cfg = readl(adc->base + JZ_ADC_REG_CFG) & ~mask;
> > > @@ -81,7 +80,6 @@ static void ingenic_adc_set_config(struct ingenic_adc
> > > *adc,
> > >  	writel(cfg, adc->base + JZ_ADC_REG_CFG);
> > >  
> > >  	mutex_unlock(&adc->lock);
> > > -	clk_disable(adc->clk);
> > >  }
> > >  
> > >  static void ingenic_adc_enable(struct ingenic_adc *adc,
> > > @@ -124,6 +122,8 @@ static int ingenic_adc_write_raw(struct iio_dev
> > > *iio_dev,
> > >  				 long m)
> > >  {
> > >  	struct ingenic_adc *adc = iio_priv(iio_dev);
> > > +	struct device *dev = iio_dev->dev.parent;
> > > +	int ret;
> > >  
> > >  	switch (m) {
> > >  	case IIO_CHAN_INFO_SCALE:
> > > @@ -131,6 +131,14 @@ static int ingenic_adc_write_raw(struct iio_dev
> > > *iio_dev,
> > >  		case INGENIC_ADC_BATTERY:
> > >  			if (!adc->soc_data->battery_vref_mode)
> > >  				return -EINVAL;
> > > +
> > > +			ret = clk_enable(adc->clk);
> > > +			if (ret) {
> > > +				dev_err(dev, "Failed to enable clock:
> > > %d\n",
> > > +					ret);
> > > +				return ret;
> > > +			}
> > > +
> > >  			if (val > JZ_ADC_BATTERY_LOW_VREF) {
> > >  				ingenic_adc_set_config(adc,
> > >  						       JZ_ADC_REG_CFG_BAT_M
> > > D,
> > > @@ -142,6 +150,9 @@ static int ingenic_adc_write_raw(struct iio_dev
> > > *iio_dev,
> > >  						       JZ_ADC_REG_CFG_BAT_M
> > > D);
> > >  				adc->low_vref_mode = true;
> > >  			}
> > > +
> > > +			clk_disable(adc->clk);
> > > +
> > >  			return 0;
> > >  		default:
> > >  			return -EINVAL;
> > > @@ -317,6 +328,13 @@ static int ingenic_adc_read_chan_info_raw(struct
> > > ingenic_adc *adc,
> > >  					  int *val)
> > >  {
> > >  	int bit, ret, engine = (chan->channel == INGENIC_ADC_BATTERY);
> > > +	struct device *dev = iio_priv_to_dev(adc)->dev.parent;  
> 
> This uses iio_priv_to_dev(), which should be removed [if it hasn't been
> already].

Gah I missed that. 
For now best bet is probably if I drop this series from here on until we have
this tidied up in a v9.  Its my fault as I messed up and lost one patch in
the middle of Alex's series somehow, hence it is still there but broken in
the tree.

Thanks.

Jonathan

> Also, with the iio_dev_opaque struct/type, using the iio_priv_to_dev()
> helper shouldn't work, or should give undefined behavior, as the offset to
> the private data should be matched against iio_dev_opaque (and not
> iio_dev).
> 
> Looking at this code, it looks like ingenic_adc_read_chan_info_raw() can
> take an 'indio_dev' reference and obtain the private information via
> iio_priv()
> 
> Maybe [for some] there's a question as to why iio_priv_to_dev() is being
> removed.
> The answer is: because the iio_dev_opaque struct/type was introduced to
> hide some iio_dev [INTERN] fields; the main reason for hiding them [aside
> from good coding practice, and because they belong to the IIO framework] is
> to not have to review new drivers being copied/adapted from old drivers.
> 
> Naturally, the iio_priv_to_dev() could have been kept around [in a reworked
> form], but it didn't make much sense, since most users of iio_priv_to_dev()
> could access a reference to indio_dev without much/any hassle.
> 
> > > +
> > > +	ret = clk_enable(adc->clk);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to enable clock: %d\n", ret);
> > > +		return ret;
> > > +	}
> > >  
> > >  	/* We cannot sample AUX/AUX2 in parallel. */
> > >  	mutex_lock(&adc->aux_lock);
> > > @@ -325,7 +343,6 @@ static int ingenic_adc_read_chan_info_raw(struct
> > > ingenic_adc *adc,
> > >  		ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_AUX_MD, bit);
> > >  	}
> > >  
> > > -	clk_enable(adc->clk);
> > >  	ret = ingenic_adc_capture(adc, engine);
> > >  	if (ret)
> > >  		goto out;
> > > @@ -342,8 +359,8 @@ static int ingenic_adc_read_chan_info_raw(struct
> > > ingenic_adc *adc,
> > >  
> > >  	ret = IIO_VAL_INT;
> > >  out:
> > > -	clk_disable(adc->clk);
> > >  	mutex_unlock(&adc->aux_lock);
> > > +	clk_disable(adc->clk);
> > >  
> > >  	return ret;
> > >  }  

[PATCH v8 3/6] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx

[Artur Rojek]

Provide an of_xlate callback in order to retrieve the correct channel
specifier index from the IIO channels array.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---

 Changes:

 v2-v8: no change

 drivers/iio/adc/ingenic-adc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index c1946a9f1cca..89019fb59d48 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -400,6 +400,21 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev,
 	}
 }
 
+static int ingenic_adc_of_xlate(struct iio_dev *iio_dev,
+				const struct of_phandle_args *iiospec)
+{
+	int i;
+
+	if (!iiospec->args_count)
+		return -EINVAL;
+
+	for (i = 0; i < iio_dev->num_channels; ++i)
+		if (iio_dev->channels[i].channel == iiospec->args[0])
+			return i;
+
+	return -EINVAL;
+}
+
 static void ingenic_adc_clk_cleanup(void *data)
 {
 	clk_unprepare(data);
@@ -409,6 +424,7 @@ static const struct iio_info ingenic_adc_info = {
 	.write_raw = ingenic_adc_write_raw,
 	.read_raw = ingenic_adc_read_raw,
 	.read_avail = ingenic_adc_read_avail,
+	.of_xlate = ingenic_adc_of_xlate,
 };
 
 static const struct iio_chan_spec ingenic_channels[] = {
-- 
2.27.0

Re: [PATCH v8 3/6] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx

[Jonathan Cameron]

On Thu,  9 Jul 2020 17:21:57 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> Provide an of_xlate callback in order to retrieve the correct channel
> specifier index from the IIO channels array.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
Applied.  Thanks,

Jonathan

> ---
> 
>  Changes:
> 
>  v2-v8: no change
> 
>  drivers/iio/adc/ingenic-adc.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index c1946a9f1cca..89019fb59d48 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -400,6 +400,21 @@ static int ingenic_adc_read_raw(struct iio_dev *iio_dev,
>  	}
>  }
>  
> +static int ingenic_adc_of_xlate(struct iio_dev *iio_dev,
> +				const struct of_phandle_args *iiospec)
> +{
> +	int i;
> +
> +	if (!iiospec->args_count)
> +		return -EINVAL;
> +
> +	for (i = 0; i < iio_dev->num_channels; ++i)
> +		if (iio_dev->channels[i].channel == iiospec->args[0])
> +			return i;
> +
> +	return -EINVAL;
> +}
> +
>  static void ingenic_adc_clk_cleanup(void *data)
>  {
>  	clk_unprepare(data);
> @@ -409,6 +424,7 @@ static const struct iio_info ingenic_adc_info = {
>  	.write_raw = ingenic_adc_write_raw,
>  	.read_raw = ingenic_adc_read_raw,
>  	.read_avail = ingenic_adc_read_avail,
> +	.of_xlate = ingenic_adc_of_xlate,
>  };
>  
>  static const struct iio_chan_spec ingenic_channels[] = {

[PATCH v8 4/6] iio/adc: ingenic: Retrieve channels list from soc data struct

[Artur Rojek]

From: Paul Cercueil <paul@crapouillou.net>

Instead of having one array of struct iio_chan_spec for all SoCs, and
have some SoCs remove the last item of the array as they can't use it,
have each SoC define its array of supported channels.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---

 Changes:

 v8: new patch

 drivers/iio/adc/ingenic-adc.c | 99 +++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 34 deletions(-)

diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 89019fb59d48..0233a9055c86 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -55,6 +55,8 @@ struct ingenic_adc_soc_data {
 	size_t battery_scale_avail_size;
 	unsigned int battery_vref_mode: 1;
 	unsigned int has_aux2: 1;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
 	int (*init_clk_div)(struct device *dev, struct ingenic_adc *adc);
 };
 
@@ -262,6 +264,61 @@ static int jz4770_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)
 	return 0;
 }
 
+static const struct iio_chan_spec jz4740_channels[] = {
+	{
+		.extend_name = "aux",
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = INGENIC_ADC_AUX,
+		.scan_index = -1,
+	},
+	{
+		.extend_name = "battery",
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
+						BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = INGENIC_ADC_BATTERY,
+		.scan_index = -1,
+	},
+};
+
+static const struct iio_chan_spec jz4770_channels[] = {
+	{
+		.extend_name = "aux",
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = INGENIC_ADC_AUX,
+		.scan_index = -1,
+	},
+	{
+		.extend_name = "battery",
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
+						BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = INGENIC_ADC_BATTERY,
+		.scan_index = -1,
+	},
+	{
+		.extend_name = "aux2",
+		.type = IIO_VOLTAGE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.indexed = 1,
+		.channel = INGENIC_ADC_AUX2,
+		.scan_index = -1,
+	},
+};
+
 static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
 	.battery_high_vref = JZ4725B_ADC_BATTERY_HIGH_VREF,
 	.battery_high_vref_bits = JZ4725B_ADC_BATTERY_HIGH_VREF_BITS,
@@ -271,6 +328,8 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
 	.battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail),
 	.battery_vref_mode = true,
 	.has_aux2 = false,
+	.channels = jz4740_channels,
+	.num_channels = ARRAY_SIZE(jz4740_channels),
 	.init_clk_div = jz4725b_adc_init_clk_div,
 };
 
@@ -283,6 +342,8 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = {
 	.battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail),
 	.battery_vref_mode = true,
 	.has_aux2 = false,
+	.channels = jz4740_channels,
+	.num_channels = ARRAY_SIZE(jz4740_channels),
 	.init_clk_div = NULL, /* no ADCLK register on JZ4740 */
 };
 
@@ -295,6 +356,8 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {
 	.battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail),
 	.battery_vref_mode = false,
 	.has_aux2 = true,
+	.channels = jz4770_channels,
+	.num_channels = ARRAY_SIZE(jz4770_channels),
 	.init_clk_div = jz4770_adc_init_clk_div,
 };
 
@@ -427,35 +490,6 @@ static const struct iio_info ingenic_adc_info = {
 	.of_xlate = ingenic_adc_of_xlate,
 };
 
-static const struct iio_chan_spec ingenic_channels[] = {
-	{
-		.extend_name = "aux",
-		.type = IIO_VOLTAGE,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-				      BIT(IIO_CHAN_INFO_SCALE),
-		.indexed = 1,
-		.channel = INGENIC_ADC_AUX,
-	},
-	{
-		.extend_name = "battery",
-		.type = IIO_VOLTAGE,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-				      BIT(IIO_CHAN_INFO_SCALE),
-		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
-						BIT(IIO_CHAN_INFO_SCALE),
-		.indexed = 1,
-		.channel = INGENIC_ADC_BATTERY,
-	},
-	{ /* Must always be last in the array. */
-		.extend_name = "aux2",
-		.type = IIO_VOLTAGE,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-				      BIT(IIO_CHAN_INFO_SCALE),
-		.indexed = 1,
-		.channel = INGENIC_ADC_AUX2,
-	},
-};
-
 static int ingenic_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -517,11 +551,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
 	iio_dev->dev.parent = dev;
 	iio_dev->name = "jz-adc";
 	iio_dev->modes = INDIO_DIRECT_MODE;
-	iio_dev->channels = ingenic_channels;
-	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
-	/* Remove AUX2 from the list of supported channels. */
-	if (!adc->soc_data->has_aux2)
-		iio_dev->num_channels -= 1;
+	iio_dev->channels = soc_data->channels;
+	iio_dev->num_channels = soc_data->num_channels;
 	iio_dev->info = &ingenic_adc_info;
 
 	ret = devm_iio_device_register(dev, iio_dev);
-- 
2.27.0

Re: [PATCH v8 4/6] iio/adc: ingenic: Retrieve channels list from soc data struct

[Jonathan Cameron]

On Thu,  9 Jul 2020 17:21:58 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> From: Paul Cercueil <paul@crapouillou.net>
> 
> Instead of having one array of struct iio_chan_spec for all SoCs, and
> have some SoCs remove the last item of the array as they can't use it,
> have each SoC define its array of supported channels.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
Applied to the togreg branch of iio.git and pushed out as testing for
autobuilders to play with it.

Thanks,

Jonathan

> ---
> 
>  Changes:
> 
>  v8: new patch
> 
>  drivers/iio/adc/ingenic-adc.c | 99 +++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index 89019fb59d48..0233a9055c86 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -55,6 +55,8 @@ struct ingenic_adc_soc_data {
>  	size_t battery_scale_avail_size;
>  	unsigned int battery_vref_mode: 1;
>  	unsigned int has_aux2: 1;
> +	const struct iio_chan_spec *channels;
> +	unsigned int num_channels;
>  	int (*init_clk_div)(struct device *dev, struct ingenic_adc *adc);
>  };
>  
> @@ -262,6 +264,61 @@ static int jz4770_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)
>  	return 0;
>  }
>  
> +static const struct iio_chan_spec jz4740_channels[] = {
> +	{
> +		.extend_name = "aux",
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_AUX,
> +		.scan_index = -1,
> +	},
> +	{
> +		.extend_name = "battery",
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> +						BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_BATTERY,
> +		.scan_index = -1,
> +	},
> +};
> +
> +static const struct iio_chan_spec jz4770_channels[] = {
> +	{
> +		.extend_name = "aux",
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_AUX,
> +		.scan_index = -1,
> +	},
> +	{
> +		.extend_name = "battery",
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> +						BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_BATTERY,
> +		.scan_index = -1,
> +	},
> +	{
> +		.extend_name = "aux2",
> +		.type = IIO_VOLTAGE,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_AUX2,
> +		.scan_index = -1,
> +	},
> +};
> +
>  static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
>  	.battery_high_vref = JZ4725B_ADC_BATTERY_HIGH_VREF,
>  	.battery_high_vref_bits = JZ4725B_ADC_BATTERY_HIGH_VREF_BITS,
> @@ -271,6 +328,8 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
>  	.battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail),
>  	.battery_vref_mode = true,
>  	.has_aux2 = false,
> +	.channels = jz4740_channels,
> +	.num_channels = ARRAY_SIZE(jz4740_channels),
>  	.init_clk_div = jz4725b_adc_init_clk_div,
>  };
>  
> @@ -283,6 +342,8 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = {
>  	.battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail),
>  	.battery_vref_mode = true,
>  	.has_aux2 = false,
> +	.channels = jz4740_channels,
> +	.num_channels = ARRAY_SIZE(jz4740_channels),
>  	.init_clk_div = NULL, /* no ADCLK register on JZ4740 */
>  };
>  
> @@ -295,6 +356,8 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {
>  	.battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail),
>  	.battery_vref_mode = false,
>  	.has_aux2 = true,
> +	.channels = jz4770_channels,
> +	.num_channels = ARRAY_SIZE(jz4770_channels),
>  	.init_clk_div = jz4770_adc_init_clk_div,
>  };
>  
> @@ -427,35 +490,6 @@ static const struct iio_info ingenic_adc_info = {
>  	.of_xlate = ingenic_adc_of_xlate,
>  };
>  
> -static const struct iio_chan_spec ingenic_channels[] = {
> -	{
> -		.extend_name = "aux",
> -		.type = IIO_VOLTAGE,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -				      BIT(IIO_CHAN_INFO_SCALE),
> -		.indexed = 1,
> -		.channel = INGENIC_ADC_AUX,
> -	},
> -	{
> -		.extend_name = "battery",
> -		.type = IIO_VOLTAGE,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -				      BIT(IIO_CHAN_INFO_SCALE),
> -		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> -						BIT(IIO_CHAN_INFO_SCALE),
> -		.indexed = 1,
> -		.channel = INGENIC_ADC_BATTERY,
> -	},
> -	{ /* Must always be last in the array. */
> -		.extend_name = "aux2",
> -		.type = IIO_VOLTAGE,
> -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -				      BIT(IIO_CHAN_INFO_SCALE),
> -		.indexed = 1,
> -		.channel = INGENIC_ADC_AUX2,
> -	},
> -};
> -
>  static int ingenic_adc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -517,11 +551,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
>  	iio_dev->dev.parent = dev;
>  	iio_dev->name = "jz-adc";
>  	iio_dev->modes = INDIO_DIRECT_MODE;
> -	iio_dev->channels = ingenic_channels;
> -	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
> -	/* Remove AUX2 from the list of supported channels. */
> -	if (!adc->soc_data->has_aux2)
> -		iio_dev->num_channels -= 1;
> +	iio_dev->channels = soc_data->channels;
> +	iio_dev->num_channels = soc_data->num_channels;
>  	iio_dev->info = &ingenic_adc_info;
>  
>  	ret = devm_iio_device_register(dev, iio_dev);

Re: [PATCH v8 4/6] iio/adc: ingenic: Retrieve channels list from soc data struct

[Jonathan Cameron]

On Sun, 12 Jul 2020 13:07:13 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Thu,  9 Jul 2020 17:21:58 +0200
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
> > From: Paul Cercueil <paul@crapouillou.net>
> > 
> > Instead of having one array of struct iio_chan_spec for all SoCs, and
> > have some SoCs remove the last item of the array as they can't use it,
> > have each SoC define its array of supported channels.
> > 
> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > Tested-by: Artur Rojek <contact@artur-rojek.eu>  
> Applied to the togreg branch of iio.git and pushed out as testing for
> autobuilders to play with it.
Actually removed from tree again.

Artur, as you sent this patch on to me, you need to sign-off on it
so we maintain the DCO related records of how this got to the kernel.
You can just reply here to give it.

Thanks,

Jonathan

> 
> Thanks,
> 
> Jonathan
> 
> > ---
> > 
> >  Changes:
> > 
> >  v8: new patch
> > 
> >  drivers/iio/adc/ingenic-adc.c | 99 +++++++++++++++++++++++------------
> >  1 file changed, 65 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> > index 89019fb59d48..0233a9055c86 100644
> > --- a/drivers/iio/adc/ingenic-adc.c
> > +++ b/drivers/iio/adc/ingenic-adc.c
> > @@ -55,6 +55,8 @@ struct ingenic_adc_soc_data {
> >  	size_t battery_scale_avail_size;
> >  	unsigned int battery_vref_mode: 1;
> >  	unsigned int has_aux2: 1;
> > +	const struct iio_chan_spec *channels;
> > +	unsigned int num_channels;
> >  	int (*init_clk_div)(struct device *dev, struct ingenic_adc *adc);
> >  };
> >  
> > @@ -262,6 +264,61 @@ static int jz4770_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)
> >  	return 0;
> >  }
> >  
> > +static const struct iio_chan_spec jz4740_channels[] = {
> > +	{
> > +		.extend_name = "aux",
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = INGENIC_ADC_AUX,
> > +		.scan_index = -1,
> > +	},
> > +	{
> > +		.extend_name = "battery",
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> > +						BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = INGENIC_ADC_BATTERY,
> > +		.scan_index = -1,
> > +	},
> > +};
> > +
> > +static const struct iio_chan_spec jz4770_channels[] = {
> > +	{
> > +		.extend_name = "aux",
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = INGENIC_ADC_AUX,
> > +		.scan_index = -1,
> > +	},
> > +	{
> > +		.extend_name = "battery",
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> > +						BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = INGENIC_ADC_BATTERY,
> > +		.scan_index = -1,
> > +	},
> > +	{
> > +		.extend_name = "aux2",
> > +		.type = IIO_VOLTAGE,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.indexed = 1,
> > +		.channel = INGENIC_ADC_AUX2,
> > +		.scan_index = -1,
> > +	},
> > +};
> > +
> >  static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
> >  	.battery_high_vref = JZ4725B_ADC_BATTERY_HIGH_VREF,
> >  	.battery_high_vref_bits = JZ4725B_ADC_BATTERY_HIGH_VREF_BITS,
> > @@ -271,6 +328,8 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail),
> >  	.battery_vref_mode = true,
> >  	.has_aux2 = false,
> > +	.channels = jz4740_channels,
> > +	.num_channels = ARRAY_SIZE(jz4740_channels),
> >  	.init_clk_div = jz4725b_adc_init_clk_div,
> >  };
> >  
> > @@ -283,6 +342,8 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = {
> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail),
> >  	.battery_vref_mode = true,
> >  	.has_aux2 = false,
> > +	.channels = jz4740_channels,
> > +	.num_channels = ARRAY_SIZE(jz4740_channels),
> >  	.init_clk_div = NULL, /* no ADCLK register on JZ4740 */
> >  };
> >  
> > @@ -295,6 +356,8 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {
> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail),
> >  	.battery_vref_mode = false,
> >  	.has_aux2 = true,
> > +	.channels = jz4770_channels,
> > +	.num_channels = ARRAY_SIZE(jz4770_channels),
> >  	.init_clk_div = jz4770_adc_init_clk_div,
> >  };
> >  
> > @@ -427,35 +490,6 @@ static const struct iio_info ingenic_adc_info = {
> >  	.of_xlate = ingenic_adc_of_xlate,
> >  };
> >  
> > -static const struct iio_chan_spec ingenic_channels[] = {
> > -	{
> > -		.extend_name = "aux",
> > -		.type = IIO_VOLTAGE,
> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > -				      BIT(IIO_CHAN_INFO_SCALE),
> > -		.indexed = 1,
> > -		.channel = INGENIC_ADC_AUX,
> > -	},
> > -	{
> > -		.extend_name = "battery",
> > -		.type = IIO_VOLTAGE,
> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > -				      BIT(IIO_CHAN_INFO_SCALE),
> > -		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
> > -						BIT(IIO_CHAN_INFO_SCALE),
> > -		.indexed = 1,
> > -		.channel = INGENIC_ADC_BATTERY,
> > -	},
> > -	{ /* Must always be last in the array. */
> > -		.extend_name = "aux2",
> > -		.type = IIO_VOLTAGE,
> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > -				      BIT(IIO_CHAN_INFO_SCALE),
> > -		.indexed = 1,
> > -		.channel = INGENIC_ADC_AUX2,
> > -	},
> > -};
> > -
> >  static int ingenic_adc_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > @@ -517,11 +551,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
> >  	iio_dev->dev.parent = dev;
> >  	iio_dev->name = "jz-adc";
> >  	iio_dev->modes = INDIO_DIRECT_MODE;
> > -	iio_dev->channels = ingenic_channels;
> > -	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
> > -	/* Remove AUX2 from the list of supported channels. */
> > -	if (!adc->soc_data->has_aux2)
> > -		iio_dev->num_channels -= 1;
> > +	iio_dev->channels = soc_data->channels;
> > +	iio_dev->num_channels = soc_data->num_channels;
> >  	iio_dev->info = &ingenic_adc_info;
> >  
> >  	ret = devm_iio_device_register(dev, iio_dev);  
> 

Re: [PATCH v8 4/6] iio/adc: ingenic: Retrieve channels list from soc data struct

[Artur Rojek]

On 2020-07-12 14:11, Jonathan Cameron wrote:
> On Sun, 12 Jul 2020 13:07:13 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> On Thu,  9 Jul 2020 17:21:58 +0200
>> Artur Rojek <contact@artur-rojek.eu> wrote:
>> 
>> > From: Paul Cercueil <paul@crapouillou.net>
>> >
>> > Instead of having one array of struct iio_chan_spec for all SoCs, and
>> > have some SoCs remove the last item of the array as they can't use it,
>> > have each SoC define its array of supported channels.
>> >
>> > Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>> > Tested-by: Artur Rojek <contact@artur-rojek.eu>
>> Applied to the togreg branch of iio.git and pushed out as testing for
>> autobuilders to play with it.
> Actually removed from tree again.
> 
> Artur, as you sent this patch on to me, you need to sign-off on it
> so we maintain the DCO related records of how this got to the kernel.
> You can just reply here to give it.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>

Thanks,
Artur

> 
> Thanks,
> 
> Jonathan
> 
>> 
>> Thanks,
>> 
>> Jonathan
>> 
>> > ---
>> >
>> >  Changes:
>> >
>> >  v8: new patch
>> >
>> >  drivers/iio/adc/ingenic-adc.c | 99 +++++++++++++++++++++++------------
>> >  1 file changed, 65 insertions(+), 34 deletions(-)
>> >
>> > diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
>> > index 89019fb59d48..0233a9055c86 100644
>> > --- a/drivers/iio/adc/ingenic-adc.c
>> > +++ b/drivers/iio/adc/ingenic-adc.c
>> > @@ -55,6 +55,8 @@ struct ingenic_adc_soc_data {
>> >  	size_t battery_scale_avail_size;
>> >  	unsigned int battery_vref_mode: 1;
>> >  	unsigned int has_aux2: 1;
>> > +	const struct iio_chan_spec *channels;
>> > +	unsigned int num_channels;
>> >  	int (*init_clk_div)(struct device *dev, struct ingenic_adc *adc);
>> >  };
>> >
>> > @@ -262,6 +264,61 @@ static int jz4770_adc_init_clk_div(struct device *dev, struct ingenic_adc *adc)
>> >  	return 0;
>> >  }
>> >
>> > +static const struct iio_chan_spec jz4740_channels[] = {
>> > +	{
>> > +		.extend_name = "aux",
>> > +		.type = IIO_VOLTAGE,
>> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > +				      BIT(IIO_CHAN_INFO_SCALE),
>> > +		.indexed = 1,
>> > +		.channel = INGENIC_ADC_AUX,
>> > +		.scan_index = -1,
>> > +	},
>> > +	{
>> > +		.extend_name = "battery",
>> > +		.type = IIO_VOLTAGE,
>> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > +				      BIT(IIO_CHAN_INFO_SCALE),
>> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
>> > +						BIT(IIO_CHAN_INFO_SCALE),
>> > +		.indexed = 1,
>> > +		.channel = INGENIC_ADC_BATTERY,
>> > +		.scan_index = -1,
>> > +	},
>> > +};
>> > +
>> > +static const struct iio_chan_spec jz4770_channels[] = {
>> > +	{
>> > +		.extend_name = "aux",
>> > +		.type = IIO_VOLTAGE,
>> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > +				      BIT(IIO_CHAN_INFO_SCALE),
>> > +		.indexed = 1,
>> > +		.channel = INGENIC_ADC_AUX,
>> > +		.scan_index = -1,
>> > +	},
>> > +	{
>> > +		.extend_name = "battery",
>> > +		.type = IIO_VOLTAGE,
>> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > +				      BIT(IIO_CHAN_INFO_SCALE),
>> > +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
>> > +						BIT(IIO_CHAN_INFO_SCALE),
>> > +		.indexed = 1,
>> > +		.channel = INGENIC_ADC_BATTERY,
>> > +		.scan_index = -1,
>> > +	},
>> > +	{
>> > +		.extend_name = "aux2",
>> > +		.type = IIO_VOLTAGE,
>> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > +				      BIT(IIO_CHAN_INFO_SCALE),
>> > +		.indexed = 1,
>> > +		.channel = INGENIC_ADC_AUX2,
>> > +		.scan_index = -1,
>> > +	},
>> > +};
>> > +
>> >  static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
>> >  	.battery_high_vref = JZ4725B_ADC_BATTERY_HIGH_VREF,
>> >  	.battery_high_vref_bits = JZ4725B_ADC_BATTERY_HIGH_VREF_BITS,
>> > @@ -271,6 +328,8 @@ static const struct ingenic_adc_soc_data jz4725b_adc_soc_data = {
>> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4725b_adc_battery_scale_avail),
>> >  	.battery_vref_mode = true,
>> >  	.has_aux2 = false,
>> > +	.channels = jz4740_channels,
>> > +	.num_channels = ARRAY_SIZE(jz4740_channels),
>> >  	.init_clk_div = jz4725b_adc_init_clk_div,
>> >  };
>> >
>> > @@ -283,6 +342,8 @@ static const struct ingenic_adc_soc_data jz4740_adc_soc_data = {
>> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4740_adc_battery_scale_avail),
>> >  	.battery_vref_mode = true,
>> >  	.has_aux2 = false,
>> > +	.channels = jz4740_channels,
>> > +	.num_channels = ARRAY_SIZE(jz4740_channels),
>> >  	.init_clk_div = NULL, /* no ADCLK register on JZ4740 */
>> >  };
>> >
>> > @@ -295,6 +356,8 @@ static const struct ingenic_adc_soc_data jz4770_adc_soc_data = {
>> >  	.battery_scale_avail_size = ARRAY_SIZE(jz4770_adc_battery_scale_avail),
>> >  	.battery_vref_mode = false,
>> >  	.has_aux2 = true,
>> > +	.channels = jz4770_channels,
>> > +	.num_channels = ARRAY_SIZE(jz4770_channels),
>> >  	.init_clk_div = jz4770_adc_init_clk_div,
>> >  };
>> >
>> > @@ -427,35 +490,6 @@ static const struct iio_info ingenic_adc_info = {
>> >  	.of_xlate = ingenic_adc_of_xlate,
>> >  };
>> >
>> > -static const struct iio_chan_spec ingenic_channels[] = {
>> > -	{
>> > -		.extend_name = "aux",
>> > -		.type = IIO_VOLTAGE,
>> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > -				      BIT(IIO_CHAN_INFO_SCALE),
>> > -		.indexed = 1,
>> > -		.channel = INGENIC_ADC_AUX,
>> > -	},
>> > -	{
>> > -		.extend_name = "battery",
>> > -		.type = IIO_VOLTAGE,
>> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > -				      BIT(IIO_CHAN_INFO_SCALE),
>> > -		.info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW) |
>> > -						BIT(IIO_CHAN_INFO_SCALE),
>> > -		.indexed = 1,
>> > -		.channel = INGENIC_ADC_BATTERY,
>> > -	},
>> > -	{ /* Must always be last in the array. */
>> > -		.extend_name = "aux2",
>> > -		.type = IIO_VOLTAGE,
>> > -		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> > -				      BIT(IIO_CHAN_INFO_SCALE),
>> > -		.indexed = 1,
>> > -		.channel = INGENIC_ADC_AUX2,
>> > -	},
>> > -};
>> > -
>> >  static int ingenic_adc_probe(struct platform_device *pdev)
>> >  {
>> >  	struct device *dev = &pdev->dev;
>> > @@ -517,11 +551,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
>> >  	iio_dev->dev.parent = dev;
>> >  	iio_dev->name = "jz-adc";
>> >  	iio_dev->modes = INDIO_DIRECT_MODE;
>> > -	iio_dev->channels = ingenic_channels;
>> > -	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
>> > -	/* Remove AUX2 from the list of supported channels. */
>> > -	if (!adc->soc_data->has_aux2)
>> > -		iio_dev->num_channels -= 1;
>> > +	iio_dev->channels = soc_data->channels;
>> > +	iio_dev->num_channels = soc_data->num_channels;
>> >  	iio_dev->info = &ingenic_adc_info;
>> >
>> >  	ret = devm_iio_device_register(dev, iio_dev);
>> 

[PATCH v8 5/6] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC

[Artur Rojek]

Introduce support for touchscreen channels found in JZ47xx SoCs.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
---

 Changes:

 v2-v7: no change

 v8: add XN/YN and XD/YD channels

 include/dt-bindings/iio/adc/ingenic,adc.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/dt-bindings/iio/adc/ingenic,adc.h b/include/dt-bindings/iio/adc/ingenic,adc.h
index 42f871ab3272..4627a00e369e 100644
--- a/include/dt-bindings/iio/adc/ingenic,adc.h
+++ b/include/dt-bindings/iio/adc/ingenic,adc.h
@@ -7,5 +7,11 @@
 #define INGENIC_ADC_AUX		0
 #define INGENIC_ADC_BATTERY	1
 #define INGENIC_ADC_AUX2	2
+#define INGENIC_ADC_TOUCH_XP	3
+#define INGENIC_ADC_TOUCH_YP	4
+#define INGENIC_ADC_TOUCH_XN	5
+#define INGENIC_ADC_TOUCH_YN	6
+#define INGENIC_ADC_TOUCH_XD	7
+#define INGENIC_ADC_TOUCH_YD	8
 
 #endif
-- 
2.27.0

Re: [PATCH v8 5/6] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC

[Rob Herring]

On Thu, 09 Jul 2020 17:21:59 +0200, Artur Rojek wrote:
> Introduce support for touchscreen channels found in JZ47xx SoCs.
> 
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
> ---
> 
>  Changes:
> 
>  v2-v7: no change
> 
>  v8: add XN/YN and XD/YD channels
> 
>  include/dt-bindings/iio/adc/ingenic,adc.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

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

[PATCH v8 6/6] IIO: Ingenic JZ47xx: Add touchscreen mode.

[Artur Rojek]

The SADC component in JZ47xx SoCs provides support for touchscreen
operations (pen position and pen down pressure) in single-ended and
differential modes.

The touchscreen component of SADC takes a significant time to stabilize
after first receiving the clock and a delay of 50ms has been empirically
proven to be a safe value before data sampling can begin.

Of the known hardware to use this controller, GCW Zero and Anbernic RG-350
utilize the touchscreen mode by having their joystick(s) attached to the
X/Y positive/negative input pins.

JZ4770 and later SoCs introduce a low-level command feature. With it, up
to 32 commands can be programmed, each one corresponding to a sampling
job. It allows to change the low-voltage reference, the high-voltage
reference, have them connected to VCC, GND, or one of the X-/X+ or Y-/Y+
pins.

This patch introduces support for 6 stream-capable channels:
- channel #0 samples X+/GND
- channel #1 samples Y+/GND
- channel #2 samples X-/GND
- channel #3 samples Y-/GND
- channel #4 samples X+/X-
- channel #5 samples Y+/Y-

Being able to sample X-/GND and Y-/GND is useful on some devices, where
one joystick is connected to the X+/Y+ pins, and a second joystick is
connected to the X-/Y- pins.

All the boards which probe this driver have the interrupt provided from
Device Tree, with no need to handle a case where the IRQ was not provided.

Co-developed-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
---

 Changes:

 v2: - improve description of the touchscreen mode,
     - get rid of the unneeded kfifo,
     - drop IIO_BUFFER_CB from Kconfig,
     - remove extended names from the touchscreen channels

 v3: remove unneeded `linux/iio/kfifo_buf.h` include

 v4: clarify irq provider source in the patch description

 v5: no change

 v6: - correct the spelling of Device Tree and IRQ in commit message
     - don't omit trailing commas from initializer lists
     - error check `clk_enable`
     - remove redundant `dev_err` from `platform_get_irq` error check

 v7: no change

 v8: add support for ADCMD low-level command feature

 drivers/iio/adc/Kconfig       |   1 +
 drivers/iio/adc/ingenic-adc.c | 250 +++++++++++++++++++++++++++++++++-
 2 files changed, 249 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index ff3569635ce0..5b57437cef75 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -500,6 +500,7 @@ config INA2XX_ADC
 config INGENIC_ADC
 	tristate "Ingenic JZ47xx SoCs ADC driver"
 	depends on MIPS || COMPILE_TEST
+	select IIO_BUFFER
 	help
 	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.
 
diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
index 0233a9055c86..976aea46fede 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -8,7 +8,9 @@
 
 #include <dt-bindings/iio/adc/ingenic,adc.h>
 #include <linux/clk.h>
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
@@ -20,19 +22,46 @@
 #define JZ_ADC_REG_CFG			0x04
 #define JZ_ADC_REG_CTRL			0x08
 #define JZ_ADC_REG_STATUS		0x0c
+#define JZ_ADC_REG_ADSAME		0x10
+#define JZ_ADC_REG_ADWAIT		0x14
 #define JZ_ADC_REG_ADTCH		0x18
 #define JZ_ADC_REG_ADBDAT		0x1c
 #define JZ_ADC_REG_ADSDAT		0x20
+#define JZ_ADC_REG_ADCMD		0x24
 #define JZ_ADC_REG_ADCLK		0x28
 
 #define JZ_ADC_REG_ENABLE_PD		BIT(7)
 #define JZ_ADC_REG_CFG_AUX_MD		(BIT(0) | BIT(1))
 #define JZ_ADC_REG_CFG_BAT_MD		BIT(4)
+#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
+#define JZ_ADC_REG_CFG_PULL_UP(n)	((n) << 16)
+#define JZ_ADC_REG_CFG_CMD_SEL		BIT(22)
+#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK	(BIT(31) | GENMASK(23, 10))
 #define JZ_ADC_REG_ADCLK_CLKDIV_LSB	0
 #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB	16
 #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB	8
 #define JZ4770_ADC_REG_ADCLK_CLKDIVMS_LSB	16
 
+#define JZ_ADC_REG_ADCMD_YNADC		BIT(7)
+#define JZ_ADC_REG_ADCMD_YPADC		BIT(8)
+#define JZ_ADC_REG_ADCMD_XNADC		BIT(9)
+#define JZ_ADC_REG_ADCMD_XPADC		BIT(10)
+#define JZ_ADC_REG_ADCMD_VREFPYP	BIT(11)
+#define JZ_ADC_REG_ADCMD_VREFPXP	BIT(12)
+#define JZ_ADC_REG_ADCMD_VREFPXN	BIT(13)
+#define JZ_ADC_REG_ADCMD_VREFPAUX	BIT(14)
+#define JZ_ADC_REG_ADCMD_VREFPVDD33	BIT(15)
+#define JZ_ADC_REG_ADCMD_VREFNYN	BIT(16)
+#define JZ_ADC_REG_ADCMD_VREFNXP	BIT(17)
+#define JZ_ADC_REG_ADCMD_VREFNXN	BIT(18)
+#define JZ_ADC_REG_ADCMD_VREFAUX	BIT(19)
+#define JZ_ADC_REG_ADCMD_YNGRU		BIT(20)
+#define JZ_ADC_REG_ADCMD_XNGRU		BIT(21)
+#define JZ_ADC_REG_ADCMD_XPGRU		BIT(22)
+#define JZ_ADC_REG_ADCMD_YPSUP		BIT(23)
+#define JZ_ADC_REG_ADCMD_XNSUP		BIT(24)
+#define JZ_ADC_REG_ADCMD_XPSUP		BIT(25)
+
 #define JZ_ADC_AUX_VREF				3300
 #define JZ_ADC_AUX_VREF_BITS			12
 #define JZ_ADC_BATTERY_LOW_VREF			2500
@@ -44,6 +73,14 @@
 #define JZ4770_ADC_BATTERY_VREF			6600
 #define JZ4770_ADC_BATTERY_VREF_BITS		12
 
+#define JZ_ADC_IRQ_AUX			BIT(0)
+#define JZ_ADC_IRQ_BATTERY		BIT(1)
+#define JZ_ADC_IRQ_TOUCH		BIT(2)
+#define JZ_ADC_IRQ_PEN_DOWN		BIT(3)
+#define JZ_ADC_IRQ_PEN_UP		BIT(4)
+#define JZ_ADC_IRQ_PEN_DOWN_SLEEP	BIT(5)
+#define JZ_ADC_IRQ_SLEEP		BIT(7)
+
 struct ingenic_adc;
 
 struct ingenic_adc_soc_data {
@@ -69,6 +106,61 @@ struct ingenic_adc {
 	bool low_vref_mode;
 };
 
+static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, unsigned long mask)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+
+	mutex_lock(&adc->lock);
+
+	/* Init ADCMD */
+	readl(adc->base + JZ_ADC_REG_ADCMD);
+
+	if (mask & 0x3) {
+		/* Second channel (INGENIC_ADC_TOUCH_YP): sample YP vs. GND */
+		writel(JZ_ADC_REG_ADCMD_XNGRU
+		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_YPADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+
+		/* First channel (INGENIC_ADC_TOUCH_XP): sample XP vs. GND */
+		writel(JZ_ADC_REG_ADCMD_YNGRU
+		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_XPADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+	}
+
+	if (mask & 0xc) {
+		/* Fourth channel (INGENIC_ADC_TOUCH_YN): sample YN vs. GND */
+		writel(JZ_ADC_REG_ADCMD_XNGRU
+		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_YNADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+
+		/* Third channel (INGENIC_ADC_TOUCH_XN): sample XN vs. GND */
+		writel(JZ_ADC_REG_ADCMD_YNGRU
+		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_XNADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+	}
+
+	if (mask & 0x30) {
+		/* Sixth channel (INGENIC_ADC_TOUCH_YD): sample YP vs. YN */
+		writel(JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_YPADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+
+		/* Fifth channel (INGENIC_ADC_TOUCH_XD): sample XP vs. XN */
+		writel(JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
+		       | JZ_ADC_REG_ADCMD_XPADC,
+		       adc->base + JZ_ADC_REG_ADCMD);
+	}
+
+	/* We're done */
+	writel(0, adc->base + JZ_ADC_REG_ADCMD);
+
+	mutex_unlock(&adc->lock);
+}
+
 static void ingenic_adc_set_config(struct ingenic_adc *adc,
 				   uint32_t mask,
 				   uint32_t val)
@@ -288,6 +380,72 @@ static const struct iio_chan_spec jz4740_channels[] = {
 };
 
 static const struct iio_chan_spec jz4770_channels[] = {
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_XP,
+		.scan_index = 0,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_YP,
+		.scan_index = 1,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_XN,
+		.scan_index = 2,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_YN,
+		.scan_index = 3,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_XD,
+		.scan_index = 4,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
+	{
+		.type = IIO_POSITIONRELATIVE,
+		.indexed = 1,
+		.channel = INGENIC_ADC_TOUCH_YD,
+		.scan_index = 5,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 12,
+			.storagebits = 16,
+		},
+	},
 	{
 		.extend_name = "aux",
 		.type = IIO_VOLTAGE,
@@ -490,13 +648,89 @@ static const struct iio_info ingenic_adc_info = {
 	.of_xlate = ingenic_adc_of_xlate,
 };
 
+static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+	int ret;
+
+	ret = clk_enable(adc->clk);
+	if (ret) {
+		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n",
+			ret);
+		return ret;
+	}
+
+	/* It takes significant time for the touchscreen hw to stabilize. */
+	msleep(50);
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
+			       JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
+			       JZ_ADC_REG_CFG_PULL_UP(4));
+
+	writew(80, adc->base + JZ_ADC_REG_ADWAIT);
+	writew(2, adc->base + JZ_ADC_REG_ADSAME);
+	writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
+	writel(0, adc->base + JZ_ADC_REG_ADTCH);
+
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL,
+			       JZ_ADC_REG_CFG_CMD_SEL);
+	ingenic_adc_set_adcmd(iio_dev, iio_dev->active_scan_mask[0]);
+
+	ingenic_adc_enable(adc, 2, true);
+
+	return 0;
+}
+
+static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+
+	ingenic_adc_enable(adc, 2, false);
+
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, 0);
+
+	writeb(0xff, adc->base + JZ_ADC_REG_CTRL);
+	writeb(0xff, adc->base + JZ_ADC_REG_STATUS);
+	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0);
+	writew(0, adc->base + JZ_ADC_REG_ADSAME);
+	writew(0, adc->base + JZ_ADC_REG_ADWAIT);
+	clk_disable(adc->clk);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops = {
+	.postenable = &ingenic_adc_buffer_enable,
+	.predisable = &ingenic_adc_buffer_disable
+};
+
+static irqreturn_t ingenic_adc_irq(int irq, void *data)
+{
+	struct iio_dev *iio_dev = data;
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+	unsigned long mask = iio_dev->active_scan_mask[0];
+	unsigned int i;
+	u32 tdat[3];
+
+	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
+		if (mask & 0x3)
+			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
+		else
+			tdat[i] = 0;
+	}
+
+	iio_push_to_buffers(iio_dev, tdat);
+	writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS);
+
+	return IRQ_HANDLED;
+}
+
 static int ingenic_adc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct iio_dev *iio_dev;
 	struct ingenic_adc *adc;
 	const struct ingenic_adc_soc_data *soc_data;
-	int ret;
+	int irq, ret;
 
 	soc_data = device_get_match_data(dev);
 	if (!soc_data)
@@ -511,6 +745,17 @@ static int ingenic_adc_probe(struct platform_device *pdev)
 	mutex_init(&adc->aux_lock);
 	adc->soc_data = soc_data;
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0,
+			       dev_name(dev), iio_dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to request irq: %d\n", ret);
+		return ret;
+	}
+
 	adc->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(adc->base))
 		return PTR_ERR(adc->base);
@@ -550,7 +795,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
 
 	iio_dev->dev.parent = dev;
 	iio_dev->name = "jz-adc";
-	iio_dev->modes = INDIO_DIRECT_MODE;
+	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	iio_dev->setup_ops = &ingenic_buffer_setup_ops;
 	iio_dev->channels = soc_data->channels;
 	iio_dev->num_channels = soc_data->num_channels;
 	iio_dev->info = &ingenic_adc_info;
-- 
2.27.0

Re: [PATCH v8 6/6] IIO: Ingenic JZ47xx: Add touchscreen mode.

[Jonathan Cameron]

On Thu,  9 Jul 2020 17:22:00 +0200
Artur Rojek <contact@artur-rojek.eu> wrote:

> The SADC component in JZ47xx SoCs provides support for touchscreen
> operations (pen position and pen down pressure) in single-ended and
> differential modes.
> 
> The touchscreen component of SADC takes a significant time to stabilize
> after first receiving the clock and a delay of 50ms has been empirically
> proven to be a safe value before data sampling can begin.
> 
> Of the known hardware to use this controller, GCW Zero and Anbernic RG-350
> utilize the touchscreen mode by having their joystick(s) attached to the
> X/Y positive/negative input pins.
> 
> JZ4770 and later SoCs introduce a low-level command feature. With it, up
> to 32 commands can be programmed, each one corresponding to a sampling
> job. It allows to change the low-voltage reference, the high-voltage
> reference, have them connected to VCC, GND, or one of the X-/X+ or Y-/Y+
> pins.
> 
> This patch introduces support for 6 stream-capable channels:
> - channel #0 samples X+/GND
> - channel #1 samples Y+/GND
> - channel #2 samples X-/GND
> - channel #3 samples Y-/GND
> - channel #4 samples X+/X-
> - channel #5 samples Y+/Y-

The one thing I noticed on this read was that we are slightly stretching
the normal IIO channel definitions.  The claim is that each of these channels
is a POSITIONREALTIVE channel, whereas that isn't really true.  They are
related to the position, but as I understand it not directly measuring it
(particularly X+ - X-) which is pretty much a reference voltage (I think!)

We might be better off just describing these as voltage channels, with
4 and 5 described as differential voltage channels.  The problem there
being that it doesn't describe the fact that the measurements are with
particular voltages also being applied to the touch screen.

Perhaps we are best off just leaving it as you have it and being a bit
'odd'.  What do you think?  Having written this down I think perhaps leaving
it alone is the best plan :(

Otherwise this all looks fine to me.

Thanks,

Jonathan

> 
> Being able to sample X-/GND and Y-/GND is useful on some devices, where
> one joystick is connected to the X+/Y+ pins, and a second joystick is
> connected to the X-/Y- pins.
> 
> All the boards which probe this driver have the interrupt provided from
> Device Tree, with no need to handle a case where the IRQ was not provided.
> 
> Co-developed-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
>  Changes:
> 
>  v2: - improve description of the touchscreen mode,
>      - get rid of the unneeded kfifo,
>      - drop IIO_BUFFER_CB from Kconfig,
>      - remove extended names from the touchscreen channels
> 
>  v3: remove unneeded `linux/iio/kfifo_buf.h` include
> 
>  v4: clarify irq provider source in the patch description
> 
>  v5: no change
> 
>  v6: - correct the spelling of Device Tree and IRQ in commit message
>      - don't omit trailing commas from initializer lists
>      - error check `clk_enable`
>      - remove redundant `dev_err` from `platform_get_irq` error check
> 
>  v7: no change
> 
>  v8: add support for ADCMD low-level command feature
> 
>  drivers/iio/adc/Kconfig       |   1 +
>  drivers/iio/adc/ingenic-adc.c | 250 +++++++++++++++++++++++++++++++++-
>  2 files changed, 249 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index ff3569635ce0..5b57437cef75 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -500,6 +500,7 @@ config INA2XX_ADC
>  config INGENIC_ADC
>  	tristate "Ingenic JZ47xx SoCs ADC driver"
>  	depends on MIPS || COMPILE_TEST
> +	select IIO_BUFFER
>  	help
>  	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.
>  
> diff --git a/drivers/iio/adc/ingenic-adc.c b/drivers/iio/adc/ingenic-adc.c
> index 0233a9055c86..976aea46fede 100644
> --- a/drivers/iio/adc/ingenic-adc.c
> +++ b/drivers/iio/adc/ingenic-adc.c
> @@ -8,7 +8,9 @@
>  
>  #include <dt-bindings/iio/adc/ingenic,adc.h>
>  #include <linux/clk.h>
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
> @@ -20,19 +22,46 @@
>  #define JZ_ADC_REG_CFG			0x04
>  #define JZ_ADC_REG_CTRL			0x08
>  #define JZ_ADC_REG_STATUS		0x0c
> +#define JZ_ADC_REG_ADSAME		0x10
> +#define JZ_ADC_REG_ADWAIT		0x14
>  #define JZ_ADC_REG_ADTCH		0x18
>  #define JZ_ADC_REG_ADBDAT		0x1c
>  #define JZ_ADC_REG_ADSDAT		0x20
> +#define JZ_ADC_REG_ADCMD		0x24
>  #define JZ_ADC_REG_ADCLK		0x28
>  
>  #define JZ_ADC_REG_ENABLE_PD		BIT(7)
>  #define JZ_ADC_REG_CFG_AUX_MD		(BIT(0) | BIT(1))
>  #define JZ_ADC_REG_CFG_BAT_MD		BIT(4)
> +#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
> +#define JZ_ADC_REG_CFG_PULL_UP(n)	((n) << 16)
> +#define JZ_ADC_REG_CFG_CMD_SEL		BIT(22)
> +#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK	(BIT(31) | GENMASK(23, 10))
>  #define JZ_ADC_REG_ADCLK_CLKDIV_LSB	0
>  #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB	16
>  #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB	8
>  #define JZ4770_ADC_REG_ADCLK_CLKDIVMS_LSB	16
>  
> +#define JZ_ADC_REG_ADCMD_YNADC		BIT(7)
> +#define JZ_ADC_REG_ADCMD_YPADC		BIT(8)
> +#define JZ_ADC_REG_ADCMD_XNADC		BIT(9)
> +#define JZ_ADC_REG_ADCMD_XPADC		BIT(10)
> +#define JZ_ADC_REG_ADCMD_VREFPYP	BIT(11)
> +#define JZ_ADC_REG_ADCMD_VREFPXP	BIT(12)
> +#define JZ_ADC_REG_ADCMD_VREFPXN	BIT(13)
> +#define JZ_ADC_REG_ADCMD_VREFPAUX	BIT(14)
> +#define JZ_ADC_REG_ADCMD_VREFPVDD33	BIT(15)
> +#define JZ_ADC_REG_ADCMD_VREFNYN	BIT(16)
> +#define JZ_ADC_REG_ADCMD_VREFNXP	BIT(17)
> +#define JZ_ADC_REG_ADCMD_VREFNXN	BIT(18)
> +#define JZ_ADC_REG_ADCMD_VREFAUX	BIT(19)
> +#define JZ_ADC_REG_ADCMD_YNGRU		BIT(20)
> +#define JZ_ADC_REG_ADCMD_XNGRU		BIT(21)
> +#define JZ_ADC_REG_ADCMD_XPGRU		BIT(22)
> +#define JZ_ADC_REG_ADCMD_YPSUP		BIT(23)
> +#define JZ_ADC_REG_ADCMD_XNSUP		BIT(24)
> +#define JZ_ADC_REG_ADCMD_XPSUP		BIT(25)
> +
>  #define JZ_ADC_AUX_VREF				3300
>  #define JZ_ADC_AUX_VREF_BITS			12
>  #define JZ_ADC_BATTERY_LOW_VREF			2500
> @@ -44,6 +73,14 @@
>  #define JZ4770_ADC_BATTERY_VREF			6600
>  #define JZ4770_ADC_BATTERY_VREF_BITS		12
>  
> +#define JZ_ADC_IRQ_AUX			BIT(0)
> +#define JZ_ADC_IRQ_BATTERY		BIT(1)
> +#define JZ_ADC_IRQ_TOUCH		BIT(2)
> +#define JZ_ADC_IRQ_PEN_DOWN		BIT(3)
> +#define JZ_ADC_IRQ_PEN_UP		BIT(4)
> +#define JZ_ADC_IRQ_PEN_DOWN_SLEEP	BIT(5)
> +#define JZ_ADC_IRQ_SLEEP		BIT(7)
> +
>  struct ingenic_adc;
>  
>  struct ingenic_adc_soc_data {
> @@ -69,6 +106,61 @@ struct ingenic_adc {
>  	bool low_vref_mode;
>  };
>  
> +static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, unsigned long mask)
> +{
> +	struct ingenic_adc *adc = iio_priv(iio_dev);
> +
> +	mutex_lock(&adc->lock);
> +
> +	/* Init ADCMD */
> +	readl(adc->base + JZ_ADC_REG_ADCMD);
> +
> +	if (mask & 0x3) {
> +		/* Second channel (INGENIC_ADC_TOUCH_YP): sample YP vs. GND */
> +		writel(JZ_ADC_REG_ADCMD_XNGRU
> +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_YPADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +
> +		/* First channel (INGENIC_ADC_TOUCH_XP): sample XP vs. GND */
> +		writel(JZ_ADC_REG_ADCMD_YNGRU
> +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_XPADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +	}
> +
> +	if (mask & 0xc) {
> +		/* Fourth channel (INGENIC_ADC_TOUCH_YN): sample YN vs. GND */
> +		writel(JZ_ADC_REG_ADCMD_XNGRU
> +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_YNADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +
> +		/* Third channel (INGENIC_ADC_TOUCH_XN): sample XN vs. GND */
> +		writel(JZ_ADC_REG_ADCMD_YNGRU
> +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_XNADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +	}
> +
> +	if (mask & 0x30) {
> +		/* Sixth channel (INGENIC_ADC_TOUCH_YD): sample YP vs. YN */
> +		writel(JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_YPADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +
> +		/* Fifth channel (INGENIC_ADC_TOUCH_XD): sample XP vs. XN */
> +		writel(JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> +		       | JZ_ADC_REG_ADCMD_XPADC,
> +		       adc->base + JZ_ADC_REG_ADCMD);
> +	}
> +
> +	/* We're done */
> +	writel(0, adc->base + JZ_ADC_REG_ADCMD);
> +
> +	mutex_unlock(&adc->lock);
> +}
> +
>  static void ingenic_adc_set_config(struct ingenic_adc *adc,
>  				   uint32_t mask,
>  				   uint32_t val)
> @@ -288,6 +380,72 @@ static const struct iio_chan_spec jz4740_channels[] = {
>  };
>  
>  static const struct iio_chan_spec jz4770_channels[] = {
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_XP,

Ah. sorry. I should have noticed this much earlier.

These aren't position channels as such.  They are channels
that are then processed into position in combination.  

Problem is I'm not sure how we 'should' describe then.

Perhaps this is the best we can do.

> +		.scan_index = 0,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_YP,
> +		.scan_index = 1,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_XN,
> +		.scan_index = 2,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_YN,
> +		.scan_index = 3,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_XD,
> +		.scan_index = 4,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
> +	{
> +		.type = IIO_POSITIONRELATIVE,
> +		.indexed = 1,
> +		.channel = INGENIC_ADC_TOUCH_YD,
> +		.scan_index = 5,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 12,
> +			.storagebits = 16,
> +		},
> +	},
>  	{
>  		.extend_name = "aux",
>  		.type = IIO_VOLTAGE,
> @@ -490,13 +648,89 @@ static const struct iio_info ingenic_adc_info = {
>  	.of_xlate = ingenic_adc_of_xlate,
>  };
>  
> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
> +{
> +	struct ingenic_adc *adc = iio_priv(iio_dev);
> +	int ret;
> +
> +	ret = clk_enable(adc->clk);
> +	if (ret) {
> +		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	/* It takes significant time for the touchscreen hw to stabilize. */
> +	msleep(50);
> +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
> +			       JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
> +			       JZ_ADC_REG_CFG_PULL_UP(4));
> +
> +	writew(80, adc->base + JZ_ADC_REG_ADWAIT);
> +	writew(2, adc->base + JZ_ADC_REG_ADSAME);
> +	writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
> +	writel(0, adc->base + JZ_ADC_REG_ADTCH);
> +
> +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL,
> +			       JZ_ADC_REG_CFG_CMD_SEL);
> +	ingenic_adc_set_adcmd(iio_dev, iio_dev->active_scan_mask[0]);
> +
> +	ingenic_adc_enable(adc, 2, true);
> +
> +	return 0;
> +}
> +
> +static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev)
> +{
> +	struct ingenic_adc *adc = iio_priv(iio_dev);
> +
> +	ingenic_adc_enable(adc, 2, false);
> +
> +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, 0);
> +
> +	writeb(0xff, adc->base + JZ_ADC_REG_CTRL);
> +	writeb(0xff, adc->base + JZ_ADC_REG_STATUS);
> +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0);
> +	writew(0, adc->base + JZ_ADC_REG_ADSAME);
> +	writew(0, adc->base + JZ_ADC_REG_ADWAIT);
> +	clk_disable(adc->clk);
> +
> +	return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops = {
> +	.postenable = &ingenic_adc_buffer_enable,
> +	.predisable = &ingenic_adc_buffer_disable
> +};
> +
> +static irqreturn_t ingenic_adc_irq(int irq, void *data)
> +{
> +	struct iio_dev *iio_dev = data;
> +	struct ingenic_adc *adc = iio_priv(iio_dev);
> +	unsigned long mask = iio_dev->active_scan_mask[0];
> +	unsigned int i;
> +	u32 tdat[3];
> +
> +	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
> +		if (mask & 0x3)
> +			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
> +		else
> +			tdat[i] = 0;
> +	}
> +
> +	iio_push_to_buffers(iio_dev, tdat);
> +	writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ingenic_adc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct iio_dev *iio_dev;
>  	struct ingenic_adc *adc;
>  	const struct ingenic_adc_soc_data *soc_data;
> -	int ret;
> +	int irq, ret;
>  
>  	soc_data = device_get_match_data(dev);
>  	if (!soc_data)
> @@ -511,6 +745,17 @@ static int ingenic_adc_probe(struct platform_device *pdev)
>  	mutex_init(&adc->aux_lock);
>  	adc->soc_data = soc_data;
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0,
> +			       dev_name(dev), iio_dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to request irq: %d\n", ret);
> +		return ret;
> +	}
> +
>  	adc->base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(adc->base))
>  		return PTR_ERR(adc->base);
> @@ -550,7 +795,8 @@ static int ingenic_adc_probe(struct platform_device *pdev)
>  
>  	iio_dev->dev.parent = dev;
>  	iio_dev->name = "jz-adc";
> -	iio_dev->modes = INDIO_DIRECT_MODE;
> +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	iio_dev->setup_ops = &ingenic_buffer_setup_ops;
>  	iio_dev->channels = soc_data->channels;
>  	iio_dev->num_channels = soc_data->num_channels;
>  	iio_dev->info = &ingenic_adc_info;

Re: [PATCH v8 6/6] IIO: Ingenic JZ47xx: Add touchscreen mode.

[Paul Cercueil]

Hi Jonathan,

Le dim. 12 juil. 2020 à 14:19, Jonathan Cameron <jic23@kernel.org> a 
écrit :
> On Thu,  9 Jul 2020 17:22:00 +0200
> Artur Rojek <contact@artur-rojek.eu> wrote:
> 
>>  The SADC component in JZ47xx SoCs provides support for touchscreen
>>  operations (pen position and pen down pressure) in single-ended and
>>  differential modes.
>> 
>>  The touchscreen component of SADC takes a significant time to 
>> stabilize
>>  after first receiving the clock and a delay of 50ms has been 
>> empirically
>>  proven to be a safe value before data sampling can begin.
>> 
>>  Of the known hardware to use this controller, GCW Zero and Anbernic 
>> RG-350
>>  utilize the touchscreen mode by having their joystick(s) attached 
>> to the
>>  X/Y positive/negative input pins.
>> 
>>  JZ4770 and later SoCs introduce a low-level command feature. With 
>> it, up
>>  to 32 commands can be programmed, each one corresponding to a 
>> sampling
>>  job. It allows to change the low-voltage reference, the high-voltage
>>  reference, have them connected to VCC, GND, or one of the X-/X+ or 
>> Y-/Y+
>>  pins.
>> 
>>  This patch introduces support for 6 stream-capable channels:
>>  - channel #0 samples X+/GND
>>  - channel #1 samples Y+/GND
>>  - channel #2 samples X-/GND
>>  - channel #3 samples Y-/GND
>>  - channel #4 samples X+/X-
>>  - channel #5 samples Y+/Y-
> 
> The one thing I noticed on this read was that we are slightly 
> stretching
> the normal IIO channel definitions.  The claim is that each of these 
> channels
> is a POSITIONREALTIVE channel, whereas that isn't really true.  They 
> are
> related to the position, but as I understand it not directly 
> measuring it
> (particularly X+ - X-) which is pretty much a reference voltage (I 
> think!)
> 
> We might be better off just describing these as voltage channels, with
> 4 and 5 described as differential voltage channels.  The problem there
> being that it doesn't describe the fact that the measurements are with
> particular voltages also being applied to the touch screen.
> 
> Perhaps we are best off just leaving it as you have it and being a bit
> 'odd'.  What do you think?  Having written this down I think perhaps 
> leaving
> it alone is the best plan :(

That's up to you really. These ADC channels will measure voltage, but 
they are supposed to be used with a touchscreen, that's why we went 
with POSITIONRELATIVE. However, with the kind of devices we're working 
with, they are never used as such, so using VOLTAGE would work too.

> Otherwise this all looks fine to me.
> 
> Thanks,
> 
> Jonathan
> 
>> 
>>  Being able to sample X-/GND and Y-/GND is useful on some devices, 
>> where
>>  one joystick is connected to the X+/Y+ pins, and a second joystick 
>> is
>>  connected to the X-/Y- pins.
>> 
>>  All the boards which probe this driver have the interrupt provided 
>> from
>>  Device Tree, with no need to handle a case where the IRQ was not 
>> provided.
>> 
>>  Co-developed-by: Paul Cercueil <paul@crapouillou.net>
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>> 
>>   Changes:
>> 
>>   v2: - improve description of the touchscreen mode,
>>       - get rid of the unneeded kfifo,
>>       - drop IIO_BUFFER_CB from Kconfig,
>>       - remove extended names from the touchscreen channels
>> 
>>   v3: remove unneeded `linux/iio/kfifo_buf.h` include
>> 
>>   v4: clarify irq provider source in the patch description
>> 
>>   v5: no change
>> 
>>   v6: - correct the spelling of Device Tree and IRQ in commit message
>>       - don't omit trailing commas from initializer lists
>>       - error check `clk_enable`
>>       - remove redundant `dev_err` from `platform_get_irq` error 
>> check
>> 
>>   v7: no change
>> 
>>   v8: add support for ADCMD low-level command feature
>> 
>>   drivers/iio/adc/Kconfig       |   1 +
>>   drivers/iio/adc/ingenic-adc.c | 250 
>> +++++++++++++++++++++++++++++++++-
>>   2 files changed, 249 insertions(+), 2 deletions(-)
>> 
>>  diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>  index ff3569635ce0..5b57437cef75 100644
>>  --- a/drivers/iio/adc/Kconfig
>>  +++ b/drivers/iio/adc/Kconfig
>>  @@ -500,6 +500,7 @@ config INA2XX_ADC
>>   config INGENIC_ADC
>>   	tristate "Ingenic JZ47xx SoCs ADC driver"
>>   	depends on MIPS || COMPILE_TEST
>>  +	select IIO_BUFFER
>>   	help
>>   	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC 
>> unit.
>> 
>>  diff --git a/drivers/iio/adc/ingenic-adc.c 
>> b/drivers/iio/adc/ingenic-adc.c
>>  index 0233a9055c86..976aea46fede 100644
>>  --- a/drivers/iio/adc/ingenic-adc.c
>>  +++ b/drivers/iio/adc/ingenic-adc.c
>>  @@ -8,7 +8,9 @@
>> 
>>   #include <dt-bindings/iio/adc/ingenic,adc.h>
>>   #include <linux/clk.h>
>>  +#include <linux/iio/buffer.h>
>>   #include <linux/iio/iio.h>
>>  +#include <linux/interrupt.h>
>>   #include <linux/io.h>
>>   #include <linux/iopoll.h>
>>   #include <linux/kernel.h>
>>  @@ -20,19 +22,46 @@
>>   #define JZ_ADC_REG_CFG			0x04
>>   #define JZ_ADC_REG_CTRL			0x08
>>   #define JZ_ADC_REG_STATUS		0x0c
>>  +#define JZ_ADC_REG_ADSAME		0x10
>>  +#define JZ_ADC_REG_ADWAIT		0x14
>>   #define JZ_ADC_REG_ADTCH		0x18
>>   #define JZ_ADC_REG_ADBDAT		0x1c
>>   #define JZ_ADC_REG_ADSDAT		0x20
>>  +#define JZ_ADC_REG_ADCMD		0x24
>>   #define JZ_ADC_REG_ADCLK		0x28
>> 
>>   #define JZ_ADC_REG_ENABLE_PD		BIT(7)
>>   #define JZ_ADC_REG_CFG_AUX_MD		(BIT(0) | BIT(1))
>>   #define JZ_ADC_REG_CFG_BAT_MD		BIT(4)
>>  +#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
>>  +#define JZ_ADC_REG_CFG_PULL_UP(n)	((n) << 16)
>>  +#define JZ_ADC_REG_CFG_CMD_SEL		BIT(22)
>>  +#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK	(BIT(31) | GENMASK(23, 10))
>>   #define JZ_ADC_REG_ADCLK_CLKDIV_LSB	0
>>   #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB	16
>>   #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB	8
>>   #define JZ4770_ADC_REG_ADCLK_CLKDIVMS_LSB	16
>> 
>>  +#define JZ_ADC_REG_ADCMD_YNADC		BIT(7)
>>  +#define JZ_ADC_REG_ADCMD_YPADC		BIT(8)
>>  +#define JZ_ADC_REG_ADCMD_XNADC		BIT(9)
>>  +#define JZ_ADC_REG_ADCMD_XPADC		BIT(10)
>>  +#define JZ_ADC_REG_ADCMD_VREFPYP	BIT(11)
>>  +#define JZ_ADC_REG_ADCMD_VREFPXP	BIT(12)
>>  +#define JZ_ADC_REG_ADCMD_VREFPXN	BIT(13)
>>  +#define JZ_ADC_REG_ADCMD_VREFPAUX	BIT(14)
>>  +#define JZ_ADC_REG_ADCMD_VREFPVDD33	BIT(15)
>>  +#define JZ_ADC_REG_ADCMD_VREFNYN	BIT(16)
>>  +#define JZ_ADC_REG_ADCMD_VREFNXP	BIT(17)
>>  +#define JZ_ADC_REG_ADCMD_VREFNXN	BIT(18)
>>  +#define JZ_ADC_REG_ADCMD_VREFAUX	BIT(19)
>>  +#define JZ_ADC_REG_ADCMD_YNGRU		BIT(20)
>>  +#define JZ_ADC_REG_ADCMD_XNGRU		BIT(21)
>>  +#define JZ_ADC_REG_ADCMD_XPGRU		BIT(22)
>>  +#define JZ_ADC_REG_ADCMD_YPSUP		BIT(23)
>>  +#define JZ_ADC_REG_ADCMD_XNSUP		BIT(24)
>>  +#define JZ_ADC_REG_ADCMD_XPSUP		BIT(25)
>>  +
>>   #define JZ_ADC_AUX_VREF				3300
>>   #define JZ_ADC_AUX_VREF_BITS			12
>>   #define JZ_ADC_BATTERY_LOW_VREF			2500
>>  @@ -44,6 +73,14 @@
>>   #define JZ4770_ADC_BATTERY_VREF			6600
>>   #define JZ4770_ADC_BATTERY_VREF_BITS		12
>> 
>>  +#define JZ_ADC_IRQ_AUX			BIT(0)
>>  +#define JZ_ADC_IRQ_BATTERY		BIT(1)
>>  +#define JZ_ADC_IRQ_TOUCH		BIT(2)
>>  +#define JZ_ADC_IRQ_PEN_DOWN		BIT(3)
>>  +#define JZ_ADC_IRQ_PEN_UP		BIT(4)
>>  +#define JZ_ADC_IRQ_PEN_DOWN_SLEEP	BIT(5)
>>  +#define JZ_ADC_IRQ_SLEEP		BIT(7)
>>  +
>>   struct ingenic_adc;
>> 
>>   struct ingenic_adc_soc_data {
>>  @@ -69,6 +106,61 @@ struct ingenic_adc {
>>   	bool low_vref_mode;
>>   };
>> 
>>  +static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, 
>> unsigned long mask)
>>  +{
>>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +
>>  +	mutex_lock(&adc->lock);
>>  +
>>  +	/* Init ADCMD */
>>  +	readl(adc->base + JZ_ADC_REG_ADCMD);
>>  +
>>  +	if (mask & 0x3) {
>>  +		/* Second channel (INGENIC_ADC_TOUCH_YP): sample YP vs. GND */
>>  +		writel(JZ_ADC_REG_ADCMD_XNGRU
>>  +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_YPADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +
>>  +		/* First channel (INGENIC_ADC_TOUCH_XP): sample XP vs. GND */
>>  +		writel(JZ_ADC_REG_ADCMD_YNGRU
>>  +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_XPADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +	}
>>  +
>>  +	if (mask & 0xc) {
>>  +		/* Fourth channel (INGENIC_ADC_TOUCH_YN): sample YN vs. GND */
>>  +		writel(JZ_ADC_REG_ADCMD_XNGRU
>>  +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_YNADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +
>>  +		/* Third channel (INGENIC_ADC_TOUCH_XN): sample XN vs. GND */
>>  +		writel(JZ_ADC_REG_ADCMD_YNGRU
>>  +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_XNADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +	}
>>  +
>>  +	if (mask & 0x30) {
>>  +		/* Sixth channel (INGENIC_ADC_TOUCH_YD): sample YP vs. YN */
>>  +		writel(JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_YPADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +
>>  +		/* Fifth channel (INGENIC_ADC_TOUCH_XD): sample XP vs. XN */
>>  +		writel(JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
>>  +		       | JZ_ADC_REG_ADCMD_XPADC,
>>  +		       adc->base + JZ_ADC_REG_ADCMD);
>>  +	}
>>  +
>>  +	/* We're done */
>>  +	writel(0, adc->base + JZ_ADC_REG_ADCMD);
>>  +
>>  +	mutex_unlock(&adc->lock);
>>  +}
>>  +
>>   static void ingenic_adc_set_config(struct ingenic_adc *adc,
>>   				   uint32_t mask,
>>   				   uint32_t val)
>>  @@ -288,6 +380,72 @@ static const struct iio_chan_spec 
>> jz4740_channels[] = {
>>   };
>> 
>>   static const struct iio_chan_spec jz4770_channels[] = {
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_XP,
> 
> Ah. sorry. I should have noticed this much earlier.
> 
> These aren't position channels as such.  They are channels
> that are then processed into position in combination.
> 
> Problem is I'm not sure how we 'should' describe then.
> 
> Perhaps this is the best we can do.

We could have the two differencial channels as IIO_POSITIONRELATIVE, 
and the four single-ended ones as IIO_VOLTAGE.

Cheers,
-Paul

>>  +		.scan_index = 0,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_YP,
>>  +		.scan_index = 1,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_XN,
>>  +		.scan_index = 2,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_YN,
>>  +		.scan_index = 3,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_XD,
>>  +		.scan_index = 4,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>  +	{
>>  +		.type = IIO_POSITIONRELATIVE,
>>  +		.indexed = 1,
>>  +		.channel = INGENIC_ADC_TOUCH_YD,
>>  +		.scan_index = 5,
>>  +		.scan_type = {
>>  +			.sign = 'u',
>>  +			.realbits = 12,
>>  +			.storagebits = 16,
>>  +		},
>>  +	},
>>   	{
>>   		.extend_name = "aux",
>>   		.type = IIO_VOLTAGE,
>>  @@ -490,13 +648,89 @@ static const struct iio_info ingenic_adc_info 
>> = {
>>   	.of_xlate = ingenic_adc_of_xlate,
>>   };
>> 
>>  +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
>>  +{
>>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +	int ret;
>>  +
>>  +	ret = clk_enable(adc->clk);
>>  +	if (ret) {
>>  +		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n",
>>  +			ret);
>>  +		return ret;
>>  +	}
>>  +
>>  +	/* It takes significant time for the touchscreen hw to stabilize. 
>> */
>>  +	msleep(50);
>>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
>>  +			       JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
>>  +			       JZ_ADC_REG_CFG_PULL_UP(4));
>>  +
>>  +	writew(80, adc->base + JZ_ADC_REG_ADWAIT);
>>  +	writew(2, adc->base + JZ_ADC_REG_ADSAME);
>>  +	writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
>>  +	writel(0, adc->base + JZ_ADC_REG_ADTCH);
>>  +
>>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL,
>>  +			       JZ_ADC_REG_CFG_CMD_SEL);
>>  +	ingenic_adc_set_adcmd(iio_dev, iio_dev->active_scan_mask[0]);
>>  +
>>  +	ingenic_adc_enable(adc, 2, true);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev)
>>  +{
>>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +
>>  +	ingenic_adc_enable(adc, 2, false);
>>  +
>>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, 0);
>>  +
>>  +	writeb(0xff, adc->base + JZ_ADC_REG_CTRL);
>>  +	writeb(0xff, adc->base + JZ_ADC_REG_STATUS);
>>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0);
>>  +	writew(0, adc->base + JZ_ADC_REG_ADSAME);
>>  +	writew(0, adc->base + JZ_ADC_REG_ADWAIT);
>>  +	clk_disable(adc->clk);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops 
>> = {
>>  +	.postenable = &ingenic_adc_buffer_enable,
>>  +	.predisable = &ingenic_adc_buffer_disable
>>  +};
>>  +
>>  +static irqreturn_t ingenic_adc_irq(int irq, void *data)
>>  +{
>>  +	struct iio_dev *iio_dev = data;
>>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +	unsigned long mask = iio_dev->active_scan_mask[0];
>>  +	unsigned int i;
>>  +	u32 tdat[3];
>>  +
>>  +	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
>>  +		if (mask & 0x3)
>>  +			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
>>  +		else
>>  +			tdat[i] = 0;
>>  +	}
>>  +
>>  +	iio_push_to_buffers(iio_dev, tdat);
>>  +	writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>   static int ingenic_adc_probe(struct platform_device *pdev)
>>   {
>>   	struct device *dev = &pdev->dev;
>>   	struct iio_dev *iio_dev;
>>   	struct ingenic_adc *adc;
>>   	const struct ingenic_adc_soc_data *soc_data;
>>  -	int ret;
>>  +	int irq, ret;
>> 
>>   	soc_data = device_get_match_data(dev);
>>   	if (!soc_data)
>>  @@ -511,6 +745,17 @@ static int ingenic_adc_probe(struct 
>> platform_device *pdev)
>>   	mutex_init(&adc->aux_lock);
>>   	adc->soc_data = soc_data;
>> 
>>  +	irq = platform_get_irq(pdev, 0);
>>  +	if (irq < 0)
>>  +		return irq;
>>  +
>>  +	ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0,
>>  +			       dev_name(dev), iio_dev);
>>  +	if (ret < 0) {
>>  +		dev_err(dev, "Failed to request irq: %d\n", ret);
>>  +		return ret;
>>  +	}
>>  +
>>   	adc->base = devm_platform_ioremap_resource(pdev, 0);
>>   	if (IS_ERR(adc->base))
>>   		return PTR_ERR(adc->base);
>>  @@ -550,7 +795,8 @@ static int ingenic_adc_probe(struct 
>> platform_device *pdev)
>> 
>>   	iio_dev->dev.parent = dev;
>>   	iio_dev->name = "jz-adc";
>>  -	iio_dev->modes = INDIO_DIRECT_MODE;
>>  +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
>>  +	iio_dev->setup_ops = &ingenic_buffer_setup_ops;
>>   	iio_dev->channels = soc_data->channels;
>>   	iio_dev->num_channels = soc_data->num_channels;
>>   	iio_dev->info = &ingenic_adc_info;
> 

Re: [PATCH v8 6/6] IIO: Ingenic JZ47xx: Add touchscreen mode.

[Jonathan Cameron]

On Sun, 12 Jul 2020 22:16:04 +0200
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi Jonathan,
> 
> Le dim. 12 juil. 2020 à 14:19, Jonathan Cameron <jic23@kernel.org> a 
> écrit :
> > On Thu,  9 Jul 2020 17:22:00 +0200
> > Artur Rojek <contact@artur-rojek.eu> wrote:
> >   
> >>  The SADC component in JZ47xx SoCs provides support for touchscreen
> >>  operations (pen position and pen down pressure) in single-ended and
> >>  differential modes.
> >> 
> >>  The touchscreen component of SADC takes a significant time to 
> >> stabilize
> >>  after first receiving the clock and a delay of 50ms has been 
> >> empirically
> >>  proven to be a safe value before data sampling can begin.
> >> 
> >>  Of the known hardware to use this controller, GCW Zero and Anbernic 
> >> RG-350
> >>  utilize the touchscreen mode by having their joystick(s) attached 
> >> to the
> >>  X/Y positive/negative input pins.
> >> 
> >>  JZ4770 and later SoCs introduce a low-level command feature. With 
> >> it, up
> >>  to 32 commands can be programmed, each one corresponding to a 
> >> sampling
> >>  job. It allows to change the low-voltage reference, the high-voltage
> >>  reference, have them connected to VCC, GND, or one of the X-/X+ or 
> >> Y-/Y+
> >>  pins.
> >> 
> >>  This patch introduces support for 6 stream-capable channels:
> >>  - channel #0 samples X+/GND
> >>  - channel #1 samples Y+/GND
> >>  - channel #2 samples X-/GND
> >>  - channel #3 samples Y-/GND
> >>  - channel #4 samples X+/X-
> >>  - channel #5 samples Y+/Y-  
> > 
> > The one thing I noticed on this read was that we are slightly 
> > stretching
> > the normal IIO channel definitions.  The claim is that each of these 
> > channels
> > is a POSITIONREALTIVE channel, whereas that isn't really true.  They 
> > are
> > related to the position, but as I understand it not directly 
> > measuring it
> > (particularly X+ - X-) which is pretty much a reference voltage (I 
> > think!)
> > 
> > We might be better off just describing these as voltage channels, with
> > 4 and 5 described as differential voltage channels.  The problem there
> > being that it doesn't describe the fact that the measurements are with
> > particular voltages also being applied to the touch screen.
> > 
> > Perhaps we are best off just leaving it as you have it and being a bit
> > 'odd'.  What do you think?  Having written this down I think perhaps 
> > leaving
> > it alone is the best plan :(  
> 
> That's up to you really. These ADC channels will measure voltage, but 
> they are supposed to be used with a touchscreen, that's why we went 
> with POSITIONRELATIVE. However, with the kind of devices we're working 
> with, they are never used as such, so using VOLTAGE would work too.
I've picked up v9 as I don't think this matters hugely one way or the other.
If it becomes an issue in the long run I'm not that bothered if we end up
with one driver with a legacy and a new interface.

Thanks,

Jonathan

> 
> > Otherwise this all looks fine to me.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> >> 
> >>  Being able to sample X-/GND and Y-/GND is useful on some devices, 
> >> where
> >>  one joystick is connected to the X+/Y+ pins, and a second joystick 
> >> is
> >>  connected to the X-/Y- pins.
> >> 
> >>  All the boards which probe this driver have the interrupt provided 
> >> from
> >>  Device Tree, with no need to handle a case where the IRQ was not 
> >> provided.
> >> 
> >>  Co-developed-by: Paul Cercueil <paul@crapouillou.net>
> >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
> >>  ---
> >> 
> >>   Changes:
> >> 
> >>   v2: - improve description of the touchscreen mode,
> >>       - get rid of the unneeded kfifo,
> >>       - drop IIO_BUFFER_CB from Kconfig,
> >>       - remove extended names from the touchscreen channels
> >> 
> >>   v3: remove unneeded `linux/iio/kfifo_buf.h` include
> >> 
> >>   v4: clarify irq provider source in the patch description
> >> 
> >>   v5: no change
> >> 
> >>   v6: - correct the spelling of Device Tree and IRQ in commit message
> >>       - don't omit trailing commas from initializer lists
> >>       - error check `clk_enable`
> >>       - remove redundant `dev_err` from `platform_get_irq` error 
> >> check
> >> 
> >>   v7: no change
> >> 
> >>   v8: add support for ADCMD low-level command feature
> >> 
> >>   drivers/iio/adc/Kconfig       |   1 +
> >>   drivers/iio/adc/ingenic-adc.c | 250 
> >> +++++++++++++++++++++++++++++++++-
> >>   2 files changed, 249 insertions(+), 2 deletions(-)
> >> 
> >>  diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> >>  index ff3569635ce0..5b57437cef75 100644
> >>  --- a/drivers/iio/adc/Kconfig
> >>  +++ b/drivers/iio/adc/Kconfig
> >>  @@ -500,6 +500,7 @@ config INA2XX_ADC
> >>   config INGENIC_ADC
> >>   	tristate "Ingenic JZ47xx SoCs ADC driver"
> >>   	depends on MIPS || COMPILE_TEST
> >>  +	select IIO_BUFFER
> >>   	help
> >>   	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC 
> >> unit.
> >> 
> >>  diff --git a/drivers/iio/adc/ingenic-adc.c 
> >> b/drivers/iio/adc/ingenic-adc.c
> >>  index 0233a9055c86..976aea46fede 100644
> >>  --- a/drivers/iio/adc/ingenic-adc.c
> >>  +++ b/drivers/iio/adc/ingenic-adc.c
> >>  @@ -8,7 +8,9 @@
> >> 
> >>   #include <dt-bindings/iio/adc/ingenic,adc.h>
> >>   #include <linux/clk.h>
> >>  +#include <linux/iio/buffer.h>
> >>   #include <linux/iio/iio.h>
> >>  +#include <linux/interrupt.h>
> >>   #include <linux/io.h>
> >>   #include <linux/iopoll.h>
> >>   #include <linux/kernel.h>
> >>  @@ -20,19 +22,46 @@
> >>   #define JZ_ADC_REG_CFG			0x04
> >>   #define JZ_ADC_REG_CTRL			0x08
> >>   #define JZ_ADC_REG_STATUS		0x0c
> >>  +#define JZ_ADC_REG_ADSAME		0x10
> >>  +#define JZ_ADC_REG_ADWAIT		0x14
> >>   #define JZ_ADC_REG_ADTCH		0x18
> >>   #define JZ_ADC_REG_ADBDAT		0x1c
> >>   #define JZ_ADC_REG_ADSDAT		0x20
> >>  +#define JZ_ADC_REG_ADCMD		0x24
> >>   #define JZ_ADC_REG_ADCLK		0x28
> >> 
> >>   #define JZ_ADC_REG_ENABLE_PD		BIT(7)
> >>   #define JZ_ADC_REG_CFG_AUX_MD		(BIT(0) | BIT(1))
> >>   #define JZ_ADC_REG_CFG_BAT_MD		BIT(4)
> >>  +#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
> >>  +#define JZ_ADC_REG_CFG_PULL_UP(n)	((n) << 16)
> >>  +#define JZ_ADC_REG_CFG_CMD_SEL		BIT(22)
> >>  +#define JZ_ADC_REG_CFG_TOUCH_OPS_MASK	(BIT(31) | GENMASK(23, 10))
> >>   #define JZ_ADC_REG_ADCLK_CLKDIV_LSB	0
> >>   #define JZ4725B_ADC_REG_ADCLK_CLKDIV10US_LSB	16
> >>   #define JZ4770_ADC_REG_ADCLK_CLKDIV10US_LSB	8
> >>   #define JZ4770_ADC_REG_ADCLK_CLKDIVMS_LSB	16
> >> 
> >>  +#define JZ_ADC_REG_ADCMD_YNADC		BIT(7)
> >>  +#define JZ_ADC_REG_ADCMD_YPADC		BIT(8)
> >>  +#define JZ_ADC_REG_ADCMD_XNADC		BIT(9)
> >>  +#define JZ_ADC_REG_ADCMD_XPADC		BIT(10)
> >>  +#define JZ_ADC_REG_ADCMD_VREFPYP	BIT(11)
> >>  +#define JZ_ADC_REG_ADCMD_VREFPXP	BIT(12)
> >>  +#define JZ_ADC_REG_ADCMD_VREFPXN	BIT(13)
> >>  +#define JZ_ADC_REG_ADCMD_VREFPAUX	BIT(14)
> >>  +#define JZ_ADC_REG_ADCMD_VREFPVDD33	BIT(15)
> >>  +#define JZ_ADC_REG_ADCMD_VREFNYN	BIT(16)
> >>  +#define JZ_ADC_REG_ADCMD_VREFNXP	BIT(17)
> >>  +#define JZ_ADC_REG_ADCMD_VREFNXN	BIT(18)
> >>  +#define JZ_ADC_REG_ADCMD_VREFAUX	BIT(19)
> >>  +#define JZ_ADC_REG_ADCMD_YNGRU		BIT(20)
> >>  +#define JZ_ADC_REG_ADCMD_XNGRU		BIT(21)
> >>  +#define JZ_ADC_REG_ADCMD_XPGRU		BIT(22)
> >>  +#define JZ_ADC_REG_ADCMD_YPSUP		BIT(23)
> >>  +#define JZ_ADC_REG_ADCMD_XNSUP		BIT(24)
> >>  +#define JZ_ADC_REG_ADCMD_XPSUP		BIT(25)
> >>  +
> >>   #define JZ_ADC_AUX_VREF				3300
> >>   #define JZ_ADC_AUX_VREF_BITS			12
> >>   #define JZ_ADC_BATTERY_LOW_VREF			2500
> >>  @@ -44,6 +73,14 @@
> >>   #define JZ4770_ADC_BATTERY_VREF			6600
> >>   #define JZ4770_ADC_BATTERY_VREF_BITS		12
> >> 
> >>  +#define JZ_ADC_IRQ_AUX			BIT(0)
> >>  +#define JZ_ADC_IRQ_BATTERY		BIT(1)
> >>  +#define JZ_ADC_IRQ_TOUCH		BIT(2)
> >>  +#define JZ_ADC_IRQ_PEN_DOWN		BIT(3)
> >>  +#define JZ_ADC_IRQ_PEN_UP		BIT(4)
> >>  +#define JZ_ADC_IRQ_PEN_DOWN_SLEEP	BIT(5)
> >>  +#define JZ_ADC_IRQ_SLEEP		BIT(7)
> >>  +
> >>   struct ingenic_adc;
> >> 
> >>   struct ingenic_adc_soc_data {
> >>  @@ -69,6 +106,61 @@ struct ingenic_adc {
> >>   	bool low_vref_mode;
> >>   };
> >> 
> >>  +static void ingenic_adc_set_adcmd(struct iio_dev *iio_dev, 
> >> unsigned long mask)
> >>  +{
> >>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
> >>  +
> >>  +	mutex_lock(&adc->lock);
> >>  +
> >>  +	/* Init ADCMD */
> >>  +	readl(adc->base + JZ_ADC_REG_ADCMD);
> >>  +
> >>  +	if (mask & 0x3) {
> >>  +		/* Second channel (INGENIC_ADC_TOUCH_YP): sample YP vs. GND */
> >>  +		writel(JZ_ADC_REG_ADCMD_XNGRU
> >>  +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_YPADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +
> >>  +		/* First channel (INGENIC_ADC_TOUCH_XP): sample XP vs. GND */
> >>  +		writel(JZ_ADC_REG_ADCMD_YNGRU
> >>  +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_XPADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +	}
> >>  +
> >>  +	if (mask & 0xc) {
> >>  +		/* Fourth channel (INGENIC_ADC_TOUCH_YN): sample YN vs. GND */
> >>  +		writel(JZ_ADC_REG_ADCMD_XNGRU
> >>  +		       | JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_YNADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +
> >>  +		/* Third channel (INGENIC_ADC_TOUCH_XN): sample XN vs. GND */
> >>  +		writel(JZ_ADC_REG_ADCMD_YNGRU
> >>  +		       | JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_XNADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +	}
> >>  +
> >>  +	if (mask & 0x30) {
> >>  +		/* Sixth channel (INGENIC_ADC_TOUCH_YD): sample YP vs. YN */
> >>  +		writel(JZ_ADC_REG_ADCMD_VREFNYN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_YPADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +
> >>  +		/* Fifth channel (INGENIC_ADC_TOUCH_XD): sample XP vs. XN */
> >>  +		writel(JZ_ADC_REG_ADCMD_VREFNXN | JZ_ADC_REG_ADCMD_VREFPVDD33
> >>  +		       | JZ_ADC_REG_ADCMD_XPADC,
> >>  +		       adc->base + JZ_ADC_REG_ADCMD);
> >>  +	}
> >>  +
> >>  +	/* We're done */
> >>  +	writel(0, adc->base + JZ_ADC_REG_ADCMD);
> >>  +
> >>  +	mutex_unlock(&adc->lock);
> >>  +}
> >>  +
> >>   static void ingenic_adc_set_config(struct ingenic_adc *adc,
> >>   				   uint32_t mask,
> >>   				   uint32_t val)
> >>  @@ -288,6 +380,72 @@ static const struct iio_chan_spec 
> >> jz4740_channels[] = {
> >>   };
> >> 
> >>   static const struct iio_chan_spec jz4770_channels[] = {
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_XP,  
> > 
> > Ah. sorry. I should have noticed this much earlier.
> > 
> > These aren't position channels as such.  They are channels
> > that are then processed into position in combination.
> > 
> > Problem is I'm not sure how we 'should' describe then.
> > 
> > Perhaps this is the best we can do.  
> 
> We could have the two differencial channels as IIO_POSITIONRELATIVE, 
> and the four single-ended ones as IIO_VOLTAGE.
> 
> Cheers,
> -Paul
> 
> >>  +		.scan_index = 0,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_YP,
> >>  +		.scan_index = 1,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_XN,
> >>  +		.scan_index = 2,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_YN,
> >>  +		.scan_index = 3,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_XD,
> >>  +		.scan_index = 4,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>  +	{
> >>  +		.type = IIO_POSITIONRELATIVE,
> >>  +		.indexed = 1,
> >>  +		.channel = INGENIC_ADC_TOUCH_YD,
> >>  +		.scan_index = 5,
> >>  +		.scan_type = {
> >>  +			.sign = 'u',
> >>  +			.realbits = 12,
> >>  +			.storagebits = 16,
> >>  +		},
> >>  +	},
> >>   	{
> >>   		.extend_name = "aux",
> >>   		.type = IIO_VOLTAGE,
> >>  @@ -490,13 +648,89 @@ static const struct iio_info ingenic_adc_info 
> >> = {
> >>   	.of_xlate = ingenic_adc_of_xlate,
> >>   };
> >> 
> >>  +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
> >>  +{
> >>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
> >>  +	int ret;
> >>  +
> >>  +	ret = clk_enable(adc->clk);
> >>  +	if (ret) {
> >>  +		dev_err(iio_dev->dev.parent, "Failed to enable clock: %d\n",
> >>  +			ret);
> >>  +		return ret;
> >>  +	}
> >>  +
> >>  +	/* It takes significant time for the touchscreen hw to stabilize. 
> >> */
> >>  +	msleep(50);
> >>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK,
> >>  +			       JZ_ADC_REG_CFG_SAMPLE_NUM(4) |
> >>  +			       JZ_ADC_REG_CFG_PULL_UP(4));
> >>  +
> >>  +	writew(80, adc->base + JZ_ADC_REG_ADWAIT);
> >>  +	writew(2, adc->base + JZ_ADC_REG_ADSAME);
> >>  +	writeb((u8)~JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_CTRL);
> >>  +	writel(0, adc->base + JZ_ADC_REG_ADTCH);
> >>  +
> >>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL,
> >>  +			       JZ_ADC_REG_CFG_CMD_SEL);
> >>  +	ingenic_adc_set_adcmd(iio_dev, iio_dev->active_scan_mask[0]);
> >>  +
> >>  +	ingenic_adc_enable(adc, 2, true);
> >>  +
> >>  +	return 0;
> >>  +}
> >>  +
> >>  +static int ingenic_adc_buffer_disable(struct iio_dev *iio_dev)
> >>  +{
> >>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
> >>  +
> >>  +	ingenic_adc_enable(adc, 2, false);
> >>  +
> >>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_CMD_SEL, 0);
> >>  +
> >>  +	writeb(0xff, adc->base + JZ_ADC_REG_CTRL);
> >>  +	writeb(0xff, adc->base + JZ_ADC_REG_STATUS);
> >>  +	ingenic_adc_set_config(adc, JZ_ADC_REG_CFG_TOUCH_OPS_MASK, 0);
> >>  +	writew(0, adc->base + JZ_ADC_REG_ADSAME);
> >>  +	writew(0, adc->base + JZ_ADC_REG_ADWAIT);
> >>  +	clk_disable(adc->clk);
> >>  +
> >>  +	return 0;
> >>  +}
> >>  +
> >>  +static const struct iio_buffer_setup_ops ingenic_buffer_setup_ops 
> >> = {
> >>  +	.postenable = &ingenic_adc_buffer_enable,
> >>  +	.predisable = &ingenic_adc_buffer_disable
> >>  +};
> >>  +
> >>  +static irqreturn_t ingenic_adc_irq(int irq, void *data)
> >>  +{
> >>  +	struct iio_dev *iio_dev = data;
> >>  +	struct ingenic_adc *adc = iio_priv(iio_dev);
> >>  +	unsigned long mask = iio_dev->active_scan_mask[0];
> >>  +	unsigned int i;
> >>  +	u32 tdat[3];
> >>  +
> >>  +	for (i = 0; i < ARRAY_SIZE(tdat); mask >>= 2, i++) {
> >>  +		if (mask & 0x3)
> >>  +			tdat[i] = readl(adc->base + JZ_ADC_REG_ADTCH);
> >>  +		else
> >>  +			tdat[i] = 0;
> >>  +	}
> >>  +
> >>  +	iio_push_to_buffers(iio_dev, tdat);
> >>  +	writeb(JZ_ADC_IRQ_TOUCH, adc->base + JZ_ADC_REG_STATUS);
> >>  +
> >>  +	return IRQ_HANDLED;
> >>  +}
> >>  +
> >>   static int ingenic_adc_probe(struct platform_device *pdev)
> >>   {
> >>   	struct device *dev = &pdev->dev;
> >>   	struct iio_dev *iio_dev;
> >>   	struct ingenic_adc *adc;
> >>   	const struct ingenic_adc_soc_data *soc_data;
> >>  -	int ret;
> >>  +	int irq, ret;
> >> 
> >>   	soc_data = device_get_match_data(dev);
> >>   	if (!soc_data)
> >>  @@ -511,6 +745,17 @@ static int ingenic_adc_probe(struct 
> >> platform_device *pdev)
> >>   	mutex_init(&adc->aux_lock);
> >>   	adc->soc_data = soc_data;
> >> 
> >>  +	irq = platform_get_irq(pdev, 0);
> >>  +	if (irq < 0)
> >>  +		return irq;
> >>  +
> >>  +	ret = devm_request_irq(dev, irq, ingenic_adc_irq, 0,
> >>  +			       dev_name(dev), iio_dev);
> >>  +	if (ret < 0) {
> >>  +		dev_err(dev, "Failed to request irq: %d\n", ret);
> >>  +		return ret;
> >>  +	}
> >>  +
> >>   	adc->base = devm_platform_ioremap_resource(pdev, 0);
> >>   	if (IS_ERR(adc->base))
> >>   		return PTR_ERR(adc->base);
> >>  @@ -550,7 +795,8 @@ static int ingenic_adc_probe(struct 
> >> platform_device *pdev)
> >> 
> >>   	iio_dev->dev.parent = dev;
> >>   	iio_dev->name = "jz-adc";
> >>  -	iio_dev->modes = INDIO_DIRECT_MODE;
> >>  +	iio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> >>  +	iio_dev->setup_ops = &ingenic_buffer_setup_ops;
> >>   	iio_dev->channels = soc_data->channels;
> >>   	iio_dev->num_channels = soc_data->num_channels;
> >>   	iio_dev->info = &ingenic_adc_info;  
> >   
> 
> 

Re: [PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Andy Shevchenko]

On Thu, Jul 9, 2020 at 6:22 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Hi all,
>
> v8 of this patchset introduces some structural changes, which I deemed
> worthy highlighting here:

Can you remind me if I gave you tags on the previous version?
If so, is the above the reason to drop them?

>
>  - adc-joystick related changes have been dropped from this patchset and
>    will be upstreamed separately. Their only connection to this patchset
>    was that they used INGENIC_ADC_TOUCH_* defines in the DTS example,
>    causing trouble to Rob's scripts.
>
>  - Integrated Paul's changes, which introduce an ADCMD low-level command
>    feature. These changes affect patches 5/6 and 6/6, with the former
>    requiring Rob to re-ack.
>
> Cheers,
> Artur
>
> Artur Rojek (5):
>   dt-bindings: iio/adc: Convert ingenic-adc docs to YAML.
>   IIO: Ingenic JZ47xx: Error check clk_enable calls.
>   IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx
>   dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC
>   IIO: Ingenic JZ47xx: Add touchscreen mode.
>
> Paul Cercueil (1):
>   iio/adc: ingenic: Retrieve channels list from soc data struct
>
>  .../bindings/iio/adc/ingenic,adc.txt          |  49 ---
>  .../bindings/iio/adc/ingenic,adc.yaml         |  71 ++++
>  drivers/iio/adc/Kconfig                       |   1 +
>  drivers/iio/adc/ingenic-adc.c                 | 386 ++++++++++++++++--
>  include/dt-bindings/iio/adc/ingenic,adc.h     |   6 +
>  5 files changed, 426 insertions(+), 87 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
>
> --
> 2.27.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Artur Rojek]

Hey Andy,

On 2020-07-09 17:43, Andy Shevchenko wrote:
> On Thu, Jul 9, 2020 at 6:22 PM Artur Rojek <contact@artur-rojek.eu> 
> wrote:
>> 
>> Hi all,
>> 
>> v8 of this patchset introduces some structural changes, which I deemed
>> worthy highlighting here:
> 
> Can you remind me if I gave you tags on the previous version?
I received no tags from you on this patchset so far.

Cheers,
Artur

> If so, is the above the reason to drop them?
> 
>> 
>>  - adc-joystick related changes have been dropped from this patchset 
>> and
>>    will be upstreamed separately. Their only connection to this 
>> patchset
>>    was that they used INGENIC_ADC_TOUCH_* defines in the DTS example,
>>    causing trouble to Rob's scripts.
>> 
>>  - Integrated Paul's changes, which introduce an ADCMD low-level 
>> command
>>    feature. These changes affect patches 5/6 and 6/6, with the former
>>    requiring Rob to re-ack.
>> 
>> Cheers,
>> Artur
>> 
>> Artur Rojek (5):
>>   dt-bindings: iio/adc: Convert ingenic-adc docs to YAML.
>>   IIO: Ingenic JZ47xx: Error check clk_enable calls.
>>   IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx
>>   dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC
>>   IIO: Ingenic JZ47xx: Add touchscreen mode.
>> 
>> Paul Cercueil (1):
>>   iio/adc: ingenic: Retrieve channels list from soc data struct
>> 
>>  .../bindings/iio/adc/ingenic,adc.txt          |  49 ---
>>  .../bindings/iio/adc/ingenic,adc.yaml         |  71 ++++
>>  drivers/iio/adc/Kconfig                       |   1 +
>>  drivers/iio/adc/ingenic-adc.c                 | 386 
>> ++++++++++++++++--
>>  include/dt-bindings/iio/adc/ingenic,adc.h     |   6 +
>>  5 files changed, 426 insertions(+), 87 deletions(-)
>>  delete mode 100644 
>> Documentation/devicetree/bindings/iio/adc/ingenic,adc.txt
>>  create mode 100644 
>> Documentation/devicetree/bindings/iio/adc/ingenic,adc.yaml
>> 
>> --
>> 2.27.0
>> 

Re: [PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Andy Shevchenko]

On Thu, Jul 9, 2020 at 7:05 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Hey Andy,
>
> On 2020-07-09 17:43, Andy Shevchenko wrote:
> > On Thu, Jul 9, 2020 at 6:22 PM Artur Rojek <contact@artur-rojek.eu>
> > wrote:
> >>
> >> Hi all,
> >>
> >> v8 of this patchset introduces some structural changes, which I deemed
> >> worthy highlighting here:
> >
> > Can you remind me if I gave you tags on the previous version?
> I received no tags from you on this patchset so far.

Thanks for reminding me!



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Heiko Stuebner]

Hi Artur,

Am Donnerstag, 9. Juli 2020, 17:21:54 CEST schrieb Artur Rojek:
> Hi all,
> 
> v8 of this patchset introduces some structural changes, which I deemed
> worthy highlighting here:
> 
>  - adc-joystick related changes have been dropped from this patchset and
>    will be upstreamed separately. Their only connection to this patchset
>    was that they used INGENIC_ADC_TOUCH_* defines in the DTS example,
>    causing trouble to Rob's scripts.

as I'm mainly eyeing your adc-joystick patch ... did you post that already
somewhere - separately as you wrote?

Thanks
Heiko

Re: [PATCH v8 0/6] iio/adc: ingenic: Cleanups & add touchscreen mode.

[Artur Rojek]

On 2020-07-14 20:33, Heiko Stuebner wrote:
> Hi Artur,
> 
> Am Donnerstag, 9. Juli 2020, 17:21:54 CEST schrieb Artur Rojek:
>> Hi all,
>> 
>> v8 of this patchset introduces some structural changes, which I deemed
>> worthy highlighting here:
>> 
>>  - adc-joystick related changes have been dropped from this patchset 
>> and
>>    will be upstreamed separately. Their only connection to this 
>> patchset
>>    was that they used INGENIC_ADC_TOUCH_* defines in the DTS example,
>>    causing trouble to Rob's scripts.
> 
> as I'm mainly eyeing your adc-joystick patch ... did you post that 
> already
> somewhere - separately as you wrote?
> 
> Thanks
> Heiko

Hi Heiko,

sorry for a late reply. As you have surely noticed in your mailbox, I 
have sent the adc-joystick changes in the following series:
https://lore.kernel.org/linux-input/20200719221103.91644-1-contact@artur-rojek.eu/

Regards,
Artur