Re: [PATCH v1] ARM: dts: sun8i: r40: add dts for Forlinx FETA40i-C & OKA40i-C

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 4087 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]