Hi, On Tue, Mar 16, 2021 at 06:48:58PM +0300, Ivan Uvarov wrote: > > There's also a bunch of checkpatch --strict issues you want to get rid > > of. > > Thanks for pointing this out. > Besides the style warnings, which I've fixed, there are also warnings pertaining to > documentation. Should I add the compatible strings to > Documentation/devicetree/bindings/, and/or myself to the MAINTAINERS file? You can ignore the one about the MAINTAINERS file, but you should address the rest > If so, would you suggest that I do so in a separate patch, or as part > of another patch that adds the relevant files? In general, bindings patches should be separate indeed > > On Thu, Mar 11, 2021 at 03:30:56PM +0300, Ivan Uvarov wrote: > > > Add support for the Forlinx FETA40i-C SoM and OKA40i-C devboard[1] > > > based on it. The DT is split into a .dtsi, which (hopefully) > > > corresponds to the functions of the SoM itself, and a .dts for the > > > devboard. > > > > > > [1]:https://linux-sunxi.org/Forlinx_OKA40i-C > > > > Instead of a URL that might be dead at some point, it would be better > > to have a proper commit log explaining why you did your patch that way > > I'm not sure I understand. > > By "that way" do you mean that I should explain why I've split up the > devicetree, or something else? > > The linked page is just a collection of information about the device; > should I describe the SoM and/or devboard in my commit log instead? Yep. Essentially the git commit should be freestanding: if we look back at this commit in 5 years, will we able to tell without any context what this is about. Just pointing to an URL is risky, since the link might change or the website gone, and then you have no idea what it's about anymore. > > > @@ -601,6 +603,14 @@ mmc2_pins: mmc2-pins { > > > bias-pull-up; > > > }; > > > > > > + mmc3_pins: mmc3-pins { > > > + pins = "PI4", "PI5", "PI6", > > > + "PI7", "PI8", "PI9"; > > > + function = "mmc3"; > > > + drive-strength = <30>; > > > + bias-pull-up; > > > + }; > > > + > > > > This should be in a separate patch > > > > > /omit-if-no-ref/ > > > spi0_pc_pins: spi0-pc-pins { > > > pins = "PC0", "PC1", "PC2"; > > > @@ -636,6 +646,16 @@ uart0_pb_pins: uart0-pb-pins { > > > function = "uart0"; > > > }; > > > > > > + uart2_pi_pins: uart2-pi-pins { > > > + pins = "PI18", "PI19"; > > > + function = "uart2"; > > > + }; > > > + > > > + uart2_rts_cts_pi_pins: > > > uart2-rts-cts-pi-pins{ > > > + pins = "PI16", "PI17"; > > > + function = "uart2"; > > > + }; > > > + > > > > Ditto, and it should have /omit-if-no-ref/ > > Should the MMC pins be in the same patch as the UART pins? Yep > Also, would it be a good idea to add /omit-if-no-ref/ to the uart3 > pins as well while I'm here? To all the nodes honestly. If you want to make a preliminary patch doing this, go ahead :) > > > uart3_pg_pins: uart3-pg-pins { > > > pins = "PG6", "PG7"; > > > function = "uart3"; > > > @@ -645,6 +665,21 @@ uart3_rts_cts_pg_pins: uart3-rts-cts-pg-pins { > > > pins = "PG8", "PG9"; > > > function = "uart3"; > > > }; > > > + > > > + uart4_pg_pins: uart4-pg-pins { > > > + pins = "PG10", "PG11"; > > > + function = "uart4"; > > > + }; > > > + > > > + uart5_ph_pins: uart5-ph-pins { > > > + pins = "PH6", "PH7"; > > > + function = "uart5"; > > > + }; > > > + > > > + uart7_pi_pins: uart7-pi-pins { > > > + pins = "PI20", "PI21"; > > > + function = "uart7"; > > > + }; > > > > Ditto > > Since this won't be a monolithic patch anymore, would it make more > sense to make this a three+-patch series: > > 1. add board+SOM DT without mmc3 or extra uarts; > 2+. add pins to r40.dtsi; > 3. add mmc3&uarts to the devboard DT, > > or to make only two(+) patches: > > 1+. add pins to r40; > 2. add board+SOM DT? The latter would be more natural indeed Maxime
Attachment:
signature.asc
Description: PGP signature
