Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular
On Fri, Dec 21, 2018 at 10:55:16AM -0500, Paul Gortmaker wrote:
> [Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular] On
> 19/12/2018 (Wed 09:17) Charles Keepax wrote:
>
> > On Mon, Dec 17, 2018 at 03:31:28PM -0500, Paul Gortmaker wrote:
> > > -MODULE_DEVICE_TABLE(i2c, wm8400_i2c_id);
> > >
> > > static struct i2c_driver wm8400_i2c_driver = {
> > > .driver = {
> > > @@ -161,7 +160,7 @@ static struct i2c_driver wm8400_i2c_driver = {
> > > };
> > > #endif
> >
> > Do we not want to add suppress_bind_attrs into the i2c_driver
> > struct here?
>
> We can add one if you/maintainers want one, but if you look at the
> original patch, this driver was using the more classic/legacy case of
> subsys_init() vs. platform_driver_register() used in other drivers.
>
> Not adding a suppress_bind_attrs here was intentional, since I'd decided
> to put in the unbind entries for code that used platform_driver_register()
> where the author had created the .remove code, on the assumption that they
> had put some thought into the process of unbind/remove - to make it
> explicit that unbind is now disabled.
>
> To be clear, using the subsys_init() doesn't implicitly disable unbind.
> However, there are lots of non-modular drivers out there; ones I've not
> even touched, and to start a project to add an unbind disable to them
> all is beyond the scope of the goals I've listed in the 00/18 preamble.
>
> I'd hope maybe we can revisit the global default setting for non-modular
> code someday - to make non-modules opt-in instead of opt-out, and
> achieve better consistency from one driver to the next, without having
> to add a new .driver sub-struct to each file for the suppress entry.
>
> I think LinusW hinted at in an earlier email in this ongoing review,
> that the default setting didn't quite make sense to him either. But in
> any case, that is a separate discussion for another time and place.
>
> Let me know if you explicitly want one added, otherwise I'll just leave
> the .remove + .suppress_bind_attrs pairing as described above.
>
Nah its ok, if you are specifically not doing this one I think I
am ok with it. Just seemed out of place compared to the the
others:
Acked-by: Charles Keepax
Thanks,
Charles
Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular
[Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular] On
19/12/2018 (Wed 09:17) Charles Keepax wrote:
> On Mon, Dec 17, 2018 at 03:31:28PM -0500, Paul Gortmaker wrote:
> > The Kconfig currently controlling compilation of this code is:
> >
> > drivers/mfd/Kconfig:config MFD_WM8400
> > drivers/mfd/Kconfig:bool "Wolfson Microelectronics WM8400"
> >
> > ...meaning that it currently is not being built as a module by anyone.
> >
> > Lets remove the modular code that is essentially orphaned, so that
> > when reading the driver there is no doubt it is builtin-only.
> >
> > Since module_init was not in use by this code, the init ordering
> > remains unchanged with this commit.
> >
> > A trivial function rename from wm8400_module_init to the name
> > wm8400_driver_init is also done to reduce possible confusion.
> >
> > Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
> >
> > We also delete the MODULE_LICENSE tag etc. since all that information
> > is already contained at the top of the file in the comments.
> >
> > Cc: Lee Jones
> > Cc: Mark Brown
> > Cc: patc...@opensource.cirrus.com
> > Signed-off-by: Paul Gortmaker
> > Acked-by: Linus Walleij
> > ---
> > -MODULE_DEVICE_TABLE(i2c, wm8400_i2c_id);
> >
> > static struct i2c_driver wm8400_i2c_driver = {
> > .driver = {
> > @@ -161,7 +160,7 @@ static struct i2c_driver wm8400_i2c_driver = {
> > };
> > #endif
>
> Do we not want to add suppress_bind_attrs into the i2c_driver
> struct here?
We can add one if you/maintainers want one, but if you look at the
original patch, this driver was using the more classic/legacy case of
subsys_init() vs. platform_driver_register() used in other drivers.
Not adding a suppress_bind_attrs here was intentional, since I'd decided
to put in the unbind entries for code that used platform_driver_register()
where the author had created the .remove code, on the assumption that they
had put some thought into the process of unbind/remove - to make it
explicit that unbind is now disabled.
To be clear, using the subsys_init() doesn't implicitly disable unbind.
However, there are lots of non-modular drivers out there; ones I've not
even touched, and to start a project to add an unbind disable to them
all is beyond the scope of the goals I've listed in the 00/18 preamble.
I'd hope maybe we can revisit the global default setting for non-modular
code someday - to make non-modules opt-in instead of opt-out, and
achieve better consistency from one driver to the next, without having
to add a new .driver sub-struct to each file for the suppress entry.
I think LinusW hinted at in an earlier email in this ongoing review,
that the default setting didn't quite make sense to him either. But in
any case, that is a separate discussion for another time and place.
Let me know if you explicitly want one added, otherwise I'll just leave
the .remove + .suppress_bind_attrs pairing as described above.
Thanks,
Paul.
--
>
> Thanks, Charles
Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular
On Mon, Dec 17, 2018 at 03:31:28PM -0500, Paul Gortmaker wrote:
> The Kconfig currently controlling compilation of this code is:
>
> drivers/mfd/Kconfig:config MFD_WM8400
> drivers/mfd/Kconfig:bool "Wolfson Microelectronics WM8400"
>
> ...meaning that it currently is not being built as a module by anyone.
>
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.
>
> Since module_init was not in use by this code, the init ordering
> remains unchanged with this commit.
>
> A trivial function rename from wm8400_module_init to the name
> wm8400_driver_init is also done to reduce possible confusion.
>
> Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
>
> We also delete the MODULE_LICENSE tag etc. since all that information
> is already contained at the top of the file in the comments.
>
> Cc: Lee Jones
> Cc: Mark Brown
> Cc: patc...@opensource.cirrus.com
> Signed-off-by: Paul Gortmaker
> Acked-by: Linus Walleij
> ---
> -MODULE_DEVICE_TABLE(i2c, wm8400_i2c_id);
>
> static struct i2c_driver wm8400_i2c_driver = {
> .driver = {
> @@ -161,7 +160,7 @@ static struct i2c_driver wm8400_i2c_driver = {
> };
> #endif
Do we not want to add suppress_bind_attrs into the i2c_driver
struct here?
Thanks,
Charles
Re: [PATCH 18/18] mfd: wm8400-core: Make it explicitly non-modular
On Fri, Dec 07, 2018 at 03:11:05PM -0500, Paul Gortmaker wrote: > The Kconfig currently controlling compilation of this code is: > > drivers/mfd/Kconfig:config MFD_WM8400 > drivers/mfd/Kconfig:bool "Wolfson Microelectronics WM8400" > > ...meaning that it currently is not being built as a module by anyone. > > Lets remove the modular code that is essentially orphaned, so that > when reading the driver there is no doubt it is builtin-only. > > Since module_init was not in use by this code, the init ordering > remains unchanged with this commit. > > A trivial function rename from wm8400_module_init to the name > wm8400_driver_init is also done to reduce possible confusion. > > Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code. > > We also delete the MODULE_LICENSE tag etc. since all that information > is already contained at the top of the file in the comments. > > Cc: Lee Jones > Cc: Mark Brown > Cc: patc...@opensource.cirrus.com > Acked-by: Linus Walleij > Signed-off-by: Paul Gortmaker > --- Acked-by: Charles Keepax Thanks, Charles
