diff mbox

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

Message ID 20171013090200.31034-5-miquel.raynal@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Miquel Raynal Oct. 13, 2017, 9:01 a.m. UTC 
From: Allen Yan <yanwei@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@marvell.com>
Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---

Changes since v1: using multiple UART ports with this driver is not a
problem anymore if not using a device tree.

 drivers/tty/serial/mvebu-uart.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Gregory CLEMENT Oct. 13, 2017, 9:40 a.m. UTC | #1 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> -	port = &mvebu_uart_ports[0];
> +	/* Assume that all UART ports have a DT alias or none has */
> +	id = of_alias_get_id(pdev->dev.of_node, "serial");

You stil need to check the pdev->dev.of_node before calling
of_alias_get_id.

Once it will be fixed, you can add:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


> +	if (!pdev->dev.of_node || id < 0)
> +		pdev->id = uart_num_counter++;
> +	else
> +		pdev->id = id;
> +
> +	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 +588,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
>
Miquel Raynal Oct. 13, 2017, 11:17 a.m. UTC | #2 
Hi Gregory,

On Fri, 13 Oct 2017 11:40:32 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Miquel,
>  
>  On ven., oct. 13 2017, Miquel Raynal
> <miquel.raynal@free-electrons.com> wrote:
> 
> > -	port = &mvebu_uart_ports[0];
> > +	/* Assume that all UART ports have a DT alias or none has
> > */
> > +	id = of_alias_get_id(pdev->dev.of_node, "serial");  
> 
> You stil need to check the pdev->dev.of_node before calling
> of_alias_get_id.

Thanks for your feedback but I don't think we still need to check it,
because the of_alias_get_id() function will either return -ENOSYS if
CONFIG_OF is not defined, or -ENODEV if the node is not found. As the
function does not dereference pdev->dev.of_node at any moment but
instead compares the value with another pointer, I think this call is
safe like this.

Thanks,
Miquèl

> 
> Once it will be fixed, you can add:
> 
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> 
> Thanks,
> 
> Gregory
> 
> 
> > +	if (!pdev->dev.of_node || id < 0)
> > +		pdev->id = uart_num_counter++;
> > +	else
> > +		pdev->id = id;
> > +
> > +	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 +588,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
> >  
>
Gregory CLEMENT Oct. 13, 2017, 11:22 a.m. UTC | #3 
Hi Miquel,
 
 On ven., oct. 13 2017, Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hi Gregory,
>
> On Fri, 13 Oct 2017 11:40:32 +0200
> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>
>> Hi Miquel,
>>  
>>  On ven., oct. 13 2017, Miquel Raynal
>> <miquel.raynal@free-electrons.com> wrote:
>> 
>> > -	port = &mvebu_uart_ports[0];
>> > +	/* Assume that all UART ports have a DT alias or none has
>> > */
>> > +	id = of_alias_get_id(pdev->dev.of_node, "serial");  
>> 
>> You stil need to check the pdev->dev.of_node before calling
>> of_alias_get_id.
>
> Thanks for your feedback but I don't think we still need to check it,
> because the of_alias_get_id() function will either return -ENOSYS if
> CONFIG_OF is not defined, or -ENODEV if the node is not found. As the
> function does not dereference pdev->dev.of_node at any moment but
> instead compares the value with another pointer, I think this call is
> safe like this.

Fair enough, I expected that the pointer was dereferenced but I was
wrong, was so no need for a new version.

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

>
> Thanks,
> Miquèl
>
>> 
>> Once it will be fixed, you can add:
>> 
>> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> 
>> Thanks,
>> 
>> Gregory
>> 
>> 
>> > +	if (!pdev->dev.of_node || id < 0)
>> > +		pdev->id = uart_num_counter++;
>> > +	else
>> > +		pdev->id = id;
>> > +
>> > +	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 +588,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
>> >  
>> 
>
>
>
> -- 
> Miquel Raynal, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
diff mbox

Patch

diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index 7e0a3e9fee15..f3c7271db32b 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -547,20 +547,36 @@  static struct uart_driver mvebu_uart_driver = {
 #endif
 };
 
+/* Counter to keep track of each UART port id when not using CONFIG_OF */
+static int uart_num_counter;
+
 static int mvebu_uart_probe(struct platform_device *pdev)
 {
 	struct resource *reg = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	struct uart_port *port;
 	struct mvebu_uart_data *data;
-	int ret;
+	int ret, id;
 
 	if (!reg || !irq) {
 		dev_err(&pdev->dev, "no registers/irq defined\n");
 		return -EINVAL;
 	}
 
-	port = &mvebu_uart_ports[0];
+	/* Assume that all UART ports have a DT alias or none has */
+	id = of_alias_get_id(pdev->dev.of_node, "serial");
+	if (!pdev->dev.of_node || id < 0)
+		pdev->id = uart_num_counter++;
+	else
+		pdev->id = id;
+
+	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 +588,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;