From: Krzysztof Kozlowski <krzk@kernel.org>
To: 구현기 <hyunki00.koo@samsung.com>
Cc: Tomasz Figa <tomasz.figa@gmail.com>,
s.nawrocki@samsung.com, linus.walleij@linaro.org,
linux-arm-kernel@lists.infradead.org,
"linux-samsung-soc@vger.kernel.org"
<linux-samsung-soc@vger.kernel.org>,
linux-gpio@vger.kernel.org,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
Date: Thu, 21 Nov 2019 15:38:08 +0800 [thread overview]
Message-ID: <CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com> (raw)
In-Reply-To: <001001d5a03d$05de1f70$119a5e50$@samsung.com>
Hi,
Thanks for the patch. Few comments below:
On Thu, 21 Nov 2019 at 15:26, 구현기 <hyunki00.koo@samsung.com> 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@samsung.com>
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@samsung.com>");
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 prev parent reply other threads:[~2019-11-21 7:38 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20191121072643epcas2p452071a503725c7764acf5084d24425b1@epcas2p4.samsung.com>
2019-11-21 7:26 ` [PATCH] pinctrl: samsung: modularize samsung pinctrl driver 구현기
2019-11-21 7:38 ` Krzysztof Kozlowski [this message]
2019-11-26 1:14 ` Hyunki Koo
2019-11-26 6:09 ` Krzysztof Kozlowski
2019-11-27 1:58 ` Krzysztof Kozlowski
2019-11-27 7:58 ` Linus Walleij
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com \
--to=krzk@kernel.org \
--cc=hyunki00.koo@samsung.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=s.nawrocki@samsung.com \
--cc=tomasz.figa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).