[PATCH 00/40] TI AM437X ADC1

On Wed, Aug 25, 2021 at 05:24:41PM +0200, Miquel Raynal wrote:

I prefer that MFDs have just 1 complete example in the MFD schema. 

+
+    tscadc: tscadc@0 {

Drop unused labels.

status, again.

+        dmas = <&edma 53 0>, <&edma 57 0>;
+        dma-names = "fifo0", "fifo1";
+
+        tsc {
+            compatible = "ti,am3359-tsc";
+            ti,wires = <4>;
+            ti,x-plate-resistance = <200>;
+            ti,coordinate-readouts = <5>;
+            ti,wire-config = <0x00 0x11 0x22 0x33>;
+            ti,charge-delay = <0x400>;
+        };
+    };
-- 
2.27.0

On Wed, 25 Aug 2021 17:24:43 +0200, Miquel Raynal wrote:

Now that the three hardware peaces described in this file have been
translated in yaml in 3 dedicated files (MFD, touchscreen controller and
ADC) it is time to remove this text file.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt | 91 -------------------
 1 file changed, 91 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt

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

Hi Rob,

quoted

+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>  

I prefer that MFDs have just 1 complete example in the MFD schema. 

Just to be on the same line, you ask me to drop the all the lines from

	tscadc:

up to

	dma-names = ...;

and keep only the tsc node, right?

I guess I should do the same in the ADC binding then.

quoted

+
+    tscadc: tscadc@0 {  

Drop unused labels.

quoted

+        compatible = "ti,am3359-tscadc";
+        reg = <0x0 0x1000>;
+        interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&adc_tsc_fck>;
+        clock-names = "fck";
+        status = "disabled";  

status, again.

quoted

+        dmas = <&edma 53 0>, <&edma 57 0>;
+        dma-names = "fifo0", "fifo1";
+
+        tsc {
+            compatible = "ti,am3359-tsc";
+            ti,wires = <4>;
+            ti,x-plate-resistance = <200>;
+            ti,coordinate-readouts = <5>;
+            ti,wire-config = <0x00 0x11 0x22 0x33>;
+            ti,charge-delay = <0x400>;
+        };
+    };
-- 
2.27.0

  

Thanks,
Miquèl

On 25/08/2021 18:25, Miquel Raynal wrote:

"ti,tracks" seems undocumented?

[....]

-- 
Best regards,
grygorii

Hi Grygorii,

Grygorii Strashko [off-list ref] wrote on Wed, 1 Sep 2021
22:26:25 +0300:

On 25/08/2021 18:25, Miquel Raynal wrote:

quoted

Introduce a new compatible that has another set of driver data,
targeting am437x SoCs with a magnetic reader instead of the
touchscreen and a more featureful set of registers.

Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
---
  drivers/mfd/ti_am335x_tscadc.c       | 43 ++++++++++++++++++++++------
  include/linux/mfd/ti_am335x_tscadc.h |  9 +++++-
  2 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 1a30610dc65f..f4f6b9db4d2a 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c

@@ -122,9 +122,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
  	const __be32 *cur;
  	struct clk *clk;
  	u32 val;
-	bool use_tsc = false;
+	bool use_tsc = false, use_mag = false;
  	int tscmag_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
-	int total_channels, err;
+	int mag_tracks = 0, total_channels, err;  
  >   	/* Allocate memory for device */  
  	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);

@@ -146,6 +146,12 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
  		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
  		if (tscmag_wires)
  			use_tsc = true;
+	} else {
+		node = of_get_child_by_name(pdev->dev.of_node, "mag");
+		of_property_read_u32(node, "ti,tracks", &mag_tracks);  

"ti,tracks" seems undocumented?

Well that's true and almost on purpose, I am not focusing on the
magnetic reader feature, it is not supported, I don't have one, I don't
plan to add support for it. But in the driver I need to know how many
"tracks" are unavailable for the ADC in order to implement the entire
logic (this block comes from TI and the naming from Jason Reeder).

I am not comfortable writing a binding file for a device that I won't
use, it's the best way to miss something and have stable broken
bindings in the future. So I assumed it was not a big deal to have this
property in the code, which may be updated/removed/enhanced later if
needed without having to mess with the code too much. What do you think?

Thanks,
Miquèl

On 02/09/2021 09:47, Miquel Raynal wrote:

Hi Grygorii,

Grygorii Strashko [off-list ref] wrote on Wed, 1 Sep 2021
22:26:25 +0300:

quoted

On 25/08/2021 18:25, Miquel Raynal wrote:

quoted

Introduce a new compatible that has another set of driver data,
targeting am437x SoCs with a magnetic reader instead of the
touchscreen and a more featureful set of registers.

Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
---
   drivers/mfd/ti_am335x_tscadc.c       | 43 ++++++++++++++++++++++------
   include/linux/mfd/ti_am335x_tscadc.h |  9 +++++-
   2 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 1a30610dc65f..f4f6b9db4d2a 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c

@@ -122,9 +122,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
   	const __be32 *cur;
   	struct clk *clk;
   	u32 val;
-	bool use_tsc = false;
+	bool use_tsc = false, use_mag = false;
   	int tscmag_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
-	int total_channels, err;
+	int mag_tracks = 0, total_channels, err;
   >   	/* Allocate memory for device */
   	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);

@@ -146,6 +146,12 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
   		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
   		if (tscmag_wires)
   			use_tsc = true;
+	} else {
+		node = of_get_child_by_name(pdev->dev.of_node, "mag");
+		of_property_read_u32(node, "ti,tracks", &mag_tracks);

"ti,tracks" seems undocumented?

Well that's true and almost on purpose, I am not focusing on the
magnetic reader feature, it is not supported, I don't have one, I don't
plan to add support for it. But in the driver I need to know how many
"tracks" are unavailable for the ADC in order to implement the entire
logic (this block comes from TI and the naming from Jason Reeder).

I am not comfortable writing a binding file for a device that I won't
use, it's the best way to miss something and have stable broken
bindings in the future. So I assumed it was not a big deal to have this
property in the code, which may be updated/removed/enhanced later if
needed without having to mess with the code too much. What do you think?

Sry, it's not ok.
You need to (a) add binding or (b) w/a it in some way -
like drop it and use const value instead, for example.

-- 
Best regards,
grygorii

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 13:47:14
+0100:

On Wed, 25 Aug 2021 17:24:40 +0200
Miquel Raynal [off-list ref] wrote:

quoted

There is a very light description of this MFD in a text file dedicated
to a touchscreen controller (which is one of the two children of the
MFD). Here is now a complete yaml description.  

Make sure to call out places where you are filling in gaps in the
txt file description.  Looks like we forgot to keep that up to date as the driver
evolved. oops.

Right, I always forget these links, will update them.

quoted

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/mfd/ti,am3359-tscadc.yaml        | 75 +++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml

diff --git a/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml b/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
new file mode 100644
index 000000000000..96b329508d8a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml

@@ -0,0 +1,75 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/ti,am3359-tscadc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI AM3359 Touchscreen controller/ADC
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+properties:
+  compatible:
+    const: ti,am3359-tscadc  

text documents the more specific ti,am654-tscadc as well.

Oops, I saw it and then forgot. Thanks for checking!

Something like

   properties:
     compatible:
       oneof:
         - const: ti,am3359-tscadc
         - items:
             - const: ti,am654-tscadc
             - const: ti,am3359-tscadc

Note this superceeds my comment on later patches about changing to enum when
introducing the new compatible. Ah well.

No problem (and thanks for the 50+ reviews btw ;) )

Thanks,
Miquèl

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 14:36:10
+0100:

On Wed, 25 Aug 2021 17:24:41 +0200
Miquel Raynal [off-list ref] wrote:

quoted

This touchscreen controller is already described in a text file:
Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt

After introducing a proper description of the MFD, this is the second
step. The file cannot be removed yet as it also contains an ADC
description.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../input/touchscreen/ti,am3359-tsc.yaml      | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml b/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
new file mode 100644
index 000000000000..2d4ce6d04f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml

@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/ti,am3359-tsc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI AM3359 Touchscreen controller
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+properties:
+  compatible:
+    const: ti,am3359-tsc
+
+  ti,wires:
+    description: Wires refer to application modes i.e. 4/5/8 wire touchscreen
+      support on the platform.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [4, 5, 8]
+
+  ti,x-plate-resistance:
+    description: X plate resistance
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  ti,coordinate-readouts:
+    description: The sequencer supports a total of 16 programmable steps. Each
+      step is used to read a single coordinate. A single readout is enough but
+      multiple reads can increase the quality. A value of 5 means, 5 reads for
+      X, 5 for Y and 2 for Z (always). This utilises 12 of the 16 software steps
+      available. The remaining 4 can be used by the ADC.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 6
+
+  ti,wire-config:
+    description: Different boards could have a different order for connecting
+      wires on touchscreen. We need to provide an 8-bit number where the
+      first four bits represent the analog lines and the next 4 bits represent
+      positive/negative terminal on that input line. Notations to represent the
+      input lines and terminals respectively are as follows, AIN0 = 0, AIN1 = 1
+      and so on untill AIN7 = 7. XP = 0, XN = 1, YP = 2, YN = 3.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 4
+    maxItems: 8
+
+  ti,charge-delay:
+    description: Length of touch screen charge delay step in terms of ADC clock
+      cycles. Charge delay value should be large in order to avoid false pen-up
+      events. This value effects the overall sampling speed, hence need to be
+      kept as low as possible, while avoiding false pen-up event. Start from a
+      lower value, say 0x400, and increase value until false pen-up events are
+      avoided. The pen-up detection happens immediately after the charge step,
+      so this does in fact function as a hardware knob for adjusting the amount
+      of "settling time".
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - ti,wires  

Are these all required?  Seems like the driver will work fine without some
of them and isn't doing appropriate error checking for their presence..

Well, I don't have a touchscreen so I can't actually verify that it
works without these properties. The logic here is to simply translate
the .txt file and the .txt file states that these properties are
mandatory. But of course if people have one and can test, it would be
good to either add the proper checks to the driver or drop these
properties from the required list. But as far as I am concerned, it is
too risky to do this kind of blind change in a binding file...

Thanks,
Miquèl

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 15:10:10
+0100:

On Wed, 25 Aug 2021 17:25:06 +0200
Miquel Raynal [off-list ref] wrote:

quoted

Introduce a new compatible that has another set of driver data,
targeting am437x SoCs with a magnetic reader instead of the
touchscreen and a more featureful set of registers.

Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>  

There is one bit in here that I think should have been back in patch 16 but
maybe I'm missing something.

Good catch, indeed I should have wired the compatible with the driver
data since patch 16. I will fix this.

Thanks,
Miquèl

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 14:56:08
+0100:

On Wed, 25 Aug 2021 17:24:58 +0200
Miquel Raynal [off-list ref] wrote:

quoted

Instead of deriving in the probe and in the resume path the value of the
ctrl register, let's do it only once in the probe, save the value of
this register in the driver's structure and use it from the resume
callback.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  

A few minor things inline.

J

quoted

---
 drivers/mfd/ti_am335x_tscadc.c       | 31 ++++++++--------------------
 include/linux/mfd/ti_am335x_tscadc.h |  2 +-
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7071344ad18e..d661e8ae66c9 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c

@@ -122,7 +122,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct clk *clk;
 	u32 val;
 	int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
-	int total_channels, ctrl, err;
+	int total_channels, err;
 
 	/* Allocate memory for device */
 	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);

@@ -215,22 +215,21 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
 	/* Set the control register bits */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID;
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
 	if (tsc_wires > 0) {
-		tscadc->tsc_wires = tsc_wires;
+		tscadc->ctrl |= CNTRLREG_TSCENB;
 		if (tsc_wires == 5)
-			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
+			tscadc->ctrl |= CNTRLREG_5WIRE;
 		else
-			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
+			tscadc->ctrl |= CNTRLREG_4WIRE;
 	}
 
 	tscadc_idle_config(tscadc);
 
 	/* Enable the TSC module enable bit */
-	ctrl |= CNTRLREG_TSCSSENB;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
 
 	/* TSC Cell */
 	if (tsc_wires > 0) {

@@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev)
 static int __maybe_unused tscadc_resume(struct device *dev)
 {
 	struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev);
-	u32 ctrl;
 
 	pm_runtime_get_sync(dev);
 
-	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
-	if (tscadc->tsc_wires > 0) {
-		if (tscadc->tsc_wires == 5)
-			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
-		else
-			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
-	}
-	ctrl |= CNTRLREG_TSCSSENB;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);  

Patch description should mention why this ordering change is here.

I actually moved the patch that reorders things earlier so that the
reviewer is not bothered by the order changes later on.

quoted

 	tscadc_idle_config(tscadc);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);  

As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious.

You might be better off keeping them in sync, but masking that bit out and then resetting
it as appropriate.

I honestly find more readable doing:

ctrl = flags;
writel(ctrl);
writel(ctrl | en_bit);

than

ctrl = flags;
writel(ctrl & ~en_bit);
writel(ctrl);

because the second version emphasis the fact that we reset the en_bit
(which is wrong, the point of this first write is to actually write all
the configuration but not the en_bit yet) while the first version
clearly shows that the second write includes an additional "enable bit".

Thanks,
Miquèl

Hi Jonathan,

quoted

+static const struct ti_tscadc_data magdata = {
+	.has_tsc = false,
+	.has_mag = true,
+	.name_tscmag = "TI-am43xx-mag",
+	.compat_tscmag = "ti,am4372-mag",
+	.name_adc = "TI-am43xx-adc",
+	.compat_adc = "ti,am4372-adc",
+	.target_clk_rate = MAG_ADC_CLK,
+};
+
 static const struct of_device_id ti_tscadc_dt_ids[] = {
-	{ .compatible = "ti,am3359-tscadc", },
+	{
+		.compatible = "ti,am3359-tscadc",
+		.data = &tscdata,  

Interesting to see match data added here and not before.
Given you don't have any code in here that seems to have
changed to use the match data, was it buggy before or is this still
not used?

As said earlier, it was buggy before. It is now fixed.

quoted

+	},
+	{
+		.compatible = "ti,am4372-magadc",
+		.data = &magdata,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);

@@ -355,6 +382,6 @@ static struct platform_driver ti_tscadc_driver = {
 
 module_platform_driver(ti_tscadc_driver);
 
-MODULE_DESCRIPTION("TI touchscreen / ADC MFD controller driver");
+MODULE_DESCRIPTION("TI touchscreen/magnetic reader/ADC MFD controller driver");
 MODULE_AUTHOR("Rachna Patil <rachna@ti.com>");
 MODULE_LICENSE("GPL");

diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 082b2af94263..31b22ec567e7 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h

@@ -129,6 +129,11 @@
 #define CNTRLREG_TSC_8WIRE	CNTRLREG_TSC_AFE_CTRL(3)
 #define CNTRLREG_TSC_ENB	BIT(7)
 
+/*Control registers bitfields  for MAGADC IP */
+#define CNTRLREG_MAGADCENB      BIT(0)
+#define CNTRLREG_MAG_PREAMP_PWRDOWN BIT(5)
+#define CNTRLREG_MAG_PREAMP_BYPASS  BIT(6)
+
 /* FIFO READ Register */
 #define FIFOREAD_DATA_MASK (0xfff << 0)
 #define FIFOREAD_CHNLID_MASK (0xf << 16)

@@ -141,7 +146,8 @@
 #define SEQ_STATUS BIT(5)
 #define CHARGE_STEP		0x11
 
-#define TSC_ADC_CLK		3000000
+#define TSC_ADC_CLK		3000000 /* 3 MHz */
+#define MAG_ADC_CLK		13000000 /* 13 MHz */  

Not sure on current status, but there is a proposed series floating
about that adds HZ_PER_MEGAHZ or something like that which would make
it easier to spot if these have right number of zeros.

Would be nice indeed, but it looks like it's not yet mainline :/

Thanks,
Miquèl

On Thu, 2 Sep 2021 21:42:47 +0200
Miquel Raynal [off-list ref] wrote:

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 14:56:08
+0100:

quoted

On Wed, 25 Aug 2021 17:24:58 +0200
Miquel Raynal [off-list ref] wrote:
  

quoted

Instead of deriving in the probe and in the resume path the value of the
ctrl register, let's do it only once in the probe, save the value of
this register in the driver's structure and use it from the resume
callback.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>    

A few minor things inline.

J
  

quoted

---
 drivers/mfd/ti_am335x_tscadc.c       | 31 ++++++++--------------------
 include/linux/mfd/ti_am335x_tscadc.h |  2 +-
 2 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7071344ad18e..d661e8ae66c9 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c

@@ -122,7 +122,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct clk *clk;
 	u32 val;
 	int tsc_wires = 0, adc_channels = 0, readouts = 0, cell_idx = 0;
-	int total_channels, ctrl, err;
+	int total_channels, err;
 
 	/* Allocate memory for device */
 	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);

@@ -215,22 +215,21 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
 	/* Set the control register bits */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID;
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
 	if (tsc_wires > 0) {
-		tscadc->tsc_wires = tsc_wires;
+		tscadc->ctrl |= CNTRLREG_TSCENB;
 		if (tsc_wires == 5)
-			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
+			tscadc->ctrl |= CNTRLREG_5WIRE;
 		else
-			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
+			tscadc->ctrl |= CNTRLREG_4WIRE;
 	}
 
 	tscadc_idle_config(tscadc);
 
 	/* Enable the TSC module enable bit */
-	ctrl |= CNTRLREG_TSCSSENB;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
 
 	/* TSC Cell */
 	if (tsc_wires > 0) {

@@ -305,25 +304,13 @@ static int __maybe_unused tscadc_suspend(struct device *dev)
 static int __maybe_unused tscadc_resume(struct device *dev)
 {
 	struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev);
-	u32 ctrl;
 
 	pm_runtime_get_sync(dev);
 
-	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
-	if (tscadc->tsc_wires > 0) {
-		if (tscadc->tsc_wires == 5)
-			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
-		else
-			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
-	}
-	ctrl |= CNTRLREG_TSCSSENB;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);    

Patch description should mention why this ordering change is here.  

I actually moved the patch that reorders things earlier so that the
reviewer is not bothered by the order changes later on.

quoted

quoted

 	tscadc_idle_config(tscadc);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);    

As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious.

You might be better off keeping them in sync, but masking that bit out and then resetting
it as appropriate.  

I honestly find more readable doing:

ctrl = flags;
writel(ctrl);
writel(ctrl | en_bit);

than

ctrl = flags;
writel(ctrl & ~en_bit);
writel(ctrl);

because the second version emphasis the fact that we reset the en_bit
(which is wrong, the point of this first write is to actually write all
the configuration but not the en_bit yet) while the first version
clearly shows that the second write includes an additional "enable bit".

Fair enough.  Perhaps it's worth throwing in a comment though if you happen
to be respining to say tcsadc->ctrl isn't actually the contents of
the register, but rather of 'most' of it.

Jonathan

Thanks,
Miquèl

On Thu, 2 Sep 2021 19:45:36 +0200
Miquel Raynal [off-list ref] wrote:

Hi Jonathan,

Jonathan Cameron [off-list ref] wrote on Mon, 30 Aug 2021 14:36:10
+0100:

quoted

On Wed, 25 Aug 2021 17:24:41 +0200
Miquel Raynal [off-list ref] wrote:
  

quoted

This touchscreen controller is already described in a text file:
Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt

After introducing a proper description of the MFD, this is the second
step. The file cannot be removed yet as it also contains an ADC
description.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../input/touchscreen/ti,am3359-tsc.yaml      | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml b/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
new file mode 100644
index 000000000000..2d4ce6d04f53
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml

@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/ti,am3359-tsc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI AM3359 Touchscreen controller
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+properties:
+  compatible:
+    const: ti,am3359-tsc
+
+  ti,wires:
+    description: Wires refer to application modes i.e. 4/5/8 wire touchscreen
+      support on the platform.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    enum: [4, 5, 8]
+
+  ti,x-plate-resistance:
+    description: X plate resistance
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  ti,coordinate-readouts:
+    description: The sequencer supports a total of 16 programmable steps. Each
+      step is used to read a single coordinate. A single readout is enough but
+      multiple reads can increase the quality. A value of 5 means, 5 reads for
+      X, 5 for Y and 2 for Z (always). This utilises 12 of the 16 software steps
+      available. The remaining 4 can be used by the ADC.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 6
+
+  ti,wire-config:
+    description: Different boards could have a different order for connecting
+      wires on touchscreen. We need to provide an 8-bit number where the
+      first four bits represent the analog lines and the next 4 bits represent
+      positive/negative terminal on that input line. Notations to represent the
+      input lines and terminals respectively are as follows, AIN0 = 0, AIN1 = 1
+      and so on untill AIN7 = 7. XP = 0, XN = 1, YP = 2, YN = 3.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 4
+    maxItems: 8
+
+  ti,charge-delay:
+    description: Length of touch screen charge delay step in terms of ADC clock
+      cycles. Charge delay value should be large in order to avoid false pen-up
+      events. This value effects the overall sampling speed, hence need to be
+      kept as low as possible, while avoiding false pen-up event. Start from a
+      lower value, say 0x400, and increase value until false pen-up events are
+      avoided. The pen-up detection happens immediately after the charge step,
+      so this does in fact function as a hardware knob for adjusting the amount
+      of "settling time".
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - ti,wires    

Are these all required?  Seems like the driver will work fine without some
of them and isn't doing appropriate error checking for their presence..  

Well, I don't have a touchscreen so I can't actually verify that it
works without these properties. The logic here is to simply translate
the .txt file and the .txt file states that these properties are
mandatory. But of course if people have one and can test, it would be
good to either add the proper checks to the driver or drop these
properties from the required list. But as far as I am concerned, it is
too risky to do this kind of blind change in a binding file...

Fair enough!

Thanks,
Miquèl