Re: [PATCH v3] ARM: dts: add DT for lan966x SoC and 2-port board pcb8291

[Arnd Bergmann]

On Thu, Jan 13, 2022 at 9:33 AM Nicolas Ferre
<nicolas.ferre@xxxxxxxxxxxxx> wrote:
>
> On 13/01/2022 at 09:00, Kavyasree Kotagiri wrote:
> > This patch adds basic DT for Microchip lan966x SoC and associated board
> > pcb8291(2-port EVB). Adds peripherals required to allow booting: IRQs,
> > clocks, timers, memory, flexcoms, GPIOs. Also adds other peripherals like
> > crypto(AES,SHA), DMA and watchdog.
> >
> > Signed-off-by: Kavyasree Kotagiri <kavyasree.kotagiri@xxxxxxxxxxxxx>
>
> Looks good to me:
> Reviewed-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxxxxxx>

I'm not quite sure what to do with this, as this was sent to:soc@xxxxxxxxxx,
which is normally for patches that are already reviewed and should just
get applied.

I can apply this, but I would normally expect board files to get picked up
in the at91 tree first. I'll drop this version from patchwork for now, as I
also have a couple of comments:

> > +
> > +/ {
> > +     model = "Microchip LAN966x family SoC";
> > +     compatible = "microchip,lan966x";

By convention, the 'compatible' strings should not contain 'x'
as a wildcard character. Just pick one of the models to be
compatible with. The .dtsi file doesn't really need a top-level
compatible or model property though, as they need to be
overridden by teh board anyway.

> > +
> > +     memory@60000000 {
> > +             device_type = "memory";
> > +             reg = <0x60000000 0x40000000>;  /* 1GB */
> > +     };

Probably also no memory node. This tends to be filled by the
boot loader, or it is part of the board when when the boot loader
is too old for that.

If the memory is part of the chip package, having it in the .dtsi
file is probably ok, but I would add a comment for that.

> > + */
> > +/dts-v1/;
> > +#include "lan966x.dtsi"
> > +
> > +/ {
> > +     model = "Microchip EVB - LAN9662";
> > +     compatible = "microchip,lan9662-pcb8291", "microchip,lan9662", "microchip,lan966";
> > +};

Here I would expect /chosen and /aliases nodes.

> > +&gpio {
> > +     fc_shrd7_pins: fc_shrd7-pins {
> > +             pins = "GPIO_49";
> > +             function = "fc_shrd7";
> > +     };

These properties don't look like most pinctrl nodes, has the binding
been reviewed?
I don't see it in Documentation/devicetree/bindings/pinctrl/

          Arnd