Re: [PATCH 3/3] arm64: dts: nuvoton: Add initial support for MA35D1

[Jacky Huang]

On 2022/4/6 下午 03:14, Krzysztof Kozlowski wrote:
> On 06/04/2022 04:58, Jacky Huang wrote:
> > > > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> > > > index 639e01a4d855..28e01442094f 100644
> > > > --- a/arch/arm64/boot/dts/Makefile
> > > > +++ b/arch/arm64/boot/dts/Makefile
> > > > @@ -30,3 +30,4 @@ subdir-y += synaptics
> > > >    subdir-y += ti
> > > >    subdir-y += toshiba
> > > >    subdir-y += xilinx
> > > > +subdir-y += nuvoton
> > > > diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile
> > > > new file mode 100644
> > > > index 000000000000..e1e0c466bf5e
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/nuvoton/Makefile
> > > > @@ -0,0 +1,2 @@
> > > > +# SPDX-License-Identifier: GPL-2.0
> > > > +dtb-$(CONFIG_ARCH_NUVOTON) += ma35d1-evb.dtb
> > > ARCH_NUVOTON does not exist.
> > I would add the following to end of arch/arm64/Kconfig.platforms,
> Don't add things at the end of files but rather in respective place
> without messing the order.

OK, I will put it to the right place in alphanumeric order.
It should be between ARCH_MXC and ARCH_QCOM.

> > and
> > add the
> > modification to this patch series.
> >
> > config ARCH_MA35D1
> >       bool "Nuvoton MA35D1 SOC Family"
> We do not add options for specific SoCs, but for entire families, so
> ARCH_NUVOTON is correct.

Yes, I would like to modify it as the following:

config ARCH_NUVOTON
    bool "Nuvoton SoC Family"
    select PINCTRL
    select PINCTRL_MA35D1
    select PM
    select GPIOLIB
    select SOC_BUS
    help
      This enables support for Nuvoton MA35D1 ARMv8 SoC.

(Currently, we have MA35D1 only in the support list for arm64 SoC.).

> >       select PINCTRL
> >       select PINCTRL_MA35D1
> >       select PM
> >       select GPIOLIB
> >       select SOC_BUS
> >       select VIDEOMODE_HELPERS
> >       select FB_MODE_HELPERS
> >       help
> >         This enables support for Nuvoton MA35D1 SOC Family.
> >
> >
> > > > diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
> > > > new file mode 100644
> > > > index 000000000000..38e4f734da0f
> > > > --- /dev/null
> > > > +++ b/arch/arm64/boot/dts/nuvoton/ma35d1-evb.dts
> > > > @@ -0,0 +1,23 @@
> > > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> > > > +/*
> > > > + * Device Tree Source for MA35D1 Evaluation Board (EVB)
> > > > + *
> > > > + * Copyright (C) 2021 Nuvoton Technology Corp.
> > > > + */
> > > > +
> > > > +/dts-v1/;
> > > > +#include "ma35d1.dtsi"
> > > > +
> > > > +/ {
> > > > +       model = "Nuvoton MA35D1-EVB";
> > > > +
> > > > +       chosen {
> > > > +               bootargs = "console=ttyS0,115200n8";
> > > No bootargs. "chosen", please.
> > OK, I would modify it as:
> >
> > chosen {
> >           stdout-path = "serial0:115200n8";
> >       };
> >
> >
> > > > +       };
> > > You need compatible and bindings.
> > I will add the compatible here
> > compatible = "nuvoton,ma35d1-evb", "nuvoton,ma35d1"
> >
> > And, I should create a new binding file
> > Documentation/devicetree/bindings/arm/nuvoton.yaml to this patch series.
> > And the property would be:
> >
> > properties:
> >     compatible:
> >       description: Nuvoton MA35D1-EVB
> >       items:
> >         - const: nuvoton,ma35d1-evb
> >         - const: nuvoton,ma35d1
> >
> >
> > Is it OK?
> Yes
>
>
>
> Best regards,
> Krzysztof

Thanks for your review.

Sincerely,
Jacky