pinctrl: samsung: modularize samsung pinctrl driver
diff mbox series

Message ID 001001d5a03d$05de1f70$119a5e50$@samsung.com
State Rejected
Headers show
Series
  • pinctrl: samsung: modularize samsung pinctrl driver
Related show

Commit Message

Hyunki Koo Nov. 21, 2019, 7:26 a.m. UTC
Enable samsung pinctrl driver to be compiled as modules.

Change-Id: I92a9953c92831a316f7f50146898ff19831549ec
Signed-off-by: Hyunki Koo <hyunki00.koo@samsung.com>
---
 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(-)

 #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);
+
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Hyunki Koo <hyunki00.koo@samsung.com>");
+MODULE_DESCRIPTION("Samsung Exynos PINCTRL driver");

Comments

Krzysztof Kozlowski Nov. 21, 2019, 7:38 a.m. UTC | #1
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
Hyunki Koo Nov. 26, 2019, 1:14 a.m. UTC | #2
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
Krzysztof Kozlowski Nov. 26, 2019, 6:09 a.m. UTC | #3
On Tue, 26 Nov 2019 at 09:14, Hyunki Koo <hyunki00.koo@samsung.com> wrote:
>
> 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.

This is driver for Exynos and S3C so why cutting off the dependency? I
mean, wait, it is a driver for Exynos so it cannot work on other
boards...

> 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.

Then you are doing it in wrong order - this patch makes it as a module
but you send it as first one.

You did not resolve however my questions about:
1. Will this even work? Will platforms boot?
2. How are you going to handle module unloading since on purpose we
added suppress_bind?

>
> >>
> >> 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

Might be a stupid question but what is "Super module"?

Second, the dependencies are there because this is a integral part of them.

I think I miss some bigger picture here because I still do not
understand what you want to achieve in the end (why doing this)?

Best regards,
Krzysztof
Krzysztof Kozlowski Nov. 27, 2019, 1:58 a.m. UTC | #4
On Tue, 26 Nov 2019 at 14:09, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Tue, 26 Nov 2019 at 09:14, Hyunki Koo <hyunki00.koo@samsung.com> wrote:
> >
> > 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.
>
> This is driver for Exynos and S3C so why cutting off the dependency? I
> mean, wait, it is a driver for Exynos so it cannot work on other
> boards...

One more thought about Generic Kernel Image for Android. It implies
that vendor stuff will be in modules... but only for out-of-tree code.
The Exynos pinctrl driver is already in mainline so moving it into
module is not necessary.

As for dependency on ARCH_EXYNOS - I do not get why this is a problem.
ARCH_EXYNOS is multiplatform ready so you can boot multi_v7... What
would be the benefit of removing this dependency?

Best regards,
Krzysztof
Linus Walleij Nov. 27, 2019, 7:58 a.m. UTC | #5
On Tue, Nov 26, 2019 at 2:14 AM Hyunki Koo <hyunki00.koo@samsung.com> wrote:

> I would like to apply GKI on the pinctrl driver
> So I would like to cut off dependency from ARCH_EXYNOS.

If the pins need to be configured for the system to boot that needs
to be in the big kernel image anyway. And then it should be bool.

Example: to mount the root filesystem on external flash pin
configuration needs to happen.

There is a slight paradigm shift sometimes: in distribution
kernels it might be required that necessary modules are
included in an initramfs attached to the kernel. I do not know
if we want to encourage that type of configuration for embedded
ARM.

Yours,
Linus Walleij

Patch
diff mbox series

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
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>