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

[Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>]

Hi Miquel,
 
 On ven., oct. 13 2017, Miquel RAYNAL <miquel.raynal@xxxxxxxxxxxxxxxxxx> wrote:

> Hi Gregory,
>
> On Fri, 13 Oct 2017 11:40:32 +0200
> Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx> wrote:
>
>> Hi Miquel,
>>  
>>  On ven., oct. 13 2017, Miquel Raynal
>> <miquel.raynal@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>

Thanks,

Gregory

>
> Thanks,
> Miquèl
>
>> 
>> Once it will be fixed, you can add:
>> 
>> Acked-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
>> 
>> 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

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html