mbox series

[00/14] mips: dts: ralink: mt7621: improve DTS style

Message ID 20240316045442.31469-1-justin.swartz@risingedge.co.za (mailing list archive)
Headers show
Series mips: dts: ralink: mt7621: improve DTS style | expand

Message

Justin Swartz March 16, 2024, 4:54 a.m. UTC
This set of patches was created with the intention of cleaning up
arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
the Devicetree Sources (DTS) Coding Style [1] [2] guide.

[1] Documentation/devicetree/bindings/dts-coding-style.rst

[2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html 

Justin Swartz (14):
  mips: dts: ralink: mt7621: reorder cpu node attributes
  mips: dts: ralink: mt7621: reorder cpuintc node attributes
  mips: dts: ralink: mt7621: reorder mmc regulator attributes
  mips: dts: ralink: mt7621: reorder sysc node attributes
  mips: dts: ralink: mt7621: reorder gpio node attributes
  mips: dts: ralink: mt7621: reorder i2c node attributes
  mips: dts: ralink: mt7621: reorder spi0 node attributes
  mips: dts: ralink: mt7621: move pinctrl and sort its children
  mips: dts: ralink: mt7621: reorder mmc node attributes
  mips: dts: ralink: mt7621: reorder gic node attributes
  mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
  mips: dts: ralink: mt7621: reorder pcie node attributes and children
  mips: dts: ralink: mt7621: reorder pci?_phy attributes
  mips: dts: ralink: mt7621: reorder the attributes of the root node

 arch/mips/boot/dts/ralink/mt7621.dtsi | 430 ++++++++++++++------------
 1 file changed, 239 insertions(+), 191 deletions(-)

Comments

Justin Swartz March 16, 2024, 2:03 p.m. UTC | #1
On 2024-03-16 11:24, Arınç ÜNAL wrote:
> On 16.03.2024 07:54, Justin Swartz wrote:
>> This set of patches was created with the intention of cleaning up
>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>> 
>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>> 
>> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>> 
>> Justin Swartz (14):
>>    mips: dts: ralink: mt7621: reorder cpu node attributes
>>    mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>    mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>    mips: dts: ralink: mt7621: reorder sysc node attributes
>>    mips: dts: ralink: mt7621: reorder gpio node attributes
>>    mips: dts: ralink: mt7621: reorder i2c node attributes
>>    mips: dts: ralink: mt7621: reorder spi0 node attributes
>>    mips: dts: ralink: mt7621: move pinctrl and sort its children
>>    mips: dts: ralink: mt7621: reorder mmc node attributes
>>    mips: dts: ralink: mt7621: reorder gic node attributes
>>    mips: dts: ralink: mt7621: reorder ethernet node attributes and 
>> kids
>>    mips: dts: ralink: mt7621: reorder pcie node attributes and 
>> children
>>    mips: dts: ralink: mt7621: reorder pci?_phy attributes
>>    mips: dts: ralink: mt7621: reorder the attributes of the root node
>> 
>>   arch/mips/boot/dts/ralink/mt7621.dtsi | 430 
>> ++++++++++++++------------
>>   1 file changed, 239 insertions(+), 191 deletions(-)
> 
> A well done patch series. Thank you very much for doing this!
> 
> Arınç

++ for reviewing them all.

As you have at least one board that features an MT7621 SoC,
please may you (and anyone else willing) take a look at a
patch [1] that I've submitted for spi-mt7621.c which allows
GPIO chip select lines to be used as well as the native chip
selects.

There's already been some back and forth with Mark Brown about
the initial version of the patch (the linked patch is v2) and,
of course I messed up how I send v2, as you'll read in the thread.

It seems you weren't included because I rely on [2] to determine
which people to address as maintainers when sending patches.

Is there a better approach for populating git send's --to argument?

Regards
Justin


[1] 
https://lore.kernel.org/linux-mediatek/20240316010302.20776-1-justin.swartz@risingedge.co.za/

[2] scripts/get_maintainer.pl --nogit --nogit-fallback --norolestats 
--nol
Sergio Paracuellos March 16, 2024, 3:49 p.m. UTC | #2
On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> This set of patches was created with the intention of cleaning up
> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>
> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>
> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>
> Justin Swartz (14):
>   mips: dts: ralink: mt7621: reorder cpu node attributes
>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>   mips: dts: ralink: mt7621: reorder sysc node attributes
>   mips: dts: ralink: mt7621: reorder gpio node attributes
>   mips: dts: ralink: mt7621: reorder i2c node attributes
>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>   mips: dts: ralink: mt7621: reorder mmc node attributes
>   mips: dts: ralink: mt7621: reorder gic node attributes
>   mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
>   mips: dts: ralink: mt7621: reorder pcie node attributes and children
>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
>   mips: dts: ralink: mt7621: reorder the attributes of the root node
>
>  arch/mips/boot/dts/ralink/mt7621.dtsi | 430 ++++++++++++++------------
>  1 file changed, 239 insertions(+), 191 deletions(-)

For the whole series:
     Reviewed-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>

Thanks for doing this.

Best regards,
    Sergio Paracuellos
Krzysztof Kozlowski March 17, 2024, 3:10 p.m. UTC | #3
On 16/03/2024 16:49, Sergio Paracuellos wrote:
> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
> <justin.swartz@risingedge.co.za> wrote:
>>
>> This set of patches was created with the intention of cleaning up
>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>>
>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>>
>> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>>
>> Justin Swartz (14):
>>   mips: dts: ralink: mt7621: reorder cpu node attributes
>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>   mips: dts: ralink: mt7621: reorder sysc node attributes
>>   mips: dts: ralink: mt7621: reorder gpio node attributes
>>   mips: dts: ralink: mt7621: reorder i2c node attributes
>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>>   mips: dts: ralink: mt7621: reorder mmc node attributes
>>   mips: dts: ralink: mt7621: reorder gic node attributes
>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
>>   mips: dts: ralink: mt7621: reorder pcie node attributes and children
>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes

These are all simple cleanups for the same file. It's one patch, not 15.

Best regards,
Krzysztof
Justin Swartz March 17, 2024, 3:22 p.m. UTC | #4
On 2024-03-17 17:10, Krzysztof Kozlowski wrote:
> On 16/03/2024 16:49, Sergio Paracuellos wrote:
>> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
>> <justin.swartz@risingedge.co.za> wrote:
>>> 
>>> This set of patches was created with the intention of cleaning up
>>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>>> 
>>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>>> 
>>> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>>> 
>>> Justin Swartz (14):
>>>   mips: dts: ralink: mt7621: reorder cpu node attributes
>>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>>   mips: dts: ralink: mt7621: reorder sysc node attributes
>>>   mips: dts: ralink: mt7621: reorder gpio node attributes
>>>   mips: dts: ralink: mt7621: reorder i2c node attributes
>>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>>>   mips: dts: ralink: mt7621: reorder mmc node attributes
>>>   mips: dts: ralink: mt7621: reorder gic node attributes
>>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and 
>>> kids
>>>   mips: dts: ralink: mt7621: reorder pcie node attributes and 
>>> children
>>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
> 
> These are all simple cleanups for the same file. It's one patch, not 
> 15.

I agree these are all simple cleanups.

Even though the cleanup pattern was the same, or very similar,
for each node affected, the intention was to isolate each change
to a single node (or a grouping of nodes of that seemed logical
to me) so that if anyone had any objections, the discussion would
be easier to follow in subthreads identifiable by patch names (and
thus subject lines) that clearly indicate the context.

But if there're no objections and it lessens the burden on
maintainers upstream to have less patches to apply, then I have no
problem combining them into a single patch.

Regards
Justin
Krzysztof Kozlowski March 17, 2024, 3:29 p.m. UTC | #5
On 17/03/2024 16:22, Justin Swartz wrote:
> On 2024-03-17 17:10, Krzysztof Kozlowski wrote:
>> On 16/03/2024 16:49, Sergio Paracuellos wrote:
>>> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
>>> <justin.swartz@risingedge.co.za> wrote:
>>>>
>>>> This set of patches was created with the intention of cleaning up
>>>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>>>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>>>>
>>>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>>>>
>>>> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>>>>
>>>> Justin Swartz (14):
>>>>   mips: dts: ralink: mt7621: reorder cpu node attributes
>>>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>>>   mips: dts: ralink: mt7621: reorder sysc node attributes
>>>>   mips: dts: ralink: mt7621: reorder gpio node attributes
>>>>   mips: dts: ralink: mt7621: reorder i2c node attributes
>>>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>>>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>>>>   mips: dts: ralink: mt7621: reorder mmc node attributes
>>>>   mips: dts: ralink: mt7621: reorder gic node attributes
>>>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and 
>>>> kids
>>>>   mips: dts: ralink: mt7621: reorder pcie node attributes and 
>>>> children
>>>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
>>
>> These are all simple cleanups for the same file. It's one patch, not 
>> 15.
> 
> I agree these are all simple cleanups.
> 
> Even though the cleanup pattern was the same, or very similar,
> for each node affected, the intention was to isolate each change
> to a single node (or a grouping of nodes of that seemed logical
> to me) so that if anyone had any objections, the discussion would
> be easier to follow in subthreads identifiable by patch names (and

Objections to what? Coding style? Coding style is defined so you either
implement it or not... and even if someone disagrees with one line swap,
why it cannot be done like for every contribution: inline?

Organize your patches how described in submitting patches: one per
logical change. Logical change is to reorder all properties in one file,
without functional impact.

> thus subject lines) that clearly indicate the context.
> 
> But if there're no objections and it lessens the burden on
> maintainers upstream to have less patches to apply, then I have no
> problem combining them into a single patch.
> 

Yeah, one review response instead of 14 responses... One commit in the
history instead of 14.

Best regards,
Krzysztof
Justin Swartz March 17, 2024, 3:43 p.m. UTC | #6
On 2024-03-17 17:29, Krzysztof Kozlowski wrote:
> On 17/03/2024 16:22, Justin Swartz wrote:
>> On 2024-03-17 17:10, Krzysztof Kozlowski wrote:
>>> On 16/03/2024 16:49, Sergio Paracuellos wrote:
>>>> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
>>>> <justin.swartz@risingedge.co.za> wrote:
>>>>> 
>>>>> This set of patches was created with the intention of cleaning up
>>>>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>>>>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>>>>> 
>>>>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>>>>> 
>>>>> [2] 
>>>>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>>>>> 
>>>>> Justin Swartz (14):
>>>>>   mips: dts: ralink: mt7621: reorder cpu node attributes
>>>>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>>>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>>>>   mips: dts: ralink: mt7621: reorder sysc node attributes
>>>>>   mips: dts: ralink: mt7621: reorder gpio node attributes
>>>>>   mips: dts: ralink: mt7621: reorder i2c node attributes
>>>>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>>>>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>>>>>   mips: dts: ralink: mt7621: reorder mmc node attributes
>>>>>   mips: dts: ralink: mt7621: reorder gic node attributes
>>>>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and
>>>>> kids
>>>>>   mips: dts: ralink: mt7621: reorder pcie node attributes and
>>>>> children
>>>>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
>>> 
>>> These are all simple cleanups for the same file. It's one patch, not
>>> 15.
>> 
>> I agree these are all simple cleanups.
>> 
>> Even though the cleanup pattern was the same, or very similar,
>> for each node affected, the intention was to isolate each change
>> to a single node (or a grouping of nodes of that seemed logical
>> to me) so that if anyone had any objections, the discussion would
>> be easier to follow in subthreads identifiable by patch names (and
> 
> Objections to what? Coding style? Coding style is defined so you either
> implement it or not... and even if someone disagrees with one line 
> swap,
> why it cannot be done like for every contribution: inline?

I had been asked to include empty lines when I had left them out when
I had contributed a patch regarding the serial nodes, which resulted in
a second version of that patch.


> Organize your patches how described in submitting patches: one per
> logical change. Logical change is to reorder all properties in one 
> file,
> without functional impact.

If I had accidentally deleted or modified an attribute in the process
of cleanup, this could have had a functional impact. It's easier to
notice this sort of omission when the wall of text you're confronted
with is as small as possible, and not multiple pages long.


>> But if there're no objections and it lessens the burden on
>> maintainers upstream to have less patches to apply, then I have no
>> problem combining them into a single patch.
>> 
> 
> Yeah, one review response instead of 14 responses... One commit in the
> history instead of 14.

I agree that 1 commit vs 14 is better.

But for future reference: is it not enough for the Reviewed-by: trailer
to be sent in response to the cover letter of a patch set if a reviewer
has looked at the entire set?

Regards
Justin
Sergio Paracuellos March 18, 2024, 8:48 a.m. UTC | #7
On Sun, Mar 17, 2024 at 4:43 PM Justin Swartz
<justin.swartz@risingedge.co.za> wrote:
>
> On 2024-03-17 17:29, Krzysztof Kozlowski wrote:
> > On 17/03/2024 16:22, Justin Swartz wrote:
> >> On 2024-03-17 17:10, Krzysztof Kozlowski wrote:
> >>> On 16/03/2024 16:49, Sergio Paracuellos wrote:
> >>>> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
> >>>> <justin.swartz@risingedge.co.za> wrote:
> >>>>>
> >>>>> This set of patches was created with the intention of cleaning up
> >>>>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
> >>>>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
> >>>>>
> >>>>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
> >>>>>
> >>>>> [2]
> >>>>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> >>>>>
> >>>>> Justin Swartz (14):
> >>>>>   mips: dts: ralink: mt7621: reorder cpu node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
> >>>>>   mips: dts: ralink: mt7621: reorder sysc node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder gpio node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder i2c node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
> >>>>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
> >>>>>   mips: dts: ralink: mt7621: reorder mmc node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder gic node attributes
> >>>>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and
> >>>>> kids
> >>>>>   mips: dts: ralink: mt7621: reorder pcie node attributes and
> >>>>> children
> >>>>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
> >>>
> >>> These are all simple cleanups for the same file. It's one patch, not
> >>> 15.
> >>
> >> I agree these are all simple cleanups.
> >>
> >> Even though the cleanup pattern was the same, or very similar,
> >> for each node affected, the intention was to isolate each change
> >> to a single node (or a grouping of nodes of that seemed logical
> >> to me) so that if anyone had any objections, the discussion would
> >> be easier to follow in subthreads identifiable by patch names (and
> >
> > Objections to what? Coding style? Coding style is defined so you either
> > implement it or not... and even if someone disagrees with one line
> > swap,
> > why it cannot be done like for every contribution: inline?
>
> I had been asked to include empty lines when I had left them out when
> I had contributed a patch regarding the serial nodes, which resulted in
> a second version of that patch.
>
>
> > Organize your patches how described in submitting patches: one per
> > logical change. Logical change is to reorder all properties in one
> > file,
> > without functional impact.
>
> If I had accidentally deleted or modified an attribute in the process
> of cleanup, this could have had a functional impact. It's easier to
> notice this sort of omission when the wall of text you're confronted
> with is as small as possible, and not multiple pages long.
>
>
> >> But if there're no objections and it lessens the burden on
> >> maintainers upstream to have less patches to apply, then I have no
> >> problem combining them into a single patch.
> >>
> >
> > Yeah, one review response instead of 14 responses... One commit in the
> > history instead of 14.
>
> I agree that 1 commit vs 14 is better.
>
> But for future reference: is it not enough for the Reviewed-by: trailer
> to be sent in response to the cover letter of a patch set if a reviewer
> has looked at the entire set?

It is enough, AFAICT. I found your patchset very easy to review so I
am ok with the patchset as it is. However, at the end this will be
through the mips tree, so let's do what Thomas prefers: add all
patches as they are or squash all of them in one commit.

Thanks,
    Sergio Paracuellos
Krzysztof Kozlowski March 18, 2024, 9:19 a.m. UTC | #8
On 18/03/2024 09:48, Sergio Paracuellos wrote:
>>
>>
>>>> But if there're no objections and it lessens the burden on
>>>> maintainers upstream to have less patches to apply, then I have no
>>>> problem combining them into a single patch.
>>>>
>>>
>>> Yeah, one review response instead of 14 responses... One commit in the
>>> history instead of 14.
>>
>> I agree that 1 commit vs 14 is better.
>>
>> But for future reference: is it not enough for the Reviewed-by: trailer
>> to be sent in response to the cover letter of a patch set if a reviewer
>> has looked at the entire set?
> 
> It is enough, AFAICT. I found your patchset very easy to review so I
> am ok with the patchset as it is. However, at the end this will be
> through the mips tree, so let's do what Thomas prefers: add all
> patches as they are or squash all of them in one commit.

Yeah, like fixing language typos in comments: one typo per patch.

Best regards,
Krzysztof
Krzysztof Kozlowski March 18, 2024, 9:23 a.m. UTC | #9
On 17/03/2024 16:43, Justin Swartz wrote:
> On 2024-03-17 17:29, Krzysztof Kozlowski wrote:
>> On 17/03/2024 16:22, Justin Swartz wrote:
>>> On 2024-03-17 17:10, Krzysztof Kozlowski wrote:
>>>> On 16/03/2024 16:49, Sergio Paracuellos wrote:
>>>>> On Sat, Mar 16, 2024 at 5:54 AM Justin Swartz
>>>>> <justin.swartz@risingedge.co.za> wrote:
>>>>>>
>>>>>> This set of patches was created with the intention of cleaning up
>>>>>> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
>>>>>> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
>>>>>>
>>>>>> [1] Documentation/devicetree/bindings/dts-coding-style.rst
>>>>>>
>>>>>> [2] 
>>>>>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
>>>>>>
>>>>>> Justin Swartz (14):
>>>>>>   mips: dts: ralink: mt7621: reorder cpu node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>>>>>>   mips: dts: ralink: mt7621: reorder sysc node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder gpio node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder i2c node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>>>>>>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>>>>>>   mips: dts: ralink: mt7621: reorder mmc node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder gic node attributes
>>>>>>   mips: dts: ralink: mt7621: reorder ethernet node attributes and
>>>>>> kids
>>>>>>   mips: dts: ralink: mt7621: reorder pcie node attributes and
>>>>>> children
>>>>>>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
>>>>
>>>> These are all simple cleanups for the same file. It's one patch, not
>>>> 15.
>>>
>>> I agree these are all simple cleanups.
>>>
>>> Even though the cleanup pattern was the same, or very similar,
>>> for each node affected, the intention was to isolate each change
>>> to a single node (or a grouping of nodes of that seemed logical
>>> to me) so that if anyone had any objections, the discussion would
>>> be easier to follow in subthreads identifiable by patch names (and
>>
>> Objections to what? Coding style? Coding style is defined so you either
>> implement it or not... and even if someone disagrees with one line 
>> swap,
>> why it cannot be done like for every contribution: inline?
> 
> I had been asked to include empty lines when I had left them out when
> I had contributed a patch regarding the serial nodes, which resulted in
> a second version of that patch.

I don't understand why would that matter. It's expected Linux
development process to receive comments inline in the patch.

> 
> 
>> Organize your patches how described in submitting patches: one per
>> logical change. Logical change is to reorder all properties in one 
>> file,
>> without functional impact.
> 
> If I had accidentally deleted or modified an attribute in the process
> of cleanup, this could have had a functional impact. It's easier to

How is it relevant? But you did not and splitting simple cleanup
one-line-per-patch is not affecting this. Just because you could make
mistake it does not affect patch readability at all.

Nothing improved with your patch split.


> notice this sort of omission when the wall of text you're confronted
> with is as small as possible, and not multiple pages long.

We are used to handle some length of patches. Multiple scrolls for
obvious cleanups are not problems. Why aren't you applying this approach
to everything? Add a new driver with one function per patch and then
finally Makefile? It would be bisectable and "easy to read" plus
absolutely unmanageable.

> 
> 
>>> But if there're no objections and it lessens the burden on
>>> maintainers upstream to have less patches to apply, then I have no
>>> problem combining them into a single patch.
>>>
>>
>> Yeah, one review response instead of 14 responses... One commit in the
>> history instead of 14.
> 
> I agree that 1 commit vs 14 is better.
> 
> But for future reference: is it not enough for the Reviewed-by: trailer
> to be sent in response to the cover letter of a patch set if a reviewer
> has looked at the entire set?

Sure, one can. I still need to open and download 14 patches.


Best regards,
Krzysztof
Justin Swartz March 18, 2024, 10:57 a.m. UTC | #10
On 2024-03-18 10:48, Sergio Paracuellos wrote:
> On Sun, Mar 17, 2024 at 4:43 PM Justin Swartz
> <justin.swartz@risingedge.co.za> wrote:
>> But for future reference: is it not enough for the Reviewed-by: 
>> trailer
>> to be sent in response to the cover letter of a patch set if a 
>> reviewer
>> has looked at the entire set?
> 
> It is enough, AFAICT. I found your patchset very easy to review so I
> am ok with the patchset as it is. However, at the end this will be
> through the mips tree, so let's do what Thomas prefers: add all
> patches as they are or squash all of them in one commit.
> 
> Thanks,
>     Sergio Paracuellos

Thank you for the review. I'm glad to squash them if need be.
Justin Swartz March 18, 2024, 11:06 a.m. UTC | #11
On 2024-03-18 11:23, Krzysztof Kozlowski wrote:
> On 17/03/2024 16:43, Justin Swartz wrote:
>> On 2024-03-17 17:29, Krzysztof Kozlowski wrote:
>>> Objections to what? Coding style? Coding style is defined so you 
>>> either
>>> implement it or not... and even if someone disagrees with one line
>>> swap,
>>> why it cannot be done like for every contribution: inline?
>> 
>> I had been asked to include empty lines when I had left them out when
>> I had contributed a patch regarding the serial nodes, which resulted 
>> in
>> a second version of that patch.
> 
> I don't understand why would that matter. It's expected Linux
> development process to receive comments inline in the patch.


>>> Organize your patches how described in submitting patches: one per
>>> logical change. Logical change is to reorder all properties in one
>>> file,
>>> without functional impact.
>> 
>> If I had accidentally deleted or modified an attribute in the process
>> of cleanup, this could have had a functional impact. It's easier to
> 
> How is it relevant? But you did not and splitting simple cleanup
> one-line-per-patch is not affecting this. Just because you could make
> mistake it does not affect patch readability at all.
> 
> Nothing improved with your patch split.


>> notice this sort of omission when the wall of text you're confronted
>> with is as small as possible, and not multiple pages long.
> 
> We are used to handle some length of patches. Multiple scrolls for
> obvious cleanups are not problems. Why aren't you applying this 
> approach
> to everything? Add a new driver with one function per patch and then
> finally Makefile? It would be bisectable and "easy to read" plus
> absolutely unmanageable.


>> But for future reference: is it not enough for the Reviewed-by: 
>> trailer
>> to be sent in response to the cover letter of a patch set if a 
>> reviewer
>> has looked at the entire set?
> 
> Sure, one can. I still need to open and download 14 patches.

Thanks for your input.

I can imagine how these sets of very minor changes might greatly reduce
your signal-to-noise ratio as an upstream maintainer.

I'll try your suggested approach next time.
Arınç ÜNAL April 8, 2024, 7:35 a.m. UTC | #12
On 16.03.2024 07:54, Justin Swartz wrote:
> This set of patches was created with the intention of cleaning up
> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
> 
> [1] Documentation/devicetree/bindings/dts-coding-style.rst
> 
> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> 
> Justin Swartz (14):
>    mips: dts: ralink: mt7621: reorder cpu node attributes
>    mips: dts: ralink: mt7621: reorder cpuintc node attributes
>    mips: dts: ralink: mt7621: reorder mmc regulator attributes
>    mips: dts: ralink: mt7621: reorder sysc node attributes
>    mips: dts: ralink: mt7621: reorder gpio node attributes
>    mips: dts: ralink: mt7621: reorder i2c node attributes
>    mips: dts: ralink: mt7621: reorder spi0 node attributes
>    mips: dts: ralink: mt7621: move pinctrl and sort its children
>    mips: dts: ralink: mt7621: reorder mmc node attributes
>    mips: dts: ralink: mt7621: reorder gic node attributes
>    mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
>    mips: dts: ralink: mt7621: reorder pcie node attributes and children
>    mips: dts: ralink: mt7621: reorder pci?_phy attributes
>    mips: dts: ralink: mt7621: reorder the attributes of the root node
> 
>   arch/mips/boot/dts/ralink/mt7621.dtsi | 430 ++++++++++++++------------
>   1 file changed, 239 insertions(+), 191 deletions(-)

Thomas, will you apply this patch series as is or should we squash it to
one patch?

Arınç
Thomas Bogendoerfer April 15, 2024, 8:32 a.m. UTC | #13
On Mon, Apr 08, 2024 at 10:35:07AM +0300, Arınç ÜNAL wrote:
> On 16.03.2024 07:54, Justin Swartz wrote:
> > This set of patches was created with the intention of cleaning up
> > arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
> > the Devicetree Sources (DTS) Coding Style [1] [2] guide.
> > 
> > [1] Documentation/devicetree/bindings/dts-coding-style.rst
> > 
> > [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> > 
> > Justin Swartz (14):
> >    mips: dts: ralink: mt7621: reorder cpu node attributes
> >    mips: dts: ralink: mt7621: reorder cpuintc node attributes
> >    mips: dts: ralink: mt7621: reorder mmc regulator attributes
> >    mips: dts: ralink: mt7621: reorder sysc node attributes
> >    mips: dts: ralink: mt7621: reorder gpio node attributes
> >    mips: dts: ralink: mt7621: reorder i2c node attributes
> >    mips: dts: ralink: mt7621: reorder spi0 node attributes
> >    mips: dts: ralink: mt7621: move pinctrl and sort its children
> >    mips: dts: ralink: mt7621: reorder mmc node attributes
> >    mips: dts: ralink: mt7621: reorder gic node attributes
> >    mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
> >    mips: dts: ralink: mt7621: reorder pcie node attributes and children
> >    mips: dts: ralink: mt7621: reorder pci?_phy attributes
> >    mips: dts: ralink: mt7621: reorder the attributes of the root node
> > 
> >   arch/mips/boot/dts/ralink/mt7621.dtsi | 430 ++++++++++++++------------
> >   1 file changed, 239 insertions(+), 191 deletions(-)
> 
> Thomas, will you apply this patch series as is or should we squash it to
> one patch?

I've applied them, no need to squash.

Thomas.
Thomas Bogendoerfer April 15, 2024, 8:33 a.m. UTC | #14
On Sat, Mar 16, 2024 at 06:54:28AM +0200, Justin Swartz wrote:
> This set of patches was created with the intention of cleaning up
> arch/mips/boot/dts/ralink/mt7621.dtsi so that it is aligned with
> the Devicetree Sources (DTS) Coding Style [1] [2] guide.
> 
> [1] Documentation/devicetree/bindings/dts-coding-style.rst
> 
> [2] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html 
> 
> Justin Swartz (14):
>   mips: dts: ralink: mt7621: reorder cpu node attributes
>   mips: dts: ralink: mt7621: reorder cpuintc node attributes
>   mips: dts: ralink: mt7621: reorder mmc regulator attributes
>   mips: dts: ralink: mt7621: reorder sysc node attributes
>   mips: dts: ralink: mt7621: reorder gpio node attributes
>   mips: dts: ralink: mt7621: reorder i2c node attributes
>   mips: dts: ralink: mt7621: reorder spi0 node attributes
>   mips: dts: ralink: mt7621: move pinctrl and sort its children
>   mips: dts: ralink: mt7621: reorder mmc node attributes
>   mips: dts: ralink: mt7621: reorder gic node attributes
>   mips: dts: ralink: mt7621: reorder ethernet node attributes and kids
>   mips: dts: ralink: mt7621: reorder pcie node attributes and children
>   mips: dts: ralink: mt7621: reorder pci?_phy attributes
>   mips: dts: ralink: mt7621: reorder the attributes of the root node
> 
>  arch/mips/boot/dts/ralink/mt7621.dtsi | 430 ++++++++++++++------------
>  1 file changed, 239 insertions(+), 191 deletions(-)

series applied to mips-next.

Thomas.