* [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
[not found] <CGME20191121072643epcas2p452071a503725c7764acf5084d24425b1@epcas2p4.samsung.com>
@ 2019-11-21 7:26 ` 구현기
2019-11-21 7:38 ` Krzysztof Kozlowski
0 siblings, 1 reply; 6+ messages in thread
From: 구현기 @ 2019-11-21 7:26 UTC (permalink / raw)
To: tomasz.figa, krzk, s.nawrocki, linus.walleij
Cc: linux-arm-kernel, linux-samsung-soc, linux-gpio, linux-kernel
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(-)
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>
#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");
--
2.15.0.rc1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
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
0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2019-11-21 7:38 UTC (permalink / raw)
To: 구현기
Cc: Tomasz Figa, s.nawrocki, linus.walleij, linux-arm-kernel,
linux-samsung-soc, linux-gpio, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
2019-11-21 7:38 ` Krzysztof Kozlowski
@ 2019-11-26 1:14 ` Hyunki Koo
2019-11-26 6:09 ` Krzysztof Kozlowski
2019-11-27 7:58 ` Linus Walleij
0 siblings, 2 replies; 6+ messages in thread
From: Hyunki Koo @ 2019-11-26 1:14 UTC (permalink / raw)
To: 'Krzysztof Kozlowski'
Cc: 'Tomasz Figa',
s.nawrocki, linus.walleij, linux-arm-kernel, linux-samsung-soc,
linux-gpio, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
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
1 sibling, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2019-11-26 6:09 UTC (permalink / raw)
To: Hyunki Koo
Cc: Tomasz Figa, s.nawrocki, linus.walleij, linux-arm-kernel,
linux-samsung-soc, linux-gpio, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
2019-11-26 6:09 ` Krzysztof Kozlowski
@ 2019-11-27 1:58 ` Krzysztof Kozlowski
0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2019-11-27 1:58 UTC (permalink / raw)
To: Hyunki Koo
Cc: Tomasz Figa, s.nawrocki, linus.walleij, linux-arm-kernel,
linux-samsung-soc, linux-gpio, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver
2019-11-26 1:14 ` Hyunki Koo
2019-11-26 6:09 ` Krzysztof Kozlowski
@ 2019-11-27 7:58 ` Linus Walleij
1 sibling, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2019-11-27 7:58 UTC (permalink / raw)
To: Hyunki Koo
Cc: Krzysztof Kozlowski, Tomasz Figa, Sylwester Nawrocki, Linux ARM,
linux-samsung-soc, open list:GPIO SUBSYSTEM, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-27 7:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[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
2019-11-26 6:09 ` Krzysztof Kozlowski
2019-11-27 1:58 ` Krzysztof Kozlowski
2019-11-27 7:58 ` Linus Walleij
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).