linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx
@ 2020-04-17 20:28 Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC Artur Rojek
                   ` (3 more replies)
  0 siblings, 4 replies; 28+ messages in thread
From: Artur Rojek @ 2020-04-17 20:28 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil
  Cc: Heiko Stuebner, linux-input, devicetree, linux-iio, linux-kernel,
	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-v5: 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 39c0a609fc94..7a24bc1dabe1 100644
--- a/drivers/iio/adc/ingenic-adc.c
+++ b/drivers/iio/adc/ingenic-adc.c
@@ -383,6 +383,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);
@@ -392,6 +407,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.26.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC
  2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
@ 2020-04-17 20:28 ` Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode Artur Rojek
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 28+ messages in thread
From: Artur Rojek @ 2020-04-17 20:28 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil
  Cc: Heiko Stuebner, linux-input, devicetree, linux-iio, linux-kernel,
	Artur Rojek, Rob Herring

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>
Acked-by: Rob Herring <robh@kernel.org>
---

 Changes:

 v2-v5: no change

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

diff --git a/include/dt-bindings/iio/adc/ingenic,adc.h b/include/dt-bindings/iio/adc/ingenic,adc.h
index 42f871ab3272..95e20a8d6dc8 100644
--- a/include/dt-bindings/iio/adc/ingenic,adc.h
+++ b/include/dt-bindings/iio/adc/ingenic,adc.h
@@ -7,5 +7,7 @@
 #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
 
 #endif
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC Artur Rojek
@ 2020-04-17 20:28 ` Artur Rojek
  2020-04-17 20:59   ` Andy Shevchenko
  2020-04-17 20:28 ` [RESEND PATCH v5 4/5] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver Artur Rojek
  3 siblings, 1 reply; 28+ messages in thread
From: Artur Rojek @ 2020-04-17 20:28 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil
  Cc: Heiko Stuebner, linux-input, devicetree, linux-iio, linux-kernel,
	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.

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.
GCW Zero comes with a single joystick and is sufficiently handled with the
currently implemented single-ended mode. Support for boards with two
joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp channels
will need to be provided in the future.

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.

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

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

 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

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

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 12bb8b7ca1ff..c3314135fa2a 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -465,6 +465,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 7a24bc1dabe1..0dafc8d5d0d8 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,6 +22,8 @@
 #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
@@ -28,6 +32,9 @@
 #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_PULL_UP(n)	((n) << 16)
+#define JZ_ADC_REG_CFG_SAMPLE_NUM(n)	((n) << 10)
+#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
@@ -44,6 +51,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 {
@@ -411,6 +426,28 @@ static const struct iio_info ingenic_adc_info = {
 };
 
 static const struct iio_chan_spec ingenic_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
+		},
+	},
 	{
 		.extend_name = "aux",
 		.type = IIO_VOLTAGE,
@@ -418,6 +455,7 @@ static const struct iio_chan_spec ingenic_channels[] = {
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_AUX,
+		.scan_index = -1
 	},
 	{
 		.extend_name = "battery",
@@ -428,6 +466,7 @@ static const struct iio_chan_spec ingenic_channels[] = {
 						BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_BATTERY,
+		.scan_index = -1
 	},
 	{ /* Must always be last in the array. */
 		.extend_name = "aux2",
@@ -436,16 +475,69 @@ static const struct iio_chan_spec ingenic_channels[] = {
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.indexed = 1,
 		.channel = INGENIC_ADC_AUX2,
+		.scan_index = -1
 	},
 };
 
+static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
+{
+	struct ingenic_adc *adc = iio_priv(iio_dev);
+
+	clk_enable(adc->clk);
+	/* 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_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);
+	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);
+	u32 tdat;
+
+	tdat = readl(adc->base + JZ_ADC_REG_ADTCH);
+	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)
@@ -460,6 +552,18 @@ 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) {
+		dev_err(dev, "Failed to get irq: %d\n", irq);
+		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);
@@ -499,7 +603,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 = ingenic_channels;
 	iio_dev->num_channels = ARRAY_SIZE(ingenic_channels);
 	/* Remove AUX2 from the list of supported channels. */
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RESEND PATCH v5 4/5] dt-bindings: input: Add docs for ADC driven joystick.
  2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode Artur Rojek
@ 2020-04-17 20:28 ` Artur Rojek
  2020-04-17 20:28 ` [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver Artur Rojek
  3 siblings, 0 replies; 28+ messages in thread
From: Artur Rojek @ 2020-04-17 20:28 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil
  Cc: Heiko Stuebner, linux-input, devicetree, linux-iio, linux-kernel,
	Artur Rojek, Rob Herring

Add documentation for the adc-joystick driver, used to provide support
for joysticks connected over ADC.

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

 Changes:

 v2: - Add `reg` property to axis subnode in order to enumerate the axes,
     - rename `linux,abs-code` property to `linux,code`,
     - drop `linux,` prefix from the remaining properties of axis subnode

 v3: no change

 v4: - remove "bindings" from the unique identifier string,
     - replace `|` with `>` for all description properties,
     - specify the number of items for `io-channels`,
     - correct the regex pattern of `axis` property,
     - specify the value range of `reg` property for each axis,
     - put `abs-range` properties under `allOf` 

 v5: add `a-f` to the regex pattern of `axis` property

 .../bindings/input/adc-joystick.yaml          | 121 ++++++++++++++++++
 1 file changed, 121 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/adc-joystick.yaml

diff --git a/Documentation/devicetree/bindings/input/adc-joystick.yaml b/Documentation/devicetree/bindings/input/adc-joystick.yaml
new file mode 100644
index 000000000000..054406bbd22b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adc-joystick.yaml
@@ -0,0 +1,121 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2019-2020 Artur Rojek
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/input/adc-joystick.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: ADC attached joystick
+
+maintainers:
+  - Artur Rojek <contact@artur-rojek.eu>
+
+description: >
+  Bindings for joystick devices connected to ADC controllers supporting
+  the Industrial I/O subsystem.
+
+properties:
+  compatible:
+    const: adc-joystick
+
+  io-channels:
+    minItems: 1
+    maxItems: 1024
+    description: >
+      List of phandle and IIO specifier pairs.
+      Each pair defines one ADC channel to which a joystick axis is connected.
+      See Documentation/devicetree/bindings/iio/iio-bindings.txt for details.
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+required:
+  - compatible
+  - io-channels
+  - '#address-cells'
+  - '#size-cells'
+
+additionalProperties: false
+
+patternProperties:
+  "^axis@[0-9a-f]+$":
+    type: object
+    description: >
+      Represents a joystick axis bound to the given ADC channel.
+      For each entry in the io-channels list, one axis subnode with a matching
+      reg property must be specified.
+
+    properties:
+      reg:
+        minimum: 0
+        maximum: 1023
+        description: Index of an io-channels list entry bound to this axis.
+
+      linux,code:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: EV_ABS specific event code generated by the axis.
+
+      abs-range:
+        allOf:
+          - $ref: /schemas/types.yaml#/definitions/uint32-array
+          - items:
+              - description: minimum value
+              - description: maximum value
+        description: >
+          Minimum and maximum values produced by the axis.
+          For an ABS_X axis this will be the left-most and right-most
+          inclination of the joystick. If min > max, it is left to userspace to
+          treat the axis as inverted.
+          This property is interpreted as two signed 32 bit values.
+
+      abs-fuzz:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: >
+          Amount of noise in the input value.
+          Omitting this property indicates the axis is precise.
+
+      abs-flat:
+        $ref: /schemas/types.yaml#/definitions/uint32
+        description: >
+          Axial "deadzone", or area around the center position, where the axis
+          is considered to be at rest.
+          Omitting this property indicates the axis always returns to exactly
+          the center position.
+
+    required:
+      - reg
+      - linux,code
+      - abs-range
+
+    additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/iio/adc/ingenic,adc.h>
+    #include <dt-bindings/input/input.h>
+
+    joystick: adc-joystick {
+      compatible = "adc-joystick";
+      io-channels = <&adc INGENIC_ADC_TOUCH_XP>,
+                    <&adc INGENIC_ADC_TOUCH_YP>;
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      axis@0 {
+              reg = <0>;
+              linux,code = <ABS_X>;
+              abs-range = <3300 0>;
+              abs-fuzz = <4>;
+              abs-flat = <200>;
+      };
+      axis@1 {
+              reg = <1>;
+              linux,code = <ABS_Y>;
+              abs-range = <0 3300>;
+              abs-fuzz = <4>;
+              abs-flat = <200>;
+      };
+    };
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
                   ` (2 preceding siblings ...)
  2020-04-17 20:28 ` [RESEND PATCH v5 4/5] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
@ 2020-04-17 20:28 ` Artur Rojek
  2020-04-17 21:10   ` Andy Shevchenko
  3 siblings, 1 reply; 28+ messages in thread
From: Artur Rojek @ 2020-04-17 20:28 UTC (permalink / raw)
  To: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil
  Cc: Heiko Stuebner, linux-input, devicetree, linux-iio, linux-kernel,
	Artur Rojek

Add a driver for joystick devices connected to ADC controllers
supporting the Industrial I/O subsystem.

Signed-off-by: Artur Rojek <contact@artur-rojek.eu>
Tested-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

 v2: - sanity check supported channel format on probe,
     - rename adc_joystick_disable to a more sensible adc_joystick_cleanup, 
     - enforce correct axis order by checking the `reg` property of
       child nodes

 v3-v5: no change

 drivers/input/joystick/Kconfig        |  10 ++
 drivers/input/joystick/Makefile       |   1 +
 drivers/input/joystick/adc-joystick.c | 245 ++++++++++++++++++++++++++
 3 files changed, 256 insertions(+)
 create mode 100644 drivers/input/joystick/adc-joystick.c

diff --git a/drivers/input/joystick/Kconfig b/drivers/input/joystick/Kconfig
index 940b744639c7..efbc20ec5099 100644
--- a/drivers/input/joystick/Kconfig
+++ b/drivers/input/joystick/Kconfig
@@ -42,6 +42,16 @@ config JOYSTICK_A3D
 	  To compile this driver as a module, choose M here: the
 	  module will be called a3d.
 
+config JOYSTICK_ADC
+	tristate "Simple joystick connected over ADC"
+	depends on IIO
+	select IIO_BUFFER_CB
+	help
+	  Say Y here if you have a simple joystick connected over ADC.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adc-joystick.
+
 config JOYSTICK_ADI
 	tristate "Logitech ADI digital joysticks and gamepads"
 	select GAMEPORT
diff --git a/drivers/input/joystick/Makefile b/drivers/input/joystick/Makefile
index 8656023f6ef5..58232b3057d3 100644
--- a/drivers/input/joystick/Makefile
+++ b/drivers/input/joystick/Makefile
@@ -6,6 +6,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_JOYSTICK_A3D)		+= a3d.o
+obj-$(CONFIG_JOYSTICK_ADC)		+= adc-joystick.o
 obj-$(CONFIG_JOYSTICK_ADI)		+= adi.o
 obj-$(CONFIG_JOYSTICK_AMIGA)		+= amijoy.o
 obj-$(CONFIG_JOYSTICK_AS5011)		+= as5011.o
diff --git a/drivers/input/joystick/adc-joystick.c b/drivers/input/joystick/adc-joystick.c
new file mode 100644
index 000000000000..9cb9896da26e
--- /dev/null
+++ b/drivers/input/joystick/adc-joystick.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Input driver for joysticks connected over ADC.
+ * Copyright (c) 2019-2020 Artur Rojek <contact@artur-rojek.eu>
+ */
+#include <linux/ctype.h>
+#include <linux/input.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+
+struct adc_joystick_axis {
+	u32 code;
+	s32 range[2];
+	s32 fuzz;
+	s32 flat;
+};
+
+struct adc_joystick {
+	struct input_dev *input;
+	struct iio_cb_buffer *buffer;
+	struct adc_joystick_axis *axes;
+	struct iio_channel *chans;
+	int num_chans;
+};
+
+static int adc_joystick_handle(const void *data, void *private)
+{
+	struct adc_joystick *joy = private;
+	enum iio_endian endianness;
+	int bytes, msb, val, i;
+	bool sign;
+
+	bytes = joy->chans[0].channel->scan_type.storagebits >> 3;
+
+	for (i = 0; i < joy->num_chans; ++i) {
+		endianness = joy->chans[i].channel->scan_type.endianness;
+		msb = joy->chans[i].channel->scan_type.realbits - 1;
+		sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');
+
+		switch (bytes) {
+		case 1:
+			val = ((const u8 *)data)[i];
+			break;
+		case 2:
+			val = ((const u16 *)data)[i];
+			if (endianness == IIO_BE)
+				val = be16_to_cpu(val);
+			else if (endianness == IIO_LE)
+				val = le16_to_cpu(val);
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		val >>= joy->chans[i].channel->scan_type.shift;
+		if (sign)
+			val = sign_extend32(val, msb);
+		else
+			val &= GENMASK(msb, 0);
+		input_report_abs(joy->input, joy->axes[i].code, val);
+	}
+
+	input_sync(joy->input);
+
+	return 0;
+}
+
+static int adc_joystick_open(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+	int ret;
+
+	ret = iio_channel_start_all_cb(joy->buffer);
+	if (ret)
+		dev_err(dev->dev.parent, "Unable to start callback buffer");
+
+	return ret;
+}
+
+static void adc_joystick_close(struct input_dev *dev)
+{
+	struct adc_joystick *joy = input_get_drvdata(dev);
+
+	iio_channel_stop_all_cb(joy->buffer);
+}
+
+static void adc_joystick_cleanup(void *data)
+{
+	iio_channel_release_all_cb(data);
+}
+
+static int adc_joystick_set_axes(struct device *dev, struct adc_joystick *joy)
+{
+	struct adc_joystick_axis *axes;
+	struct fwnode_handle *child;
+	int num_axes, ret, i;
+
+	num_axes = device_get_child_node_count(dev);
+	if (!num_axes) {
+		dev_err(dev, "Unable to find child nodes");
+		return -EINVAL;
+	}
+
+	if (num_axes != joy->num_chans) {
+		dev_err(dev, "Got %d child nodes for %d channels",
+			num_axes, joy->num_chans);
+		return -EINVAL;
+	}
+
+	axes = devm_kmalloc_array(dev, num_axes, sizeof(*axes), GFP_KERNEL);
+	if (!axes)
+		return -ENOMEM;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &i);
+		if (ret || i >= num_axes) {
+			dev_err(dev, "reg invalid or missing");
+			goto err;
+		}
+
+		if (fwnode_property_read_u32(child, "linux,code",
+					     &axes[i].code)) {
+			dev_err(dev, "linux,code invalid or missing");
+			goto err;
+		}
+
+		if (fwnode_property_read_u32_array(child, "abs-range",
+						   axes[i].range, 2)) {
+			dev_err(dev, "abs-range invalid or missing");
+			goto err;
+		}
+
+		fwnode_property_read_u32(child, "abs-fuzz",
+					 &axes[i].fuzz);
+		fwnode_property_read_u32(child, "abs-flat",
+					 &axes[i].flat);
+
+		input_set_abs_params(joy->input, axes[i].code,
+				     axes[i].range[0], axes[i].range[1],
+				     axes[i].fuzz,
+				     axes[i].flat);
+		input_set_capability(joy->input, EV_ABS, axes[i].code);
+	}
+
+	joy->axes = axes;
+
+	return 0;
+
+err:
+	fwnode_handle_put(child);
+	return -EINVAL;
+}
+
+static int adc_joystick_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct adc_joystick *joy;
+	struct input_dev *input;
+	int bits, ret, i;
+
+	joy = devm_kzalloc(dev, sizeof(*joy), GFP_KERNEL);
+	if (!joy)
+		return -ENOMEM;
+
+	joy->chans = devm_iio_channel_get_all(dev);
+	if (IS_ERR(joy->chans)) {
+		ret = PTR_ERR(joy->chans);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Unable to get IIO channels");
+		return ret;
+	}
+
+	/* Count how many channels we got. NULL terminated. */
+	while (joy->chans[joy->num_chans].indio_dev)
+		joy->num_chans++;
+
+	bits = joy->chans[0].channel->scan_type.storagebits;
+	if (!bits || (bits >> 3) > 2) {
+		dev_err(dev, "Unsupported channel storage size");
+		return -EINVAL;
+	}
+	for (i = 1; i < joy->num_chans; ++i)
+		if (joy->chans[i].channel->scan_type.storagebits != bits) {
+			dev_err(dev, "Channels must have equal storage size");
+			return -EINVAL;
+		}
+
+	input = devm_input_allocate_device(dev);
+	if (!input) {
+		dev_err(dev, "Unable to allocate input device");
+		return -ENOMEM;
+	}
+
+	joy->input = input;
+	input->name = pdev->name;
+	input->id.bustype = BUS_HOST;
+	input->open = adc_joystick_open;
+	input->close = adc_joystick_close;
+
+	ret = adc_joystick_set_axes(dev, joy);
+	if (ret)
+		return ret;
+
+	input_set_drvdata(input, joy);
+	ret = input_register_device(input);
+	if (ret) {
+		dev_err(dev, "Unable to register input device: %d", ret);
+		return ret;
+	}
+
+	joy->buffer = iio_channel_get_all_cb(dev, adc_joystick_handle, joy);
+	if (IS_ERR(joy->buffer)) {
+		dev_err(dev, "Unable to allocate callback buffer");
+		return PTR_ERR(joy->buffer);
+	}
+
+	ret = devm_add_action_or_reset(dev, adc_joystick_cleanup, joy->buffer);
+	if (ret)
+		dev_err(dev, "Unable to add action");
+
+	return ret;
+}
+
+static const struct of_device_id adc_joystick_of_match[] = {
+	{ .compatible = "adc-joystick", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
+
+static struct platform_driver adc_joystick_driver = {
+	.driver = {
+		.name = "adc-joystick",
+		.of_match_table = of_match_ptr(adc_joystick_of_match),
+	},
+	.probe = adc_joystick_probe,
+};
+module_platform_driver(adc_joystick_driver);
+
+MODULE_DESCRIPTION("Input driver for joysticks connected over ADC");
+MODULE_AUTHOR("Artur Rojek <contact@artur-rojek.eu>");
+MODULE_LICENSE("GPL");
-- 
2.26.1


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 20:28 ` [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode Artur Rojek
@ 2020-04-17 20:59   ` Andy Shevchenko
  2020-04-17 21:04     ` Paul Cercueil
  2020-04-19 12:19     ` Artur Rojek
  0 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 20:59 UTC (permalink / raw)
  To: Artur Rojek
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Fri, Apr 17, 2020 at 11:21 PM 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.
>
> 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.
> GCW Zero comes with a single joystick and is sufficiently handled with the
> currently implemented single-ended mode. Support for boards with two
> joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp channels
> will need to be provided in the future.
>
> 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.
>
> All the boards which probe this driver have the interrupt provided from
> devicetree, with no need to handle a case where the irq was not provided.

Device Tree
IRQ

...

> +               .scan_type = {
> +                       .sign = 'u',
> +                       .realbits = 12,

> +                       .storagebits = 16

It's slightly better to leave comma in such cases.

> +               },

> +               .scan_type = {
> +                       .sign = 'u',
> +                       .realbits = 12,

> +                       .storagebits = 16

Ditto.

> +               },

...

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_AUX,
> +               .scan_index = -1

Ditto. You see above? Isn't it nice that you didn't touch that line?
So, perhaps next developer can leverage this subtle kind of things.

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_BATTERY,
> +               .scan_index = -1

Ditto.

>                 .indexed = 1,
>                 .channel = INGENIC_ADC_AUX2,
> +               .scan_index = -1

Ditto.

...

> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
> +{
> +       struct ingenic_adc *adc = iio_priv(iio_dev);
> +

> +       clk_enable(adc->clk);

Error check?

> +       /* 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);

Why casting?

> +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
> +       ingenic_adc_enable(adc, 2, true);
> +
> +       return 0;
> +}

> +       irq = platform_get_irq(pdev, 0);

Before it worked w/o IRQ, here is a regression you introduced.

> +       if (irq < 0) {

> +               dev_err(dev, "Failed to get irq: %d\n", irq);

Redundant message.

> +               return irq;
> +       }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 20:59   ` Andy Shevchenko
@ 2020-04-17 21:04     ` Paul Cercueil
  2020-04-17 21:13       ` Andy Shevchenko
  2020-04-19 12:19     ` Artur Rojek
  1 sibling, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 21:04 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

Hi Andy,

Le ven. 17 avril 2020 à 23:59, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Fri, Apr 17, 2020 at 11:21 PM 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.
>> 
>>  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.
>>  GCW Zero comes with a single joystick and is sufficiently handled 
>> with the
>>  currently implemented single-ended mode. Support for boards with two
>>  joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp 
>> channels
>>  will need to be provided in the future.
>> 
>>  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.
>> 
>>  All the boards which probe this driver have the interrupt provided 
>> from
>>  devicetree, with no need to handle a case where the irq was not 
>> provided.
> 
> Device Tree
> IRQ
> 
> ...
> 
>>  +               .scan_type = {
>>  +                       .sign = 'u',
>>  +                       .realbits = 12,
> 
>>  +                       .storagebits = 16
> 
> It's slightly better to leave comma in such cases.
> 
>>  +               },
> 
>>  +               .scan_type = {
>>  +                       .sign = 'u',
>>  +                       .realbits = 12,
> 
>>  +                       .storagebits = 16
> 
> Ditto.
> 
>>  +               },
> 
> ...
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_AUX,
>>  +               .scan_index = -1
> 
> Ditto. You see above? Isn't it nice that you didn't touch that line?
> So, perhaps next developer can leverage this subtle kind of things.
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_BATTERY,
>>  +               .scan_index = -1
> 
> Ditto.
> 
>>                  .indexed = 1,
>>                  .channel = INGENIC_ADC_AUX2,
>>  +               .scan_index = -1
> 
> Ditto.
> 
> ...
> 
>>  +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
>>  +{
>>  +       struct ingenic_adc *adc = iio_priv(iio_dev);
>>  +
> 
>>  +       clk_enable(adc->clk);
> 
> Error check?
> 
>>  +       /* 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);
> 
> Why casting?
> 
>>  +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
>>  +       ingenic_adc_enable(adc, 2, true);
>>  +
>>  +       return 0;
>>  +}
> 
>>  +       irq = platform_get_irq(pdev, 0);
> 
> Before it worked w/o IRQ, here is a regression you introduced.

Before it simply did not need the IRQ, which is provided by the 
devicetree anyway. No regression here.

-Paul

> 
>>  +       if (irq < 0) {
> 
>>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> 
> Redundant message.
> 
>>  +               return irq;
>>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 20:28 ` [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver Artur Rojek
@ 2020-04-17 21:10   ` Andy Shevchenko
  2020-04-17 21:23     ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 21:10 UTC (permalink / raw)
  To: Artur Rojek
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu> wrote:
>
> Add a driver for joystick devices connected to ADC controllers
> supporting the Industrial I/O subsystem.

...

> +#include <linux/of.h>

Do you really need this? (See below as well)

...

> +               sign = (tolower(joy->chans[i].channel->scan_type.sign) == 's');

Too many parentheses. But here it's up to you,

...

> +               case 2:

> +                       val = ((const u16 *)data)[i];

Can't you do this in each branch below?

> +                       if (endianness == IIO_BE)
> +                               val = be16_to_cpu(val);
> +                       else if (endianness == IIO_LE)
> +                               val = le16_to_cpu(val);
> +                       break;

...

> +       device_for_each_child_node(dev, child) {
> +               ret = fwnode_property_read_u32(child, "reg", &i);
> +               if (ret || i >= num_axes) {
> +                       dev_err(dev, "reg invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32(child, "linux,code",
> +                                            &axes[i].code)) {
> +                       dev_err(dev, "linux,code invalid or missing");
> +                       goto err;
> +               }
> +
> +               if (fwnode_property_read_u32_array(child, "abs-range",
> +                                                  axes[i].range, 2)) {
> +                       dev_err(dev, "abs-range invalid or missing");
> +                       goto err;
> +               }

> +       }
> +
> +       joy->axes = axes;
> +
> +       return 0;
> +
> +err:
> +       fwnode_handle_put(child);

> +       return -EINVAL;

Can we avoid shadowing the actual error code?

...

> +       bits = joy->chans[0].channel->scan_type.storagebits;

> +       if (!bits || (bits >> 3) > 2) {

Wouldn't be clear to use simple 'bits > 16'?

> +               dev_err(dev, "Unsupported channel storage size");
> +               return -EINVAL;
> +       }

...

> +static const struct of_device_id adc_joystick_of_match[] = {
> +       { .compatible = "adc-joystick", },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> +
> +static struct platform_driver adc_joystick_driver = {
> +       .driver = {
> +               .name = "adc-joystick",

> +               .of_match_table = of_match_ptr(adc_joystick_of_match),

Drop this a bit harmful of_match_ptr() macro. It should go with ugly
#ifdeffery. Here you simple introduced a compiler warning.
On top of that, you are using device property API, OF use in this case
is contradictory (at lest to some extend).

> +       },
> +       .probe = adc_joystick_probe,
> +};

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:04     ` Paul Cercueil
@ 2020-04-17 21:13       ` Andy Shevchenko
  2020-04-17 21:18         ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 21:13 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <paul@crapouillou.net> wrote:
> Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu>
> > wrote:

...

> >>  +       irq = platform_get_irq(pdev, 0);
> >
> > Before it worked w/o IRQ, here is a regression you introduced.
>
> Before it simply did not need the IRQ, which is provided by the
> devicetree anyway. No regression here.

Does it work without IRQ? Or it was a dead code till now?
For me it's clear regression. Otherwise something is really wrong in a
process of development of this driver.

> >>  +       if (irq < 0) {
> >
> >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> >
> > Redundant message.
> >
> >>  +               return irq;
> >>  +       }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:13       ` Andy Shevchenko
@ 2020-04-17 21:18         ` Paul Cercueil
  2020-04-17 21:42           ` Andy Shevchenko
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 21:18 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 0:13, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek 
>> <contact@artur-rojek.eu>
>>  > wrote:
> 
> ...
> 
>>  >>  +       irq = platform_get_irq(pdev, 0);
>>  >
>>  > Before it worked w/o IRQ, here is a regression you introduced.
>> 
>>  Before it simply did not need the IRQ, which is provided by the
>>  devicetree anyway. No regression here.
> 
> Does it work without IRQ? Or it was a dead code till now?
> For me it's clear regression. Otherwise something is really wrong in a
> process of development of this driver.

Nothing wrong here. The IRQ was not used by the driver for the 
functionality it provided before. It is required now to support the 
touchscreen channels.

-Paul


>>  >>  +       if (irq < 0) {
>>  >
>>  >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
>>  >
>>  > Redundant message.
>>  >
>>  >>  +               return irq;
>>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 21:10   ` Andy Shevchenko
@ 2020-04-17 21:23     ` Paul Cercueil
  2020-04-17 21:49       ` Andy Shevchenko
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 21:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

Hi Andy,

Le sam. 18 avril 2020 à 0:10, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu> 
> wrote:
>> 
>>  Add a driver for joystick devices connected to ADC controllers
>>  supporting the Industrial I/O subsystem.
> 
> ...
> 
>>  +#include <linux/of.h>
> 
> Do you really need this? (See below as well)
> 
> ...
> 
>>  +               sign = 
>> (tolower(joy->chans[i].channel->scan_type.sign) == 's');
> 
> Too many parentheses. But here it's up to you,
> 
> ...
> 
>>  +               case 2:
> 
>>  +                       val = ((const u16 *)data)[i];
> 
> Can't you do this in each branch below?
> 
>>  +                       if (endianness == IIO_BE)
>>  +                               val = be16_to_cpu(val);
>>  +                       else if (endianness == IIO_LE)
>>  +                               val = le16_to_cpu(val);
>>  +                       break;
> 
> ...
> 
>>  +       device_for_each_child_node(dev, child) {
>>  +               ret = fwnode_property_read_u32(child, "reg", &i);
>>  +               if (ret || i >= num_axes) {
>>  +                       dev_err(dev, "reg invalid or missing");
>>  +                       goto err;
>>  +               }
>>  +
>>  +               if (fwnode_property_read_u32(child, "linux,code",
>>  +                                            &axes[i].code)) {
>>  +                       dev_err(dev, "linux,code invalid or 
>> missing");
>>  +                       goto err;
>>  +               }
>>  +
>>  +               if (fwnode_property_read_u32_array(child, 
>> "abs-range",
>>  +                                                  axes[i].range, 
>> 2)) {
>>  +                       dev_err(dev, "abs-range invalid or 
>> missing");
>>  +                       goto err;
>>  +               }
> 
>>  +       }
>>  +
>>  +       joy->axes = axes;
>>  +
>>  +       return 0;
>>  +
>>  +err:
>>  +       fwnode_handle_put(child);
> 
>>  +       return -EINVAL;
> 
> Can we avoid shadowing the actual error code?
> 
> ...
> 
>>  +       bits = joy->chans[0].channel->scan_type.storagebits;
> 
>>  +       if (!bits || (bits >> 3) > 2) {
> 
> Wouldn't be clear to use simple 'bits > 16'?
> 
>>  +               dev_err(dev, "Unsupported channel storage size");
>>  +               return -EINVAL;
>>  +       }
> 
> ...
> 
>>  +static const struct of_device_id adc_joystick_of_match[] = {
>>  +       { .compatible = "adc-joystick", },
>>  +       { },
>>  +};
>>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>>  +
>>  +static struct platform_driver adc_joystick_driver = {
>>  +       .driver = {
>>  +               .name = "adc-joystick",
> 
>>  +               .of_match_table = 
>> of_match_ptr(adc_joystick_of_match),
> 
> Drop this a bit harmful of_match_ptr() macro. It should go with ugly
> #ifdeffery. Here you simple introduced a compiler warning.

I assume you mean #ifdef around the of_device_id + module table macro?

> On top of that, you are using device property API, OF use in this case
> is contradictory (at lest to some extend).

I don't see why. The fact that the driver can work when probed from 
platform code, doesn't mean that it shouldn't have a table to probe 
from devicetree.

-Paul

> 
>>  +       },
>>  +       .probe = adc_joystick_probe,
>>  +};
> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:18         ` Paul Cercueil
@ 2020-04-17 21:42           ` Andy Shevchenko
  2020-04-17 21:45             ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 21:42 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net> wrote:
> Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :
> >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >> <contact@artur-rojek.eu>
> >>  > wrote:
> >
> > ...
> >
> >>  >>  +       irq = platform_get_irq(pdev, 0);
> >>  >
> >>  > Before it worked w/o IRQ, here is a regression you introduced.
> >>
> >>  Before it simply did not need the IRQ, which is provided by the
> >>  devicetree anyway. No regression here.
> >
> > Does it work without IRQ? Or it was a dead code till now?
> > For me it's clear regression. Otherwise something is really wrong in a
> > process of development of this driver.
>
> Nothing wrong here. The IRQ was not used by the driver for the
> functionality it provided before. It is required now to support the
> touchscreen channels.

This is exactly what's wrong.
Previous DTS for my (hypothetical) case has no IRQ defined. Everything
works, right?
Now, due to this change it breaks my setup. Don't you see the problem?

> >>  >>  +       if (irq < 0) {
> >>  >
> >>  >>  +               dev_err(dev, "Failed to get irq: %d\n", irq);
> >>  >
> >>  > Redundant message.
> >>  >
> >>  >>  +               return irq;
> >>  >>  +       }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:42           ` Andy Shevchenko
@ 2020-04-17 21:45             ` Paul Cercueil
  2020-04-17 21:52               ` Andy Shevchenko
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 21:45 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 0:42, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >> <contact@artur-rojek.eu>
>>  >>  > wrote:
>>  >
>>  > ...
>>  >
>>  >>  >>  +       irq = platform_get_irq(pdev, 0);
>>  >>  >
>>  >>  > Before it worked w/o IRQ, here is a regression you introduced.
>>  >>
>>  >>  Before it simply did not need the IRQ, which is provided by the
>>  >>  devicetree anyway. No regression here.
>>  >
>>  > Does it work without IRQ? Or it was a dead code till now?
>>  > For me it's clear regression. Otherwise something is really wrong 
>> in a
>>  > process of development of this driver.
>> 
>>  Nothing wrong here. The IRQ was not used by the driver for the
>>  functionality it provided before. It is required now to support the
>>  touchscreen channels.
> 
> This is exactly what's wrong.
> Previous DTS for my (hypothetical) case has no IRQ defined. Everything
> works, right?
> Now, due to this change it breaks my setup. Don't you see the problem?

The IRQ has been provided by every concerned DTS file since the 
introduction of this driver and the related bindings, even though it 
was not used by the driver.

-Paul

>>  >>  >>  +       if (irq < 0) {
>>  >>  >
>>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n", 
>> irq);
>>  >>  >
>>  >>  > Redundant message.
>>  >>  >
>>  >>  >>  +               return irq;
>>  >>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 21:23     ` Paul Cercueil
@ 2020-04-17 21:49       ` Andy Shevchenko
  2020-04-17 22:48         ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 21:49 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil <paul@crapouillou.net> wrote:
> Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek <contact@artur-rojek.eu>
> > wrote:

...

> >>  +#include <linux/of.h>
> >
> > Do you really need this? (See below as well)

> >>  +static const struct of_device_id adc_joystick_of_match[] = {
> >>  +       { .compatible = "adc-joystick", },
> >>  +       { },
> >>  +};
> >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> >>  +
> >>  +static struct platform_driver adc_joystick_driver = {
> >>  +       .driver = {
> >>  +               .name = "adc-joystick",
> >
> >>  +               .of_match_table =
> >> of_match_ptr(adc_joystick_of_match),
> >
> > Drop this a bit harmful of_match_ptr() macro. It should go with ugly
> > #ifdeffery. Here you simple introduced a compiler warning.
>
> I assume you mean #ifdef around the of_device_id + module table macro?

Yes.

> > On top of that, you are using device property API, OF use in this case
> > is contradictory (at lest to some extend).
>
> I don't see why. The fact that the driver can work when probed from
> platform code

Ha-ha, tell me how. I would like to be very surprised.

> doesn't mean that it shouldn't have a table to probe
> from devicetree.

I didn't get what you are talking about here. The idea of _unified_
device property API is to get rid of OF-centric code in favour of more
generic approach. Mixing those two can be done only in specific cases
(here is not the one).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:45             ` Paul Cercueil
@ 2020-04-17 21:52               ` Andy Shevchenko
  2020-04-17 21:56                 ` Paul Cercueil
  2020-04-19 12:54                 ` Ezequiel Garcia
  0 siblings, 2 replies; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-17 21:52 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@crapouillou.net> wrote:
> Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :
> >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
> >> <paul@crapouillou.net>
> >>  > wrote:
> >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> >>  >>  <andy.shevchenko@gmail.com> a écrit :
> >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >>  >> <contact@artur-rojek.eu>
> >>  >>  > wrote:
> >>  >
> >>  > ...
> >>  >
> >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
> >>  >>  >
> >>  >>  > Before it worked w/o IRQ, here is a regression you introduced.
> >>  >>
> >>  >>  Before it simply did not need the IRQ, which is provided by the
> >>  >>  devicetree anyway. No regression here.
> >>  >
> >>  > Does it work without IRQ? Or it was a dead code till now?
> >>  > For me it's clear regression. Otherwise something is really wrong
> >> in a
> >>  > process of development of this driver.
> >>
> >>  Nothing wrong here. The IRQ was not used by the driver for the
> >>  functionality it provided before. It is required now to support the
> >>  touchscreen channels.
> >
> > This is exactly what's wrong.
> > Previous DTS for my (hypothetical) case has no IRQ defined. Everything
> > works, right?
> > Now, due to this change it breaks my setup. Don't you see the problem?
>
> The IRQ has been provided by every concerned DTS file since the
> introduction of this driver and the related bindings, even though it
> was not used by the driver.

Can you speak for all possible DTSs/DTBs in the wild?
Okay, in any case it will be problem of maintainers and yours if
somebody complains.
I'm not going to push this anyway -- your choice.

But I see a (potential) regression.

> >>  >>  >>  +       if (irq < 0) {
> >>  >>  >
> >>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n",
> >> irq);
> >>  >>  >
> >>  >>  > Redundant message.
> >>  >>  >
> >>  >>  >>  +               return irq;
> >>  >>  >>  +       }

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:52               ` Andy Shevchenko
@ 2020-04-17 21:56                 ` Paul Cercueil
  2020-04-19 12:54                 ` Ezequiel Garcia
  1 sibling, 0 replies; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 21:56 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 0:52, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>>  >> <paul@crapouillou.net>
>>  >>  > wrote:
>>  >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  >>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >>  >> <contact@artur-rojek.eu>
>>  >>  >>  > wrote:
>>  >>  >
>>  >>  > ...
>>  >>  >
>>  >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
>>  >>  >>  >
>>  >>  >>  > Before it worked w/o IRQ, here is a regression you 
>> introduced.
>>  >>  >>
>>  >>  >>  Before it simply did not need the IRQ, which is provided by 
>> the
>>  >>  >>  devicetree anyway. No regression here.
>>  >>  >
>>  >>  > Does it work without IRQ? Or it was a dead code till now?
>>  >>  > For me it's clear regression. Otherwise something is really 
>> wrong
>>  >> in a
>>  >>  > process of development of this driver.
>>  >>
>>  >>  Nothing wrong here. The IRQ was not used by the driver for the
>>  >>  functionality it provided before. It is required now to support 
>> the
>>  >>  touchscreen channels.
>>  >
>>  > This is exactly what's wrong.
>>  > Previous DTS for my (hypothetical) case has no IRQ defined. 
>> Everything
>>  > works, right?
>>  > Now, due to this change it breaks my setup. Don't you see the 
>> problem?
>> 
>>  The IRQ has been provided by every concerned DTS file since the
>>  introduction of this driver and the related bindings, even though it
>>  was not used by the driver.
> 
> Can you speak for all possible DTSs/DTBs in the wild?
> Okay, in any case it will be problem of maintainers and yours if
> somebody complains.

I can, since I wrote all of them.

-Paul

> I'm not going to push this anyway -- your choice.
> 
> But I see a (potential) regression.
> 
>>  >>  >>  >>  +       if (irq < 0) {
>>  >>  >>  >
>>  >>  >>  >>  +               dev_err(dev, "Failed to get irq: %d\n",
>>  >> irq);
>>  >>  >>  >
>>  >>  >>  > Redundant message.
>>  >>  >>  >
>>  >>  >>  >>  +               return irq;
>>  >>  >>  >>  +       }
> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 21:49       ` Andy Shevchenko
@ 2020-04-17 22:48         ` Paul Cercueil
  2020-04-18 11:57           ` Andy Shevchenko
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-17 22:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 0:49, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek 
>> <contact@artur-rojek.eu>
>>  > wrote:
> 
> ...
> 
>>  >>  +#include <linux/of.h>
>>  >
>>  > Do you really need this? (See below as well)
> 
>>  >>  +static const struct of_device_id adc_joystick_of_match[] = {
>>  >>  +       { .compatible = "adc-joystick", },
>>  >>  +       { },
>>  >>  +};
>>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>>  >>  +
>>  >>  +static struct platform_driver adc_joystick_driver = {
>>  >>  +       .driver = {
>>  >>  +               .name = "adc-joystick",
>>  >
>>  >>  +               .of_match_table =
>>  >> of_match_ptr(adc_joystick_of_match),
>>  >
>>  > Drop this a bit harmful of_match_ptr() macro. It should go with 
>> ugly
>>  > #ifdeffery. Here you simple introduced a compiler warning.
>> 
>>  I assume you mean #ifdef around the of_device_id + module table 
>> macro?
> 
> Yes.
> 
>>  > On top of that, you are using device property API, OF use in this 
>> case
>>  > is contradictory (at lest to some extend).
>> 
>>  I don't see why. The fact that the driver can work when probed from
>>  platform code
> 
> Ha-ha, tell me how. I would like to be very surprised.

iio_map_array_register(),
pinctrl_register_mappings(),
platform_add_devices(),

you're welcome.

>>  doesn't mean that it shouldn't have a table to probe
>>  from devicetree.
> 
> I didn't get what you are talking about here. The idea of _unified_
> device property API is to get rid of OF-centric code in favour of more
> generic approach. Mixing those two can be done only in specific cases
> (here is not the one).

And how are we mixing those two here? The only OF-centric thing here is 
the device table, which is required if we want the driver to probe from 
devicetree.

-Paul

> 
> --
> With Best Regards,
> Andy Shevchenko



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-17 22:48         ` Paul Cercueil
@ 2020-04-18 11:57           ` Andy Shevchenko
  2020-04-18 12:10             ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-18 11:57 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :
> >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >> <contact@artur-rojek.eu>
> >>  > wrote:
> >
> > ...
> >
> >>  >>  +#include <linux/of.h>
> >>  >
> >>  > Do you really need this? (See below as well)
> >
> >>  >>  +static const struct of_device_id adc_joystick_of_match[] = {
> >>  >>  +       { .compatible = "adc-joystick", },
> >>  >>  +       { },
> >>  >>  +};
> >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> >>  >>  +
> >>  >>  +static struct platform_driver adc_joystick_driver = {
> >>  >>  +       .driver = {
> >>  >>  +               .name = "adc-joystick",
> >>  >
> >>  >>  +               .of_match_table =
> >>  >> of_match_ptr(adc_joystick_of_match),
> >>  >
> >>  > Drop this a bit harmful of_match_ptr() macro. It should go with
> >> ugly
> >>  > #ifdeffery. Here you simple introduced a compiler warning.
> >>
> >>  I assume you mean #ifdef around the of_device_id + module table
> >> macro?
> >
> > Yes.
> >
> >>  > On top of that, you are using device property API, OF use in this
> >> case
> >>  > is contradictory (at lest to some extend).
> >>
> >>  I don't see why. The fact that the driver can work when probed from
> >>  platform code
> >
> > Ha-ha, tell me how. I would like to be very surprised.
>
> iio_map_array_register(),
> pinctrl_register_mappings(),
> platform_add_devices(),
>
> you're welcome.

I think above has no relation to what I'm talking about.

How *this* driver can work as a platform instantiated one?
We seems have a conceptual misunderstanding here.

For example, how can probe of this driver not fail, if it is not
backed by a DT/ACPI properties?

> >>  doesn't mean that it shouldn't have a table to probe
> >>  from devicetree.
> >
> > I didn't get what you are talking about here. The idea of _unified_
> > device property API is to get rid of OF-centric code in favour of more
> > generic approach. Mixing those two can be done only in specific cases
> > (here is not the one).
>
> And how are we mixing those two here? The only OF-centric thing here is
> the device table, which is required if we want the driver to probe from
> devicetree.

Table is fine(JFYI the types and sections are defined outside of OF
stuff, though being [heavily] used by it) , API (of_match_ptr() macro
use) is not.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 11:57           ` Andy Shevchenko
@ 2020-04-18 12:10             ` Paul Cercueil
  2020-04-18 12:42               ` Andy Shevchenko
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-18 12:10 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 14:57, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>> 
>> 
>> 
>>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >> <contact@artur-rojek.eu>
>>  >>  > wrote:
>>  >
>>  > ...
>>  >
>>  >>  >>  +#include <linux/of.h>
>>  >>  >
>>  >>  > Do you really need this? (See below as well)
>>  >
>>  >>  >>  +static const struct of_device_id adc_joystick_of_match[] = 
>> {
>>  >>  >>  +       { .compatible = "adc-joystick", },
>>  >>  >>  +       { },
>>  >>  >>  +};
>>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>>  >>  >>  +
>>  >>  >>  +static struct platform_driver adc_joystick_driver = {
>>  >>  >>  +       .driver = {
>>  >>  >>  +               .name = "adc-joystick",
>>  >>  >
>>  >>  >>  +               .of_match_table =
>>  >>  >> of_match_ptr(adc_joystick_of_match),
>>  >>  >
>>  >>  > Drop this a bit harmful of_match_ptr() macro. It should go 
>> with
>>  >> ugly
>>  >>  > #ifdeffery. Here you simple introduced a compiler warning.
>>  >>
>>  >>  I assume you mean #ifdef around the of_device_id + module table
>>  >> macro?
>>  >
>>  > Yes.
>>  >
>>  >>  > On top of that, you are using device property API, OF use in 
>> this
>>  >> case
>>  >>  > is contradictory (at lest to some extend).
>>  >>
>>  >>  I don't see why. The fact that the driver can work when probed 
>> from
>>  >>  platform code
>>  >
>>  > Ha-ha, tell me how. I would like to be very surprised.
>> 
>>  iio_map_array_register(),
>>  pinctrl_register_mappings(),
>>  platform_add_devices(),
>> 
>>  you're welcome.
> 
> I think above has no relation to what I'm talking about.

Yes it does. It allows you to map the IIO channels, set the pinctrl 
configurations and register a device from platform code instead of 
devicetree.

> How *this* driver can work as a platform instantiated one?
> We seems have a conceptual misunderstanding here.
> 
> For example, how can probe of this driver not fail, if it is not
> backed by a DT/ACPI properties?

platform_device_add_properties().

> 
>>  >>  doesn't mean that it shouldn't have a table to probe
>>  >>  from devicetree.
>>  >
>>  > I didn't get what you are talking about here. The idea of 
>> _unified_
>>  > device property API is to get rid of OF-centric code in favour of 
>> more
>>  > generic approach. Mixing those two can be done only in specific 
>> cases
>>  > (here is not the one).
>> 
>>  And how are we mixing those two here? The only OF-centric thing 
>> here is
>>  the device table, which is required if we want the driver to probe 
>> from
>>  devicetree.
> 
> Table is fine(JFYI the types and sections are defined outside of OF
> stuff, though being [heavily] used by it) , API (of_match_ptr() macro
> use) is not.

Sorry, but that's just stupid. Please have a look at how of_match_ptr() 
macro is defined in <linux/of.h>.

-Paul



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 12:10             ` Paul Cercueil
@ 2020-04-18 12:42               ` Andy Shevchenko
  2020-04-18 13:24                 ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Andy Shevchenko @ 2020-04-18 12:42 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil <paul@crapouillou.net> wrote:
> Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
> <andy.shevchenko@gmail.com> a écrit :
> > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :
> >>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil
> >> <paul@crapouillou.net>
> >>  > wrote:
> >>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
> >>  >>  <andy.shevchenko@gmail.com> a écrit :
> >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> >>  >> <contact@artur-rojek.eu>
> >>  >>  > wrote:

...

> >>  >>  >>  +#include <linux/of.h>
> >>  >>  >
> >>  >>  > Do you really need this? (See below as well)
> >>  >
> >>  >>  >>  +static const struct of_device_id adc_joystick_of_match[] =
> >> {
> >>  >>  >>  +       { .compatible = "adc-joystick", },
> >>  >>  >>  +       { },
> >>  >>  >>  +};
> >>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> >>  >>  >>  +
> >>  >>  >>  +static struct platform_driver adc_joystick_driver = {
> >>  >>  >>  +       .driver = {
> >>  >>  >>  +               .name = "adc-joystick",
> >>  >>  >
> >>  >>  >>  +               .of_match_table =
> >>  >>  >> of_match_ptr(adc_joystick_of_match),
> >>  >>  >
> >>  >>  > Drop this a bit harmful of_match_ptr() macro. It should go
> >> with
> >>  >> ugly
> >>  >>  > #ifdeffery. Here you simple introduced a compiler warning.
> >>  >>
> >>  >>  I assume you mean #ifdef around the of_device_id + module table
> >>  >> macro?
> >>  >
> >>  > Yes.
> >>  >
> >>  >>  > On top of that, you are using device property API, OF use in
> >> this
> >>  >> case
> >>  >>  > is contradictory (at lest to some extend).
> >>  >>
> >>  >>  I don't see why. The fact that the driver can work when probed
> >> from
> >>  >>  platform code
> >>  >
> >>  > Ha-ha, tell me how. I would like to be very surprised.
> >>
> >>  iio_map_array_register(),
> >>  pinctrl_register_mappings(),
> >>  platform_add_devices(),
> >>
> >>  you're welcome.
> >
> > I think above has no relation to what I'm talking about.
>
> Yes it does. It allows you to map the IIO channels, set the pinctrl
> configurations and register a device from platform code instead of
> devicetree.

I'm not talking about other drivers, I'm talking about this driver and
how it will be instantiated. Above, according to the code, can't be
comprehensive to fulfill this.

> > How *this* driver can work as a platform instantiated one?
> > We seems have a conceptual misunderstanding here.
> >
> > For example, how can probe of this driver not fail, if it is not
> > backed by a DT/ACPI properties?
>
> platform_device_add_properties().

Yes, I waited for this. And seems you don't understand the (scope of)
API, you are trying to insist this driver can be used as a platform
one.
Sorry, I must to disappoint you, it can't. Above interface is created
solely for quirks to support (broken) DT/ACPI tables. It's not
supposed to be used as a main source for the device properties.

> >>  >>  doesn't mean that it shouldn't have a table to probe
> >>  >>  from devicetree.
> >>  >
> >>  > I didn't get what you are talking about here. The idea of
> >> _unified_
> >>  > device property API is to get rid of OF-centric code in favour of
> >> more
> >>  > generic approach. Mixing those two can be done only in specific
> >> cases
> >>  > (here is not the one).
> >>
> >>  And how are we mixing those two here? The only OF-centric thing
> >> here is
> >>  the device table, which is required if we want the driver to probe
> >> from
> >>  devicetree.
> >
> > Table is fine(JFYI the types and sections are defined outside of OF
> > stuff, though being [heavily] used by it) , API (of_match_ptr() macro
> > use) is not.
>
> Sorry, but that's just stupid. Please have a look at how of_match_ptr()
> macro is defined in <linux/of.h>.

Call it whatever you want, but above code is broken.
It needs either of:
- ugly ifdeffery
- dropping of_match_ptr()
- explicit dependence to OF

My choice is second one. Because it makes code better and allows also
ACPI to use this driver (usually) without changes.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 12:42               ` Andy Shevchenko
@ 2020-04-18 13:24                 ` Paul Cercueil
  2020-04-18 14:22                   ` Jonathan Cameron
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-18 13:24 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Artur Rojek, Dmitry Torokhov, Rob Herring, Mark Rutland,
	Jonathan Cameron, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List



Le sam. 18 avril 2020 à 15:42, Andy Shevchenko 
<andy.shevchenko@gmail.com> a écrit :
> On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil <paul@crapouillou.net> 
> wrote:
>>  Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil
>>  >> <paul@crapouillou.net>
>>  >>  > wrote:
>>  >>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>>  >>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >>  >> <contact@artur-rojek.eu>
>>  >>  >>  > wrote:
> 
> ...
> 
>>  >>  >>  >>  +#include <linux/of.h>
>>  >>  >>  >
>>  >>  >>  > Do you really need this? (See below as well)
>>  >>  >
>>  >>  >>  >>  +static const struct of_device_id 
>> adc_joystick_of_match[] =
>>  >> {
>>  >>  >>  >>  +       { .compatible = "adc-joystick", },
>>  >>  >>  >>  +       { },
>>  >>  >>  >>  +};
>>  >>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>>  >>  >>  >>  +
>>  >>  >>  >>  +static struct platform_driver adc_joystick_driver = {
>>  >>  >>  >>  +       .driver = {
>>  >>  >>  >>  +               .name = "adc-joystick",
>>  >>  >>  >
>>  >>  >>  >>  +               .of_match_table =
>>  >>  >>  >> of_match_ptr(adc_joystick_of_match),
>>  >>  >>  >
>>  >>  >>  > Drop this a bit harmful of_match_ptr() macro. It should go
>>  >> with
>>  >>  >> ugly
>>  >>  >>  > #ifdeffery. Here you simple introduced a compiler warning.
>>  >>  >>
>>  >>  >>  I assume you mean #ifdef around the of_device_id + module 
>> table
>>  >>  >> macro?
>>  >>  >
>>  >>  > Yes.
>>  >>  >
>>  >>  >>  > On top of that, you are using device property API, OF use 
>> in
>>  >> this
>>  >>  >> case
>>  >>  >>  > is contradictory (at lest to some extend).
>>  >>  >>
>>  >>  >>  I don't see why. The fact that the driver can work when 
>> probed
>>  >> from
>>  >>  >>  platform code
>>  >>  >
>>  >>  > Ha-ha, tell me how. I would like to be very surprised.
>>  >>
>>  >>  iio_map_array_register(),
>>  >>  pinctrl_register_mappings(),
>>  >>  platform_add_devices(),
>>  >>
>>  >>  you're welcome.
>>  >
>>  > I think above has no relation to what I'm talking about.
>> 
>>  Yes it does. It allows you to map the IIO channels, set the pinctrl
>>  configurations and register a device from platform code instead of
>>  devicetree.
> 
> I'm not talking about other drivers, I'm talking about this driver and
> how it will be instantiated. Above, according to the code, can't be
> comprehensive to fulfill this.

This is how the platform devices were instanciated on JZ4740 before we 
switched everything to devicetree.

>>  > How *this* driver can work as a platform instantiated one?
>>  > We seems have a conceptual misunderstanding here.
>>  >
>>  > For example, how can probe of this driver not fail, if it is not
>>  > backed by a DT/ACPI properties?
>> 
>>  platform_device_add_properties().
> 
> Yes, I waited for this. And seems you don't understand the (scope of)
> API, you are trying to insist this driver can be used as a platform
> one.
> Sorry, I must to disappoint you, it can't. Above interface is created
> solely for quirks to support (broken) DT/ACPI tables. It's not
> supposed to be used as a main source for the device properties.

The fact that it was designed for something else doesn't mean it can't 
be used.

Anyway, this discussion is pointless. I don't think anybody would want 
to do that.

>>  >>  >>  doesn't mean that it shouldn't have a table to probe
>>  >>  >>  from devicetree.
>>  >>  >
>>  >>  > I didn't get what you are talking about here. The idea of
>>  >> _unified_
>>  >>  > device property API is to get rid of OF-centric code in 
>> favour of
>>  >> more
>>  >>  > generic approach. Mixing those two can be done only in 
>> specific
>>  >> cases
>>  >>  > (here is not the one).
>>  >>
>>  >>  And how are we mixing those two here? The only OF-centric thing
>>  >> here is
>>  >>  the device table, which is required if we want the driver to 
>> probe
>>  >> from
>>  >>  devicetree.
>>  >
>>  > Table is fine(JFYI the types and sections are defined outside of 
>> OF
>>  > stuff, though being [heavily] used by it) , API (of_match_ptr() 
>> macro
>>  > use) is not.
>> 
>>  Sorry, but that's just stupid. Please have a look at how 
>> of_match_ptr()
>>  macro is defined in <linux/of.h>.
> 
> Call it whatever you want, but above code is broken.

of_match_ptr() is basically defined like this:

#ifdef CONFIG_OF
#define of_match_ptr(x) (x)
#else
#define of_match_ptr(x) NULL
#endif

So please, enlighten me, tell me what is so wrong about what's being 
done here.

> It needs either of:
> - ugly ifdeffery
> - dropping of_match_ptr()
> - explicit dependence to OF
> 
> My choice is second one. Because it makes code better and allows also
> ACPI to use this driver (usually) without changes.

And how is unconditionally compiling the of_match_table make it 
magically probe from ACPI, without a acpi_match_table?

-Paul



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 13:24                 ` Paul Cercueil
@ 2020-04-18 14:22                   ` Jonathan Cameron
  2020-04-18 17:25                     ` Paul Cercueil
  0 siblings, 1 reply; 28+ messages in thread
From: Jonathan Cameron @ 2020-04-18 14:22 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Andy Shevchenko, Artur Rojek, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Heiko Stuebner, linux-input, devicetree, linux-iio,
	Linux Kernel Mailing List

On Sat, 18 Apr 2020 15:24:58 +0200
Paul Cercueil <paul@crapouillou.net> wrote:

> Le sam. 18 avril 2020 à 15:42, Andy Shevchenko 
> <andy.shevchenko@gmail.com> a écrit :
> > On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil <paul@crapouillou.net> 
> > wrote:  
> >>  Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil   
> >> <paul@crapouillou.net>  
> >>  > wrote:  
> >>  >>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
> >>  >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  >>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil  
> >>  >> <paul@crapouillou.net>  
> >>  >>  > wrote:  
> >>  >>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
> >>  >>  >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek  
> >>  >>  >> <contact@artur-rojek.eu>  
> >>  >>  >>  > wrote:  
> > 
> > ...
> >   
> >>  >>  >>  >>  +#include <linux/of.h>  
> >>  >>  >>  >
> >>  >>  >>  > Do you really need this? (See below as well)  
> >>  >>  >  
> >>  >>  >>  >>  +static const struct of_device_id   
> >> adc_joystick_of_match[] =  
> >>  >> {  
> >>  >>  >>  >>  +       { .compatible = "adc-joystick", },
> >>  >>  >>  >>  +       { },
> >>  >>  >>  >>  +};
> >>  >>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> >>  >>  >>  >>  +
> >>  >>  >>  >>  +static struct platform_driver adc_joystick_driver = {
> >>  >>  >>  >>  +       .driver = {
> >>  >>  >>  >>  +               .name = "adc-joystick",  
> >>  >>  >>  >  
> >>  >>  >>  >>  +               .of_match_table =
> >>  >>  >>  >> of_match_ptr(adc_joystick_of_match),  
> >>  >>  >>  >
> >>  >>  >>  > Drop this a bit harmful of_match_ptr() macro. It should go  
> >>  >> with  
> >>  >>  >> ugly  
> >>  >>  >>  > #ifdeffery. Here you simple introduced a compiler warning.  
> >>  >>  >>
> >>  >>  >>  I assume you mean #ifdef around the of_device_id + module   
> >> table  
> >>  >>  >> macro?  
> >>  >>  >
> >>  >>  > Yes.
> >>  >>  >  
> >>  >>  >>  > On top of that, you are using device property API, OF use   
> >> in  
> >>  >> this  
> >>  >>  >> case  
> >>  >>  >>  > is contradictory (at lest to some extend).  
> >>  >>  >>
> >>  >>  >>  I don't see why. The fact that the driver can work when   
> >> probed  
> >>  >> from  
> >>  >>  >>  platform code  
> >>  >>  >
> >>  >>  > Ha-ha, tell me how. I would like to be very surprised.  
> >>  >>
> >>  >>  iio_map_array_register(),
> >>  >>  pinctrl_register_mappings(),
> >>  >>  platform_add_devices(),
> >>  >>
> >>  >>  you're welcome.  
> >>  >
> >>  > I think above has no relation to what I'm talking about.  
> >> 
> >>  Yes it does. It allows you to map the IIO channels, set the pinctrl
> >>  configurations and register a device from platform code instead of
> >>  devicetree.  
> > 
> > I'm not talking about other drivers, I'm talking about this driver and
> > how it will be instantiated. Above, according to the code, can't be
> > comprehensive to fulfill this.  
> 
> This is how the platform devices were instanciated on JZ4740 before we 
> switched everything to devicetree.
> 
> >>  > How *this* driver can work as a platform instantiated one?
> >>  > We seems have a conceptual misunderstanding here.
> >>  >
> >>  > For example, how can probe of this driver not fail, if it is not
> >>  > backed by a DT/ACPI properties?  
> >> 
> >>  platform_device_add_properties().  
> > 
> > Yes, I waited for this. And seems you don't understand the (scope of)
> > API, you are trying to insist this driver can be used as a platform
> > one.
> > Sorry, I must to disappoint you, it can't. Above interface is created
> > solely for quirks to support (broken) DT/ACPI tables. It's not
> > supposed to be used as a main source for the device properties.  
> 
> The fact that it was designed for something else doesn't mean it can't 
> be used.
> 
> Anyway, this discussion is pointless. I don't think anybody would want 
> to do that.
> 
> >>  >>  >>  doesn't mean that it shouldn't have a table to probe
> >>  >>  >>  from devicetree.  
> >>  >>  >
> >>  >>  > I didn't get what you are talking about here. The idea of  
> >>  >> _unified_  
> >>  >>  > device property API is to get rid of OF-centric code in   
> >> favour of  
> >>  >> more  
> >>  >>  > generic approach. Mixing those two can be done only in   
> >> specific  
> >>  >> cases  
> >>  >>  > (here is not the one).  
> >>  >>
> >>  >>  And how are we mixing those two here? The only OF-centric thing
> >>  >> here is
> >>  >>  the device table, which is required if we want the driver to   
> >> probe  
> >>  >> from
> >>  >>  devicetree.  
> >>  >
> >>  > Table is fine(JFYI the types and sections are defined outside of   
> >> OF  
> >>  > stuff, though being [heavily] used by it) , API (of_match_ptr()   
> >> macro  
> >>  > use) is not.  
> >> 
> >>  Sorry, but that's just stupid. Please have a look at how 
> >> of_match_ptr()
> >>  macro is defined in <linux/of.h>.  
> > 
> > Call it whatever you want, but above code is broken.  
> 
> of_match_ptr() is basically defined like this:
> 
> #ifdef CONFIG_OF
> #define of_match_ptr(x) (x)
> #else
> #define of_match_ptr(x) NULL
> #endif
> 
> So please, enlighten me, tell me what is so wrong about what's being 
> done here.
> 
> > It needs either of:
> > - ugly ifdeffery
> > - dropping of_match_ptr()
> > - explicit dependence to OF
> > 
> > My choice is second one. Because it makes code better and allows also
> > ACPI to use this driver (usually) without changes.  
> 
> And how is unconditionally compiling the of_match_table make it 
> magically probe from ACPI, without a acpi_match_table?
> 
> -Paul

Look up PRP0001 ACPI ID.  Magic trick ;)

https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001

It allows you to define an ACPI device in DSDT that is instantiated
from what is effectively the DT binding including the id table.

Jonathan

> 
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 14:22                   ` Jonathan Cameron
@ 2020-04-18 17:25                     ` Paul Cercueil
  2020-04-18 18:20                       ` Jonathan Cameron
  0 siblings, 1 reply; 28+ messages in thread
From: Paul Cercueil @ 2020-04-18 17:25 UTC (permalink / raw)
  To: Jonathan Cameron
  Cc: Andy Shevchenko, Artur Rojek, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Heiko Stuebner, linux-input, devicetree, linux-iio,
	Linux Kernel Mailing List

Hi Jonathan,

Le sam. 18 avril 2020 à 15:22, Jonathan Cameron <jic23@kernel.org> a 
écrit :
> On Sat, 18 Apr 2020 15:24:58 +0200
> Paul Cercueil <paul@crapouillou.net> wrote:
> 
>>  Le sam. 18 avril 2020 à 15:42, Andy Shevchenko
>>  <andy.shevchenko@gmail.com> a écrit :
>>  > On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > wrote:
>>  >>  Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
>>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil
>>  >> <paul@crapouillou.net>
>>  >>  > wrote:
>>  >>  >>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
>>  >>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  >>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil
>>  >>  >> <paul@crapouillou.net>
>>  >>  >>  > wrote:
>>  >>  >>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
>>  >>  >>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  >>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  >>  >>  >> <contact@artur-rojek.eu>
>>  >>  >>  >>  > wrote:
>>  >
>>  > ...
>>  >
>>  >>  >>  >>  >>  +#include <linux/of.h>
>>  >>  >>  >>  >
>>  >>  >>  >>  > Do you really need this? (See below as well)
>>  >>  >>  >
>>  >>  >>  >>  >>  +static const struct of_device_id
>>  >> adc_joystick_of_match[] =
>>  >>  >> {
>>  >>  >>  >>  >>  +       { .compatible = "adc-joystick", },
>>  >>  >>  >>  >>  +       { },
>>  >>  >>  >>  >>  +};
>>  >>  >>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
>>  >>  >>  >>  >>  +
>>  >>  >>  >>  >>  +static struct platform_driver adc_joystick_driver 
>> = {
>>  >>  >>  >>  >>  +       .driver = {
>>  >>  >>  >>  >>  +               .name = "adc-joystick",
>>  >>  >>  >>  >
>>  >>  >>  >>  >>  +               .of_match_table =
>>  >>  >>  >>  >> of_match_ptr(adc_joystick_of_match),
>>  >>  >>  >>  >
>>  >>  >>  >>  > Drop this a bit harmful of_match_ptr() macro. It 
>> should go
>>  >>  >> with
>>  >>  >>  >> ugly
>>  >>  >>  >>  > #ifdeffery. Here you simple introduced a compiler 
>> warning.
>>  >>  >>  >>
>>  >>  >>  >>  I assume you mean #ifdef around the of_device_id + 
>> module
>>  >> table
>>  >>  >>  >> macro?
>>  >>  >>  >
>>  >>  >>  > Yes.
>>  >>  >>  >
>>  >>  >>  >>  > On top of that, you are using device property API, OF 
>> use
>>  >> in
>>  >>  >> this
>>  >>  >>  >> case
>>  >>  >>  >>  > is contradictory (at lest to some extend).
>>  >>  >>  >>
>>  >>  >>  >>  I don't see why. The fact that the driver can work when
>>  >> probed
>>  >>  >> from
>>  >>  >>  >>  platform code
>>  >>  >>  >
>>  >>  >>  > Ha-ha, tell me how. I would like to be very surprised.
>>  >>  >>
>>  >>  >>  iio_map_array_register(),
>>  >>  >>  pinctrl_register_mappings(),
>>  >>  >>  platform_add_devices(),
>>  >>  >>
>>  >>  >>  you're welcome.
>>  >>  >
>>  >>  > I think above has no relation to what I'm talking about.
>>  >>
>>  >>  Yes it does. It allows you to map the IIO channels, set the 
>> pinctrl
>>  >>  configurations and register a device from platform code instead 
>> of
>>  >>  devicetree.
>>  >
>>  > I'm not talking about other drivers, I'm talking about this 
>> driver and
>>  > how it will be instantiated. Above, according to the code, can't 
>> be
>>  > comprehensive to fulfill this.
>> 
>>  This is how the platform devices were instanciated on JZ4740 before 
>> we
>>  switched everything to devicetree.
>> 
>>  >>  > How *this* driver can work as a platform instantiated one?
>>  >>  > We seems have a conceptual misunderstanding here.
>>  >>  >
>>  >>  > For example, how can probe of this driver not fail, if it is 
>> not
>>  >>  > backed by a DT/ACPI properties?
>>  >>
>>  >>  platform_device_add_properties().
>>  >
>>  > Yes, I waited for this. And seems you don't understand the (scope 
>> of)
>>  > API, you are trying to insist this driver can be used as a 
>> platform
>>  > one.
>>  > Sorry, I must to disappoint you, it can't. Above interface is 
>> created
>>  > solely for quirks to support (broken) DT/ACPI tables. It's not
>>  > supposed to be used as a main source for the device properties.
>> 
>>  The fact that it was designed for something else doesn't mean it 
>> can't
>>  be used.
>> 
>>  Anyway, this discussion is pointless. I don't think anybody would 
>> want
>>  to do that.
>> 
>>  >>  >>  >>  doesn't mean that it shouldn't have a table to probe
>>  >>  >>  >>  from devicetree.
>>  >>  >>  >
>>  >>  >>  > I didn't get what you are talking about here. The idea of
>>  >>  >> _unified_
>>  >>  >>  > device property API is to get rid of OF-centric code in
>>  >> favour of
>>  >>  >> more
>>  >>  >>  > generic approach. Mixing those two can be done only in
>>  >> specific
>>  >>  >> cases
>>  >>  >>  > (here is not the one).
>>  >>  >>
>>  >>  >>  And how are we mixing those two here? The only OF-centric 
>> thing
>>  >>  >> here is
>>  >>  >>  the device table, which is required if we want the driver to
>>  >> probe
>>  >>  >> from
>>  >>  >>  devicetree.
>>  >>  >
>>  >>  > Table is fine(JFYI the types and sections are defined outside 
>> of
>>  >> OF
>>  >>  > stuff, though being [heavily] used by it) , API 
>> (of_match_ptr()
>>  >> macro
>>  >>  > use) is not.
>>  >>
>>  >>  Sorry, but that's just stupid. Please have a look at how
>>  >> of_match_ptr()
>>  >>  macro is defined in <linux/of.h>.
>>  >
>>  > Call it whatever you want, but above code is broken.
>> 
>>  of_match_ptr() is basically defined like this:
>> 
>>  #ifdef CONFIG_OF
>>  #define of_match_ptr(x) (x)
>>  #else
>>  #define of_match_ptr(x) NULL
>>  #endif
>> 
>>  So please, enlighten me, tell me what is so wrong about what's being
>>  done here.
>> 
>>  > It needs either of:
>>  > - ugly ifdeffery
>>  > - dropping of_match_ptr()
>>  > - explicit dependence to OF
>>  >
>>  > My choice is second one. Because it makes code better and allows 
>> also
>>  > ACPI to use this driver (usually) without changes.
>> 
>>  And how is unconditionally compiling the of_match_table make it
>>  magically probe from ACPI, without a acpi_match_table?
>> 
>>  -Paul
> 
> Look up PRP0001 ACPI ID.  Magic trick ;)
> 
> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001
> 
> It allows you to define an ACPI device in DSDT that is instantiated
> from what is effectively the DT binding including the id table.

So what you're saying, is that the OF table should be present, even 
though CONFIG_OF is not set, just in case it is probed from ACPI?

-Paul



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver.
  2020-04-18 17:25                     ` Paul Cercueil
@ 2020-04-18 18:20                       ` Jonathan Cameron
  0 siblings, 0 replies; 28+ messages in thread
From: Jonathan Cameron @ 2020-04-18 18:20 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: Andy Shevchenko, Artur Rojek, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Heiko Stuebner, linux-input, devicetree, linux-iio,
	Linux Kernel Mailing List

On Sat, 18 Apr 2020 19:25:15 +0200
Paul Cercueil <paul@crapouillou.net> wrote:

> Hi Jonathan,
> 
> Le sam. 18 avril 2020 à 15:22, Jonathan Cameron <jic23@kernel.org> a 
> écrit :
> > On Sat, 18 Apr 2020 15:24:58 +0200
> > Paul Cercueil <paul@crapouillou.net> wrote:
> >   
> >>  Le sam. 18 avril 2020 à 15:42, Andy Shevchenko
> >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  > On Sat, Apr 18, 2020 at 3:10 PM Paul Cercueil   
> >> <paul@crapouillou.net>  
> >>  > wrote:  
> >>  >>  Le sam. 18 avril 2020 à 14:57, Andy Shevchenko
> >>  >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  >>  > On Sat, Apr 18, 2020 at 1:48 AM Paul Cercueil  
> >>  >> <paul@crapouillou.net>  
> >>  >>  > wrote:  
> >>  >>  >>  Le sam. 18 avril 2020 à 0:49, Andy Shevchenko
> >>  >>  >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  >>  >>  > On Sat, Apr 18, 2020 at 12:24 AM Paul Cercueil  
> >>  >>  >> <paul@crapouillou.net>  
> >>  >>  >>  > wrote:  
> >>  >>  >>  >>  Le sam. 18 avril 2020 à 0:10, Andy Shevchenko
> >>  >>  >>  >>  <andy.shevchenko@gmail.com> a écrit :  
> >>  >>  >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek  
> >>  >>  >>  >> <contact@artur-rojek.eu>  
> >>  >>  >>  >>  > wrote:  
> >>  >
> >>  > ...
> >>  >  
> >>  >>  >>  >>  >>  +#include <linux/of.h>  
> >>  >>  >>  >>  >
> >>  >>  >>  >>  > Do you really need this? (See below as well)  
> >>  >>  >>  >  
> >>  >>  >>  >>  >>  +static const struct of_device_id  
> >>  >> adc_joystick_of_match[] =  
> >>  >>  >> {  
> >>  >>  >>  >>  >>  +       { .compatible = "adc-joystick", },
> >>  >>  >>  >>  >>  +       { },
> >>  >>  >>  >>  >>  +};
> >>  >>  >>  >>  >>  +MODULE_DEVICE_TABLE(of, adc_joystick_of_match);
> >>  >>  >>  >>  >>  +
> >>  >>  >>  >>  >>  +static struct platform_driver adc_joystick_driver   
> >> = {  
> >>  >>  >>  >>  >>  +       .driver = {
> >>  >>  >>  >>  >>  +               .name = "adc-joystick",  
> >>  >>  >>  >>  >  
> >>  >>  >>  >>  >>  +               .of_match_table =
> >>  >>  >>  >>  >> of_match_ptr(adc_joystick_of_match),  
> >>  >>  >>  >>  >
> >>  >>  >>  >>  > Drop this a bit harmful of_match_ptr() macro. It   
> >> should go  
> >>  >>  >> with  
> >>  >>  >>  >> ugly  
> >>  >>  >>  >>  > #ifdeffery. Here you simple introduced a compiler   
> >> warning.  
> >>  >>  >>  >>
> >>  >>  >>  >>  I assume you mean #ifdef around the of_device_id +   
> >> module  
> >>  >> table  
> >>  >>  >>  >> macro?  
> >>  >>  >>  >
> >>  >>  >>  > Yes.
> >>  >>  >>  >  
> >>  >>  >>  >>  > On top of that, you are using device property API, OF   
> >> use  
> >>  >> in  
> >>  >>  >> this  
> >>  >>  >>  >> case  
> >>  >>  >>  >>  > is contradictory (at lest to some extend).  
> >>  >>  >>  >>
> >>  >>  >>  >>  I don't see why. The fact that the driver can work when  
> >>  >> probed  
> >>  >>  >> from  
> >>  >>  >>  >>  platform code  
> >>  >>  >>  >
> >>  >>  >>  > Ha-ha, tell me how. I would like to be very surprised.  
> >>  >>  >>
> >>  >>  >>  iio_map_array_register(),
> >>  >>  >>  pinctrl_register_mappings(),
> >>  >>  >>  platform_add_devices(),
> >>  >>  >>
> >>  >>  >>  you're welcome.  
> >>  >>  >
> >>  >>  > I think above has no relation to what I'm talking about.  
> >>  >>
> >>  >>  Yes it does. It allows you to map the IIO channels, set the   
> >> pinctrl  
> >>  >>  configurations and register a device from platform code instead   
> >> of  
> >>  >>  devicetree.  
> >>  >
> >>  > I'm not talking about other drivers, I'm talking about this   
> >> driver and  
> >>  > how it will be instantiated. Above, according to the code, can't   
> >> be  
> >>  > comprehensive to fulfill this.  
> >> 
> >>  This is how the platform devices were instanciated on JZ4740 before 
> >> we
> >>  switched everything to devicetree.
> >>   
> >>  >>  > How *this* driver can work as a platform instantiated one?
> >>  >>  > We seems have a conceptual misunderstanding here.
> >>  >>  >
> >>  >>  > For example, how can probe of this driver not fail, if it is   
> >> not  
> >>  >>  > backed by a DT/ACPI properties?  
> >>  >>
> >>  >>  platform_device_add_properties().  
> >>  >
> >>  > Yes, I waited for this. And seems you don't understand the (scope   
> >> of)  
> >>  > API, you are trying to insist this driver can be used as a   
> >> platform  
> >>  > one.
> >>  > Sorry, I must to disappoint you, it can't. Above interface is   
> >> created  
> >>  > solely for quirks to support (broken) DT/ACPI tables. It's not
> >>  > supposed to be used as a main source for the device properties.  
> >> 
> >>  The fact that it was designed for something else doesn't mean it 
> >> can't
> >>  be used.
> >> 
> >>  Anyway, this discussion is pointless. I don't think anybody would 
> >> want
> >>  to do that.
> >>   
> >>  >>  >>  >>  doesn't mean that it shouldn't have a table to probe
> >>  >>  >>  >>  from devicetree.  
> >>  >>  >>  >
> >>  >>  >>  > I didn't get what you are talking about here. The idea of  
> >>  >>  >> _unified_  
> >>  >>  >>  > device property API is to get rid of OF-centric code in  
> >>  >> favour of  
> >>  >>  >> more  
> >>  >>  >>  > generic approach. Mixing those two can be done only in  
> >>  >> specific  
> >>  >>  >> cases  
> >>  >>  >>  > (here is not the one).  
> >>  >>  >>
> >>  >>  >>  And how are we mixing those two here? The only OF-centric   
> >> thing  
> >>  >>  >> here is
> >>  >>  >>  the device table, which is required if we want the driver to  
> >>  >> probe  
> >>  >>  >> from
> >>  >>  >>  devicetree.  
> >>  >>  >
> >>  >>  > Table is fine(JFYI the types and sections are defined outside   
> >> of  
> >>  >> OF  
> >>  >>  > stuff, though being [heavily] used by it) , API   
> >> (of_match_ptr()  
> >>  >> macro  
> >>  >>  > use) is not.  
> >>  >>
> >>  >>  Sorry, but that's just stupid. Please have a look at how
> >>  >> of_match_ptr()
> >>  >>  macro is defined in <linux/of.h>.  
> >>  >
> >>  > Call it whatever you want, but above code is broken.  
> >> 
> >>  of_match_ptr() is basically defined like this:
> >> 
> >>  #ifdef CONFIG_OF
> >>  #define of_match_ptr(x) (x)
> >>  #else
> >>  #define of_match_ptr(x) NULL
> >>  #endif
> >> 
> >>  So please, enlighten me, tell me what is so wrong about what's being
> >>  done here.
> >>   
> >>  > It needs either of:
> >>  > - ugly ifdeffery
> >>  > - dropping of_match_ptr()
> >>  > - explicit dependence to OF
> >>  >
> >>  > My choice is second one. Because it makes code better and allows   
> >> also  
> >>  > ACPI to use this driver (usually) without changes.  
> >> 
> >>  And how is unconditionally compiling the of_match_table make it
> >>  magically probe from ACPI, without a acpi_match_table?
> >> 
> >>  -Paul  
> > 
> > Look up PRP0001 ACPI ID.  Magic trick ;)
> > 
> > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/enumeration.html?highlight=PRP0001
> > 
> > It allows you to define an ACPI device in DSDT that is instantiated
> > from what is effectively the DT binding including the id table.  
> 
> So what you're saying, is that the OF table should be present, even 
> though CONFIG_OF is not set, just in case it is probed from ACPI?

Exactly.  Weird isn't it :)



> 
> -Paul
> 
> 


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 20:59   ` Andy Shevchenko
  2020-04-17 21:04     ` Paul Cercueil
@ 2020-04-19 12:19     ` Artur Rojek
  1 sibling, 0 replies; 28+ messages in thread
From: Artur Rojek @ 2020-04-19 12:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Dmitry Torokhov, Rob Herring, Mark Rutland, Jonathan Cameron,
	Paul Cercueil, Heiko Stuebner, linux-input, devicetree,
	linux-iio, Linux Kernel Mailing List

Hi Andy,
thanks for the review. Comments inline.

On 2020-04-17 22:59, Andy Shevchenko wrote:
> On Fri, Apr 17, 2020 at 11:21 PM 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.
>> 
>> 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.
>> GCW Zero comes with a single joystick and is sufficiently handled with 
>> the
>> currently implemented single-ended mode. Support for boards with two
>> joysticks, where one is hooked up to Xn/Yn and the other to Xp/Yp 
>> channels
>> will need to be provided in the future.
>> 
>> 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.
>> 
>> All the boards which probe this driver have the interrupt provided 
>> from
>> devicetree, with no need to handle a case where the irq was not 
>> provided.
> 
> Device Tree
> IRQ
> 
> ...
> 
>> +               .scan_type = {
>> +                       .sign = 'u',
>> +                       .realbits = 12,
> 
>> +                       .storagebits = 16
> 
> It's slightly better to leave comma in such cases.
> 
>> +               },
> 
>> +               .scan_type = {
>> +                       .sign = 'u',
>> +                       .realbits = 12,
> 
>> +                       .storagebits = 16
> 
> Ditto.
> 
>> +               },
> 
> ...
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_AUX,
>> +               .scan_index = -1
> 
> Ditto. You see above? Isn't it nice that you didn't touch that line?
> So, perhaps next developer can leverage this subtle kind of things.
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_BATTERY,
>> +               .scan_index = -1
> 
> Ditto.
> 
>>                 .indexed = 1,
>>                 .channel = INGENIC_ADC_AUX2,
>> +               .scan_index = -1
> 
> Ditto.
> 
> ...
> 
>> +static int ingenic_adc_buffer_enable(struct iio_dev *iio_dev)
>> +{
>> +       struct ingenic_adc *adc = iio_priv(iio_dev);
>> +
> 
>> +       clk_enable(adc->clk);
> 
> Error check?
> 
>> +       /* 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);
> 
> Why casting?
After flipping the bits, the resulting value can't be represented by u8. 
Since we care only about the first 8 bits anyway, explicit cast here is 
to silence a compiler warning.
> 
>> +       writel(0, adc->base + JZ_ADC_REG_ADTCH);
>> +       ingenic_adc_enable(adc, 2, true);
>> +
>> +       return 0;
>> +}
> 
>> +       irq = platform_get_irq(pdev, 0);
> 
> Before it worked w/o IRQ, here is a regression you introduced.
> 
>> +       if (irq < 0) {
> 
>> +               dev_err(dev, "Failed to get irq: %d\n", irq);
> 
> Redundant message.
> 
>> +               return irq;
>> +       }

- Artur

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-17 21:52               ` Andy Shevchenko
  2020-04-17 21:56                 ` Paul Cercueil
@ 2020-04-19 12:54                 ` Ezequiel Garcia
  2020-04-19 13:23                   ` Paul Cercueil
  2020-04-19 13:31                   ` Artur Rojek
  1 sibling, 2 replies; 28+ messages in thread
From: Ezequiel Garcia @ 2020-04-19 12:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Paul Cercueil, Artur Rojek, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Jonathan Cameron, Heiko Stuebner, linux-input,
	devicetree, linux-iio, Linux Kernel Mailing List

On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@crapouillou.net> wrote:
> > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
> > <andy.shevchenko@gmail.com> a écrit :
> > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net>
> > > wrote:
> > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
> > >>  <andy.shevchenko@gmail.com> a écrit :
> > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
> > >> <paul@crapouillou.net>
> > >>  > wrote:
> > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
> > >>  >>  <andy.shevchenko@gmail.com> a écrit :
> > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
> > >>  >> <contact@artur-rojek.eu>
> > >>  >>  > wrote:
> > >>  >
> > >>  > ...
> > >>  >
> > >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
> > >>  >>  >
> > >>  >>  > Before it worked w/o IRQ, here is a regression you introduced.
> > >>  >>
> > >>  >>  Before it simply did not need the IRQ, which is provided by the
> > >>  >>  devicetree anyway. No regression here.
> > >>  >
> > >>  > Does it work without IRQ? Or it was a dead code till now?
> > >>  > For me it's clear regression. Otherwise something is really wrong
> > >> in a
> > >>  > process of development of this driver.
> > >>
> > >>  Nothing wrong here. The IRQ was not used by the driver for the
> > >>  functionality it provided before. It is required now to support the
> > >>  touchscreen channels.
> > >
> > > This is exactly what's wrong.
> > > Previous DTS for my (hypothetical) case has no IRQ defined. Everything
> > > works, right?
> > > Now, due to this change it breaks my setup. Don't you see the problem?
> >
> > The IRQ has been provided by every concerned DTS file since the
> > introduction of this driver and the related bindings, even though it
> > was not used by the driver.
>
> Can you speak for all possible DTSs/DTBs in the wild?
> Okay, in any case it will be problem of maintainers and yours if
> somebody complains.
> I'm not going to push this anyway -- your choice.
>
> But I see a (potential) regression.
>

So, there are a few things to keep in mind here.

Let's abstract ourselves from this specific driver
for a minute.

First, and just as Andy pointed out, we can never be fully
sure about DTBs out there. These could be out of tree,
so out of our control. By introducing a new requirement
we break them, which may be seen as a regression.

Second, the interrupt is not required as per
current mainline bindings/iio/adc/ingenic,adc.txt,
so it is perfectly legal for users to not have an interrupt
specified.

Now, back to this case, I think we can get away with this
change, provided this hardware is not that widespread
among developers/users that follow upstream closely.

I suspect anyone developing a serious platform
with this SoC is most likely using some vendor kernel.

If that is not the case, i.e. if you have users _actually_
using this upstream driver, then we should consider
making the interrupt optional instead of required.

Or we can also just break it and hope nobody
complaints.

BTW, this series looks great and I'm happy
to see JZ47xx activity :-)

Arthur: perhaps you can consider converting the txt dt binding
to yaml?

Cheers,
Ezequiel

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-19 12:54                 ` Ezequiel Garcia
@ 2020-04-19 13:23                   ` Paul Cercueil
  2020-04-19 13:31                   ` Artur Rojek
  1 sibling, 0 replies; 28+ messages in thread
From: Paul Cercueil @ 2020-04-19 13:23 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Andy Shevchenko, Artur Rojek, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Jonathan Cameron, Heiko Stuebner, linux-input,
	devicetree, linux-iio, Linux Kernel Mailing List

Hi Ezequiel,


Le dim. 19 avril 2020 à 9:54, Ezequiel Garcia 
<ezequiel@vanguardiasur.com.ar> a écrit :
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko 
> <andy.shevchenko@gmail.com> wrote:
>> 
>>  On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil 
>> <paul@crapouillou.net> wrote:
>>  > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>>  > <andy.shevchenko@gmail.com> a écrit :
>>  > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil 
>> <paul@crapouillou.net>
>>  > > wrote:
>>  > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>>  > >>  <andy.shevchenko@gmail.com> a écrit :
>>  > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>>  > >> <paul@crapouillou.net>
>>  > >>  > wrote:
>>  > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>>  > >>  >>  <andy.shevchenko@gmail.com> a écrit :
>>  > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>>  > >>  >> <contact@artur-rojek.eu>
>>  > >>  >>  > wrote:
>>  > >>  >
>>  > >>  > ...
>>  > >>  >
>>  > >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
>>  > >>  >>  >
>>  > >>  >>  > Before it worked w/o IRQ, here is a regression you 
>> introduced.
>>  > >>  >>
>>  > >>  >>  Before it simply did not need the IRQ, which is provided 
>> by the
>>  > >>  >>  devicetree anyway. No regression here.
>>  > >>  >
>>  > >>  > Does it work without IRQ? Or it was a dead code till now?
>>  > >>  > For me it's clear regression. Otherwise something is really 
>> wrong
>>  > >> in a
>>  > >>  > process of development of this driver.
>>  > >>
>>  > >>  Nothing wrong here. The IRQ was not used by the driver for the
>>  > >>  functionality it provided before. It is required now to 
>> support the
>>  > >>  touchscreen channels.
>>  > >
>>  > > This is exactly what's wrong.
>>  > > Previous DTS for my (hypothetical) case has no IRQ defined. 
>> Everything
>>  > > works, right?
>>  > > Now, due to this change it breaks my setup. Don't you see the 
>> problem?
>>  >
>>  > The IRQ has been provided by every concerned DTS file since the
>>  > introduction of this driver and the related bindings, even though 
>> it
>>  > was not used by the driver.
>> 
>>  Can you speak for all possible DTSs/DTBs in the wild?
>>  Okay, in any case it will be problem of maintainers and yours if
>>  somebody complains.
>>  I'm not going to push this anyway -- your choice.
>> 
>>  But I see a (potential) regression.
>> 
> 
> So, there are a few things to keep in mind here.
> 
> Let's abstract ourselves from this specific driver
> for a minute.
> 
> First, and just as Andy pointed out, we can never be fully
> sure about DTBs out there. These could be out of tree,
> so out of our control. By introducing a new requirement
> we break them, which may be seen as a regression.
> 
> Second, the interrupt is not required as per
> current mainline bindings/iio/adc/ingenic,adc.txt,
> so it is perfectly legal for users to not have an interrupt
> specified.
> 
> Now, back to this case, I think we can get away with this
> change, provided this hardware is not that widespread
> among developers/users that follow upstream closely.
> 
> I suspect anyone developing a serious platform
> with this SoC is most likely using some vendor kernel.
> 
> If that is not the case, i.e. if you have users _actually_
> using this upstream driver, then we should consider
> making the interrupt optional instead of required.
> 
> Or we can also just break it and hope nobody
> complaints.

The vast majority of Ingenic devices running Linux use a 3.x kernel 
with a lot of patches on top. These kernels don't support devicetree. 
So there is no problem with legacy devicetree files: there are no 
legacy devicetree files.

Of the few Ingenic devices running mainline kernels, all of them with 
an ADC node in the devicetree have the 'interrupts' property specified, 
out-of-tree or in-tree.

As the informal Ingenic SoCs maintainer I'm pretty aware of these 
things, and I can assure that we're not breaking anything. The only 
thing broken is the documentation which doesn't specify that the 
'interrupts' property is required.

> BTW, this series looks great and I'm happy
> to see JZ47xx activity :-)
> 
> Arthur: perhaps you can consider converting the txt dt binding
> to yaml?

That would be great.

-Paul



^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode.
  2020-04-19 12:54                 ` Ezequiel Garcia
  2020-04-19 13:23                   ` Paul Cercueil
@ 2020-04-19 13:31                   ` Artur Rojek
  1 sibling, 0 replies; 28+ messages in thread
From: Artur Rojek @ 2020-04-19 13:31 UTC (permalink / raw)
  To: Ezequiel Garcia
  Cc: Andy Shevchenko, Paul Cercueil, Dmitry Torokhov, Rob Herring,
	Mark Rutland, Jonathan Cameron, Heiko Stuebner, linux-input,
	devicetree, linux-iio, Linux Kernel Mailing List

Hi Ezequiel,

On 2020-04-19 14:54, Ezequiel Garcia wrote:
> On Fri, 17 Apr 2020 at 18:54, Andy Shevchenko 
> <andy.shevchenko@gmail.com> wrote:
>> 
>> On Sat, Apr 18, 2020 at 12:45 AM Paul Cercueil <paul@crapouillou.net> 
>> wrote:
>> > Le sam. 18 avril 2020 à 0:42, Andy Shevchenko
>> > <andy.shevchenko@gmail.com> a écrit :
>> > > On Sat, Apr 18, 2020 at 12:18 AM Paul Cercueil <paul@crapouillou.net>
>> > > wrote:
>> > >>  Le sam. 18 avril 2020 à 0:13, Andy Shevchenko
>> > >>  <andy.shevchenko@gmail.com> a écrit :
>> > >>  > On Sat, Apr 18, 2020 at 12:05 AM Paul Cercueil
>> > >> <paul@crapouillou.net>
>> > >>  > wrote:
>> > >>  >>  Le ven. 17 avril 2020 à 23:59, Andy Shevchenko
>> > >>  >>  <andy.shevchenko@gmail.com> a écrit :
>> > >>  >>  > On Fri, Apr 17, 2020 at 11:21 PM Artur Rojek
>> > >>  >> <contact@artur-rojek.eu>
>> > >>  >>  > wrote:
>> > >>  >
>> > >>  > ...
>> > >>  >
>> > >>  >>  >>  +       irq = platform_get_irq(pdev, 0);
>> > >>  >>  >
>> > >>  >>  > Before it worked w/o IRQ, here is a regression you introduced.
>> > >>  >>
>> > >>  >>  Before it simply did not need the IRQ, which is provided by the
>> > >>  >>  devicetree anyway. No regression here.
>> > >>  >
>> > >>  > Does it work without IRQ? Or it was a dead code till now?
>> > >>  > For me it's clear regression. Otherwise something is really wrong
>> > >> in a
>> > >>  > process of development of this driver.
>> > >>
>> > >>  Nothing wrong here. The IRQ was not used by the driver for the
>> > >>  functionality it provided before. It is required now to support the
>> > >>  touchscreen channels.
>> > >
>> > > This is exactly what's wrong.
>> > > Previous DTS for my (hypothetical) case has no IRQ defined. Everything
>> > > works, right?
>> > > Now, due to this change it breaks my setup. Don't you see the problem?
>> >
>> > The IRQ has been provided by every concerned DTS file since the
>> > introduction of this driver and the related bindings, even though it
>> > was not used by the driver.
>> 
>> Can you speak for all possible DTSs/DTBs in the wild?
>> Okay, in any case it will be problem of maintainers and yours if
>> somebody complains.
>> I'm not going to push this anyway -- your choice.
>> 
>> But I see a (potential) regression.
>> 
> 
> So, there are a few things to keep in mind here.
> 
> Let's abstract ourselves from this specific driver
> for a minute.
> 
> First, and just as Andy pointed out, we can never be fully
> sure about DTBs out there. These could be out of tree,
> so out of our control. By introducing a new requirement
> we break them, which may be seen as a regression.
> 
> Second, the interrupt is not required as per
> current mainline bindings/iio/adc/ingenic,adc.txt,
> so it is perfectly legal for users to not have an interrupt
> specified.
> 
> Now, back to this case, I think we can get away with this
> change, provided this hardware is not that widespread
> among developers/users that follow upstream closely.
> 
> I suspect anyone developing a serious platform
> with this SoC is most likely using some vendor kernel.
> 
> If that is not the case, i.e. if you have users _actually_
> using this upstream driver, then we should consider
> making the interrupt optional instead of required.
> 
> Or we can also just break it and hope nobody
> complaints.
> 
> BTW, this series looks great and I'm happy
> to see JZ47xx activity :-)
> 
> Arthur: perhaps you can consider converting the txt dt binding
> to yaml?
Sure, it will come with v6 of this patchset.
And this time I'll make the `interrupts` property required :-)

- Artur
> 
> Cheers,
> Ezequiel

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-04-19 13:32 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 20:28 [RESEND PATCH v5 1/5] IIO: Ingenic JZ47xx: Add xlate cb to retrieve correct channel idx Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 2/5] dt-bindings: iio/adc: Add touchscreen idx for JZ47xx SoC ADC Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 3/5] IIO: Ingenic JZ47xx: Add touchscreen mode Artur Rojek
2020-04-17 20:59   ` Andy Shevchenko
2020-04-17 21:04     ` Paul Cercueil
2020-04-17 21:13       ` Andy Shevchenko
2020-04-17 21:18         ` Paul Cercueil
2020-04-17 21:42           ` Andy Shevchenko
2020-04-17 21:45             ` Paul Cercueil
2020-04-17 21:52               ` Andy Shevchenko
2020-04-17 21:56                 ` Paul Cercueil
2020-04-19 12:54                 ` Ezequiel Garcia
2020-04-19 13:23                   ` Paul Cercueil
2020-04-19 13:31                   ` Artur Rojek
2020-04-19 12:19     ` Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 4/5] dt-bindings: input: Add docs for ADC driven joystick Artur Rojek
2020-04-17 20:28 ` [RESEND PATCH v5 5/5] input: joystick: Add ADC attached joystick driver Artur Rojek
2020-04-17 21:10   ` Andy Shevchenko
2020-04-17 21:23     ` Paul Cercueil
2020-04-17 21:49       ` Andy Shevchenko
2020-04-17 22:48         ` Paul Cercueil
2020-04-18 11:57           ` Andy Shevchenko
2020-04-18 12:10             ` Paul Cercueil
2020-04-18 12:42               ` Andy Shevchenko
2020-04-18 13:24                 ` Paul Cercueil
2020-04-18 14:22                   ` Jonathan Cameron
2020-04-18 17:25                     ` Paul Cercueil
2020-04-18 18:20                       ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).