Re: [PATCH 1/2] power: reset: Add generic SYSCON register mapped poweroff.

[Paul Bolle <pebolle@tiscali.nl>]

Moritz,

On Wed, 2015-03-11 at 15:00 -0700, Moritz Fischer wrote:
> thanks for your feedback. While developing this I looked at other drivers in
> the tree and many of your comments would apply to them, too?

Could be. I didn't check. It's basically stuff that get's silently
handled by the preprocessor. So I wouldn't be surprised if a lot of
drivers do similar things.

> On Wed, Mar 11, 2015 at 2:31 AM, Paul Bolle <pebolle@tiscali
> 
> >> +config POWER_RESET_SYSCON_POWEROFF
> >> +     bool "Generic SYSCON regmap poweroff driver"
> >
> > This adds a bool symbol.
> 
> Is that an issue or is it just to substantiate the rest of your comments below?

It's just an observation.

> >> +obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o
> >
> > So this objectfile can never be part of a module.
> 
> Same comment would apply for power/reset/qnap-poweroff.c, right?

That could be. I didn't check it while looking at this patch.

> >> +#include <linux/module.h>
> >
> > Is that include needed?
> 
> Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is used?

Perhaps, but I don't think those macros are used.

> >> +MODULE_DEVICE_TABLE(of, syscon_poweroff_of_match);
> >
> > This will be preprocessed away.
> 
> In my experiments it didn't work without this, are you sure it's not needed?

MODULE_DEVICE_TABLE() is defined only in include/linux/module.h. The few
related lines read (in next-20150311):
    #ifdef MODULE
    /* Creates an alias so file2alias.c can find device table. */
    #define MODULE_DEVICE_TABLE(type, name)					\
    extern const typeof(name) __mod_##type##__##name##_device_table		\
      __attribute__ ((unused, alias(__stringify(name))))
    #else  /* !MODULE */
    #define MODULE_DEVICE_TABLE(type, name)
    #endif

So I think MODULE_DEVICE_TABLE() is always preprocessed away for boolean
code. Did I get that right? 

> >> +module_platform_driver(syscon_poweroff_driver);
> >
> > I think the built-in equivalent of this would be adding a wrapper that
> > only does
> >     platform_driver_register(&syscon_poweroff_driver);
> 
> I couldn't find other examples in the tree doing this. Any pointers?

Please note "think". But see drivers/cpufreq/dbx500-cpufreq.c:
    static int __init dbx500_cpufreq_register(void)
    {
            return platform_driver_register(&dbx500_cpufreq_plat_driver);
    }
    device_initcall(dbx500_cpufreq_register);

or drivers/gpio/gpio-vf610.c:
    static int __init gpio_vf610_init(void)
    {
            return platform_driver_register(&vf610_gpio_driver);
    }
    device_initcall(gpio_vf610_init);

(There are probably more.) Please note that both files use MODULE_*
macros but can actually only be built-in.

> >> +MODULE_LICENSE("GPL v2");
> >
> > You probably meant
> >     MODULE_LICENSE("GPL");
> 
> Sorry about that. Will fix.

That's a very easy mistake to make anyway. There's currently a thread
were (another) Dmitry and I discuss the merits of differentiating
between "GPL" and "GPL v2" in MODULE_LICENSE().

> >> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@ettus.com>");
> >> +MODULE_DESCRIPTION("Generic SYSCON poweroff driver");
> >> +MODULE_ALIAS("platform:syscon-poweroff");
> >
> > But these four macros will all be effectively preprocessed away, anyhow.
> 
> I'll remove them.

Thanks,


Paul Bolle