[PATCH v3 1/3] input: touchscreem: ad7877: add match table

[PATCH v3 1/3] input: touchscreem: ad7877: add match table

[Antoniu Miclaus]

Add match table for the ad7877 driver and define the compatible string.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
no changes in v3.
 drivers/input/touchscreen/ad7877.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index a0598e9c7aff..7886454a19c6 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -805,10 +805,17 @@ static int ad7877_resume(struct device *dev)
 
 static DEFINE_SIMPLE_DEV_PM_OPS(ad7877_pm, ad7877_suspend, ad7877_resume);
 
+static const struct of_device_id ad7877_of_match[] = {
+	{ .compatible = "adi,ad7877", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ad7877_of_match);
+
 static struct spi_driver ad7877_driver = {
 	.driver = {
 		.name		= "ad7877",
 		.dev_groups	= ad7877_groups,
+		.of_match_table = ad7877_of_match,
 		.pm		= pm_sleep_ptr(&ad7877_pm),
 	},
 	.probe		= ad7877_probe,
-- 
2.46.0

[PATCH v3 2/3] input: touchscreen: ad7877: add dt support

[Antoniu Miclaus]

Add devicetree support within the driver.

Remove old platform data approach since it is no longer used.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v3:
 - remove platform data instead of making it optional, as suggested.
 - use touchscreen_parse_properties and touchscreen_report_pos
 drivers/input/touchscreen/ad7877.c | 150 +++++++++++++++++++++++------
 include/linux/spi/ad7877.h         |  25 -----
 2 files changed, 119 insertions(+), 56 deletions(-)
 delete mode 100644 include/linux/spi/ad7877.h

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 7886454a19c6..bb1799bcace3 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -25,11 +25,12 @@
 #include <linux/device.h>
 #include <linux/delay.h>
 #include <linux/input.h>
+#include <linux/input/touchscreen.h>
 #include <linux/interrupt.h>
 #include <linux/pm.h>
+#include <linux/property.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
-#include <linux/spi/ad7877.h>
 #include <linux/module.h>
 #include <asm/irq.h>
 
@@ -174,6 +175,8 @@ struct ad7877 {
 	u8			averaging;
 	u8			pen_down_acc_interval;
 
+	struct touchscreen_properties prop;
+
 	struct spi_transfer	xfer[AD7877_NR_SENSE + 2];
 	struct spi_message	msg;
 
@@ -353,8 +356,7 @@ static int ad7877_process_data(struct ad7877 *ts)
 		if (!timer_pending(&ts->timer))
 			input_report_key(input_dev, BTN_TOUCH, 1);
 
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
+		touchscreen_report_pos(input_dev, &ts->prop, x, y, false);
 		input_report_abs(input_dev, ABS_PRESSURE, Rt);
 		input_sync(input_dev);
 
@@ -667,11 +669,118 @@ static void ad7877_setup_ts_def_msg(struct spi_device *spi, struct ad7877 *ts)
 	}
 }
 
+static int ad7877_parse_props(struct ad7877 *ts)
+{
+	struct device *dev = &ts->spi->dev;
+	u32 value, average;
+	int ret;
+
+	ts->model = (uintptr_t)device_get_match_data(dev);
+	if (!ts->model)
+		ts->model = 7877;
+
+	ret = device_property_match_string(dev, "adi,stopacq-polarity", "low");
+	if (ret < 0) {
+		ret = device_property_match_string(dev, "adi,stopacq-polarity", "high");
+		if (ret < 0)
+			ts->stopacq_polarity = 0;
+		ts->stopacq_polarity = 1;
+	} else {
+		ts->stopacq_polarity = 0;
+	}
+
+	ret = device_property_read_u32(dev, "adi,first-conv-delay-ns", &value);
+	if (!ret) {
+		switch (value) {
+		case 500:
+			ts->first_conversion_delay = 0;
+			break;
+		case 128000:
+			ts->first_conversion_delay = 1;
+			break;
+		case 1000000:
+			ts->first_conversion_delay = 2;
+			break;
+		case 8000000:
+			ts->first_conversion_delay = 3;
+			break;
+		default:
+			return dev_err_probe(dev, -EINVAL,
+					"Invalid adi,first-conv-delay-ns value\n");
+		}
+	}
+
+	device_property_read_u32(dev, "adi,pen-down-acc-interval",
+				 &value);
+	ts->pen_down_acc_interval = (u8)value;
+
+	ret = device_property_read_u32(dev, "adi,acquisition-time-us", &value);
+	if (!ret) {
+		switch (value) {
+		case 2:
+			ts->acquisition_time = 0;
+			break;
+		case 4:
+			ts->acquisition_time = 1;
+			break;
+		case 8:
+			ts->acquisition_time = 2;
+			break;
+		case 16:
+			ts->acquisition_time = 3;
+			break;
+		default:
+			return dev_err_probe(dev, -EINVAL,
+					"Invalid adi,first-conv-delay-ns value\n");
+		}
+	}
+
+	device_property_read_u32(dev, "adi,vref-delay-us",
+				 &value);
+	if (!value)
+		ts->vref_delay_usecs = 100;
+	else
+		ts->vref_delay_usecs = (u16)value;
+
+	device_property_read_u32(dev, "touchscreen-x-plate-ohms", &value);
+	if (value)
+		ts->x_plate_ohms = (u16)value;
+	else
+		ts->x_plate_ohms = 400;
+
+	/*
+	 * The property is parsed also in the touchscreen_parse_properties()
+	 * but is required for the ad7877_process_data() so we need to store it.
+	 */
+	device_property_read_u32(dev, "touchscreen-max-pressure", &value);
+	ts->pressure_max = (u16)value;
+
+	device_property_read_u32(dev, "touchscreen-average-samples", &average);
+	switch (average) {
+	case 1:
+		ts->averaging = 0;
+		break;
+	case 4:
+		ts->averaging = 1;
+		break;
+	case 8:
+		ts->averaging = 2;
+		break;
+	case 16:
+		ts->averaging = 3;
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL,
+				     "touchscreen-average-samples must be 1, 4, 8, or 16\n");
+	}
+
+	return 0;
+}
+
 static int ad7877_probe(struct spi_device *spi)
 {
 	struct ad7877			*ts;
 	struct input_dev		*input_dev;
-	struct ad7877_platform_data	*pdata = dev_get_platdata(&spi->dev);
 	int				err;
 	u16				verify;
 
@@ -680,11 +789,6 @@ static int ad7877_probe(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	if (!pdata) {
-		dev_dbg(&spi->dev, "no platform data?\n");
-		return -ENODEV;
-	}
-
 	/* don't exceed max specified SPI CLK frequency */
 	if (spi->max_speed_hz > MAX_SPI_FREQ_HZ) {
 		dev_dbg(&spi->dev, "SPI CLK %d Hz?\n",spi->max_speed_hz);
@@ -714,27 +818,22 @@ static int ad7877_probe(struct spi_device *spi)
 	ts->spi = spi;
 	ts->input = input_dev;
 
+	err = ad7877_parse_props(ts);
+	if (err)
+		return err;
+
 	timer_setup(&ts->timer, ad7877_timer, 0);
 	mutex_init(&ts->mutex);
 	spin_lock_init(&ts->lock);
 
-	ts->model = pdata->model ? : 7877;
-	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
-	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
-	ts->pressure_max = pdata->pressure_max ? : ~0;
-
-	ts->stopacq_polarity = pdata->stopacq_polarity;
-	ts->first_conversion_delay = pdata->first_conversion_delay;
-	ts->acquisition_time = pdata->acquisition_time;
-	ts->averaging = pdata->averaging;
-	ts->pen_down_acc_interval = pdata->pen_down_acc_interval;
-
 	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
 
 	input_dev->name = "AD7877 Touchscreen";
 	input_dev->phys = ts->phys;
 	input_dev->dev.parent = &spi->dev;
 
+	touchscreen_parse_properties(ts->input, false, &ts->prop);
+
 	__set_bit(EV_KEY, input_dev->evbit);
 	__set_bit(BTN_TOUCH, input_dev->keybit);
 	__set_bit(EV_ABS, input_dev->evbit);
@@ -742,17 +841,6 @@ static int ad7877_probe(struct spi_device *spi)
 	__set_bit(ABS_Y, input_dev->absbit);
 	__set_bit(ABS_PRESSURE, input_dev->absbit);
 
-	input_set_abs_params(input_dev, ABS_X,
-			pdata->x_min ? : 0,
-			pdata->x_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_Y,
-			pdata->y_min ? : 0,
-			pdata->y_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_PRESSURE,
-			pdata->pressure_min, pdata->pressure_max, 0, 0);
-
 	ad7877_write(spi, AD7877_REG_SEQ1, AD7877_MM_SEQUENCE);
 
 	verify = ad7877_read(spi, AD7877_REG_SEQ1);
diff --git a/include/linux/spi/ad7877.h b/include/linux/spi/ad7877.h
deleted file mode 100644
index b7be843c88e2..000000000000
--- a/include/linux/spi/ad7877.h
+++ /dev/null
@@ -1,25 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/* linux/spi/ad7877.h */
-
-/* Touchscreen characteristics vary between boards and models.  The
- * platform_data for the device's "struct device" holds this information.
- *
- * It's OK if the min/max values are zero.
- */
-struct ad7877_platform_data {
-	u16	model;			/* 7877 */
-	u16	vref_delay_usecs;	/* 0 for external vref; etc */
-	u16	x_plate_ohms;
-	u16	y_plate_ohms;
-
-	u16	x_min, x_max;
-	u16	y_min, y_max;
-	u16	pressure_min, pressure_max;
-
-	u8	stopacq_polarity;	/* 1 = Active HIGH, 0 = Active LOW */
-	u8	first_conversion_delay;	/* 0 = 0.5us, 1 = 128us, 2 = 1ms, 3 = 8ms */
-	u8	acquisition_time;	/* 0 = 2us, 1 = 4us, 2 = 8us, 3 = 16us */
-	u8	averaging;		/* 0 = 1, 1 = 4, 2 = 8, 3 = 16 */
-	u8	pen_down_acc_interval;	/* 0 = covert once, 1 = every 0.5 ms,
-					   2 = ever 1 ms,   3 = every 8 ms,*/
-};
-- 
2.46.0

[PATCH v3 3/3] dt-bindings: touchscreen: ad7877: add bindings

[Antoniu Miclaus]

Add device tree bindings for the ad7877 driver.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v3:
 - use strings for polarity.
 - use unit siffix where applies.
 - add defaults where applies.
 - add complete example.
 .../input/touchscreen/adi,ad7877.yaml         | 109 ++++++++++++++++++
 1 file changed, 109 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
new file mode 100644
index 000000000000..7603ce63af7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/adi,ad7877.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7877 Touch Screen Controller
+
+maintainers:
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+  Analog Devices Touch Screen Controller
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7877.pdf
+
+allOf:
+  - $ref: touchscreen.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,ad7877
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  spi-max-frequency:
+    description: AD7877 SPI bus clock frequency.
+    minimum: 10000
+    maximum: 20000000
+
+  adi,stopacq-polarity:
+    description: The polarity of the signal applied to the STOPACQ pin.
+    $ref: /schemas/types.yaml#/definitions/string
+    enum: [low, high]
+
+  adi,first-conv-delay-ns:
+    description: Delay in ns before the first conversion.
+    enum: [500, 128000, 1000000, 8000000]
+
+  adi,pen-down-acc-interval:
+    description: Enable the ADC to repeatedly perform conversions.
+                  0 = convert once
+                  1 = every 0.5 ms
+                  2 = every 1 ms
+                  3 = every 8 ms
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [0, 1, 2, 3]
+    default: 0
+
+  adi,acquisition-time-us:
+    description: Select acquisition times in us for the ADC.
+    enum: [2, 4, 8, 16]
+
+  adi,vref-delay-us:
+    description: Delay required for the SPI transfers depending on the VREF used.
+
+  touchscreen-average-samples:
+    enum: [1, 4, 8, 16]
+
+  touchscreen-x-plate-ohms:
+    default: 400
+
+  touchscreen-min-x: true
+  touchscreen-min-y: true
+  touchscreen-size-x: true
+  touchscreen-size-y: true
+  touchscreen-max-pressure: true
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - touchscreen-average-samples
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      touchscreen@0 {
+        compatible = "adi,ad7877";
+        reg = <0>;
+        spi-max-frequency = <20000000>;
+        interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-parent = <&gpio>;
+        adi,vref-delay-us = <100>;
+        adi,stopacq-polarity = "low";
+        adi,first-conv-delay-ns = <500>;
+        adi,pen-down-acc-interval = <0>;
+        adi,acquisition-time-us = <2>;
+        touchscreen-average-samples = <16>;
+        touchscreen-x-plate-ohms = <400>;
+        touchscreen-min-x = <0>;
+        touchscreen-min-y = <0>;
+        touchscreen-size-x = <800>;
+        touchscreen-size-y = <480>;
+        touchscreen-max-pressure = <4095>;
+      };
+    };
+...
-- 
2.46.0

Re: [PATCH v3 1/3] input: touchscreem: ad7877: add match table

[Krzysztof Kozlowski]

On Mon, Sep 09, 2024 at 12:30:11PM +0300, Antoniu Miclaus wrote:
> Add match table for the ad7877 driver and define the compatible string.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> no changes in v3.
>  drivers/input/touchscreen/ad7877.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
> index a0598e9c7aff..7886454a19c6 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -805,10 +805,17 @@ static int ad7877_resume(struct device *dev)
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(ad7877_pm, ad7877_suspend, ad7877_resume);
>  
> +static const struct of_device_id ad7877_of_match[] = {
> +	{ .compatible = "adi,ad7877", },

Bindings are before their users.

Best regards,
Krzysztof

Re: [PATCH v3 2/3] input: touchscreen: ad7877: add dt support

[Krzysztof Kozlowski]

On Mon, Sep 09, 2024 at 12:30:12PM +0300, Antoniu Miclaus wrote:
> Add devicetree support within the driver.
> 
> Remove old platform data approach since it is no longer used.

What is the point of the previous patch? How can the device work if you
apply only patch 1?

This is somehow odd split...

Best regards,
Krzysztof

Re: [PATCH v3 3/3] dt-bindings: touchscreen: ad7877: add bindings

[Krzysztof Kozlowski]

On Mon, Sep 09, 2024 at 12:30:13PM +0300, Antoniu Miclaus wrote:
> Add device tree bindings for the ad7877 driver.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v3:
>  - use strings for polarity.
>  - use unit siffix where applies.
>  - add defaults where applies.
>  - add complete example.
>  .../input/touchscreen/adi,ad7877.yaml         | 109 ++++++++++++++++++
>  1 file changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> new file mode 100644
> index 000000000000..7603ce63af7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> @@ -0,0 +1,109 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/adi,ad7877.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7877 Touch Screen Controller
> +
> +maintainers:
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +  Analog Devices Touch Screen Controller
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7877.pdf
> +
> +allOf:
> +  - $ref: touchscreen.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7877
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    description: AD7877 SPI bus clock frequency.
> +    minimum: 10000
> +    maximum: 20000000
> +
> +  adi,stopacq-polarity:
> +    description: The polarity of the signal applied to the STOPACQ pin.
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [low, high]
> +
> +  adi,first-conv-delay-ns:
> +    description: Delay in ns before the first conversion.
> +    enum: [500, 128000, 1000000, 8000000]
> +
> +  adi,pen-down-acc-interval:
> +    description: Enable the ADC to repeatedly perform conversions.
> +                  0 = convert once
> +                  1 = every 0.5 ms
> +                  2 = every 1 ms
> +                  3 = every 8 ms
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [0, 1, 2, 3]
> +    default: 0

You got the comment about using units, right? It applied *everywhere*.
This is clearly a property in ms.

> +
> +  adi,acquisition-time-us:
> +    description: Select acquisition times in us for the ADC.
> +    enum: [2, 4, 8, 16]

What is the default? The property is not reuired, so how does it work
without it?

> +
> +  adi,vref-delay-us:
> +    description: Delay required for the SPI transfers depending on the VREF used.

Same question.

Best regards,
Krzysztof

Re: [PATCH v3 3/3] dt-bindings: touchscreen: ad7877: add bindings

[Krzysztof Kozlowski]

On Mon, Sep 09, 2024 at 12:30:13PM +0300, Antoniu Miclaus wrote:
> Add device tree bindings for the ad7877 driver.

Bindings are for hardware, not driver.

Subject: "add bindings" is quite redundant. Rather say what device you
are adding.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] input: touchscreem: ad7877: add match table

[Nuno Sá]

On Mon, 2024-09-09 at 12:30 +0300, Antoniu Miclaus wrote:
> Add match table for the ad7877 driver and define the compatible string.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> no changes in v3.
>  drivers/input/touchscreen/ad7877.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/input/touchscreen/ad7877.c
> b/drivers/input/touchscreen/ad7877.c
> index a0598e9c7aff..7886454a19c6 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -805,10 +805,17 @@ static int ad7877_resume(struct device *dev)
>  
>  static DEFINE_SIMPLE_DEV_PM_OPS(ad7877_pm, ad7877_suspend, ad7877_resume);
>  
> +static const struct of_device_id ad7877_of_match[] = {
> +	{ .compatible = "adi,ad7877", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ad7877_of_match);
> +

Just curious, is there any reason for this patch to be split from patch 2? Also,
this patch should directly include mod_devicetable.h for 'struct of_device_id'
(instead of relying in other headers).

- Nuno Sá