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, I can split it if the various maintainers
prefer. The clk patch can be merged alone, the MFD and the ADC changes
unfortunately are interdependent so, reviewed, the MFD maintainers will
have to create an immutable tag after applying so that ADC maintainers
can merge it. For now, this is open for reviews :) !
Thanks,
Miquèl
Miquel Raynal (40):
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: 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: Move the driver structure allocation earlier
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: 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: Gather the ctrl register logic at one place
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: Move control register configuration
mfd: ti_am335x_tscadc: Add ADC1/magnetic reader support
mfd: ti_am335x_tscadc: Support the correctly spelled DT property
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: Wait the idle state to avoid stalls
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 | 83 +++++++
.../input/touchscreen/ti,am3359-tsc.yaml | 89 +++++++
.../bindings/input/touchscreen/ti-tsc-adc.txt | 91 -------
.../bindings/mfd/ti,am3359-tscadc.yaml | 80 ++++++
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 | 217 +++++++++-------
drivers/mfd/ti_am335x_tscadc.c | 232 ++++++++++--------
include/dt-bindings/clock/am4.h | 1 +
include/linux/mfd/ti_am335x_tscadc.h | 42 ++--
13 files changed, 582 insertions(+), 302 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
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>
---
.../bindings/mfd/ti,am3359-tscadc.yaml | 75 +++++++++++++++++++
1 file changed, 75 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
This touchscreen controller is already described in a text file:
Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
After introducing a proper description of the MFD, this is the second
step. The file cannot be removed yet as it also contains an ADC
description.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
.../input/touchscreen/ti,am3359-tsc.yaml | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
@@ -0,0 +1,89 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/input/touchscreen/ti,am3359-tsc.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:TI AM3359 Touchscreen controller++maintainers:+-Miquel Raynal <miquel.raynal@bootlin.com>++properties:+compatible:+const:ti,am3359-tsc++ti,wires:+description:Wires refer to application modes i.e. 4/5/8 wire touchscreen+support on the platform.+$ref:/schemas/types.yaml#/definitions/uint32+enum:[4,5,8]++ti,x-plate-resistance:+description:X plate resistance+$ref:/schemas/types.yaml#/definitions/uint32++ti,coordinate-readouts:+description:The sequencer supports a total of 16 programmable steps. Each+step is used to read a single coordinate. A single readout is enough but+multiple reads can increase the quality. A value of 5 means, 5 reads for+X, 5 for Y and 2 for Z (always). This utilises 12 of the 16 software steps+available. The remaining 4 can be used by the ADC.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:1+maximum:6++ti,wire-config:+description:Different boards could have a different order for connecting+wires on touchscreen. We need to provide an 8-bit number where the+first four bits represent the analog lines and the next 4 bits represent+positive/negative terminal on that input line. Notations to represent the+input lines and terminals respectively are as follows, AIN0 = 0, AIN1 = 1+and so on untill AIN7 = 7. XP = 0, XN = 1, YP = 2, YN = 3.+$ref:/schemas/types.yaml#/definitions/uint32-array+minItems:4+maxItems:8++ti,charge-delay:+description:Length of touch screen charge delay step in terms of ADC clock+cycles. Charge delay value should be large in order to avoid false pen-up+events. This value effects the overall sampling speed, hence need to be+kept as low as possible, while avoiding false pen-up event. Start from a+lower value, say 0x400, and increase value until false pen-up events are+avoided. The pen-up detection happens immediately after the charge step,+so this does in fact function as a hardware knob for adjusting the amount+of "settling time".+$ref:/schemas/types.yaml#/definitions/uint32++required:+-compatible+-ti,wires+-ti,x-plate-resistance+-ti,coordinate-readouts+-ti,wire-config++additionalProperties:false++examples:+-|+#include <dt-bindings/interrupt-controller/arm-gic.h>++tscadc:tscadc@0 {+compatible = "ti,am3359-tscadc";+reg = <0x0 0x1000>;+interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;+clocks = <&adc_tsc_fck>;+clock-names = "fck";+status = "disabled";+dmas = <&edma 53 0>, <&edma 57 0>;+dma-names = "fifo0", "fifo1";++tsc {+compatible = "ti,am3359-tsc";+ti,wires = <4>;+ti,x-plate-resistance = <200>;+ti,coordinate-readouts = <5>;+ti,wire-config = <0x00 0x11 0x22 0x33>;+ti,charge-delay = <0x400>;+};+};
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>
---
.../bindings/iio/adc/ti,am3359-adc.yaml | 81 +++++++++++++++++++
1 file changed, 81 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml
@@ -0,0 +1,81 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.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 untill 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:+-|+#include <dt-bindings/interrupt-controller/arm-gic.h>++tscadc:tscadc@0 {+compatible = "ti,am3359-tscadc";+reg = <0x0 0x1000>;+interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;+clocks = <&adc_tsc_fck>;+clock-names = "fck";+status = "disabled";+dmas = <&edma 53 0>, <&edma 57 0>;+dma-names = "fifo0", "fifo1";++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>;+};+};
Now that the three hardware peaces described in this file have been
translated in yaml in 3 dedicated files (MFD, touchscreen controller and
ADC) it is time to remove this text file.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
.../bindings/input/touchscreen/ti-tsc-adc.txt | 91 -------------------
1 file changed, 91 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -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>;- };- }
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>
---
.../devicetree/bindings/mfd/ti,am3359-tscadc.yaml | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
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>
---
Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Drop the text license and replace it with an equivalent SPDX license tag
identifier.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
@@ -140,15 +140,17 @@ 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);return-EINVAL;}}+total_channels=tsc_wires+adc_channels;if(total_channels>8){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;
@@ -170,8 +172,9 @@ static int ti_tscadc_probe(struct platform_device *pdev)if(err<0){dev_err(&pdev->dev,"no irq ID is specified.\n");gotoret;-}else+}else{tscadc->irq=err;+}res=platform_get_resource(pdev,IORESOURCE_MEM,0);tscadc->tscadc_base=devm_ioremap_resource(&pdev->dev,res);
@@ -169,7 +169,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)err=platform_get_irq(pdev,0);if(err<0){dev_err(&pdev->dev,"no irq ID is specified.\n");-gotoret;+returnerr;}else{tscadc->irq=err;}
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>
---
drivers/mfd/ti_am335x_tscadc.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
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>
---
drivers/mfd/ti_am335x_tscadc.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
@@ -124,6 +124,13 @@ static int ti_tscadc_probe(struct platform_device *pdev)inttsc_wires=0,adc_channels=0,readouts=0;inttotal_channels,clock_rate,ctrl,err;+/* 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;
@@ -159,13 +166,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){dev_err(&pdev->dev,"no irq ID is specified.\n");
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>
---
drivers/mfd/ti_am335x_tscadc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
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(-)
@@ -202,7 +202,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)*ThisfrequencyisvalidsinceTSC_ADC_SScontrollerdesign*assumestheOCPclockisatleast6xfasterthantheADCclock.*/-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);
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>
---
drivers/mfd/ti_am335x_tscadc.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
@@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)structclk*clk;u32val;inttsc_wires=0,adc_channels=0,readouts=0;-inttotal_channels,clock_rate,ctrl,err;+inttotal_channels,ctrl,err;/* Allocate memory for device */tscadc=devm_kzalloc(&pdev->dev,sizeof(*tscadc),GFP_KERNEL);
@@ -208,11 +208,8 @@ static int ti_tscadc_probe(struct platform_device *pdev)err=PTR_ERR(clk);gotoerr_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 */
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.
Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
---
drivers/mfd/ti_am335x_tscadc.c | 20 +++++++++++++++-----
include/linux/mfd/ti_am335x_tscadc.h | 9 +++++++++
2 files changed, 24 insertions(+), 5 deletions(-)
@@ -136,6 +136,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);
@@ -209,7 +211,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)gotoerr_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 */
@@ -238,8 +240,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);}
@@ -248,8 +250,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);}
@@ -335,6 +337,14 @@ static int __maybe_unused tscadc_resume(struct device *dev)staticSIMPLE_DEV_PM_OPS(tscadc_pm_ops,tscadc_suspend,tscadc_resume);+staticconststructti_tscadc_datatscdata={+.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,+};+staticconststructof_device_idti_tscadc_dt_ids[]={{.compatible="ti,am3359-tscadc",},{}
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>
---
drivers/mfd/ti_am335x_tscadc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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>
---
drivers/mfd/ti_am335x_tscadc.c | 15 ++++-----------
include/linux/mfd/ti_am335x_tscadc.h | 3 ---
2 files changed, 4 insertions(+), 14 deletions(-)
@@ -177,10 +177,7 @@ struct ti_tscadc_dev {phys_addr_ttscadc_phys_base;conststructti_tscadc_data*data;intirq;-intused_cells;/* 1-2 */inttsc_wires;-inttsc_cell;/* -1 if not used */-intadc_cell;/* -1 if not used */structmfd_cellcells[TSCADC_CELLS];u32reg_se_cache;booladc_waiting;
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>
---
drivers/mfd/ti_am335x_tscadc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
@@ -218,16 +218,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;elsectrl|=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);
@@ -318,12 +318,12 @@ static int __maybe_unused tscadc_resume(struct device *dev)ctrl|=CNTRLREG_5WIRE|CNTRLREG_TSCENB;elsectrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;-tscadc_idle_config(tscadc);}ctrl|=CNTRLREG_TSCSSENB;regmap_write(tscadc->regmap,REG_CTRL,ctrl);regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);+tscadc_idle_config(tscadc);return0;}
Instead of deriving in the probe and in the resume path the value of the
ctrl register, let's do it only once in the probe, save the value of
this register in the driver's structure and use it from the resume
callback.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 31 ++++++++--------------------
include/linux/mfd/ti_am335x_tscadc.h | 2 +-
2 files changed, 10 insertions(+), 23 deletions(-)
@@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)structclk*clk;u32val;inttsc_wires=0,adc_channels=0,readouts=0,cell_idx=0;-inttotal_channels,ctrl,err;+inttotal_channels,err;/* Allocate memory for device */tscadc=devm_kzalloc(&pdev->dev,sizeof(*tscadc),GFP_KERNEL);
@@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;-regmap_write(tscadc->regmap,REG_CTRL,ctrl);+tscadc->ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);if(tsc_wires>0){-tscadc->tsc_wires=tsc_wires;+tscadc->ctrl|=CNTRLREG_TSCENB;if(tsc_wires==5)-ctrl|=CNTRLREG_5WIRE|CNTRLREG_TSCENB;+tscadc->ctrl|=CNTRLREG_5WIRE;else-ctrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;+tscadc->ctrl|=CNTRLREG_4WIRE;}tscadc_idle_config(tscadc);/* Enable the TSC module enable bit */-ctrl|=CNTRLREG_TSCSSENB;-regmap_write(tscadc->regmap,REG_CTRL,ctrl);+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl|CNTRLREG_TSCSSENB);/* TSC Cell */if(tsc_wires>0){
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>
---
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(-)
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>
---
drivers/mfd/ti_am335x_tscadc.c | 10 +++++-----
include/linux/mfd/ti_am335x_tscadc.h | 16 ++++++++--------
2 files changed, 13 insertions(+), 13 deletions(-)
@@ -215,15 +215,15 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-tscadc->ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;+tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);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;}tscadc_idle_config(tscadc);
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>
---
drivers/mfd/ti_am335x_tscadc.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -121,7 +121,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)const__be32*cur;structclk*clk;u32val;-inttsc_wires=0,adc_channels=0,readouts=0,cell_idx=0;+inttscmag_wires=0,adc_channels=0,readouts=0,cell_idx=0;inttotal_channels,err;/* Allocate memory for device */
@@ -139,7 +139,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);node=of_get_child_by_name(pdev->dev.of_node,"adc");
@@ -152,7 +152,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)}}-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;
@@ -218,9 +218,9 @@ static int ti_tscadc_probe(struct platform_device *pdev)tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);-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;elsetscadc->ctrl|=CNTRLREG_TSC_4WIRE;
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 corret this comment.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
@@ -206,7 +206,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);gotoerr_disable_clk;}
Just checking the number of wires will soon not be enough, add a boolean
to indicated the actual use or not of the touchscreen.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
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>
---
drivers/mfd/ti_am335x_tscadc.c | 37 +++++++++++++++++-----------
include/linux/mfd/ti_am335x_tscadc.h | 1 +
2 files changed, 23 insertions(+), 15 deletions(-)
@@ -139,11 +140,13 @@ 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);-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);+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){
@@ -218,15 +221,18 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;-regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);+tscadc->ctrl=CNTRLREG_STEPID;+if(tscadc->data->has_tsc){+tscadc->ctrl|=CNTRLREG_TSC_STEPCONFIGWRT;+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);+if(use_tsc){+tscadc->ctrl|=CNTRLREG_TSC_ENB;+if(tscmag_wires==5)+tscadc->ctrl|=CNTRLREG_TSC_5WIRE;+else+tscadc->ctrl|=CNTRLREG_TSC_4WIRE;+}-if(use_tsc){-tscadc->ctrl|=CNTRLREG_TSC_ENB;-if(tscmag_wires==5)-tscadc->ctrl|=CNTRLREG_TSC_5WIRE;-else-tscadc->ctrl|=CNTRLREG_TSC_4WIRE;}tscadc_idle_config(tscadc);
@@ -321,6 +327,7 @@ static int __maybe_unused tscadc_resume(struct device *dev)staticSIMPLE_DEV_PM_OPS(tscadc_pm_ops,tscadc_suspend,tscadc_resume);staticconststructti_tscadc_datatscdata={+.has_tsc=true,.name_tscmag="TI-am335x-tsc",.compat_tscmag="ti,am3359-tsc",.name_adc="TI-am335x-adc",
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>
---
drivers/mfd/ti_am335x_tscadc.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
The datasheet states that most of the configuration should be set in the
control register in the first place, before actually enabling the
hardware. So far only half of the configuration was made in the first
step, which does not make really sense and would complicating the code
when introducing support for the am437x hardware.
Let's move that register write a bit below to enclose more configuration.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Introduce a new compatible that has another set of driver data,
targeting am437x SoCs with a magnetic reader instead of the
touchscreen and a more featureful set of registers.
Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
---
drivers/mfd/ti_am335x_tscadc.c | 43 ++++++++++++++++++++++------
include/linux/mfd/ti_am335x_tscadc.h | 9 +++++-
2 files changed, 43 insertions(+), 9 deletions(-)
Drop the text license and replace it with an equivalent SPDX license tag
identifier.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/iio/adc/ti_am335x_adc.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
@@ -246,6 +244,7 @@ static int tiadc_start_dma(struct iio_dev *indio_dev)structdma_async_tx_descriptor*desc;dma->current_period=0;/* We start to fill period 0 */+/**Makethefifothreshasthemultipleoftotalnumberof*channelsenabled,somakesurethatcyclicDMAperiod
@@ -255,9 +254,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);
@@ -288,9 +288,9 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev)structtiadc_device*adc_dev=iio_priv(indio_dev);inti,fifo1count;-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);
@@ -320,8 +320,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)
@@ -337,8 +338,9 @@ static int tiadc_buffer_predisable(struct iio_dev *indio_dev)structtiadc_dma*dma=&adc_dev->dma;intfifo1count,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;
@@ -459,9 +459,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)
@@ -473,6 +472,7 @@ static int tiadc_read_raw(struct iio_dev *indio_dev,gotoerr_unlock;}}+map_val=adc_dev->channel_step[chan->scan_index];/*
@@ -490,13 +490,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);
@@ -537,6 +538,7 @@ static int tiadc_request_dma(struct platform_device *pdev,gotoerr;return0;+err:dma_release_channel(dma->chan);return-ENOMEM;
@@ -586,7 +588,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;}
@@ -608,12 +610,11 @@ static int tiadc_probe(struct platform_device *pdev)returnerr;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)gotoerr_free_channels;
@@ -664,8 +665,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);return0;}
@@ -678,12 +678,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);return0;}
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>
---
drivers/iio/adc/ti_am335x_adc.c | 2 +-
include/linux/mfd/ti_am335x_tscadc.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
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>
---
drivers/iio/adc/ti_am335x_adc.c | 47 +++++++++++++++++++--------------
1 file changed, 27 insertions(+), 20 deletions(-)
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>
---
drivers/iio/adc/ti_am335x_adc.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
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>
---
drivers/iio/adc/ti_am335x_adc.c | 1 +
1 file changed, 1 insertion(+)
Back in 2014 a property got mispelled "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(-)
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(-)
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(+)
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>
---
drivers/iio/adc/ti_am335x_adc.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:28:31
On Wed, 25 Aug 2021 17:24:42 +0200
Miquel Raynal [off-list ref] wrote:
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>
Hi Miquel,
We'd do this different today, but this seems to be an accurate conversion
of what we have. I'll assume this series may well go via mfd, so
Acked-by: Jonathan Cameron <redacted>
@@ -0,0 +1,81 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.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 untill 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:+-|+#include <dt-bindings/interrupt-controller/arm-gic.h>++tscadc:tscadc@0 {+compatible = "ti,am3359-tscadc";+reg = <0x0 0x1000>;+interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;+clocks = <&adc_tsc_fck>;+clock-names = "fck";+status = "disabled";+dmas = <&edma 53 0>, <&edma 57 0>;+dma-names = "fifo0", "fifo1";++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>;+};+};
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:31:29
On Wed, 25 Aug 2021 17:24:44 +0200
Miquel Raynal [off-list ref] wrote:
quoted hunk
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>
---
.../devicetree/bindings/mfd/ti,am3359-tscadc.yaml | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:32:04
On Wed, 25 Aug 2021 17:24:45 +0200
Miquel Raynal [off-list ref] wrote:
quoted hunk
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>
---
Documentation/devicetree/bindings/iio/adc/ti,am3359-adc.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:35:14
On Wed, 25 Aug 2021 17:24:46 +0200
Miquel Raynal [off-list ref] wrote:
Drop the text license and replace it with an equivalent SPDX license tag
identifier.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Matches with the MODULE_LICENSE(GPL) so make sense to me.
Reviewed-by: Jonathan Cameron <redacted>
I wonder how this one managed to escape being updated before? Ah well..
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:36:02
On Wed, 25 Aug 2021 17:24:43 +0200
Miquel Raynal [off-list ref] 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 <redacted>
Nice to have another one of these go away :)
@@ -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>;- };- }
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:44:10
On Wed, 25 Aug 2021 17:24:40 +0200
Miquel Raynal [off-list ref] wrote:
There is a very light description of this MFD in a text file dedicated
to a touchscreen controller (which is one of the two children of the
MFD). Here is now a complete yaml description.
Make sure to call out places where you are filling in gaps in the
txt file description. Looks like we forgot to keep that up to date as the driver
evolved. oops.
text documents the more specific ti,am654-tscadc as well.
Something like
properties:
compatible:
oneof:
- const: ti,am3359-tscadc
- items:
- const: ti,am654-tscadc
- const: ti,am3359-tscadc
Note this superceeds my comment on later patches about changing to enum when
introducing the new compatible. Ah well.
example for this would be something like
Documentation/devicetree/bindings/iio/adc/amlogic,meson-saradc.yam;
@@ -140,15 +140,17 @@ 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);return-EINVAL;}}+total_channels=tsc_wires+adc_channels;if(total_channels>8){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;
@@ -170,8 +172,9 @@ static int ti_tscadc_probe(struct platform_device *pdev)if(err<0){dev_err(&pdev->dev,"no irq ID is specified.\n");gotoret;-}else+}else{tscadc->irq=err;+}res=platform_get_resource(pdev,IORESOURCE_MEM,0);tscadc->tscadc_base=devm_ioremap_resource(&pdev->dev,res);
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 12:49:12
On Wed, 25 Aug 2021 17:24:48 +0200
Miquel Raynal [off-list ref] wrote:
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>
I agree with dropping the spaces, but not so much with reordering the elements.
Whilst it obviously doesn't make any difference to padding etc, it does
require more thought that I'd want to expend reviewing a patch like this
to establish whether there was a semantic meaning to the original order that
we are disrupting.
Jonathan
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:33:02
On Wed, 25 Aug 2021 17:24:41 +0200
Miquel Raynal [off-list ref] wrote:
quoted hunk
This touchscreen controller is already described in a text file:
Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
After introducing a proper description of the MFD, this is the second
step. The file cannot be removed yet as it also contains an ADC
description.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
.../input/touchscreen/ti,am3359-tsc.yaml | 89 +++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/ti,am3359-tsc.yaml
@@ -0,0 +1,89 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/input/touchscreen/ti,am3359-tsc.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:TI AM3359 Touchscreen controller++maintainers:+-Miquel Raynal <miquel.raynal@bootlin.com>++properties:+compatible:+const:ti,am3359-tsc++ti,wires:+description:Wires refer to application modes i.e. 4/5/8 wire touchscreen+support on the platform.+$ref:/schemas/types.yaml#/definitions/uint32+enum:[4,5,8]++ti,x-plate-resistance:+description:X plate resistance+$ref:/schemas/types.yaml#/definitions/uint32++ti,coordinate-readouts:+description:The sequencer supports a total of 16 programmable steps. Each+step is used to read a single coordinate. A single readout is enough but+multiple reads can increase the quality. A value of 5 means, 5 reads for+X, 5 for Y and 2 for Z (always). This utilises 12 of the 16 software steps+available. The remaining 4 can be used by the ADC.+$ref:/schemas/types.yaml#/definitions/uint32+minimum:1+maximum:6++ti,wire-config:+description:Different boards could have a different order for connecting+wires on touchscreen. We need to provide an 8-bit number where the+first four bits represent the analog lines and the next 4 bits represent+positive/negative terminal on that input line. Notations to represent the+input lines and terminals respectively are as follows, AIN0 = 0, AIN1 = 1+and so on untill AIN7 = 7. XP = 0, XN = 1, YP = 2, YN = 3.+$ref:/schemas/types.yaml#/definitions/uint32-array+minItems:4+maxItems:8++ti,charge-delay:+description:Length of touch screen charge delay step in terms of ADC clock+cycles. Charge delay value should be large in order to avoid false pen-up+events. This value effects the overall sampling speed, hence need to be+kept as low as possible, while avoiding false pen-up event. Start from a+lower value, say 0x400, and increase value until false pen-up events are+avoided. The pen-up detection happens immediately after the charge step,+so this does in fact function as a hardware knob for adjusting the amount+of "settling time".+$ref:/schemas/types.yaml#/definitions/uint32++required:+-compatible+-ti,wires
Are these all required? Seems like the driver will work fine without some
of them and isn't doing appropriate error checking for their presence..
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:36:48
On Wed, 25 Aug 2021 17:24:49 +0200
Miquel Raynal [off-list ref] wrote:
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 <redacted>
As a side note, I'm not seeing the expected calls to of_put_node() for
the child nodes. So we are leaking some stuff on those calls.
Fixing that might make this patch no longer correct.
@@ -169,7 +169,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)err=platform_get_irq(pdev,0);if(err<0){dev_err(&pdev->dev,"no irq ID is specified.\n");-gotoret;+returnerr;}else{tscadc->irq=err;}
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:40:18
On Wed, 25 Aug 2021 17:24:51 +0200
Miquel Raynal [off-list ref] wrote:
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>
Fair enough. It does seem a little clearer to me after your rewording.
Reviewed-by: Jonathan Cameron <redacted>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:44:44
On Wed, 25 Aug 2021 17:24:53 +0200
Miquel Raynal [off-list ref] wrote:
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>
@@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)structclk*clk;u32val;inttsc_wires=0,adc_channels=0,readouts=0;-inttotal_channels,clock_rate,ctrl,err;+inttotal_channels,ctrl,err;/* Allocate memory for device */tscadc=devm_kzalloc(&pdev->dev,sizeof(*tscadc),GFP_KERNEL);
@@ -208,11 +208,8 @@ static int ti_tscadc_probe(struct platform_device *pdev)err=PTR_ERR(clk);gotoerr_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 */
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:53:06
On Wed, 25 Aug 2021 17:24:58 +0200
Miquel Raynal [off-list ref] wrote:
Instead of deriving in the probe and in the resume path the value of the
ctrl register, let's do it only once in the probe, save the value of
this register in the driver's structure and use it from the resume
callback.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
@@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)structclk*clk;u32val;inttsc_wires=0,adc_channels=0,readouts=0,cell_idx=0;-inttotal_channels,ctrl,err;+inttotal_channels,err;/* Allocate memory for device */tscadc=devm_kzalloc(&pdev->dev,sizeof(*tscadc),GFP_KERNEL);
@@ -215,22 +215,21 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;-regmap_write(tscadc->regmap,REG_CTRL,ctrl);+tscadc->ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);if(tsc_wires>0){-tscadc->tsc_wires=tsc_wires;+tscadc->ctrl|=CNTRLREG_TSCENB;if(tsc_wires==5)-ctrl|=CNTRLREG_5WIRE|CNTRLREG_TSCENB;+tscadc->ctrl|=CNTRLREG_5WIRE;else-ctrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;+tscadc->ctrl|=CNTRLREG_4WIRE;}tscadc_idle_config(tscadc);/* Enable the TSC module enable bit */-ctrl|=CNTRLREG_TSCSSENB;-regmap_write(tscadc->regmap,REG_CTRL,ctrl);+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl|CNTRLREG_TSCSSENB);/* TSC Cell */if(tsc_wires>0){
As the value of tscadc->ctrl is not the same as REG_CTRL this is a bit non obvious.
You might be better off keeping them in sync, but masking that bit out and then resetting
it as appropriate.
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:55:25
On Wed, 25 Aug 2021 17:25:02 +0200
Miquel Raynal [off-list ref] wrote:
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 corret this comment.
@@ -206,7 +206,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);gotoerr_disable_clk;}
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:56:55
On Wed, 25 Aug 2021 17:25:03 +0200
Miquel Raynal [off-list ref] wrote:
quoted hunk
Just checking the number of wires will soon not be enough, add a boolean
to indicated the actual use or not of the touchscreen.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
drivers/mfd/ti_am335x_tscadc.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 13:59:01
On Wed, 25 Aug 2021 17:25:04 +0200
Miquel Raynal [off-list ref] wrote:
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>
Seems sensible to me.
Reviewed-by: Jonathan Cameron <redacted>
@@ -139,11 +140,13 @@ 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);-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);+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){
@@ -218,15 +221,18 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;-regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);+tscadc->ctrl=CNTRLREG_STEPID;+if(tscadc->data->has_tsc){+tscadc->ctrl|=CNTRLREG_TSC_STEPCONFIGWRT;+regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);+if(use_tsc){+tscadc->ctrl|=CNTRLREG_TSC_ENB;+if(tscmag_wires==5)+tscadc->ctrl|=CNTRLREG_TSC_5WIRE;+else+tscadc->ctrl|=CNTRLREG_TSC_4WIRE;+}-if(use_tsc){-tscadc->ctrl|=CNTRLREG_TSC_ENB;-if(tscmag_wires==5)-tscadc->ctrl|=CNTRLREG_TSC_5WIRE;-else-tscadc->ctrl|=CNTRLREG_TSC_4WIRE;}tscadc_idle_config(tscadc);
@@ -321,6 +327,7 @@ static int __maybe_unused tscadc_resume(struct device *dev)staticSIMPLE_DEV_PM_OPS(tscadc_pm_ops,tscadc_suspend,tscadc_resume);staticconststructti_tscadc_datatscdata={+.has_tsc=true,.name_tscmag="TI-am335x-tsc",.compat_tscmag="ti,am3359-tsc",.name_adc="TI-am335x-adc",
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:00:15
On Wed, 25 Aug 2021 17:25:05 +0200
Miquel Raynal [off-list ref] wrote:
The datasheet states that most of the configuration should be set in the
control register in the first place, before actually enabling the
hardware.
Reference always good.
So far only half of the configuration was made in the first
step, which does not make really sense and would complicating the code
when introducing support for the am437x hardware.
Let's move that register write a bit below to enclose more configuration.
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:07:08
On Wed, 25 Aug 2021 17:25:06 +0200
Miquel Raynal [off-list ref] 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.
Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
There is one bit in here that I think should have been back in patch 16 but
maybe I'm missing something.
Jonathan
Interesting to see match data added here and not before.
Given you don't have any code in here that seems to have
changed to use the match data, was it buggy before or is this still
not used?
Not sure on current status, but there is a proposed series floating
about that adds HZ_PER_MEGAHZ or something like that which would make
it easier to spot if these have right number of zeros.
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:08:21
On Wed, 25 Aug 2021 17:25:07 +0200
Miquel Raynal [off-list ref] 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>
@@ -246,6 +244,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
@@ -255,9 +254,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);
@@ -288,9 +288,9 @@ static int tiadc_buffer_preenable(struct iio_dev *indio_dev) struct tiadc_device *adc_dev = iio_priv(indio_dev); int i, fifo1count;- 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);
@@ -411,12 +412,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];
@@ -434,8 +434,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 ret = IIO_VAL_INT;
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:16:22
On Wed, 25 Aug 2021 17:25:11 +0200
Miquel Raynal [off-list ref] wrote:
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>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:17:06
On Wed, 25 Aug 2021 17:25:12 +0200
Miquel Raynal [off-list ref] wrote:
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>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:19:17
On Wed, 25 Aug 2021 17:25:13 +0200
Miquel Raynal [off-list ref] wrote:
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.
Interesting and non obvious... Good work figuring that out.
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>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:20:46
On Wed, 25 Aug 2021 17:25:14 +0200
Miquel Raynal [off-list ref] wrote:
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>
Odd no one did this before, but such is life ;)
Acked-by: Jonathan Cameron <redacted>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:21:18
On Wed, 25 Aug 2021 17:25:15 +0200
Miquel Raynal [off-list ref] wrote:
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>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:22:08
On Wed, 25 Aug 2021 17:24:50 +0200
Miquel Raynal [off-list ref] wrote:
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>
No problem with this, but I would expect either this patch or the one
immediately after it to actually take advantage of this being
available earlier in probe().
As things stand it's not clear to a reviewer that this is actually
a useful bit of refactoring.
@@ -124,6 +124,13 @@ static int ti_tscadc_probe(struct platform_device *pdev)inttsc_wires=0,adc_channels=0,readouts=0;inttotal_channels,clock_rate,ctrl,err;+/* 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;
@@ -159,13 +166,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){dev_err(&pdev->dev,"no irq ID is specified.\n");
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:43:27
On Wed, 25 Aug 2021 17:24:54 +0200
Miquel Raynal [off-list ref] wrote:
quoted hunk
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.
Co-developed-by: Jason Reeder <redacted>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Signed-off-by: Jason Reeder <redacted>
---
drivers/mfd/ti_am335x_tscadc.c | 20 +++++++++++++++-----
include/linux/mfd/ti_am335x_tscadc.h | 9 +++++++++
2 files changed, 24 insertions(+), 5 deletions(-)
@@ -136,6 +136,8 @@ static int ti_tscadc_probe(struct platform_device *pdev)return-EINVAL;}+tscadc->data=of_device_get_match_data(&pdev->dev);
As mentioned in later patch, I can't see that you actually wired this up here.
match_data doesn't seem to be set.
Otherwise looks fine to me.
Reviewed-by: Jonathan Cameron <redacted>
@@ -218,16 +218,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;elsectrl|=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);
@@ -318,12 +318,12 @@ static int __maybe_unused tscadc_resume(struct device *dev)ctrl|=CNTRLREG_5WIRE|CNTRLREG_TSCENB;elsectrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;-tscadc_idle_config(tscadc);}ctrl|=CNTRLREG_TSCSSENB;regmap_write(tscadc->regmap,REG_CTRL,ctrl);regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);+tscadc_idle_config(tscadc);return0;}
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:47:42
On Wed, 25 Aug 2021 17:24:59 +0200
Miquel Raynal [off-list ref] wrote:
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>
I don't feel strongly either way on this one but if you feel it's a good idea fair enough.
Acked-by: Jonathan Cameron <redacted>
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:49:51
On Wed, 25 Aug 2021 17:25:00 +0200
Miquel Raynal [off-list ref] wrote:
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>
A few things inline that aren't strictly related to this patch, but might be
nice to tidy up whilst you are here.
Jonathan
@@ -215,15 +215,15 @@ static int ti_tscadc_probe(struct platform_device *pdev)regmap_write(tscadc->regmap,REG_CLKDIV,tscadc->clk_div);/* Set the control register bits */-tscadc->ctrl=CNTRLREG_STEPCONFIGWRT|CNTRLREG_STEPID;+tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);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;}tscadc_idle_config(tscadc);
From: Jonathan Cameron <jic23@kernel.org> Date: 2021-08-30 14:50:44
On Wed, 25 Aug 2021 17:25:01 +0200
Miquel Raynal [off-list ref] wrote:
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>
Seems sensible.
Reviewed-by: Jonathan Cameron <redacted>
@@ -121,7 +121,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)const__be32*cur;structclk*clk;u32val;-inttsc_wires=0,adc_channels=0,readouts=0,cell_idx=0;+inttscmag_wires=0,adc_channels=0,readouts=0,cell_idx=0;inttotal_channels,err;/* Allocate memory for device */
@@ -139,7 +139,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);node=of_get_child_by_name(pdev->dev.of_node,"adc");
@@ -152,7 +152,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)}}-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;
@@ -218,9 +218,9 @@ static int ti_tscadc_probe(struct platform_device *pdev)tscadc->ctrl=CNTRLREG_TSC_STEPCONFIGWRT|CNTRLREG_STEPID;regmap_write(tscadc->regmap,REG_CTRL,tscadc->ctrl);-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;elsetscadc->ctrl|=CNTRLREG_TSC_4WIRE;
From: Rob Herring <robh@kernel.org> Date: 2021-08-31 19:33:27
On Wed, Aug 25, 2021 at 05:24:40PM +0200, Miquel Raynal wrote:
quoted hunk
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>
---
.../bindings/mfd/ti,am3359-tscadc.yaml | 75 +++++++++++++++++++
1 file changed, 75 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/ti,am3359-tscadc.yaml
@@ -0,0 +1,75 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/mfd/ti,am3359-tscadc.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:TI AM3359 Touchscreen controller/ADC++maintainers:+-Miquel Raynal <miquel.raynal@bootlin.com>++properties:+compatible:+const:ti,am3359-tscadc++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++patternProperties:+"^adc$":+description:ADC++"^tsc$":+description:Touchscreen controller
These are fixed strings, so move to 'properties'. They also need 'type:
object' to indicate they are nodes.