Re: [PATCH v3 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT

From: Lee Jones <lee.jones@linaro.org>
To: Alexandre Torgue <alexandre.torgue@st.com>
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
	lars@metafoo.de, pmeerw@pmeerw.net, linux-pwm@vger.kernel.org,
	linux-iio@vger.kernel.org, linus.walleij@linaro.org,
	arnaud.pouliquen@st.com, linux-kernel@vger.kernel.org,
	robh+dt@kernel.org, thierry.reding@gmail.com,
	linux-arm-kernel@lists.infradead.org,
	Benjamin Gaignard <benjamin.gaignard@linaro.org>,
	knaack.h@gmx.de, gerald.baeza@st.com, fabrice.gasnier@st.com,
	linaro-kernel@lists.linaro.org, jic23@kernel.org,
	Benjamin Gaignard <benjamin.gaignard@st.com>
Subject: Re: [PATCH v3 7/7] ARM: dts: stm32: add stm32 general purpose timer driver in DT
Date: Tue, 6 Dec 2016 09:48:35 +0000	[thread overview]
Message-ID: <20161206094835.GB25385@dell.home> (raw)
In-Reply-To: <e15186cb-a93e-c2e7-13bb-6704be5e8949@st.com>

On Mon, 05 Dec 2016, Alexandre Torgue wrote:
> On 12/02/2016 02:22 PM, Lee Jones wrote:
> > On Fri, 02 Dec 2016, Benjamin Gaignard wrote:
> > 
> > > Add general purpose timers and it sub-nodes into DT for stm32f4.
> > > Define and enable pwm1 and pwm3 for stm32f469 discovery board
> > > 
> > > version 3:
> > > - use "st,stm32-timer-trigger" in DT
> > > 
> > > version 2:
> > > - use parameters to describe hardware capabilities
> > > - do not use references for pwm and iio timer subnodes
> > > 
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@st.com>
> > > ---
> > >  arch/arm/boot/dts/stm32f429.dtsi      | 333 +++++++++++++++++++++++++++++++++-
> > >  arch/arm/boot/dts/stm32f469-disco.dts |  28 +++
> > >  2 files changed, 360 insertions(+), 1 deletion(-)

[...]

If you're only commenting on a little piece of the patch, it's always
a good idea to trim the rest.

> > > diff --git a/arch/arm/boot/dts/stm32f469-disco.dts b/arch/arm/boot/dts/stm32f469-disco.dts
> > > index 8a163d7..df4ca7e 100644
> > > --- a/arch/arm/boot/dts/stm32f469-disco.dts
> > > +++ b/arch/arm/boot/dts/stm32f469-disco.dts
> > > @@ -81,3 +81,31 @@
> > >  &usart3 {
> > >  	status = "okay";
> > >  };
> > > +
> > > +&gptimer1 {
> > > +	status = "okay";
> > > +
> > > +	pwm1@0 {
> > > +		pinctrl-0	= <&pwm1_pins>;
> > > +		pinctrl-names	= "default";
> > > +		status = "okay";
> > > +	};
> > > +
> > > +	timer1@0 {
> > > +		status = "okay";
> > > +	};
> > > +};
> > 
> > This is a much *better* format than before.
> > 
> > I still don't like the '&' syntax though.
> 
> Please keep "&" format to match with existing nodes.

Right.  I wasn't suggesting that he differs from the current format in
*this* set.  I am suggesting that we change the format in a subsequent
set though.

> > > +&gptimer3 {
> > > +	status = "okay";
> > > +
> > > +	pwm3@0 {
> > > +		pinctrl-0	= <&pwm3_pins>;
> > > +		pinctrl-names	= "default";
> > > +		status = "okay";
> > > +	};
> > > +
> > > +	timer3@0 {
> > > +		status = "okay";
> > > +	};
> > > +};
> > 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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