[PATCH] ARM: dts: am33xx-l4: Add missing touchscreen clock properties

[PATCH] ARM: dts: am33xx-l4: Add missing touchscreen clock properties

[Miquel Raynal]

When adding support for TI magadc, the MFD driver (common to the
touchscreen and the ADC) got updated to ease the insertion of a new DT
node for the ADC, with its own compatible, clocks, etc. Commit
235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our
clock") removed one compatible specific information which was the clock
name, because the clock was looked up from scratch in the DT while this
hardware block was only fed by a single clock, already defined and
properly filled in the DT.

Problem is, this change was only validated with an am437x-based board,
where the clocks are effectively correctly defined and referenced. But
on am33xx, the ADC clock is also correctly defined but is not referenced
with a clock phandle as it out to be.

The touchscreen bindings clearly state that the clocks/clock-names
properties are mandatory, but they have been forgotten in one DTSI. This
was probably not noticed in the first place because of the clock
actually existing and the clk_get() call going through all the tree
anyway.

Add the missing clock phandles in the am33xx touchscreen description.

Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
Fixes: 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our clock")
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---

Hello Nikolaus, as I told you I don't have the relevant hardware to
verify that this actually fixes your situation but I am rather
confident. Could you please give this a try?
Thanks! Miquel

 arch/arm/boot/dts/am33xx-l4.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index c9629cb5ccd1..7da42a5b959c 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -263,6 +263,8 @@ tscadc: tscadc@0 {
 				compatible = "ti,am3359-tscadc";
 				reg = <0x0 0x1000>;
 				interrupts = <16>;
+				clocks = <&adc_tsc_fck>;
+				clock-names = "fck";
 				status = "disabled";
 				dmas = <&edma 53 0>, <&edma 57 0>;
 				dma-names = "fifo0", "fifo1";
-- 
2.27.0

Re: [PATCH] ARM: dts: am33xx-l4: Add missing touchscreen clock properties

[H. Nikolaus Schaller]

Hi Miquel,
some tiny typos...

> Am 07.03.2022 um 12:14 schrieb Miquel Raynal <miquel.raynal@bootlin.com>:
> 
> When adding support for TI magadc, the MFD driver (common to the

"magadc"?

> touchscreen and the ADC) got updated to ease the insertion of a new DT
> node for the ADC, with its own compatible, clocks, etc. Commit
> 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our
> clock") removed one compatible specific information which was the clock
> name, because the clock was looked up from scratch in the DT while this
> hardware block was only fed by a single clock, already defined and
> properly filled in the DT.
> 
> Problem is, this change was only validated with an am437x-based board,
> where the clocks are effectively correctly defined and referenced. But
> on am33xx, the ADC clock is also correctly defined but is not referenced
> with a clock phandle as it out to be.

maybe you mean "ought to be"?

> 
> The touchscreen bindings clearly state that the clocks/clock-names
> properties are mandatory, but they have been forgotten in one DTSI. This
> was probably not noticed in the first place because of the clock
> actually existing and the clk_get() call going through all the tree
> anyway.
> 
> Add the missing clock phandles in the am33xx touchscreen description.

Yes, makes touch on BeagleBoard Black with Chipsee 4"3 panel work again!

> 
> Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
> Fixes: 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our clock")
Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> 
> Hello Nikolaus, as I told you I don't have the relevant hardware to
> verify that this actually fixes your situation but I am rather
> confident. Could you please give this a try?
> Thanks! Miquel

> 
> arch/arm/boot/dts/am33xx-l4.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> index c9629cb5ccd1..7da42a5b959c 100644
> --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> @@ -263,6 +263,8 @@ tscadc: tscadc@0 {
> 				compatible = "ti,am3359-tscadc";
> 				reg = <0x0 0x1000>;
> 				interrupts = <16>;
> +				clocks = <&adc_tsc_fck>;
> +				clock-names = "fck";
> 				status = "disabled";
> 				dmas = <&edma 53 0>, <&edma 57 0>;
> 				dma-names = "fifo0", "fifo1";
> -- 
> 2.27.0
> 

BR and thanks,
Nikolaus

Re: [PATCH] ARM: dts: am33xx-l4: Add missing touchscreen clock properties

[Miquel Raynal]

Hi H.,

hns@goldelico.com wrote on Mon, 7 Mar 2022 20:19:48 +0100:

> Hi Miquel,
> some tiny typos...
> 
> > Am 07.03.2022 um 12:14 schrieb Miquel Raynal <miquel.raynal@bootlin.com>:
> > 
> > When adding support for TI magadc, the MFD driver (common to the  
> 
> "magadc"?

It's actually the name of the hardware block. It stands for 'magnetic
stripe reader and/or ADC', very much like the first ADC which has a
specific Touchscreen hardware feature as well. You can wire X lines to
a touchscreen, and TOTAL - X lines to the ADC, same applies to the
magnetic stripe reader.

I can s|magadc|Magnetic Stripe Reader/second ADC| to clarify.

> > touchscreen and the ADC) got updated to ease the insertion of a new DT
> > node for the ADC, with its own compatible, clocks, etc. Commit
> > 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our
> > clock") removed one compatible specific information which was the clock
> > name, because the clock was looked up from scratch in the DT while this
> > hardware block was only fed by a single clock, already defined and
> > properly filled in the DT.
> > 
> > Problem is, this change was only validated with an am437x-based board,
> > where the clocks are effectively correctly defined and referenced. But
> > on am33xx, the ADC clock is also correctly defined but is not referenced
> > with a clock phandle as it out to be.  
> 
> maybe you mean "ought to be"?

I knew there was something wrong with it, but not what exactly :)

> > The touchscreen bindings clearly state that the clocks/clock-names
> > properties are mandatory, but they have been forgotten in one DTSI. This
> > was probably not noticed in the first place because of the clock
> > actually existing and the clk_get() call going through all the tree
> > anyway.
> > 
> > Add the missing clock phandles in the am33xx touchscreen description.  
> 
> Yes, makes touch on BeagleBoard Black with Chipsee 4"3 panel work again!
> 
> > 
> > Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
> > Fixes: 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our clock")  
> Tested-by: H. Nikolaus Schaller <hns@goldelico.com>

Thanks!
Miquèl

> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> > 
> > Hello Nikolaus, as I told you I don't have the relevant hardware to
> > verify that this actually fixes your situation but I am rather
> > confident. Could you please give this a try?
> > Thanks! Miquel  
> 
> > 
> > arch/arm/boot/dts/am33xx-l4.dtsi | 2 ++
> > 1 file changed, 2 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
> > index c9629cb5ccd1..7da42a5b959c 100644
> > --- a/arch/arm/boot/dts/am33xx-l4.dtsi
> > +++ b/arch/arm/boot/dts/am33xx-l4.dtsi
> > @@ -263,6 +263,8 @@ tscadc: tscadc@0 {
> > 				compatible = "ti,am3359-tscadc";
> > 				reg = <0x0 0x1000>;
> > 				interrupts = <16>;
> > +				clocks = <&adc_tsc_fck>;
> > +				clock-names = "fck";
> > 				status = "disabled";
> > 				dmas = <&edma 53 0>, <&edma 57 0>;
> > 				dma-names = "fifo0", "fifo1";
> > -- 
> > 2.27.0
> >   
> 
> BR and thanks,
> Nikolaus
> 
> 

Re: [PATCH] ARM: dts: am33xx-l4: Add missing touchscreen clock properties

[H. Nikolaus Schaller]

Hi Miquel,

> Am 08.03.2022 um 10:48 schrieb Miquel Raynal <miquel.raynal@bootlin.com>:
> 
> Hi H.,
> 
> hns@goldelico.com wrote on Mon, 7 Mar 2022 20:19:48 +0100:
> 
>> Hi Miquel,
>> some tiny typos...
>> 
>>> Am 07.03.2022 um 12:14 schrieb Miquel Raynal <miquel.raynal@bootlin.com>:
>>> 
>>> When adding support for TI magadc, the MFD driver (common to the  
>> 
>> "magadc"?
> 
> It's actually the name of the hardware block. It stands for 'magnetic
> stripe reader and/or ADC', very much like the first ADC which has a
> specific Touchscreen hardware feature as well. You can wire X lines to
> a touchscreen, and TOTAL - X lines to the ADC, same applies to the
> magnetic stripe reader.
> 
> I can s|magadc|Magnetic Stripe Reader/second ADC| to clarify.

Ok, I see. I had googled and grepped through the kernel logs and found
no hint about "magadc". But if support is newly added, this explains.

> 
>>> touchscreen and the ADC) got updated to ease the insertion of a new DT
>>> node for the ADC, with its own compatible, clocks, etc. Commit
>>> 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our
>>> clock") removed one compatible specific information which was the clock
>>> name, because the clock was looked up from scratch in the DT while this
>>> hardware block was only fed by a single clock, already defined and
>>> properly filled in the DT.
>>> 
>>> Problem is, this change was only validated with an am437x-based board,
>>> where the clocks are effectively correctly defined and referenced. But
>>> on am33xx, the ADC clock is also correctly defined but is not referenced
>>> with a clock phandle as it out to be.  
>> 
>> maybe you mean "ought to be"?
> 
> I knew there was something wrong with it, but not what exactly :)

That is what peer review is good for...

> 
>>> The touchscreen bindings clearly state that the clocks/clock-names
>>> properties are mandatory, but they have been forgotten in one DTSI. This
>>> was probably not noticed in the first place because of the clock
>>> actually existing and the clk_get() call going through all the tree
>>> anyway.
>>> 
>>> Add the missing clock phandles in the am33xx touchscreen description.  
>> 
>> Yes, makes touch on BeagleBoard Black with Chipsee 4"3 panel work again!
>> 
>>> 
>>> Reported-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> Fixes: 235a96e92c16 ("mfd: ti_am335x_tscadc: Don't search the tree for our clock")  
>> Tested-by: H. Nikolaus Schaller <hns@goldelico.com>
> 
> Thanks!
> Miquèl

BR and thanks,
Nikolaus