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

[Moritz Fischer]

Paul,

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?

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?

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

>> +#include <linux/module.h>
>
> Is that include needed?

Wouldn't that be needed if MODULE_DEVICE_TABLE etc, is 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_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?

>> +MODULE_LICENSE("GPL v2");
>
> You probably meant
>     MODULE_LICENSE("GPL");

Sorry about that. Will fix.
>
>> +MODULE_AUTHOR("Moritz Fischer <moritz.fischer@xxxxxxxxx>");
>> +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,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/