From: Krzysztof Kozlowski <krzk@kernel.org> To: 구현기 <hyunki00.koo@samsung.com> 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-samsung-soc@vger.kernel.org>, linux-gpio@vger.kernel.org, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver Date: Thu, 21 Nov 2019 15:38:08 +0800 [thread overview] Message-ID: <CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com> (raw) In-Reply-To: <001001d5a03d$05de1f70$119a5e50$@samsung.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzk@kernel.org> To: 구현기 <hyunki00.koo@samsung.com> Cc: "linux-samsung-soc@vger.kernel.org" <linux-samsung-soc@vger.kernel.org>, linus.walleij@linaro.org, Tomasz Figa <tomasz.figa@gmail.com>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, linux-gpio@vger.kernel.org, s.nawrocki@samsung.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] pinctrl: samsung: modularize samsung pinctrl driver Date: Thu, 21 Nov 2019 15:38:08 +0800 [thread overview] Message-ID: <CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com> (raw) In-Reply-To: <001001d5a03d$05de1f70$119a5e50$@samsung.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-21 7:38 UTC|newest] Thread overview: 12+ 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:26 ` 구현기 2019-11-21 7:38 ` Krzysztof Kozlowski [this message] 2019-11-21 7:38 ` Krzysztof Kozlowski 2019-11-26 1:14 ` Hyunki Koo 2019-11-26 1:14 ` Hyunki Koo 2019-11-26 6:09 ` Krzysztof Kozlowski 2019-11-26 6:09 ` Krzysztof Kozlowski 2019-11-27 1:58 ` Krzysztof Kozlowski 2019-11-27 1:58 ` Krzysztof Kozlowski 2019-11-27 7:58 ` Linus Walleij 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=CAJKOXPckbRowhCmnJfT8-DT3gYaTpDOf0wVxmxdf-tZpOyM5ew@mail.gmail.com \ --to=krzk@kernel.org \ --cc=hyunki00.koo@samsung.com \ --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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.