On Thu, 21 Nov 2019 at 16:38, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote:
> 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.
>
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@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?
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@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.
>
Okay I will apply your comment
