| Message ID | 20240316045442.31469-1-justin.swartz@risingedge.co.za (mailing list archive) |
|---|---|
| Headers | show |
| Series | mips: dts: ralink: mt7621: improve DTS style | expand |
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ç
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
On 16.03.2024 17:03, Justin Swartz wrote: > 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. I'd prefer not to be involved. It's not my area of expertise. > > Is there a better approach for populating git send's --to argument? This is how I used to submit patches before I started using b4 [1]. git format-patch HEAD~X --subject-prefix "PATCH (RFC) (tree) (vX)" --cover-letter ./scripts/get_maintainer.pl --norolestats --nol --nor 00* > to.txt ./scripts/get_maintainer.pl --norolestats --nom 00* > cc.txt git send-email --no-chain-reply-to \ --to-cmd="cat to.txt && cat > /dev/null" \ --cc-cmd="cat cc.txt && cat > /dev/null" \ 00* [1] https://b4.docs.kernel.org/en/latest/index.html Arınç
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
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
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
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
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
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
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
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
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.
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.
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ç
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.
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.