On 08/03/2023 13:52, Philippe Schenker wrote:
> From: Philippe Schenker <[email protected]>
>
> Sort properties according to the following order and inside these
> alphabetically.
>
> 1. compatible
> 2. reg
> 3. standard properties
> 4. specific properties
> 5. status
Is this approved coding style for IMX DTS?
>
> Signed-off-by: Philippe Schenker <[email protected]>
> ---
>
> .../boot/dts/freescale/imx8x-colibri.dtsi | 142 +++++++++---------
> 1 file changed, 71 insertions(+), 71 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8x-colibri.dtsi b/arch/arm64/boot/dts/freescale/imx8x-colibri.dtsi
> index 12056b77d22e..6f86a83bc957 100644
> --- a/arch/arm64/boot/dts/freescale/imx8x-colibri.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8x-colibri.dtsi
> @@ -10,9 +10,9 @@ chosen {
>
> reg_module_3v3: regulator-module-3v3 {
> compatible = "regulator-fixed";
> - regulator-name = "+V3.3";
> - regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> + regulator-min-microvolt = <3300000>;
> +
Best regards,
Krzysztof
On Wed, 2023-03-08 at 13:57 +0100, Krzysztof Kozlowski wrote:
> Is this approved coding style for IMX DTS?
How the ordering should be done is nowhere specifically documented (at
least this is my current understanding).
The ordering how I noted it is what we gathered from multiple feedback
on mailinglist discussions.
With that ordering I hope everyone is happy.
Philippe
On 08/03/2023 14:29, Philippe Schenker wrote:
> On Wed, 2023-03-08 at 13:57 +0100, Krzysztof Kozlowski wrote:
>> Is this approved coding style for IMX DTS?
>
> How the ordering should be done is nowhere specifically documented (at
> least this is my current understanding).
> The ordering how I noted it is what we gathered from multiple feedback
> on mailinglist discussions.
>
> With that ordering I hope everyone is happy.
>
> Philippe
Yeah, but what if next developer next month re-orders all your nodes
again because he will use different coding style?
Best regards,
Krzysztof
On Wed, 2023-03-08 at 15:32 +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 14:29, Philippe Schenker wrote:
> > On Wed, 2023-03-08 at 13:57 +0100, Krzysztof Kozlowski wrote:
> > > Is this approved coding style for IMX DTS?
> >
> > How the ordering should be done is nowhere specifically documented
> > (at
> > least this is my current understanding).
> > The ordering how I noted it is what we gathered from multiple
> > feedback
> > on mailinglist discussions.
> >
> > With that ordering I hope everyone is happy.
> >
> > Philippe
>
> Yeah, but what if next developer next month re-orders all your nodes
> again because he will use different coding style?
Someone from Toradex will complain that we want to have it the way I
sent now, since this is the way we agreed on internally.
>
> Best regards,
> Krzysztof
>
On 08/03/2023 15:50, Philippe Schenker wrote:
> On Wed, 2023-03-08 at 15:32 +0100, Krzysztof Kozlowski wrote:
>> On 08/03/2023 14:29, Philippe Schenker wrote:
>>> On Wed, 2023-03-08 at 13:57 +0100, Krzysztof Kozlowski wrote:
>>>> Is this approved coding style for IMX DTS?
>>>
>>> How the ordering should be done is nowhere specifically documented
>>> (at
>>> least this is my current understanding).
>>> The ordering how I noted it is what we gathered from multiple
>>> feedback
>>> on mailinglist discussions.
>>>
>>> With that ordering I hope everyone is happy.
>>>
>>> Philippe
>>
>> Yeah, but what if next developer next month re-orders all your nodes
>> again because he will use different coding style?
>
> Someone from Toradex will complain that we want to have it the way I
> sent now, since this is the way we agreed on internally.
I am sorry, but coding style is per subsystem, not per Toradex boards.
Even if my example is not applicable, someone might come soon with
something different for entire iMX and again re-order it.
Either make a rule for entire iMX or do not make re-shufflings. They are
not useful.
Best regards,
Krzysztof
Hello Krzysztof, first thanks for your review.
Let's try to get some clarity on this with the help of Shawn.
On Wed, Mar 08, 2023 at 01:57:38PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 13:52, Philippe Schenker wrote:
> > From: Philippe Schenker <[email protected]>
> >
> > Sort properties according to the following order and inside these
> > alphabetically.
> >
> > 1. compatible
> > 2. reg
> > 3. standard properties
> > 4. specific properties
> > 5. status
>
> Is this approved coding style for IMX DTS?
I 100% understand your concerns here.
With that said let me try to briefly explain the reasoning here, in
various threads we were asked in the past to move node around based on
some not 100% defined rules [0][1].
On Sun, 2023-01-29 at 11:19 +0800, Shawn Guo wrote:
>> +&usbotg1 {
>> + adp-disable;
>> + ci-disable-lpm;
>> + hnp-disable;
>> + over-current-active-low;
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&pinctrl_usbotg1>;
>
>We generally want to put such generic properties before device specific
>ones.
In addition to that we find convenient to have properties sorted
alphabetically when no other rule is available, it just prevents any
kind of discussion, minimize merge conflicts and make comparing files
easier.
I also agree that the difference between "generic"/"specific" is fuzzy
at best.
With all that said ...
Shawn: What should we do? We can of course avoid any kind of re-ordering
from now on.
I am fine to be very pragmatic here, no-reordering on existing DTS
files, newly added DTS files we discuss whatever is the reasoning of the
reviewer/maintainer on a case-by-case basis.
Francesco
[0] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/all/20230129031932.GO20713@T480/
On Thu, Mar 09, 2023 at 01:19:13PM +0100, Francesco Dolcini wrote:
> Hello Krzysztof, first thanks for your review.
>
> Let's try to get some clarity on this with the help of Shawn.
>
> On Wed, Mar 08, 2023 at 01:57:38PM +0100, Krzysztof Kozlowski wrote:
> > On 08/03/2023 13:52, Philippe Schenker wrote:
> > > From: Philippe Schenker <[email protected]>
> > >
> > > Sort properties according to the following order and inside these
> > > alphabetically.
> > >
> > > 1. compatible
> > > 2. reg
> > > 3. standard properties
> > > 4. specific properties
> > > 5. status
> >
> > Is this approved coding style for IMX DTS?
>
> I 100% understand your concerns here.
>
> With that said let me try to briefly explain the reasoning here, in
> various threads we were asked in the past to move node around based on
> some not 100% defined rules [0][1].
>
> On Sun, 2023-01-29 at 11:19 +0800, Shawn Guo wrote:
> >> +&usbotg1 {
> >> + adp-disable;
> >> + ci-disable-lpm;
> >> + hnp-disable;
> >> + over-current-active-low;
> >> + pinctrl-names = "default";
> >> + pinctrl-0 = <&pinctrl_usbotg1>;
> >
> >We generally want to put such generic properties before device specific
> >ones.
>
> In addition to that we find convenient to have properties sorted
> alphabetically when no other rule is available, it just prevents any
> kind of discussion, minimize merge conflicts and make comparing files
> easier.
>
> I also agree that the difference between "generic"/"specific" is fuzzy
> at best.
>
> With all that said ...
>
> Shawn: What should we do? We can of course avoid any kind of re-ordering
> from now on.
We are practically asking for 1, 2 and 5 for i.MX DTS files, but pretty
flexible for the rest.
> I am fine to be very pragmatic here, no-reordering on existing DTS
> files, newly added DTS files we discuss whatever is the reasoning of the
> reviewer/maintainer on a case-by-case basis.
Sounds good to me! While I personally like your ordering, I do not want
it to churn the existing DTS files.
I'm happy to take this patch as a special case though :)
Shawn
On Tue, Mar 14, 2023 at 04:17:35PM +0800, Shawn Guo wrote:
> On Thu, Mar 09, 2023 at 01:19:13PM +0100, Francesco Dolcini wrote:
> > Hello Krzysztof, first thanks for your review.
> >
> > Let's try to get some clarity on this with the help of Shawn.
> >
> > On Wed, Mar 08, 2023 at 01:57:38PM +0100, Krzysztof Kozlowski wrote:
> > > On 08/03/2023 13:52, Philippe Schenker wrote:
> > > > From: Philippe Schenker <[email protected]>
> > > >
> > > > Sort properties according to the following order and inside these
> > > > alphabetically.
> > > >
> > > > 1. compatible
> > > > 2. reg
> > > > 3. standard properties
> > > > 4. specific properties
> > > > 5. status
> > >
> > > Is this approved coding style for IMX DTS?
> >
> > I 100% understand your concerns here.
> >
> > With that said let me try to briefly explain the reasoning here, in
> > various threads we were asked in the past to move node around based on
> > some not 100% defined rules [0][1].
> >
> > On Sun, 2023-01-29 at 11:19 +0800, Shawn Guo wrote:
> > >> +&usbotg1 {
> > >> + adp-disable;
> > >> + ci-disable-lpm;
> > >> + hnp-disable;
> > >> + over-current-active-low;
> > >> + pinctrl-names = "default";
> > >> + pinctrl-0 = <&pinctrl_usbotg1>;
> > >
> > >We generally want to put such generic properties before device specific
> > >ones.
> >
> > In addition to that we find convenient to have properties sorted
> > alphabetically when no other rule is available, it just prevents any
> > kind of discussion, minimize merge conflicts and make comparing files
> > easier.
> >
> > I also agree that the difference between "generic"/"specific" is fuzzy
> > at best.
> >
> > With all that said ...
> >
> > Shawn: What should we do? We can of course avoid any kind of re-ordering
> > from now on.
>
> We are practically asking for 1, 2 and 5 for i.MX DTS files, but pretty
> flexible for the rest.
>
> > I am fine to be very pragmatic here, no-reordering on existing DTS
> > files, newly added DTS files we discuss whatever is the reasoning of the
> > reviewer/maintainer on a case-by-case basis.
>
> Sounds good to me! While I personally like your ordering, I do not want
> it to churn the existing DTS files.
Agreed.
>
> I'm happy to take this patch as a special case though :)
Philippe just rebased all the stuff getting rid of the sort commit :-)
No special case needed, he will send an updated series in a hour.
Thanks a lot,
Francesco