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
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
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
@@ -0,0 +1,76 @@+# 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 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>;+};
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
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
@@ -0,0 +1,68 @@+# 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 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>;+};
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
@@ -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>;- };- }
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
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(+)
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(-)
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(+)
@@ -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");
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(-)
@@ -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;
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(-)
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(-)
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(-)
@@ -206,7 +206,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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/mfd/ti_am335x_tscadc.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
@@ -122,7 +122,6 @@ static int ti_tscadc_probe(struct platform_device *pdev)const__be32*cur;u32val;interr,ctrl;-intclock_rate;inttsc_wires=0,adc_channels=0,total_channels;intreadouts=0;
@@ -212,11 +211,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 */
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(-)
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(-)
@@ -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)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 */
@@ -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)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",},+{+.compatible="ti,am3359-tscadc",+.data=&tscdata,+},{}};MODULE_DEVICE_TABLE(of,ti_tscadc_dt_ids);
From: Lee Jones <hidden> Date: 2021-09-22 15:01:58
On Wed, 15 Sep 2021, Miquel Raynal 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.
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(-)
@@ -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)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 */
@@ -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)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",},+{+.compatible="ti,am3359-tscadc",+.data=&tscdata,+},{}};MODULE_DEVICE_TABLE(of,ti_tscadc_dt_ids);
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
Hi Lee,
lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:01:51 +0100:
On Wed, 15 Sep 2021, Miquel Raynal wrote:
quoted
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(-)
@@ -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)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 */
@@ -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)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",},+{+.compatible="ti,am3359-tscadc",+.data=&tscdata,+},{}};MODULE_DEVICE_TABLE(of,ti_tscadc_dt_ids);
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!
quoted
+ 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;
From: Lee Jones <hidden> Date: 2021-09-22 15:54:24
On Wed, 22 Sep 2021, Miquel Raynal wrote:
Hi Lee,
lee.jones@linaro.org wrote on Wed, 22 Sep 2021 16:01:51 +0100:
quoted
On Wed, 15 Sep 2021, Miquel Raynal wrote:
quoted
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(-)
@@ -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)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 */
@@ -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)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",},+{+.compatible="ti,am3359-tscadc",+.data=&tscdata,+},{}};MODULE_DEVICE_TABLE(of,ti_tscadc_dt_ids);
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.
quoted
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.
quoted
What is a magnetic reader anyway?
Does it read the magnetic stripe on a payment card?
Yes!
The mag_stripe_reader might be nice.
quoted
quoted
+ 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
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(-)
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(-)
@@ -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>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
drivers/mfd/ti_am335x_tscadc.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
@@ -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;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);
@@ -321,8 +321,10 @@ static int __maybe_unused tscadc_resume(struct device *dev)ctrl|=CNTRLREG_5WIRE|CNTRLREG_TSCENB;elsectrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;-tscadc_idle_config(tscadc);}++tscadc_idle_config(tscadc);+ctrl|=CNTRLREG_TSCSSENB;regmap_write(tscadc->regmap,REG_CTRL,ctrl);
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(-)
@@ -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)elsectrl|=CNTRLREG_4WIRE|CNTRLREG_TSCENB;}+regmap_write(tscadc->regmap,REG_CTRL,ctrl);tscadc_idle_config(tscadc);
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(-)
@@ -121,7 +121,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)structproperty*prop;const__be32*cur;u32val;-interr,ctrl;+interr;inttsc_wires=0,adc_channels=0,cell_idx=0,total_channels;intreadouts=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;+/*+*Setthecontrolregisterbits.tscadc->ctrlstorestheconfiguration+*oftheCTRLregisterbutnotthesubsystemenablebitwhichmustbe+*addedmanuallywhentimely.+*/+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){
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(-)
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(-)
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>
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(-)
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>
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!
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(-)
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
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(-)
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)
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
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!
quoted
"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 :)
quoted
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)
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
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(-)
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(-)
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(-)
@@ -122,7 +122,7 @@ static int ti_tscadc_probe(struct platform_device *pdev)const__be32*cur;u32val;interr;-inttsc_wires=0,adc_channels=0,cell_idx=0,total_channels;+inttscmag_wires=0,adc_channels=0,cell_idx=0,total_channels;intreadouts=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)*addedmanuallywhentimely.*/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;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 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(-)
@@ -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);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.
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(-)
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(-)
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(-)
From: Lee Jones <hidden> Date: 2021-09-22 16:00:26
On Wed, 15 Sep 2021, Miquel Raynal wrote:
quoted hunk
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(-)
Hi Lee,
lee.jones@linaro.org wrote on Wed, 22 Sep 2021 17:00:20 +0100:
On Wed, 15 Sep 2021, Miquel Raynal wrote:
quoted
Introduce a new compatible that has another set of driver data,
targeting am437x SoCs with a magnetic reader instead of the
touchscreen and a more featureful set of registers.
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(-)
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.
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.
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