Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver

From: Krzysztof Kozlowski
Date: Thu Nov 21 2019 - 02:38:24 EST

Next message: Christoph Hellwig: "Re: [PATCH v3 0/4] dma-mapping: introduce new dma unmap and sync variants"
Previous message: Uwe Kleine-König: "Re: [PATCH v7 5/8] pwm: sun4i: Add support to output source clock directly"
In reply to: 구현기: "[PATCH] pinctrl: samsung: modularize samsung pinctrl driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

Thanks for the patch. Few comments below:

On Thu, 21 Nov 2019 at 15:26, êíê <hyunki00.koo@xxxxxxxxxxx> wrote:
>
> Enable samsung pinctrl driver to be compiled as modules.

Why? What's the benefit? Are platforms capable of such boot? Pinctrl
is needed early - even before mounting rootfs...
What about module unloading? Is it reasonable?
Please answer to all this also in commit message.

>
> Change-Id: I92a9953c92831a316f7f50146898ff19831549ec

This does not belong to Git.

> Signed-off-by: Hyunki Koo <hyunki00.koo@xxxxxxxxxxx>

You "From" name is different than written here in Signed-off-by. They
should match and I do not know Korean to be able to tell whether they
really match or not :).
How about using Latin transliteration also in "From" field?

> ---
> drivers/pinctrl/samsung/Kconfig | 5 +----
> drivers/pinctrl/samsung/Makefile | 13 +++++++------
> drivers/pinctrl/samsung/pinctrl-exynos-arm.c | 2 ++
> drivers/pinctrl/samsung/pinctrl-exynos-arm64.c | 2 ++
> drivers/pinctrl/samsung/pinctrl-exynos.c | 2 ++
> drivers/pinctrl/samsung/pinctrl-samsung.c | 13 +++++++++++++
> 6 files changed, 27 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/pinctrl/samsung/Kconfig
> b/drivers/pinctrl/samsung/Kconfig
> index 425fadd6c346..25e16984ef23 100644
> --- a/drivers/pinctrl/samsung/Kconfig
> +++ b/drivers/pinctrl/samsung/Kconfig
> @@ -3,14 +3,13 @@
> # Samsung Pin control drivers
> #
> config PINCTRL_SAMSUNG
> - bool
> + tristate "Pinctrl driver data for Samsung SoCs"
> select PINMUX
> select PINCONF
>
> config PINCTRL_EXYNOS
> bool "Pinctrl driver data for Samsung EXYNOS SoCs"
> depends on OF && GPIOLIB && (ARCH_EXYNOS || ARCH_S5PV210)
> - select PINCTRL_SAMSUNG
> select PINCTRL_EXYNOS_ARM if ARM && (ARCH_EXYNOS || ARCH_S5PV210)
> select PINCTRL_EXYNOS_ARM64 if ARM64 && ARCH_EXYNOS
>
> @@ -25,9 +24,7 @@ config PINCTRL_EXYNOS_ARM64
> config PINCTRL_S3C24XX
> bool "Samsung S3C24XX SoC pinctrl driver"
> depends on ARCH_S3C24XX && OF
> - select PINCTRL_SAMSUNG
>
> config PINCTRL_S3C64XX
> bool "Samsung S3C64XX SoC pinctrl driver"
> depends on ARCH_S3C64XX
> - select PINCTRL_SAMSUNG
> diff --git a/drivers/pinctrl/samsung/Makefile
> b/drivers/pinctrl/samsung/Makefile
> index ed951df6a112..b3ac01838b8a 100644
> --- a/drivers/pinctrl/samsung/Makefile
> +++ b/drivers/pinctrl/samsung/Makefile
> @@ -1,9 +1,10 @@
> # SPDX-License-Identifier: GPL-2.0
> # Samsung pin control drivers
>
> -obj-$(CONFIG_PINCTRL_SAMSUNG) += pinctrl-samsung.o
> -obj-$(CONFIG_PINCTRL_EXYNOS) += pinctrl-exynos.o
> -obj-$(CONFIG_PINCTRL_EXYNOS_ARM) += pinctrl-exynos-arm.o
> -obj-$(CONFIG_PINCTRL_EXYNOS_ARM64) += pinctrl-exynos-arm64.o
> -obj-$(CONFIG_PINCTRL_S3C24XX) += pinctrl-s3c24xx.o
> -obj-$(CONFIG_PINCTRL_S3C64XX) += pinctrl-s3c64xx.o
> +obj-$(CONFIG_PINCTRL_SAMSUNG) += pinctrl-samsung-super.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_SAMSUNG) += pinctrl-samsung.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_EXYNOS) += pinctrl-exynos.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_EXYNOS_ARM) += pinctrl-exynos-
> arm.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_EXYNOS_ARM64) += pinctrl-exynos-
> arm64.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_S3C24XX) += pinctrl-s3c24xx.o
> +pinctrl-samsung-super-$(CONFIG_PINCTRL_S3C64XX) += pinctrl-s3c64xx.o

I don't get why you need to rename obj to pinctrl-samsung-super?

> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
> b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
> index 85ddf49a5188..28906bf213c4 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm.c
> @@ -14,6 +14,7 @@
> // external gpio and wakeup interrupt support.
>
> #include <linux/device.h>
> +#include <linux/module.h>
> #include <linux/of_address.h>
> #include <linux/slab.h>
> #include <linux/err.h>
> @@ -891,3 +892,4 @@ const struct samsung_pinctrl_of_match_data
> exynos5420_of_data __initconst = {
> .ctrl = exynos5420_pin_ctrl,
> .num_ctrl = ARRAY_SIZE(exynos5420_pin_ctrl),
> };
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> index b6e56422a700..2b19476ad5ff 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos-arm64.c
> @@ -14,6 +14,7 @@
> // external gpio and wakeup interrupt support.
>
> #include <linux/slab.h>
> +#include <linux/module.h>
> #include <linux/soc/samsung/exynos-regs-pmu.h>
>
> #include "pinctrl-samsung.h"
> @@ -422,3 +423,4 @@ const struct samsung_pinctrl_of_match_data
> exynos7_of_data __initconst = {
> .ctrl = exynos7_pin_ctrl,
> .num_ctrl = ARRAY_SIZE(exynos7_pin_ctrl),
> };
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pinctrl/samsung/pinctrl-exynos.c
> b/drivers/pinctrl/samsung/pinctrl-exynos.c
> index ebc27b06718c..630d606330f1 100644
> --- a/drivers/pinctrl/samsung/pinctrl-exynos.c
> +++ b/drivers/pinctrl/samsung/pinctrl-exynos.c
> @@ -18,6 +18,7 @@
> #include <linux/irqdomain.h>
> #include <linux/irq.h>
> #include <linux/irqchip/chained_irq.h>
> +#include <linux/module.h>
> #include <linux/of.h>
> #include <linux/of_irq.h>
> #include <linux/slab.h>
> @@ -713,3 +714,4 @@ exynos_retention_init(struct samsung_pinctrl_drv_data
> *drvdata,
>
> return ctrl;
> }
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/pinctrl/samsung/pinctrl-samsung.c
> b/drivers/pinctrl/samsung/pinctrl-samsung.c
> index de0477bb469d..4483eaed27f8 100644
> --- a/drivers/pinctrl/samsung/pinctrl-samsung.c
> +++ b/drivers/pinctrl/samsung/pinctrl-samsung.c
> @@ -15,6 +15,7 @@
> // but provides extensions to which platform specific implementation of
> the gpio
> // and wakeup interrupts can be hooked to.
>
> +#include <linux/module.h>
> #include <linux/init.h>
> #include <linux/platform_device.h>
> #include <linux/io.h>
> @@ -1275,6 +1276,7 @@ static const struct of_device_id
> samsung_pinctrl_dt_match[] = {
> #endif
> {},
> };
> +MODULE_DEVICE_TABLE(of, samsung_pinctrl_dt_match);
>
> static const struct dev_pm_ops samsung_pinctrl_pm_ops = {
> SET_LATE_SYSTEM_SLEEP_PM_OPS(samsung_pinctrl_suspend,
> @@ -1296,3 +1298,14 @@ static int __init samsung_pinctrl_drv_register(void)
> return platform_driver_register(&samsung_pinctrl_driver);
> }
> postcore_initcall(samsung_pinctrl_drv_register);
> +
> +static void __exit samsung_pinctrl_drv_unregister(void)
> +{
> + platform_driver_unregister(&samsung_pinctrl_driver);
> +}
> +module_exit(samsung_pinctrl_drv_unregister);

Since .suppress_bind_attrs are defined, I find it weird to be able to
unload module... Another warning sign...

> +
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Hyunki Koo <hyunki00.koo@xxxxxxxxxxx>");

I cannot find any contributions to this file from you. The author
should be the main contributor(s). You need to go through history...

> +MODULE_DESCRIPTION("Samsung Exynos PINCTRL driver");

That's not Exynos but Samsung-generic part... Samsung Exynos/S3C/S5P
pinctrl driver.

Best regards,
Krzysztof

Next message: Christoph Hellwig: "Re: [PATCH v3 0/4] dma-mapping: introduce new dma unmap and sync variants"
Previous message: Uwe Kleine-König: "Re: [PATCH v7 5/8] pwm: sun4i: Add support to output source clock directly"
In reply to: 구현기: "[PATCH] pinctrl: samsung: modularize samsung pinctrl driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]