On Sun, Dec 01, 2019 at 04:59:14PM +0900, Hyunki Koo wrote:
> From: Shinbeom Choi <[email protected]>
>
> This commit enables modulization of samsung uart driver.
>
> There was no way to make use of this driver in other module,
> because uart functions were static.
>
> By exporting required functions, user can use this driver
> in other module.
>
> Signed-off-by: Shinbeom Choi <[email protected]>
> Signed-off-by: Hyunki Koo <[email protected]>
> ---
> drivers/tty/serial/samsung.h | 32 ++++++++++++
> drivers/tty/serial/samsung_tty.c | 85 +++++++++++++++-----------------
> 2 files changed, 73 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung.h b/drivers/tty/serial/samsung.h
> index f93022113f59..25be0962284d 100644
> --- a/drivers/tty/serial/samsung.h
> +++ b/drivers/tty/serial/samsung.h
> @@ -144,4 +144,36 @@ static inline void s3c24xx_clear_bit(struct uart_port *port, int idx,
> local_irq_restore(flags);
> }
>
> +#if defined(CONFIG_ARCH_EXYNOS)
> +#define EXYNOS_COMMON_SERIAL_DRV_DATA \
> + .info = &(struct s3c24xx_uart_info) { \
> + .name = "Samsung Exynos UART", \
> + .type = PORT_S3C6400, \
> + .has_divslot = 1, \
> + .rx_fifomask = S5PV210_UFSTAT_RXMASK, \
> + .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT, \
> + .rx_fifofull = S5PV210_UFSTAT_RXFULL, \
> + .tx_fifofull = S5PV210_UFSTAT_TXFULL, \
> + .tx_fifomask = S5PV210_UFSTAT_TXMASK, \
> + .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT, \
> + .def_clk_sel = S3C2410_UCON_CLKSEL0, \
> + .num_clks = 1, \
> + .clksel_mask = 0, \
> + .clksel_shift = 0, \
> + }, \
> + .def_cfg = &(struct s3c2410_uartcfg) { \
> + .ucon = S5PV210_UCON_DEFAULT, \
> + .ufcon = S5PV210_UFCON_DEFAULT, \
> + .has_fracval = 1, \
> + } \
> +
> +#endif
> +
> +int s3c24xx_serial_get_ports(struct s3c24xx_uart_port **ourport, int index);
> +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> + struct platform_device *platdev);
> +int s3c24xx_serial_unregister_port(struct platform_device *dev);
> +int s3c24xx_serial_suspend(struct device *dev);
> +int s3c24xx_serial_resume(struct device *dev);
> +int s3c24xx_serial_resume_noirq(struct device *dev);
> #endif
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index 83fd51607741..15414ecd9008 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1735,7 +1735,7 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> * initialise a single serial port from the platform device given
> */
>
> -static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> struct platform_device *platdev)
> {
> struct uart_port *port = &ourport->port;
> @@ -1842,12 +1842,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> /* reset the fifos (and setup the uart) */
> s3c24xx_serial_resetport(port, cfg);
>
> + if (!s3c24xx_uart_drv.state) {
> + ret = uart_register_driver(&s3c24xx_uart_drv);
> + if (ret < 0) {
> + dev_err(port->dev, "Failed to register Samsung UART driver\n");
> + return ret;
> + }
> + }
> +
> + dbg("%s: adding port\n", __func__);
> + uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> +
> return 0;
>
> err:
> port->mapbase = 0;
> return ret;
> }
> +EXPORT_SYMBOL_GPL(s3c24xx_serial_init_port);
Why are you exporting all of these functions? What other code uses
them? Why are you converting them all to global functions I don't see
any other in-kernel callers, so why are those changes needed here?
totally confused,
greg k-h
On Sun, 1 Dec 2019 at 09:05, Greg KH <[email protected]> wrote:
>
> On Sun, Dec 01, 2019 at 04:59:14PM +0900, Hyunki Koo wrote:
> > From: Shinbeom Choi <[email protected]>
> >
> > This commit enables modulization of samsung uart driver.
> >
> > There was no way to make use of this driver in other module,
> > because uart functions were static.
> >
> > By exporting required functions, user can use this driver
> > in other module.
> >
> > Signed-off-by: Shinbeom Choi <[email protected]>
> > Signed-off-by: Hyunki Koo <[email protected]>
> > ---
> > drivers/tty/serial/samsung.h | 32 ++++++++++++
> > drivers/tty/serial/samsung_tty.c | 85 +++++++++++++++-----------------
> > 2 files changed, 73 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung.h b/drivers/tty/serial/samsung.h
> > index f93022113f59..25be0962284d 100644
> > --- a/drivers/tty/serial/samsung.h
> > +++ b/drivers/tty/serial/samsung.h
> > @@ -144,4 +144,36 @@ static inline void s3c24xx_clear_bit(struct uart_port *port, int idx,
> > local_irq_restore(flags);
> > }
> >
> > +#if defined(CONFIG_ARCH_EXYNOS)
> > +#define EXYNOS_COMMON_SERIAL_DRV_DATA \
> > + .info = &(struct s3c24xx_uart_info) { \
> > + .name = "Samsung Exynos UART", \
> > + .type = PORT_S3C6400, \
> > + .has_divslot = 1, \
> > + .rx_fifomask = S5PV210_UFSTAT_RXMASK, \
> > + .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT, \
> > + .rx_fifofull = S5PV210_UFSTAT_RXFULL, \
> > + .tx_fifofull = S5PV210_UFSTAT_TXFULL, \
> > + .tx_fifomask = S5PV210_UFSTAT_TXMASK, \
> > + .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT, \
> > + .def_clk_sel = S3C2410_UCON_CLKSEL0, \
> > + .num_clks = 1, \
> > + .clksel_mask = 0, \
> > + .clksel_shift = 0, \
> > + }, \
> > + .def_cfg = &(struct s3c2410_uartcfg) { \
> > + .ucon = S5PV210_UCON_DEFAULT, \
> > + .ufcon = S5PV210_UFCON_DEFAULT, \
> > + .has_fracval = 1, \
> > + } \
> > +
> > +#endif
> > +
> > +int s3c24xx_serial_get_ports(struct s3c24xx_uart_port **ourport, int index);
> > +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > + struct platform_device *platdev);
> > +int s3c24xx_serial_unregister_port(struct platform_device *dev);
> > +int s3c24xx_serial_suspend(struct device *dev);
> > +int s3c24xx_serial_resume(struct device *dev);
> > +int s3c24xx_serial_resume_noirq(struct device *dev);
> > #endif
> > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > index 83fd51607741..15414ecd9008 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -1735,7 +1735,7 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> > * initialise a single serial port from the platform device given
> > */
> >
> > -static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > struct platform_device *platdev)
> > {
> > struct uart_port *port = &ourport->port;
> > @@ -1842,12 +1842,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > /* reset the fifos (and setup the uart) */
> > s3c24xx_serial_resetport(port, cfg);
> >
> > + if (!s3c24xx_uart_drv.state) {
> > + ret = uart_register_driver(&s3c24xx_uart_drv);
> > + if (ret < 0) {
> > + dev_err(port->dev, "Failed to register Samsung UART driver\n");
> > + return ret;
> > + }
> > + }
> > +
> > + dbg("%s: adding port\n", __func__);
> > + uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> > +
> > return 0;
> >
> > err:
> > port->mapbase = 0;
> > return ret;
> > }
> > +EXPORT_SYMBOL_GPL(s3c24xx_serial_init_port);
>
> Why are you exporting all of these functions? What other code uses
> them? Why are you converting them all to global functions I don't see
> any other in-kernel callers, so why are those changes needed here?
>
> totally confused,
I cannot find the original email from Hyunki on mailing lists (neither
LKML nor serial) so this was not even public till Greg replied.
Anyway, probably this is for new Android and some out-of-tree usage...
but it is wrong.
Best regards,
Krzysztof
On Thu, Dec 05, 2019 at 04:36:48PM +0100, Krzysztof Kozlowski wrote:
> On Sun, 1 Dec 2019 at 09:05, Greg KH <[email protected]> wrote:
> >
> > On Sun, Dec 01, 2019 at 04:59:14PM +0900, Hyunki Koo wrote:
> > > From: Shinbeom Choi <[email protected]>
> > >
> > > This commit enables modulization of samsung uart driver.
> > >
> > > There was no way to make use of this driver in other module,
> > > because uart functions were static.
> > >
> > > By exporting required functions, user can use this driver
> > > in other module.
> > >
> > > Signed-off-by: Shinbeom Choi <[email protected]>
> > > Signed-off-by: Hyunki Koo <[email protected]>
> > > ---
> > > drivers/tty/serial/samsung.h | 32 ++++++++++++
> > > drivers/tty/serial/samsung_tty.c | 85 +++++++++++++++-----------------
> > > 2 files changed, 73 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/samsung.h b/drivers/tty/serial/samsung.h
> > > index f93022113f59..25be0962284d 100644
> > > --- a/drivers/tty/serial/samsung.h
> > > +++ b/drivers/tty/serial/samsung.h
> > > @@ -144,4 +144,36 @@ static inline void s3c24xx_clear_bit(struct uart_port *port, int idx,
> > > local_irq_restore(flags);
> > > }
> > >
> > > +#if defined(CONFIG_ARCH_EXYNOS)
> > > +#define EXYNOS_COMMON_SERIAL_DRV_DATA \
> > > + .info = &(struct s3c24xx_uart_info) { \
> > > + .name = "Samsung Exynos UART", \
> > > + .type = PORT_S3C6400, \
> > > + .has_divslot = 1, \
> > > + .rx_fifomask = S5PV210_UFSTAT_RXMASK, \
> > > + .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT, \
> > > + .rx_fifofull = S5PV210_UFSTAT_RXFULL, \
> > > + .tx_fifofull = S5PV210_UFSTAT_TXFULL, \
> > > + .tx_fifomask = S5PV210_UFSTAT_TXMASK, \
> > > + .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT, \
> > > + .def_clk_sel = S3C2410_UCON_CLKSEL0, \
> > > + .num_clks = 1, \
> > > + .clksel_mask = 0, \
> > > + .clksel_shift = 0, \
> > > + }, \
> > > + .def_cfg = &(struct s3c2410_uartcfg) { \
> > > + .ucon = S5PV210_UCON_DEFAULT, \
> > > + .ufcon = S5PV210_UFCON_DEFAULT, \
> > > + .has_fracval = 1, \
> > > + } \
> > > +
> > > +#endif
> > > +
> > > +int s3c24xx_serial_get_ports(struct s3c24xx_uart_port **ourport, int index);
> > > +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > > + struct platform_device *platdev);
> > > +int s3c24xx_serial_unregister_port(struct platform_device *dev);
> > > +int s3c24xx_serial_suspend(struct device *dev);
> > > +int s3c24xx_serial_resume(struct device *dev);
> > > +int s3c24xx_serial_resume_noirq(struct device *dev);
> > > #endif
> > > diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> > > index 83fd51607741..15414ecd9008 100644
> > > --- a/drivers/tty/serial/samsung_tty.c
> > > +++ b/drivers/tty/serial/samsung_tty.c
> > > @@ -1735,7 +1735,7 @@ static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> > > * initialise a single serial port from the platform device given
> > > */
> > >
> > > -static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > > +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > > struct platform_device *platdev)
> > > {
> > > struct uart_port *port = &ourport->port;
> > > @@ -1842,12 +1842,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > > /* reset the fifos (and setup the uart) */
> > > s3c24xx_serial_resetport(port, cfg);
> > >
> > > + if (!s3c24xx_uart_drv.state) {
> > > + ret = uart_register_driver(&s3c24xx_uart_drv);
> > > + if (ret < 0) {
> > > + dev_err(port->dev, "Failed to register Samsung UART driver\n");
> > > + return ret;
> > > + }
> > > + }
> > > +
> > > + dbg("%s: adding port\n", __func__);
> > > + uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> > > +
> > > return 0;
> > >
> > > err:
> > > port->mapbase = 0;
> > > return ret;
> > > }
> > > +EXPORT_SYMBOL_GPL(s3c24xx_serial_init_port);
> >
> > Why are you exporting all of these functions? What other code uses
> > them? Why are you converting them all to global functions I don't see
> > any other in-kernel callers, so why are those changes needed here?
> >
> > totally confused,
>
> I cannot find the original email from Hyunki on mailing lists (neither
> LKML nor serial) so this was not even public till Greg replied.
I think it might have been sent in html format.
> Anyway, probably this is for new Android and some out-of-tree usage...
> but it is wrong.
Making the driver be able to be built as a module is a good thing, no
matter what project is causing the work to have happen.
And yes, it's Android :)
thanks,
greg k-h
To support module for Samsung serial driver,
I would like to split the file into 2 files.
Because it cannot be supported in one file both early console and
module driver
Thus some function need to change to EXPORT_SYMBOL to use in module
driver file.
I'm not pushed yet for module driver.
> -----Original Message-----
> From: Greg KH <[email protected]>
> Sent: Friday, December 6, 2019 1:03 AM
> To: Krzysztof Kozlowski <[email protected]>
> Cc: Hyunki Koo <[email protected]>; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; Shinbeom Choi <[email protected]>; Hyunki Koo
> <[email protected]>
> Subject: Re: [PATCH] tty: serial: samsung: support driver modulization
>
> On Thu, Dec 05, 2019 at 04:36:48PM +0100, Krzysztof Kozlowski wrote:
> > On Sun, 1 Dec 2019 at 09:05, Greg KH <[email protected]>
> wrote:
> > >
> > > On Sun, Dec 01, 2019 at 04:59:14PM +0900, Hyunki Koo wrote:
> > > > From: Shinbeom Choi <[email protected]>
> > > >
> > > > This commit enables modulization of samsung uart driver.
> > > >
> > > > There was no way to make use of this driver in other module,
> > > > because uart functions were static.
> > > >
> > > > By exporting required functions, user can use this driver in
> other
> > > > module.
> > > >
> > > > Signed-off-by: Shinbeom Choi <[email protected]>
> > > > Signed-off-by: Hyunki Koo <[email protected]>
> > > > ---
> > > > drivers/tty/serial/samsung.h | 32 ++++++++++++
> > > > drivers/tty/serial/samsung_tty.c | 85
> > > > +++++++++++++++-----------------
> > > > 2 files changed, 73 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/tty/serial/samsung.h
> > > > b/drivers/tty/serial/samsung.h index f93022113f59..25be0962284d
> > > > 100644
> > > > --- a/drivers/tty/serial/samsung.h
> > > > +++ b/drivers/tty/serial/samsung.h
> > > > @@ -144,4 +144,36 @@ static inline void s3c24xx_clear_bit(struct
> uart_port *port, int idx,
> > > > local_irq_restore(flags);
> > > > }
> > > >
> > > > +#if defined(CONFIG_ARCH_EXYNOS)
> > > > +#define EXYNOS_COMMON_SERIAL_DRV_DATA
\
> > > > + .info = &(struct s3c24xx_uart_info) { \
> > > > + .name = "Samsung Exynos UART", \
> > > > + .type = PORT_S3C6400, \
> > > > + .has_divslot = 1, \
> > > > + .rx_fifomask = S5PV210_UFSTAT_RXMASK, \
> > > > + .rx_fifoshift = S5PV210_UFSTAT_RXSHIFT, \
> > > > + .rx_fifofull = S5PV210_UFSTAT_RXFULL, \
> > > > + .tx_fifofull = S5PV210_UFSTAT_TXFULL, \
> > > > + .tx_fifomask = S5PV210_UFSTAT_TXMASK, \
> > > > + .tx_fifoshift = S5PV210_UFSTAT_TXSHIFT, \
> > > > + .def_clk_sel = S3C2410_UCON_CLKSEL0, \
> > > > + .num_clks = 1, \
> > > > + .clksel_mask = 0, \
> > > > + .clksel_shift = 0, \
> > > > + }, \
> > > > + .def_cfg = &(struct s3c2410_uartcfg) { \
> > > > + .ucon = S5PV210_UCON_DEFAULT, \
> > > > + .ufcon = S5PV210_UFCON_DEFAULT, \
> > > > + .has_fracval = 1, \
> > > > + } \
> > > > +
> > > > +#endif
> > > > +
> > > > +int s3c24xx_serial_get_ports(struct s3c24xx_uart_port
**ourport,
> > > > +int index); int s3c24xx_serial_init_port(struct
> s3c24xx_uart_port *ourport,
> > > > + struct platform_device
> > > > +*platdev); int s3c24xx_serial_unregister_port(struct
> > > > +platform_device *dev); int s3c24xx_serial_suspend(struct device
> > > > +*dev); int s3c24xx_serial_resume(struct device *dev); int
> > > > +s3c24xx_serial_resume_noirq(struct device *dev);
> > > > #endif
> > > > diff --git a/drivers/tty/serial/samsung_tty.c
> > > > b/drivers/tty/serial/samsung_tty.c
> > > > index 83fd51607741..15414ecd9008 100644
> > > > --- a/drivers/tty/serial/samsung_tty.c
> > > > +++ b/drivers/tty/serial/samsung_tty.c
> > > > @@ -1735,7 +1735,7 @@ static int
> s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> > > > * initialise a single serial port from the platform device
> given
> > > > */
> > > >
> > > > -static int s3c24xx_serial_init_port(struct s3c24xx_uart_port
> > > > *ourport,
> > > > +int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
> > > > struct platform_device
*platdev)
> > > > {
> > > > struct uart_port *port = &ourport->port; @@ -1842,12
> > > > +1842,24 @@ static int s3c24xx_serial_init_port(struct
> s3c24xx_uart_port *ourport,
> > > > /* reset the fifos (and setup the uart) */
> > > > s3c24xx_serial_resetport(port, cfg);
> > > >
> > > > + if (!s3c24xx_uart_drv.state) {
> > > > + ret = uart_register_driver(&s3c24xx_uart_drv);
> > > > + if (ret < 0) {
> > > > + dev_err(port->dev, "Failed to register
Samsung
> UART driver\n");
> > > > + return ret;
> > > > + }
> > > > + }
> > > > +
> > > > + dbg("%s: adding port\n", __func__);
> > > > + uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
> > > > +
> > > > return 0;
> > > >
> > > > err:
> > > > port->mapbase = 0;
> > > > return ret;
> > > > }
> > > > +EXPORT_SYMBOL_GPL(s3c24xx_serial_init_port);
> > >
> > > Why are you exporting all of these functions? What other code
> uses
> > > them? Why are you converting them all to global functions I don't
> > > see any other in-kernel callers, so why are those changes needed
> here?
> > >
> > > totally confused,
> >
> > I cannot find the original email from Hyunki on mailing lists
> (neither
> > LKML nor serial) so this was not even public till Greg replied.
>
> I think it might have been sent in html format.
>
> > Anyway, probably this is for new Android and some out-of-tree
> usage...
> > but it is wrong.
>
> Making the driver be able to be built as a module is a good thing, no
> matter what project is causing the work to have happen.
>
> And yes, it's Android :)
>
> thanks,
>
> greg k-h
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Fri, Dec 06, 2019 at 10:44:20AM +0900, ������/HYUN-KI KOO wrote:
> To support module for Samsung serial driver,
> I would like to split the file into 2 files.
But you did not do that here in this patch, right?
> Because it cannot be supported in one file both early console and
> module driver
> Thus some function need to change to EXPORT_SYMBOL to use in module
> driver file.
> I'm not pushed yet for module driver.
I do not understand, this patch feels wrong and incomplete as-is, right?
Please fix it up to work properly.
thanks,
greg k-h
On Fri, 6 Dec 2019 at 02:44, 구현기/HYUN-KI KOO <[email protected]> wrote:
>
> To support module for Samsung serial driver,
> I would like to split the file into 2 files.
> Because it cannot be supported in one file both early console and
> module driver
> Thus some function need to change to EXPORT_SYMBOL to use in module
> driver file.
> I'm not pushed yet for module driver.
Hi,
You sent few patches independently of each other. If we did not ask,
we would not get the idea why you need them. This is not how the Linux
kernel development works. Instead, start preparing a proper patchset
to achieve your goal. This patchset will contain multiple patches. It
should go with a cover letter (see git format-patch) explaining why
you do it. This way reviewers will know the big picture. The entire
work should not break existing systems (see comments for IRQ combiner
or pinctrl). You should consider how your change will affect existing
systems.
Best regards,
Krzysztof