Re: [DO NOT MERGE v6 22/37] dt-bindings: display: smi,sm501: SMI SM501 binding json-schema

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: linux-sh@vger.kernel.org, "Damien Le Moal" <dlemoal@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Conor Dooley" <conor+dt@kernel.org>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Thomas Zimmermann" <tzimmermann@suse.de>,
	"David Airlie" <airlied@gmail.com>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Jiri Slaby" <jirislaby@kernel.org>,
	"Magnus Damm" <magnus.damm@gmail.com>,
	"Daniel Lezcano" <daniel.lezcano@linaro.org>,
	"Rich Felker" <dalias@libc.org>,
	"John Paul Adrian Glaubitz" <glaubitz@physik.fu-berlin.de>,
	"Lee Jones" <lee@kernel.org>, "Helge Deller" <deller@gmx.de>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Jernej Skrabec" <jernej.skrabec@gmail.com>,
	"Chris Morgan" <macromorgan@hotmail.com>,
	"Yang Xiwen" <forbidden405@foxmail.com>,
	"Sebastian Reichel" <sre@kernel.org>,
	"Linus Walleij" <linus.walleij@linaro.org>,
	"Randy Dunlap" <rdunlap@infradead.org>,
	"Arnd Bergmann" <arnd@arndb.de>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	"Hyeonggon Yoo" <42.hyeyoo@gmail.com>,
	"David Rientjes" <rientjes@google.com>,
	"Baoquan He" <bhe@redhat.com>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Guenter Roeck" <linux@roeck-us.net>,
	"Stephen Rothwell" <sfr@canb.auug.org.au>,
	"Azeem Shaikh" <azeemshaikh38@gmail.com>,
	"Javier Martinez Canillas" <javierm@redhat.com>,
	"Max Filippov" <jcmvbkbc@gmail.com>,
	"Palmer Dabbelt" <palmer@rivosinc.com>,
	"Bin Meng" <bmeng@tinylab.org>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Jacky Huang" <ychuang3@nuvoton.com>,
	"Lukas Bulwahn" <lukas.bulwahn@gmail.com>,
	"Biju Das" <biju.das.jz@bp.renesas.com>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Sam Ravnborg" <sam@ravnborg.org>,
	"Sergey Shtylyov" <s.shtylyov@omp.ru>,
	"Michael Karcher" <kernel@mkarcher.dialup.fu-berlin.de>,
	"Laurent Pinchart" <laurent.pinchart+renesas@ideasonboard.com>,
	linux-ide@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-clk@vger.kernel.org, dri-devel@lists.freedesktop.org,
	linux-pci@vger.kernel.org, linux-serial@vger.kernel.org,
	linux-fbdev@vger.kernel.org
Subject: Re: [DO NOT MERGE v6 22/37] dt-bindings: display: smi,sm501: SMI SM501 binding json-schema
Date: Mon, 15 Jan 2024 10:52:36 +0100	[thread overview]
Message-ID: <CAMuHMdWSR3ikL7VZYkNOb1Y8mPU5LaUnc8+WLj-Ec99EOWxs_w@mail.gmail.com> (raw)
In-Reply-To: <9c3a9caaa1e2fc7e515cac67f07a20af071bd1be.1704788539.git.ysato@users.sourceforge.jp>

Hi Sato-san,

On Tue, Jan 9, 2024 at 9:24 AM Yoshinori Sato
<ysato@users.sourceforge.jp> wrote:
> Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>

Thanks for your patch!

> ---
>  .../bindings/display/smi,sm501.yaml           | 417 ++++++++++++++++++
>  1 file changed, 417 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/smi,sm501.yaml

Surely Documentation/devicetree/bindings/display/sm501fb.txt should
be removed, too?

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/smi,sm501.yaml

> +  crt:
> +    type: object
> +    description: CRT output control
> +    properties:
> +      edid:
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        description: |
> +          verbatim EDID data block describing attached display.
> +          Data from the detailed timing descriptor will be used to
> +          program the display controller.
> +
> +      smi,flags:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description: Display control flags.
> +        items:
> +          anyOf:
> +            - const: use-init-done
> +            - const: disable-at-exit
> +            - const: use-hwcursor
> +            - const: use-hwaccel

The "use-*" flags look like software policy, not hardware description,
and thus do not belong in DT?

> +            - const: panel-no-fpen
> +            - const: panel-no-vbiasen
> +            - const: panel-inv-fpen
> +            - const: panel-inv-vbiasen
> +        maxItems: 8
> +
> +      bpp:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Color depth
> +
> +  panel:
> +    type: object
> +    description: Panel output control
> +    properties:
> +      edid:
> +        $ref: /schemas/types.yaml#/definitions/uint8-array
> +        description: |
> +          verbatim EDID data block describing attached display.
> +          Data from the detailed timing descriptor will be used to
> +          program the display controller.
> +
> +      smi,flags:
> +        $ref: /schemas/types.yaml#/definitions/string-array
> +        description: Display control flags.
> +        items:
> +          anyOf:
> +            - const: use-init-done
> +            - const: disable-at-exit
> +            - const: use-hwcursor
> +            - const: use-hwaccel

The "use-*" flags look like software policy, not hardware description,
and thus do not belong in DT?

> +            - const: panel-no-fpen
> +            - const: panel-no-vbiasen
> +            - const: panel-inv-fpen
> +            - const: panel-inv-vbiasen
> +        maxItems: 8
> +
> +      bpp:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Color depth
> +
> +  smi,devices:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description: Select SM501 device functions.
> +    items:
> +      anyOf:
> +        - const: usb-host
> +        - const: usb-slave
> +        - const: ssp0
> +        - const: ssp1
> +        - const: uart0
> +        - const: uart1
> +        - const: fbaccel
> +        - const: ac97
> +        - const: i2s
> +        - const: gpio
> +    minItems: 1
> +    maxItems: 10

I think it would be better to have individual subnodes for the sub devices,
with status = "ok"/"disabled".

If you go that route, you do need some fallback code to handle the lack
of subnodes in the existing user in arch/powerpc/boot/dts/charon.dts.

BTW, why can sm501_pci_initdata get away with setting ".devices
= SM501_USE_ALL"?  Or, would it hurt to enable all subdevices
unconditionally?

> +
> +  smi,mclk:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: mclk frequency.
> +
> +  smi,m1xclk:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: m1xclk frequency.

These two should be clock specifiers (i.e. phandles pointing to clock
nodes + optional clock indices).

> +
> +  misc-timing:
> +    type: object
> +    description: Miscellaneous Timing register values.
> +    properties:
> +      ex:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        description: Extend bus holding time.
> +        enum: [0, 16, 32, 48, 64, 80, 96, 112, 128, 144, 160, 176, 192, 208, 224, 240]
> +
> +      xc:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        description: Xscale clock input select.
> +        items:
> +          enum:
> +            - internal-pll
> +            - hclk
> +            - gpio33

Software policy instead of hardware description again?

I am not familiar with how the SM501 works, so I cannot comment on
the other properties, but several of them look like they need rework.

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