Re: [PATCH 2/2] arm: dts: sunxi: Add Neutis N5H3 support

[Georgii Staroselskii <georgii.staroselskii@emlid.com>]

On Tue, Nov 12, 2019 at 01:15:58PM +0100, Maxime Ripard wrote:
> Hi,
> 

Hi!

> On Wed, Nov 06, 2019 at 05:03:18PM +0300, Georgii Staroselskii wrote:
> > Emlid Neutis N5H3 is a version of Emlid Neutis SoM with H3 instead of H5
> > inside.
> >
> > 6eeb4180d4b9 ("ARM: dts: sunxi: h3-h5: Add Bananapi M2+ v1.2 device")
> > was used as reference.
> >
> > Signed-off-by: Georgii Staroselskii <georgii.staroselskii@emlid.com>
> > ---
> >  arch/arm/boot/dts/Makefile                         |  1 +
> >  .../dts/sun8i-h3-emlid-neutis-n5h3-devboard.dts    | 61 ++++++++++++++++++++++
> >  arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3.dtsi  | 11 ++++
> >  3 files changed, 73 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3-devboard.dts
> >  create mode 100644 arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3.dtsi
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 3f13b88..c997b0c 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1119,6 +1119,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> >  	sun8i-h3-orangepi-plus2e.dtb \
> >  	sun8i-h3-orangepi-zero-plus2.dtb \
> >  	sun8i-h3-rervision-dvk.dtb \
> > +	sun8i-h3-emlid-neutis-n5h3-devboard.dtb \
> 
> There's no need to duplicate the H3 in the name, we can just call it
> 
> sun8i-h3-emlid-neutis-n5-devboard.dts
> 
> Unless you expect some other boards named in a similar matter?
> 

The thing is that Neutis N5H3 is the name of the board. So I guess it
makes sense to let this name be the part of the dts as well.

> >  	sun8i-r16-bananapi-m2m.dtb \
> >  	sun8i-r16-nintendo-nes-classic.dtb \
> >  	sun8i-r16-nintendo-super-nes-classic.dtb \
> > diff --git a/arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3-devboard.dts b/arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3-devboard.dts
> > new file mode 100644
> > index 00000000..3b68750
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/sun8i-h3-emlid-neutis-n5h3-devboard.dts
> > @@ -0,0 +1,61 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later OR MIT
> > +/*
> > + * DTS for Emlid Neutis N5 Dev board.
> > + *
> > + * Copyright (C) 2019 Georgii Staroselskii <georgiii.staroselskii@emlid.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sun8i-h3-emlid-neutis-n5h3.dtsi"
> > +
> > +/ {
> > +	model = "Emlid Neutis N5H3 Developer board";
> > +	compatible = "emlid,neutis-n5h3-devboard",
> > +		     "emlid,neutis-n5h3",
> > +		     "allwinner,sun8i-h3";
> 
> Same remarks for the compatible, we have the h3 compatible here to
> differentiate between the two.
> 
> You should also document this combination to
> Documentation/devicetree/bindings/arm/sunxi.yaml.
> 

Thanks, will do.

> > +
> > +	vdd_cpux: gpio-regulator {
> > +		compatible = "regulator-gpio";
> > +		regulator-name = "vdd-cpux";
> > +		regulator-type = "voltage";
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +		regulator-min-microvolt = <1100000>;
> > +		regulator-max-microvolt = <1300000>;
> > +		regulator-ramp-delay = <50>; /* 4ms */
> > +		gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; /* PL6 */
> > +		gpios-states = <0x1>;
> > +		states = <1100000 0x0
> > +			  1300000 0x1>;
> 
> While DTC outputs the same thing, and it works, you should make this
> an array of 2 items of 2 cells, instead of a array of 1 item of 4
> cells.
> 
> Like this: states = <1100000 0x0>, <1300000 0x1>;
> 
> While this doesn't change anything with DTC, other cases (like DT
> validation) care about this.
> 

Thank you!

> Look good otherwise.

I have spotted a couple of issues with the DTSI that I'm going to
address in V2.

Thanks for the review.

> Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel