[PATCH v4 0/3] Add support for touchscreen on Colibri VF50

[PATCH v4 0/3] Add support for touchscreen on Colibri VF50

[Sanchayan Maity]

Hello,

The patchset adds support for 4 wire touchscreen on Toradex Colibri
VF50 modules.

Thanks Dmitry for your feedback.

Changes since v3:
1. Add a #define for the average readings measurement.
2. Instead of configuring gpios every open, configure them on request.
3. Use a memory barrier and synchronize irq call to make sure IRQ is
not running past close.
4. Drop inline for vf50_ts_get_gpiod
5. Convert "ret" to "error"
6. Use devm_add_action to add a function for releasing iio channels and
drop remove completely.
7. Kill __set_bits()
8. Instead of requesting for pen irq as gpio and then irq, use the
interrupts property for DT and let it be handled automatically. Anyways
we never read it's state.
9. Just use IRQF_ONESHOT here, rely on the platform/OF to read and set up
the trigger properly (via "interrupts" property).

Changes since v2:
1. Fix pin multiplexing for pins in idle state. Configuration of the
pen detect pull up viz. PTA19__GPIO_9 resulted in generation of pen
irq's on a continuous basis.
2. Fix pinmux of the ADC pins as per the recommended pinmux in TRM.
3. Use a threaded irq handler instead of a irq handler plus workqueue
approach.
4. Use a low level trigger with oneshot flag specifier instead of the
previous falling edge triggered irq's. This coupled with the fix in
point 1 fixes the previous continuous spurious irq generation bug.
5. Change/fix the TS measurement logic to account for the fact that
iio_channel_read_raw might actually return an error. To be more
specific use break instead of continue and take care to close the
FET's in case of channel read error.
6. Drop the first patch "Add io-channel-cells property for ADC node"
as it has already been applied.
7. Move the iio channel get call again at the start. Having it in
the end resulted in crashes sometimes when iio was not probed and
the ts device got probed and opened earlier.

Changes since v1:
1. Fix/drop comments
2. Use an inline function for multiple gpiod_get calls in probe
3. Remove the pull up in the pinmux specified in DT for touchctrl_gpios
4. Add the io-channel-cells property before status property.
5. Add GPIOLIB as dependency in the Kconfig file

Version 3 of the patchset can be found here
https://lkml.org/lkml/2015/8/5/173

Version 2 of the patchset can be found here
https://www.mail-archive.com/linux-input@vger.kernel.org/msg18090.html

Version 1 of the patchset can be found here
https://lkml.org/lkml/2015/6/30/103

Thank you very much for the feedback till now.

Regards,
Sanchayan.

Sanchayan Maity (3):
  ARM: dts: vf500-colibri: Add device tree node for touchscreen support
  input: Add DT binding documentation for Colibri VF50 touchscreen
  touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

 .../bindings/input/touchscreen/colibri-vf50-ts.txt |  36 ++
 arch/arm/boot/dts/vf500-colibri-eval-v3.dts        |   4 +
 arch/arm/boot/dts/vf500-colibri.dtsi               |  47 +++
 drivers/input/touchscreen/Kconfig                  |  12 +
 drivers/input/touchscreen/Makefile                 |   1 +
 drivers/input/touchscreen/colibri-vf50-ts.c        | 370 +++++++++++++++++++++
 6 files changed, 470 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
 create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c

-- 
2.5.0

[PATCH v4 1/3] ARM: dts: vf500-colibri: Add device tree node for touchscreen support

[Sanchayan Maity]

Add device tree node for touchscreen support on Colibri VF50. The
touchscreen functionality on VF50 uses the ADC channels of Vybrid
and some GPIOs. Also add pinctrl nodes for proper pinmux.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 arch/arm/boot/dts/vf500-colibri-eval-v3.dts |  4 +++
 arch/arm/boot/dts/vf500-colibri.dtsi        | 47 +++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
index 7fc782c..14c0b00 100644
--- a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
+++ b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
@@ -15,3 +15,7 @@
 	model = "Toradex Colibri VF50 on Colibri Evaluation Board";
 	compatible = "toradex,vf500-colibri_vf50-on-eval", "toradex,vf500-colibri_vf50", "fsl,vf500";
 };
+
+&touchscreen {
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/vf500-colibri.dtsi b/arch/arm/boot/dts/vf500-colibri.dtsi
index cee34a3..84f091d 100644
--- a/arch/arm/boot/dts/vf500-colibri.dtsi
+++ b/arch/arm/boot/dts/vf500-colibri.dtsi
@@ -17,4 +17,51 @@
 	memory {
 		reg = <0x80000000 0x8000000>;
 	};
+
+	touchscreen: vf50-touchscreen {
+		compatible = "toradex,vf50-touchscreen";
+		io-channels = <&adc1 0>,<&adc0 0>,
+				<&adc0 1>,<&adc1 2>;
+		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
+		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
+		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
+		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "idle","default","gpios";
+		pinctrl-0 = <&pinctrl_touchctrl_idle>;
+		pinctrl-1 = <&pinctrl_touchctrl_default>;
+		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
+		vf50-ts-min-pressure = <200>;
+		status = "disabled";
+	};
+};
+
+&iomuxc {
+	vf610-colibri {
+		pinctrl_touchctrl_idle: touchctrl_idle {
+			fsl,pins = <
+				VF610_PAD_PTA18__GPIO_8		0x006d
+				VF610_PAD_PTA19__GPIO_9		0x006c
+				>;
+		};
+
+		pinctrl_touchctrl_default: touchctrl_default {
+			fsl,pins = <
+				VF610_PAD_PTA18__ADC0_SE0	0x0040
+				VF610_PAD_PTA19__ADC0_SE1	0x0040
+				VF610_PAD_PTA16__ADC1_SE0	0x0040
+				VF610_PAD_PTB2__ADC1_SE2	0x0040
+				>;
+		};
+
+		pinctrl_touchctrl_gpios: touchctrl_gpios {
+			fsl,pins = <
+				VF610_PAD_PTA23__GPIO_13	0x22e9
+				VF610_PAD_PTB23__GPIO_93	0x22e9
+				VF610_PAD_PTA22__GPIO_12	0x22e9
+				VF610_PAD_PTA11__GPIO_4		0x22e9
+				>;
+		};
+	};
 };
-- 
2.5.0

Re: [PATCH v4 1/3] ARM: dts: vf500-colibri: Add device tree node for touchscreen support

[Stefan Agner]

On 2015-08-21 06:26, Sanchayan Maity wrote:
> Add device tree node for touchscreen support on Colibri VF50. The
> touchscreen functionality on VF50 uses the ADC channels of Vybrid
> and some GPIOs. Also add pinctrl nodes for proper pinmux.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vf500-colibri-eval-v3.dts |  4 +++
>  arch/arm/boot/dts/vf500-colibri.dtsi        | 47 +++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> index 7fc782c..14c0b00 100644
> --- a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> +++ b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> @@ -15,3 +15,7 @@
>  	model = "Toradex Colibri VF50 on Colibri Evaluation Board";
>  	compatible = "toradex,vf500-colibri_vf50-on-eval",
> "toradex,vf500-colibri_vf50", "fsl,vf500";
>  };
> +
> +&touchscreen {
> +	status = "okay";
> +};
> diff --git a/arch/arm/boot/dts/vf500-colibri.dtsi
> b/arch/arm/boot/dts/vf500-colibri.dtsi
> index cee34a3..84f091d 100644
> --- a/arch/arm/boot/dts/vf500-colibri.dtsi
> +++ b/arch/arm/boot/dts/vf500-colibri.dtsi
> @@ -17,4 +17,51 @@
>  	memory {
>  		reg = <0x80000000 0x8000000>;
>  	};
> +
> +	touchscreen: vf50-touchscreen {
> +		compatible = "toradex,vf50-touchscreen";
> +		io-channels = <&adc1 0>,<&adc0 0>,
> +				<&adc0 1>,<&adc1 2>;
> +		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
> +		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
> +		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> +		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-names = "idle","default","gpios";
> +		pinctrl-0 = <&pinctrl_touchctrl_idle>;
> +		pinctrl-1 = <&pinctrl_touchctrl_default>;
> +		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
> +		vf50-ts-min-pressure = <200>;

Since this is a touch screen related property, we even would want to
have that in the board specific device-tree (vf500-colibri-eval-v3.dts).


--
Stefan

Re: [PATCH v4 1/3] ARM: dts: vf500-colibri: Add device tree node for touchscreen support

[maitysanchayan]

On 15-08-22 18:54:28, Stefan Agner wrote:
> On 2015-08-21 06:26, Sanchayan Maity wrote:
> > Add device tree node for touchscreen support on Colibri VF50. The
> > touchscreen functionality on VF50 uses the ADC channels of Vybrid
> > and some GPIOs. Also add pinctrl nodes for proper pinmux.
> > 
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> >  arch/arm/boot/dts/vf500-colibri-eval-v3.dts |  4 +++
> >  arch/arm/boot/dts/vf500-colibri.dtsi        | 47 +++++++++++++++++++++++++++++
> >  2 files changed, 51 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> > b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> > index 7fc782c..14c0b00 100644
> > --- a/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> > +++ b/arch/arm/boot/dts/vf500-colibri-eval-v3.dts
> > @@ -15,3 +15,7 @@
> >  	model = "Toradex Colibri VF50 on Colibri Evaluation Board";
> >  	compatible = "toradex,vf500-colibri_vf50-on-eval",
> > "toradex,vf500-colibri_vf50", "fsl,vf500";
> >  };
> > +
> > +&touchscreen {
> > +	status = "okay";
> > +};
> > diff --git a/arch/arm/boot/dts/vf500-colibri.dtsi
> > b/arch/arm/boot/dts/vf500-colibri.dtsi
> > index cee34a3..84f091d 100644
> > --- a/arch/arm/boot/dts/vf500-colibri.dtsi
> > +++ b/arch/arm/boot/dts/vf500-colibri.dtsi
> > @@ -17,4 +17,51 @@
> >  	memory {
> >  		reg = <0x80000000 0x8000000>;
> >  	};
> > +
> > +	touchscreen: vf50-touchscreen {
> > +		compatible = "toradex,vf50-touchscreen";
> > +		io-channels = <&adc1 0>,<&adc0 0>,
> > +				<&adc0 1>,<&adc1 2>;
> > +		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
> > +		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
> > +		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> > +		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> > +		interrupt-parent = <&gpio0>;
> > +		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> > +		pinctrl-names = "idle","default","gpios";
> > +		pinctrl-0 = <&pinctrl_touchctrl_idle>;
> > +		pinctrl-1 = <&pinctrl_touchctrl_default>;
> > +		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
> > +		vf50-ts-min-pressure = <200>;
> 
> Since this is a touch screen related property, we even would want to
> have that in the board specific device-tree (vf500-colibri-eval-v3.dts).

So the same property specified again in vf500-colibri-eval-v3.dts? Ok will
include the same.

- Sanchayan.

[PATCH v4 2/3] input: Add DT binding documentation for Colibri VF50 touchscreen

[Sanchayan Maity]

This adds device tree binding documentation for the Colibri VF50
touchscreen driver.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 .../bindings/input/touchscreen/colibri-vf50-ts.txt | 36 ++++++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt

diff --git a/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
new file mode 100644
index 0000000..e615aa9
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
@@ -0,0 +1,36 @@
+* Toradex Colibri VF50 Touchscreen driver
+
+Required Properties:
+- compatible must be toradex,vf50-touchscreen
+- io-channels: adc channels being used by the Colibri VF50 module
+- xp-gpios: FET gate driver for input of X+
+- xm-gpios: FET gate driver for input of X-
+- yp-gpios: FET gate driver for input of Y+
+- ym-gpios: FET gate driver for input of Y-
+- interrupt-parent: phandle for the interrupt controller
+- interrupts: pen irq interrupt for touch detection
+- pinctrl-names: "idle", "default", "gpios"
+- pinctrl-0: pinctrl node for idle state gpio pinmux
+- pinctrl-1: pinctrl node for touch detection state pinmux
+- pinctrl-2: pinctrl node for gpios functioning as FET gate drivers
+- vf50-ts-min-pressure: pressure level at which to stop measuring X/Y values
+
+Example:
+
+	touchctrl: vf50_touchctrl {
+		compatible = "toradex,vf50-touchscreen";
+		io-channels = <&adc1 0>,<&adc0 0>,
+				<&adc0 1>,<&adc1 2>;
+		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
+		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
+		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
+		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+		pinctrl-names = "idle","default","gpios";
+		pinctrl-0 = <&pinctrl_touchctrl_idle>;
+		pinctrl-1 = <&pinctrl_touchctrl_default>;
+		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
+		vf50-ts-min-pressure = <200>;
+		status = "disabled";
+	};
-- 
2.5.0

Re: [PATCH v4 2/3] input: Add DT binding documentation for Colibri VF50 touchscreen

[Stefan Agner]

On 2015-08-21 06:26, Sanchayan Maity wrote:
> This adds device tree binding documentation for the Colibri VF50
> touchscreen driver.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  .../bindings/input/touchscreen/colibri-vf50-ts.txt | 36 ++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> new file mode 100644
> index 0000000..e615aa9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> @@ -0,0 +1,36 @@
> +* Toradex Colibri VF50 Touchscreen driver
> +
> +Required Properties:
> +- compatible must be toradex,vf50-touchscreen
> +- io-channels: adc channels being used by the Colibri VF50 module
> +- xp-gpios: FET gate driver for input of X+
> +- xm-gpios: FET gate driver for input of X-
> +- yp-gpios: FET gate driver for input of Y+
> +- ym-gpios: FET gate driver for input of Y-
> +- interrupt-parent: phandle for the interrupt controller
> +- interrupts: pen irq interrupt for touch detection

I like the use of the interrupt interface for the touch detection pin.
Maybe you can mention here that this is (typically) a GPIO...

> +- pinctrl-names: "idle", "default", "gpios"
> +- pinctrl-0: pinctrl node for idle state gpio pinmux
> +- pinctrl-1: pinctrl node for touch detection state pinmux

"touch detection" for default is rather confusing or even wrong. The
Idle state is used for pen/touch detection, default is used for X/Y and
Pressure measurement (maybe better described as touch measurement).

> +- pinctrl-2: pinctrl node for gpios functioning as FET gate drivers
> +- vf50-ts-min-pressure: pressure level at which to stop measuring X/Y values
> +
> +Example:
> +
> +	touchctrl: vf50_touchctrl {
> +		compatible = "toradex,vf50-touchscreen";
> +		io-channels = <&adc1 0>,<&adc0 0>,
> +				<&adc0 1>,<&adc1 2>;
> +		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
> +		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
> +		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> +		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-names = "idle","default","gpios";
> +		pinctrl-0 = <&pinctrl_touchctrl_idle>;
> +		pinctrl-1 = <&pinctrl_touchctrl_default>;
> +		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
> +		vf50-ts-min-pressure = <200>;
> +		status = "disabled";
> +	};

Otherwise I agree with 1 and 2:

Acked-by: Stefan Agner <stefan@agner.ch>

--
Stefan

Re: [PATCH v4 2/3] input: Add DT binding documentation for Colibri VF50 touchscreen

[maitysanchayan]

On 15-08-22 18:36:54, Stefan Agner wrote:
> On 2015-08-21 06:26, Sanchayan Maity wrote:
> > This adds device tree binding documentation for the Colibri VF50
> > touchscreen driver.
> > 
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> >  .../bindings/input/touchscreen/colibri-vf50-ts.txt | 36 ++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> > b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> > new file mode 100644
> > index 0000000..e615aa9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/colibri-vf50-ts.txt
> > @@ -0,0 +1,36 @@
> > +* Toradex Colibri VF50 Touchscreen driver
> > +
> > +Required Properties:
> > +- compatible must be toradex,vf50-touchscreen
> > +- io-channels: adc channels being used by the Colibri VF50 module
> > +- xp-gpios: FET gate driver for input of X+
> > +- xm-gpios: FET gate driver for input of X-
> > +- yp-gpios: FET gate driver for input of Y+
> > +- ym-gpios: FET gate driver for input of Y-
> > +- interrupt-parent: phandle for the interrupt controller
> > +- interrupts: pen irq interrupt for touch detection
> 
> I like the use of the interrupt interface for the touch detection pin.
> Maybe you can mention here that this is (typically) a GPIO...
> 
> > +- pinctrl-names: "idle", "default", "gpios"
> > +- pinctrl-0: pinctrl node for idle state gpio pinmux
> > +- pinctrl-1: pinctrl node for touch detection state pinmux
> 
> "touch detection" for default is rather confusing or even wrong. The
> Idle state is used for pen/touch detection, default is used for X/Y and
> Pressure measurement (maybe better described as touch measurement).

Ok. Understood. I see how my wordings can create confusion or are
misleading. Will fix.

- Sanchayan.

> 
> > +- pinctrl-2: pinctrl node for gpios functioning as FET gate drivers
> > +- vf50-ts-min-pressure: pressure level at which to stop measuring X/Y values
> > +
> > +Example:
> > +
> > +	touchctrl: vf50_touchctrl {
> > +		compatible = "toradex,vf50-touchscreen";
> > +		io-channels = <&adc1 0>,<&adc0 0>,
> > +				<&adc0 1>,<&adc1 2>;
> > +		xp-gpios = <&gpio0 13 GPIO_ACTIVE_LOW>;
> > +		xm-gpios = <&gpio2 29 GPIO_ACTIVE_HIGH>;
> > +		yp-gpios = <&gpio0 12 GPIO_ACTIVE_LOW>;
> > +		ym-gpios = <&gpio0 4 GPIO_ACTIVE_HIGH>;
> > +		interrupt-parent = <&gpio0>;
> > +		interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> > +		pinctrl-names = "idle","default","gpios";
> > +		pinctrl-0 = <&pinctrl_touchctrl_idle>;
> > +		pinctrl-1 = <&pinctrl_touchctrl_default>;
> > +		pinctrl-2 = <&pinctrl_touchctrl_gpios>;
> > +		vf50-ts-min-pressure = <200>;
> > +		status = "disabled";
> > +	};
> 
> Otherwise I agree with 1 and 2:
> 
> Acked-by: Stefan Agner <stefan@agner.ch>
> 
> --
> Stefan
> 

[PATCH v4 3/3] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Sanchayan Maity]

The Colibri Vybrid VF50 module supports 4-wire touchscreens using
FETs and ADC inputs. This driver uses the IIO consumer interface
and relies on the vf610_adc driver based on the IIO framework.

Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
---
 drivers/input/touchscreen/Kconfig           |  12 +
 drivers/input/touchscreen/Makefile          |   1 +
 drivers/input/touchscreen/colibri-vf50-ts.c | 370 ++++++++++++++++++++++++++++
 3 files changed, 383 insertions(+)
 create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c

diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
index 80f6386..28948ca 100644
--- a/drivers/input/touchscreen/Kconfig
+++ b/drivers/input/touchscreen/Kconfig
@@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
 	  To compile this driver as a module, choose M here: the
 	  module will be called zforce_ts.
 
+config TOUCHSCREEN_COLIBRI_VF50
+	tristate "Toradex Colibri on board touchscreen driver"
+	depends on GPIOLIB && IIO && VF610_ADC
+	help
+	  Say Y here if you have a Colibri VF50 and plan to use
+	  the on-board provided 4-wire touchscreen driver.
+
+	  If unsure, say N.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called colibri_vf50_ts.
+
 endif
diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
index 44deea7..93746a0 100644
--- a/drivers/input/touchscreen/Makefile
+++ b/drivers/input/touchscreen/Makefile
@@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
 obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
 obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
 obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
+obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
new file mode 100644
index 0000000..0793fdc
--- /dev/null
+++ b/drivers/input/touchscreen/colibri-vf50-ts.c
@@ -0,0 +1,370 @@
+/* Copyright 2015 Toradex AG
+ *
+ * Toradex Colibri VF50 Touchscreen driver
+ *
+ * Originally authored by Stefan Agner for 3.0 kernel
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/iio/consumer.h>
+#include <linux/iio/types.h>
+#include <linux/input.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define DRIVER_NAME "colibri-vf50-ts"
+#define DRV_VERSION "1.0"
+
+#define VF_ADC_MAX ((1 << 12) - 1)
+
+#define COLI_TOUCH_MIN_DELAY_US 1000
+#define COLI_TOUCH_MAX_DELAY_US 2000
+#define COLI_TOUCH_NO_OF_AVGS	5
+
+struct vf50_touch_device {
+	struct platform_device	*pdev;
+	struct input_dev	*ts_input;
+	struct iio_channel	*channels;
+	struct gpio_desc *gpio_xp;
+	struct gpio_desc *gpio_xm;
+	struct gpio_desc *gpio_yp;
+	struct gpio_desc *gpio_ym;
+	struct gpio_desc *gpio_pen_detect;
+	int pen_irq;
+	int min_pressure;
+	bool stop_touchscreen;
+};
+
+/*
+ * Enables given plates and measures touch parameters using ADC
+ */
+static int adc_ts_measure(struct iio_channel *channel,
+		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
+{
+	int i, value = 0, val = 0;
+	int ret;
+
+	gpiod_set_value(plate_p, 1);
+	gpiod_set_value(plate_m, 1);
+
+	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
+
+	for (i = 0; i < COLI_TOUCH_NO_OF_AVGS; i++) {
+		ret = iio_read_channel_raw(channel, &val);
+		if (ret < 0) {
+			value = ret;
+			goto error_iio_read;
+		}
+
+		value += val;
+	}
+
+	value /= COLI_TOUCH_NO_OF_AVGS;
+
+error_iio_read:
+	gpiod_set_value(plate_p, 0);
+	gpiod_set_value(plate_m, 0);
+
+	return value;
+}
+
+/*
+ * Enable touch detection using falling edge detection on XM
+ */
+static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
+{
+	/* Enable plate YM (needs to be strong GND, high active) */
+	gpiod_set_value(vf50_ts->gpio_ym, 1);
+
+	/*
+	 * Let the platform mux to idle state in order to enable
+	 * Pull-Up on GPIO
+	 */
+	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
+}
+
+/*
+ * ADC touch screen sampling bottom half irq handler
+ */
+static irqreturn_t vf50_ts_irq_bh(int irq, void *private)
+{
+	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)private;
+	struct device *dev = &vf50_ts->pdev->dev;
+	int val_x, val_y, val_z1, val_z2, val_p = 0;
+	bool discard_val_on_start = true;
+
+	/* Disable the touch detection plates */
+	gpiod_set_value(vf50_ts->gpio_ym, 0);
+
+	/* Let the platform mux to default state in order to mux as ADC */
+	pinctrl_pm_select_default_state(dev);
+
+	while (!vf50_ts->stop_touchscreen) {
+		/* X-Direction */
+		val_x = adc_ts_measure(&vf50_ts->channels[0],
+				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
+		if (val_x < 0)
+			break;
+
+		/* Y-Direction */
+		val_y = adc_ts_measure(&vf50_ts->channels[1],
+				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
+		if (val_y < 0)
+			break;
+
+		/*
+		 * Touch pressure
+		 * Measure on XP/YM
+		 */
+		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
+				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
+		if (val_z1 < 0)
+			break;
+		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
+				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
+		if (val_z2 < 0)
+			break;
+
+		/* Validate signal (avoid calculation using noise) */
+		if (val_z1 > 64 && val_x > 64) {
+			/*
+			 * Calculate resistance between the plates
+			 * lower resistance means higher pressure
+			 */
+			int r_x = (1000 * val_x) / VF_ADC_MAX;
+
+			val_p = (r_x * val_z2) / val_z1 - r_x;
+
+		} else {
+			val_p = 2000;
+		}
+
+		val_p = 2000 - val_p;
+		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
+			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
+
+		/*
+		 * If touch pressure is too low, stop measuring and reenable
+		 * touch detection
+		 */
+		if (val_p < vf50_ts->min_pressure || val_p > 2000)
+			break;
+
+		/*
+		 * The pressure may not be enough for the first x and the
+		 * second y measurement, but, the pressure is ok when the
+		 * driver is doing the third and fourth measurement. To
+		 * take care of this, we drop the first measurement always.
+		 */
+		if (discard_val_on_start) {
+			discard_val_on_start = false;
+		} else {
+			/*
+			 * Report touch position and sleep for
+			 * next measurement
+			 */
+			input_report_abs(vf50_ts->ts_input,
+					ABS_X, VF_ADC_MAX - val_x);
+			input_report_abs(vf50_ts->ts_input,
+					ABS_Y, VF_ADC_MAX - val_y);
+			input_report_abs(vf50_ts->ts_input,
+					ABS_PRESSURE, val_p);
+			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
+			input_sync(vf50_ts->ts_input);
+		}
+
+		msleep(10);
+	}
+
+	/* Report no more touch, reenable touch detection */
+	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
+	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
+	input_sync(vf50_ts->ts_input);
+
+	vf50_ts_enable_touch_detection(vf50_ts);
+
+	/* Wait for the pull-up to be stable on high */
+	msleep(10);
+
+	return IRQ_HANDLED;
+}
+
+static int vf50_ts_open(struct input_dev *dev_input)
+{
+	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
+	struct device *dev = &touchdev->pdev->dev;
+
+	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
+			dev_input->name);
+
+	touchdev->stop_touchscreen = false;
+
+	/* Mux detection before request IRQ, wait for pull-up to settle */
+	vf50_ts_enable_touch_detection(touchdev);
+	msleep(10);
+
+	return 0;
+}
+
+static void vf50_ts_close(struct input_dev *dev_input)
+{
+	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
+	struct device *dev = &touchdev->pdev->dev;
+
+	touchdev->stop_touchscreen = true;
+
+	/* Make sure IRQ is not running past close */
+	mb();
+	synchronize_irq(touchdev->pen_irq);
+
+	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
+		dev_input->name);
+}
+
+static int vf50_ts_get_gpiod(struct device *dev,
+	struct gpio_desc **gpio_d, const char *con_id, enum gpiod_flags flags)
+{
+	int error;
+
+	*gpio_d = devm_gpiod_get(dev, con_id, flags);
+	if (IS_ERR(*gpio_d)) {
+		error = PTR_ERR(*gpio_d);
+		dev_err(dev, "Could not get gpio_%s %d\n", con_id, error);
+		return error;
+	}
+
+	return 0;
+}
+
+static void vf50_ts_channel_release(void *data)
+{
+	struct vf50_touch_device *touchdev = data;
+
+	iio_channel_release_all(touchdev->channels);
+}
+
+static int vf50_ts_probe(struct platform_device *pdev)
+{
+	struct input_dev *input;
+	struct iio_channel *channels;
+	struct device *dev = &pdev->dev;
+	struct vf50_touch_device *touchdev;
+	int error;
+
+	channels = iio_channel_get_all(dev);
+	if (IS_ERR(channels))
+		return PTR_ERR(channels);
+
+	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
+	if (!touchdev) {
+		error = -ENOMEM;
+		return error;
+	}
+
+	error = of_property_read_u32(dev->of_node, "vf50-ts-min-pressure",
+				 &touchdev->min_pressure);
+	if (error)
+		return error;
+
+	error = devm_add_action(dev, vf50_ts_channel_release, channels);
+	if (error) {
+		dev_err(dev, "Failed to register iio channel release action");
+		return error;
+	}
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "Failed to allocate TS input device\n");
+		error = -ENOMEM;
+		return error;
+	}
+
+	platform_set_drvdata(pdev, touchdev);
+
+	touchdev->pdev = pdev;
+	touchdev->channels = channels;
+
+	input->name = DRIVER_NAME;
+	input->id.bustype = BUS_HOST;
+	input->dev.parent = dev;
+	input->open = vf50_ts_open;
+	input->close = vf50_ts_close;
+
+	input_set_capability(input, EV_KEY, BTN_TOUCH);
+	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
+	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
+	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
+
+	touchdev->ts_input = input;
+	input_set_drvdata(input, touchdev);
+	error = input_register_device(input);
+	if (error) {
+		dev_err(dev, "Failed to register input device\n");
+		return error;
+	}
+
+	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp,
+				"xp", GPIOD_OUT_LOW);
+	if (error)
+		return error;
+
+	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm,
+				"xm", GPIOD_OUT_LOW);
+	if (error)
+		return error;
+
+	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp,
+				"yp", GPIOD_OUT_LOW);
+	if (error)
+		return error;
+
+	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym,
+				"ym", GPIOD_OUT_LOW);
+	if (error)
+		return error;
+
+	touchdev->pen_irq = platform_get_irq(pdev, 0);
+	if (touchdev->pen_irq < 0)
+		return touchdev->pen_irq;
+
+	error = devm_request_threaded_irq(dev, touchdev->pen_irq, NULL,
+			vf50_ts_irq_bh, IRQF_ONESHOT,
+			"vf50 touch", touchdev);
+	if (error < 0)
+		return error;
+
+	return 0;
+}
+
+static const struct of_device_id vf50_touch_of_match[] = {
+	{ .compatible = "toradex,vf50-touchscreen", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
+
+static struct platform_driver vf50_touch_driver = {
+	.driver = {
+		.name = "toradex,vf50_touchctrl",
+		.of_match_table = vf50_touch_of_match,
+	},
+	.probe = vf50_ts_probe,
+};
+
+module_platform_driver(vf50_touch_driver);
+
+MODULE_AUTHOR("Sanchayan Maity");
+MODULE_DESCRIPTION("Colibri VF50 Touchscreen driver");
+MODULE_LICENSE("GPL v2");
+MODULE_VERSION(DRV_VERSION);
-- 
2.5.0

Re: [PATCH v4 3/3] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Dmitry Torokhov]

Hi Sanchayan,

On Fri, Aug 21, 2015 at 06:56:32PM +0530, Sanchayan Maity wrote:
> The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> FETs and ADC inputs. This driver uses the IIO consumer interface
> and relies on the vf610_adc driver based on the IIO framework.
> 

Thank you for making changes. I have a few comments still.

> Signed-off-by: Sanchayan Maity <maitysanchayan-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/input/touchscreen/Kconfig           |  12 +
>  drivers/input/touchscreen/Makefile          |   1 +
>  drivers/input/touchscreen/colibri-vf50-ts.c | 370 ++++++++++++++++++++++++++++
>  3 files changed, 383 insertions(+)
>  create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c
> 
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 80f6386..28948ca 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called zforce_ts.
>  
> +config TOUCHSCREEN_COLIBRI_VF50
> +	tristate "Toradex Colibri on board touchscreen driver"
> +	depends on GPIOLIB && IIO && VF610_ADC

Can we possibly add "|| COMPILE_TEST" dependency? Will it compile
without IIO or VF610_ADC enabled?


> +	help
> +	  Say Y here if you have a Colibri VF50 and plan to use
> +	  the on-board provided 4-wire touchscreen driver.
> +
> +	  If unsure, say N.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called colibri_vf50_ts.
> +
>  endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index 44deea7..93746a0 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
>  obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
>  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
>  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> +obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
> diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
> new file mode 100644
> index 0000000..0793fdc
> --- /dev/null
> +++ b/drivers/input/touchscreen/colibri-vf50-ts.c
> @@ -0,0 +1,370 @@
> +/* Copyright 2015 Toradex AG
> + *
> + * Toradex Colibri VF50 Touchscreen driver
> + *
> + * Originally authored by Stefan Agner for 3.0 kernel
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/iio/types.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pinctrl/consumer.h>

Don't you need gpio/consumer.h?

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +#define DRIVER_NAME "colibri-vf50-ts"
> +#define DRV_VERSION "1.0"
> +
> +#define VF_ADC_MAX ((1 << 12) - 1)
> +
> +#define COLI_TOUCH_MIN_DELAY_US 1000
> +#define COLI_TOUCH_MAX_DELAY_US 2000
> +#define COLI_TOUCH_NO_OF_AVGS	5
> +
> +struct vf50_touch_device {
> +	struct platform_device	*pdev;
> +	struct input_dev	*ts_input;
> +	struct iio_channel	*channels;
> +	struct gpio_desc *gpio_xp;
> +	struct gpio_desc *gpio_xm;
> +	struct gpio_desc *gpio_yp;
> +	struct gpio_desc *gpio_ym;
> +	struct gpio_desc *gpio_pen_detect;

I do not see gpio_pen_detect being used anymore.

> +	int pen_irq;
> +	int min_pressure;
> +	bool stop_touchscreen;
> +};
> +
> +/*
> + * Enables given plates and measures touch parameters using ADC
> + */
> +static int adc_ts_measure(struct iio_channel *channel,
> +		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
> +{
> +	int i, value = 0, val = 0;
> +	int ret;
> +
> +	gpiod_set_value(plate_p, 1);
> +	gpiod_set_value(plate_m, 1);
> +
> +	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
> +
> +	for (i = 0; i < COLI_TOUCH_NO_OF_AVGS; i++) {
> +		ret = iio_read_channel_raw(channel, &val);
> +		if (ret < 0) {
> +			value = ret;
> +			goto error_iio_read;
> +		}
> +
> +		value += val;
> +	}
> +
> +	value /= COLI_TOUCH_NO_OF_AVGS;
> +
> +error_iio_read:
> +	gpiod_set_value(plate_p, 0);
> +	gpiod_set_value(plate_m, 0);
> +
> +	return value;
> +}
> +
> +/*
> + * Enable touch detection using falling edge detection on XM
> + */
> +static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
> +{
> +	/* Enable plate YM (needs to be strong GND, high active) */
> +	gpiod_set_value(vf50_ts->gpio_ym, 1);
> +
> +	/*
> +	 * Let the platform mux to idle state in order to enable
> +	 * Pull-Up on GPIO
> +	 */
> +	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
> +}
> +
> +/*
> + * ADC touch screen sampling bottom half irq handler
> + */
> +static irqreturn_t vf50_ts_irq_bh(int irq, void *private)
> +{
> +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)private;

No need to cast from void * pointer.

> +	struct device *dev = &vf50_ts->pdev->dev;
> +	int val_x, val_y, val_z1, val_z2, val_p = 0;
> +	bool discard_val_on_start = true;
> +
> +	/* Disable the touch detection plates */
> +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> +
> +	/* Let the platform mux to default state in order to mux as ADC */
> +	pinctrl_pm_select_default_state(dev);
> +
> +	while (!vf50_ts->stop_touchscreen) {
> +		/* X-Direction */
> +		val_x = adc_ts_measure(&vf50_ts->channels[0],
> +				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
> +		if (val_x < 0)
> +			break;
> +
> +		/* Y-Direction */
> +		val_y = adc_ts_measure(&vf50_ts->channels[1],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
> +		if (val_y < 0)
> +			break;
> +
> +		/*
> +		 * Touch pressure
> +		 * Measure on XP/YM
> +		 */
> +		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> +		if (val_z1 < 0)
> +			break;
> +		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
> +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> +		if (val_z2 < 0)
> +			break;
> +
> +		/* Validate signal (avoid calculation using noise) */
> +		if (val_z1 > 64 && val_x > 64) {
> +			/*
> +			 * Calculate resistance between the plates
> +			 * lower resistance means higher pressure
> +			 */
> +			int r_x = (1000 * val_x) / VF_ADC_MAX;
> +
> +			val_p = (r_x * val_z2) / val_z1 - r_x;
> +
> +		} else {
> +			val_p = 2000;
> +		}
> +
> +		val_p = 2000 - val_p;
> +		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
> +			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
> +
> +		/*
> +		 * If touch pressure is too low, stop measuring and reenable
> +		 * touch detection
> +		 */
> +		if (val_p < vf50_ts->min_pressure || val_p > 2000)
> +			break;
> +
> +		/*
> +		 * The pressure may not be enough for the first x and the
> +		 * second y measurement, but, the pressure is ok when the
> +		 * driver is doing the third and fourth measurement. To
> +		 * take care of this, we drop the first measurement always.
> +		 */
> +		if (discard_val_on_start) {
> +			discard_val_on_start = false;
> +		} else {
> +			/*
> +			 * Report touch position and sleep for
> +			 * next measurement
> +			 */
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_X, VF_ADC_MAX - val_x);
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_Y, VF_ADC_MAX - val_y);
> +			input_report_abs(vf50_ts->ts_input,
> +					ABS_PRESSURE, val_p);
> +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> +			input_sync(vf50_ts->ts_input);
> +		}
> +
> +		msleep(10);
> +	}
> +
> +	/* Report no more touch, reenable touch detection */
> +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> +	input_sync(vf50_ts->ts_input);
> +
> +	vf50_ts_enable_touch_detection(vf50_ts);
> +
> +	/* Wait for the pull-up to be stable on high */
> +	msleep(10);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int vf50_ts_open(struct input_dev *dev_input)
> +{
> +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> +	struct device *dev = &touchdev->pdev->dev;
> +
> +	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
> +			dev_input->name);
> +
> +	touchdev->stop_touchscreen = false;
> +
> +	/* Mux detection before request IRQ, wait for pull-up to settle */
> +	vf50_ts_enable_touch_detection(touchdev);
> +	msleep(10);
> +
> +	return 0;
> +}
> +
> +static void vf50_ts_close(struct input_dev *dev_input)
> +{
> +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> +	struct device *dev = &touchdev->pdev->dev;
> +
> +	touchdev->stop_touchscreen = true;
> +
> +	/* Make sure IRQ is not running past close */
> +	mb();
> +	synchronize_irq(touchdev->pen_irq);
> +

Should we drive the gpio_ym inactive here? What about pin control?

> +	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
> +		dev_input->name);
> +}
> +
> +static int vf50_ts_get_gpiod(struct device *dev,
> +	struct gpio_desc **gpio_d, const char *con_id, enum gpiod_flags flags)
> +{
> +	int error;
> +
> +	*gpio_d = devm_gpiod_get(dev, con_id, flags);
> +	if (IS_ERR(*gpio_d)) {
> +		error = PTR_ERR(*gpio_d);
> +		dev_err(dev, "Could not get gpio_%s %d\n", con_id, error);
> +		return error;
> +	}
> +
> +	return 0;
> +}
> +
> +static void vf50_ts_channel_release(void *data)
> +{
> +	struct vf50_touch_device *touchdev = data;
> +
> +	iio_channel_release_all(touchdev->channels);
> +}
> +
> +static int vf50_ts_probe(struct platform_device *pdev)
> +{
> +	struct input_dev *input;
> +	struct iio_channel *channels;
> +	struct device *dev = &pdev->dev;
> +	struct vf50_touch_device *touchdev;
> +	int error;
> +
> +	channels = iio_channel_get_all(dev);
> +	if (IS_ERR(channels))
> +		return PTR_ERR(channels);
> +
> +	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
> +	if (!touchdev) {
> +		error = -ENOMEM;
> +		return error;

Why not simply

		return -ENOMEM;

here?

Also you are leaking your iio channels here. You need to install the
custom action earlier.

> +	}
> +
> +	error = of_property_read_u32(dev->of_node, "vf50-ts-min-pressure",
> +				 &touchdev->min_pressure);
> +	if (error)
> +		return error;
> +
> +	error = devm_add_action(dev, vf50_ts_channel_release, channels);
> +	if (error) {
> +		dev_err(dev, "Failed to register iio channel release action");

You also need

		vf50_ts_channel_release(touchdev);

> +		return error;
> +	}
> +
> +	input = devm_input_allocate_device(dev);
> +	if (!input) {
> +		dev_err(dev, "Failed to allocate TS input device\n");
> +		error = -ENOMEM;
> +		return error;

Again simply return error code, no need to assign to the error variable.

> +	}
> +
> +	platform_set_drvdata(pdev, touchdev);
> +
> +	touchdev->pdev = pdev;
> +	touchdev->channels = channels;
> +
> +	input->name = DRIVER_NAME;
> +	input->id.bustype = BUS_HOST;
> +	input->dev.parent = dev;
> +	input->open = vf50_ts_open;
> +	input->close = vf50_ts_close;
> +
> +	input_set_capability(input, EV_KEY, BTN_TOUCH);
> +	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
> +	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
> +	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
> +
> +	touchdev->ts_input = input;
> +	input_set_drvdata(input, touchdev);
> +	error = input_register_device(input);
> +	if (error) {
> +		dev_err(dev, "Failed to register input device\n");
> +		return error;
> +	}
> +
> +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp,
> +				"xp", GPIOD_OUT_LOW);
> +	if (error)
> +		return error;
> +
> +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm,
> +				"xm", GPIOD_OUT_LOW);
> +	if (error)
> +		return error;
> +
> +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp,
> +				"yp", GPIOD_OUT_LOW);
> +	if (error)
> +		return error;
> +
> +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym,
> +				"ym", GPIOD_OUT_LOW);
> +	if (error)
> +		return error;
> +
> +	touchdev->pen_irq = platform_get_irq(pdev, 0);
> +	if (touchdev->pen_irq < 0)
> +		return touchdev->pen_irq;
> +
> +	error = devm_request_threaded_irq(dev, touchdev->pen_irq, NULL,
> +			vf50_ts_irq_bh, IRQF_ONESHOT,
> +			"vf50 touch", touchdev);
> +	if (error < 0)
> +		return error;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vf50_touch_of_match[] = {
> +	{ .compatible = "toradex,vf50-touchscreen", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> +
> +static struct platform_driver vf50_touch_driver = {
> +	.driver = {
> +		.name = "toradex,vf50_touchctrl",
> +		.of_match_table = vf50_touch_of_match,
> +	},
> +	.probe = vf50_ts_probe,
> +};
> +
> +module_platform_driver(vf50_touch_driver);
> +
> +MODULE_AUTHOR("Sanchayan Maity");
> +MODULE_DESCRIPTION("Colibri VF50 Touchscreen driver");
> +MODULE_LICENSE("GPL v2");

Please change it to "GPL" which means GPL v2+ to match license notice at
the top.

> +MODULE_VERSION(DRV_VERSION);
> -- 
> 2.5.0
> 

Thank you.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 3/3] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

Hello Dmitry,

On 15-08-21 18:30:57, Dmitry Torokhov wrote:
> Hi Sanchayan,
> 
> On Fri, Aug 21, 2015 at 06:56:32PM +0530, Sanchayan Maity wrote:
> > The Colibri Vybrid VF50 module supports 4-wire touchscreens using
> > FETs and ADC inputs. This driver uses the IIO consumer interface
> > and relies on the vf610_adc driver based on the IIO framework.
> > 
> 
> Thank you for making changes. I have a few comments still.

It has really improved with your comments. Thanks for your feedback.

> 
> > Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> > ---
> >  drivers/input/touchscreen/Kconfig           |  12 +
> >  drivers/input/touchscreen/Makefile          |   1 +
> >  drivers/input/touchscreen/colibri-vf50-ts.c | 370 ++++++++++++++++++++++++++++
> >  3 files changed, 383 insertions(+)
> >  create mode 100644 drivers/input/touchscreen/colibri-vf50-ts.c
> > 
> > diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> > index 80f6386..28948ca 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -1027,4 +1027,16 @@ config TOUCHSCREEN_ZFORCE
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called zforce_ts.
> >  
> > +config TOUCHSCREEN_COLIBRI_VF50
> > +	tristate "Toradex Colibri on board touchscreen driver"
> > +	depends on GPIOLIB && IIO && VF610_ADC
> 
> Can we possibly add "|| COMPILE_TEST" dependency? Will it compile
> without IIO or VF610_ADC enabled?

I will add and do the checks. It should compile without VF610_ADC
atleast, I don't see how it will without the others. I will check
once at my end also with other configs.

> 
> 
> > +	help
> > +	  Say Y here if you have a Colibri VF50 and plan to use
> > +	  the on-board provided 4-wire touchscreen driver.
> > +
> > +	  If unsure, say N.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called colibri_vf50_ts.
> > +
> >  endif
> > diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> > index 44deea7..93746a0 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -84,3 +84,4 @@ obj-$(CONFIG_TOUCHSCREEN_W90X900)	+= w90p910_ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_SX8654)	+= sx8654.o
> >  obj-$(CONFIG_TOUCHSCREEN_TPS6507X)	+= tps6507x-ts.o
> >  obj-$(CONFIG_TOUCHSCREEN_ZFORCE)	+= zforce_ts.o
> > +obj-$(CONFIG_TOUCHSCREEN_COLIBRI_VF50)	+= colibri-vf50-ts.o
> > diff --git a/drivers/input/touchscreen/colibri-vf50-ts.c b/drivers/input/touchscreen/colibri-vf50-ts.c
> > new file mode 100644
> > index 0000000..0793fdc
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/colibri-vf50-ts.c
> > @@ -0,0 +1,370 @@
> > +/* Copyright 2015 Toradex AG
> > + *
> > + * Toradex Colibri VF50 Touchscreen driver
> > + *
> > + * Originally authored by Stefan Agner for 3.0 kernel
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/iio/consumer.h>
> > +#include <linux/iio/types.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pinctrl/consumer.h>
> 
> Don't you need gpio/consumer.h?

Yes.

> 
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +
> > +#define DRIVER_NAME "colibri-vf50-ts"
> > +#define DRV_VERSION "1.0"
> > +
> > +#define VF_ADC_MAX ((1 << 12) - 1)
> > +
> > +#define COLI_TOUCH_MIN_DELAY_US 1000
> > +#define COLI_TOUCH_MAX_DELAY_US 2000
> > +#define COLI_TOUCH_NO_OF_AVGS	5
> > +
> > +struct vf50_touch_device {
> > +	struct platform_device	*pdev;
> > +	struct input_dev	*ts_input;
> > +	struct iio_channel	*channels;
> > +	struct gpio_desc *gpio_xp;
> > +	struct gpio_desc *gpio_xm;
> > +	struct gpio_desc *gpio_yp;
> > +	struct gpio_desc *gpio_ym;
> > +	struct gpio_desc *gpio_pen_detect;
> 
> I do not see gpio_pen_detect being used anymore.

Sorry. My mistake. It needs to be deleted. Will fix.

> 
> > +	int pen_irq;
> > +	int min_pressure;
> > +	bool stop_touchscreen;
> > +};
> > +
> > +/*
> > + * Enables given plates and measures touch parameters using ADC
> > + */
> > +static int adc_ts_measure(struct iio_channel *channel,
> > +		  struct gpio_desc *plate_p, struct gpio_desc *plate_m)
> > +{
> > +	int i, value = 0, val = 0;
> > +	int ret;
> > +
> > +	gpiod_set_value(plate_p, 1);
> > +	gpiod_set_value(plate_m, 1);
> > +
> > +	usleep_range(COLI_TOUCH_MIN_DELAY_US, COLI_TOUCH_MAX_DELAY_US);
> > +
> > +	for (i = 0; i < COLI_TOUCH_NO_OF_AVGS; i++) {
> > +		ret = iio_read_channel_raw(channel, &val);
> > +		if (ret < 0) {
> > +			value = ret;
> > +			goto error_iio_read;
> > +		}
> > +
> > +		value += val;
> > +	}
> > +
> > +	value /= COLI_TOUCH_NO_OF_AVGS;
> > +
> > +error_iio_read:
> > +	gpiod_set_value(plate_p, 0);
> > +	gpiod_set_value(plate_m, 0);
> > +
> > +	return value;
> > +}
> > +
> > +/*
> > + * Enable touch detection using falling edge detection on XM
> > + */
> > +static void vf50_ts_enable_touch_detection(struct vf50_touch_device *vf50_ts)
> > +{
> > +	/* Enable plate YM (needs to be strong GND, high active) */
> > +	gpiod_set_value(vf50_ts->gpio_ym, 1);
> > +
> > +	/*
> > +	 * Let the platform mux to idle state in order to enable
> > +	 * Pull-Up on GPIO
> > +	 */
> > +	pinctrl_pm_select_idle_state(&vf50_ts->pdev->dev);
> > +}
> > +
> > +/*
> > + * ADC touch screen sampling bottom half irq handler
> > + */
> > +static irqreturn_t vf50_ts_irq_bh(int irq, void *private)
> > +{
> > +	struct vf50_touch_device *vf50_ts = (struct vf50_touch_device *)private;
> 
> No need to cast from void * pointer.

Ok.

> 
> > +	struct device *dev = &vf50_ts->pdev->dev;
> > +	int val_x, val_y, val_z1, val_z2, val_p = 0;
> > +	bool discard_val_on_start = true;
> > +
> > +	/* Disable the touch detection plates */
> > +	gpiod_set_value(vf50_ts->gpio_ym, 0);
> > +
> > +	/* Let the platform mux to default state in order to mux as ADC */
> > +	pinctrl_pm_select_default_state(dev);
> > +
> > +	while (!vf50_ts->stop_touchscreen) {
> > +		/* X-Direction */
> > +		val_x = adc_ts_measure(&vf50_ts->channels[0],
> > +				vf50_ts->gpio_xp, vf50_ts->gpio_xm);
> > +		if (val_x < 0)
> > +			break;
> > +
> > +		/* Y-Direction */
> > +		val_y = adc_ts_measure(&vf50_ts->channels[1],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_ym);
> > +		if (val_y < 0)
> > +			break;
> > +
> > +		/*
> > +		 * Touch pressure
> > +		 * Measure on XP/YM
> > +		 */
> > +		val_z1 = adc_ts_measure(&vf50_ts->channels[2],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> > +		if (val_z1 < 0)
> > +			break;
> > +		val_z2 = adc_ts_measure(&vf50_ts->channels[3],
> > +				vf50_ts->gpio_yp, vf50_ts->gpio_xm);
> > +		if (val_z2 < 0)
> > +			break;
> > +
> > +		/* Validate signal (avoid calculation using noise) */
> > +		if (val_z1 > 64 && val_x > 64) {
> > +			/*
> > +			 * Calculate resistance between the plates
> > +			 * lower resistance means higher pressure
> > +			 */
> > +			int r_x = (1000 * val_x) / VF_ADC_MAX;
> > +
> > +			val_p = (r_x * val_z2) / val_z1 - r_x;
> > +
> > +		} else {
> > +			val_p = 2000;
> > +		}
> > +
> > +		val_p = 2000 - val_p;
> > +		dev_dbg(dev, "Measured values: x: %d, y: %d, z1: %d, z2: %d, "
> > +			"p: %d\n", val_x, val_y, val_z1, val_z2, val_p);
> > +
> > +		/*
> > +		 * If touch pressure is too low, stop measuring and reenable
> > +		 * touch detection
> > +		 */
> > +		if (val_p < vf50_ts->min_pressure || val_p > 2000)
> > +			break;
> > +
> > +		/*
> > +		 * The pressure may not be enough for the first x and the
> > +		 * second y measurement, but, the pressure is ok when the
> > +		 * driver is doing the third and fourth measurement. To
> > +		 * take care of this, we drop the first measurement always.
> > +		 */
> > +		if (discard_val_on_start) {
> > +			discard_val_on_start = false;
> > +		} else {
> > +			/*
> > +			 * Report touch position and sleep for
> > +			 * next measurement
> > +			 */
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_X, VF_ADC_MAX - val_x);
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_Y, VF_ADC_MAX - val_y);
> > +			input_report_abs(vf50_ts->ts_input,
> > +					ABS_PRESSURE, val_p);
> > +			input_report_key(vf50_ts->ts_input, BTN_TOUCH, 1);
> > +			input_sync(vf50_ts->ts_input);
> > +		}
> > +
> > +		msleep(10);
> > +	}
> > +
> > +	/* Report no more touch, reenable touch detection */
> > +	input_report_abs(vf50_ts->ts_input, ABS_PRESSURE, 0);
> > +	input_report_key(vf50_ts->ts_input, BTN_TOUCH, 0);
> > +	input_sync(vf50_ts->ts_input);
> > +
> > +	vf50_ts_enable_touch_detection(vf50_ts);
> > +
> > +	/* Wait for the pull-up to be stable on high */
> > +	msleep(10);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int vf50_ts_open(struct input_dev *dev_input)
> > +{
> > +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> > +	struct device *dev = &touchdev->pdev->dev;
> > +
> > +	dev_dbg(dev, "Input device %s opened, starting touch detection\n",
> > +			dev_input->name);
> > +
> > +	touchdev->stop_touchscreen = false;
> > +
> > +	/* Mux detection before request IRQ, wait for pull-up to settle */
> > +	vf50_ts_enable_touch_detection(touchdev);
> > +	msleep(10);
> > +
> > +	return 0;
> > +}
> > +
> > +static void vf50_ts_close(struct input_dev *dev_input)
> > +{
> > +	struct vf50_touch_device *touchdev = input_get_drvdata(dev_input);
> > +	struct device *dev = &touchdev->pdev->dev;
> > +
> > +	touchdev->stop_touchscreen = true;
> > +
> > +	/* Make sure IRQ is not running past close */
> > +	mb();
> > +	synchronize_irq(touchdev->pen_irq);
> > +
> 
> Should we drive the gpio_ym inactive here? What about pin control?

My understanding is that the last IRQ handler run should leave things in
the pen irq detect and idle pictrl state. And since these lines are not
provided for any other use on the module, that state is left as is since
it is the same state we assume in open. However yes, I should turn off the
ym gpio inactive.

> 
> > +	dev_dbg(dev, "Input device %s closed, disable touch detection\n",
> > +		dev_input->name);
> > +}
> > +
> > +static int vf50_ts_get_gpiod(struct device *dev,
> > +	struct gpio_desc **gpio_d, const char *con_id, enum gpiod_flags flags)
> > +{
> > +	int error;
> > +
> > +	*gpio_d = devm_gpiod_get(dev, con_id, flags);
> > +	if (IS_ERR(*gpio_d)) {
> > +		error = PTR_ERR(*gpio_d);
> > +		dev_err(dev, "Could not get gpio_%s %d\n", con_id, error);
> > +		return error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void vf50_ts_channel_release(void *data)
> > +{
> > +	struct vf50_touch_device *touchdev = data;
> > +
> > +	iio_channel_release_all(touchdev->channels);
> > +}
> > +
> > +static int vf50_ts_probe(struct platform_device *pdev)
> > +{
> > +	struct input_dev *input;
> > +	struct iio_channel *channels;
> > +	struct device *dev = &pdev->dev;
> > +	struct vf50_touch_device *touchdev;
> > +	int error;
> > +
> > +	channels = iio_channel_get_all(dev);
> > +	if (IS_ERR(channels))
> > +		return PTR_ERR(channels);
> > +
> > +	touchdev = devm_kzalloc(dev, sizeof(*touchdev), GFP_KERNEL);
> > +	if (!touchdev) {
> > +		error = -ENOMEM;
> > +		return error;
> 
> Why not simply
> 
> 		return -ENOMEM;
> 
> here?

Ok.

> 
> Also you are leaking your iio channels here. You need to install the
> custom action earlier.

I will reorder this set of four calls here which will be better.

> 
> > +	}
> > +
> > +	error = of_property_read_u32(dev->of_node, "vf50-ts-min-pressure",
> > +				 &touchdev->min_pressure);
> > +	if (error)
> > +		return error;
> > +
> > +	error = devm_add_action(dev, vf50_ts_channel_release, channels);
> > +	if (error) {
> > +		dev_err(dev, "Failed to register iio channel release action");
> 
> You also need
> 
> 		vf50_ts_channel_release(touchdev);
> 
> > +		return error;
> > +	}
> > +
> > +	input = devm_input_allocate_device(dev);
> > +	if (!input) {
> > +		dev_err(dev, "Failed to allocate TS input device\n");
> > +		error = -ENOMEM;
> > +		return error;
> 
> Again simply return error code, no need to assign to the error variable.

Ok.

> 
> > +	}
> > +
> > +	platform_set_drvdata(pdev, touchdev);
> > +
> > +	touchdev->pdev = pdev;
> > +	touchdev->channels = channels;
> > +
> > +	input->name = DRIVER_NAME;
> > +	input->id.bustype = BUS_HOST;
> > +	input->dev.parent = dev;
> > +	input->open = vf50_ts_open;
> > +	input->close = vf50_ts_close;
> > +
> > +	input_set_capability(input, EV_KEY, BTN_TOUCH);
> > +	input_set_abs_params(input, ABS_X, 0, VF_ADC_MAX, 0, 0);
> > +	input_set_abs_params(input, ABS_Y, 0, VF_ADC_MAX, 0, 0);
> > +	input_set_abs_params(input, ABS_PRESSURE, 0, VF_ADC_MAX, 0, 0);
> > +
> > +	touchdev->ts_input = input;
> > +	input_set_drvdata(input, touchdev);
> > +	error = input_register_device(input);
> > +	if (error) {
> > +		dev_err(dev, "Failed to register input device\n");
> > +		return error;
> > +	}
> > +
> > +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xp,
> > +				"xp", GPIOD_OUT_LOW);
> > +	if (error)
> > +		return error;
> > +
> > +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_xm,
> > +				"xm", GPIOD_OUT_LOW);
> > +	if (error)
> > +		return error;
> > +
> > +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_yp,
> > +				"yp", GPIOD_OUT_LOW);
> > +	if (error)
> > +		return error;
> > +
> > +	error = vf50_ts_get_gpiod(dev, &touchdev->gpio_ym,
> > +				"ym", GPIOD_OUT_LOW);
> > +	if (error)
> > +		return error;
> > +
> > +	touchdev->pen_irq = platform_get_irq(pdev, 0);
> > +	if (touchdev->pen_irq < 0)
> > +		return touchdev->pen_irq;
> > +
> > +	error = devm_request_threaded_irq(dev, touchdev->pen_irq, NULL,
> > +			vf50_ts_irq_bh, IRQF_ONESHOT,
> > +			"vf50 touch", touchdev);
> > +	if (error < 0)
> > +		return error;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id vf50_touch_of_match[] = {
> > +	{ .compatible = "toradex,vf50-touchscreen", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, vf50_touch_of_match);
> > +
> > +static struct platform_driver vf50_touch_driver = {
> > +	.driver = {
> > +		.name = "toradex,vf50_touchctrl",
> > +		.of_match_table = vf50_touch_of_match,
> > +	},
> > +	.probe = vf50_ts_probe,
> > +};
> > +
> > +module_platform_driver(vf50_touch_driver);
> > +
> > +MODULE_AUTHOR("Sanchayan Maity");
> > +MODULE_DESCRIPTION("Colibri VF50 Touchscreen driver");
> > +MODULE_LICENSE("GPL v2");
> 
> Please change it to "GPL" which means GPL v2+ to match license notice at
> the top.

Ok.

--  Sanchayan.

> 
> > +MODULE_VERSION(DRV_VERSION);
> > -- 
> > 2.5.0
> > 
> 
> Thank you.
> 
> -- 
> Dmitry

Re: [PATCH v4 3/3] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[Stefan Agner]

Hi Sanchayan,

On 2015-08-21 06:26, Sanchayan Maity wrote:
> +static int vf50_ts_probe(struct platform_device *pdev)
> +{
> +	struct input_dev *input;
> +	struct iio_channel *channels;
> +	struct device *dev = &pdev->dev;
> +	struct vf50_touch_device *touchdev;
> +	int error;
> +
> +	channels = iio_channel_get_all(dev);
> +	if (IS_ERR(channels))
> +		return PTR_ERR(channels);

Hm, we expect here that four channels are returned, however a faulty
device tree could contain less. Access to the fourth channel above would
lead to a bug.

Can you check the amount of channels returned? The returned list is
explicitly terminated with a null entry, you can rely on that. Something
similar to hwmon should do the job.
http://lxr.free-electrons.com/source/drivers/hwmon/iio_hwmon.c#L86


Otherwise, Acked-by: Stefan Agner <stefan@agner.ch>

--
Stefan

Re: [PATCH v4 3/3] touchscreen: colibri-vf50-ts: Add touchscreen support for Colibri VF50

[maitysanchayan]

On 15-08-22 18:52:28, Stefan Agner wrote:
> Hi Sanchayan,
> 
> On 2015-08-21 06:26, Sanchayan Maity wrote:
> > +static int vf50_ts_probe(struct platform_device *pdev)
> > +{
> > +	struct input_dev *input;
> > +	struct iio_channel *channels;
> > +	struct device *dev = &pdev->dev;
> > +	struct vf50_touch_device *touchdev;
> > +	int error;
> > +
> > +	channels = iio_channel_get_all(dev);
> > +	if (IS_ERR(channels))
> > +		return PTR_ERR(channels);
> 
> Hm, we expect here that four channels are returned, however a faulty
> device tree could contain less. Access to the fourth channel above would
> lead to a bug.
> 
> Can you check the amount of channels returned? The returned list is
> explicitly terminated with a null entry, you can rely on that. Something
> similar to hwmon should do the job.
> http://lxr.free-electrons.com/source/drivers/hwmon/iio_hwmon.c#L86

I agree. It did be nice to have this error check. Thanks. I will add
this check so we can bail out if less channels are specified.

- Sanchayan.

> 
> 
> Otherwise, Acked-by: Stefan Agner <stefan@agner.ch>
> 
> --
> Stefan