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

[Hyunki Koo]

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