Re: [PATCH 16/16] arm64: dts: marvell: armada-3720-espressobin: fill UART nodes

[Miquel RAYNAL <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

Hello Gregory,

On Thu, 12 Oct 2017 13:24:42 +0200
Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hi Miquel,
>  
>  On lun., oct. 09 2017, Miquel RAYNAL
> <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> > Hi Thomas, Gregory,
> >
> > On Fri, 6 Oct 2017 15:15:21 +0200
> > Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> >  
> >> Hello,
> >> 
> >> On Fri, 06 Oct 2017 15:01:18 +0200, Gregory CLEMENT wrote:
> >>   
> >> >  /*
> >> >   * To enable the second UART on J17 (pins 24,26) refer to the
> >> > uart1
> >> >   * node from armada-3720-db.dts.
> >> >   * Note that TX and RX signal are the ones coming directly from
> >> > the SoC:
> >> >   * 1.8V TTL.
> >> >   */    
> >> 
> >> One issue with this comment (and Miquèl's version as well) is that
> >> it does not explain why you don't enable this UART by default.
> >> 
> >> The real reason is in the commit log from Miquèl, and should
> >> probably be part of the comment. Perhaps something like:
> >> 
> >> /*
> >> 
> >>  * Connector J17 (pins X, Y, Z) exposes a number of different
> >>  * features:
> >>  *  - UART1 (pins 24 = RX, pins 26 = TX), see armada-3720-db.dts
> >> for an
> >>  *    example on how to enable UART1. Beware that the signals are
> >> 1.8V
> >>  *    TTL.
> >>  *  - SPIxyz
> >>  *  - I2Cxyz
> >>  */  
> >
> > Thanks for both your comments, there is my version, inspired from
> > both comments:
> >
> > /*
> >  * Connector J17 exposes a number of different features. Some pins
> > are
> >  * multiplexed. This is the case for the UART1 feature (pins 24 =
> > RX,
> >  * pins 26 = TX). See armada-3720-db.dts for an example of how to
> > enable it.
> >  * Beware that the signals are 1.8V TTL.
> >  */  
> 
> Seems good for me however I prefer Thomas version, easier to read and
> to extend latter with the description of other pins if needed.

Ok, I reused the 'dash' presentation to be later extended.

See v2 coming soon.

Thanks,
Miquèl

> 
> Gregory
> 
> >
> > Thanks,
> > Miquèl
> >  
> >> 
> >> Otherwise, it's not clear at all why you don't just enable UART1.
> >> Or perhaps I misunderstood Miquèl's commit log ?
> >> 
> >> Best regards,
> >> 
> >> Thomas  
> >
> >
> >
> > -- 
> > Miquel Raynal, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com  
> 



-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html