Message-ID: <18327bbe-10ad-4b39-ab70-2f74f0d4fb08@linaro.org>
Date: Mon, 18 Mar 2024 10:23:22 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Justin Swartz <justin.swartz@...ingedge.co.za>
Cc: Sergio Paracuellos <sergio.paracuellos@...il.com>,
 Arınç ÜNAL <arinc.unal@...nc9.com>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>,
 Thomas Bogendoerfer <tsbogend@...ha.franken.de>,
 Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
 linux-mips@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH 00/14] mips: dts: ralink: mt7621: improve DTS style

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@...ingedge.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

Powered by blists - more mailing lists