Re: [PATCH 1/2] ARM: dts: r8a7740: Add LCDC nodes

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Magnus Damm <magnus.damm@gmail.com>,
	linux-renesas-soc@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/2] ARM: dts: r8a7740: Add LCDC nodes
Date: Fri, 23 Jun 2023 17:07:44 +0200	[thread overview]
Message-ID: <CAMuHMdU8Pc3oJGkoN5RiQFhXtqnmYa3jheS-FMO7G7SQPTtmHg@mail.gmail.com> (raw)
In-Reply-To: <20230623130318.GA2112@pendragon.ideasonboard.com>

Hi Laurent,

On Fri, Jun 23, 2023 at 3:03 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, Jun 22, 2023 at 11:23:14AM +0200, Geert Uytterhoeven wrote:
> > Add device nodes for the two LCD Controllers (LCDC) on the R-Mobile A1
> > SoC, and for the two optional external LCDL clock inputs.
> >
> > Note that the HDMI clock for LCDC1 is not added, as this clock is not
> > yet supported.
> >
> > Based on a patch by Laurent Pinchart adding the first LCDC device node.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Changes compared to Laurent's original:
> >   - Add lcdc0 label,
> >   - Rename node from display to lcdc-controller,
> >   - Rename compatible value from "renesas,lcdc-r8a7740" to
> >     "renesas,r8a7740-lcdc",
> >   - Correct syntax of reg property,
> >   - Use GIC_SPI macro,
> >   - Add more clocks,
> >   - Add power-domains property,
> >   - Add status disabled,
> >   - Remove second port from lcdc0, as only lcdc1 has an HDMI port,
> >   - Add lcdc1 device node.
> > ---
> >  arch/arm/boot/dts/r8a7740.dtsi | 65 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
> > index 1b2cf5fa322b2985..55884ec701f8dab4 100644
> > --- a/arch/arm/boot/dts/r8a7740.dtsi
> > +++ b/arch/arm/boot/dts/r8a7740.dtsi
> > @@ -398,6 +398,61 @@ sh_fsi2: sound@fe1f0000 {
> >               status = "disabled";
> >       };
> >
> > +     lcdc0: lcd-controller@fe940000 {
> > +             compatible = "renesas,r8a7740-lcdc";
> > +             reg = <0xfe940000 0x4000>;
> > +             interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&mstp1_clks R8A7740_CLK_LCDC0>,
> > +                      <&cpg_clocks R8A7740_CLK_M3>, <&lcdlclk0_clk>,
> > +                      <&vou_clk>;
> > +             clock-names = "fck", "media", "lclk", "video";
> > +             power-domains = <&pd_a4lc>;
> > +             status = "disabled";
> > +
> > +             ports {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +
> > +                     port@0 {
> > +                             reg = <0>;
> > +
> > +                             lcdc0_rgb: endpoint {
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +
> > +     lcdc1: lcd-controller@fe944000 {
> > +             compatible = "renesas,r8a7740-lcdc";
> > +             reg = <0xfe944000 0x4000>;
> > +             interrupts = <GIC_SPI 178 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&mstp1_clks R8A7740_CLK_LCDC1>,
> > +                      <&cpg_clocks R8A7740_CLK_M3>, <&lcdlclk1_clk>,
> > +                      <&vou_clk>;
> > +             clock-names = "fck", "media", "lclk", "video";
> > +             power-domains = <&pd_a4lc>;
> > +             status = "disabled";
> > +
> > +             ports {
> > +                     #address-cells = <1>;
> > +                     #size-cells = <0>;
> > +
> > +                     port@0 {
> > +                             reg = <0>;
> > +
> > +                             lcdc1_rgb: endpoint {
> > +                             };
> > +                     };
> > +
> > +                     port@1 {
> > +                             reg = <1>;
> > +
> > +                             lcdc1_hdmi: endpoint {
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +
> >       tmu0: timer@fff80000 {
> >               compatible = "renesas,tmu-r8a7740", "renesas,tmu";
> >               reg = <0xfff80000 0x2c>;
> > @@ -474,6 +529,16 @@ fsibck_clk: fsibck {
> >                       #clock-cells = <0>;
> >                       clock-frequency = <0>;
> >               };
> > +             lcdlclk0_clk: lcdlclk0 {
> > +                     compatible = "fixed-clock";
> > +                     #clock-cells = <0>;
> > +                     clock-frequency = <0>;
> > +             };
> > +             lcdlclk1_clk: lcdlclk1 {
> > +                     compatible = "fixed-clock";
> > +                     #clock-cells = <0>;
> > +                     clock-frequency = <0>;
> > +             };
>
> The clock frequency seems quite low :-) As far as I understand, there
> are the clocks fed to the external LCDLCLK pins. They belong to the
> board DTS, not here. If a board doesn't provide an external clock, the
> clock should simply be ommitted. The driver must thus treat it as
> optional.

Usually, we do provide clock nodes with a zero rate in the SoC-specific
.dtsi for optional clocks.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds