| Message ID | 1556369542-13247-37-git-send-email-info@metux.net (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [01/41] drivers: tty: serial: dz: use dev_err() instead of printk() | expand |
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 > > 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? Which hardware did you test this on? > @@ -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) > - 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. > + 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); ...
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
On Mon, Apr 29, 2019 at 04:55:05PM +0200, Enrico Weigelt, metux IT consult wrote: > 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: > >> - 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 ? You do two things here: one of them is removing ret and other relative changes. This should be split to a separate patch. > > 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. Up to Greg. For me it's harder to read all those port-> in several parameters.
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h index ebfb0bd..89e3f09 100644 --- a/drivers/tty/serial/8250/8250.h +++ b/drivers/tty/serial/8250/8250.h @@ -255,3 +255,5 @@ static inline int serial_index(struct uart_port *port) { return port->minor - 64; } + +unsigned int serial8250_port_size(struct uart_8250_port *pt); diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c index 71a398b..a9d4ba1 100644 --- a/drivers/tty/serial/8250/8250_core.c +++ b/drivers/tty/serial/8250/8250_core.c @@ -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); + mutex_lock(&serial_mutex); uart = serial8250_find_match_or_unused(&up->port); diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index d2f3310..d09af4c 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -2829,7 +2829,7 @@ void serial8250_do_pm(struct uart_port *port, unsigned int state, serial8250_do_pm(port, state, oldstate); } -static unsigned int serial8250_port_size(struct uart_8250_port *pt) +unsigned int serial8250_port_size(struct uart_8250_port *pt) { if (pt->port.mapsize) return pt->port.mapsize; @@ -2849,9 +2849,7 @@ static unsigned int serial8250_port_size(struct uart_8250_port *pt) */ static int serial8250_request_std_resource(struct uart_8250_port *up) { - unsigned int size = serial8250_port_size(up); struct uart_port *port = &up->port; - int ret = 0; switch (port->iotype) { case UPIO_AU: @@ -2863,32 +2861,31 @@ static int serial8250_request_std_resource(struct uart_8250_port *up) if (!port->mapbase) break; - if (!request_mem_region(port->mapbase, size, "serial")) { - ret = -EBUSY; - break; - } + if (!request_mem_region(port->mapbase, + port->mapsize, "serial")) + return -EBUSY; if (port->flags & UPF_IOREMAP) { - port->membase = ioremap_nocache(port->mapbase, size); - if (!port->membase) { - release_mem_region(port->mapbase, size); - ret = -ENOMEM; - } + port->membase = ioremap_nocache(port->mapbase, + port->mapsize); + if (!port->membase) + release_mem_region(port->mapbase, + port->mapsize); + return -ENOMEM; } break; case UPIO_HUB6: case UPIO_PORT: - if (!request_region(port->iobase, size, "serial")) - ret = -EBUSY; + if (!request_region(port->iobase, port->mapsize, "serial")) + return -EBUSY; break; } - return ret; + return 0; } static void serial8250_release_std_resource(struct uart_8250_port *up) { - unsigned int size = serial8250_port_size(up); struct uart_port *port = &up->port; switch (port->iotype) { @@ -2906,12 +2903,12 @@ static void serial8250_release_std_resource(struct uart_8250_port *up) port->membase = NULL; } - release_mem_region(port->mapbase, size); + release_mem_region(port->mapbase, port->mapsize); break; case UPIO_HUB6: case UPIO_PORT: - release_region(port->iobase, size); + release_region(port->iobase, port->mapsize); break; } }
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) 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. Signed-off-by: Enrico Weigelt <info@metux.net> --- drivers/tty/serial/8250/8250.h | 2 ++ drivers/tty/serial/8250/8250_core.c | 3 +++ drivers/tty/serial/8250/8250_port.c | 33 +++++++++++++++------------------ 3 files changed, 20 insertions(+), 18 deletions(-)