| Message ID | 1421123484-21387-1-git-send-email-wens@csie.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: > Using label references is preferred when override settings from the > included dtsi. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > --- > > My AXP221 series touches this file. I thought I'd convert it first. > > This looks like a lot of changes. But if you filter out all the > indentation changes, it's just the opening lines for each node. > > --- > arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 ++++++++++++++-------------- > 1 file changed, 88 insertions(+), 93 deletions(-) > > diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > index ebd5f7854b1b..97dbaeb76416 100644 > --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > @@ -61,101 +61,96 @@ > chosen { > bootargs = "earlyprintk console=ttyS0,115200"; > }; > +}; > + > +&mmc0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; > + vmmc-supply = <®_vcc3v0>; > + bus-width = <4>; > + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ > + cd-inverted; > + status = "okay"; > +}; > + > +&usbphy { > + usb1_vbus-supply = <®_usb1_vbus>; > + status = "okay"; > +}; > + > +&ehci0 { > + status = "okay"; > +}; > + > +&ohci0 { > + status = "okay"; > +}; > + > +&pio { > + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { > + allwinner,pins = "PA8"; > + allwinner,function = "gpio_in"; > + allwinner,drive = <SUN4I_PINCTRL_10_MA>; > + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > + }; > +}; > + > +&mmc0_pins_a { > + /* external pull-ups missing for some pins */ > + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > +}; > + > +&usb1_vbus_pin_a { > + /* different pin from sunxi-common-regulators */ > + allwinner,pins = "PH24"; > +}; > + > +&uart0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&uart0_pins_a>; > + status = "okay"; > +}; > + > +&i2c0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&i2c0_pins_a>; > + /* pull-ups and devices require AXP221 DLDO3 */ > + status = "failed"; > +}; I think we should define a convention about how to sort these nodes before we actually start merging some of it. This of course also apply to the other patches doing that, hence why Hans is CC'd. I guess sorting them by label alphabetical order would make sense. What do you think? Maxime
On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >> Using label references is preferred when override settings from the >> included dtsi. >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> --- >> >> My AXP221 series touches this file. I thought I'd convert it first. >> >> This looks like a lot of changes. But if you filter out all the >> indentation changes, it's just the opening lines for each node. >> >> --- >> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 ++++++++++++++-------------- >> 1 file changed, 88 insertions(+), 93 deletions(-) >> >> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >> index ebd5f7854b1b..97dbaeb76416 100644 >> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >> @@ -61,101 +61,96 @@ >> chosen { >> bootargs = "earlyprintk console=ttyS0,115200"; >> }; >> +}; >> + >> +&mmc0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >> + vmmc-supply = <®_vcc3v0>; >> + bus-width = <4>; >> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >> + cd-inverted; >> + status = "okay"; >> +}; >> + >> +&usbphy { >> + usb1_vbus-supply = <®_usb1_vbus>; >> + status = "okay"; >> +}; >> + >> +&ehci0 { >> + status = "okay"; >> +}; >> + >> +&ohci0 { >> + status = "okay"; >> +}; >> + >> +&pio { >> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >> + allwinner,pins = "PA8"; >> + allwinner,function = "gpio_in"; >> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >> + }; >> +}; >> + >> +&mmc0_pins_a { >> + /* external pull-ups missing for some pins */ >> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >> +}; >> + >> +&usb1_vbus_pin_a { >> + /* different pin from sunxi-common-regulators */ >> + allwinner,pins = "PH24"; >> +}; >> + >> +&uart0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&uart0_pins_a>; >> + status = "okay"; >> +}; >> + >> +&i2c0 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&i2c0_pins_a>; >> + /* pull-ups and devices require AXP221 DLDO3 */ >> + status = "failed"; >> +}; > > I think we should define a convention about how to sort these nodes > before we actually start merging some of it. > > This of course also apply to the other patches doing that, hence why > Hans is CC'd. > > I guess sorting them by label alphabetical order would make > sense. What do you think? I'm currently using the ordering from the dtsi, which is based on address. Even if it's not visible, if you're creating the dts by looking at the dtsi and enabling the devices available, that's the order you add them by, so it kind of makes sense. The pins are grouped around &pio, and sorted by name. And the regulators are listed at the bottom. ChenYu
On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: > On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: > >> Using label references is preferred when override settings from the > >> included dtsi. > >> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> --- > >> > >> My AXP221 series touches this file. I thought I'd convert it first. > >> > >> This looks like a lot of changes. But if you filter out all the > >> indentation changes, it's just the opening lines for each node. > >> > >> --- > >> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 ++++++++++++++-------------- > >> 1 file changed, 88 insertions(+), 93 deletions(-) > >> > >> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >> index ebd5f7854b1b..97dbaeb76416 100644 > >> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >> @@ -61,101 +61,96 @@ > >> chosen { > >> bootargs = "earlyprintk console=ttyS0,115200"; > >> }; > >> +}; > >> + > >> +&mmc0 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; > >> + vmmc-supply = <®_vcc3v0>; > >> + bus-width = <4>; > >> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ > >> + cd-inverted; > >> + status = "okay"; > >> +}; > >> + > >> +&usbphy { > >> + usb1_vbus-supply = <®_usb1_vbus>; > >> + status = "okay"; > >> +}; > >> + > >> +&ehci0 { > >> + status = "okay"; > >> +}; > >> + > >> +&ohci0 { > >> + status = "okay"; > >> +}; > >> + > >> +&pio { > >> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { > >> + allwinner,pins = "PA8"; > >> + allwinner,function = "gpio_in"; > >> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; > >> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > >> + }; > >> +}; > >> + > >> +&mmc0_pins_a { > >> + /* external pull-ups missing for some pins */ > >> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > >> +}; > >> + > >> +&usb1_vbus_pin_a { > >> + /* different pin from sunxi-common-regulators */ > >> + allwinner,pins = "PH24"; > >> +}; > >> + > >> +&uart0 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&uart0_pins_a>; > >> + status = "okay"; > >> +}; > >> + > >> +&i2c0 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&i2c0_pins_a>; > >> + /* pull-ups and devices require AXP221 DLDO3 */ > >> + status = "failed"; > >> +}; > > > > I think we should define a convention about how to sort these nodes > > before we actually start merging some of it. > > > > This of course also apply to the other patches doing that, hence why > > Hans is CC'd. > > > > I guess sorting them by label alphabetical order would make > > sense. What do you think? > > I'm currently using the ordering from the dtsi, which is based > on address. Even if it's not visible, if you're creating the > dts by looking at the dtsi and enabling the devices available, > that's the order you add them by, so it kind of makes sense. I know you're doing just that, and that it makes some kind of sense whenever you convert an old DTS to the label based syntax, but whenever you create a new one, it's a bit harder to get it right. And the fact that Hans didn't follow that convention illustrate that very well. I guess a sorting logic internal to the DTS itself would be much easier to understand and follow, hence why I suggested the alphabetical order: it just stands out without any external reference.
Hi, On 13-01-15 17:20, Maxime Ripard wrote: > On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >> <maxime.ripard@free-electrons.com> wrote: >>> Hi, >>> >>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>> Using label references is preferred when override settings from the >>>> included dtsi. >>>> >>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>> --- >>>> >>>> My AXP221 series touches this file. I thought I'd convert it first. >>>> >>>> This looks like a lot of changes. But if you filter out all the >>>> indentation changes, it's just the opening lines for each node. >>>> >>>> --- >>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 ++++++++++++++-------------- >>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>> >>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>> @@ -61,101 +61,96 @@ >>>> chosen { >>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>> }; >>>> +}; >>>> + >>>> +&mmc0 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>> + vmmc-supply = <®_vcc3v0>; >>>> + bus-width = <4>; >>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>> + cd-inverted; >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&usbphy { >>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&ehci0 { >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&ohci0 { >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&pio { >>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>> + allwinner,pins = "PA8"; >>>> + allwinner,function = "gpio_in"; >>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>> + }; >>>> +}; >>>> + >>>> +&mmc0_pins_a { >>>> + /* external pull-ups missing for some pins */ >>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>> +}; >>>> + >>>> +&usb1_vbus_pin_a { >>>> + /* different pin from sunxi-common-regulators */ >>>> + allwinner,pins = "PH24"; >>>> +}; >>>> + >>>> +&uart0 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&uart0_pins_a>; >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&i2c0 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&i2c0_pins_a>; >>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>> + status = "failed"; >>>> +}; >>> >>> I think we should define a convention about how to sort these nodes >>> before we actually start merging some of it. >>> >>> This of course also apply to the other patches doing that, hence why >>> Hans is CC'd. >>> >>> I guess sorting them by label alphabetical order would make >>> sense. What do you think? >> >> I'm currently using the ordering from the dtsi, which is based >> on address. Even if it's not visible, if you're creating the >> dts by looking at the dtsi and enabling the devices available, >> that's the order you add them by, so it kind of makes sense. Right, that is what I'm doing too. > I know you're doing just that, and that it makes some kind of sense > whenever you convert an old DTS to the label based syntax, but > whenever you create a new one, it's a bit harder to get it right. > > And the fact that Hans didn't follow that convention illustrate that > very well. Erm, that is not true, I did follow that convention as my 2 new dts files started in the old format too, and I simply re-indented most nodes. > I guess a sorting logic internal to the DTS itself would be much > easier to understand and follow, hence why I suggested the > alphabetical order: it just stands out without any external reference. If we agree on this, we also need to think about what to do with new board specific powersupplies, as those need to be in the root node, if you look at the bananapro dts from the v2 patch you will see what I mean. Regards, Hans
On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 13-01-15 17:20, Maxime Ripard wrote: >> >> On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >>> >>> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >>> <maxime.ripard@free-electrons.com> wrote: >>>> >>>> Hi, >>>> >>>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>>> >>>>> Using label references is preferred when override settings from the >>>>> included dtsi. >>>>> >>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>> --- >>>>> >>>>> My AXP221 series touches this file. I thought I'd convert it first. >>>>> >>>>> This looks like a lot of changes. But if you filter out all the >>>>> indentation changes, it's just the opening lines for each node. >>>>> >>>>> --- >>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 >>>>> ++++++++++++++-------------- >>>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>>> >>>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>> b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>> @@ -61,101 +61,96 @@ >>>>> chosen { >>>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>>> }; >>>>> +}; >>>>> + >>>>> +&mmc0 { >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>>> + vmmc-supply = <®_vcc3v0>; >>>>> + bus-width = <4>; >>>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>>> + cd-inverted; >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&usbphy { >>>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&ehci0 { >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&ohci0 { >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&pio { >>>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>>> + allwinner,pins = "PA8"; >>>>> + allwinner,function = "gpio_in"; >>>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>> + }; >>>>> +}; >>>>> + >>>>> +&mmc0_pins_a { >>>>> + /* external pull-ups missing for some pins */ >>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>> +}; >>>>> + >>>>> +&usb1_vbus_pin_a { >>>>> + /* different pin from sunxi-common-regulators */ >>>>> + allwinner,pins = "PH24"; >>>>> +}; >>>>> + >>>>> +&uart0 { >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&uart0_pins_a>; >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&i2c0 { >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&i2c0_pins_a>; >>>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>>> + status = "failed"; >>>>> +}; >>>> >>>> >>>> I think we should define a convention about how to sort these nodes >>>> before we actually start merging some of it. >>>> >>>> This of course also apply to the other patches doing that, hence why >>>> Hans is CC'd. >>>> >>>> I guess sorting them by label alphabetical order would make >>>> sense. What do you think? >>> >>> >>> I'm currently using the ordering from the dtsi, which is based >>> on address. Even if it's not visible, if you're creating the >>> dts by looking at the dtsi and enabling the devices available, >>> that's the order you add them by, so it kind of makes sense. > > > Right, that is what I'm doing too. > >> I know you're doing just that, and that it makes some kind of sense >> whenever you convert an old DTS to the label based syntax, but >> whenever you create a new one, it's a bit harder to get it right. >> >> And the fact that Hans didn't follow that convention illustrate that >> very well. > > > Erm, that is not true, I did follow that convention as my 2 new > dts files started in the old format too, and I simply re-indented > most nodes. I think it also makes sense for new boards as well. One would look at the dtsi to know what devices on the soc are available, and copy the ones that can be used. But the ordering does make adding new devices with new drivers a bit problematic. It is easy to place new nodes in the wrong place. >> I guess a sorting logic internal to the DTS itself would be much >> easier to understand and follow, hence why I suggested the >> alphabetical order: it just stands out without any external reference. > > > If we agree on this, we also need to think about what to do with new > board specific powersupplies, as those need to be in the root node, > if you look at the bananapro dts from the v2 patch you will see what > I mean. How about the root node at the top? With the board description, chosen, aliases (if any), and then the _new_ regulators (sorted by name)? One can then easily identify which are board specific, and which are more or less generic. ChenYu
Hi, On 14-01-15 09:41, Chen-Yu Tsai wrote: > On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 13-01-15 17:20, Maxime Ripard wrote: >>> >>> On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >>>> >>>> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >>>> <maxime.ripard@free-electrons.com> wrote: >>>>> >>>>> Hi, >>>>> >>>>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>>>> >>>>>> Using label references is preferred when override settings from the >>>>>> included dtsi. >>>>>> >>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>> --- >>>>>> >>>>>> My AXP221 series touches this file. I thought I'd convert it first. >>>>>> >>>>>> This looks like a lot of changes. But if you filter out all the >>>>>> indentation changes, it's just the opening lines for each node. >>>>>> >>>>>> --- >>>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 >>>>>> ++++++++++++++-------------- >>>>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>> b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>> @@ -61,101 +61,96 @@ >>>>>> chosen { >>>>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>>>> }; >>>>>> +}; >>>>>> + >>>>>> +&mmc0 { >>>>>> + pinctrl-names = "default"; >>>>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>>>> + vmmc-supply = <®_vcc3v0>; >>>>>> + bus-width = <4>; >>>>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>>>> + cd-inverted; >>>>>> + status = "okay"; >>>>>> +}; >>>>>> + >>>>>> +&usbphy { >>>>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>>>> + status = "okay"; >>>>>> +}; >>>>>> + >>>>>> +&ehci0 { >>>>>> + status = "okay"; >>>>>> +}; >>>>>> + >>>>>> +&ohci0 { >>>>>> + status = "okay"; >>>>>> +}; >>>>>> + >>>>>> +&pio { >>>>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>>>> + allwinner,pins = "PA8"; >>>>>> + allwinner,function = "gpio_in"; >>>>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>> + }; >>>>>> +}; >>>>>> + >>>>>> +&mmc0_pins_a { >>>>>> + /* external pull-ups missing for some pins */ >>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>> +}; >>>>>> + >>>>>> +&usb1_vbus_pin_a { >>>>>> + /* different pin from sunxi-common-regulators */ >>>>>> + allwinner,pins = "PH24"; >>>>>> +}; >>>>>> + >>>>>> +&uart0 { >>>>>> + pinctrl-names = "default"; >>>>>> + pinctrl-0 = <&uart0_pins_a>; >>>>>> + status = "okay"; >>>>>> +}; >>>>>> + >>>>>> +&i2c0 { >>>>>> + pinctrl-names = "default"; >>>>>> + pinctrl-0 = <&i2c0_pins_a>; >>>>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>>>> + status = "failed"; >>>>>> +}; >>>>> >>>>> >>>>> I think we should define a convention about how to sort these nodes >>>>> before we actually start merging some of it. >>>>> >>>>> This of course also apply to the other patches doing that, hence why >>>>> Hans is CC'd. >>>>> >>>>> I guess sorting them by label alphabetical order would make >>>>> sense. What do you think? >>>> >>>> >>>> I'm currently using the ordering from the dtsi, which is based >>>> on address. Even if it's not visible, if you're creating the >>>> dts by looking at the dtsi and enabling the devices available, >>>> that's the order you add them by, so it kind of makes sense. >> >> >> Right, that is what I'm doing too. >> >>> I know you're doing just that, and that it makes some kind of sense >>> whenever you convert an old DTS to the label based syntax, but >>> whenever you create a new one, it's a bit harder to get it right. >>> >>> And the fact that Hans didn't follow that convention illustrate that >>> very well. >> >> >> Erm, that is not true, I did follow that convention as my 2 new >> dts files started in the old format too, and I simply re-indented >> most nodes. > > I think it also makes sense for new boards as well. One would look > at the dtsi to know what devices on the soc are available, and > copy the ones that can be used. > > But the ordering does make adding new devices with new drivers > a bit problematic. It is easy to place new nodes in the wrong > place. > >>> I guess a sorting logic internal to the DTS itself would be much >>> easier to understand and follow, hence why I suggested the >>> alphabetical order: it just stands out without any external reference. >> >> >> If we agree on this, we also need to think about what to do with new >> board specific powersupplies, as those need to be in the root node, >> if you look at the bananapro dts from the v2 patch you will see what >> I mean. > > How about the root node at the top? With the board description, chosen, > aliases (if any), and then the _new_ regulators (sorted by name)? > > One can then easily identify which are board specific, and which are > more or less generic. Sure, that works for me, what about the leds section, also in the root node at the top? Before or after regulators ? Regards, Hans
On Wed, Jan 14, 2015 at 4:46 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > > On 14-01-15 09:41, Chen-Yu Tsai wrote: >> >> On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> >> wrote: >>> >>> Hi, >>> >>> >>> On 13-01-15 17:20, Maxime Ripard wrote: >>>> >>>> >>>> On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >>>>> >>>>> >>>>> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >>>>> <maxime.ripard@free-electrons.com> wrote: >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>>>>> >>>>>>> >>>>>>> Using label references is preferred when override settings from the >>>>>>> included dtsi. >>>>>>> >>>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>>> --- >>>>>>> >>>>>>> My AXP221 series touches this file. I thought I'd convert it first. >>>>>>> >>>>>>> This looks like a lot of changes. But if you filter out all the >>>>>>> indentation changes, it's just the opening lines for each node. >>>>>>> >>>>>>> --- >>>>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 >>>>>>> ++++++++++++++-------------- >>>>>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>>>>> >>>>>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>> b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>>>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>> @@ -61,101 +61,96 @@ >>>>>>> chosen { >>>>>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>>>>> }; >>>>>>> +}; >>>>>>> + >>>>>>> +&mmc0 { >>>>>>> + pinctrl-names = "default"; >>>>>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>>>>> + vmmc-supply = <®_vcc3v0>; >>>>>>> + bus-width = <4>; >>>>>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>>>>> + cd-inverted; >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>>> + >>>>>>> +&usbphy { >>>>>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>>> + >>>>>>> +&ehci0 { >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>>> + >>>>>>> +&ohci0 { >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>>> + >>>>>>> +&pio { >>>>>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>>>>> + allwinner,pins = "PA8"; >>>>>>> + allwinner,function = "gpio_in"; >>>>>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>> + }; >>>>>>> +}; >>>>>>> + >>>>>>> +&mmc0_pins_a { >>>>>>> + /* external pull-ups missing for some pins */ >>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>> +}; >>>>>>> + >>>>>>> +&usb1_vbus_pin_a { >>>>>>> + /* different pin from sunxi-common-regulators */ >>>>>>> + allwinner,pins = "PH24"; >>>>>>> +}; >>>>>>> + >>>>>>> +&uart0 { >>>>>>> + pinctrl-names = "default"; >>>>>>> + pinctrl-0 = <&uart0_pins_a>; >>>>>>> + status = "okay"; >>>>>>> +}; >>>>>>> + >>>>>>> +&i2c0 { >>>>>>> + pinctrl-names = "default"; >>>>>>> + pinctrl-0 = <&i2c0_pins_a>; >>>>>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>>>>> + status = "failed"; >>>>>>> +}; >>>>>> >>>>>> >>>>>> >>>>>> I think we should define a convention about how to sort these nodes >>>>>> before we actually start merging some of it. >>>>>> >>>>>> This of course also apply to the other patches doing that, hence why >>>>>> Hans is CC'd. >>>>>> >>>>>> I guess sorting them by label alphabetical order would make >>>>>> sense. What do you think? >>>>> >>>>> >>>>> >>>>> I'm currently using the ordering from the dtsi, which is based >>>>> on address. Even if it's not visible, if you're creating the >>>>> dts by looking at the dtsi and enabling the devices available, >>>>> that's the order you add them by, so it kind of makes sense. >>> >>> >>> >>> Right, that is what I'm doing too. >>> >>>> I know you're doing just that, and that it makes some kind of sense >>>> whenever you convert an old DTS to the label based syntax, but >>>> whenever you create a new one, it's a bit harder to get it right. >>>> >>>> And the fact that Hans didn't follow that convention illustrate that >>>> very well. >>> >>> >>> >>> Erm, that is not true, I did follow that convention as my 2 new >>> dts files started in the old format too, and I simply re-indented >>> most nodes. >> >> >> I think it also makes sense for new boards as well. One would look >> at the dtsi to know what devices on the soc are available, and >> copy the ones that can be used. >> >> But the ordering does make adding new devices with new drivers >> a bit problematic. It is easy to place new nodes in the wrong >> place. >> >>>> I guess a sorting logic internal to the DTS itself would be much >>>> easier to understand and follow, hence why I suggested the >>>> alphabetical order: it just stands out without any external reference. >>> >>> >>> >>> If we agree on this, we also need to think about what to do with new >>> board specific powersupplies, as those need to be in the root node, >>> if you look at the bananapro dts from the v2 patch you will see what >>> I mean. >> >> >> How about the root node at the top? With the board description, chosen, >> aliases (if any), and then the _new_ regulators (sorted by name)? >> >> One can then easily identify which are board specific, and which are >> more or less generic. > > > Sure, that works for me, what about the leds section, also in the > root node at the top? Before or after regulators ? Before the regulators? Because a) L comes before R and b) this is what's currently done in our dts files. ChenYu
Hi, On 14-01-15 10:29, Chen-Yu Tsai wrote: > On Wed, Jan 14, 2015 at 4:46 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> >> On 14-01-15 09:41, Chen-Yu Tsai wrote: >>> >>> On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> >>> wrote: >>>> >>>> Hi, >>>> >>>> >>>> On 13-01-15 17:20, Maxime Ripard wrote: >>>>> >>>>> >>>>> On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >>>>>> >>>>>> >>>>>> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >>>>>> <maxime.ripard@free-electrons.com> wrote: >>>>>>> >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>>>>>> >>>>>>>> >>>>>>>> Using label references is preferred when override settings from the >>>>>>>> included dtsi. >>>>>>>> >>>>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>>>> --- >>>>>>>> >>>>>>>> My AXP221 series touches this file. I thought I'd convert it first. >>>>>>>> >>>>>>>> This looks like a lot of changes. But if you filter out all the >>>>>>>> indentation changes, it's just the opening lines for each node. >>>>>>>> >>>>>>>> --- >>>>>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 >>>>>>>> ++++++++++++++-------------- >>>>>>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>>>>>> >>>>>>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>> b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>>>>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>> @@ -61,101 +61,96 @@ >>>>>>>> chosen { >>>>>>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>>>>>> }; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&mmc0 { >>>>>>>> + pinctrl-names = "default"; >>>>>>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>>>>>> + vmmc-supply = <®_vcc3v0>; >>>>>>>> + bus-width = <4>; >>>>>>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>>>>>> + cd-inverted; >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&usbphy { >>>>>>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&ehci0 { >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&ohci0 { >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&pio { >>>>>>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>>>>>> + allwinner,pins = "PA8"; >>>>>>>> + allwinner,function = "gpio_in"; >>>>>>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>>> + }; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&mmc0_pins_a { >>>>>>>> + /* external pull-ups missing for some pins */ >>>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&usb1_vbus_pin_a { >>>>>>>> + /* different pin from sunxi-common-regulators */ >>>>>>>> + allwinner,pins = "PH24"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&uart0 { >>>>>>>> + pinctrl-names = "default"; >>>>>>>> + pinctrl-0 = <&uart0_pins_a>; >>>>>>>> + status = "okay"; >>>>>>>> +}; >>>>>>>> + >>>>>>>> +&i2c0 { >>>>>>>> + pinctrl-names = "default"; >>>>>>>> + pinctrl-0 = <&i2c0_pins_a>; >>>>>>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>>>>>> + status = "failed"; >>>>>>>> +}; >>>>>>> >>>>>>> >>>>>>> >>>>>>> I think we should define a convention about how to sort these nodes >>>>>>> before we actually start merging some of it. >>>>>>> >>>>>>> This of course also apply to the other patches doing that, hence why >>>>>>> Hans is CC'd. >>>>>>> >>>>>>> I guess sorting them by label alphabetical order would make >>>>>>> sense. What do you think? >>>>>> >>>>>> >>>>>> >>>>>> I'm currently using the ordering from the dtsi, which is based >>>>>> on address. Even if it's not visible, if you're creating the >>>>>> dts by looking at the dtsi and enabling the devices available, >>>>>> that's the order you add them by, so it kind of makes sense. >>>> >>>> >>>> >>>> Right, that is what I'm doing too. >>>> >>>>> I know you're doing just that, and that it makes some kind of sense >>>>> whenever you convert an old DTS to the label based syntax, but >>>>> whenever you create a new one, it's a bit harder to get it right. >>>>> >>>>> And the fact that Hans didn't follow that convention illustrate that >>>>> very well. >>>> >>>> >>>> >>>> Erm, that is not true, I did follow that convention as my 2 new >>>> dts files started in the old format too, and I simply re-indented >>>> most nodes. >>> >>> >>> I think it also makes sense for new boards as well. One would look >>> at the dtsi to know what devices on the soc are available, and >>> copy the ones that can be used. >>> >>> But the ordering does make adding new devices with new drivers >>> a bit problematic. It is easy to place new nodes in the wrong >>> place. >>> >>>>> I guess a sorting logic internal to the DTS itself would be much >>>>> easier to understand and follow, hence why I suggested the >>>>> alphabetical order: it just stands out without any external reference. >>>> >>>> >>>> >>>> If we agree on this, we also need to think about what to do with new >>>> board specific powersupplies, as those need to be in the root node, >>>> if you look at the bananapro dts from the v2 patch you will see what >>>> I mean. >>> >>> >>> How about the root node at the top? With the board description, chosen, >>> aliases (if any), and then the _new_ regulators (sorted by name)? >>> >>> One can then easily identify which are board specific, and which are >>> more or less generic. >> >> >> Sure, that works for me, what about the leds section, also in the >> root node at the top? Before or after regulators ? > > Before the regulators? Because a) L comes before R and b) this > is what's currently done in our dts files. OK, Maxime, do you agree ? Regards, Hans
On Wed, Jan 14, 2015 at 11:17:00AM +0100, Hans de Goede wrote: > Hi, > > On 14-01-15 10:29, Chen-Yu Tsai wrote: > >On Wed, Jan 14, 2015 at 4:46 PM, Hans de Goede <hdegoede@redhat.com> wrote: > >>Hi, > >> > >> > >>On 14-01-15 09:41, Chen-Yu Tsai wrote: > >>> > >>>On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> > >>>wrote: > >>>> > >>>>Hi, > >>>> > >>>> > >>>>On 13-01-15 17:20, Maxime Ripard wrote: > >>>>> > >>>>> > >>>>>On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: > >>>>>> > >>>>>> > >>>>>>On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard > >>>>>><maxime.ripard@free-electrons.com> wrote: > >>>>>>> > >>>>>>> > >>>>>>>Hi, > >>>>>>> > >>>>>>>On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>>Using label references is preferred when override settings from the > >>>>>>>>included dtsi. > >>>>>>>> > >>>>>>>>Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >>>>>>>>--- > >>>>>>>> > >>>>>>>>My AXP221 series touches this file. I thought I'd convert it first. > >>>>>>>> > >>>>>>>>This looks like a lot of changes. But if you filter out all the > >>>>>>>>indentation changes, it's just the opening lines for each node. > >>>>>>>> > >>>>>>>>--- > >>>>>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 > >>>>>>>>++++++++++++++-------------- > >>>>>>>> 1 file changed, 88 insertions(+), 93 deletions(-) > >>>>>>>> > >>>>>>>>diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >>>>>>>>b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >>>>>>>>index ebd5f7854b1b..97dbaeb76416 100644 > >>>>>>>>--- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >>>>>>>>+++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts > >>>>>>>>@@ -61,101 +61,96 @@ > >>>>>>>> chosen { > >>>>>>>> bootargs = "earlyprintk console=ttyS0,115200"; > >>>>>>>> }; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&mmc0 { > >>>>>>>>+ pinctrl-names = "default"; > >>>>>>>>+ pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; > >>>>>>>>+ vmmc-supply = <®_vcc3v0>; > >>>>>>>>+ bus-width = <4>; > >>>>>>>>+ cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ > >>>>>>>>+ cd-inverted; > >>>>>>>>+ status = "okay"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&usbphy { > >>>>>>>>+ usb1_vbus-supply = <®_usb1_vbus>; > >>>>>>>>+ status = "okay"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&ehci0 { > >>>>>>>>+ status = "okay"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&ohci0 { > >>>>>>>>+ status = "okay"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&pio { > >>>>>>>>+ mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { > >>>>>>>>+ allwinner,pins = "PA8"; > >>>>>>>>+ allwinner,function = "gpio_in"; > >>>>>>>>+ allwinner,drive = <SUN4I_PINCTRL_10_MA>; > >>>>>>>>+ allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > >>>>>>>>+ }; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&mmc0_pins_a { > >>>>>>>>+ /* external pull-ups missing for some pins */ > >>>>>>>>+ allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&usb1_vbus_pin_a { > >>>>>>>>+ /* different pin from sunxi-common-regulators */ > >>>>>>>>+ allwinner,pins = "PH24"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&uart0 { > >>>>>>>>+ pinctrl-names = "default"; > >>>>>>>>+ pinctrl-0 = <&uart0_pins_a>; > >>>>>>>>+ status = "okay"; > >>>>>>>>+}; > >>>>>>>>+ > >>>>>>>>+&i2c0 { > >>>>>>>>+ pinctrl-names = "default"; > >>>>>>>>+ pinctrl-0 = <&i2c0_pins_a>; > >>>>>>>>+ /* pull-ups and devices require AXP221 DLDO3 */ > >>>>>>>>+ status = "failed"; > >>>>>>>>+}; > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>>I think we should define a convention about how to sort these nodes > >>>>>>>before we actually start merging some of it. > >>>>>>> > >>>>>>>This of course also apply to the other patches doing that, hence why > >>>>>>>Hans is CC'd. > >>>>>>> > >>>>>>>I guess sorting them by label alphabetical order would make > >>>>>>>sense. What do you think? > >>>>>> > >>>>>> > >>>>>> > >>>>>>I'm currently using the ordering from the dtsi, which is based > >>>>>>on address. Even if it's not visible, if you're creating the > >>>>>>dts by looking at the dtsi and enabling the devices available, > >>>>>>that's the order you add them by, so it kind of makes sense. > >>>> > >>>> > >>>> > >>>>Right, that is what I'm doing too. > >>>> > >>>>>I know you're doing just that, and that it makes some kind of sense > >>>>>whenever you convert an old DTS to the label based syntax, but > >>>>>whenever you create a new one, it's a bit harder to get it right. > >>>>> > >>>>>And the fact that Hans didn't follow that convention illustrate that > >>>>>very well. > >>>> > >>>> > >>>> > >>>>Erm, that is not true, I did follow that convention as my 2 new > >>>>dts files started in the old format too, and I simply re-indented > >>>>most nodes. > >>> > >>> > >>>I think it also makes sense for new boards as well. One would look > >>>at the dtsi to know what devices on the soc are available, and > >>>copy the ones that can be used. > >>> > >>>But the ordering does make adding new devices with new drivers > >>>a bit problematic. It is easy to place new nodes in the wrong > >>>place. > >>> > >>>>>I guess a sorting logic internal to the DTS itself would be much > >>>>>easier to understand and follow, hence why I suggested the > >>>>>alphabetical order: it just stands out without any external reference. > >>>> > >>>> > >>>> > >>>>If we agree on this, we also need to think about what to do with new > >>>>board specific powersupplies, as those need to be in the root node, > >>>>if you look at the bananapro dts from the v2 patch you will see what > >>>>I mean. > >>> > >>> > >>>How about the root node at the top? With the board description, chosen, > >>>aliases (if any), and then the _new_ regulators (sorted by name)? > >>> > >>>One can then easily identify which are board specific, and which are > >>>more or less generic. > >> > >> > >>Sure, that works for me, what about the leds section, also in the > >>root node at the top? Before or after regulators ? > > > >Before the regulators? Because a) L comes before R and b) this > >is what's currently done in our dts files. > > OK, Maxime, do you agree ? See, we're getting to the alphabetical order :) I don't what your workflow is, but usually I start from a close DTS, never from a DTSI. We will indeed still have the root node with the leds, regulators, buttons, any board-specific platform driver actually. It indeed makes sense to place that on top. And if we're going to have an alphabetical order within that node, won't it be weird if we have what looks like a random one below? Maxime
Hi, On 14-01-15 20:46, Maxime Ripard wrote: > On Wed, Jan 14, 2015 at 11:17:00AM +0100, Hans de Goede wrote: >> Hi, >> >> On 14-01-15 10:29, Chen-Yu Tsai wrote: >>> On Wed, Jan 14, 2015 at 4:46 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> >>>> On 14-01-15 09:41, Chen-Yu Tsai wrote: >>>>> >>>>> On Wed, Jan 14, 2015 at 3:29 PM, Hans de Goede <hdegoede@redhat.com> >>>>> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> >>>>>> On 13-01-15 17:20, Maxime Ripard wrote: >>>>>>> >>>>>>> >>>>>>> On Tue, Jan 13, 2015 at 11:54:21PM +0800, Chen-Yu Tsai wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Tue, Jan 13, 2015 at 11:44 PM, Maxime Ripard >>>>>>>> <maxime.ripard@free-electrons.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Tue, Jan 13, 2015 at 12:31:24PM +0800, Chen-Yu Tsai wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Using label references is preferred when override settings from the >>>>>>>>>> included dtsi. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> My AXP221 series touches this file. I thought I'd convert it first. >>>>>>>>>> >>>>>>>>>> This looks like a lot of changes. But if you filter out all the >>>>>>>>>> indentation changes, it's just the opening lines for each node. >>>>>>>>>> >>>>>>>>>> --- >>>>>>>>>> arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 >>>>>>>>>> ++++++++++++++-------------- >>>>>>>>>> 1 file changed, 88 insertions(+), 93 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>>>> b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>>>> index ebd5f7854b1b..97dbaeb76416 100644 >>>>>>>>>> --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>>>> +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts >>>>>>>>>> @@ -61,101 +61,96 @@ >>>>>>>>>> chosen { >>>>>>>>>> bootargs = "earlyprintk console=ttyS0,115200"; >>>>>>>>>> }; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&mmc0 { >>>>>>>>>> + pinctrl-names = "default"; >>>>>>>>>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; >>>>>>>>>> + vmmc-supply = <®_vcc3v0>; >>>>>>>>>> + bus-width = <4>; >>>>>>>>>> + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ >>>>>>>>>> + cd-inverted; >>>>>>>>>> + status = "okay"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&usbphy { >>>>>>>>>> + usb1_vbus-supply = <®_usb1_vbus>; >>>>>>>>>> + status = "okay"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&ehci0 { >>>>>>>>>> + status = "okay"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&ohci0 { >>>>>>>>>> + status = "okay"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&pio { >>>>>>>>>> + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { >>>>>>>>>> + allwinner,pins = "PA8"; >>>>>>>>>> + allwinner,function = "gpio_in"; >>>>>>>>>> + allwinner,drive = <SUN4I_PINCTRL_10_MA>; >>>>>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>>>>> + }; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&mmc0_pins_a { >>>>>>>>>> + /* external pull-ups missing for some pins */ >>>>>>>>>> + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&usb1_vbus_pin_a { >>>>>>>>>> + /* different pin from sunxi-common-regulators */ >>>>>>>>>> + allwinner,pins = "PH24"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&uart0 { >>>>>>>>>> + pinctrl-names = "default"; >>>>>>>>>> + pinctrl-0 = <&uart0_pins_a>; >>>>>>>>>> + status = "okay"; >>>>>>>>>> +}; >>>>>>>>>> + >>>>>>>>>> +&i2c0 { >>>>>>>>>> + pinctrl-names = "default"; >>>>>>>>>> + pinctrl-0 = <&i2c0_pins_a>; >>>>>>>>>> + /* pull-ups and devices require AXP221 DLDO3 */ >>>>>>>>>> + status = "failed"; >>>>>>>>>> +}; >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> I think we should define a convention about how to sort these nodes >>>>>>>>> before we actually start merging some of it. >>>>>>>>> >>>>>>>>> This of course also apply to the other patches doing that, hence why >>>>>>>>> Hans is CC'd. >>>>>>>>> >>>>>>>>> I guess sorting them by label alphabetical order would make >>>>>>>>> sense. What do you think? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> I'm currently using the ordering from the dtsi, which is based >>>>>>>> on address. Even if it's not visible, if you're creating the >>>>>>>> dts by looking at the dtsi and enabling the devices available, >>>>>>>> that's the order you add them by, so it kind of makes sense. >>>>>> >>>>>> >>>>>> >>>>>> Right, that is what I'm doing too. >>>>>> >>>>>>> I know you're doing just that, and that it makes some kind of sense >>>>>>> whenever you convert an old DTS to the label based syntax, but >>>>>>> whenever you create a new one, it's a bit harder to get it right. >>>>>>> >>>>>>> And the fact that Hans didn't follow that convention illustrate that >>>>>>> very well. >>>>>> >>>>>> >>>>>> >>>>>> Erm, that is not true, I did follow that convention as my 2 new >>>>>> dts files started in the old format too, and I simply re-indented >>>>>> most nodes. >>>>> >>>>> >>>>> I think it also makes sense for new boards as well. One would look >>>>> at the dtsi to know what devices on the soc are available, and >>>>> copy the ones that can be used. >>>>> >>>>> But the ordering does make adding new devices with new drivers >>>>> a bit problematic. It is easy to place new nodes in the wrong >>>>> place. >>>>> >>>>>>> I guess a sorting logic internal to the DTS itself would be much >>>>>>> easier to understand and follow, hence why I suggested the >>>>>>> alphabetical order: it just stands out without any external reference. >>>>>> >>>>>> >>>>>> >>>>>> If we agree on this, we also need to think about what to do with new >>>>>> board specific powersupplies, as those need to be in the root node, >>>>>> if you look at the bananapro dts from the v2 patch you will see what >>>>>> I mean. >>>>> >>>>> >>>>> How about the root node at the top? With the board description, chosen, >>>>> aliases (if any), and then the _new_ regulators (sorted by name)? >>>>> >>>>> One can then easily identify which are board specific, and which are >>>>> more or less generic. >>>> >>>> >>>> Sure, that works for me, what about the leds section, also in the >>>> root node at the top? Before or after regulators ? >>> >>> Before the regulators? Because a) L comes before R and b) this >>> is what's currently done in our dts files. >> >> OK, Maxime, do you agree ? > > See, we're getting to the alphabetical order :) > > I don't what your workflow is, but usually I start from a close DTS, > never from a DTSI. > > We will indeed still have the root node with the leds, regulators, > buttons, any board-specific platform driver actually. It indeed makes > sense to place that on top. > > And if we're going to have an alphabetical order within that node, > won't it be weird if we have what looks like a random one below? Ok, so I'll respin my recent set with 2 board additions to follow this scheme. Regards, Hans
diff --git a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts index ebd5f7854b1b..97dbaeb76416 100644 --- a/arch/arm/boot/dts/sun6i-a31-hummingbird.dts +++ b/arch/arm/boot/dts/sun6i-a31-hummingbird.dts @@ -61,101 +61,96 @@ chosen { bootargs = "earlyprintk console=ttyS0,115200"; }; +}; + +&mmc0 { + pinctrl-names = "default"; + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; + vmmc-supply = <®_vcc3v0>; + bus-width = <4>; + cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ + cd-inverted; + status = "okay"; +}; + +&usbphy { + usb1_vbus-supply = <®_usb1_vbus>; + status = "okay"; +}; + +&ehci0 { + status = "okay"; +}; + +&ohci0 { + status = "okay"; +}; + +&pio { + mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { + allwinner,pins = "PA8"; + allwinner,function = "gpio_in"; + allwinner,drive = <SUN4I_PINCTRL_10_MA>; + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; + }; +}; + +&mmc0_pins_a { + /* external pull-ups missing for some pins */ + allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; +}; + +&usb1_vbus_pin_a { + /* different pin from sunxi-common-regulators */ + allwinner,pins = "PH24"; +}; + +&uart0 { + pinctrl-names = "default"; + pinctrl-0 = <&uart0_pins_a>; + status = "okay"; +}; + +&i2c0 { + pinctrl-names = "default"; + pinctrl-0 = <&i2c0_pins_a>; + /* pull-ups and devices require AXP221 DLDO3 */ + status = "failed"; +}; - soc@01c00000 { - mmc0: mmc@01c0f000 { - pinctrl-names = "default"; - pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin_hummingbird>; - vmmc-supply = <®_vcc3v0>; - bus-width = <4>; - cd-gpios = <&pio 0 8 GPIO_ACTIVE_HIGH>; /* PA8 */ - cd-inverted; - status = "okay"; - }; - - usbphy: phy@01c19400 { - usb1_vbus-supply = <®_usb1_vbus>; - status = "okay"; - }; - - ehci0: usb@01c1a000 { - status = "okay"; - }; - - ohci0: usb@01c1a400 { - status = "okay"; - }; - - pio: pinctrl@01c20800 { - mmc0_pins_a: mmc0@0 { - /* external pull-ups missing for some pins */ - allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; - }; - - mmc0_cd_pin_hummingbird: mmc0_cd_pin@0 { - allwinner,pins = "PA8"; - allwinner,function = "gpio_in"; - allwinner,drive = <SUN4I_PINCTRL_10_MA>; - allwinner,pull = <SUN4I_PINCTRL_PULL_UP>; - }; - - usb1_vbus_pin_a: usb1_vbus_pin@0 { - allwinner,pins = "PH24"; - allwinner,function = "gpio_out"; - allwinner,drive = <SUN4I_PINCTRL_10_MA>; - allwinner,pull = <SUN4I_PINCTRL_NO_PULL>; - }; - }; - - uart0: serial@01c28000 { - pinctrl-names = "default"; - pinctrl-0 = <&uart0_pins_a>; - status = "okay"; - }; - - i2c0: i2c@01c2ac00 { - pinctrl-names = "default"; - pinctrl-0 = <&i2c0_pins_a>; - /* pull-ups and devices require AXP221 DLDO3 */ - status = "failed"; - }; - - i2c1: i2c@01c2b000 { - pinctrl-names = "default"; - pinctrl-0 = <&i2c1_pins_a>; - status = "okay"; - }; - - i2c2: i2c@01c2b400 { - pinctrl-names = "default"; - pinctrl-0 = <&i2c2_pins_a>; - status = "okay"; - - pcf8563: rtc@51 { - compatible = "nxp,pcf8563"; - reg = <0x51>; - }; - }; - - gmac: ethernet@01c30000 { - pinctrl-names = "default"; - pinctrl-0 = <&gmac_pins_rgmii_a>; - phy = <&phy1>; - phy-mode = "rgmii"; - snps,reset-gpio = <&pio 0 21 GPIO_ACTIVE_HIGH>; - snps,reset-active-low; - snps,reset-delays-us = <0 10000 30000>; - status = "okay"; - - phy1: ethernet-phy@1 { - reg = <1>; - }; - }; +&i2c1 { + pinctrl-names = "default"; + pinctrl-0 = <&i2c1_pins_a>; + status = "okay"; +}; + +&i2c2 { + pinctrl-names = "default"; + pinctrl-0 = <&i2c2_pins_a>; + status = "okay"; + + pcf8563: rtc@51 { + compatible = "nxp,pcf8563"; + reg = <0x51>; }; +}; - reg_usb1_vbus: usb1-vbus { - pinctrl-0 = <&usb1_vbus_pin_a>; - gpio = <&pio 7 24 GPIO_ACTIVE_HIGH>; /* PH24 */ - status = "okay"; +&gmac { + pinctrl-names = "default"; + pinctrl-0 = <&gmac_pins_rgmii_a>; + phy = <&phy1>; + phy-mode = "rgmii"; + snps,reset-gpio = <&pio 0 21 GPIO_ACTIVE_HIGH>; + snps,reset-active-low; + snps,reset-delays-us = <0 10000 30000>; + status = "okay"; + + phy1: ethernet-phy@1 { + reg = <1>; }; }; + +®_usb1_vbus { + gpio = <&pio 7 24 GPIO_ACTIVE_HIGH>; /* PH24 */ + status = "okay"; +};
Using label references is preferred when override settings from the included dtsi. Signed-off-by: Chen-Yu Tsai <wens@csie.org> --- My AXP221 series touches this file. I thought I'd convert it first. This looks like a lot of changes. But if you filter out all the indentation changes, it's just the opening lines for each node. --- arch/arm/boot/dts/sun6i-a31-hummingbird.dts | 181 ++++++++++++++-------------- 1 file changed, 88 insertions(+), 93 deletions(-)