[PATCH 04/16] serial: mvebu-uart: support probe of multiple ports

[Gregory CLEMENT]

Hi Miquel,
 
 On lun., oct. 09 2017, Miquel RAYNAL <miquel.raynal at free-electrons.com> wrote:

> On Fri, 06 Oct 2017 14:23:55 +0200
> Gregory CLEMENT <gregory.clement at free-electrons.com> wrote:
>
>> Hi Miquel,
>>  
>>  On ven., oct. 06 2017, Miquel Raynal
>> <miquel.raynal at free-electrons.com> wrote:
>> 
>> > From: Allen Yan <yanwei at marvell.com>
>> >
>> > Until now, the mvebu-uart driver only supported probing a single
>> > UART port. However, some platforms have multiple instances of this
>> > UART controller, and therefore the driver should support multiple
>> > ports.
>> >
>> > In order to achieve this, we make sure to assign port->line
>> > properly, instead of hardcoding it to zero.
>> >
>> > Signed-off-by: Allen Yan <yanwei at marvell.com>
>> > Signed-off-by: Miquel Raynal <miquel.raynal at free-electrons.com>
>> > ---
>> >  drivers/tty/serial/mvebu-uart.c | 13 +++++++++++--
>> >  1 file changed, 11 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/tty/serial/mvebu-uart.c
>> > b/drivers/tty/serial/mvebu-uart.c index 7e0a3e9fee15..25b11ede3a97
>> > 100644 --- a/drivers/tty/serial/mvebu-uart.c
>> > +++ b/drivers/tty/serial/mvebu-uart.c
>> > @@ -560,7 +560,16 @@ static int mvebu_uart_probe(struct
>> > platform_device *pdev) return -EINVAL;
>> >  	}
>> >  
>> > -	port = &mvebu_uart_ports[0];
>> > +	if (pdev->dev.of_node)
>> > +		pdev->id = of_alias_get_id(pdev->dev.of_node,
>> > "serial");  
>> 
>> If the id is retrieved using an of_ function, then I think that the
>> driver would depend on OF_CONFIG.
>
> Is this okay?
>
> if (pdev->dev.of_node && IS_ENABLED(CONFIG_OF))
>         pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");

Actually if CONFIG_OF is not enabled, then dev->dev.of_node. So you can
keep the test but just remove the test on IS_ENABLED(CONFIG_OF).

But my remark about CONFIG_OF, was more to point that if there is no
device tree support then this part of code won't work well if you have
several ports using the same driver.

So either you keep your driver as is but make it depends on CONFIG_OF to
make it clear that it won't work with ACPI. Or you add the case for
ACPI, but I think you don't have a board with ACPI support so you won't
be able to test it.

A third solution would be to have a fallback when of_alias_get_id failed
(either because there is no alias in the device tree or there is no
device tree at all). In this case you can just increment the id for each
new port.

Gregory


> else
>         pdev->id = 0;
>
> BTW, I could not test without CONFIG_OF as it is defined by default in
> our case: Selected by: ARM64 [=y]
> I don't think there will be a 32-bit SoC with this UART IP?
>
> Thanks,
> Miquèl
>
>> 
>> Gregory
>> 
>> 
>> > +
>> > +	if (pdev->id >= MVEBU_NR_UARTS) {
>> > +		dev_err(&pdev->dev, "cannot have more than %d UART
>> > ports\n",
>> > +			MVEBU_NR_UARTS);
>> > +		return -EINVAL;
>> > +	}
>> > +
>> > +	port = &mvebu_uart_ports[pdev->id];
>> >  
>> >  	spin_lock_init(&port->lock);
>> >  
>> > @@ -572,7 +581,7 @@ static int mvebu_uart_probe(struct
>> > platform_device *pdev) port->fifosize   = 32;
>> >  	port->iotype     = UPIO_MEM32;
>> >  	port->flags      = UPF_FIXED_PORT;
>> > -	port->line       = 0; /* single port: force line number
>> > to  0 */
>> > +	port->line       = pdev->id;
>> >  
>> >  	port->irq        = irq->start;
>> >  	port->irqflags   = 0;
>> > -- 
>> > 2.11.0
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel at lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel  
>> 
>
>
>
> -- 
> Miquel Raynal, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com