Messages in this thread | | | Date | Tue, 31 Mar 2015 12:56:50 +0200 | From | Ralf Baechle <> | Subject | Re: [PATCH v2] MIPS: IP32: Add platform data hooks to use DS1685 driver |
| |
On Thu, Feb 26, 2015 at 09:23:50PM -0500, Joshua Kinard wrote:
> This modifies the IP32 (SGI O2) platform and reset code to utilize the new > rtc-ds1685 driver. The old mc146818rtc.h header is removed and ip32_defconfig > is updated as well.
In general - good cleanup. But:
> index 511e9ff..ec9eb7f 100644 > --- a/arch/mips/sgi-ip32/ip32-platform.c > +++ b/arch/mips/sgi-ip32/ip32-platform.c [...] > MODULE_AUTHOR("Ralf Baechle <ralf@linux-mips.org>"); > MODULE_LICENSE("GPL"); > -MODULE_DESCRIPTION("8250 UART probe driver for SGI IP32 aka O2"); > +MODULE_DESCRIPTION("IP32 platform setup for SGI IP32 aka O2");
This isn't even a kernel module so I've just nuked all these MODULE_* calls.
> diff --git a/arch/mips/sgi-ip32/ip32-reset.c b/arch/mips/sgi-ip32/ip32-reset.c > index 44b3470..ef21706 100644 > --- a/arch/mips/sgi-ip32/ip32-reset.c > +++ b/arch/mips/sgi-ip32/ip32-reset.c [...] > -static void ip32_machine_restart(char *cmd) > +static __noreturn void ip32_poweroff(void *data) > { > - crime->control = CRIME_CONTROL_HARD_RESET; > - while (1); > -} > + void (*poweroff_func)(struct platform_device *) = > + symbol_get(ds1685_rtc_poweroff); > + > +#ifdef CONFIG_MODULES > + /* If the first __symbol_get failed, our module wasn't loaded. */ > + if (!poweroff_func) { > + request_module("rtc-ds1685"); > + poweroff_func = symbol_get(ds1685_rtc_poweroff); > + } > +#endif
symbol_get() calls are high on my list of items that indicate a piece of code is probably ill-structured.
While RTCs often deal with power the RTC really only wants to deal with time and so power stuff should rather go elsewhere. I suggest to take a look at drivers/power/reset/. A small driver there could set pm_power_off approriately. drivers/power/reset/restart-poweroff.c is a very compact example.
> - shuting_down = 1; > + shutting_down = 1;
I'm amazed nobody of the church of speel patchology has caught this earlier.
> @@ -190,15 +141,12 @@ static __init int ip32_reboot_setup(void) > > _machine_restart = ip32_machine_restart; > _machine_halt = ip32_machine_halt; > - pm_power_off = ip32_machine_power_off; > + pm_power_off = ip32_machine_halt;
So halt and power_off no do the same?
Ralf
| |