[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-rtc
Subject:    Re: [PATCH v2] MIPS: IP32: Add platform data hooks to use DS1685 driver
From:       Ralf Baechle <ralf () linux-mips ! org>
Date:       2015-03-31 10:56:50
Message-ID: 20150331105650.GA28951 () linux-mips ! org
[Download RAW message or body]

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

-- 
-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" \
group. To unsubscribe from this group and stop receiving emails from it, send an \
email to rtc-linux+unsubscribe@googlegroups.com. For more options, visit \
https://groups.google.com/d/optout.


[prev in list] [next in list] [prev in thread] [next in thread]