Re: [PATCH 36/41] drivers: tty: serial: 8250: store mmio resource size in port struct

Enrico Weigelt, metux IT consult
2019-04-29T14:55:48Z

On 28.04.19 17:18, Andy Shevchenko wrote:
> On Sat, Apr 27, 2019 at 02:52:17PM +0200, Enrico Weigelt, metux IT consult wrote:
>> The io resource size is currently recomputed when it's needed but this
>> actually needs to be done once (or drivers could specify fixed values)
>
> io -> IO

fixed.

>> Simplify that by doing this computation only once and storing the result
>> into the mapsize field. serial8250_register_8250_port() is now called
>> only once on driver init, the previous call sites now just fetch the
>> value from the mapsize field.
>
> Do I understand correctly that this has no side effects?

I don't know of any. (except someting changes things like regshift after
the initialization phase ... :o)

>> @@ -979,6 +979,9 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
>> if (up->port.uartclk == 0)
>> return -EINVAL;
>>
>> + /* compute the mapsize in case the driver didn't specify one */
>> + up->mapsize = serial8250_port_size(up);
>
> I don't know all quirks in 8250 drivers by heart, though can you guarantee that
> at this point the device reports correct IO resource size? (If I'm not mistaken
> some broken hardware needs some magic to be done before card can be properly
> handled)

Actually, I don't see anything talking to the hardware at all here.
It's all derived from values that are set up before
serial8250_register_8250_port() is called.

>> - unsigned int size = serial8250_port_size(up);
>> struct uart_port *port = &up->port;
>
>> - int ret = 0;
>
> This and Co is a separate change that can be done in its own patch.

I don't really understand :(
Do you mean the splitting off the retval part from the rest ?

>> + port->membase = ioremap_nocache(port->mapbase,
>> + port->mapsize);
>
> You may increase readability by introducing temporary variables
>
> ... mapbase = port->mapbase;
> ... mapsize = port->mapsize;
> ...
> port->membase = ioremap_nocache(mapbase, mapsize);
> ...

Is that really necessary ? Maybe it's just my personal taste, but I
don't feel the more more verbose one is really easier to read.

--mtx

--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287