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

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hi Thomas, Gregory,

On Fri, 6 Oct 2017 15:15:21 +0200
Thomas Petazzoni <thomas.petazzoni@free-electrons.com> 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.
 */

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