[PATCH v2 0/6] serial: 8250: fixes for modular build

[PATCH v2 0/6] serial: 8250: fixes for modular build

[Arnd Bergmann]

This is a re-send of three patches I sent a couple of days ago,
with a revert of two patches that Paul did, taking the code in
the opposite direction now, towards ensuring the drivers actually
work as loadable modules, which helps with 'allmodconfig' build
testing.

I've also added another patch for uniphier, and fixed smaller
related issues that came up in further testing.

	Arnd

[PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"

[Arnd Bergmann]

This reverts commit d72d391c126e, which tried to remove dead code but
left the driver in a useless state when the main 8250 driver is a
loadable module. This would normally result in a link error, but
as the entire drivers/tty/serial/8250/ directory is only entered
when CONFIG_SERIAL_8250 is set, we never notice that the driver does
not get built in this configuration.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: d72d391c126e ("drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular")
---
 drivers/tty/serial/8250/8250_mtk.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index 0e590b233f03..78883ca64ddd 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -16,7 +16,7 @@
  */
 #include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
@@ -245,6 +245,23 @@ static int mtk8250_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int mtk8250_remove(struct platform_device *pdev)
+{
+	struct mtk8250_data *data = platform_get_drvdata(pdev);
+
+	pm_runtime_get_sync(&pdev->dev);
+
+	serial8250_unregister_port(data->line);
+
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_put_noidle(&pdev->dev);
+
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		mtk8250_runtime_suspend(&pdev->dev);
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int mtk8250_suspend(struct device *dev)
 {
@@ -275,18 +292,18 @@ static const struct of_device_id mtk8250_of_match[] = {
 	{ .compatible = "mediatek,mt6577-uart" },
 	{ /* Sentinel */ }
 };
+MODULE_DEVICE_TABLE(of, mtk8250_of_match);
 
 static struct platform_driver mtk8250_platform_driver = {
 	.driver = {
-		.name			= "mt6577-uart",
-		.pm			= &mtk8250_pm_ops,
-		.of_match_table		= mtk8250_of_match,
-		.suppress_bind_attrs	= true,
-
+		.name		= "mt6577-uart",
+		.pm		= &mtk8250_pm_ops,
+		.of_match_table	= mtk8250_of_match,
 	},
 	.probe			= mtk8250_probe,
+	.remove			= mtk8250_remove,
 };
-builtin_platform_driver(mtk8250_platform_driver);
+module_platform_driver(mtk8250_platform_driver);
 
 #ifdef CONFIG_SERIAL_8250_CONSOLE
 static int __init early_mtk8250_setup(struct earlycon_device *device,
@@ -302,3 +319,7 @@ static int __init early_mtk8250_setup(struct earlycon_device *device,
 
 OF_EARLYCON_DECLARE(mtk8250, "mediatek,mt6577-uart", early_mtk8250_setup);
 #endif
+
+MODULE_AUTHOR("Matthias Brugger");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Mediatek 8250 serial port driver");
-- 
2.7.0

Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"

[Paul Gortmaker]

[[PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:

> This reverts commit d72d391c126e, which tried to remove dead code but
> left the driver in a useless state when the main 8250 driver is a

Am I misunderstanding something?  The commit didn't cause the driver to
be in a useless state for 8250=m.  But rather isn't that it was a
pre-existing condition, independent of the change to 8250_mtk.c to
remove the dead code in d72d391c126e?

Since the commit did not touch Kconfig or Makefile, I can't see how it
could cause some new useless state that did not already exist, and hence
the "Fixes:" tag is invalid as well.

Paul.
--

> loadable module. This would normally result in a link error, but
> as the entire drivers/tty/serial/8250/ directory is only entered
> when CONFIG_SERIAL_8250 is set, we never notice that the driver does
> not get built in this configuration.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: d72d391c126e ("drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular")
> ---
>  drivers/tty/serial/8250/8250_mtk.c | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index 0e590b233f03..78883ca64ddd 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -16,7 +16,7 @@
>   */
>  #include <linux/clk.h>
>  #include <linux/io.h>
> -#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> @@ -245,6 +245,23 @@ static int mtk8250_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int mtk8250_remove(struct platform_device *pdev)
> +{
> +	struct mtk8250_data *data = platform_get_drvdata(pdev);
> +
> +	pm_runtime_get_sync(&pdev->dev);
> +
> +	serial8250_unregister_port(data->line);
> +
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		mtk8250_runtime_suspend(&pdev->dev);
> +
> +	return 0;
> +}
> +
>  #ifdef CONFIG_PM_SLEEP
>  static int mtk8250_suspend(struct device *dev)
>  {
> @@ -275,18 +292,18 @@ static const struct of_device_id mtk8250_of_match[] = {
>  	{ .compatible = "mediatek,mt6577-uart" },
>  	{ /* Sentinel */ }
>  };
> +MODULE_DEVICE_TABLE(of, mtk8250_of_match);
>  
>  static struct platform_driver mtk8250_platform_driver = {
>  	.driver = {
> -		.name			= "mt6577-uart",
> -		.pm			= &mtk8250_pm_ops,
> -		.of_match_table		= mtk8250_of_match,
> -		.suppress_bind_attrs	= true,
> -
> +		.name		= "mt6577-uart",
> +		.pm		= &mtk8250_pm_ops,
> +		.of_match_table	= mtk8250_of_match,
>  	},
>  	.probe			= mtk8250_probe,
> +	.remove			= mtk8250_remove,
>  };
> -builtin_platform_driver(mtk8250_platform_driver);
> +module_platform_driver(mtk8250_platform_driver);
>  
>  #ifdef CONFIG_SERIAL_8250_CONSOLE
>  static int __init early_mtk8250_setup(struct earlycon_device *device,
> @@ -302,3 +319,7 @@ static int __init early_mtk8250_setup(struct earlycon_device *device,
>  
>  OF_EARLYCON_DECLARE(mtk8250, "mediatek,mt6577-uart", early_mtk8250_setup);
>  #endif
> +
> +MODULE_AUTHOR("Matthias Brugger");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Mediatek 8250 serial port driver");
> -- 
> 2.7.0
> 

Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"

[Arnd Bergmann]

On Thursday 11 February 2016 11:00:22 Paul Gortmaker wrote:
> [[PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:
> 
> > This reverts commit d72d391c126e, which tried to remove dead code but
> > left the driver in a useless state when the main 8250 driver is a
> 
> Am I misunderstanding something?  The commit didn't cause the driver to
> be in a useless state for 8250=m.  But rather isn't that it was a
> pre-existing condition, independent of the change to 8250_mtk.c to
> remove the dead code in d72d391c126e?
> 
> Since the commit did not touch Kconfig or Makefile, I can't see how it
> could cause some new useless state that did not already exist, and hence
> the "Fixes:" tag is invalid as well.

My wording may have been bad here. What I meant to say is that it
was broken before the patch, and still broken after the patch.

The Fixes tag was meant to just be a reference to the commit I'm
reverting.

	Arnd

Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"

[Paul Gortmaker]

[Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 17:06) Arnd Bergmann wrote:

> On Thursday 11 February 2016 11:00:22 Paul Gortmaker wrote:
> > [[PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:
> > 
> > > This reverts commit d72d391c126e, which tried to remove dead code but
> > > left the driver in a useless state when the main 8250 driver is a
> > 
> > Am I misunderstanding something?  The commit didn't cause the driver to
> > be in a useless state for 8250=m.  But rather isn't that it was a
> > pre-existing condition, independent of the change to 8250_mtk.c to
> > remove the dead code in d72d391c126e?
> > 
> > Since the commit did not touch Kconfig or Makefile, I can't see how it
> > could cause some new useless state that did not already exist, and hence
> > the "Fixes:" tag is invalid as well.
> 
> My wording may have been bad here. What I meant to say is that it
> was broken before the patch, and still broken after the patch.

OK, no problem. I just didn't want Greg/Jiri to think I was sending them
broken commits.   Will need a v3 to get rid of the extra module.h
anyway, so that gives you a chance to reword.

> 
> The Fixes tag was meant to just be a reference to the commit I'm
> reverting.

Yeah, but since the stable people trigger off of that, and since the
revert doesn't really fix anything, that is probably best removed.
The stable trees don't need the revert.

Thanks,
Paul.

Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"

[Arnd Bergmann]

On Thursday 11 February 2016 11:28:52 Paul Gortmaker wrote:
> [Re: [PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 17:06) Arnd Bergmann wrote:
> 
> > On Thursday 11 February 2016 11:00:22 Paul Gortmaker wrote:
> > > [[PATCH v2 1/6] Revert "drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular"] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:
> > > 
> > > > This reverts commit d72d391c126e, which tried to remove dead code but
> > > > left the driver in a useless state when the main 8250 driver is a
> > > 
> > > Am I misunderstanding something?  The commit didn't cause the driver to
> > > be in a useless state for 8250=m.  But rather isn't that it was a
> > > pre-existing condition, independent of the change to 8250_mtk.c to
> > > remove the dead code in d72d391c126e?
> > > 
> > > Since the commit did not touch Kconfig or Makefile, I can't see how it
> > > could cause some new useless state that did not already exist, and hence
> > > the "Fixes:" tag is invalid as well.
> > 
> > My wording may have been bad here. What I meant to say is that it
> > was broken before the patch, and still broken after the patch.
> 
> OK, no problem. I just didn't want Greg/Jiri to think I was sending them
> broken commits.   Will need a v3 to get rid of the extra module.h
> anyway, so that gives you a chance to reword.

Sure.

> > 
> > The Fixes tag was meant to just be a reference to the commit I'm
> > reverting.
> 
> Yeah, but since the stable people trigger off of that, and since the
> revert doesn't really fix anything, that is probably best removed.
> The stable trees don't need the revert.

Yes, good point.

	Arnd

[PATCH v2 2/6] Revert "drivers/tty/serial: make 8250/8250_ingenic.c explicitly non-modular"

[Arnd Bergmann]

This reverts commit cafe1ac64023, which tried to remove dead code but
left the driver in a useless state when the main 8250 driver is a
loadable module. This would normally result in a link error, but
as the entire drivers/tty/serial/8250/ directory is only entered
when CONFIG_SERIAL_8250 is set, we never notice that the driver does
not get built in this configuration.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Fixes: cafe1ac64023 ("drivers/tty: make serial 8250_ingenic.c explicitly non-modular")
---
 drivers/tty/serial/8250/8250_ingenic.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
index b4e1d39805b2..97b78558caed 100644
--- a/drivers/tty/serial/8250/8250_ingenic.c
+++ b/drivers/tty/serial/8250/8250_ingenic.c
@@ -4,8 +4,6 @@
  *
  * Ingenic SoC UART support
  *
- * Author: Paul Burton <paul.burton@imgtec.com>
- *
  * This program is free software; you can redistribute	 it and/or modify it
  * under  the terms of	 the GNU General  Public License as published by the
  * Free Software Foundation;  either version 2 of the	License, or (at your
@@ -20,7 +18,7 @@
 #include <linux/console.h>
 #include <linux/io.h>
 #include <linux/libfdt.h>
-#include <linux/init.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_fdt.h>
 #include <linux/of_device.h>
@@ -303,6 +301,16 @@ out:
 	return err;
 }
 
+static int ingenic_uart_remove(struct platform_device *pdev)
+{
+	struct ingenic_uart_data *data = platform_get_drvdata(pdev);
+
+	serial8250_unregister_port(data->line);
+	clk_disable_unprepare(data->clk_module);
+	clk_disable_unprepare(data->clk_baud);
+	return 0;
+}
+
 static const struct ingenic_uart_config jz4740_uart_config = {
 	.tx_loadsz = 8,
 	.fifosize = 16,
@@ -325,13 +333,19 @@ static const struct of_device_id of_match[] = {
 	{ .compatible = "ingenic,jz4780-uart", .data = &jz4780_uart_config },
 	{ /* sentinel */ }
 };
+MODULE_DEVICE_TABLE(of, of_match);
 
 static struct platform_driver ingenic_uart_platform_driver = {
 	.driver = {
-		.name			= "ingenic-uart",
-		.of_match_table		= of_match,
-		.suppress_bind_attrs	= true,
+		.name		= "ingenic-uart",
+		.of_match_table	= of_match,
 	},
 	.probe			= ingenic_uart_probe,
+	.remove			= ingenic_uart_remove,
 };
-builtin_platform_driver(ingenic_uart_platform_driver);
+
+module_platform_driver(ingenic_uart_platform_driver);
+
+MODULE_AUTHOR("Paul Burton");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Ingenic SoC UART driver");
-- 
2.7.0

Re: [PATCH v2 2/6] Revert "drivers/tty/serial: make 8250/8250_ingenic.c explicitly non-modular"

[Paul Gortmaker]

[[PATCH v2 2/6] Revert "drivers/tty/serial: make 8250/8250_ingenic.c explicitly non-modular"] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:

> This reverts commit cafe1ac64023, which tried to remove dead code but
> left the driver in a useless state when the main 8250 driver is a
> loadable module. This would normally result in a link error, but
> as the entire drivers/tty/serial/8250/ directory is only entered
> when CONFIG_SERIAL_8250 is set, we never notice that the driver does
> not get built in this configuration.

Same comments apply here as per 1/6.

OTOH, if you were to say that 8250=m and this driver =y is a useless
configuration, and the easiest way to rectify that was to convert it
to tristate, and hence we revert cafe1ac64023 to regain some modular
code -- that would make more sense to me.

P.
--

> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: cafe1ac64023 ("drivers/tty: make serial 8250_ingenic.c explicitly non-modular")
> ---
>  drivers/tty/serial/8250/8250_ingenic.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index b4e1d39805b2..97b78558caed 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -4,8 +4,6 @@
>   *
>   * Ingenic SoC UART support
>   *
> - * Author: Paul Burton <paul.burton@imgtec.com>
> - *
>   * This program is free software; you can redistribute	 it and/or modify it
>   * under  the terms of	 the GNU General  Public License as published by the
>   * Free Software Foundation;  either version 2 of the	License, or (at your
> @@ -20,7 +18,7 @@
>  #include <linux/console.h>
>  #include <linux/io.h>
>  #include <linux/libfdt.h>
> -#include <linux/init.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_fdt.h>
>  #include <linux/of_device.h>
> @@ -303,6 +301,16 @@ out:
>  	return err;
>  }
>  
> +static int ingenic_uart_remove(struct platform_device *pdev)
> +{
> +	struct ingenic_uart_data *data = platform_get_drvdata(pdev);
> +
> +	serial8250_unregister_port(data->line);
> +	clk_disable_unprepare(data->clk_module);
> +	clk_disable_unprepare(data->clk_baud);
> +	return 0;
> +}
> +
>  static const struct ingenic_uart_config jz4740_uart_config = {
>  	.tx_loadsz = 8,
>  	.fifosize = 16,
> @@ -325,13 +333,19 @@ static const struct of_device_id of_match[] = {
>  	{ .compatible = "ingenic,jz4780-uart", .data = &jz4780_uart_config },
>  	{ /* sentinel */ }
>  };
> +MODULE_DEVICE_TABLE(of, of_match);
>  
>  static struct platform_driver ingenic_uart_platform_driver = {
>  	.driver = {
> -		.name			= "ingenic-uart",
> -		.of_match_table		= of_match,
> -		.suppress_bind_attrs	= true,
> +		.name		= "ingenic-uart",
> +		.of_match_table	= of_match,
>  	},
>  	.probe			= ingenic_uart_probe,
> +	.remove			= ingenic_uart_remove,
>  };
> -builtin_platform_driver(ingenic_uart_platform_driver);
> +
> +module_platform_driver(ingenic_uart_platform_driver);
> +
> +MODULE_AUTHOR("Paul Burton");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Ingenic SoC UART driver");
> -- 
> 2.7.0
> 

[PATCH v2 3/6] serial: 8250/mediatek: mark PM functions as __maybe_unused

[Arnd Bergmann]

The mtk8250_runtime_suspend function is not used when runtime PM is
disabled, so we get a warning about an unused function:

drivers/tty/serial/8250/8250_mtk.c:119:12: error: 'mtk8250_runtime_suspend' defined but not used [-Werror=unused-function]
 static int mtk8250_runtime_suspend(struct device *dev)

This marks all the PM functions as __maybe_unused to avoid the warning,
and removes the #ifdef around the PM_SLEEP functions for consistency.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Acked-by: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/tty/serial/8250/8250_mtk.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index 78883ca64ddd..6ecc6e3e82dc 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -116,7 +116,7 @@ mtk8250_set_termios(struct uart_port *port, struct ktermios *termios,
 		tty_termios_encode_baud_rate(termios, baud, baud);
 }
 
-static int mtk8250_runtime_suspend(struct device *dev)
+static int __maybe_unused mtk8250_runtime_suspend(struct device *dev)
 {
 	struct mtk8250_data *data = dev_get_drvdata(dev);
 
@@ -126,7 +126,7 @@ static int mtk8250_runtime_suspend(struct device *dev)
 	return 0;
 }
 
-static int mtk8250_runtime_resume(struct device *dev)
+static int __maybe_unused mtk8250_runtime_resume(struct device *dev)
 {
 	struct mtk8250_data *data = dev_get_drvdata(dev);
 	int err;
@@ -262,8 +262,7 @@ static int mtk8250_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
-static int mtk8250_suspend(struct device *dev)
+static int __maybe_unused mtk8250_suspend(struct device *dev)
 {
 	struct mtk8250_data *data = dev_get_drvdata(dev);
 
@@ -272,7 +271,7 @@ static int mtk8250_suspend(struct device *dev)
 	return 0;
 }
 
-static int mtk8250_resume(struct device *dev)
+static int __maybe_unused mtk8250_resume(struct device *dev)
 {
 	struct mtk8250_data *data = dev_get_drvdata(dev);
 
@@ -280,7 +279,6 @@ static int mtk8250_resume(struct device *dev)
 
 	return 0;
 }
-#endif /* CONFIG_PM_SLEEP */
 
 static const struct dev_pm_ops mtk8250_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(mtk8250_suspend, mtk8250_resume)
-- 
2.7.0

[PATCH v2 4/6] serial: 8250/uniphier: fix modular build

[Arnd Bergmann]

The newly added uniphier serial port driver fails to build as
a loadable module when the base 8250 driver is built-in and
its console support enabled:

ERROR: "early_serial8250_setup" [drivers/tty/serial/8250/8250_uniphier.ko] undefined!

This changes the driver to only provide the early console support
if it is built-in itself as well.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/8250/8250_uniphier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
index bab6b3ae2540..1b7bd26555b7 100644
--- a/drivers/tty/serial/8250/8250_uniphier.c
+++ b/drivers/tty/serial/8250/8250_uniphier.c
@@ -35,7 +35,7 @@ struct uniphier8250_priv {
 	spinlock_t atomic_write_lock;
 };
 
-#ifdef CONFIG_SERIAL_8250_CONSOLE
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
 static int __init uniphier_early_console_setup(struct earlycon_device *device,
 					       const char *options)
 {
-- 
2.7.0

Re: [PATCH v2 4/6] serial: 8250/uniphier: fix modular build

[Masahiro Yamada]

Hi Arnd,




2016-02-12 0:41 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> The newly added uniphier serial port driver fails to build as
> a loadable module when the base 8250 driver is built-in and
> its console support enabled:
>
> ERROR: "early_serial8250_setup" [drivers/tty/serial/8250/8250_uniphier.ko] undefined!
>
> This changes the driver to only provide the early console support
> if it is built-in itself as well.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/tty/serial/8250/8250_uniphier.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_uniphier.c b/drivers/tty/serial/8250/8250_uniphier.c
> index bab6b3ae2540..1b7bd26555b7 100644
> --- a/drivers/tty/serial/8250/8250_uniphier.c
> +++ b/drivers/tty/serial/8250/8250_uniphier.c
> @@ -35,7 +35,7 @@ struct uniphier8250_priv {
>         spinlock_t atomic_write_lock;
>  };
>
> -#ifdef CONFIG_SERIAL_8250_CONSOLE
> +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
>  static int __init uniphier_early_console_setup(struct earlycon_device *device,
>                                                const char *options)
>  {
> --
> 2.7.0
>

I noticed commit 2eaa790989e03900298ad24f77f1086dbbc1aebd
revived this link error, but I am not happy with seeing
this patch again and again.


Can you check this patch?
https://patchwork.kernel.org/patch/8289231/



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 4/6] serial: 8250/uniphier: fix modular build

[Arnd Bergmann]

On Friday 12 February 2016 18:37:08 Masahiro Yamada wrote:
> >  };
> >
> > -#ifdef CONFIG_SERIAL_8250_CONSOLE
> > +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
> >  static int __init uniphier_early_console_setup(struct earlycon_device *device,
> >                                                const char *options)
> >  {
> > --
> > 2.7.0
> >
> 
> I noticed commit 2eaa790989e03900298ad24f77f1086dbbc1aebd
> revived this link error, but I am not happy with seeing
> this patch again and again.
> 
> 
> Can you check this patch?
> https://patchwork.kernel.org/patch/8289231/
> 
> 
> 

Yes, I think your patch is nicer than mine. Let's see what the others
think, then I can resend my remaining patches on top of yours.

	Arnd

Re: [PATCH v2 4/6] serial: 8250/uniphier: fix modular build

[Masahiro Yamada]

Hi Arnd,


2016-02-12 19:07 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Friday 12 February 2016 18:37:08 Masahiro Yamada wrote:
>> >  };
>> >
>> > -#ifdef CONFIG_SERIAL_8250_CONSOLE
>> > +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
>> >  static int __init uniphier_early_console_setup(struct earlycon_device *device,
>> >                                                const char *options)
>> >  {
>> > --
>> > 2.7.0
>> >
>>
>> I noticed commit 2eaa790989e03900298ad24f77f1086dbbc1aebd
>> revived this link error, but I am not happy with seeing
>> this patch again and again.
>>
>>
>> Can you check this patch?
>> https://patchwork.kernel.org/patch/8289231/
>>
>>
>>
>
> Yes, I think your patch is nicer than mine. Let's see what the others
> think, then I can resend my remaining patches on top of yours.

OK, thanks!

I noticed some typos, so I've posted v2.
(no change in code-diff).



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2 4/6] serial: 8250/uniphier: fix modular build

[Masahiro Yamada]

Hi Arnd,


2016-02-12 19:07 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Friday 12 February 2016 18:37:08 Masahiro Yamada wrote:
>> >  };
>> >
>> > -#ifdef CONFIG_SERIAL_8250_CONSOLE
>> > +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
>> >  static int __init uniphier_early_console_setup(struct earlycon_device *device,
>> >                                                const char *options)
>> >  {
>> > --
>> > 2.7.0
>> >
>>
>> I noticed commit 2eaa790989e03900298ad24f77f1086dbbc1aebd
>> revived this link error, but I am not happy with seeing
>> this patch again and again.
>>
>>
>> Can you check this patch?
>> https://patchwork.kernel.org/patch/8289231/
>>
>>
>>
>
> Yes, I think your patch is nicer than mine. Let's see what the others
> think, then I can resend my remaining patches on top of yours.
>

As Peter pointed out, my patch would not work without CONFIG_OF.

I can not suggest any other alternative,
so I am OK with this patch.

Unfortunately, the "tty-linus" branch is getting dirty with
apply, revert, apply again.



-- 
Best Regards
Masahiro Yamada

[PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m

[Arnd Bergmann]

The Mediatek 8250 driver has a 'bool' Kconfig symbol, but that
breaks when SERIAL_8250 is a loadable module:

drivers/tty/built-in.o: In function `mtk8250_set_termios':
:(.text+0x1bee8): undefined reference to `serial8250_do_set_termios'
:(.text+0x1bf10): undefined reference to `uart_get_baud_rate'
:(.text+0x1c09c): undefined reference to `uart_get_divisor'
drivers/tty/built-in.o: In function `mtk8250_do_pm':
:(.text+0x1c0d0): undefined reference to `serial8250_do_pm'
drivers/tty/built-in.o: In function `mtk8250_probe':
:(.text+0x1c2e4): undefined reference to `serial8250_register_8250_port'
serial/8250/8250_mtk.c:287:242: error: data definition has no type or storage class [-Werror]
serial/8250/8250_mtk.c:287:122: error: 'mtk8250_platform_driver_init' defined but not used [-Werror=unused-function]

This changes the symbol to a 'tristate', so the dependency on
SERIAL_8250 also works when that is set to 'm'.
To actually build the driver, we also need to include <linux/module.h>.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/8250/8250_mtk.c | 3 ++-
 drivers/tty/serial/8250/Kconfig    | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index 6ecc6e3e82dc..55be90c1d85e 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/serial_8250.h>
@@ -303,7 +304,7 @@ static struct platform_driver mtk8250_platform_driver = {
 };
 module_platform_driver(mtk8250_platform_driver);
 
-#ifdef CONFIG_SERIAL_8250_CONSOLE
+#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
 static int __init early_mtk8250_setup(struct earlycon_device *device,
 					const char *options)
 {
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 67ad6b0d595b..6ae0fae4f796 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,7 +370,7 @@ config SERIAL_8250_LPC18XX
 	  serial port, say Y to this option. If unsure, say Y.
 
 config SERIAL_8250_MT6577
-	bool "Mediatek serial port support"
+	tristate "Mediatek serial port support"
 	depends on SERIAL_8250 && ARCH_MEDIATEK
 	help
 	  If you have a Mediatek based board and want to use the
-- 
2.7.0

Re: [PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m

[Paul Gortmaker]

[[PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m] On 11/02/2016 (Thu 16:41) Arnd Bergmann wrote:

> The Mediatek 8250 driver has a 'bool' Kconfig symbol, but that
> breaks when SERIAL_8250 is a loadable module:
> 
> drivers/tty/built-in.o: In function `mtk8250_set_termios':
> :(.text+0x1bee8): undefined reference to `serial8250_do_set_termios'
> :(.text+0x1bf10): undefined reference to `uart_get_baud_rate'
> :(.text+0x1c09c): undefined reference to `uart_get_divisor'
> drivers/tty/built-in.o: In function `mtk8250_do_pm':
> :(.text+0x1c0d0): undefined reference to `serial8250_do_pm'
> drivers/tty/built-in.o: In function `mtk8250_probe':
> :(.text+0x1c2e4): undefined reference to `serial8250_register_8250_port'
> serial/8250/8250_mtk.c:287:242: error: data definition has no type or storage class [-Werror]
> serial/8250/8250_mtk.c:287:122: error: 'mtk8250_platform_driver_init' defined but not used [-Werror=unused-function]
> 
> This changes the symbol to a 'tristate', so the dependency on
> SERIAL_8250 also works when that is set to 'm'.
> To actually build the driver, we also need to include <linux/module.h>.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/tty/serial/8250/8250_mtk.c | 3 ++-
>  drivers/tty/serial/8250/Kconfig    | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index 6ecc6e3e82dc..55be90c1d85e 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
> +#include <linux/module.h>

Something isn't right here ; I can see module.h three lines up in the
context.  Guessing your addition of the revert makes this addition no
longer required...

P.
--

>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/serial_8250.h>
> @@ -303,7 +304,7 @@ static struct platform_driver mtk8250_platform_driver = {
>  };
>  module_platform_driver(mtk8250_platform_driver);
>  
> -#ifdef CONFIG_SERIAL_8250_CONSOLE
> +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
>  static int __init early_mtk8250_setup(struct earlycon_device *device,
>  					const char *options)
>  {
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index 67ad6b0d595b..6ae0fae4f796 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -370,7 +370,7 @@ config SERIAL_8250_LPC18XX
>  	  serial port, say Y to this option. If unsure, say Y.
>  
>  config SERIAL_8250_MT6577
> -	bool "Mediatek serial port support"
> +	tristate "Mediatek serial port support"
>  	depends on SERIAL_8250 && ARCH_MEDIATEK
>  	help
>  	  If you have a Mediatek based board and want to use the
> -- 
> 2.7.0
> 

Re: [PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m

[Arnd Bergmann]

On Thursday 11 February 2016 11:04:38 you wrote:
> > diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> > index 6ecc6e3e82dc..55be90c1d85e 100644
> > --- a/drivers/tty/serial/8250/8250_mtk.c
> > +++ b/drivers/tty/serial/8250/8250_mtk.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/module.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/of_platform.h>
> > +#include <linux/module.h>
> 
> Something isn't right here ; I can see module.h three lines up in the
> context.  Guessing your addition of the revert makes this addition no
> longer required...
> 

Ok, thanks for noticing this. Also, I really should know my alphabet
at this point. I saw that the headers were sorted alphabetically and
thought I had added it in the right place, which would have worked fine
after the revert. Oh well.

	Arnd

Re: [PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m

[Matthias Brugger]

On 11/02/16 16:41, Arnd Bergmann wrote:
> The Mediatek 8250 driver has a 'bool' Kconfig symbol, but that
> breaks when SERIAL_8250 is a loadable module:
>
> drivers/tty/built-in.o: In function `mtk8250_set_termios':
> :(.text+0x1bee8): undefined reference to `serial8250_do_set_termios'
> :(.text+0x1bf10): undefined reference to `uart_get_baud_rate'
> :(.text+0x1c09c): undefined reference to `uart_get_divisor'
> drivers/tty/built-in.o: In function `mtk8250_do_pm':
> :(.text+0x1c0d0): undefined reference to `serial8250_do_pm'
> drivers/tty/built-in.o: In function `mtk8250_probe':
> :(.text+0x1c2e4): undefined reference to `serial8250_register_8250_port'
> serial/8250/8250_mtk.c:287:242: error: data definition has no type or storage class [-Werror]
> serial/8250/8250_mtk.c:287:122: error: 'mtk8250_platform_driver_init' defined but not used [-Werror=unused-function]
>
> This changes the symbol to a 'tristate', so the dependency on
> SERIAL_8250 also works when that is set to 'm'.
> To actually build the driver, we also need to include <linux/module.h>.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Acked-by: Matthias Brugger <matthias.bgg@gmail.com>

Re: [PATCH v2 5/6] serial: 8250/mediatek: fix building with SERIAL_8250=m

[Masahiro Yamada]

Hi Arnd,


2016-02-12 0:41 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> The Mediatek 8250 driver has a 'bool' Kconfig symbol, but that
> breaks when SERIAL_8250 is a loadable module:
>
> drivers/tty/built-in.o: In function `mtk8250_set_termios':
> :(.text+0x1bee8): undefined reference to `serial8250_do_set_termios'
> :(.text+0x1bf10): undefined reference to `uart_get_baud_rate'
> :(.text+0x1c09c): undefined reference to `uart_get_divisor'
> drivers/tty/built-in.o: In function `mtk8250_do_pm':
> :(.text+0x1c0d0): undefined reference to `serial8250_do_pm'
> drivers/tty/built-in.o: In function `mtk8250_probe':
> :(.text+0x1c2e4): undefined reference to `serial8250_register_8250_port'
> serial/8250/8250_mtk.c:287:242: error: data definition has no type or storage class [-Werror]
> serial/8250/8250_mtk.c:287:122: error: 'mtk8250_platform_driver_init' defined but not used [-Werror=unused-function]
>
> This changes the symbol to a 'tristate', so the dependency on
> SERIAL_8250 also works when that is set to 'm'.
> To actually build the driver, we also need to include <linux/module.h>.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/tty/serial/8250/8250_mtk.c | 3 ++-
>  drivers/tty/serial/8250/Kconfig    | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index 6ecc6e3e82dc..55be90c1d85e 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -19,6 +19,7 @@
>  #include <linux/module.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
> +#include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/serial_8250.h>
> @@ -303,7 +304,7 @@ static struct platform_driver mtk8250_platform_driver = {
>  };
>  module_platform_driver(mtk8250_platform_driver);
>
> -#ifdef CONFIG_SERIAL_8250_CONSOLE
> +#if defined(CONFIG_SERIAL_8250_CONSOLE) && !defined(MODULE)
>  static int __init early_mtk8250_setup(struct earlycon_device *device,
>                                         const char *options)
>  {


As mentioned in 4/6, this hunk might become unneeded if
https://patchwork.kernel.org/patch/8289231/
is accepted.



-- 
Best Regards
Masahiro Yamada

[PATCH v2 6/6] serial: 8250/ingenic: fix building with SERIAL_8250=m

[Arnd Bergmann]

The Ingenic 8250 driver has a 'bool' Kconfig symbol, but that
breaks when SERIAL_8250 is a loadable module:

drivers/tty/built-in.o: In function `ingenic_uart_probe':
8250_ingenic.c:(.text+0x1c1a0): undefined reference to `serial8250_register_8250_port'

This changes the symbol to a 'tristate', plus a dependency on
SERIAL_8250, which makes it work again. Unlike the other
soc-specific backends, this one has no dependency on an
architecture or a platform. I'm adding a dependency on
MIPS || COMPILE_TEST as well here, to avoid showing the driver
on architectures that are not interested in it.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/tty/serial/8250/8250_ingenic.c | 2 +-
 drivers/tty/serial/8250/Kconfig        | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
index 97b78558caed..b0677f610863 100644
--- a/drivers/tty/serial/8250/8250_ingenic.c
+++ b/drivers/tty/serial/8250/8250_ingenic.c
@@ -48,7 +48,7 @@ static const struct of_device_id of_match[];
 #define UART_MCR_MDCE	BIT(7)
 #define UART_MCR_FCM	BIT(6)
 
-#ifdef CONFIG_SERIAL_EARLYCON
+#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
 static struct earlycon_device *early_device;
 
 static uint8_t __init early_in(struct uart_port *port, int offset)
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 6ae0fae4f796..32df7d5a1733 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -384,9 +384,10 @@ config SERIAL_8250_UNIPHIER
 	  serial ports, say Y to this option. If unsure, say N.
 
 config SERIAL_8250_INGENIC
-	bool "Support for Ingenic SoC serial ports"
-	depends on OF_FLATTREE
-	select LIBFDT
+	tristate "Support for Ingenic SoC serial ports"
+	depends on SERIAL_8250
+	depends on (OF_FLATTREE && SERIAL_8250_CONSOLE) || !SERIAL_8250_CONSOLE
+	depends on MIPS || COMPILE_TEST
 	help
 	  If you have a system using an Ingenic SoC and wish to make use of
 	  its UARTs, say Y to this option. If unsure, say N.
-- 
2.7.0

Re: [PATCH v2 6/6] serial: 8250/ingenic: fix building with SERIAL_8250=m

[Masahiro Yamada]

Hi Arnd,


2016-02-12 0:41 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> The Ingenic 8250 driver has a 'bool' Kconfig symbol, but that
> breaks when SERIAL_8250 is a loadable module:
>
> drivers/tty/built-in.o: In function `ingenic_uart_probe':
> 8250_ingenic.c:(.text+0x1c1a0): undefined reference to `serial8250_register_8250_port'
>
> This changes the symbol to a 'tristate', plus a dependency on
> SERIAL_8250, which makes it work again. Unlike the other
> soc-specific backends, this one has no dependency on an
> architecture or a platform. I'm adding a dependency on
> MIPS || COMPILE_TEST as well here, to avoid showing the driver
> on architectures that are not interested in it.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/tty/serial/8250/8250_ingenic.c | 2 +-
>  drivers/tty/serial/8250/Kconfig        | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/8250/8250_ingenic.c b/drivers/tty/serial/8250/8250_ingenic.c
> index 97b78558caed..b0677f610863 100644
> --- a/drivers/tty/serial/8250/8250_ingenic.c
> +++ b/drivers/tty/serial/8250/8250_ingenic.c
> @@ -48,7 +48,7 @@ static const struct of_device_id of_match[];
>  #define UART_MCR_MDCE  BIT(7)
>  #define UART_MCR_FCM   BIT(6)
>
> -#ifdef CONFIG_SERIAL_EARLYCON
> +#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
>  static struct earlycon_device *early_device;
>
>  static uint8_t __init early_in(struct uart_port *port, int offset)


The same comment applies here as 5/6.





-- 
Best Regards
Masahiro Yamada