Re: [PATCH 2/3] dts: socfpga: Add Google Chameleon v3 devicetree

[Alexandru M Stan]

Hello Krzysztof

On Mon, May 30, 2022 at 11:56 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 30/05/2022 15:08, Paweł Anikiel wrote:
> > Add devicetree for the Google Chameleon v3 board.
> >
> > Signed-off-by: Paweł Anikiel <pan@xxxxxxxxxxxx>
> > Signed-off-by: Alexandru M Stan <amstan@xxxxxxxxxxxx>
>
> Your SoB chain looks odd. Who did what here?

Sorry about this.

It was mainly Pawel but I did some small changes at some point before
it landed in our tree (particularly the GPIOs).

>
> > ---
> >  arch/arm/boot/dts/Makefile                    |  1 +
> >  .../boot/dts/socfpga_arria10_chameleonv3.dts  | 90 +++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 023c8b4ba45c..9417106d3289 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1146,6 +1146,7 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
> >       s5pv210-torbreck.dtb
> >  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
> >       socfpga_arria5_socdk.dtb \
> > +     socfpga_arria10_chameleonv3.dtb \
> >       socfpga_arria10_socdk_nand.dtb \
> >       socfpga_arria10_socdk_qspi.dtb \
> >       socfpga_arria10_socdk_sdmmc.dtb \
> > diff --git a/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> > new file mode 100644
> > index 000000000000..988cc445438e
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/socfpga_arria10_chameleonv3.dts
> > @@ -0,0 +1,90 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright 2022 Google LLC
> > + */
> > +/dts-v1/;
> > +#include "socfpga_arria10_mercury_aa1.dtsi"
> > +
> > +/ {
> > +     model = "Google Chameleon V3";
> > +     compatible = "google,chameleon-v3",
>
> You miss here enclustra compatible.

Does this make sense? I don't expect this device tree to boot/work on
an enclustra motherboard. It's only really compatible with a
"chameleon-v3".

>
> > +                  "altr,socfpga-arria10", "altr,socfpga";
> > +
> > +     aliases {
> > +             serial0 = &uart0;
> > +             i2c0 = &i2c0;
> > +             i2c1 = &i2c1;
> > +     };
> > +};
> > +
> > +&gmac0 {
> > +     status = "okay";
> > +};
> > +
> > +&gpio0 {
> > +     status = "okay";
> > +};
> > +
> > +&gpio1 {
> > +     status = "okay";
> > +};
> > +
> > +&gpio2 {
> > +     status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +     status = "okay";
> > +
> > +     ssm2603: ssm2603@1a {
>
> Generic node names.

Dumb question: what does this mean?

Are you saying the name is too generic? As someone reading the
schematics this would be immediately clear what chip it's talking
about.

>
> > +             compatible = "adi,ssm2603";
> > +             reg = <0x1a>;
> > +     };
> > +};
> > +
> > +&i2c1 {
> > +     status = "okay";
> > +
> > +     u80: u80@21 {
> > +             compatible = "nxp,pca9535";
>
> Generic node names.

FWIW: Schematic is full of these pca9535 io expanders, only one (U80)
is visible to linux on an I2C bus.

>
>
>
> Best regards,
> Krzysztof

Thanks,
Alexandru Stan (amstan)