by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [PATCH 08/10] ARM: sun7i: DT: Add UART muxing options to the DTSI

Hello.

On 07/30/2013 06:59 PM, Maxime Ripard wrote:

> The UARTs on the A20 can be muxed to several pins. Add a few options to
> the DTSI so that we can start using them in the boards' DT.

> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> arch/arm/boot/dts/sun7i-a20.dtsi | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)

> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 704fca9..9f45e8d 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -70,6 +70,27 @@
> #address-cells = <1>;
> #size-cells = <0>;
> #gpio-cells = <3>;
> +
> + uart0_pins_a: uart0@0 {

Why have address part of the node name if there's not "reg" property?
And the preferred node name is "serial", not "uart", according to the ePAPR
spec (http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf).

> + allwinner,pins = "PB22", "PB23";
> + allwinner,function = "uart0";
> + allwinner,drive = <0>;
> + allwinner,pull = <0>;
> + };
> +
> + uart6_pins_a: uart6@0 {
> + allwinner,pins = "PI12", "PI13";
> + allwinner,function = "uart6";
> + allwinner,drive = <0>;
> + allwinner,pull = <0>;
> + };
> +
> + uart7_pins_a: uart7@0 {
> + allwinner,pins = "PI20", "PI21";
> + allwinner,function = "uart7";
> + allwinner,drive = <0>;
> + allwinner,pull = <0>;
> + };

WBR, Sergei

2013-07-30 20:10:10

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH 08/10] ARM: sun7i: DT: Add UART muxing options to the DTSI

Hi Sergei,

On Tue, Jul 30, 2013 at 11:07:21PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 07/30/2013 06:59 PM, Maxime Ripard wrote:
>
> >The UARTs on the A20 can be muxed to several pins. Add a few options to
> >the DTSI so that we can start using them in the boards' DT.
>
> >Signed-off-by: Maxime Ripard <[email protected]>
> >---
> > arch/arm/boot/dts/sun7i-a20.dtsi | 21 +++++++++++++++++++++
> > 1 file changed, 21 insertions(+)
>
> >diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> >index 704fca9..9f45e8d 100644
> >--- a/arch/arm/boot/dts/sun7i-a20.dtsi
> >+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> >@@ -70,6 +70,27 @@
> > #address-cells = <1>;
> > #size-cells = <0>;
> > #gpio-cells = <3>;
> >+
> >+ uart0_pins_a: uart0@0 {
>
> Why have address part of the node name if there's not "reg" property?

It allows to easily differentiate distinct pin groups for the same IP.

A second group of pin that can be muxed to uart0 would be defined as
uart0_pins_b: uart0@1.

I could have used any other construct, like uart0-0 and uart0-1, I just
find that it fits more in the usual DT constructs.

> And the preferred node name is "serial", not "uart", according to
> the ePAPR spec (http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf).

But only for actual serial driver right? This is a pinctrl group
declaration.

Thanks,
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (1.50 kB)
signature.asc (836.00 B)
Digital signature Download all attachments

2013-07-30 20:31:18

by Maxime Ripard

[permalink] [raw]

Subject: Re: [linux-sunxi] [PATCH 04/10] pinctrl: sunxi: Wrap long compatible declaration lines

Hi Ian,

On Tue, Jul 30, 2013 at 04:09:13PM +0100, Ian Campbell wrote:
> On Tue, 2013-07-30 at 16:59 +0200, Maxime Ripard wrote:
> > The compatible declaration and their associated data are exceeding the
> > 80-characters width limit.
> >
> > Split these lines to make them more readable and fit in the kernel
> > coding guidelines.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > Cc: Linus Walleij <[email protected]>
> > ---
> > drivers/pinctrl/pinctrl-sunxi.c | 20 ++++++++++++++++----
> > 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-sunxi.c b/drivers/pinctrl/pinctrl-sunxi.c
> > index 5985e60..246310b 100644
> > --- a/drivers/pinctrl/pinctrl-sunxi.c
> > +++ b/drivers/pinctrl/pinctrl-sunxi.c
> > @@ -628,10 +628,22 @@ static void sunxi_pinctrl_irq_handler(unsigned irq, struct irq_desc *desc)
> > }
> >
> > static struct of_device_id sunxi_pinctrl_match[] = {
> > - { .compatible = "allwinner,sun4i-a10-pinctrl", .data = (void *)&sun4i_a10_pinctrl_data },
> > - { .compatible = "allwinner,sun5i-a10s-pinctrl", .data = (void *)&sun5i_a10s_pinctrl_data },
> > - { .compatible = "allwinner,sun5i-a13-pinctrl", .data = (void *)&sun5i_a13_pinctrl_data },
> > - { .compatible = "allwinner,sun6i-a31-pinctrl", .data = (void *)&sun6i_a31_pinctrl_data },
> > + {
> > + .compatible = "allwinner,sun4i-a10-pinctrl",
> > + .data = (void *)&sun4i_a10_pinctrl_data,
> > + },
>
> I'm not sure this is actually an improvement, the original reads quite
> naturally even if it is a little long. What about a #define such that
> you can write:
> COMPAT("allwinner,sun4i-a10-pinctrl", sun4i_a10_pinctrl_data),
> and thereby stay within 80-chars?
>
> If you wanted to go overboard you could code the macro such that:
> SUNXI_COMPAT(sun4i,a10),
> expanded to what you want, that's pretty obfuscated though.

To be honest, I'm not convinced by any of the constructs seen above. The
one we had is too long, the 4-lines one is not that great either, and
the macro only obfuscates things away.

You know what, I'll drop this patch.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachments:

(No filename) (2.20 kB)
signature.asc (836.00 B)
Digital signature Download all attachments