Re: [linux][PATCH v2 2/2] dts: arm: at91: Add SAMA5D3-EDS Board

[Nicolas Ferre]

Hi Jerry,

On 23/08/2022 at 09:28, Claudiu Beznea - M18063 wrote:
> On 22.08.2022 21:13, Jerry Ray wrote:
> > The SAMA5D3-EDS board is an Ethernet Development Platform allowing for
> > evaluating many Microchip ethernet switch and PHY products.  Various
> > daughter cards can connect up via an RGMII connector or an RMII connector.
> >
> > The EDS board is not intended for stand-alone use and has no ethernet
> > capabilities when no daughter board is connected.  As such, this device
> > tree is intended to be used with a DT overlay defining the add-on board.
> > To better ensure consistency, some items are defined here as a form of
> > documentation so that all add-on overlays will use the same terms.
> >
> > Google search keywords: "Microchip SAMA5D3-EDS"
> >
> > Signed-off-by: Jerry Ray <jerry.ray@xxxxxxxxxxxxx>
> > ---
> > v1->v2:
> >   - Modified the compatible field in the device tree to reflect Microchip
> >     Ethernet Development System Board.
> > ---
> >   arch/arm/boot/dts/at91-sama5d3_eds.dts | 314 +++++++++++++++++++++++++
> >   1 file changed, 314 insertions(+)
> >   create mode 100644 arch/arm/boot/dts/at91-sama5d3_eds.dts
> >
> > diff --git a/arch/arm/boot/dts/at91-sama5d3_eds.dts b/arch/arm/boot/dts/at91-sama5d3_eds.dts
> > new file mode 100644
>
> You have to add this file to Makefile to be compiled.
>
> > index 000000000000..626f7bbe0164
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/at91-sama5d3_eds.dts
> > @@ -0,0 +1,314 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later

I would prefer that we also allow MIT:

// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> > +/*
> > + * at91-sama5d3_eds.dts - Device Tree file for the SAMA5D3 Ethernet
> > + *    Development System board.
> > + *
> > + *  Copyright (C) 2022 Microchip Technology, Inc. and its subsidiarie >> + *               2022 Jerry Ray <jerry.ray@xxxxxxxxxxxxx>

Let's have only Microchip as a copyright holder and add yourself as 
Author like:

// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
/*
 * Brief description goes here.
 *
 * Copyright (C) 2022 Microchip Technology Inc. and its subsidiaries
 *
 * Author: John Doe <john.doe@xxxxxxxxxxxxx>
 *
 * Longer description goes here.
 * Even on multiple lines.
 */

> > + */
> > +/dts-v1/;
> > +#include "sama5d36.dtsi"
> > +
> > +/ {
> > +       model = "SAMA5D3 Ethernet Development System";
> > +       compatible = "microchip,sama5d3-eds", "atmel,sama5d3", "atmel,sama5";
> > +

[..]

> > +&pinctrl {
> > +       board {
> > +               pinctrl_i2c0_pu: i2c0_pu {
> > +                       atmel,pins =
> > +                               <AT91_PIOA 30 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>,
> > +                               <AT91_PIOA 31 AT91_PERIPH_A AT91_PINCTRL_PULL_UP>;
> > +               };
> > +
> > +               pinctrl_i2c2_pu: i2c2_pu {
> > +                       atmel,pins =
> > +                               <AT91_PIOA 18 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>,
> > +                               <AT91_PIOA 19 AT91_PERIPH_B AT91_PINCTRL_PULL_UP>;
> > +               };
> > +
> > +               pinctrl_key_gpio: key_gpio_0 {
> > +                       atmel,pins =
> > +                               <AT91_PIOE 29 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
> > +               };
> > +
> > +               pinctrl_mmc0_cd: mmc0_cd {
> > +                       atmel,pins =
> > +                               <AT91_PIOE 0 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
> > +               };
> > +
> > +               pinctrl_spi0_cs: spi0_cs_default {
> > +                       atmel,pins =
> > +                               <AT91_PIOD 13 AT91_PERIPH_GPIO AT91_PINCTRL_NONE
> > +                                AT91_PIOD 16 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>;
> > +               };
> > +
> > +               pinctrl_spi1_cs: spi1_cs_default {
> > +                       atmel,pins = <AT91_PIOC 25 AT91_PERIPH_GPIO AT91_PINCTRL_NONE
> > +                                     AT91_PIOC 28 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>;
> > +               };
> > +
> > +               pinctrl_usba_vbus: usba_vbus {
> > +                       atmel,pins =
> > +                               <AT91_PIOE 9 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>;
> > +               };
> > +
> > +               pinctrl_usb_default: usb_default {
> > +                       atmel,pins =
> > +                               <AT91_PIOE 3 AT91_PERIPH_GPIO AT91_PINCTRL_NONE
> > +                                AT91_PIOE 4 AT91_PERIPH_GPIO AT91_PINCTRL_NONE>;
> > +               };
> > +
> > +               pinctrl_vcc_mmc0_reg_gpio: vcc_mmc0_reg_gpio_default {
> > +                       atmel,pins = <AT91_PIOE 2 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP>;
> > +               };
> > +
> > +               /* Reserved for reset signal to the RGMII connector. */
> > +               pinctrl_rgmii_rstn: rgmii_rstn {

It's better if you sort these nodes alphabetically: please rearrange the 
end of this list of pinctrl nodes.

> > +                       atmel,pins =
> > +                               <AT91_PIOD 18 AT91_PERIPH_GPIO AT91_PINCTRL_PULL_UP_DEGLITCH>;
> > +               };
> > +
> > +               /* Reserved for an interrupt line from the RMII and RGMII connectors. */
> > +               pinctrl_spi_irqn: spi_irqn {
> > +                       atmel,pins =
> > +                               <AT91_PIOB 28 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>;
> > +               };
> > +
> > +               /* Reserved for VBUS fault interrupt. */
> > +               pinctrl_vbusfault_irqn: vbusfault_irqn {
> > +                       atmel,pins =
> > +                               <AT91_PIOE 5 AT91_PERIPH_GPIO AT91_PINCTRL_DEGLITCH>;
> > +               };
> > +       };
> > +};

Thanks for your patch Jerry. Best regards,
  Nicolas


--
Nicolas Ferre