From: "Hyunki Koo" <hyunki00.koo@samsung.com>
To: "'Krzysztof Kozlowski'" <krzk@kernel.org>
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-gpio@vger.kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
Date: Tue, 26 Nov 2019 10:14:42 +0900 [thread overview]
Message-ID: <028901d5a3f6$e2d72310$a8856930$@samsung.com> (raw)
In-Reply-To: <CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com>
On Thu, 21 Nov 2019 at 16:38, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 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.
>
Sorry to late and Thank you for your comment, I would like to apply GKI on the pinctrl driver
So I would like to cut off dependency from ARCH_EXYNOS.
I will split into two part,
First, cut off the dependency with ARCH_EXYNOS in arch/arm64/Kconfig.platform.
Second, I will make the pinctrl driver as a module.
>>
>> 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?
I would like to make pinctrl as a super module. It is many dependency between pinctrl-exynos, pinctr-exynos-arm64 and pinctrl-samsung
>
>> 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...
Okay I will apply your comment
>
>> +
>> +
>> +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.
>
Okay I will apply your comment
next prev parent reply other threads:[~2019-11-26 1:14 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
2019-11-26 1:14 ` Hyunki Koo [this message]
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='028901d5a3f6$e2d72310$a8856930$@samsung.com' \
--to=hyunki00.koo@samsung.com \
--cc=krzk@kernel.org \
--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).