| Message ID | 1523302721-19439-5-git-send-email-kramasub@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Mon, Apr 09, 2018 at 01:38:37PM -0600, Karthikeyan Ramasubramanian wrote: > Perform static initialization of console_port instead of initializing > during module_init. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > --- > drivers/tty/serial/qcom_geni_serial.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 21e9160..67fd285c 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -105,7 +105,7 @@ struct qcom_geni_serial_port { > bool brk; > }; > > -static const struct uart_ops qcom_geni_serial_pops; > +static const struct uart_ops qcom_geni_console_pops; > static struct uart_driver qcom_geni_console_driver; > static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop); > static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); > @@ -118,7 +118,14 @@ struct qcom_geni_serial_port { > #define to_dev_port(ptr, member) \ > container_of(ptr, struct qcom_geni_serial_port, member) > > -static struct qcom_geni_serial_port qcom_geni_console_port; > +static struct qcom_geni_serial_port qcom_geni_console_port = { > + .uport = { > + .iotype = UPIO_MEM, > + .ops = &qcom_geni_console_pops, > + .flags = UPF_BOOT_AUTOCONF, > + .line = 0, > + }, > +}; This change itself looks good to me, however I wonder why there is a single qcom_geni_serial_port object that is returned by get_port_from_line() for any line. Neither the initial commit message of the driver not the comments in this file mention a limitation to a single port. Is support for more than one UART planned for future revisions or are there hardware/firmware limitations that impede this? > static int qcom_geni_serial_request_port(struct uart_port *uport) > { > @@ -1133,11 +1140,6 @@ static int __init qcom_geni_serial_init(void) > { > int ret; > > - qcom_geni_console_port.uport.iotype = UPIO_MEM; > - qcom_geni_console_port.uport.ops = &qcom_geni_console_pops; > - qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF; > - qcom_geni_console_port.uport.line = 0; > - > ret = console_register(&qcom_geni_console_driver); > if (ret) > return ret; Reviewed-by: Matthias Kaehlcke <mka@chromium.org> -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/12/2018 4:54 PM, Matthias Kaehlcke wrote: >> >> -static struct qcom_geni_serial_port qcom_geni_console_port; >> +static struct qcom_geni_serial_port qcom_geni_console_port = { >> + .uport = { >> + .iotype = UPIO_MEM, >> + .ops = &qcom_geni_console_pops, >> + .flags = UPF_BOOT_AUTOCONF, >> + .line = 0, >> + }, >> +}; > > This change itself looks good to me, however I wonder why there is > a single qcom_geni_serial_port object that is returned by > get_port_from_line() for any line. Neither the initial commit message > of the driver not the comments in this file mention a limitation to a > single port. > > Is support for more than one UART planned for future revisions or are > there hardware/firmware limitations that impede this? > Yes we can support multiple console ports if required in the future. Also we have plans to add non-console UARTs going forward. Regards, Karthik.
Quoting Karthikeyan Ramasubramanian (2018-04-09 12:38:37) > Perform static initialization of console_port instead of initializing > during module_init. Commit text is supposed to explain why we want such a change, not what the patch is doing which is usually obvious from the code. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/17/2018 12:05 AM, Stephen Boyd wrote: > Quoting Karthikeyan Ramasubramanian (2018-04-09 12:38:37) >> Perform static initialization of console_port instead of initializing >> during module_init. > > Commit text is supposed to explain why we want such a change, not what > the patch is doing which is usually obvious from the code. > I will update the commit text accordingly. Regards, Karthik.
On 4/12/2018 4:54 PM, Matthias Kaehlcke wrote: > On Mon, Apr 09, 2018 at 01:38:37PM -0600, Karthikeyan Ramasubramanian wrote: > >> -static struct qcom_geni_serial_port qcom_geni_console_port; >> +static struct qcom_geni_serial_port qcom_geni_console_port = { >> + .uport = { >> + .iotype = UPIO_MEM, >> + .ops = &qcom_geni_console_pops, >> + .flags = UPF_BOOT_AUTOCONF, >> + .line = 0, >> + }, >> +}; > > This change itself looks good to me, however I wonder why there is > a single qcom_geni_serial_port object that is returned by > get_port_from_line() for any line. Neither the initial commit message > of the driver not the comments in this file mention a limitation to a > single port. > > Is support for more than one UART planned for future revisions or are > there hardware/firmware limitations that impede this? Yes, in addition to console, there are other non-console UART use-cases planned for future revisions > >> static int qcom_geni_serial_request_port(struct uart_port *uport) Regards, Karthik.
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 21e9160..67fd285c 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -105,7 +105,7 @@ struct qcom_geni_serial_port { bool brk; }; -static const struct uart_ops qcom_geni_serial_pops; +static const struct uart_ops qcom_geni_console_pops; static struct uart_driver qcom_geni_console_driver; static int handle_rx_console(struct uart_port *uport, u32 bytes, bool drop); static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port); @@ -118,7 +118,14 @@ struct qcom_geni_serial_port { #define to_dev_port(ptr, member) \ container_of(ptr, struct qcom_geni_serial_port, member) -static struct qcom_geni_serial_port qcom_geni_console_port; +static struct qcom_geni_serial_port qcom_geni_console_port = { + .uport = { + .iotype = UPIO_MEM, + .ops = &qcom_geni_console_pops, + .flags = UPF_BOOT_AUTOCONF, + .line = 0, + }, +}; static int qcom_geni_serial_request_port(struct uart_port *uport) { @@ -1133,11 +1140,6 @@ static int __init qcom_geni_serial_init(void) { int ret; - qcom_geni_console_port.uport.iotype = UPIO_MEM; - qcom_geni_console_port.uport.ops = &qcom_geni_console_pops; - qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF; - qcom_geni_console_port.uport.line = 0; - ret = console_register(&qcom_geni_console_driver); if (ret) return ret;
Perform static initialization of console_port instead of initializing during module_init. Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> --- drivers/tty/serial/qcom_geni_serial.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)