[PATCH v3 00/47] TI AM437X ADC1

[PATCH v3 00/47] TI AM437X ADC1

[Miquel Raynal]

Hello,

This is a (fairly big) series bringing support of AM437X ADC1.
On TI AM33XX SoCs family there is an ADC that can also be connected to a
touchscreen. This hardware has been extended and is present on certain
SoCs from the AM437X family. In particular, the touchscreen has been
replaced by a magnetic card reader. In both cases, the representation is
an MFD device with two children:
* on AM33XX: the touchscreen controller and the ADC
* on AM437X: the magnetic card reader and the ADC

This series really targets small and atomic changes so that the overall
review is eased, even though it leads to a lot of rather small patches.
Here are the steps:
* Supporting the missing clock
* Translating a single text file containing the description for the
  MFD, the touchscreen and the ADC into three independent yaml files.
* Cleaning/preparing the MFD driver.
* Supporting ADC1 in the MFD driver.
* Cleaning/preparing of the ADC driver.
* Supporting ADC1 in the ADC driver.
* Updating various device trees.

Here is the full series again, almost reviewed and acked entirely, but
rebased on the last -rc1. The clock patch has been acked, the ADC
patches as well, so we expect the series to go through the MFD
tree if hte maintainers agree with it.

Thanks,
Miquèl

Changes in v3:
* Rebased on top of v5.15-rc1.
* R-by/A-by tags added.
* Light reordering to let the of_put_node() fix to be applied more easily
* Dropped a patch made useless because of the previous reordering
* Explained how the tscadc->ctrl variable was used.
* Fixed a couple of typos.
* Included the change for the HZ macro.
* Went further in the BIT()/FIELD_PREP() cleanup.
* Added maximum definitions for sample delay/open delay.
* Removed useless definitions.
* Fixed a couple of rebase conflicts (the series was not bisectable).

Changes in v2:
* Added various R-by/A-by tags.
* Various typos & style fixes.
[Bindings]
* Included the missing ti,am654-tscadc compatible.
* Reworded the compatible lines as requested by Jonathan.
* Reworded the bindings content a little bit as advised by Rob (subnodes
  being objects, MFD descriptions provided once, status and unused
  labels removed).
[SPDX changes]
* Mentioned that the license macro and the license text matched.
* Also added an SPDX tag in the MFD header.
[MFD header]
* Used the BIT(), GENMASK() and PREP_FIELD() macros when relevant.
[MFD driver]
* Did not reordered the variables declared on the probe stack as advised
  by Jonathan.
* Added missing of_node_put() calls.
* Moved the patch changing the place where the main structure is
  allocated to directly precede the patch using this change.
* Fixed the driver data wiring (bug happening between ex patches 16 and
  28).
* Added a commit just to explain the reordering of the register writes
  during initialization/resume.
* Explained the check about 'use_tsc' in the commit message.
* Added a link to the TRM in a commit message referencing it.
* Removed the use of the ti,tracks property, used a constant value
  instead.
* Dropped the error check when retrieving the "wrong" DT property
  (coordiante-readouts) which is unused.


Miquel Raynal (47):
  clk: ti: am43xx: Add clkctrl data for am43xx ADC1
  dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this
    MFD
  dt-bindings: touchscreen: ti,am3359-tsc: New yaml description
  dt-bindings: iio: adc: ti,am3359-adc: New yaml description
  dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file
  dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible
  dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible
  mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put
  mfd: ti_am335x_tscadc: Replace license text with SPDX tag
  mfd: ti_am335x_tscadc: Fix style
  mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack
    variables
  mfd: ti_am335x_tscadc: Get rid of useless gotos
  mfd: ti_am335x_tscadc: Reword the comment explaining the dividers
  mfd: ti_am335x_tscadc: Don't search the tree for our clock
  mfd: ti_am335x_tscadc: Simplify divisor calculation
  mfd: ti_am335x_tscadc: Move the driver structure allocation earlier
  mfd: ti_am335x_tscadc: Use driver data
  mfd: ti_am335x_tscadc: Mimic the probe from resume()
  mfd: ti_am335x_tscadc: Drop useless variables from the driver
    structure
  mfd: ti_am335x_tscadc: Always provide an idle configuration
  mfd: ti_am335x_tscadc: Reorder the initialization steps
  mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place
  mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag
  mfd: ti_am335x_tscadc: Fix header spacing
  mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro
  mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when
    relevant
  mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries
  mfd: ti_am335x_tscadc: Drop useless definitions from the header
  mfd: ti_am335x_tscadc: Rename the subsystem enable macro
  mfd: ti_am335x_tscadc: Add TSC prefix in certain macros
  mfd: ti_am335x_tscadc: Rename a variable
  mfd: ti_am335x_tscadc: Fix an error message
  mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a
    touchscreen
  mfd: ti_am335x_tscadc: Introduce has_tsc
  mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support
  mfd: ti_am335x_tscadc: Support the correctly spelled DT property
  iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls
  iio: adc: ti_am335x_adc: Replace license text with SPDX tag
  iio: adc: ti_am335x_adc: Fix style
  iio: adc: ti_am335x_adc: Get rid of useless gotos
  iio: adc: ti_am335x_adc: Gather the checks on the delays
  iio: adc: ti_am335x_adc: Add a unit to the timeout delay
  iio: adc: ti_am335x_adc: Add the scale information
  iio: adc: ti_am335x_adc: Add the am437x compatible
  ARM: dts: am437x-cm-t43: Use a correctly spelled DT property
  ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module
  ARM: dts: am437x-gp-evm: enable ADC1

 .../bindings/iio/adc/ti,am3359-adc.yaml       |  70 +++++
 .../input/touchscreen/ti,am3359-tsc.yaml      |  76 ++++++
 .../bindings/input/touchscreen/ti-tsc-adc.txt |  91 -------
 .../bindings/mfd/ti,am3359-tscadc.yaml        |  84 ++++++
 arch/arm/boot/dts/am437x-cm-t43.dts           |   2 +-
 arch/arm/boot/dts/am437x-gp-evm.dts           |   8 +
 arch/arm/boot/dts/am437x-l4.dtsi              |  31 ++-
 arch/arm/boot/dts/am43xx-clocks.dtsi          |   7 +
 drivers/clk/ti/clk-43xx.c                     |   1 +
 drivers/iio/adc/ti_am335x_adc.c               | 220 +++++++++-------
 drivers/mfd/ti_am335x_tscadc.c                | 243 ++++++++++--------
 include/dt-bindings/clock/am4.h               |   1 +
 include/linux/mfd/ti_am335x_tscadc.h          | 115 ++++-----
 13 files changed, 599 insertions(+), 350 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
 create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
 delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml

-- 
2.27.0

[PATCH v3 01/47] clk: ti: am43xx: Add clkctrl data for am43xx ADC1

[Miquel Raynal]

Declare ADC1 clkctrl which feeds the magnetic-reader/ADC1 hardware
module.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/ti/clk-43xx.c       | 1 +
 include/dt-bindings/clock/am4.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/clk/ti/clk-43xx.c b/drivers/clk/ti/clk-43xx.c
index 46c0add99570..6e97a541cfd3 100644
--- a/drivers/clk/ti/clk-43xx.c
+++ b/drivers/clk/ti/clk-43xx.c
@@ -116,6 +116,7 @@ static const struct omap_clkctrl_reg_data am4_l3s_clkctrl_regs[] __initconst = {
 	{ AM4_L3S_VPFE0_CLKCTRL, NULL, CLKF_SW_SUP, "l3_gclk" },
 	{ AM4_L3S_VPFE1_CLKCTRL, NULL, CLKF_SW_SUP, "l3_gclk" },
 	{ AM4_L3S_GPMC_CLKCTRL, NULL, CLKF_SW_SUP, "l3s_gclk" },
+	{ AM4_L3S_ADC1_CLKCTRL, NULL, CLKF_SW_SUP, "l3s_gclk" },
 	{ AM4_L3S_MCASP0_CLKCTRL, NULL, CLKF_SW_SUP, "mcasp0_fck" },
 	{ AM4_L3S_MCASP1_CLKCTRL, NULL, CLKF_SW_SUP, "mcasp1_fck" },
 	{ AM4_L3S_MMC3_CLKCTRL, NULL, CLKF_SW_SUP, "mmc_clk" },
diff --git a/include/dt-bindings/clock/am4.h b/include/dt-bindings/clock/am4.h
index d961e7cb3682..4be6c5961f34 100644
--- a/include/dt-bindings/clock/am4.h
+++ b/include/dt-bindings/clock/am4.h
@@ -158,6 +158,7 @@
 #define AM4_L3S_VPFE0_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x68)
 #define AM4_L3S_VPFE1_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x70)
 #define AM4_L3S_GPMC_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x220)
+#define AM4_L3S_ADC1_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x230)
 #define AM4_L3S_MCASP0_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x238)
 #define AM4_L3S_MCASP1_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x240)
 #define AM4_L3S_MMC3_CLKCTRL	AM4_L3S_CLKCTRL_INDEX(0x248)
-- 
2.27.0

Re: [PATCH v3 01/47] clk: ti: am43xx: Add clkctrl data for am43xx ADC1

[Tony Lindgren]

* Miquel Raynal <miquel.raynal@bootlin.com> [210915 15:59]:
> Declare ADC1 clkctrl which feeds the magnetic-reader/ADC1 hardware
> module.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Stephen Boyd <sboyd@kernel.org>

Looks good to me:

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH v3 02/47] dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this MFD

[Miquel Raynal]

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.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 .../bindings/mfd/ti,am3359-tscadc.yaml        | 79 +++++++++++++++++++
 1 file changed, 79 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..b2bff85f2329
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
@@ -0,0 +1,79 @@
+# 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:
+    oneOf:
+      - const: ti,am3359-tscadc
+      - items:
+          - const: ti,am654-tscadc
+          - const: ti,am3359-tscadc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  clock-names:
+    const: fck
+
+  dmas:
+    items:
+      - description: DMA controller phandle and request line for FIFO0
+      - description: DMA controller phandle and request line for FIFO1
+
+  dma-names:
+    items:
+      - const: fifo0
+      - const: fifo1
+
+  adc:
+    type: object
+    description: ADC child
+
+  tsc:
+    type: object
+    description: Touchscreen controller child
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - dmas
+  - dma-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    tscadc@0 {
+        compatible = "ti,am3359-tscadc";
+        reg = <0x0 0x1000>;
+        interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&adc_tsc_fck>;
+        clock-names = "fck";
+        dmas = <&edma 53 0>, <&edma 57 0>;
+        dma-names = "fifo0", "fifo1";
+
+        tsc {
+        };
+
+        adc {
+        };
+    };
-- 
2.27.0

[PATCH v3 03/47] dt-bindings: touchscreen: ti,am3359-tsc: New yaml description

[Miquel Raynal]

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>
Reviewed-by: Rob Herring <robh@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 .../input/touchscreen/ti,am3359-tsc.yaml      | 76 +++++++++++++++++++
 1 file changed, 76 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..e44cc65abc8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
@@ -0,0 +1,76 @@
+# 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 until 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
+  - ti,x-plate-resistance
+  - ti,coordinate-readouts
+  - ti,wire-config
+
+additionalProperties: false
+
+examples:
+  - |
+    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

Re: [PATCH v3 03/47] dt-bindings: touchscreen: ti,am3359-tsc: New yaml description

[Dmitry Torokhov]

On Wed, Sep 15, 2021 at 05:58:24PM +0200, Miquel Raynal wrote:
> 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>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I expect this will be merged through MFD tree with the rest of the
patches, so

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

[PATCH v3 04/47] dt-bindings: iio: adc: ti,am3359-adc: New yaml description

[Miquel Raynal]

This ADC was already described in a text file also containing an MFD
description an a touchscreen description:
Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
Let's add a proper description for this hardware.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/iio/adc/ti,am3359-adc.yaml       | 68 +++++++++++++++++++
 1 file changed, 68 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml b/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
new file mode 100644
index 000000000000..f262402ca037
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/ti,am3359-adc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TI AM3359 ADC
+
+maintainers:
+  - Miquel Raynal <miquel.raynal@bootlin.com>
+
+properties:
+  compatible:
+    const: ti,am3359-adc
+
+  '#io-channel-cells':
+    const: 1
+
+  ti,adc-channels:
+    description: List of analog inputs available for ADC. AIN0 = 0, AIN1 = 1 and
+      so on until AIN7 = 7.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+
+  ti,chan-step-opendelay:
+    description: List of open delays for each channel of ADC in the order of
+      ti,adc-channels. The value corresponds to the number of ADC clock cycles
+      to wait after applying the step configuration registers and before sending
+      the start of ADC conversion. Maximum value is 0x3FFFF.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+
+  ti,chan-step-sampledelay:
+    description: List of sample delays for each channel of ADC in the order of
+      ti,adc-channels. The value corresponds to the number of ADC clock cycles
+      to sample (to hold start of conversion high). Maximum value is 0xFF.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+
+  ti,chan-step-avg:
+    description: Number of averages to be performed for each channel of ADC. If
+      average is 16 (this is also the maximum) then input is sampled 16 times
+      and averaged to get more accurate value. This increases the time taken by
+      ADC to generate a sample. Maximum value is 16.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 8
+
+required:
+  - compatible
+  - '#io-channel-cells'
+  - ti,adc-channels
+
+additionalProperties: false
+
+examples:
+  - |
+    adc {
+        compatible = "ti,am3359-adc";
+        #io-channel-cells = <1>;
+        ti,adc-channels = <4 5 6 7>;
+        ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
+        ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
+        ti,chan-step-avg = <16 2 4 8>;
+    };
-- 
2.27.0

[PATCH v3 05/47] dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file

[Miquel Raynal]

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>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../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

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
deleted file mode 100644
index aad5e34965eb..000000000000
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ /dev/null
@@ -1,91 +0,0 @@
-* TI - TSC ADC (Touschscreen and analog digital converter)
-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-
-Required properties:
-- mfd
-	compatible: Should be
-		"ti,am3359-tscadc" for AM335x/AM437x SoCs
-		"ti,am654-tscadc", "ti,am3359-tscadc" for AM654 SoCs
-- child "tsc"
-	compatible: Should be "ti,am3359-tsc".
-	ti,wires: Wires refer to application modes i.e. 4/5/8 wire touchscreen
-		  support on the platform.
-	ti,x-plate-resistance: X plate resistance
-	ti,coordinate-readouts: 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.
-	ti,wire-config: Different boards could have a different order for
-			connecting wires on touchscreen. We need to provide an
-			8 bit number where in the 1st 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 resoectively
-			is as follows:
-			AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
-			XP  = 0, XN = 1, YP = 2, YN = 3.
-- child "adc"
-	compatible: Should be
-		    "ti,am3359-adc" for AM335x/AM437x SoCs
-		    "ti,am654-adc", "ti,am3359-adc" for AM654 SoCs
-	ti,adc-channels: List of analog inputs available for ADC.
-			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
-
-Optional properties:
-- child "tsc"
-	ti,charge-delay: 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".
-
-- child "adc"
-	ti,chan-step-opendelay: List of open delays for each channel of
-				ADC in the order of ti,adc-channels. The
-				value corresponds to the number of ADC
-				clock cycles to wait after applying the
-				step configuration registers and before
-				sending the start of ADC conversion.
-				Maximum value is 0x3FFFF.
-       ti,chan-step-sampledelay: List of sample delays for each channel
-				  of ADC in the order of ti,adc-channels.
-				  The value corresponds to the number of
-				  ADC clock cycles to sample (to hold
-				  start of conversion high).
-				  Maximum value is 0xFF.
-       ti,chan-step-avg: Number of averages to be performed for each
-			  channel of ADC. If average is 16 then input
-			  is sampled 16 times and averaged to get more
-			  accurate value. This increases the time taken
-			  by ADC to generate a sample. Valid range is 0
-			  average to 16 averages. Maximum value is 16.
-
-Example:
-	tscadc: tscadc@44e0d000 {
-		compatible = "ti,am3359-tscadc";
-		tsc {
-			ti,wires = <4>;
-			ti,x-plate-resistance = <200>;
-			ti,coordiante-readouts = <5>;
-			ti,wire-config = <0x00 0x11 0x22 0x33>;
-			ti,charge-delay = <0x400>;
-		};
-
-		adc {
-			ti,adc-channels = <4 5 6 7>;
-			ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
-			ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
-			ti,chan-step-avg = <16 2 4 8>;
-		};
-	}
-- 
2.27.0

Re: [PATCH v3 05/47] dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file

[Dmitry Torokhov]

On Wed, Sep 15, 2021 at 05:58:26PM +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>
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Acked-by: Rob Herring <robh@kernel.org>

I expect this will be merged through MFD tree with the rest of the
patches, so

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

[PATCH v3 06/47] dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible

[Miquel Raynal]

A more recent version of the am3359 ADC/Touchscreen controller is
available on certain SoCs of the am437x family. This IP has evolved a
little bit and deserves its own compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml b/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
index b2bff85f2329..34bf6a01436f 100644
--- a/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
+++ b/Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
@@ -16,6 +16,7 @@ properties:
       - items:
           - const: ti,am654-tscadc
           - const: ti,am3359-tscadc
+      - const: ti,am4372-magadc
 
   reg:
     maxItems: 1
@@ -47,6 +48,10 @@ properties:
     type: object
     description: Touchscreen controller child
 
+  mag:
+    type: object
+    description: Magnetic reader
+
 required:
   - compatible
   - reg
-- 
2.27.0

[PATCH v3 07/47] dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible

[Miquel Raynal]

A more recent version of the am3359 ADC is available on certain SoCs of
the am437x family. This IP has evolved a little bit and deserves its own
compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml b/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
index f262402ca037..d6f21d5cccd7 100644
--- a/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
@@ -11,7 +11,9 @@ maintainers:
 
 properties:
   compatible:
-    const: ti,am3359-adc
+    enum:
+      - ti,am3359-adc
+      - ti,am4372-adc
 
   '#io-channel-cells':
     const: 1
-- 
2.27.0

[PATCH v3 08/47] mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put

[Miquel Raynal]

of_node_put() should be called after a successful of_get_child_by_name().

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 55adc379f94b..eb5c23e3eacd 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -142,6 +142,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
 	of_property_read_u32(node, "ti,wires", &tsc_wires);
 	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
+	of_node_put(node);
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
@@ -149,9 +150,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		if (val > 7) {
 			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
 					val);
+			of_node_put(node);
 			return -EINVAL;
 		}
 	}
+
+	of_node_put(node);
+
 	total_channels = tsc_wires + adc_channels;
 	if (total_channels > 8) {
 		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
-- 
2.27.0

[PATCH v3 09/47] mfd: ti_am335x_tscadc: Replace license text with SPDX tag

[Miquel Raynal]

Drop the text license and replace it with an equivalent SPDX license tag
identifier which also matches the MODULE_LICENSE(GPL) macro.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index eb5c23e3eacd..82ace1cd6d2e 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -1,16 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * TI Touch Screen / ADC MFD driver
  *
  * Copyright (C) 2012 Texas Instruments Incorporated - https://www.ti.com/
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation version 2.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether express or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #include <linux/module.h>
-- 
2.27.0

[PATCH v3 10/47] mfd: ti_am335x_tscadc: Fix style

[Miquel Raynal]

These are mostly deffects reported by checkpatch.pl.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 82ace1cd6d2e..00428d12e6b8 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -141,7 +141,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		adc_channels++;
 		if (val > 7) {
 			dev_err(&pdev->dev, " PIN numbers are 0..7 (not %d)\n",
-					val);
+				val);
 			of_node_put(node);
 			return -EINVAL;
 		}
@@ -154,6 +154,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
 		return -EINVAL;
 	}
+
 	if (total_channels == 0) {
 		dev_err(&pdev->dev, "Need atleast one channel.\n");
 		return -EINVAL;
@@ -184,7 +185,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	tscadc->tscadc_phys_base = res->start;
 	tscadc->regmap = devm_regmap_init_mmio(&pdev->dev,
-			tscadc->tscadc_base, &tscadc_regmap_config);
+					       tscadc->tscadc_base,
+					       &tscadc_regmap_config);
 	if (IS_ERR(tscadc->regmap)) {
 		dev_err(&pdev->dev, "regmap init failed\n");
 		err = PTR_ERR(tscadc->regmap);
-- 
2.27.0

[PATCH v3 11/47] mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack variables

[Miquel Raynal]

Many variables will be updated (renamed, dropped, added) in the upcoming
changes, so let's simplify the style to avoid messing with spaces over
and over again.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 00428d12e6b8..07da4059d7f7 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -113,18 +113,18 @@ static void tscadc_idle_config(struct ti_tscadc_dev *tscadc)
 
 static	int ti_tscadc_probe(struct platform_device *pdev)
 {
-	struct ti_tscadc_dev	*tscadc;
-	struct resource		*res;
-	struct clk		*clk;
-	struct device_node	*node;
-	struct mfd_cell		*cell;
-	struct property         *prop;
-	const __be32            *cur;
-	u32			val;
-	int			err, ctrl;
-	int			clock_rate;
-	int			tsc_wires = 0, adc_channels = 0, total_channels;
-	int			readouts = 0;
+	struct ti_tscadc_dev *tscadc;
+	struct resource *res;
+	struct clk *clk;
+	struct device_node *node;
+	struct mfd_cell *cell;
+	struct property *prop;
+	const __be32 *cur;
+	u32 val;
+	int err, ctrl;
+	int clock_rate;
+	int tsc_wires = 0, adc_channels = 0, total_channels;
+	int readouts = 0;
 
 	if (!pdev->dev.of_node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
@@ -280,7 +280,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 static int ti_tscadc_remove(struct platform_device *pdev)
 {
-	struct ti_tscadc_dev	*tscadc = platform_get_drvdata(pdev);
+	struct ti_tscadc_dev *tscadc = platform_get_drvdata(pdev);
 
 	regmap_write(tscadc->regmap, REG_SE, 0x00);
 
@@ -299,7 +299,7 @@ static int __maybe_unused ti_tscadc_can_wakeup(struct device *dev, void *data)
 
 static int __maybe_unused tscadc_suspend(struct device *dev)
 {
-	struct ti_tscadc_dev	*tscadc = dev_get_drvdata(dev);
+	struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev);
 
 	regmap_write(tscadc->regmap, REG_SE, 0x00);
 	if (device_for_each_child(dev, NULL, ti_tscadc_can_wakeup)) {
@@ -317,7 +317,7 @@ 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);
+	struct ti_tscadc_dev *tscadc = dev_get_drvdata(dev);
 	u32 ctrl;
 
 	pm_runtime_get_sync(dev);
-- 
2.27.0

[PATCH v3 12/47] mfd: ti_am335x_tscadc: Get rid of useless gotos

[Miquel Raynal]

Goto's jumping to a return statement are not really useful, drop them.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 07da4059d7f7..a211b35ad106 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -174,7 +174,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	err = platform_get_irq(pdev, 0);
 	if (err < 0)
-		goto ret;
+		return err;
 	else
 		tscadc->irq = err;
 
@@ -189,8 +189,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 					       &tscadc_regmap_config);
 	if (IS_ERR(tscadc->regmap)) {
 		dev_err(&pdev->dev, "regmap init failed\n");
-		err = PTR_ERR(tscadc->regmap);
-		goto ret;
+		return PTR_ERR(tscadc->regmap);
 	}
 
 	spin_lock_init(&tscadc->reg_lock);
@@ -274,7 +273,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 err_disable_clk:
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-ret:
+
 	return err;
 }
 
-- 
2.27.0

[PATCH v3 13/47] mfd: ti_am335x_tscadc: Reword the comment explaining the dividers

[Miquel Raynal]

The comment misses the main information which is that we assume that a
sample takes 15 ADC clock cycles to be generated. Let's take the
occasion to rework a little bit this comment.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index a211b35ad106..7aeabac37990 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -199,12 +199,12 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	pm_runtime_get_sync(&pdev->dev);
 
 	/*
-	 * The TSC_ADC_Subsystem has 2 clock domains
-	 * OCP_CLK and ADC_CLK.
-	 * The ADC clock is expected to run at target of 3MHz,
-	 * and expected to capture 12-bit data at a rate of 200 KSPS.
-	 * The TSC_ADC_SS controller design assumes the OCP clock is
-	 * at least 6x faster than the ADC clock.
+	 * The TSC_ADC_Subsystem has 2 clock domains: OCP_CLK and ADC_CLK.
+	 * ADCs produce a 12-bit sample every 15 ADC_CLK cycles.
+	 * am33xx ADCs expect to capture 200ksps.
+	 * We need the ADC clocks to run at 3MHz.
+	 * This frequency is valid since TSC_ADC_SS controller design
+	 * assumes the OCP clock is at least 6x faster than the ADC clock.
 	 */
 	clk = devm_clk_get(&pdev->dev, "adc_tsc_fck");
 	if (IS_ERR(clk)) {
-- 
2.27.0

[PATCH v3 14/47] mfd: ti_am335x_tscadc: Don't search the tree for our clock

[Miquel Raynal]

There is a single clock available in our node, which is named
"fck". The clock handler then points to adc_tsc_fck but no need to point
directly to it and do a full tree search.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7aeabac37990..7f7b8e79a7bf 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -206,7 +206,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * This frequency is valid since TSC_ADC_SS controller design
 	 * assumes the OCP clock is at least 6x faster than the ADC clock.
 	 */
-	clk = devm_clk_get(&pdev->dev, "adc_tsc_fck");
+	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		dev_err(&pdev->dev, "failed to get TSC fck\n");
 		err = PTR_ERR(clk);
-- 
2.27.0

[PATCH v3 15/47] mfd: ti_am335x_tscadc: Simplify divisor calculation

[Miquel Raynal]

Let's rewrite this on a single line, it does not hurt the readability
and saves a useless temporary variable.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7f7b8e79a7bf..20d8ddee27fe 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -122,7 +122,6 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	const __be32 *cur;
 	u32 val;
 	int err, ctrl;
-	int clock_rate;
 	int tsc_wires = 0, adc_channels = 0, total_channels;
 	int readouts = 0;
 
@@ -212,11 +211,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		err = PTR_ERR(clk);
 		goto err_disable_clk;
 	}
-	clock_rate = clk_get_rate(clk);
-	tscadc->clk_div = clock_rate / ADC_CLK;
 
-	/* TSCADC_CLKDIV needs to be configured to the value minus 1 */
-	tscadc->clk_div--;
+	tscadc->clk_div = (clk_get_rate(clk) / ADC_CLK) - 1;
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
 	/* Set the control register bits */
-- 
2.27.0

[PATCH v3 16/47] mfd: ti_am335x_tscadc: Move the driver structure allocation earlier

[Miquel Raynal]

Allocating the driver structure should be done earlier in the probe so
that we can used its members from the beginning.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 20d8ddee27fe..ba821109e98b 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -125,6 +125,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	int tsc_wires = 0, adc_channels = 0, total_channels;
 	int readouts = 0;
 
+	/* Allocate memory for device */
+	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
+	if (!tscadc)
+		return -ENOMEM;
+
+	tscadc->dev = &pdev->dev;
+
 	if (!pdev->dev.of_node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
 		return -EINVAL;
@@ -164,13 +171,6 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/* Allocate memory for device */
-	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
-	if (!tscadc)
-		return -ENOMEM;
-
-	tscadc->dev = &pdev->dev;
-
 	err = platform_get_irq(pdev, 0);
 	if (err < 0)
 		return err;
-- 
2.27.0

[PATCH v3 17/47] mfd: ti_am335x_tscadc: Use driver data

[Miquel Raynal]

So far every sub-cell parameter in this driver was hardcoded: cell name,
cell compatible, specific clock name and desired clock frequency.

As we are about to introduce support for ADC1/magnetic reader, we need a
bit of flexibility. Let's add a driver data structure which will contain
these information.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 25 +++++++++++++++++++------
 include/linux/mfd/ti_am335x_tscadc.h |  9 +++++++++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index ba821109e98b..fbc8e338188a 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -137,6 +137,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	tscadc->data = of_device_get_match_data(&pdev->dev);
+
 	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
 	of_property_read_u32(node, "ti,wires", &tsc_wires);
 	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
@@ -212,7 +214,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		goto err_disable_clk;
 	}
 
-	tscadc->clk_div = (clk_get_rate(clk) / ADC_CLK) - 1;
+	tscadc->clk_div = (clk_get_rate(clk) / tscadc->data->target_clk_rate) - 1;
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
 	/* Set the control register bits */
@@ -241,8 +243,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (tsc_wires > 0) {
 		tscadc->tsc_cell = tscadc->used_cells;
 		cell = &tscadc->cells[tscadc->used_cells++];
-		cell->name = "TI-am335x-tsc";
-		cell->of_compatible = "ti,am3359-tsc";
+		cell->name = tscadc->data->name_tscmag;
+		cell->of_compatible = tscadc->data->compat_tscmag;
 		cell->platform_data = &tscadc;
 		cell->pdata_size = sizeof(tscadc);
 	}
@@ -251,8 +253,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (adc_channels > 0) {
 		tscadc->adc_cell = tscadc->used_cells;
 		cell = &tscadc->cells[tscadc->used_cells++];
-		cell->name = "TI-am335x-adc";
-		cell->of_compatible = "ti,am3359-adc";
+		cell->name = tscadc->data->name_adc;
+		cell->of_compatible = tscadc->data->compat_adc;
 		cell->platform_data = &tscadc;
 		cell->pdata_size = sizeof(tscadc);
 	}
@@ -338,8 +340,19 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
 
+static const struct ti_tscadc_data tscdata = {
+	.name_tscmag = "TI-am335x-tsc",
+	.compat_tscmag = "ti,am3359-tsc",
+	.name_adc = "TI-am335x-adc",
+	.compat_adc = "ti,am3359-adc",
+	.target_clk_rate = ADC_CLK,
+};
+
 static const struct of_device_id ti_tscadc_dt_ids[] = {
-	{ .compatible = "ti,am3359-tscadc", },
+	{
+		.compatible = "ti,am3359-tscadc",
+		.data = &tscdata,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index ffc091b77633..0f581c15d95a 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -162,11 +162,20 @@
 
 #define TSCADC_CELLS		2
 
+struct ti_tscadc_data {
+	char *name_tscmag;
+	char *compat_tscmag;
+	char *name_adc;
+	char *compat_adc;
+	unsigned int target_clk_rate;
+};
+
 struct ti_tscadc_dev {
 	struct device *dev;
 	struct regmap *regmap;
 	void __iomem *tscadc_base;
 	phys_addr_t tscadc_phys_base;
+	const struct ti_tscadc_data *data;
 	int irq;
 	int used_cells;	/* 1-2 */
 	int tsc_wires;
-- 
2.27.0

Re: [PATCH v3 17/47] mfd: ti_am335x_tscadc: Use driver data

[Lee Jones]

On Wed, 15 Sep 2021, Miquel Raynal wrote:

> So far every sub-cell parameter in this driver was hardcoded: cell name,
> cell compatible, specific clock name and desired clock frequency.
> 
> As we are about to introduce support for ADC1/magnetic reader, we need a
> bit of flexibility. Let's add a driver data structure which will contain
> these information.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c       | 25 +++++++++++++++++++------
>  include/linux/mfd/ti_am335x_tscadc.h |  9 +++++++++
>  2 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index ba821109e98b..fbc8e338188a 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -137,6 +137,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	tscadc->data = of_device_get_match_data(&pdev->dev);
> +
>  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>  	of_property_read_u32(node, "ti,wires", &tsc_wires);
>  	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> @@ -212,7 +214,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  		goto err_disable_clk;
>  	}
>  
> -	tscadc->clk_div = (clk_get_rate(clk) / ADC_CLK) - 1;
> +	tscadc->clk_div = (clk_get_rate(clk) / tscadc->data->target_clk_rate) - 1;
>  	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
>  
>  	/* Set the control register bits */
> @@ -241,8 +243,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	if (tsc_wires > 0) {
>  		tscadc->tsc_cell = tscadc->used_cells;
>  		cell = &tscadc->cells[tscadc->used_cells++];
> -		cell->name = "TI-am335x-tsc";
> -		cell->of_compatible = "ti,am3359-tsc";
> +		cell->name = tscadc->data->name_tscmag;
> +		cell->of_compatible = tscadc->data->compat_tscmag;
>  		cell->platform_data = &tscadc;
>  		cell->pdata_size = sizeof(tscadc);
>  	}
> @@ -251,8 +253,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	if (adc_channels > 0) {
>  		tscadc->adc_cell = tscadc->used_cells;
>  		cell = &tscadc->cells[tscadc->used_cells++];
> -		cell->name = "TI-am335x-adc";
> -		cell->of_compatible = "ti,am3359-adc";
> +		cell->name = tscadc->data->name_adc;
> +		cell->of_compatible = tscadc->data->compat_adc;
>  		cell->platform_data = &tscadc;
>  		cell->pdata_size = sizeof(tscadc);
>  	}
> @@ -338,8 +340,19 @@ static int __maybe_unused tscadc_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
>  
> +static const struct ti_tscadc_data tscdata = {
> +	.name_tscmag = "TI-am335x-tsc",
> +	.compat_tscmag = "ti,am3359-tsc",
> +	.name_adc = "TI-am335x-adc",
> +	.compat_adc = "ti,am3359-adc",
> +	.target_clk_rate = ADC_CLK,
> +};
> +
>  static const struct of_device_id ti_tscadc_dt_ids[] = {
> -	{ .compatible = "ti,am3359-tscadc", },
> +	{
> +		.compatible = "ti,am3359-tscadc",
> +		.data = &tscdata,
> +	},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index ffc091b77633..0f581c15d95a 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -162,11 +162,20 @@
>  
>  #define TSCADC_CELLS		2
>  
> +struct ti_tscadc_data {
> +	char *name_tscmag;
> +	char *compat_tscmag;
> +	char *name_adc;
> +	char *compat_adc;

I think these names should be improved.

What is tscmag?

Does that represent both the Magnetic Reader and the Touchscreen?

If so, I'd prefer that you split them.  If not, I need more info.

For readability, I suggest;

  touchscreen_name
  touchscreen_compatible
  mag_reader_name
  mag_reader_compatible
  adc_name
  adc_compatible
  etc

What is a magnetic reader anyway?

Does it read the magnetic stripe on a payment card?

> +	unsigned int target_clk_rate;
> +};
> +
>  struct ti_tscadc_dev {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	void __iomem *tscadc_base;
>  	phys_addr_t tscadc_phys_base;
> +	const struct ti_tscadc_data *data;
>  	int irq;
>  	int used_cells;	/* 1-2 */
>  	int tsc_wires;

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 17/47] mfd: ti_am335x_tscadc: Use driver data

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:01:51 +0100:

> On Wed, 15 Sep 2021, Miquel Raynal wrote:
> 
> > So far every sub-cell parameter in this driver was hardcoded: cell name,
> > cell compatible, specific clock name and desired clock frequency.
> > 
> > As we are about to introduce support for ADC1/magnetic reader, we need a
> > bit of flexibility. Let's add a driver data structure which will contain
> > these information.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/mfd/ti_am335x_tscadc.c       | 25 +++++++++++++++++++------
> >  include/linux/mfd/ti_am335x_tscadc.h |  9 +++++++++
> >  2 files changed, 28 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > index ba821109e98b..fbc8e338188a 100644
> > --- a/drivers/mfd/ti_am335x_tscadc.c
> > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > @@ -137,6 +137,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  		return -EINVAL;
> >  	}
> >  
> > +	tscadc->data = of_device_get_match_data(&pdev->dev);
> > +
> >  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> >  	of_property_read_u32(node, "ti,wires", &tsc_wires);
> >  	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > @@ -212,7 +214,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  		goto err_disable_clk;
> >  	}
> >  
> > -	tscadc->clk_div = (clk_get_rate(clk) / ADC_CLK) - 1;
> > +	tscadc->clk_div = (clk_get_rate(clk) / tscadc->data->target_clk_rate) - 1;
> >  	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
> >  
> >  	/* Set the control register bits */
> > @@ -241,8 +243,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	if (tsc_wires > 0) {
> >  		tscadc->tsc_cell = tscadc->used_cells;
> >  		cell = &tscadc->cells[tscadc->used_cells++];
> > -		cell->name = "TI-am335x-tsc";
> > -		cell->of_compatible = "ti,am3359-tsc";
> > +		cell->name = tscadc->data->name_tscmag;
> > +		cell->of_compatible = tscadc->data->compat_tscmag;
> >  		cell->platform_data = &tscadc;
> >  		cell->pdata_size = sizeof(tscadc);
> >  	}
> > @@ -251,8 +253,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	if (adc_channels > 0) {
> >  		tscadc->adc_cell = tscadc->used_cells;
> >  		cell = &tscadc->cells[tscadc->used_cells++];
> > -		cell->name = "TI-am335x-adc";
> > -		cell->of_compatible = "ti,am3359-adc";
> > +		cell->name = tscadc->data->name_adc;
> > +		cell->of_compatible = tscadc->data->compat_adc;
> >  		cell->platform_data = &tscadc;
> >  		cell->pdata_size = sizeof(tscadc);
> >  	}
> > @@ -338,8 +340,19 @@ static int __maybe_unused tscadc_resume(struct device *dev)
> >  
> >  static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
> >  
> > +static const struct ti_tscadc_data tscdata = {
> > +	.name_tscmag = "TI-am335x-tsc",
> > +	.compat_tscmag = "ti,am3359-tsc",
> > +	.name_adc = "TI-am335x-adc",
> > +	.compat_adc = "ti,am3359-adc",
> > +	.target_clk_rate = ADC_CLK,
> > +};
> > +
> >  static const struct of_device_id ti_tscadc_dt_ids[] = {
> > -	{ .compatible = "ti,am3359-tscadc", },
> > +	{
> > +		.compatible = "ti,am3359-tscadc",
> > +		.data = &tscdata,
> > +	},
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
> > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> > index ffc091b77633..0f581c15d95a 100644
> > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > @@ -162,11 +162,20 @@
> >  
> >  #define TSCADC_CELLS		2
> >  
> > +struct ti_tscadc_data {
> > +	char *name_tscmag;
> > +	char *compat_tscmag;
> > +	char *name_adc;
> > +	char *compat_adc;  
> 
> I think these names should be improved.
> 
> What is tscmag?
> 
> Does that represent both the Magnetic Reader and the Touchscreen?

Not exactly, it represents *either* the magnetic reader *or* the
touchscreen.

Basically you can have either one version of the hardware which
is a regular ADC that can be also used as a touchscreen controller, or
you can have another version of the hardware which is a regular ADC
that can be also used as a magnetic reader.

Both features can be used as the same time (ts + adc or mag + adc),
hence we need a name for the touchscreen child node and for the adc
child node *or* a name for the magnetic reader chil node and for the adc
child node.

> If so, I'd prefer that you split them.  If not, I need more info.
> 
> For readability, I suggest;
> 
>   touchscreen_name
>   touchscreen_compatible
>   mag_reader_name
>   mag_reader_compatible
>   adc_name
>   adc_compatible
>   etc
> 

I can certainly improve the names though.

> What is a magnetic reader anyway?
> 
> Does it read the magnetic stripe on a payment card?

Yes!

> 
> > +	unsigned int target_clk_rate;
> > +};
> > +
> >  struct ti_tscadc_dev {
> >  	struct device *dev;
> >  	struct regmap *regmap;
> >  	void __iomem *tscadc_base;
> >  	phys_addr_t tscadc_phys_base;
> > +	const struct ti_tscadc_data *data;
> >  	int irq;
> >  	int used_cells;	/* 1-2 */
> >  	int tsc_wires;  
> 


Thanks,
Miquèl

Re: [PATCH v3 17/47] mfd: ti_am335x_tscadc: Use driver data

[Lee Jones]

On Wed, 22 Sep 2021, Miquel Raynal wrote:

> Hi Lee,
> 
> lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:01:51 +0100:
> 
> > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > 
> > > So far every sub-cell parameter in this driver was hardcoded: cell name,
> > > cell compatible, specific clock name and desired clock frequency.
> > > 
> > > As we are about to introduce support for ADC1/magnetic reader, we need a
> > > bit of flexibility. Let's add a driver data structure which will contain
> > > these information.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  drivers/mfd/ti_am335x_tscadc.c       | 25 +++++++++++++++++++------
> > >  include/linux/mfd/ti_am335x_tscadc.h |  9 +++++++++
> > >  2 files changed, 28 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > index ba821109e98b..fbc8e338188a 100644
> > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > @@ -137,6 +137,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > +	tscadc->data = of_device_get_match_data(&pdev->dev);
> > > +
> > >  	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > >  	of_property_read_u32(node, "ti,wires", &tsc_wires);
> > >  	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > @@ -212,7 +214,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  		goto err_disable_clk;
> > >  	}
> > >  
> > > -	tscadc->clk_div = (clk_get_rate(clk) / ADC_CLK) - 1;
> > > +	tscadc->clk_div = (clk_get_rate(clk) / tscadc->data->target_clk_rate) - 1;
> > >  	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
> > >  
> > >  	/* Set the control register bits */
> > > @@ -241,8 +243,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  	if (tsc_wires > 0) {
> > >  		tscadc->tsc_cell = tscadc->used_cells;
> > >  		cell = &tscadc->cells[tscadc->used_cells++];
> > > -		cell->name = "TI-am335x-tsc";
> > > -		cell->of_compatible = "ti,am3359-tsc";
> > > +		cell->name = tscadc->data->name_tscmag;
> > > +		cell->of_compatible = tscadc->data->compat_tscmag;
> > >  		cell->platform_data = &tscadc;
> > >  		cell->pdata_size = sizeof(tscadc);
> > >  	}
> > > @@ -251,8 +253,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  	if (adc_channels > 0) {
> > >  		tscadc->adc_cell = tscadc->used_cells;
> > >  		cell = &tscadc->cells[tscadc->used_cells++];
> > > -		cell->name = "TI-am335x-adc";
> > > -		cell->of_compatible = "ti,am3359-adc";
> > > +		cell->name = tscadc->data->name_adc;
> > > +		cell->of_compatible = tscadc->data->compat_adc;
> > >  		cell->platform_data = &tscadc;
> > >  		cell->pdata_size = sizeof(tscadc);
> > >  	}
> > > @@ -338,8 +340,19 @@ static int __maybe_unused tscadc_resume(struct device *dev)
> > >  
> > >  static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
> > >  
> > > +static const struct ti_tscadc_data tscdata = {
> > > +	.name_tscmag = "TI-am335x-tsc",
> > > +	.compat_tscmag = "ti,am3359-tsc",
> > > +	.name_adc = "TI-am335x-adc",
> > > +	.compat_adc = "ti,am3359-adc",
> > > +	.target_clk_rate = ADC_CLK,
> > > +};
> > > +
> > >  static const struct of_device_id ti_tscadc_dt_ids[] = {
> > > -	{ .compatible = "ti,am3359-tscadc", },
> > > +	{
> > > +		.compatible = "ti,am3359-tscadc",
> > > +		.data = &tscdata,
> > > +	},
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
> > > diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> > > index ffc091b77633..0f581c15d95a 100644
> > > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > > @@ -162,11 +162,20 @@
> > >  
> > >  #define TSCADC_CELLS		2
> > >  
> > > +struct ti_tscadc_data {
> > > +	char *name_tscmag;
> > > +	char *compat_tscmag;
> > > +	char *name_adc;
> > > +	char *compat_adc;  
> > 
> > I think these names should be improved.
> > 
> > What is tscmag?
> > 
> > Does that represent both the Magnetic Reader and the Touchscreen?
> 
> Not exactly, it represents *either* the magnetic reader *or* the
> touchscreen.
> 
> Basically you can have either one version of the hardware which
> is a regular ADC that can be also used as a touchscreen controller, or
> you can have another version of the hardware which is a regular ADC
> that can be also used as a magnetic reader.
> 
> Both features can be used as the same time (ts + adc or mag + adc),
> hence we need a name for the touchscreen child node and for the adc
> child node *or* a name for the magnetic reader chil node and for the adc
> child node.
> 
> > If so, I'd prefer that you split them.  If not, I need more info.
> > 
> > For readability, I suggest;
> > 
> >   touchscreen_name
> >   touchscreen_compatible
> >   mag_reader_name
> >   mag_reader_compatible
> >   adc_name
> >   adc_compatible
> >   etc
> > 
> 
> I can certainly improve the names though.

Thanks.

> > What is a magnetic reader anyway?
> > 
> > Does it read the magnetic stripe on a payment card?
> 
> Yes!

The mag_stripe_reader might be nice.

> > > +	unsigned int target_clk_rate;
> > > +};
> > > +
> > >  struct ti_tscadc_dev {
> > >  	struct device *dev;
> > >  	struct regmap *regmap;
> > >  	void __iomem *tscadc_base;
> > >  	phys_addr_t tscadc_phys_base;
> > > +	const struct ti_tscadc_data *data;
> > >  	int irq;
> > >  	int used_cells;	/* 1-2 */
> > >  	int tsc_wires;  
> > 
> 
> 
> Thanks,
> Miquèl

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v3 18/47] mfd: ti_am335x_tscadc: Mimic the probe from resume()

[Miquel Raynal]

Use the number of touchscreen wires (tsc_wires) instead of the flag
indicating a touchscreen cell (tsc_cell) to mimic the logic from the
probe.

Besides keeping a certain harmony in the driver, the main benefit is
that we are going to simplify the number of entries in the main MFD
structure and tsc_cell can now be removed more easily.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index fbc8e338188a..81f0ebf1a415 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -323,7 +323,7 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-	if (tscadc->tsc_cell != -1) {
+	if (tscadc->tsc_wires > 0) {
 		if (tscadc->tsc_wires == 5)
 			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
 		else
-- 
2.27.0

[PATCH v3 19/47] mfd: ti_am335x_tscadc: Drop useless variables from the driver structure

[Miquel Raynal]

Keeping the count of tsc_cells and adc_cells is completely redundant, we
can derive this information from other variables. Plus, these variables
are not used anywhere else now. Let's get rid of them.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 15 ++++-----------
 include/linux/mfd/ti_am335x_tscadc.h |  3 ---
 2 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 81f0ebf1a415..8404fc662725 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)
 	const __be32 *cur;
 	u32 val;
 	int err, ctrl;
-	int tsc_wires = 0, adc_channels = 0, total_channels;
+	int tsc_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
 	int readouts = 0;
 
 	/* Allocate memory for device */
@@ -235,14 +235,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	ctrl |= CNTRLREG_TSCSSENB;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-	tscadc->used_cells = 0;
-	tscadc->tsc_cell = -1;
-	tscadc->adc_cell = -1;
-
 	/* TSC Cell */
 	if (tsc_wires > 0) {
-		tscadc->tsc_cell = tscadc->used_cells;
-		cell = &tscadc->cells[tscadc->used_cells++];
+		cell = &tscadc->cells[cell_idx++];
 		cell->name = tscadc->data->name_tscmag;
 		cell->of_compatible = tscadc->data->compat_tscmag;
 		cell->platform_data = &tscadc;
@@ -251,8 +246,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	/* ADC Cell */
 	if (adc_channels > 0) {
-		tscadc->adc_cell = tscadc->used_cells;
-		cell = &tscadc->cells[tscadc->used_cells++];
+		cell = &tscadc->cells[cell_idx++];
 		cell->name = tscadc->data->name_adc;
 		cell->of_compatible = tscadc->data->compat_adc;
 		cell->platform_data = &tscadc;
@@ -260,8 +254,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	}
 
 	err = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
-			      tscadc->cells, tscadc->used_cells, NULL,
-			      0, NULL);
+			      tscadc->cells, cell_idx, NULL, 0, NULL);
 	if (err < 0)
 		goto err_disable_clk;
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 0f581c15d95a..e734fb97dff8 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -177,10 +177,7 @@ struct ti_tscadc_dev {
 	phys_addr_t tscadc_phys_base;
 	const struct ti_tscadc_data *data;
 	int irq;
-	int used_cells;	/* 1-2 */
 	int tsc_wires;
-	int tsc_cell;	/* -1 if not used */
-	int adc_cell;	/* -1 if not used */
 	struct mfd_cell cells[TSCADC_CELLS];
 	u32 reg_se_cache;
 	bool adc_waiting;
-- 
2.27.0

[PATCH v3 20/47] mfd: ti_am335x_tscadc: Always provide an idle configuration

[Miquel Raynal]

The idle register is valid no matter if the touchscreen is used or not,
let's always configure it.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 8404fc662725..610853814024 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -221,16 +221,16 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-	/* Set register bits for Idle Config Mode */
 	if (tsc_wires > 0) {
 		tscadc->tsc_wires = tsc_wires;
 		if (tsc_wires == 5)
 			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
 		else
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
-		tscadc_idle_config(tscadc);
 	}
 
+	tscadc_idle_config(tscadc);
+
 	/* Enable the TSC module enable bit */
 	ctrl |= CNTRLREG_TSCSSENB;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
@@ -321,8 +321,10 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
 		else
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
-		tscadc_idle_config(tscadc);
 	}
+
+	tscadc_idle_config(tscadc);
+
 	ctrl |= CNTRLREG_TSCSSENB;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-- 
2.27.0

[PATCH v3 21/47] mfd: ti_am335x_tscadc: Reorder the initialization steps

[Miquel Raynal]

TI AM335X TRM [1] states that most of the configuration should be set in
the control register in the first place, before actually enabling the
hardware with the subsystem enable bit.

So far only half of the configuration was made in the first step (before
enabling the "subsystem"), which does not make really sense. Also, the
probe and the resume patch were acting differently. Let's harmonize all
this by following these steps:
1/ Configure the CLKDIV register
2/ Configure the CTRL register
3/ Configure the idle configuration
4/ Really enable the device by rewriting the CTRL register with the
   subsystem enable bit set.

[1] https://www.ti.com/lit/pdf/spruh73

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 610853814024..951734801139 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -219,8 +219,6 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	/* Set the control register bits */
 	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
 	if (tsc_wires > 0) {
 		tscadc->tsc_wires = tsc_wires;
 		if (tsc_wires == 5)
@@ -228,6 +226,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		else
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
 	}
+	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
 	tscadc_idle_config(tscadc);
 
@@ -313,9 +312,9 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 	pm_runtime_get_sync(dev);
 
 	/* context restore */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
+	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
 	if (tscadc->tsc_wires > 0) {
 		if (tscadc->tsc_wires == 5)
 			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
@@ -323,13 +322,13 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
 	}
 
+	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+
 	tscadc_idle_config(tscadc);
 
 	ctrl |= CNTRLREG_TSCSSENB;
 	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 
-	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
-
 	return 0;
 }
 
-- 
2.27.0

[PATCH v3 22/47] mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place

[Miquel Raynal]

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 (all but the subsystem enable bit) in the driver's
structure and use it from the resume callback.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 39 ++++++++++------------------
 include/linux/mfd/ti_am335x_tscadc.h |  2 +-
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 951734801139..f627405d16f0 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -121,7 +121,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct property *prop;
 	const __be32 *cur;
 	u32 val;
-	int err, ctrl;
+	int err;
 	int tsc_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
 	int readouts = 0;
 
@@ -217,22 +217,25 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	tscadc->clk_div = (clk_get_rate(clk) / tscadc->data->target_clk_rate) - 1;
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 
-	/* Set the control register bits */
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
+	/*
+	 * Set the control register bits. tscadc->ctrl stores the configuration
+	 * of the CTRL register but not the subsystem enable bit which must be
+	 * added manually when timely.
+	 */
+	tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID;
 	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;
 	}
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
 	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) {
@@ -307,27 +310,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 */
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
-
-	ctrl = CNTRLREG_STEPCONFIGWRT |	CNTRLREG_STEPID;
-	if (tscadc->tsc_wires > 0) {
-		if (tscadc->tsc_wires == 5)
-			ctrl |= CNTRLREG_5WIRE | CNTRLREG_TSCENB;
-		else
-			ctrl |= CNTRLREG_4WIRE | CNTRLREG_TSCENB;
-	}
-
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
-
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 	tscadc_idle_config(tscadc);
-
-	ctrl |= CNTRLREG_TSCSSENB;
-	regmap_write(tscadc->regmap, REG_CTRL, ctrl);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
 
 	return 0;
 }
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e734fb97dff8..02963b6ebbac 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -177,8 +177,8 @@ struct ti_tscadc_dev {
 	phys_addr_t tscadc_phys_base;
 	const struct ti_tscadc_data *data;
 	int irq;
-	int tsc_wires;
 	struct mfd_cell cells[TSCADC_CELLS];
+	u32 ctrl;
 	u32 reg_se_cache;
 	bool adc_waiting;
 	bool adc_in_use;
-- 
2.27.0

[PATCH v3 23/47] mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag

[Miquel Raynal]

Drop the text license and replace it with an equivalent SPDX license tag
identifier.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/mfd/ti_am335x_tscadc.h | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 02963b6ebbac..334ce1a879df 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -1,21 +1,13 @@
-#ifndef __LINUX_TI_AM335X_TSCADC_MFD_H
-#define __LINUX_TI_AM335X_TSCADC_MFD_H
-
+/* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * TI Touch Screen / ADC MFD driver
  *
  * Copyright (C) 2012 Texas Instruments Incorporated - https://www.ti.com/
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation version 2.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether express or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
+#ifndef __LINUX_TI_AM335X_TSCADC_MFD_H
+#define __LINUX_TI_AM335X_TSCADC_MFD_H
+
 #include <linux/mfd/core.h>
 
 #define REG_RAWIRQSTATUS	0x024
-- 
2.27.0

[PATCH v3 24/47] mfd: ti_am335x_tscadc: Fix header spacing

[Miquel Raynal]

Harmonize the spacing within macro definitions.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 include/linux/mfd/ti_am335x_tscadc.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 334ce1a879df..efafecfc87a7 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -41,7 +41,7 @@
 /* Step Enable */
 #define STEPENB_MASK		(0x1FFFF << 0)
 #define STEPENB(val)		((val) << 0)
-#define ENB(val)			(1 << (val))
+#define ENB(val)		(1 << (val))
 #define STPENB_STEPENB		STEPENB(0x1FFFF)
 #define STPENB_STEPENB_TC	STEPENB(0x1FFF)
 
@@ -122,15 +122,15 @@
 #define CNTRLREG_TSCENB		BIT(7)
 
 /* FIFO READ Register */
-#define FIFOREAD_DATA_MASK (0xfff << 0)
-#define FIFOREAD_CHNLID_MASK (0xf << 16)
+#define FIFOREAD_DATA_MASK	(0xfff << 0)
+#define FIFOREAD_CHNLID_MASK	(0xf << 16)
 
 /* DMA ENABLE/CLEAR Register */
 #define DMA_FIFO0		BIT(0)
 #define DMA_FIFO1		BIT(1)
 
 /* Sequencer Status */
-#define SEQ_STATUS BIT(5)
+#define SEQ_STATUS		BIT(5)
 #define CHARGE_STEP		0x11
 
 #define ADC_CLK			3000000
@@ -150,7 +150,7 @@
  *
  * max processing time: 266431 * 308ns = 83ms(approx)
  */
-#define IDLE_TIMEOUT 83 /* milliseconds */
+#define IDLE_TIMEOUT		83 /* milliseconds */
 
 #define TSCADC_CELLS		2
 
-- 
2.27.0

[PATCH v3 25/47] mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro

[Miquel Raynal]

Before adding another frequency with even more zeroes, use the
HZ_PER_MHZ macro to clarify the number.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mfd/ti_am335x_tscadc.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index efafecfc87a7..d98ec8937dd9 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -9,6 +9,7 @@
 #define __LINUX_TI_AM335X_TSCADC_MFD_H
 
 #include <linux/mfd/core.h>
+#include <linux/units.h>
 
 #define REG_RAWIRQSTATUS	0x024
 #define REG_IRQSTATUS		0x028
@@ -133,7 +134,7 @@
 #define SEQ_STATUS		BIT(5)
 #define CHARGE_STEP		0x11
 
-#define ADC_CLK			3000000
+#define ADC_CLK			(3 * HZ_PER_MHZ)
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 #define FIFO1_THRESHOLD		19
-- 
2.27.0

Re: [PATCH v3 25/47] mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro

[Jonathan Cameron]

On Wed, 15 Sep 2021 17:58:46 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Before adding another frequency with even more zeroes, use the
> HZ_PER_MHZ macro to clarify the number.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Great.
FWIW
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  include/linux/mfd/ti_am335x_tscadc.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index efafecfc87a7..d98ec8937dd9 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -9,6 +9,7 @@
>  #define __LINUX_TI_AM335X_TSCADC_MFD_H
>  
>  #include <linux/mfd/core.h>
> +#include <linux/units.h>
>  
>  #define REG_RAWIRQSTATUS	0x024
>  #define REG_IRQSTATUS		0x028
> @@ -133,7 +134,7 @@
>  #define SEQ_STATUS		BIT(5)
>  #define CHARGE_STEP		0x11
>  
> -#define ADC_CLK			3000000
> +#define ADC_CLK			(3 * HZ_PER_MHZ)
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
>  #define FIFO1_THRESHOLD		19

[PATCH v3 26/47] mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when relevant

[Miquel Raynal]

Clean the ti_am335x_tscadc.h header by:
* converting masks to GENMASK()
* converting regular shifts to BIT()
* using FIELD_PREP() when relevant

Sometimes reorder the lines to be able to use the relevant bitmask.

Mind the s/%d/%ld/ change in a log due to the type change following the
use of FIELD_PREP() in the header.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/ti_am335x_adc.c      |  2 +-
 include/linux/mfd/ti_am335x_tscadc.h | 71 ++++++++++++++--------------
 2 files changed, 37 insertions(+), 36 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 855cc2d64ac8..3dec115e68ee 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -126,7 +126,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 		chan = adc_dev->channel_line[i];
 
 		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
-			dev_warn(dev, "chan %d step_avg truncating to %d\n",
+			dev_warn(dev, "chan %d step_avg truncating to %ld\n",
 				 chan, STEPCONFIG_AVG_16);
 			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
 		}
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index d98ec8937dd9..e6fe623bb1aa 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -8,6 +8,7 @@
 #ifndef __LINUX_TI_AM335X_TSCADC_MFD_H
 #define __LINUX_TI_AM335X_TSCADC_MFD_H
 
+#include <linux/bitfield.h>
 #include <linux/mfd/core.h>
 #include <linux/units.h>
 
@@ -40,11 +41,11 @@
 #define IRQWKUP_ENB		BIT(0)
 
 /* Step Enable */
-#define STEPENB_MASK		(0x1FFFF << 0)
-#define STEPENB(val)		((val) << 0)
-#define ENB(val)		(1 << (val))
-#define STPENB_STEPENB		STEPENB(0x1FFFF)
-#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
+#define STEPENB_MASK		GENMASK(16, 0)
+#define STEPENB(val)		FIELD_PREP(STEPENB_MASK, (val))
+#define ENB(val)		BIT(val)
+#define STPENB_STEPENB		STEPENB(GENMASK(16, 0))
+#define STPENB_STEPENB_TC	STEPENB(GENMASK(12, 0))
 
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
@@ -58,12 +59,12 @@
 #define IRQENB_PENUP		BIT(9)
 
 /* Step Configuration */
-#define STEPCONFIG_MODE_MASK	(3 << 0)
-#define STEPCONFIG_MODE(val)	((val) << 0)
+#define STEPCONFIG_MODE_MASK	GENMASK(1, 0)
+#define STEPCONFIG_MODE(val)	FIELD_PREP(STEPCONFIG_MODE_MASK, (val))
 #define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
 #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
-#define STEPCONFIG_AVG_MASK	(7 << 2)
-#define STEPCONFIG_AVG(val)	((val) << 2)
+#define STEPCONFIG_AVG_MASK	GENMASK(4, 2)
+#define STEPCONFIG_AVG(val)	FIELD_PREP(STEPCONFIG_AVG_MASK, (val))
 #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
 #define STEPCONFIG_XPP		BIT(5)
 #define STEPCONFIG_XNN		BIT(6)
@@ -71,43 +72,43 @@
 #define STEPCONFIG_YNN		BIT(8)
 #define STEPCONFIG_XNP		BIT(9)
 #define STEPCONFIG_YPN		BIT(10)
-#define STEPCONFIG_RFP(val)	((val) << 12)
-#define STEPCONFIG_RFP_VREFP	(0x3 << 12)
-#define STEPCONFIG_INM_MASK	(0xF << 15)
-#define STEPCONFIG_INM(val)	((val) << 15)
+#define STEPCONFIG_RFP_VREFP	GENMASK(13, 12)
+#define STEPCONFIG_RFP(val)	FIELD_PREP(STEPCONFIG_RFP_VREFP, (val))
+#define STEPCONFIG_INM_MASK	GENMASK(18, 15)
+#define STEPCONFIG_INM(val)	FIELD_PREP(STEPCONFIG_INM_MASK, (val))
 #define STEPCONFIG_INM_ADCREFM	STEPCONFIG_INM(8)
-#define STEPCONFIG_INP_MASK	(0xF << 19)
-#define STEPCONFIG_INP(val)	((val) << 19)
+#define STEPCONFIG_INP_MASK	GENMASK(22, 19)
+#define STEPCONFIG_INP(val)	FIELD_PREP(STEPCONFIG_INP_MASK, (val))
 #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
 #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
 #define STEPCONFIG_FIFO1	BIT(26)
-#define STEPCONFIG_RFM(val)	((val) << 23)
-#define STEPCONFIG_RFM_VREFN	(0x3 << 23)
+#define STEPCONFIG_RFM_VREFN	GENMASK(24, 23)
+#define STEPCONFIG_RFM(val)	FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
 
 /* Delay register */
-#define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
-#define STEPDELAY_OPEN(val)	((val) << 0)
+#define STEPDELAY_OPEN_MASK	GENMASK(17, 0)
+#define STEPDELAY_OPEN(val)	FIELD_PREP(STEPDELAY_OPEN_MASK, (val))
 #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
-#define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
-#define STEPDELAY_SAMPLE(val)	((val) << 24)
+#define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
+#define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
 #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
 
 /* Charge Config */
-#define STEPCHARGE_RFP_MASK	(7 << 12)
-#define STEPCHARGE_RFP(val)	((val) << 12)
+#define STEPCHARGE_RFP_MASK	GENMASK(14, 12)
+#define STEPCHARGE_RFP(val)	FIELD_PREP(STEPCHARGE_RFP_MASK, (val))
 #define STEPCHARGE_RFP_XPUL	STEPCHARGE_RFP(1)
-#define STEPCHARGE_INM_MASK	(0xF << 15)
-#define STEPCHARGE_INM(val)	((val) << 15)
+#define STEPCHARGE_INM_MASK	GENMASK(18, 15)
+#define STEPCHARGE_INM(val)	FIELD_PREP(STEPCHARGE_INM_MASK, (val))
 #define STEPCHARGE_INM_AN1	STEPCHARGE_INM(1)
-#define STEPCHARGE_INP_MASK	(0xF << 19)
-#define STEPCHARGE_INP(val)	((val) << 19)
-#define STEPCHARGE_RFM_MASK	(3 << 23)
-#define STEPCHARGE_RFM(val)	((val) << 23)
+#define STEPCHARGE_INP_MASK	GENMASK(22, 19)
+#define STEPCHARGE_INP(val)	FIELD_PREP(STEPCHARGE_INP_MASK, (val))
+#define STEPCHARGE_RFM_MASK	GENMASK(24, 23)
+#define STEPCHARGE_RFM(val)	FIELD_PREP(STEPCHARGE_RFM_MASK, (val))
 #define STEPCHARGE_RFM_XNUR	STEPCHARGE_RFM(1)
 
 /* Charge delay */
-#define CHARGEDLY_OPEN_MASK	(0x3FFFF << 0)
-#define CHARGEDLY_OPEN(val)	((val) << 0)
+#define CHARGEDLY_OPEN_MASK	GENMASK(17, 0)
+#define CHARGEDLY_OPEN(val)	FIELD_PREP(CHARGEDLY_OPEN_MASK, (val))
 #define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0x400)
 
 /* Control register */
@@ -115,16 +116,16 @@
 #define CNTRLREG_STEPID		BIT(1)
 #define CNTRLREG_STEPCONFIGWRT	BIT(2)
 #define CNTRLREG_POWERDOWN	BIT(4)
-#define CNTRLREG_AFE_CTRL_MASK	(3 << 5)
-#define CNTRLREG_AFE_CTRL(val)	((val) << 5)
+#define CNTRLREG_AFE_CTRL_MASK	GENMASK(6, 5)
+#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val))
 #define CNTRLREG_4WIRE		CNTRLREG_AFE_CTRL(1)
 #define CNTRLREG_5WIRE		CNTRLREG_AFE_CTRL(2)
 #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
 #define CNTRLREG_TSCENB		BIT(7)
 
 /* FIFO READ Register */
-#define FIFOREAD_DATA_MASK	(0xfff << 0)
-#define FIFOREAD_CHNLID_MASK	(0xf << 16)
+#define FIFOREAD_DATA_MASK	GENMASK(11, 0)
+#define FIFOREAD_CHNLID_MASK	GENMASK(19, 16)
 
 /* DMA ENABLE/CLEAR Register */
 #define DMA_FIFO0		BIT(0)
-- 
2.27.0

Re: [PATCH v3 26/47] mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when relevant

[Jonathan Cameron]

On Wed, 15 Sep 2021 17:58:47 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Clean the ti_am335x_tscadc.h header by:
> * converting masks to GENMASK()
> * converting regular shifts to BIT()
> * using FIELD_PREP() when relevant
> 
> Sometimes reorder the lines to be able to use the relevant bitmask.
> 
> Mind the s/%d/%ld/ change in a log due to the type change following the
> use of FIELD_PREP() in the header.
> 
> Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

Looks good as an independent patch. However,a comment inline that it would
have been simpler / better to move the patch dropping definitions before this
one as then you would have less to rework.

That's particularly true of the ENB() definition which you change twice
before then dropping it entirely.

Anyhow, either as is (if Lee doesn't mind), or with whatever is left after
doing those drops earlier

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> ---
>  drivers/iio/adc/ti_am335x_adc.c      |  2 +-
>  include/linux/mfd/ti_am335x_tscadc.h | 71 ++++++++++++++--------------
>  2 files changed, 37 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 855cc2d64ac8..3dec115e68ee 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -126,7 +126,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  		chan = adc_dev->channel_line[i];
>  
>  		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
> -			dev_warn(dev, "chan %d step_avg truncating to %d\n",
> +			dev_warn(dev, "chan %d step_avg truncating to %ld\n",
>  				 chan, STEPCONFIG_AVG_16);
>  			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
>  		}
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index d98ec8937dd9..e6fe623bb1aa 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -8,6 +8,7 @@
>  #ifndef __LINUX_TI_AM335X_TSCADC_MFD_H
>  #define __LINUX_TI_AM335X_TSCADC_MFD_H
>  
> +#include <linux/bitfield.h>
>  #include <linux/mfd/core.h>
>  #include <linux/units.h>
>  
> @@ -40,11 +41,11 @@
>  #define IRQWKUP_ENB		BIT(0)
>  
>  /* Step Enable */
> -#define STEPENB_MASK		(0x1FFFF << 0)
> -#define STEPENB(val)		((val) << 0)
> -#define ENB(val)		(1 << (val))
> -#define STPENB_STEPENB		STEPENB(0x1FFFF)
> -#define STPENB_STEPENB_TC	STEPENB(0x1FFF)
> +#define STEPENB_MASK		GENMASK(16, 0)
> +#define STEPENB(val)		FIELD_PREP(STEPENB_MASK, (val))
> +#define ENB(val)		BIT(val)
> +#define STPENB_STEPENB		STEPENB(GENMASK(16, 0))
> +#define STPENB_STEPENB_TC	STEPENB(GENMASK(12, 0))

This particular block seems a bit odd so I went to try and understand how it is used.
It isn't...  And you drop it in a later patch.

It's not very important but if you are going to drop code do it before any of it
is touched in a series as cuts down on the amount reviewers need to look at!

>  
>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
> @@ -58,12 +59,12 @@
>  #define IRQENB_PENUP		BIT(9)
>  
>  /* Step Configuration */
> -#define STEPCONFIG_MODE_MASK	(3 << 0)
> -#define STEPCONFIG_MODE(val)	((val) << 0)
> +#define STEPCONFIG_MODE_MASK	GENMASK(1, 0)
> +#define STEPCONFIG_MODE(val)	FIELD_PREP(STEPCONFIG_MODE_MASK, (val))
>  #define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
>  #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
> -#define STEPCONFIG_AVG_MASK	(7 << 2)
> -#define STEPCONFIG_AVG(val)	((val) << 2)
> +#define STEPCONFIG_AVG_MASK	GENMASK(4, 2)
> +#define STEPCONFIG_AVG(val)	FIELD_PREP(STEPCONFIG_AVG_MASK, (val))
>  #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
>  #define STEPCONFIG_XPP		BIT(5)
>  #define STEPCONFIG_XNN		BIT(6)
> @@ -71,43 +72,43 @@
>  #define STEPCONFIG_YNN		BIT(8)
>  #define STEPCONFIG_XNP		BIT(9)
>  #define STEPCONFIG_YPN		BIT(10)
> -#define STEPCONFIG_RFP(val)	((val) << 12)
> -#define STEPCONFIG_RFP_VREFP	(0x3 << 12)
> -#define STEPCONFIG_INM_MASK	(0xF << 15)
> -#define STEPCONFIG_INM(val)	((val) << 15)
> +#define STEPCONFIG_RFP_VREFP	GENMASK(13, 12)
> +#define STEPCONFIG_RFP(val)	FIELD_PREP(STEPCONFIG_RFP_VREFP, (val))
> +#define STEPCONFIG_INM_MASK	GENMASK(18, 15)
> +#define STEPCONFIG_INM(val)	FIELD_PREP(STEPCONFIG_INM_MASK, (val))
>  #define STEPCONFIG_INM_ADCREFM	STEPCONFIG_INM(8)
> -#define STEPCONFIG_INP_MASK	(0xF << 19)
> -#define STEPCONFIG_INP(val)	((val) << 19)
> +#define STEPCONFIG_INP_MASK	GENMASK(22, 19)
> +#define STEPCONFIG_INP(val)	FIELD_PREP(STEPCONFIG_INP_MASK, (val))
>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>  #define STEPCONFIG_FIFO1	BIT(26)
> -#define STEPCONFIG_RFM(val)	((val) << 23)
> -#define STEPCONFIG_RFM_VREFN	(0x3 << 23)
> +#define STEPCONFIG_RFM_VREFN	GENMASK(24, 23)
> +#define STEPCONFIG_RFM(val)	FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
>  
>  /* Delay register */
> -#define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
> -#define STEPDELAY_OPEN(val)	((val) << 0)
> +#define STEPDELAY_OPEN_MASK	GENMASK(17, 0)
> +#define STEPDELAY_OPEN(val)	FIELD_PREP(STEPDELAY_OPEN_MASK, (val))
>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
> -#define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
> -#define STEPDELAY_SAMPLE(val)	((val) << 24)
> +#define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
> +#define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
>  
>  /* Charge Config */
> -#define STEPCHARGE_RFP_MASK	(7 << 12)
> -#define STEPCHARGE_RFP(val)	((val) << 12)
> +#define STEPCHARGE_RFP_MASK	GENMASK(14, 12)
> +#define STEPCHARGE_RFP(val)	FIELD_PREP(STEPCHARGE_RFP_MASK, (val))
>  #define STEPCHARGE_RFP_XPUL	STEPCHARGE_RFP(1)
> -#define STEPCHARGE_INM_MASK	(0xF << 15)
> -#define STEPCHARGE_INM(val)	((val) << 15)
> +#define STEPCHARGE_INM_MASK	GENMASK(18, 15)
> +#define STEPCHARGE_INM(val)	FIELD_PREP(STEPCHARGE_INM_MASK, (val))
>  #define STEPCHARGE_INM_AN1	STEPCHARGE_INM(1)
> -#define STEPCHARGE_INP_MASK	(0xF << 19)
> -#define STEPCHARGE_INP(val)	((val) << 19)
> -#define STEPCHARGE_RFM_MASK	(3 << 23)
> -#define STEPCHARGE_RFM(val)	((val) << 23)
> +#define STEPCHARGE_INP_MASK	GENMASK(22, 19)
> +#define STEPCHARGE_INP(val)	FIELD_PREP(STEPCHARGE_INP_MASK, (val))
> +#define STEPCHARGE_RFM_MASK	GENMASK(24, 23)
> +#define STEPCHARGE_RFM(val)	FIELD_PREP(STEPCHARGE_RFM_MASK, (val))
>  #define STEPCHARGE_RFM_XNUR	STEPCHARGE_RFM(1)
>  
>  /* Charge delay */
> -#define CHARGEDLY_OPEN_MASK	(0x3FFFF << 0)
> -#define CHARGEDLY_OPEN(val)	((val) << 0)
> +#define CHARGEDLY_OPEN_MASK	GENMASK(17, 0)
> +#define CHARGEDLY_OPEN(val)	FIELD_PREP(CHARGEDLY_OPEN_MASK, (val))
>  #define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0x400)
>  
>  /* Control register */
> @@ -115,16 +116,16 @@
>  #define CNTRLREG_STEPID		BIT(1)
>  #define CNTRLREG_STEPCONFIGWRT	BIT(2)
>  #define CNTRLREG_POWERDOWN	BIT(4)
> -#define CNTRLREG_AFE_CTRL_MASK	(3 << 5)
> -#define CNTRLREG_AFE_CTRL(val)	((val) << 5)
> +#define CNTRLREG_AFE_CTRL_MASK	GENMASK(6, 5)
> +#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val))
>  #define CNTRLREG_4WIRE		CNTRLREG_AFE_CTRL(1)
>  #define CNTRLREG_5WIRE		CNTRLREG_AFE_CTRL(2)
>  #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
>  #define CNTRLREG_TSCENB		BIT(7)
>  
>  /* FIFO READ Register */
> -#define FIFOREAD_DATA_MASK	(0xfff << 0)
> -#define FIFOREAD_CHNLID_MASK	(0xf << 16)
> +#define FIFOREAD_DATA_MASK	GENMASK(11, 0)
> +#define FIFOREAD_CHNLID_MASK	GENMASK(19, 16)
>  
>  /* DMA ENABLE/CLEAR Register */
>  #define DMA_FIFO0		BIT(0)

[PATCH v3 27/47] mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries

[Miquel Raynal]

Clearly define the maximum open delay and sample delay. Use these
definitions in place of a mask (which works because this is the first
field in the register) and an open-coded value. While at it reword a
little bit the error messages to make them look clearer and similar.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/iio/adc/ti_am335x_adc.c      | 18 +++++++++---------
 include/linux/mfd/ti_am335x_tscadc.h |  2 ++
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 3dec115e68ee..a241e6fa3564 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -126,7 +126,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 		chan = adc_dev->channel_line[i];
 
 		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
-			dev_warn(dev, "chan %d step_avg truncating to %ld\n",
+			dev_warn(dev, "chan %d: wrong step avg, truncated to %ld\n",
 				 chan, STEPCONFIG_AVG_16);
 			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
 		}
@@ -147,16 +147,16 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 				STEPCONFIG_RFP_VREFP |
 				STEPCONFIG_RFM_VREFN);
 
-		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK) {
-			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
-				 chan);
-			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
+		if (adc_dev->open_delay[i] > STEPCONFIG_MAX_OPENDLY) {
+			dev_warn(dev, "chan %d: wrong open delay, truncated to 0x%lX\n",
+				 chan, STEPCONFIG_MAX_OPENDLY);
+			adc_dev->open_delay[i] = STEPCONFIG_MAX_OPENDLY;
 		}
 
-		if (adc_dev->sample_delay[i] > 0xFF) {
-			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
-				 chan);
-			adc_dev->sample_delay[i] = 0xFF;
+		if (adc_dev->sample_delay[i] > STEPCONFIG_MAX_SAMPLE) {
+			dev_warn(dev, "chan %d: wrong sample delay, truncated to 0x%lX\n",
+				 chan, STEPCONFIG_MAX_SAMPLE);
+			adc_dev->sample_delay[i] = STEPCONFIG_MAX_SAMPLE;
 		}
 
 		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index e6fe623bb1aa..babc2e36c5d0 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -91,7 +91,9 @@
 #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
 #define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
 #define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
+#define STEPCONFIG_MAX_OPENDLY	GENMASK(17, 0)
 #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
+#define STEPCONFIG_MAX_SAMPLE	GENMASK(7, 0)
 
 /* Charge Config */
 #define STEPCHARGE_RFP_MASK	GENMASK(14, 12)
-- 
2.27.0

Re: [PATCH v3 27/47] mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries

[Jonathan Cameron]

On Wed, 15 Sep 2021 17:58:48 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Clearly define the maximum open delay and sample delay. Use these
> definitions in place of a mask (which works because this is the first
> field in the register) and an open-coded value. While at it reword a
> little bit the error messages to make them look clearer and similar.

I wouldn't bother explaining why the old method of using the mask happened
to work.  It confused me when reading this description :)

Otherwise, lgtm
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/adc/ti_am335x_adc.c      | 18 +++++++++---------
>  include/linux/mfd/ti_am335x_tscadc.h |  2 ++
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 3dec115e68ee..a241e6fa3564 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -126,7 +126,7 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  		chan = adc_dev->channel_line[i];
>  
>  		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
> -			dev_warn(dev, "chan %d step_avg truncating to %ld\n",
> +			dev_warn(dev, "chan %d: wrong step avg, truncated to %ld\n",
>  				 chan, STEPCONFIG_AVG_16);
>  			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
>  		}
> @@ -147,16 +147,16 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  				STEPCONFIG_RFP_VREFP |
>  				STEPCONFIG_RFM_VREFN);
>  
> -		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK) {
> -			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
> -				 chan);
> -			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
> +		if (adc_dev->open_delay[i] > STEPCONFIG_MAX_OPENDLY) {
> +			dev_warn(dev, "chan %d: wrong open delay, truncated to 0x%lX\n",
> +				 chan, STEPCONFIG_MAX_OPENDLY);
> +			adc_dev->open_delay[i] = STEPCONFIG_MAX_OPENDLY;
>  		}
>  
> -		if (adc_dev->sample_delay[i] > 0xFF) {
> -			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
> -				 chan);
> -			adc_dev->sample_delay[i] = 0xFF;
> +		if (adc_dev->sample_delay[i] > STEPCONFIG_MAX_SAMPLE) {
> +			dev_warn(dev, "chan %d: wrong sample delay, truncated to 0x%lX\n",
> +				 chan, STEPCONFIG_MAX_SAMPLE);
> +			adc_dev->sample_delay[i] = STEPCONFIG_MAX_SAMPLE;
>  		}
>  
>  		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index e6fe623bb1aa..babc2e36c5d0 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -91,7 +91,9 @@
>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
>  #define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
>  #define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
> +#define STEPCONFIG_MAX_OPENDLY	GENMASK(17, 0)
>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
> +#define STEPCONFIG_MAX_SAMPLE	GENMASK(7, 0)
>  
>  /* Charge Config */
>  #define STEPCHARGE_RFP_MASK	GENMASK(14, 12)

[PATCH v3 28/47] mfd: ti_am335x_tscadc: Drop useless definitions from the header

[Miquel Raynal]

Drop unused and useless definitions from the header. Besides the STEP
ENABLE register which is highly unclear (and not used), drop all the
"masks" definitions which are only used by the following definition. It
could be possible to got even further by removing these definitions
entirely and use FIELD_PREP() macros from the code directly, but while I
have no troubles making these changes in the header, changing the values
in the code directly could IMHO darkening a bit the logic and
furthermore hardening future git-blames.

Certain macros are using GENMASK() to define the value of a particular
field, while this is purely "by chance" that the value and the mask have
the same value. In this case, drop the "mask" definition, use
FIELD_PREP() and GENMASK() in the macro defining the field, and use the
new macro to define the particular value by feeding directly the actual
number advertised in the datasheet into that macro, as in:
	-#define STEPCONFIG_RFM_VREFN   GENMASK(24, 23)
	-#define STEPCONFIG_RFM(val)    FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
	+#define STEPCONFIG_RFM(val)    FIELD_PREP(GENMASK(24, 23), (val))
	+#define STEPCONFIG_RFM_VREFN   STEPCONFIG_RFM(3)

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 include/linux/mfd/ti_am335x_tscadc.h | 51 +++++++++-------------------
 1 file changed, 16 insertions(+), 35 deletions(-)

diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index babc2e36c5d0..32b26e56eebb 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -40,13 +40,6 @@
 /* IRQ wakeup enable */
 #define IRQWKUP_ENB		BIT(0)
 
-/* Step Enable */
-#define STEPENB_MASK		GENMASK(16, 0)
-#define STEPENB(val)		FIELD_PREP(STEPENB_MASK, (val))
-#define ENB(val)		BIT(val)
-#define STPENB_STEPENB		STEPENB(GENMASK(16, 0))
-#define STPENB_STEPENB_TC	STEPENB(GENMASK(12, 0))
-
 /* IRQ enable */
 #define IRQENB_HW_PEN		BIT(0)
 #define IRQENB_EOS		BIT(1)
@@ -59,12 +52,10 @@
 #define IRQENB_PENUP		BIT(9)
 
 /* Step Configuration */
-#define STEPCONFIG_MODE_MASK	GENMASK(1, 0)
-#define STEPCONFIG_MODE(val)	FIELD_PREP(STEPCONFIG_MODE_MASK, (val))
+#define STEPCONFIG_MODE(val)	FIELD_PREP(GENMASK(1, 0), (val))
 #define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
 #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
-#define STEPCONFIG_AVG_MASK	GENMASK(4, 2)
-#define STEPCONFIG_AVG(val)	FIELD_PREP(STEPCONFIG_AVG_MASK, (val))
+#define STEPCONFIG_AVG(val)	FIELD_PREP(GENMASK(4, 2), (val))
 #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
 #define STEPCONFIG_XPP		BIT(5)
 #define STEPCONFIG_XNN		BIT(6)
@@ -72,45 +63,36 @@
 #define STEPCONFIG_YNN		BIT(8)
 #define STEPCONFIG_XNP		BIT(9)
 #define STEPCONFIG_YPN		BIT(10)
-#define STEPCONFIG_RFP_VREFP	GENMASK(13, 12)
-#define STEPCONFIG_RFP(val)	FIELD_PREP(STEPCONFIG_RFP_VREFP, (val))
-#define STEPCONFIG_INM_MASK	GENMASK(18, 15)
-#define STEPCONFIG_INM(val)	FIELD_PREP(STEPCONFIG_INM_MASK, (val))
+#define STEPCONFIG_RFP(val)	FIELD_PREP(GENMASK(13, 12), (val))
+#define STEPCONFIG_RFP_VREFP	STEPCONFIG_RFP(3)
+#define STEPCONFIG_INM(val)	FIELD_PREP(GENMASK(18, 15), (val))
 #define STEPCONFIG_INM_ADCREFM	STEPCONFIG_INM(8)
-#define STEPCONFIG_INP_MASK	GENMASK(22, 19)
-#define STEPCONFIG_INP(val)	FIELD_PREP(STEPCONFIG_INP_MASK, (val))
+#define STEPCONFIG_INP(val)	FIELD_PREP(GENMASK(22, 19), (val))
 #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
 #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
 #define STEPCONFIG_FIFO1	BIT(26)
-#define STEPCONFIG_RFM_VREFN	GENMASK(24, 23)
-#define STEPCONFIG_RFM(val)	FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
+#define STEPCONFIG_RFM(val)	FIELD_PREP(GENMASK(24, 23), (val))
+#define STEPCONFIG_RFM_VREFN	STEPCONFIG_RFM(3)
 
 /* Delay register */
-#define STEPDELAY_OPEN_MASK	GENMASK(17, 0)
-#define STEPDELAY_OPEN(val)	FIELD_PREP(STEPDELAY_OPEN_MASK, (val))
+#define STEPDELAY_OPEN(val)	FIELD_PREP(GENMASK(17, 0), (val))
 #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
-#define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
-#define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
 #define STEPCONFIG_MAX_OPENDLY	GENMASK(17, 0)
+#define STEPDELAY_SAMPLE(val)	FIELD_PREP(GENMASK(31, 24), (val))
 #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
 #define STEPCONFIG_MAX_SAMPLE	GENMASK(7, 0)
 
 /* Charge Config */
-#define STEPCHARGE_RFP_MASK	GENMASK(14, 12)
-#define STEPCHARGE_RFP(val)	FIELD_PREP(STEPCHARGE_RFP_MASK, (val))
+#define STEPCHARGE_RFP(val)	FIELD_PREP(GENMASK(14, 12), (val))
 #define STEPCHARGE_RFP_XPUL	STEPCHARGE_RFP(1)
-#define STEPCHARGE_INM_MASK	GENMASK(18, 15)
-#define STEPCHARGE_INM(val)	FIELD_PREP(STEPCHARGE_INM_MASK, (val))
+#define STEPCHARGE_INM(val)	FIELD_PREP(GENMASK(18, 15), (val))
 #define STEPCHARGE_INM_AN1	STEPCHARGE_INM(1)
-#define STEPCHARGE_INP_MASK	GENMASK(22, 19)
-#define STEPCHARGE_INP(val)	FIELD_PREP(STEPCHARGE_INP_MASK, (val))
-#define STEPCHARGE_RFM_MASK	GENMASK(24, 23)
-#define STEPCHARGE_RFM(val)	FIELD_PREP(STEPCHARGE_RFM_MASK, (val))
+#define STEPCHARGE_INP(val)	FIELD_PREP(GENMASK(22, 19), (val))
+#define STEPCHARGE_RFM(val)	FIELD_PREP(GENMASK(24, 23), (val))
 #define STEPCHARGE_RFM_XNUR	STEPCHARGE_RFM(1)
 
 /* Charge delay */
-#define CHARGEDLY_OPEN_MASK	GENMASK(17, 0)
-#define CHARGEDLY_OPEN(val)	FIELD_PREP(CHARGEDLY_OPEN_MASK, (val))
+#define CHARGEDLY_OPEN(val)	FIELD_PREP(GENMASK(17, 0), (val))
 #define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0x400)
 
 /* Control register */
@@ -118,8 +100,7 @@
 #define CNTRLREG_STEPID		BIT(1)
 #define CNTRLREG_STEPCONFIGWRT	BIT(2)
 #define CNTRLREG_POWERDOWN	BIT(4)
-#define CNTRLREG_AFE_CTRL_MASK	GENMASK(6, 5)
-#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val))
+#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(GENMASK(6, 5), (val))
 #define CNTRLREG_4WIRE		CNTRLREG_AFE_CTRL(1)
 #define CNTRLREG_5WIRE		CNTRLREG_AFE_CTRL(2)
 #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
-- 
2.27.0

Re: [PATCH v3 28/47] mfd: ti_am335x_tscadc: Drop useless definitions from the header

[Jonathan Cameron]

On Wed, 15 Sep 2021 17:58:49 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Drop unused and useless definitions from the header. Besides the STEP
> ENABLE register which is highly unclear (and not used), drop all the

Agreed - I started trying to figure out what they were in the earlier patch!

> "masks" definitions which are only used by the following definition. It
> could be possible to got even further by removing these definitions
> entirely and use FIELD_PREP() macros from the code directly, but while I
> have no troubles making these changes in the header, changing the values
> in the code directly could IMHO darkening a bit the logic and
> furthermore hardening future git-blames.

Hmm. Maybe on that...  I'm not that bothered either way but there is
definitely clarity in FIELD_PREP being used inline for writes to a device.
You can very clearly see what is going on.

Note that it only really works here because the driver only ever uses
the masks to 'set' the value, but never to read any of them back from the
hardware.

Your point about it making a messy history is true of almost any change :)

> 
> Certain macros are using GENMASK() to define the value of a particular
> field, while this is purely "by chance" that the value and the mask have
> the same value. In this case, drop the "mask" definition, use
> FIELD_PREP() and GENMASK() in the macro defining the field, and use the
> new macro to define the particular value by feeding directly the actual
> number advertised in the datasheet into that macro, as in:
> 	-#define STEPCONFIG_RFM_VREFN   GENMASK(24, 23)
> 	-#define STEPCONFIG_RFM(val)    FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
> 	+#define STEPCONFIG_RFM(val)    FIELD_PREP(GENMASK(24, 23), (val))
> 	+#define STEPCONFIG_RFM_VREFN   STEPCONFIG_RFM(3)

This is indeed an improvement.

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

I'm a bit in two minds out about how you should handle the multiple patches
involved in cleaning these up.   Definitely not good to do modifications on
elements you are going to drop - so for those pull them out earlier.

The others are a little odd because you first introduce some of the GENMASK stuff
then rework it in this patch.  Perhaps this split is the best way to handle those.


Jonathan


> ---
>  include/linux/mfd/ti_am335x_tscadc.h | 51 +++++++++-------------------
>  1 file changed, 16 insertions(+), 35 deletions(-)
> 
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index babc2e36c5d0..32b26e56eebb 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -40,13 +40,6 @@
>  /* IRQ wakeup enable */
>  #define IRQWKUP_ENB		BIT(0)
>  
> -/* Step Enable */
> -#define STEPENB_MASK		GENMASK(16, 0)
> -#define STEPENB(val)		FIELD_PREP(STEPENB_MASK, (val))
> -#define ENB(val)		BIT(val)
> -#define STPENB_STEPENB		STEPENB(GENMASK(16, 0))
> -#define STPENB_STEPENB_TC	STEPENB(GENMASK(12, 0))
> -

With this first block moved much earlier in the series - to before
any of the other patches touch it.

Reviwed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

>  /* IRQ enable */
>  #define IRQENB_HW_PEN		BIT(0)
>  #define IRQENB_EOS		BIT(1)
> @@ -59,12 +52,10 @@
>  #define IRQENB_PENUP		BIT(9)
>  
>  /* Step Configuration */
> -#define STEPCONFIG_MODE_MASK	GENMASK(1, 0)
> -#define STEPCONFIG_MODE(val)	FIELD_PREP(STEPCONFIG_MODE_MASK, (val))
> +#define STEPCONFIG_MODE(val)	FIELD_PREP(GENMASK(1, 0), (val))
>  #define STEPCONFIG_MODE_SWCNT	STEPCONFIG_MODE(1)
>  #define STEPCONFIG_MODE_HWSYNC	STEPCONFIG_MODE(2)
> -#define STEPCONFIG_AVG_MASK	GENMASK(4, 2)
> -#define STEPCONFIG_AVG(val)	FIELD_PREP(STEPCONFIG_AVG_MASK, (val))
> +#define STEPCONFIG_AVG(val)	FIELD_PREP(GENMASK(4, 2), (val))
>  #define STEPCONFIG_AVG_16	STEPCONFIG_AVG(4)
>  #define STEPCONFIG_XPP		BIT(5)
>  #define STEPCONFIG_XNN		BIT(6)
> @@ -72,45 +63,36 @@
>  #define STEPCONFIG_YNN		BIT(8)
>  #define STEPCONFIG_XNP		BIT(9)
>  #define STEPCONFIG_YPN		BIT(10)
> -#define STEPCONFIG_RFP_VREFP	GENMASK(13, 12)
> -#define STEPCONFIG_RFP(val)	FIELD_PREP(STEPCONFIG_RFP_VREFP, (val))
> -#define STEPCONFIG_INM_MASK	GENMASK(18, 15)
> -#define STEPCONFIG_INM(val)	FIELD_PREP(STEPCONFIG_INM_MASK, (val))
> +#define STEPCONFIG_RFP(val)	FIELD_PREP(GENMASK(13, 12), (val))
> +#define STEPCONFIG_RFP_VREFP	STEPCONFIG_RFP(3)
> +#define STEPCONFIG_INM(val)	FIELD_PREP(GENMASK(18, 15), (val))
>  #define STEPCONFIG_INM_ADCREFM	STEPCONFIG_INM(8)
> -#define STEPCONFIG_INP_MASK	GENMASK(22, 19)
> -#define STEPCONFIG_INP(val)	FIELD_PREP(STEPCONFIG_INP_MASK, (val))
> +#define STEPCONFIG_INP(val)	FIELD_PREP(GENMASK(22, 19), (val))
>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>  #define STEPCONFIG_FIFO1	BIT(26)
> -#define STEPCONFIG_RFM_VREFN	GENMASK(24, 23)
> -#define STEPCONFIG_RFM(val)	FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
> +#define STEPCONFIG_RFM(val)	FIELD_PREP(GENMASK(24, 23), (val))
> +#define STEPCONFIG_RFM_VREFN	STEPCONFIG_RFM(3)
>  
>  /* Delay register */
> -#define STEPDELAY_OPEN_MASK	GENMASK(17, 0)
> -#define STEPDELAY_OPEN(val)	FIELD_PREP(STEPDELAY_OPEN_MASK, (val))
> +#define STEPDELAY_OPEN(val)	FIELD_PREP(GENMASK(17, 0), (val))
>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
> -#define STEPDELAY_SAMPLE_MASK	GENMASK(31, 24)
> -#define STEPDELAY_SAMPLE(val)	FIELD_PREP(STEPDELAY_SAMPLE_MASK, (val))
>  #define STEPCONFIG_MAX_OPENDLY	GENMASK(17, 0)
> +#define STEPDELAY_SAMPLE(val)	FIELD_PREP(GENMASK(31, 24), (val))
>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
>  #define STEPCONFIG_MAX_SAMPLE	GENMASK(7, 0)
>  
>  /* Charge Config */
> -#define STEPCHARGE_RFP_MASK	GENMASK(14, 12)
> -#define STEPCHARGE_RFP(val)	FIELD_PREP(STEPCHARGE_RFP_MASK, (val))
> +#define STEPCHARGE_RFP(val)	FIELD_PREP(GENMASK(14, 12), (val))
>  #define STEPCHARGE_RFP_XPUL	STEPCHARGE_RFP(1)
> -#define STEPCHARGE_INM_MASK	GENMASK(18, 15)
> -#define STEPCHARGE_INM(val)	FIELD_PREP(STEPCHARGE_INM_MASK, (val))
> +#define STEPCHARGE_INM(val)	FIELD_PREP(GENMASK(18, 15), (val))
>  #define STEPCHARGE_INM_AN1	STEPCHARGE_INM(1)
> -#define STEPCHARGE_INP_MASK	GENMASK(22, 19)
> -#define STEPCHARGE_INP(val)	FIELD_PREP(STEPCHARGE_INP_MASK, (val))
> -#define STEPCHARGE_RFM_MASK	GENMASK(24, 23)
> -#define STEPCHARGE_RFM(val)	FIELD_PREP(STEPCHARGE_RFM_MASK, (val))
> +#define STEPCHARGE_INP(val)	FIELD_PREP(GENMASK(22, 19), (val))
> +#define STEPCHARGE_RFM(val)	FIELD_PREP(GENMASK(24, 23), (val))
>  #define STEPCHARGE_RFM_XNUR	STEPCHARGE_RFM(1)
>  
>  /* Charge delay */
> -#define CHARGEDLY_OPEN_MASK	GENMASK(17, 0)
> -#define CHARGEDLY_OPEN(val)	FIELD_PREP(CHARGEDLY_OPEN_MASK, (val))
> +#define CHARGEDLY_OPEN(val)	FIELD_PREP(GENMASK(17, 0), (val))
>  #define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0x400)
>  
>  /* Control register */
> @@ -118,8 +100,7 @@
>  #define CNTRLREG_STEPID		BIT(1)
>  #define CNTRLREG_STEPCONFIGWRT	BIT(2)
>  #define CNTRLREG_POWERDOWN	BIT(4)
> -#define CNTRLREG_AFE_CTRL_MASK	GENMASK(6, 5)
> -#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(CNTRLREG_AFE_CTRL_MASK, (val))
> +#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(GENMASK(6, 5), (val))
>  #define CNTRLREG_4WIRE		CNTRLREG_AFE_CTRL(1)
>  #define CNTRLREG_5WIRE		CNTRLREG_AFE_CTRL(2)
>  #define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)

Re: [PATCH v3 28/47] mfd: ti_am335x_tscadc: Drop useless definitions from the header

[Miquel Raynal]

Hi Jonathan,

jic23@kernel.org wrote on Sat, 18 Sep 2021 17:31:54 +0100:

> On Wed, 15 Sep 2021 17:58:49 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > Drop unused and useless definitions from the header. Besides the STEP
> > ENABLE register which is highly unclear (and not used), drop all the  
> 
> Agreed - I started trying to figure out what they were in the earlier patch!
> 
> > "masks" definitions which are only used by the following definition. It
> > could be possible to got even further by removing these definitions
> > entirely and use FIELD_PREP() macros from the code directly, but while I
> > have no troubles making these changes in the header, changing the values
> > in the code directly could IMHO darkening a bit the logic and
> > furthermore hardening future git-blames.  
> 
> Hmm. Maybe on that...  I'm not that bothered either way but there is
> definitely clarity in FIELD_PREP being used inline for writes to a device.
> You can very clearly see what is going on.
> 
> Note that it only really works here because the driver only ever uses
> the masks to 'set' the value, but never to read any of them back from the
> hardware.
> 
> Your point about it making a messy history is true of almost any change :)
> 
> > 
> > Certain macros are using GENMASK() to define the value of a particular
> > field, while this is purely "by chance" that the value and the mask have
> > the same value. In this case, drop the "mask" definition, use
> > FIELD_PREP() and GENMASK() in the macro defining the field, and use the
> > new macro to define the particular value by feeding directly the actual
> > number advertised in the datasheet into that macro, as in:
> > 	-#define STEPCONFIG_RFM_VREFN   GENMASK(24, 23)
> > 	-#define STEPCONFIG_RFM(val)    FIELD_PREP(STEPCONFIG_RFM_VREFN, (val))
> > 	+#define STEPCONFIG_RFM(val)    FIELD_PREP(GENMASK(24, 23), (val))
> > 	+#define STEPCONFIG_RFM_VREFN   STEPCONFIG_RFM(3)  
> 
> This is indeed an improvement.
> 
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> I'm a bit in two minds out about how you should handle the multiple patches
> involved in cleaning these up.   Definitely not good to do modifications on
> elements you are going to drop - so for those pull them out earlier.
> 
> The others are a little odd because you first introduce some of the GENMASK stuff
> then rework it in this patch.  Perhaps this split is the best way to handle those.

I must admit I got lazy, the ordering reflects the order of my
decisions and once these made, it was too painful to rebase and move
this patch earlier but I fully understand the request :) I will ping Lee
in the cover letter to ask him what is his feedback over the entire
series and if he agrees with the main idea I whish I could only respin
these three patches in the right order in v4 and request him to take v3
for all the other patches.

Thanks,
Miquèl

[PATCH v3 29/47] mfd: ti_am335x_tscadc: Rename the subsystem enable macro

[Miquel Raynal]

This bit is common to all devices (ADC, Touchscreen, Magnetic reader) so
make it clear that it can be used from any location by operating a
mechanical rename:
s/CNTRLREG_TSCSSENB/CNTRLREG_SSENB/

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c      | 6 +++---
 drivers/mfd/ti_am335x_tscadc.c       | 6 +++---
 include/linux/mfd/ti_am335x_tscadc.h | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a241e6fa3564..1cc2efbb2875 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -184,7 +184,7 @@ static irqreturn_t tiadc_irq_h(int irq, void *private)
 	if (status & IRQENB_FIFO1OVRRUN) {
 		/* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
 		config = tiadc_readl(adc_dev, REG_CTRL);
-		config &= ~(CNTRLREG_TSCSSENB);
+		config &= ~(CNTRLREG_SSENB);
 		tiadc_writel(adc_dev, REG_CTRL, config);
 		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
 				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
@@ -197,7 +197,7 @@ static irqreturn_t tiadc_irq_h(int irq, void *private)
 			adc_fsm = tiadc_readl(adc_dev, REG_ADCFSM);
 		} while (adc_fsm != 0x10 && count++ < 100);
 
-		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
+		tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_SSENB));
 		return IRQ_HANDLED;
 	} else if (status & IRQENB_FIFO1THRES) {
 		/* Disable irq and wake worker thread */
@@ -671,7 +671,7 @@ static int __maybe_unused tiadc_suspend(struct device *dev)
 	unsigned int idle;
 
 	idle = tiadc_readl(adc_dev, REG_CTRL);
-	idle &= ~(CNTRLREG_TSCSSENB);
+	idle &= ~(CNTRLREG_SSENB);
 	tiadc_writel(adc_dev, REG_CTRL, (idle |
 			CNTRLREG_POWERDOWN));
 
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index f627405d16f0..53a6686a4c5d 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -235,7 +235,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	tscadc_idle_config(tscadc);
 
 	/* Enable the TSC module enable bit */
-	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
 
 	/* TSC Cell */
 	if (tsc_wires > 0) {
@@ -299,7 +299,7 @@ static int __maybe_unused tscadc_suspend(struct device *dev)
 
 		regmap_read(tscadc->regmap, REG_CTRL, &ctrl);
 		ctrl &= ~(CNTRLREG_POWERDOWN);
-		ctrl |= CNTRLREG_TSCSSENB;
+		ctrl |= CNTRLREG_SSENB;
 		regmap_write(tscadc->regmap, REG_CTRL, ctrl);
 	}
 	pm_runtime_put_sync(dev);
@@ -316,7 +316,7 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 	regmap_write(tscadc->regmap, REG_CLKDIV, tscadc->clk_div);
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 	tscadc_idle_config(tscadc);
-	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_TSCSSENB);
+	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
 
 	return 0;
 }
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 32b26e56eebb..5546a779e6ad 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -96,7 +96,7 @@
 #define CHARGEDLY_OPENDLY	CHARGEDLY_OPEN(0x400)
 
 /* Control register */
-#define CNTRLREG_TSCSSENB	BIT(0)
+#define CNTRLREG_SSENB		BIT(0)
 #define CNTRLREG_STEPID		BIT(1)
 #define CNTRLREG_STEPCONFIGWRT	BIT(2)
 #define CNTRLREG_POWERDOWN	BIT(4)
-- 
2.27.0

[PATCH v3 30/47] mfd: ti_am335x_tscadc: Add TSC prefix in certain macros

[Miquel Raynal]

While the register list (and names) between ADC0 and ADC1 are pretty
close, the bits inside changed a little bit. To avoid any future
confusion, let's add the TSC prefix when some bits are in a register
that is common to both revisions of the ADC, but are specific to the
am33xx hardware.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 10 +++++-----
 include/linux/mfd/ti_am335x_tscadc.h | 14 +++++++-------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 53a6686a4c5d..5242857993bc 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -222,13 +222,13 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * of the CTRL register but not the subsystem enable bit which must be
 	 * added manually when timely.
 	 */
-	tscadc->ctrl = CNTRLREG_STEPCONFIGWRT | CNTRLREG_STEPID;
+	tscadc->ctrl = CNTRLREG_TSC_STEPCONFIGWRT | CNTRLREG_STEPID;
 	if (tsc_wires > 0) {
-		tscadc->ctrl |= CNTRLREG_TSCENB;
+		tscadc->ctrl |= CNTRLREG_TSC_ENB;
 		if (tsc_wires == 5)
-			tscadc->ctrl |= CNTRLREG_5WIRE;
+			tscadc->ctrl |= CNTRLREG_TSC_5WIRE;
 		else
-			tscadc->ctrl |= CNTRLREG_4WIRE;
+			tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
 	}
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
@@ -328,7 +328,7 @@ static const struct ti_tscadc_data tscdata = {
 	.compat_tscmag = "ti,am3359-tsc",
 	.name_adc = "TI-am335x-adc",
 	.compat_adc = "ti,am3359-adc",
-	.target_clk_rate = ADC_CLK,
+	.target_clk_rate = TSC_ADC_CLK,
 };
 
 static const struct of_device_id ti_tscadc_dt_ids[] = {
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 5546a779e6ad..26f3055635ac 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -98,13 +98,13 @@
 /* Control register */
 #define CNTRLREG_SSENB		BIT(0)
 #define CNTRLREG_STEPID		BIT(1)
-#define CNTRLREG_STEPCONFIGWRT	BIT(2)
+#define CNTRLREG_TSC_STEPCONFIGWRT BIT(2)
 #define CNTRLREG_POWERDOWN	BIT(4)
-#define CNTRLREG_AFE_CTRL(val)	FIELD_PREP(GENMASK(6, 5), (val))
-#define CNTRLREG_4WIRE		CNTRLREG_AFE_CTRL(1)
-#define CNTRLREG_5WIRE		CNTRLREG_AFE_CTRL(2)
-#define CNTRLREG_8WIRE		CNTRLREG_AFE_CTRL(3)
-#define CNTRLREG_TSCENB		BIT(7)
+#define CNTRLREG_TSC_AFE_CTRL(val) FIELD_PREP(GENMASK(6, 5), (val))
+#define CNTRLREG_TSC_4WIRE	CNTRLREG_TSC_AFE_CTRL(1)
+#define CNTRLREG_TSC_5WIRE	CNTRLREG_TSC_AFE_CTRL(2)
+#define CNTRLREG_TSC_8WIRE	CNTRLREG_TSC_AFE_CTRL(3)
+#define CNTRLREG_TSC_ENB	BIT(7)
 
 /* FIFO READ Register */
 #define FIFOREAD_DATA_MASK	GENMASK(11, 0)
@@ -118,7 +118,7 @@
 #define SEQ_STATUS		BIT(5)
 #define CHARGE_STEP		0x11
 
-#define ADC_CLK			(3 * HZ_PER_MHZ)
+#define TSC_ADC_CLK		(3 * HZ_PER_MHZ)
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 #define FIFO1_THRESHOLD		19
-- 
2.27.0

[PATCH v3 31/47] mfd: ti_am335x_tscadc: Rename a variable

[Miquel Raynal]

We need to retrieve the number of wires used by the "secondary" device
(the touchscreen or the magnetic reader). Let's rename tsc_wires to
become tscmag_wires to clarify the fact that this variable can be used
in both situations.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 5242857993bc..7ab3a8328861 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)
 	const __be32 *cur;
 	u32 val;
 	int err;
-	int tsc_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
+	int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
 	int readouts = 0;
 
 	/* Allocate memory for device */
@@ -140,7 +140,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	tscadc->data = of_device_get_match_data(&pdev->dev);
 
 	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
-	of_property_read_u32(node, "ti,wires", &tsc_wires);
+	of_property_read_u32(node, "ti,wires", &tscmag_wires);
 	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
 	of_node_put(node);
 
@@ -157,7 +157,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	of_node_put(node);
 
-	total_channels = tsc_wires + adc_channels;
+	total_channels = tscmag_wires + adc_channels;
 	if (total_channels > 8) {
 		dev_err(&pdev->dev, "Number of i/p channels more than 8\n");
 		return -EINVAL;
@@ -223,9 +223,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * added manually when timely.
 	 */
 	tscadc->ctrl = CNTRLREG_TSC_STEPCONFIGWRT | CNTRLREG_STEPID;
-	if (tsc_wires > 0) {
+	if (tscmag_wires > 0) {
 		tscadc->ctrl |= CNTRLREG_TSC_ENB;
-		if (tsc_wires == 5)
+		if (tscmag_wires == 5)
 			tscadc->ctrl |= CNTRLREG_TSC_5WIRE;
 		else
 			tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
@@ -238,7 +238,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
 
 	/* TSC Cell */
-	if (tsc_wires > 0) {
+	if (tscmag_wires > 0) {
 		cell = &tscadc->cells[cell_idx++];
 		cell->name = tscadc->data->name_tscmag;
 		cell->of_compatible = tscadc->data->compat_tscmag;
-- 
2.27.0

[PATCH v3 32/47] mfd: ti_am335x_tscadc: Fix an error message

[Miquel Raynal]

The error message if we cannot retrieve the clock tells us that the
touchscreen controller clock was unavailable. This is wrong, this is the
"main" clock for the hardware block, it is not specific to the
touchscreen and won't change when we will introduce ADC1/magnetic reader
support so let's correct this comment.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 7ab3a8328861..a181b5b07012 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -209,7 +209,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 */
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
-		dev_err(&pdev->dev, "failed to get TSC fck\n");
+		dev_err(&pdev->dev, "failed to get fck\n");
 		err = PTR_ERR(clk);
 		goto err_disable_clk;
 	}
-- 
2.27.0

[PATCH v3 33/47] mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a touchscreen

[Miquel Raynal]

Just checking the number of wires will soon not be enough, add a boolean
to indicated the actual use or not of the touchscreen.

Certain checks only make sense when there is a touchscreen wired. Make
these checks explicitly depend on the presence of the touchscreen.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index a181b5b07012..ab6c0e82d389 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -120,6 +120,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct mfd_cell *cell;
 	struct property *prop;
 	const __be32 *cur;
+	bool use_tsc = false;
 	u32 val;
 	int err;
 	int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
@@ -143,6 +144,8 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	of_property_read_u32(node, "ti,wires", &tscmag_wires);
 	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
 	of_node_put(node);
+	if (tscmag_wires)
+		use_tsc = true;
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
@@ -168,7 +171,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	if (readouts * 2 + 2 + adc_channels > 16) {
+	if (use_tsc && (readouts * 2 + 2 + adc_channels > 16)) {
 		dev_err(&pdev->dev, "Too many step configurations requested\n");
 		return -EINVAL;
 	}
@@ -223,7 +226,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * added manually when timely.
 	 */
 	tscadc->ctrl = CNTRLREG_TSC_STEPCONFIGWRT | CNTRLREG_STEPID;
-	if (tscmag_wires > 0) {
+	if (use_tsc) {
 		tscadc->ctrl |= CNTRLREG_TSC_ENB;
 		if (tscmag_wires == 5)
 			tscadc->ctrl |= CNTRLREG_TSC_5WIRE;
-- 
2.27.0

[PATCH v3 34/47] mfd: ti_am335x_tscadc: Introduce has_tsc

[Miquel Raynal]

One way of knowing which hardware we are dealing with is to check its
compatible. When this must be done at several places, it's best to use a
proper boolean for that. ->has_tsc indicates if there is a touchscreen
controller available (meaning it's a am33xx-like ADC) or not but does
not indicate if it is actually used (that is the purpose of the use_tsc
boolean in the probe).

Introducing this boolean helps making a difference in the code between
what is generic to both types of ADCs and what is specific to the am33xx
hardware before introducing support for the am437x hardware.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 37 +++++++++++++++++-----------
 include/linux/mfd/ti_am335x_tscadc.h |  1 +
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index ab6c0e82d389..65c0fa735a71 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -105,8 +105,9 @@ static void tscadc_idle_config(struct ti_tscadc_dev *tscadc)
 {
 	unsigned int idleconfig;
 
-	idleconfig = STEPCONFIG_YNN | STEPCONFIG_INM_ADCREFM |
-			STEPCONFIG_INP_ADCREFM | STEPCONFIG_YPN;
+	idleconfig = STEPCONFIG_INM_ADCREFM | STEPCONFIG_INP_ADCREFM;
+	if (tscadc->data->has_tsc)
+		idleconfig |= STEPCONFIG_YNN | STEPCONFIG_YPN;
 
 	regmap_write(tscadc->regmap, REG_IDLECONFIG, idleconfig);
 }
@@ -140,12 +141,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 
 	tscadc->data = of_device_get_match_data(&pdev->dev);
 
-	node = of_get_child_by_name(pdev->dev.of_node, "tsc");
-	of_property_read_u32(node, "ti,wires", &tscmag_wires);
-	of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
-	of_node_put(node);
-	if (tscmag_wires)
-		use_tsc = true;
+	if (tscadc->data->has_tsc) {
+		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
+		of_property_read_u32(node, "ti,wires", &tscmag_wires);
+		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
+		of_node_put(node);
+		if (tscmag_wires)
+			use_tsc = true;
+	}
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
@@ -225,13 +228,16 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * of the CTRL register but not the subsystem enable bit which must be
 	 * added manually when timely.
 	 */
-	tscadc->ctrl = CNTRLREG_TSC_STEPCONFIGWRT | CNTRLREG_STEPID;
-	if (use_tsc) {
-		tscadc->ctrl |= CNTRLREG_TSC_ENB;
-		if (tscmag_wires == 5)
-			tscadc->ctrl |= CNTRLREG_TSC_5WIRE;
-		else
-			tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
+	tscadc->ctrl = CNTRLREG_STEPID;
+	if (tscadc->data->has_tsc) {
+		tscadc->ctrl |= CNTRLREG_TSC_STEPCONFIGWRT;
+		if (use_tsc) {
+			tscadc->ctrl |= CNTRLREG_TSC_ENB;
+			if (tscmag_wires == 5)
+				tscadc->ctrl |= CNTRLREG_TSC_5WIRE;
+			else
+				tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
+		}
 	}
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
@@ -327,6 +333,7 @@ static int __maybe_unused tscadc_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
 
 static const struct ti_tscadc_data tscdata = {
+	.has_tsc = true,
 	.name_tscmag = "TI-am335x-tsc",
 	.compat_tscmag = "ti,am3359-tsc",
 	.name_adc = "TI-am335x-adc",
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 26f3055635ac..467b3bec4335 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -140,6 +140,7 @@
 #define TSCADC_CELLS		2
 
 struct ti_tscadc_data {
+	bool has_tsc;
 	char *name_tscmag;
 	char *compat_tscmag;
 	char *name_adc;
-- 
2.27.0

[PATCH v3 35/47] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support

[Miquel Raynal]

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.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 41 ++++++++++++++++++++++++----
 include/linux/mfd/ti_am335x_tscadc.h |  7 +++++
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 65c0fa735a71..155a8ed879b3 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -121,11 +121,11 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	struct mfd_cell *cell;
 	struct property *prop;
 	const __be32 *cur;
-	bool use_tsc = false;
+	bool use_tsc = false, use_mag = false;
 	u32 val;
 	int err;
 	int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
-	int readouts = 0;
+	int readouts = 0, mag_tracks = 0;
 
 	/* Allocate memory for device */
 	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
@@ -148,6 +148,16 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 		of_node_put(node);
 		if (tscmag_wires)
 			use_tsc = true;
+	} else {
+		/*
+		 * When adding support for the magnetic reader, here is the
+		 * place to look for the number of tracks used from device tree.
+		 * Let's default to 0 for now.
+		 */
+		mag_tracks = 0;
+		tscmag_wires = mag_tracks * 2;
+		if (tscmag_wires)
+			use_mag = true;
 	}
 
 	node = of_get_child_by_name(pdev->dev.of_node, "adc");
@@ -209,8 +219,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	 * The TSC_ADC_Subsystem has 2 clock domains: OCP_CLK and ADC_CLK.
 	 * ADCs produce a 12-bit sample every 15 ADC_CLK cycles.
 	 * am33xx ADCs expect to capture 200ksps.
-	 * We need the ADC clocks to run at 3MHz.
-	 * This frequency is valid since TSC_ADC_SS controller design
+	 * am47xx ADCs expect to capture 867ksps.
+	 * We need ADC clocks respectively running at 3MHz and 13MHz.
+	 * These frequencies are valid since TSC_ADC_SS controller design
 	 * assumes the OCP clock is at least 6x faster than the ADC clock.
 	 */
 	clk = devm_clk_get(&pdev->dev, NULL);
@@ -238,6 +249,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 			else
 				tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
 		}
+	} else {
+		tscadc->ctrl |= CNTRLREG_MAG_PREAMP_PWRDOWN |
+				CNTRLREG_MAG_PREAMP_BYPASS;
 	}
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
 
@@ -246,7 +260,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	/* Enable the TSC module enable bit */
 	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
 
-	/* TSC Cell */
+	/* TSC or MAG Cell */
 	if (tscmag_wires > 0) {
 		cell = &tscadc->cells[cell_idx++];
 		cell->name = tscadc->data->name_tscmag;
@@ -334,6 +348,7 @@ static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
 
 static const struct ti_tscadc_data tscdata = {
 	.has_tsc = true,
+	.has_mag = false,
 	.name_tscmag = "TI-am335x-tsc",
 	.compat_tscmag = "ti,am3359-tsc",
 	.name_adc = "TI-am335x-adc",
@@ -341,11 +356,25 @@ static const struct ti_tscadc_data tscdata = {
 	.target_clk_rate = TSC_ADC_CLK,
 };
 
+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",
 		.data = &tscdata,
 	},
+	{
+		.compatible = "ti,am4372-magadc",
+		.data = &magdata,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
@@ -363,6 +392,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 467b3bec4335..9a412e6d764b 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -106,6 +106,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	GENMASK(11, 0)
 #define FIFOREAD_CHNLID_MASK	GENMASK(19, 16)
@@ -119,6 +124,7 @@
 #define CHARGE_STEP		0x11
 
 #define TSC_ADC_CLK		(3 * HZ_PER_MHZ)
+#define MAG_ADC_CLK		(13 * HZ_PER_MHZ)
 #define TOTAL_STEPS		16
 #define TOTAL_CHANNELS		8
 #define FIFO1_THRESHOLD		19
@@ -141,6 +147,7 @@
 
 struct ti_tscadc_data {
 	bool has_tsc;
+	bool has_mag;
 	char *name_tscmag;
 	char *compat_tscmag;
 	char *name_adc;
-- 
2.27.0

Re: [PATCH v3 35/47] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support

[Lee Jones]

On Wed, 15 Sep 2021, Miquel Raynal wrote:

> 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.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c       | 41 ++++++++++++++++++++++++----
>  include/linux/mfd/ti_am335x_tscadc.h |  7 +++++
>  2 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 65c0fa735a71..155a8ed879b3 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -121,11 +121,11 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	struct mfd_cell *cell;
>  	struct property *prop;
>  	const __be32 *cur;
> -	bool use_tsc = false;
> +	bool use_tsc = false, use_mag = false;
>  	u32 val;
>  	int err;
>  	int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
> -	int readouts = 0;
> +	int readouts = 0, mag_tracks = 0;
>  
>  	/* Allocate memory for device */
>  	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
> @@ -148,6 +148,16 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  		of_node_put(node);
>  		if (tscmag_wires)
>  			use_tsc = true;
> +	} else {
> +		/*
> +		 * When adding support for the magnetic reader, here is the
> +		 * place to look for the number of tracks used from device tree.
> +		 * Let's default to 0 for now.
> +		 */
> +		mag_tracks = 0;
> +		tscmag_wires = mag_tracks * 2;
> +		if (tscmag_wires)
> +			use_mag = true;
>  	}
>  
>  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
> @@ -209,8 +219,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	 * The TSC_ADC_Subsystem has 2 clock domains: OCP_CLK and ADC_CLK.
>  	 * ADCs produce a 12-bit sample every 15 ADC_CLK cycles.
>  	 * am33xx ADCs expect to capture 200ksps.
> -	 * We need the ADC clocks to run at 3MHz.
> -	 * This frequency is valid since TSC_ADC_SS controller design
> +	 * am47xx ADCs expect to capture 867ksps.
> +	 * We need ADC clocks respectively running at 3MHz and 13MHz.
> +	 * These frequencies are valid since TSC_ADC_SS controller design
>  	 * assumes the OCP clock is at least 6x faster than the ADC clock.
>  	 */
>  	clk = devm_clk_get(&pdev->dev, NULL);
> @@ -238,6 +249,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  			else
>  				tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
>  		}
> +	} else {
> +		tscadc->ctrl |= CNTRLREG_MAG_PREAMP_PWRDOWN |
> +				CNTRLREG_MAG_PREAMP_BYPASS;
>  	}
>  	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
>  
> @@ -246,7 +260,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	/* Enable the TSC module enable bit */
>  	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
>  
> -	/* TSC Cell */
> +	/* TSC or MAG Cell */
>  	if (tscmag_wires > 0) {
>  		cell = &tscadc->cells[cell_idx++];
>  		cell->name = tscadc->data->name_tscmag;
> @@ -334,6 +348,7 @@ static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
>  
>  static const struct ti_tscadc_data tscdata = {
>  	.has_tsc = true,
> +	.has_mag = false,
>  	.name_tscmag = "TI-am335x-tsc",
>  	.compat_tscmag = "ti,am3359-tsc",
>  	.name_adc = "TI-am335x-adc",
> @@ -341,11 +356,25 @@ static const struct ti_tscadc_data tscdata = {
>  	.target_clk_rate = TSC_ADC_CLK,
>  };
>  
> +static const struct ti_tscadc_data magdata = {
> +	.has_tsc = false,
> +	.has_mag = true,

Why 2 bools?

Better to have a single u8:

  MODE_TOUCHSCREEN
  MODE_MAG_STRING
  MODE_NONE

> +	.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",
>  		.data = &tscdata,
>  	},
> +	{
> +		.compatible = "ti,am4372-magadc",
> +		.data = &magdata,
> +	},
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
> @@ -363,6 +392,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");

Magnetic what?  Tape, disk, stripe?

>  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 467b3bec4335..9a412e6d764b 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -106,6 +106,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	GENMASK(11, 0)
>  #define FIFOREAD_CHNLID_MASK	GENMASK(19, 16)
> @@ -119,6 +124,7 @@
>  #define CHARGE_STEP		0x11
>  
>  #define TSC_ADC_CLK		(3 * HZ_PER_MHZ)
> +#define MAG_ADC_CLK		(13 * HZ_PER_MHZ)
>  #define TOTAL_STEPS		16
>  #define TOTAL_CHANNELS		8
>  #define FIFO1_THRESHOLD		19
> @@ -141,6 +147,7 @@
>  
>  struct ti_tscadc_data {
>  	bool has_tsc;
> +	bool has_mag;
>  	char *name_tscmag;
>  	char *compat_tscmag;
>  	char *name_adc;

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 35/47] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Wed, 22 Sep 2021 17:00:20 +0100:

> On Wed, 15 Sep 2021, Miquel Raynal wrote:
> 
> > 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.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/mfd/ti_am335x_tscadc.c       | 41 ++++++++++++++++++++++++----
> >  include/linux/mfd/ti_am335x_tscadc.h |  7 +++++
> >  2 files changed, 42 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > index 65c0fa735a71..155a8ed879b3 100644
> > --- a/drivers/mfd/ti_am335x_tscadc.c
> > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > @@ -121,11 +121,11 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	struct mfd_cell *cell;
> >  	struct property *prop;
> >  	const __be32 *cur;
> > -	bool use_tsc = false;
> > +	bool use_tsc = false, use_mag = false;
> >  	u32 val;
> >  	int err;
> >  	int tscmag_wires = 0, adc_channels = 0, cell_idx = 0, total_channels;
> > -	int readouts = 0;
> > +	int readouts = 0, mag_tracks = 0;
> >  
> >  	/* Allocate memory for device */
> >  	tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
> > @@ -148,6 +148,16 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  		of_node_put(node);
> >  		if (tscmag_wires)
> >  			use_tsc = true;
> > +	} else {
> > +		/*
> > +		 * When adding support for the magnetic reader, here is the
> > +		 * place to look for the number of tracks used from device tree.
> > +		 * Let's default to 0 for now.
> > +		 */
> > +		mag_tracks = 0;
> > +		tscmag_wires = mag_tracks * 2;
> > +		if (tscmag_wires)
> > +			use_mag = true;
> >  	}
> >  
> >  	node = of_get_child_by_name(pdev->dev.of_node, "adc");
> > @@ -209,8 +219,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	 * The TSC_ADC_Subsystem has 2 clock domains: OCP_CLK and ADC_CLK.
> >  	 * ADCs produce a 12-bit sample every 15 ADC_CLK cycles.
> >  	 * am33xx ADCs expect to capture 200ksps.
> > -	 * We need the ADC clocks to run at 3MHz.
> > -	 * This frequency is valid since TSC_ADC_SS controller design
> > +	 * am47xx ADCs expect to capture 867ksps.
> > +	 * We need ADC clocks respectively running at 3MHz and 13MHz.
> > +	 * These frequencies are valid since TSC_ADC_SS controller design
> >  	 * assumes the OCP clock is at least 6x faster than the ADC clock.
> >  	 */
> >  	clk = devm_clk_get(&pdev->dev, NULL);
> > @@ -238,6 +249,9 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  			else
> >  				tscadc->ctrl |= CNTRLREG_TSC_4WIRE;
> >  		}
> > +	} else {
> > +		tscadc->ctrl |= CNTRLREG_MAG_PREAMP_PWRDOWN |
> > +				CNTRLREG_MAG_PREAMP_BYPASS;
> >  	}
> >  	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl);
> >  
> > @@ -246,7 +260,7 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	/* Enable the TSC module enable bit */
> >  	regmap_write(tscadc->regmap, REG_CTRL, tscadc->ctrl | CNTRLREG_SSENB);
> >  
> > -	/* TSC Cell */
> > +	/* TSC or MAG Cell */
> >  	if (tscmag_wires > 0) {
> >  		cell = &tscadc->cells[cell_idx++];
> >  		cell->name = tscadc->data->name_tscmag;
> > @@ -334,6 +348,7 @@ static SIMPLE_DEV_PM_OPS(tscadc_pm_ops, tscadc_suspend, tscadc_resume);
> >  
> >  static const struct ti_tscadc_data tscdata = {
> >  	.has_tsc = true,
> > +	.has_mag = false,
> >  	.name_tscmag = "TI-am335x-tsc",
> >  	.compat_tscmag = "ti,am3359-tsc",
> >  	.name_adc = "TI-am335x-adc",
> > @@ -341,11 +356,25 @@ static const struct ti_tscadc_data tscdata = {
> >  	.target_clk_rate = TSC_ADC_CLK,
> >  };
> >  
> > +static const struct ti_tscadc_data magdata = {
> > +	.has_tsc = false,
> > +	.has_mag = true,  
> 
> Why 2 bools?
> 
> Better to have a single u8:
> 
>   MODE_TOUCHSCREEN
>   MODE_MAG_STRING
>   MODE_NONE

It's not a mode like "please use this feature", it is more like a
feature that is present and we need to know which one it is.

Anyway, I can use an enum for this, fine.

> 
> > +	.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",
> >  		.data = &tscdata,
> >  	},
> > +	{
> > +		.compatible = "ti,am4372-magadc",
> > +		.data = &magdata,
> > +	},
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, ti_tscadc_dt_ids);
> > @@ -363,6 +392,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");  
> 
> Magnetic what?  Tape, disk, stripe?

I guess my current knowledge of English tricked me as I thought
"magnetic reader" was clear enough for everybody, but I'll make the
commit logs and this module description clearer by specifying the
"stripe" keyword, sure.
 
> 
> >  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 467b3bec4335..9a412e6d764b 100644
> > --- a/include/linux/mfd/ti_am335x_tscadc.h
> > +++ b/include/linux/mfd/ti_am335x_tscadc.h
> > @@ -106,6 +106,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	GENMASK(11, 0)
> >  #define FIFOREAD_CHNLID_MASK	GENMASK(19, 16)
> > @@ -119,6 +124,7 @@
> >  #define CHARGE_STEP		0x11
> >  
> >  #define TSC_ADC_CLK		(3 * HZ_PER_MHZ)
> > +#define MAG_ADC_CLK		(13 * HZ_PER_MHZ)
> >  #define TOTAL_STEPS		16
> >  #define TOTAL_CHANNELS		8
> >  #define FIFO1_THRESHOLD		19
> > @@ -141,6 +147,7 @@
> >  
> >  struct ti_tscadc_data {
> >  	bool has_tsc;
> > +	bool has_mag;
> >  	char *name_tscmag;
> >  	char *compat_tscmag;
> >  	char *name_adc;  
> 


Thanks,
Miquèl

Re: [PATCH v3 35/47] mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support

[Miquel Raynal]

Hi Lee,

> >  static const struct ti_tscadc_data tscdata = {
> >  	.has_tsc = true,
> > +	.has_mag = false,
> >  	.name_tscmag = "TI-am335x-tsc",
> >  	.compat_tscmag = "ti,am3359-tsc",
> >  	.name_adc = "TI-am335x-adc",
> > @@ -341,11 +356,25 @@ static const struct ti_tscadc_data tscdata = {
> >  	.target_clk_rate = TSC_ADC_CLK,
> >  };
> >  
> > +static const struct ti_tscadc_data magdata = {
> > +	.has_tsc = false,
> > +	.has_mag = true,  
> 
> Why 2 bools?
> 
> Better to have a single u8:
> 
>   MODE_TOUCHSCREEN
>   MODE_MAG_STRING
>   MODE_NONE

I was about to add an enum for these but actually it is totally
redundant with the compatible name. Instead, I have added a
helper which checks the compatible string and gives the same
information.

Thanks,
Miquèl

[PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Miquel Raynal]

There was in the past a typo in the coordinate readouts property. The
bindings have been updated, the touchscreen driver as well and now
supports both. However, the MFD driver that is in charge of verifying
the validity of the property only checks the bogus one. Add support for
the correctly spelled DT property.

Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 155a8ed879b3..aa46ed669016 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
 	if (tscadc->data->has_tsc) {
 		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
 		of_property_read_u32(node, "ti,wires", &tscmag_wires);
-		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
+		err = of_property_read_u32(node, "ti,coordinate-readouts",
+					   &readouts);
+		if (err < 0)
+			of_property_read_u32(node, "ti,coordiante-readouts",
+					     &readouts);
+
 		of_node_put(node);
+
 		if (tscmag_wires)
 			use_tsc = true;
 	} else {
-- 
2.27.0

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Lee Jones]

On Wed, 15 Sep 2021, Miquel Raynal wrote:

> There was in the past a typo in the coordinate readouts property. The
> bindings have been updated, the touchscreen driver as well and now
> supports both. However, the MFD driver that is in charge of verifying
> the validity of the property only checks the bogus one. Add support for
> the correctly spelled DT property.
> 
> Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> index 155a8ed879b3..aa46ed669016 100644
> --- a/drivers/mfd/ti_am335x_tscadc.c
> +++ b/drivers/mfd/ti_am335x_tscadc.c
> @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
>  	if (tscadc->data->has_tsc) {
>  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
>  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> +					   &readouts);
> +		if (err < 0)
> +			of_property_read_u32(node, "ti,coordiante-readouts",
> +					     &readouts);
> +

How long are you proposing that we support this churn?

>  		of_node_put(node);
> +
>  		if (tscmag_wires)
>  			use_tsc = true;
>  	} else {

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:

> On Wed, 15 Sep 2021, Miquel Raynal wrote:
> 
> > There was in the past a typo in the coordinate readouts property. The
> > bindings have been updated, the touchscreen driver as well and now
> > supports both. However, the MFD driver that is in charge of verifying
> > the validity of the property only checks the bogus one. Add support for
> > the correctly spelled DT property.
> > 
> > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > index 155a8ed879b3..aa46ed669016 100644
> > --- a/drivers/mfd/ti_am335x_tscadc.c
> > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> >  	if (tscadc->data->has_tsc) {
> >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > +					   &readouts);
> > +		if (err < 0)
> > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > +					     &readouts);
> > +  
> 
> How long are you proposing that we support this churn?

Well, I am not proposing anything, I am just "fixing" the driver so
that it fits the bindings :) Given the fact that at the end of this
series there is a patch that changes the "coordiante" typo to
"coordinate" in a device tree source file, I believe it is still too
soon...


Thanks,
Miquèl

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Lee Jones]

On Thu, 23 Sep 2021, Miquel Raynal wrote:

> Hi Lee,
> 
> lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> 
> > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > 
> > > There was in the past a typo in the coordinate readouts property. The
> > > bindings have been updated, the touchscreen driver as well and now
> > > supports both. However, the MFD driver that is in charge of verifying
> > > the validity of the property only checks the bogus one. Add support for
> > > the correctly spelled DT property.
> > > 
> > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > index 155a8ed879b3..aa46ed669016 100644
> > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > >  	if (tscadc->data->has_tsc) {
> > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > +					   &readouts);
> > > +		if (err < 0)
> > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > +					     &readouts);
> > > +  
> > 
> > How long are you proposing that we support this churn?
> 
> Well, I am not proposing anything, I am just "fixing" the driver so
> that it fits the bindings :) Given the fact that at the end of this
> series there is a patch that changes the "coordiante" typo to
> "coordinate" in a device tree source file, I believe it is still too
> soon...

If this is something you're changing in this set, please reconsider.

I'd rather have a slightly misspelled documented property than being
forced to support 2 for any length of time.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:

> On Thu, 23 Sep 2021, Miquel Raynal wrote:
> 
> > Hi Lee,
> > 
> > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> >   
> > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > >   
> > > > There was in the past a typo in the coordinate readouts property. The
> > > > bindings have been updated, the touchscreen driver as well and now
> > > > supports both. However, the MFD driver that is in charge of verifying
> > > > the validity of the property only checks the bogus one. Add support for
> > > > the correctly spelled DT property.
> > > > 
> > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > ---
> > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > index 155a8ed879b3..aa46ed669016 100644
> > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > >  	if (tscadc->data->has_tsc) {
> > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > +					   &readouts);
> > > > +		if (err < 0)
> > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > +					     &readouts);
> > > > +    
> > > 
> > > How long are you proposing that we support this churn?  
> > 
> > Well, I am not proposing anything, I am just "fixing" the driver so
> > that it fits the bindings :) Given the fact that at the end of this
> > series there is a patch that changes the "coordiante" typo to
> > "coordinate" in a device tree source file, I believe it is still too
> > soon...  
> 
> If this is something you're changing in this set, please reconsider.
> 
> I'd rather have a slightly misspelled documented property than being
> forced to support 2 for any length of time.

I am not suggesting anything here:
- "coordiante" was introduced back in 2014.
- "coordinate" was introduced shortly later.
- "coordiante" was considered part of the "stable API" back in 2014 and
  even though the right spelling got introduced very shortly after, DT
  stability rules wanted us to support it forever.
- The touchscreen driver has immediately be fixed to support both but
  not the MFD driver, and this does not make any sense!

We *should* either support only one property (1) or support both in the
two drivers (2), but supporting the two in one driver and supporting
only one in the other one does not make any sense (given the fact that
these two drives are tied together, the touchscreen driver does not
exist without the MFD driver). That is what I am fixing here.

Is #1 valid? Theoretically it's the best scenario. In practice it is
not (yet) possible because the two versions are still used in the
mainline device trees:
$ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
1
$ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
5

So in this series I am fixing the MFD driver to be sure it handles
correctly the correctly spelled DT property which is used by 5 boards
since at least 6 years and I am also updating the remaining DT to use
the correctly spelled property as well.

I suppose we could propose to drop support for the "coordiante"
version of that property in a few years but if we decide to do it right
now we're definitely gonna break users.

Thanks,
Miquèl

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Lee Jones]

On Thu, 23 Sep 2021, Miquel Raynal wrote:

> Hi Lee,
> 
> lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:
> 
> > On Thu, 23 Sep 2021, Miquel Raynal wrote:
> > 
> > > Hi Lee,
> > > 
> > > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> > >   
> > > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > > >   
> > > > > There was in the past a typo in the coordinate readouts property. The
> > > > > bindings have been updated, the touchscreen driver as well and now
> > > > > supports both. However, the MFD driver that is in charge of verifying
> > > > > the validity of the property only checks the bogus one. Add support for
> > > > > the correctly spelled DT property.
> > > > > 
> > > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > ---
> > > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > > index 155a8ed879b3..aa46ed669016 100644
> > > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > > >  	if (tscadc->data->has_tsc) {
> > > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > > +					   &readouts);
> > > > > +		if (err < 0)
> > > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > > +					     &readouts);
> > > > > +    
> > > > 
> > > > How long are you proposing that we support this churn?  
> > > 
> > > Well, I am not proposing anything, I am just "fixing" the driver so
> > > that it fits the bindings :) Given the fact that at the end of this
> > > series there is a patch that changes the "coordiante" typo to
> > > "coordinate" in a device tree source file, I believe it is still too
> > > soon...  
> > 
> > If this is something you're changing in this set, please reconsider.
> > 
> > I'd rather have a slightly misspelled documented property than being
> > forced to support 2 for any length of time.
> 
> I am not suggesting anything here:
> - "coordiante" was introduced back in 2014.
> - "coordinate" was introduced shortly later.
> - "coordiante" was considered part of the "stable API" back in 2014 and
>   even though the right spelling got introduced very shortly after, DT
>   stability rules wanted us to support it forever.
> - The touchscreen driver has immediately be fixed to support both but
>   not the MFD driver, and this does not make any sense!
> 
> We *should* either support only one property (1) or support both in the
> two drivers (2), but supporting the two in one driver and supporting
> only one in the other one does not make any sense (given the fact that
> these two drives are tied together, the touchscreen driver does not
> exist without the MFD driver). That is what I am fixing here.
> 
> Is #1 valid? Theoretically it's the best scenario. In practice it is
> not (yet) possible because the two versions are still used in the
> mainline device trees:
> $ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
> 1
> $ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
> 5
> 
> So in this series I am fixing the MFD driver to be sure it handles
> correctly the correctly spelled DT property which is used by 5 boards
> since at least 6 years and I am also updating the remaining DT to use
> the correctly spelled property as well.
> 
> I suppose we could propose to drop support for the "coordiante"
> version of that property in a few years but if we decide to do it right
> now we're definitely gonna break users.

It's a shame that the misspelled one is still in use.  If they were
all changed over at the start, there would be a strong case for
dropping support for it.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 36/47] mfd: ti_am335x_tscadc: Support the correctly spelled DT property

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Mon, 27 Sep 2021 09:09:32 +0100:

> On Thu, 23 Sep 2021, Miquel Raynal wrote:
> 
> > Hi Lee,
> > 
> > lee.jones@linaro.org wrote on Thu, 23 Sep 2021 10:11:13 +0100:
> >   
> > > On Thu, 23 Sep 2021, Miquel Raynal wrote:
> > >   
> > > > Hi Lee,
> > > > 
> > > > lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:57:10 +0100:
> > > >     
> > > > > On Wed, 15 Sep 2021, Miquel Raynal wrote:
> > > > >     
> > > > > > There was in the past a typo in the coordinate readouts property. The
> > > > > > bindings have been updated, the touchscreen driver as well and now
> > > > > > supports both. However, the MFD driver that is in charge of verifying
> > > > > > the validity of the property only checks the bogus one. Add support for
> > > > > > the correctly spelled DT property.
> > > > > > 
> > > > > > Fixes: c9aeb249bf72 ("Input: ti_am335x_tsc - fix spelling mistake in TSC/ADC DT binding")
> > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > > > > ---
> > > > > >  drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
> > > > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
> > > > > > index 155a8ed879b3..aa46ed669016 100644
> > > > > > --- a/drivers/mfd/ti_am335x_tscadc.c
> > > > > > +++ b/drivers/mfd/ti_am335x_tscadc.c
> > > > > > @@ -144,8 +144,14 @@ static	int ti_tscadc_probe(struct platform_device *pdev)
> > > > > >  	if (tscadc->data->has_tsc) {
> > > > > >  		node = of_get_child_by_name(pdev->dev.of_node, "tsc");
> > > > > >  		of_property_read_u32(node, "ti,wires", &tscmag_wires);
> > > > > > -		of_property_read_u32(node, "ti,coordiante-readouts", &readouts);
> > > > > > +		err = of_property_read_u32(node, "ti,coordinate-readouts",
> > > > > > +					   &readouts);
> > > > > > +		if (err < 0)
> > > > > > +			of_property_read_u32(node, "ti,coordiante-readouts",
> > > > > > +					     &readouts);
> > > > > > +      
> > > > > 
> > > > > How long are you proposing that we support this churn?    
> > > > 
> > > > Well, I am not proposing anything, I am just "fixing" the driver so
> > > > that it fits the bindings :) Given the fact that at the end of this
> > > > series there is a patch that changes the "coordiante" typo to
> > > > "coordinate" in a device tree source file, I believe it is still too
> > > > soon...    
> > > 
> > > If this is something you're changing in this set, please reconsider.
> > > 
> > > I'd rather have a slightly misspelled documented property than being
> > > forced to support 2 for any length of time.  
> > 
> > I am not suggesting anything here:
> > - "coordiante" was introduced back in 2014.
> > - "coordinate" was introduced shortly later.
> > - "coordiante" was considered part of the "stable API" back in 2014 and
> >   even though the right spelling got introduced very shortly after, DT
> >   stability rules wanted us to support it forever.
> > - The touchscreen driver has immediately be fixed to support both but
> >   not the MFD driver, and this does not make any sense!
> > 
> > We *should* either support only one property (1) or support both in the
> > two drivers (2), but supporting the two in one driver and supporting
> > only one in the other one does not make any sense (given the fact that
> > these two drives are tied together, the touchscreen driver does not
> > exist without the MFD driver). That is what I am fixing here.
> > 
> > Is #1 valid? Theoretically it's the best scenario. In practice it is
> > not (yet) possible because the two versions are still used in the
> > mainline device trees:
> > $ git grep coordiante-readouts -- arch/arm/boot/dts/ | wc -l
> > 1
> > $ git grep coordinate-readouts -- arch/arm/boot/dts/ | wc -l
> > 5
> > 
> > So in this series I am fixing the MFD driver to be sure it handles
> > correctly the correctly spelled DT property which is used by 5 boards
> > since at least 6 years and I am also updating the remaining DT to use
> > the correctly spelled property as well.
> > 
> > I suppose we could propose to drop support for the "coordiante"
> > version of that property in a few years but if we decide to do it right
> > now we're definitely gonna break users.  
> 
> It's a shame that the misspelled one is still in use.  If they were
> all changed over at the start, there would be a strong case for
> dropping support for it.

It is, clearly. I believe all DT got changed in one go, but a
contributor at the same time proposed a new DT with the legacy
property "copy-pasted" from the "existing" device trees and this went
off the radar...

Anyway, I'll send a v4 soon with all your suggestions taken into
account.

Thanks,
Miquèl

[PATCH v3 37/47] iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls

[Miquel Raynal]

At least on a am4372, a simple:
$ cat /sys/bus/iio/devices/iio\:deviceX/in_voltage*_raw
can stall forever. It seems that it comes from the fact that the
internal state machine does not have enough time to return to its idle
state in this situation before receiving another request, leading to an
internal stall.

Add a tiadc_wait_idle() helper to ensure no new conversion is requested
while the FSM is still busy.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 1cc2efbb2875..0a4d66a541ed 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -25,6 +25,7 @@
 #include <linux/of_device.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
+#include <linux/iopoll.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 #include <linux/iio/buffer.h>
@@ -102,6 +103,15 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
 	return 1 << adc_dev->channel_step[chan];
 }
 
+static int tiadc_wait_idle(struct tiadc_device *adc_dev)
+{
+	u32 val;
+
+	return readl_poll_timeout(adc_dev->mfd_tscadc->tscadc_base + REG_ADCFSM,
+				  val, !(val & SEQ_STATUS), 10,
+				  IDLE_TIMEOUT * 1000 * adc_dev->channels);
+}
+
 static void tiadc_step_config(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
@@ -295,6 +305,11 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	int i, fifo1count;
+	int ret;
+
+	ret = tiadc_wait_idle(adc_dev);
+	if (ret)
+		return ret;
 
 	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
 				IRQENB_FIFO1OVRRUN |
@@ -446,12 +461,12 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 		int *val, int *val2, long mask)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-	int ret = IIO_VAL_INT;
 	int i, map_val;
 	unsigned int fifo1count, read, stepid;
 	bool found = false;
 	u32 step_en;
 	unsigned long timeout;
+	int ret;
 
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
@@ -461,6 +476,11 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 		return -EINVAL;
 
 	mutex_lock(&adc_dev->fifo1_lock);
+
+	ret = tiadc_wait_idle(adc_dev);
+	if (ret)
+		goto err_unlock;
+
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	while (fifo1count--)
 		tiadc_readl(adc_dev, REG_FIFO1);
@@ -508,7 +528,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 
 err_unlock:
 	mutex_unlock(&adc_dev->fifo1_lock);
-	return ret;
+	return ret ? ret : IIO_VAL_INT;
 }
 
 static const struct iio_info tiadc_info = {
-- 
2.27.0

[PATCH v3 38/47] iio: adc: ti_am335x_adc: Replace license text with SPDX tag

[Miquel Raynal]

Drop the text license and replace it with an equivalent SPDX license tag
identifier which also matches the MODULE_LICENSE("GPL").

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 0a4d66a541ed..11109c84f971 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -1,16 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * TI ADC MFD driver
  *
  * Copyright (C) 2012 Texas Instruments Incorporated - https://www.ti.com/
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation version 2.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether express or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
  */
 
 #include <linux/kernel.h>
-- 
2.27.0

[PATCH v3 39/47] iio: adc: ti_am335x_adc: Fix style

[Miquel Raynal]

These warnings are reported by checkpatch.pl essentially.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 104 ++++++++++++++++----------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 11109c84f971..496603ab7841 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -58,7 +58,7 @@ static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 }
 
 static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
-					unsigned int val)
+			 unsigned int val)
 {
 	writel(val, adc->mfd_tscadc->tscadc_base + reg);
 }
@@ -73,7 +73,7 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
 }
 
 static u32 get_adc_chan_step_mask(struct tiadc_device *adc_dev,
-		struct iio_chan_spec const *chan)
+				  struct iio_chan_spec const *chan)
 {
 	int i;
 
@@ -120,8 +120,6 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 	 * Channel would represent which analog input
 	 * needs to be given to ADC to digitalize data.
 	 */
-
-
 	for (i = 0; i < adc_dev->channels; i++) {
 		int chan;
 
@@ -134,9 +132,8 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 		}
 
 		if (adc_dev->step_avg[i])
-			stepconfig =
-			STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
-			STEPCONFIG_FIFO1;
+			stepconfig = STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
+				     STEPCONFIG_FIFO1;
 		else
 			stepconfig = STEPCONFIG_FIFO1;
 
@@ -144,10 +141,9 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 			stepconfig |= STEPCONFIG_MODE_SWCNT;
 
 		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
-				stepconfig | STEPCONFIG_INP(chan) |
-				STEPCONFIG_INM_ADCREFM |
-				STEPCONFIG_RFP_VREFP |
-				STEPCONFIG_RFM_VREFN);
+			     stepconfig | STEPCONFIG_INP(chan) |
+			     STEPCONFIG_INM_ADCREFM | STEPCONFIG_RFP_VREFP |
+			     STEPCONFIG_RFM_VREFN);
 
 		if (adc_dev->open_delay[i] > STEPCONFIG_MAX_OPENDLY) {
 			dev_warn(dev, "chan %d: wrong open delay, truncated to 0x%lX\n",
@@ -162,8 +158,8 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 		}
 
 		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
-				STEPDELAY_OPEN(adc_dev->open_delay[i]) |
-				STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
+			     STEPDELAY_OPEN(adc_dev->open_delay[i]) |
+			     STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
 
 		adc_dev->channel_step[i] = steps;
 		steps++;
@@ -188,10 +184,12 @@ static irqreturn_t tiadc_irq_h(int irq, void *private)
 		config = tiadc_readl(adc_dev, REG_CTRL);
 		config &= ~(CNTRLREG_SSENB);
 		tiadc_writel(adc_dev, REG_CTRL, config);
-		tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
-				| IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
+		tiadc_writel(adc_dev, REG_IRQSTATUS,
+			     IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW |
+			     IRQENB_FIFO1THRES);
 
-		/* wait for idle state.
+		/*
+		 * Wait for the idle state.
 		 * ADC needs to finish the current conversion
 		 * before disabling the module
 		 */
@@ -219,11 +217,11 @@ static irqreturn_t tiadc_worker_h(int irq, void *private)
 
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 	for (k = 0; k < fifo1count; k = k + i) {
-		for (i = 0; i < (indio_dev->scan_bytes)/2; i++) {
+		for (i = 0; i < indio_dev->scan_bytes / 2; i++) {
 			read = tiadc_readl(adc_dev, REG_FIFO1);
 			data[i] = read & FIFOREAD_DATA_MASK;
 		}
-		iio_push_to_buffers(indio_dev, (u8 *) data);
+		iio_push_to_buffers(indio_dev, (u8 *)data);
 	}
 
 	tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1THRES);
@@ -256,6 +254,7 @@ static int tiadc_start_dma(struct iio_dev *indio_dev)
 	struct dma_async_tx_descriptor *desc;
 
 	dma->current_period = 0; /* We start to fill period 0 */
+
 	/*
 	 * Make the fifo thresh as the multiple of total number of
 	 * channels enabled, so make sure that cyclic DMA period
@@ -265,9 +264,10 @@ static int tiadc_start_dma(struct iio_dev *indio_dev)
 	 */
 	dma->fifo_thresh = rounddown(FIFO1_THRESHOLD + 1,
 				     adc_dev->total_ch_enabled) - 1;
+
 	/* Make sure that period length is multiple of fifo thresh level */
 	dma->period_size = rounddown(DMA_BUFFER_SIZE / 2,
-				    (dma->fifo_thresh + 1) * sizeof(u16));
+				     (dma->fifo_thresh + 1) * sizeof(u16));
 
 	dma->conf.src_maxburst = dma->fifo_thresh + 1;
 	dmaengine_slave_config(dma->chan, &dma->conf);
@@ -303,9 +303,9 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev)
 	if (ret)
 		return ret;
 
-	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
-				IRQENB_FIFO1OVRRUN |
-				IRQENB_FIFO1UNDRFLW));
+	tiadc_writel(adc_dev, REG_IRQCLR,
+		     IRQENB_FIFO1THRES | IRQENB_FIFO1OVRRUN |
+		     IRQENB_FIFO1UNDRFLW);
 
 	/* Flush FIFO. Needed in corner cases in simultaneous tsc/adc use */
 	fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
@@ -335,8 +335,9 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
 
 	am335x_tsc_se_set_cache(adc_dev->mfd_tscadc, enb);
 
-	tiadc_writel(adc_dev,  REG_IRQSTATUS, IRQENB_FIFO1THRES
-				| IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW);
+	tiadc_writel(adc_dev, REG_IRQSTATUS,
+		     IRQENB_FIFO1THRES | IRQENB_FIFO1OVRRUN |
+		     IRQENB_FIFO1UNDRFLW);
 
 	irq_enable = IRQENB_FIFO1OVRRUN;
 	if (!dma->chan)
@@ -352,8 +353,9 @@ static int tiadc_buffer_predisable(struct iio_dev *indio_dev)
 	struct tiadc_dma *dma = &adc_dev->dma;
 	int fifo1count, i;
 
-	tiadc_writel(adc_dev, REG_IRQCLR, (IRQENB_FIFO1THRES |
-				IRQENB_FIFO1OVRRUN | IRQENB_FIFO1UNDRFLW));
+	tiadc_writel(adc_dev, REG_IRQCLR,
+		     IRQENB_FIFO1THRES | IRQENB_FIFO1OVRRUN |
+		     IRQENB_FIFO1UNDRFLW);
 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, adc_dev->buffer_en_ch_steps);
 	adc_dev->buffer_en_ch_steps = 0;
 	adc_dev->total_ch_enabled = 0;
@@ -385,12 +387,11 @@ static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
 };
 
 static int tiadc_iio_buffered_hardware_setup(struct device *dev,
-	struct iio_dev *indio_dev,
-	irqreturn_t (*pollfunc_bh)(int irq, void *p),
-	irqreturn_t (*pollfunc_th)(int irq, void *p),
-	int irq,
-	unsigned long flags,
-	const struct iio_buffer_setup_ops *setup_ops)
+					     struct iio_dev *indio_dev,
+					     irqreturn_t (*pollfunc_bh)(int irq, void *p),
+					     irqreturn_t (*pollfunc_th)(int irq, void *p),
+					     int irq, unsigned long flags,
+					     const struct iio_buffer_setup_ops *setup_ops)
 {
 	int ret;
 
@@ -401,7 +402,7 @@ static int tiadc_iio_buffered_hardware_setup(struct device *dev,
 		return ret;
 
 	return devm_request_threaded_irq(dev, irq, pollfunc_th, pollfunc_bh,
-				flags, indio_dev->name, indio_dev);
+					 flags, indio_dev->name, indio_dev);
 }
 
 static const char * const chan_name_ain[] = {
@@ -426,12 +427,11 @@ static int tiadc_channel_init(struct device *dev, struct iio_dev *indio_dev,
 	indio_dev->num_channels = channels;
 	chan_array = devm_kcalloc(dev, channels, sizeof(*chan_array),
 				  GFP_KERNEL);
-	if (chan_array == NULL)
+	if (!chan_array)
 		return -ENOMEM;
 
 	chan = chan_array;
 	for (i = 0; i < channels; i++, chan++) {
-
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
 		chan->channel = adc_dev->channel_line[i];
@@ -449,8 +449,8 @@ static int tiadc_channel_init(struct device *dev, struct iio_dev *indio_dev,
 }
 
 static int tiadc_read_raw(struct iio_dev *indio_dev,
-		struct iio_chan_spec const *chan,
-		int *val, int *val2, long mask)
+			  struct iio_chan_spec const *chan, int *val, int *val2,
+			  long mask)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	int i, map_val;
@@ -479,9 +479,8 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 
 	am335x_tsc_se_set_once(adc_dev->mfd_tscadc, step_en);
 
-	timeout = jiffies + msecs_to_jiffies
-				(IDLE_TIMEOUT * adc_dev->channels);
 	/* Wait for Fifo threshold interrupt */
+	timeout = jiffies + msecs_to_jiffies(IDLE_TIMEOUT * adc_dev->channels);
 	while (1) {
 		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 		if (fifo1count)
@@ -493,6 +492,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 			goto err_unlock;
 		}
 	}
+
 	map_val = adc_dev->channel_step[chan->scan_index];
 
 	/*
@@ -510,13 +510,14 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 		if (stepid == map_val) {
 			read = read & FIFOREAD_DATA_MASK;
 			found = true;
-			*val = (u16) read;
+			*val = (u16)read;
 		}
 	}
+
 	am335x_tsc_se_adc_done(adc_dev->mfd_tscadc);
 
 	if (!found)
-		ret =  -EBUSY;
+		ret = -EBUSY;
 
 err_unlock:
 	mutex_unlock(&adc_dev->fifo1_lock);
@@ -557,6 +558,7 @@ static int tiadc_request_dma(struct platform_device *pdev,
 		goto err;
 
 	return 0;
+
 err:
 	dma_release_channel(dma->chan);
 	return -ENOMEM;
@@ -606,7 +608,7 @@ static int tiadc_probe(struct platform_device *pdev)
 	}
 
 	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*adc_dev));
-	if (indio_dev == NULL) {
+	if (!indio_dev) {
 		dev_err(&pdev->dev, "failed to allocate iio device\n");
 		return -ENOMEM;
 	}
@@ -628,12 +630,11 @@ static int tiadc_probe(struct platform_device *pdev)
 		return err;
 
 	err = tiadc_iio_buffered_hardware_setup(&pdev->dev, indio_dev,
-		&tiadc_worker_h,
-		&tiadc_irq_h,
-		adc_dev->mfd_tscadc->irq,
-		IRQF_SHARED,
-		&tiadc_buffer_setup_ops);
-
+						&tiadc_worker_h,
+						&tiadc_irq_h,
+						adc_dev->mfd_tscadc->irq,
+						IRQF_SHARED,
+						&tiadc_buffer_setup_ops);
 	if (err)
 		goto err_free_channels;
 
@@ -684,8 +685,7 @@ static int __maybe_unused tiadc_suspend(struct device *dev)
 
 	idle = tiadc_readl(adc_dev, REG_CTRL);
 	idle &= ~(CNTRLREG_SSENB);
-	tiadc_writel(adc_dev, REG_CTRL, (idle |
-			CNTRLREG_POWERDOWN));
+	tiadc_writel(adc_dev, REG_CTRL, idle | CNTRLREG_POWERDOWN);
 
 	return 0;
 }
@@ -698,12 +698,12 @@ static int __maybe_unused tiadc_resume(struct device *dev)
 
 	/* Make sure ADC is powered up */
 	restore = tiadc_readl(adc_dev, REG_CTRL);
-	restore &= ~(CNTRLREG_POWERDOWN);
+	restore &= ~CNTRLREG_POWERDOWN;
 	tiadc_writel(adc_dev, REG_CTRL, restore);
 
 	tiadc_step_config(indio_dev);
 	am335x_tsc_se_set_cache(adc_dev->mfd_tscadc,
-			adc_dev->buffer_en_ch_steps);
+				adc_dev->buffer_en_ch_steps);
 	return 0;
 }
 
-- 
2.27.0

[PATCH v3 40/47] iio: adc: ti_am335x_adc: Get rid of useless gotos

[Miquel Raynal]

Gotos jumping to a return statement are not really useful, drop them.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 496603ab7841..aa151d702a14 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -636,11 +636,11 @@ static int tiadc_probe(struct platform_device *pdev)
 						IRQF_SHARED,
 						&tiadc_buffer_setup_ops);
 	if (err)
-		goto err_free_channels;
+		return err;
 
 	err = iio_device_register(indio_dev);
 	if (err)
-		goto err_buffer_unregister;
+		return err;
 
 	platform_set_drvdata(pdev, indio_dev);
 
@@ -652,8 +652,7 @@ static int tiadc_probe(struct platform_device *pdev)
 
 err_dma:
 	iio_device_unregister(indio_dev);
-err_buffer_unregister:
-err_free_channels:
+
 	return err;
 }
 
-- 
2.27.0

[PATCH v3 41/47] iio: adc: ti_am335x_adc: Gather the checks on the delays

[Miquel Raynal]

Move the checks over the delays provided in the device tree to the
location where these values are read to clarify where they come from.

There are no functional changes besides the device structure used to
display the warnings: let's use the ADC instead of the MFD device.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 50 ++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index aa151d702a14..6f47a1ace3d4 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -107,7 +107,6 @@ static int tiadc_wait_idle(struct tiadc_device *adc_dev)
 static void tiadc_step_config(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-	struct device *dev = adc_dev->mfd_tscadc->dev;
 	unsigned int stepconfig;
 	int i, steps = 0;
 
@@ -125,12 +124,6 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 
 		chan = adc_dev->channel_line[i];
 
-		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
-			dev_warn(dev, "chan %d: wrong step avg, truncated to %ld\n",
-				 chan, STEPCONFIG_AVG_16);
-			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
-		}
-
 		if (adc_dev->step_avg[i])
 			stepconfig = STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
 				     STEPCONFIG_FIFO1;
@@ -145,18 +138,6 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 			     STEPCONFIG_INM_ADCREFM | STEPCONFIG_RFP_VREFP |
 			     STEPCONFIG_RFM_VREFN);
 
-		if (adc_dev->open_delay[i] > STEPCONFIG_MAX_OPENDLY) {
-			dev_warn(dev, "chan %d: wrong open delay, truncated to 0x%lX\n",
-				 chan, STEPCONFIG_MAX_OPENDLY);
-			adc_dev->open_delay[i] = STEPCONFIG_MAX_OPENDLY;
-		}
-
-		if (adc_dev->sample_delay[i] > STEPCONFIG_MAX_SAMPLE) {
-			dev_warn(dev, "chan %d: wrong sample delay, truncated to 0x%lX\n",
-				 chan, STEPCONFIG_MAX_SAMPLE);
-			adc_dev->sample_delay[i] = STEPCONFIG_MAX_SAMPLE;
-		}
-
 		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
 			     STEPDELAY_OPEN(adc_dev->open_delay[i]) |
 			     STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
@@ -572,6 +553,7 @@ static int tiadc_parse_dt(struct platform_device *pdev,
 	const __be32 *cur;
 	int channels = 0;
 	u32 val;
+	int i;
 
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
 		adc_dev->channel_line[channels] = val;
@@ -584,6 +566,8 @@ static int tiadc_parse_dt(struct platform_device *pdev,
 		channels++;
 	}
 
+	adc_dev->channels = channels;
+
 	of_property_read_u32_array(node, "ti,chan-step-avg",
 				   adc_dev->step_avg, channels);
 	of_property_read_u32_array(node, "ti,chan-step-opendelay",
@@ -591,7 +575,33 @@ static int tiadc_parse_dt(struct platform_device *pdev,
 	of_property_read_u32_array(node, "ti,chan-step-sampledelay",
 				   adc_dev->sample_delay, channels);
 
-	adc_dev->channels = channels;
+	for (i = 0; i < adc_dev->channels; i++) {
+		int chan;
+
+		chan = adc_dev->channel_line[i];
+
+		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
+			dev_warn(&pdev->dev,
+				 "chan %d: wrong step avg, truncated to %ld\n",
+				 chan, STEPCONFIG_AVG_16);
+			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
+		}
+
+		if (adc_dev->open_delay[i] > STEPCONFIG_MAX_OPENDLY) {
+			dev_warn(&pdev->dev,
+				 "chan %d: wrong open delay, truncated to 0x%lX\n",
+				 chan, STEPCONFIG_MAX_OPENDLY);
+			adc_dev->open_delay[i] = STEPCONFIG_MAX_OPENDLY;
+		}
+
+		if (adc_dev->sample_delay[i] > STEPCONFIG_MAX_SAMPLE) {
+			dev_warn(&pdev->dev,
+				 "chan %d: wrong sample delay, truncated to 0x%lX\n",
+				 chan, STEPCONFIG_MAX_SAMPLE);
+			adc_dev->sample_delay[i] = STEPCONFIG_MAX_SAMPLE;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.27.0

[PATCH v3 42/47] iio: adc: ti_am335x_adc: Add a unit to the timeout delay

[Miquel Raynal]

The lack of unit in the macro name kind of tricked me when I was
troubleshooting an issue. Physical constants should always get a unit.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c      | 4 ++--
 include/linux/mfd/ti_am335x_tscadc.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 6f47a1ace3d4..e7dba10b29b4 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -101,7 +101,7 @@ static int tiadc_wait_idle(struct tiadc_device *adc_dev)
 
 	return readl_poll_timeout(adc_dev->mfd_tscadc->tscadc_base + REG_ADCFSM,
 				  val, !(val & SEQ_STATUS), 10,
-				  IDLE_TIMEOUT * 1000 * adc_dev->channels);
+				  IDLE_TIMEOUT_MS * 1000 * adc_dev->channels);
 }
 
 static void tiadc_step_config(struct iio_dev *indio_dev)
@@ -461,7 +461,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	am335x_tsc_se_set_once(adc_dev->mfd_tscadc, step_en);
 
 	/* Wait for Fifo threshold interrupt */
-	timeout = jiffies + msecs_to_jiffies(IDLE_TIMEOUT * adc_dev->channels);
+	timeout = jiffies + msecs_to_jiffies(IDLE_TIMEOUT_MS * adc_dev->channels);
 	while (1) {
 		fifo1count = tiadc_readl(adc_dev, REG_FIFO1CNT);
 		if (fifo1count)
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 9a412e6d764b..223e5840f3a3 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -141,7 +141,7 @@
  *
  * max processing time: 266431 * 308ns = 83ms(approx)
  */
-#define IDLE_TIMEOUT		83 /* milliseconds */
+#define IDLE_TIMEOUT_MS		83 /* milliseconds */
 
 #define TSCADC_CELLS		2
 
-- 
2.27.0

[PATCH v3 43/47] iio: adc: ti_am335x_adc: Add the scale information

[Miquel Raynal]

Without the scale, the values returned to the user are just a picture of
the input voltage against the full scale range of the ADC. We need to
provide the actual conversion factor to get milli-Volts values.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index e7dba10b29b4..867690b6c2dd 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -417,6 +417,7 @@ static int tiadc_channel_init(struct device *dev, struct iio_dev *indio_dev,
 		chan->indexed = 1;
 		chan->channel = adc_dev->channel_line[i];
 		chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
+		chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
 		chan->datasheet_name = chan_name_ain[chan->channel];
 		chan->scan_index = i;
 		chan->scan_type.sign = 'u';
@@ -441,6 +442,23 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,
 	unsigned long timeout;
 	int ret;
 
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = 1800;
+			*val2 = chan->scan_type.realbits;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
+		}
+		break;
+	default:
+		return -EINVAL;
+	}
+
 	if (iio_buffer_enabled(indio_dev))
 		return -EBUSY;
 
-- 
2.27.0

[PATCH v3 44/47] iio: adc: ti_am335x_adc: Add the am437x compatible

[Miquel Raynal]

This driver supports both the legacy controller (am33xx) and the
extended one (am437x), so let's add a new compatible.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 867690b6c2dd..dbdc1ef48566 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -738,6 +738,7 @@ static SIMPLE_DEV_PM_OPS(tiadc_pm_ops, tiadc_suspend, tiadc_resume);
 
 static const struct of_device_id ti_adc_dt_ids[] = {
 	{ .compatible = "ti,am3359-adc", },
+	{ .compatible = "ti,am4372-adc", },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_adc_dt_ids);
-- 
2.27.0

[PATCH v3 45/47] ARM: dts: am437x-cm-t43: Use a correctly spelled DT property

[Miquel Raynal]

Back in 2014 a property got misspelled "ti,coordiante-readouts" instead
of "ti,coordinate-readouts".

The year after it got fixed but both are still supported, although this
is not a reason to continue using this old deprecated property.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/am437x-cm-t43.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am437x-cm-t43.dts b/arch/arm/boot/dts/am437x-cm-t43.dts
index 5ce8e684e7d3..3e3354780db8 100644
--- a/arch/arm/boot/dts/am437x-cm-t43.dts
+++ b/arch/arm/boot/dts/am437x-cm-t43.dts
@@ -399,7 +399,7 @@ &tscadc {
 	tsc {
 		ti,wires = <4>;
 		ti,x-plate-resistance = <200>;
-		ti,coordiante-readouts = <5>;
+		ti,coordinate-readouts = <5>;
 		ti,wire-config = <0x00 0x11 0x22 0x33>;
 	};
 
-- 
2.27.0

Re: [PATCH v3 45/47] ARM: dts: am437x-cm-t43: Use a correctly spelled DT property

[Tony Lindgren]

* Miquel Raynal <miquel.raynal@bootlin.com> [210915 16:01]:
> Back in 2014 a property got misspelled "ti,coordiante-readouts" instead
> of "ti,coordinate-readouts".
> 
> The year after it got fixed but both are still supported, although this
> is not a reason to continue using this old deprecated property.

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH v3 46/47] ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module

[Miquel Raynal]

This hardware module is close to the am33xx ADC module but instead of
featuring a touchscreen it has a magnetic reader capability.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/am437x-l4.dtsi     | 31 ++++++++++++++++++++++++++--
 arch/arm/boot/dts/am43xx-clocks.dtsi |  7 +++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/am437x-l4.dtsi b/arch/arm/boot/dts/am437x-l4.dtsi
index ba58e6b0da1d..8f2268c02778 100644
--- a/arch/arm/boot/dts/am437x-l4.dtsi
+++ b/arch/arm/boot/dts/am437x-l4.dtsi
@@ -2378,11 +2378,38 @@ hdq: hdq@0 {
 		};
 
 		target-module@4c000 {			/* 0x4834c000, ap 114 72.0 */
-			compatible = "ti,sysc";
-			status = "disabled";
+			compatible = "ti,sysc-omap4", "ti,sysc";
+			reg = <0x4c000 0x4>,
+			      <0x4c010 0x4>;
+			reg-names = "rev", "sysc";
+			ti,sysc-sidle = <SYSC_IDLE_FORCE>,
+					<SYSC_IDLE_NO>,
+					<SYSC_IDLE_SMART>;
+			clocks = <&l3s_clkctrl AM4_L3S_ADC1_CLKCTRL 0>;
+			clock-names = "fck";
 			#address-cells = <1>;
 			#size-cells = <1>;
 			ranges = <0x0 0x4c000 0x2000>;
+
+			magadc: magadc@0 {
+				compatible = "ti,am4372-magadc";
+				reg = <0x0 0x2000>;
+				interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&adc_mag_fck>;
+				clock-names = "fck";
+				dmas = <&edma 54 0>, <&edma 55 0>;
+				dma-names = "fifo0", "fifo1";
+				status = "disabled";
+
+				mag {
+					compatible = "ti,am4372-mag";
+				};
+
+				adc {
+					#io-channel-cells = <1>;
+					compatible ="ti,am4372-adc";
+				};
+			};
 		};
 
 		target-module@80000 {			/* 0x48380000, ap 123 42.0 */
diff --git a/arch/arm/boot/dts/am43xx-clocks.dtsi b/arch/arm/boot/dts/am43xx-clocks.dtsi
index 314fc5975acb..66e892fa3398 100644
--- a/arch/arm/boot/dts/am43xx-clocks.dtsi
+++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
@@ -444,6 +444,13 @@ wdt1_fck: wdt1_fck@422c {
 		reg = <0x422c>;
 	};
 
+	adc_mag_fck: adc_mag_fck@424c {
+		#clock-cells = <0>;
+		compatible = "ti,mux-clock";
+		clocks = <&sys_clkin_ck>, <&dpll_per_m2_ck>;
+		reg = <0x424c>;
+	};
+
 	l3_gclk: l3_gclk {
 		#clock-cells = <0>;
 		compatible = "fixed-factor-clock";
-- 
2.27.0

Re: [PATCH v3 46/47] ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module

[Tony Lindgren]

* Miquel Raynal <miquel.raynal@bootlin.com> [210915 16:01]:
> This hardware module is close to the am33xx ADC module but instead of
> featuring a touchscreen it has a magnetic reader capability.

Acked-by: Tony Lindgren <tony@atomide.com>

[PATCH v3 47/47] ARM: dts: am437x-gp-evm: enable ADC1

[Miquel Raynal]

ADC0 and ADC1 pins are available on external connector J22.

Enable ADC1 which was missing.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 arch/arm/boot/dts/am437x-gp-evm.dts | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts b/arch/arm/boot/dts/am437x-gp-evm.dts
index c2e4896076e7..4416ddb559e4 100644
--- a/arch/arm/boot/dts/am437x-gp-evm.dts
+++ b/arch/arm/boot/dts/am437x-gp-evm.dts
@@ -775,6 +775,14 @@ adc {
 	};
 };
 
+&magadc {
+	status = "okay";
+
+	adc {
+		ti,adc-channels = <0 1 2 3 4 5 6 7>;
+	};
+};
+
 &ecap0 {
 	status = "okay";
 	pinctrl-names = "default";
-- 
2.27.0

Re: [PATCH v3 00/47] TI AM437X ADC1

[Miquel Raynal]

Hello Lee,

miquel.raynal@bootlin.com wrote on Wed, 15 Sep 2021 17:58:21 +0200:

> Hello,
> 
> This is a (fairly big) series bringing support of AM437X ADC1.
> On TI AM33XX SoCs family there is an ADC that can also be connected to a
> touchscreen. This hardware has been extended and is present on certain
> SoCs from the AM437X family. In particular, the touchscreen has been
> replaced by a magnetic card reader. In both cases, the representation is
> an MFD device with two children:
> * on AM33XX: the touchscreen controller and the ADC
> * on AM437X: the magnetic card reader and the ADC
> 
> This series really targets small and atomic changes so that the overall
> review is eased, even though it leads to a lot of rather small patches.
> Here are the steps:
> * Supporting the missing clock
> * Translating a single text file containing the description for the
>   MFD, the touchscreen and the ADC into three independent yaml files.
> * Cleaning/preparing the MFD driver.
> * Supporting ADC1 in the MFD driver.
> * Cleaning/preparing of the ADC driver.
> * Supporting ADC1 in the ADC driver.
> * Updating various device trees.
> 
> Here is the full series again, almost reviewed and acked entirely, but
> rebased on the last -rc1. The clock patch has been acked, the ADC
> patches as well, so we expect the series to go through the MFD
> tree if hte maintainers agree with it.

Lee, I would welcome your feedback about this big series. I received
the acknowledgment of:
* Stephen for the clock patch
* Jonathan and Rob for the binding patches
* Jonathan for the IIO patches
* Jonathan again for the MFD patches

There is currently only one issue left: I should reorder 28/47 before
26/47 and change the commit message of patch 27/47.

If this is fine for you I would like to:
* let you pick patches from 1 to 25 of v3
* I will send a v4 with only three patches: 26, 27, 28 with Jonathan's
  last comments addressed.
* Finally you could take the remaining patches of v3 up to patch 44 (the
  last 3 are DT patches).

Would this work for you? If possible I would like to avoid resending
the entire series once again. Unless of course you have additional
comments which I will obviously address.

Thanks,
Miquèl

> Changes in v3:
> * Rebased on top of v5.15-rc1.
> * R-by/A-by tags added.
> * Light reordering to let the of_put_node() fix to be applied more easily
> * Dropped a patch made useless because of the previous reordering
> * Explained how the tscadc->ctrl variable was used.
> * Fixed a couple of typos.
> * Included the change for the HZ macro.
> * Went further in the BIT()/FIELD_PREP() cleanup.
> * Added maximum definitions for sample delay/open delay.
> * Removed useless definitions.
> * Fixed a couple of rebase conflicts (the series was not bisectable).
> 
> Changes in v2:
> * Added various R-by/A-by tags.
> * Various typos & style fixes.
> [Bindings]
> * Included the missing ti,am654-tscadc compatible.
> * Reworded the compatible lines as requested by Jonathan.
> * Reworded the bindings content a little bit as advised by Rob (subnodes
>   being objects, MFD descriptions provided once, status and unused
>   labels removed).
> [SPDX changes]
> * Mentioned that the license macro and the license text matched.
> * Also added an SPDX tag in the MFD header.
> [MFD header]
> * Used the BIT(), GENMASK() and PREP_FIELD() macros when relevant.
> [MFD driver]
> * Did not reordered the variables declared on the probe stack as advised
>   by Jonathan.
> * Added missing of_node_put() calls.
> * Moved the patch changing the place where the main structure is
>   allocated to directly precede the patch using this change.
> * Fixed the driver data wiring (bug happening between ex patches 16 and
>   28).
> * Added a commit just to explain the reordering of the register writes
>   during initialization/resume.
> * Explained the check about 'use_tsc' in the commit message.
> * Added a link to the TRM in a commit message referencing it.
> * Removed the use of the ti,tracks property, used a constant value
>   instead.
> * Dropped the error check when retrieving the "wrong" DT property
>   (coordiante-readouts) which is unused.
> 
> 
> Miquel Raynal (47):
>   clk: ti: am43xx: Add clkctrl data for am43xx ADC1
>   dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this
>     MFD
>   dt-bindings: touchscreen: ti,am3359-tsc: New yaml description
>   dt-bindings: iio: adc: ti,am3359-adc: New yaml description
>   dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file
>   dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible
>   dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible
>   mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put
>   mfd: ti_am335x_tscadc: Replace license text with SPDX tag
>   mfd: ti_am335x_tscadc: Fix style
>   mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack
>     variables
>   mfd: ti_am335x_tscadc: Get rid of useless gotos
>   mfd: ti_am335x_tscadc: Reword the comment explaining the dividers
>   mfd: ti_am335x_tscadc: Don't search the tree for our clock
>   mfd: ti_am335x_tscadc: Simplify divisor calculation
>   mfd: ti_am335x_tscadc: Move the driver structure allocation earlier
>   mfd: ti_am335x_tscadc: Use driver data
>   mfd: ti_am335x_tscadc: Mimic the probe from resume()
>   mfd: ti_am335x_tscadc: Drop useless variables from the driver
>     structure
>   mfd: ti_am335x_tscadc: Always provide an idle configuration
>   mfd: ti_am335x_tscadc: Reorder the initialization steps
>   mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place
>   mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag
>   mfd: ti_am335x_tscadc: Fix header spacing
>   mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro
>   mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when
>     relevant
>   mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries
>   mfd: ti_am335x_tscadc: Drop useless definitions from the header
>   mfd: ti_am335x_tscadc: Rename the subsystem enable macro
>   mfd: ti_am335x_tscadc: Add TSC prefix in certain macros
>   mfd: ti_am335x_tscadc: Rename a variable
>   mfd: ti_am335x_tscadc: Fix an error message
>   mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a
>     touchscreen
>   mfd: ti_am335x_tscadc: Introduce has_tsc
>   mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support
>   mfd: ti_am335x_tscadc: Support the correctly spelled DT property
>   iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls
>   iio: adc: ti_am335x_adc: Replace license text with SPDX tag
>   iio: adc: ti_am335x_adc: Fix style
>   iio: adc: ti_am335x_adc: Get rid of useless gotos
>   iio: adc: ti_am335x_adc: Gather the checks on the delays
>   iio: adc: ti_am335x_adc: Add a unit to the timeout delay
>   iio: adc: ti_am335x_adc: Add the scale information
>   iio: adc: ti_am335x_adc: Add the am437x compatible
>   ARM: dts: am437x-cm-t43: Use a correctly spelled DT property
>   ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module
>   ARM: dts: am437x-gp-evm: enable ADC1
> 
>  .../bindings/iio/adc/ti,am3359-adc.yaml       |  70 +++++
>  .../input/touchscreen/ti,am3359-tsc.yaml      |  76 ++++++
>  .../bindings/input/touchscreen/ti-tsc-adc.txt |  91 -------
>  .../bindings/mfd/ti,am3359-tscadc.yaml        |  84 ++++++
>  arch/arm/boot/dts/am437x-cm-t43.dts           |   2 +-
>  arch/arm/boot/dts/am437x-gp-evm.dts           |   8 +
>  arch/arm/boot/dts/am437x-l4.dtsi              |  31 ++-
>  arch/arm/boot/dts/am43xx-clocks.dtsi          |   7 +
>  drivers/clk/ti/clk-43xx.c                     |   1 +
>  drivers/iio/adc/ti_am335x_adc.c               | 220 +++++++++-------
>  drivers/mfd/ti_am335x_tscadc.c                | 243 ++++++++++--------
>  include/dt-bindings/clock/am4.h               |   1 +
>  include/linux/mfd/ti_am335x_tscadc.h          | 115 ++++-----
>  13 files changed, 599 insertions(+), 350 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
>  delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
> 

Re: [PATCH v3 00/47] TI AM437X ADC1

[Lee Jones]

On Wed, 15 Sep 2021, Miquel Raynal wrote:

> Hello,
> 
> This is a (fairly big) series bringing support of AM437X ADC1.
> On TI AM33XX SoCs family there is an ADC that can also be connected to a
> touchscreen. This hardware has been extended and is present on certain
> SoCs from the AM437X family. In particular, the touchscreen has been
> replaced by a magnetic card reader. In both cases, the representation is
> an MFD device with two children:
> * on AM33XX: the touchscreen controller and the ADC
> * on AM437X: the magnetic card reader and the ADC
> 
> This series really targets small and atomic changes so that the overall
> review is eased, even though it leads to a lot of rather small patches.
> Here are the steps:
> * Supporting the missing clock
> * Translating a single text file containing the description for the
>   MFD, the touchscreen and the ADC into three independent yaml files.
> * Cleaning/preparing the MFD driver.
> * Supporting ADC1 in the MFD driver.
> * Cleaning/preparing of the ADC driver.
> * Supporting ADC1 in the ADC driver.
> * Updating various device trees.
> 
> Here is the full series again, almost reviewed and acked entirely, but
> rebased on the last -rc1. The clock patch has been acked, the ADC
> patches as well, so we expect the series to go through the MFD
> tree if hte maintainers agree with it.
> 
> Thanks,
> Miquèl
> 
> Changes in v3:
> * Rebased on top of v5.15-rc1.
> * R-by/A-by tags added.
> * Light reordering to let the of_put_node() fix to be applied more easily
> * Dropped a patch made useless because of the previous reordering
> * Explained how the tscadc->ctrl variable was used.
> * Fixed a couple of typos.
> * Included the change for the HZ macro.
> * Went further in the BIT()/FIELD_PREP() cleanup.
> * Added maximum definitions for sample delay/open delay.
> * Removed useless definitions.
> * Fixed a couple of rebase conflicts (the series was not bisectable).
> 
> Changes in v2:
> * Added various R-by/A-by tags.
> * Various typos & style fixes.
> [Bindings]
> * Included the missing ti,am654-tscadc compatible.
> * Reworded the compatible lines as requested by Jonathan.
> * Reworded the bindings content a little bit as advised by Rob (subnodes
>   being objects, MFD descriptions provided once, status and unused
>   labels removed).
> [SPDX changes]
> * Mentioned that the license macro and the license text matched.
> * Also added an SPDX tag in the MFD header.
> [MFD header]
> * Used the BIT(), GENMASK() and PREP_FIELD() macros when relevant.
> [MFD driver]
> * Did not reordered the variables declared on the probe stack as advised
>   by Jonathan.
> * Added missing of_node_put() calls.
> * Moved the patch changing the place where the main structure is
>   allocated to directly precede the patch using this change.
> * Fixed the driver data wiring (bug happening between ex patches 16 and
>   28).
> * Added a commit just to explain the reordering of the register writes
>   during initialization/resume.
> * Explained the check about 'use_tsc' in the commit message.
> * Added a link to the TRM in a commit message referencing it.
> * Removed the use of the ti,tracks property, used a constant value
>   instead.
> * Dropped the error check when retrieving the "wrong" DT property
>   (coordiante-readouts) which is unused.
> 
> 
> Miquel Raynal (47):
>   clk: ti: am43xx: Add clkctrl data for am43xx ADC1
>   dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this
>     MFD
>   dt-bindings: touchscreen: ti,am3359-tsc: New yaml description
>   dt-bindings: iio: adc: ti,am3359-adc: New yaml description
>   dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file
>   dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible
>   dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible
>   mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put
>   mfd: ti_am335x_tscadc: Replace license text with SPDX tag
>   mfd: ti_am335x_tscadc: Fix style
>   mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack
>     variables
>   mfd: ti_am335x_tscadc: Get rid of useless gotos
>   mfd: ti_am335x_tscadc: Reword the comment explaining the dividers
>   mfd: ti_am335x_tscadc: Don't search the tree for our clock
>   mfd: ti_am335x_tscadc: Simplify divisor calculation
>   mfd: ti_am335x_tscadc: Move the driver structure allocation earlier
>   mfd: ti_am335x_tscadc: Use driver data
>   mfd: ti_am335x_tscadc: Mimic the probe from resume()
>   mfd: ti_am335x_tscadc: Drop useless variables from the driver
>     structure
>   mfd: ti_am335x_tscadc: Always provide an idle configuration
>   mfd: ti_am335x_tscadc: Reorder the initialization steps
>   mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place
>   mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag
>   mfd: ti_am335x_tscadc: Fix header spacing
>   mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro
>   mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when
>     relevant
>   mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries
>   mfd: ti_am335x_tscadc: Drop useless definitions from the header
>   mfd: ti_am335x_tscadc: Rename the subsystem enable macro
>   mfd: ti_am335x_tscadc: Add TSC prefix in certain macros
>   mfd: ti_am335x_tscadc: Rename a variable
>   mfd: ti_am335x_tscadc: Fix an error message
>   mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a
>     touchscreen
>   mfd: ti_am335x_tscadc: Introduce has_tsc
>   mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support
>   mfd: ti_am335x_tscadc: Support the correctly spelled DT property

The MFD patches I haven't reviewed/replied to seem fine.

>   iio: adc: ti_am335x_adc: Wait the idle state to avoid stalls
>   iio: adc: ti_am335x_adc: Replace license text with SPDX tag
>   iio: adc: ti_am335x_adc: Fix style
>   iio: adc: ti_am335x_adc: Get rid of useless gotos
>   iio: adc: ti_am335x_adc: Gather the checks on the delays
>   iio: adc: ti_am335x_adc: Add a unit to the timeout delay
>   iio: adc: ti_am335x_adc: Add the scale information
>   iio: adc: ti_am335x_adc: Add the am437x compatible
>   ARM: dts: am437x-cm-t43: Use a correctly spelled DT property
>   ARM: dts: am43xx: Describe the magnetic reader/ADC1 hardware module
>   ARM: dts: am437x-gp-evm: enable ADC1
> 
>  .../bindings/iio/adc/ti,am3359-adc.yaml       |  70 +++++
>  .../input/touchscreen/ti,am3359-tsc.yaml      |  76 ++++++
>  .../bindings/input/touchscreen/ti-tsc-adc.txt |  91 -------
>  .../bindings/mfd/ti,am3359-tscadc.yaml        |  84 ++++++
>  arch/arm/boot/dts/am437x-cm-t43.dts           |   2 +-
>  arch/arm/boot/dts/am437x-gp-evm.dts           |   8 +
>  arch/arm/boot/dts/am437x-l4.dtsi              |  31 ++-
>  arch/arm/boot/dts/am43xx-clocks.dtsi          |   7 +
>  drivers/clk/ti/clk-43xx.c                     |   1 +
>  drivers/iio/adc/ti_am335x_adc.c               | 220 +++++++++-------
>  drivers/mfd/ti_am335x_tscadc.c                | 243 ++++++++++--------
>  include/dt-bindings/clock/am4.h               |   1 +
>  include/linux/mfd/ti_am335x_tscadc.h          | 115 ++++-----
>  13 files changed, 599 insertions(+), 350 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
>  create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
>  delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>  create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
> 

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v3 00/47] TI AM437X ADC1

[Miquel Raynal]

Hi Lee,

lee.jones@linaro.org wrote on Wed, 22 Sep 2021 17:02:00 +0100:

> On Wed, 15 Sep 2021, Miquel Raynal wrote:
> 
> > Hello,
> > 
> > This is a (fairly big) series bringing support of AM437X ADC1.
> > On TI AM33XX SoCs family there is an ADC that can also be connected to a
> > touchscreen. This hardware has been extended and is present on certain
> > SoCs from the AM437X family. In particular, the touchscreen has been
> > replaced by a magnetic card reader. In both cases, the representation is
> > an MFD device with two children:
> > * on AM33XX: the touchscreen controller and the ADC
> > * on AM437X: the magnetic card reader and the ADC
> > 
> > This series really targets small and atomic changes so that the overall
> > review is eased, even though it leads to a lot of rather small patches.
> > Here are the steps:
> > * Supporting the missing clock
> > * Translating a single text file containing the description for the
> >   MFD, the touchscreen and the ADC into three independent yaml files.
> > * Cleaning/preparing the MFD driver.
> > * Supporting ADC1 in the MFD driver.
> > * Cleaning/preparing of the ADC driver.
> > * Supporting ADC1 in the ADC driver.
> > * Updating various device trees.
> > 
> > Here is the full series again, almost reviewed and acked entirely, but
> > rebased on the last -rc1. The clock patch has been acked, the ADC
> > patches as well, so we expect the series to go through the MFD
> > tree if hte maintainers agree with it.
> > 
> > Thanks,
> > Miquèl
> > 
> > Changes in v3:
> > * Rebased on top of v5.15-rc1.
> > * R-by/A-by tags added.
> > * Light reordering to let the of_put_node() fix to be applied more easily
> > * Dropped a patch made useless because of the previous reordering
> > * Explained how the tscadc->ctrl variable was used.
> > * Fixed a couple of typos.
> > * Included the change for the HZ macro.
> > * Went further in the BIT()/FIELD_PREP() cleanup.
> > * Added maximum definitions for sample delay/open delay.
> > * Removed useless definitions.
> > * Fixed a couple of rebase conflicts (the series was not bisectable).
> > 
> > Changes in v2:
> > * Added various R-by/A-by tags.
> > * Various typos & style fixes.
> > [Bindings]
> > * Included the missing ti,am654-tscadc compatible.
> > * Reworded the compatible lines as requested by Jonathan.
> > * Reworded the bindings content a little bit as advised by Rob (subnodes
> >   being objects, MFD descriptions provided once, status and unused
> >   labels removed).
> > [SPDX changes]
> > * Mentioned that the license macro and the license text matched.
> > * Also added an SPDX tag in the MFD header.
> > [MFD header]
> > * Used the BIT(), GENMASK() and PREP_FIELD() macros when relevant.
> > [MFD driver]
> > * Did not reordered the variables declared on the probe stack as advised
> >   by Jonathan.
> > * Added missing of_node_put() calls.
> > * Moved the patch changing the place where the main structure is
> >   allocated to directly precede the patch using this change.
> > * Fixed the driver data wiring (bug happening between ex patches 16 and
> >   28).
> > * Added a commit just to explain the reordering of the register writes
> >   during initialization/resume.
> > * Explained the check about 'use_tsc' in the commit message.
> > * Added a link to the TRM in a commit message referencing it.
> > * Removed the use of the ti,tracks property, used a constant value
> >   instead.
> > * Dropped the error check when retrieving the "wrong" DT property
> >   (coordiante-readouts) which is unused.
> > 
> > 
> > Miquel Raynal (47):
> >   clk: ti: am43xx: Add clkctrl data for am43xx ADC1
> >   dt-bindings: mfd: ti,am3359-tscadc: Add a yaml description for this
> >     MFD
> >   dt-bindings: touchscreen: ti,am3359-tsc: New yaml description
> >   dt-bindings: iio: adc: ti,am3359-adc: New yaml description
> >   dt-bindings: touchscreen: ti,am3359-tsc: Remove deprecated text file
> >   dt-bindings: mfd: ti,am3359-tscadc: Describe am4372 MFD compatible
> >   dt-bindings: iio: adc: ti,am3359-adc: Describe am4372 ADC compatible
> >   mfd: ti_am335x_tscadc: Ensure a balanced number of node get/put
> >   mfd: ti_am335x_tscadc: Replace license text with SPDX tag
> >   mfd: ti_am335x_tscadc: Fix style
> >   mfd: ti_am335x_tscadc: Drop extra spacing when declaring stack
> >     variables
> >   mfd: ti_am335x_tscadc: Get rid of useless gotos
> >   mfd: ti_am335x_tscadc: Reword the comment explaining the dividers
> >   mfd: ti_am335x_tscadc: Don't search the tree for our clock
> >   mfd: ti_am335x_tscadc: Simplify divisor calculation
> >   mfd: ti_am335x_tscadc: Move the driver structure allocation earlier
> >   mfd: ti_am335x_tscadc: Use driver data
> >   mfd: ti_am335x_tscadc: Mimic the probe from resume()
> >   mfd: ti_am335x_tscadc: Drop useless variables from the driver
> >     structure
> >   mfd: ti_am335x_tscadc: Always provide an idle configuration
> >   mfd: ti_am335x_tscadc: Reorder the initialization steps
> >   mfd: ti_am335x_tscadc: Gather the ctrl register logic in one place
> >   mfd: ti_am335x_tscadc: Replace the header license text with SPDX tag
> >   mfd: ti_am335x_tscadc: Fix header spacing
> >   mfd: ti_am335x_tscadc: Use the new HZ_PER_MHZ macro
> >   mfd: ti_am335x_tscadc: Use BIT(), GENMASK() and FIELD_PREP() when
> >     relevant
> >   mfd: ti_am335x_tscadc: Clarify the maximum values for DT entries
> >   mfd: ti_am335x_tscadc: Drop useless definitions from the header
> >   mfd: ti_am335x_tscadc: Rename the subsystem enable macro
> >   mfd: ti_am335x_tscadc: Add TSC prefix in certain macros
> >   mfd: ti_am335x_tscadc: Rename a variable
> >   mfd: ti_am335x_tscadc: Fix an error message
> >   mfd: ti_am335x_tscadc: Add a boolean to clarify the presence of a
> >     touchscreen
> >   mfd: ti_am335x_tscadc: Introduce has_tsc
> >   mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support
> >   mfd: ti_am335x_tscadc: Support the correctly spelled DT property  
> 
> The MFD patches I haven't reviewed/replied to seem fine.

Great!

I'll address all your comments, bring-in the latest
Reviewed-by/Acked-by that I received on the other patches and send a
v4. Thanks for the feedback.

Cheers,
Miquèl