Re: [RFC 08/18] MIPS: ath79: add common watchdog device

[Gabor Juhos <juhosg@openwrt.org>]

Hi Sergei,

> On 13-11-2010 0:51, Gabor Juhos wrote:
> 
>> All supported SoCs have a built-in hardware watchdog driver. This patch
>> registers a platform_device for that to make it usable.
> 
>> Signed-off-by: Gabor Juhos<juhosg@openwrt.org>
>> Signed-off-by: Imre Kaloz<kaloz@openwrt.org>
> [...]
> 
>> diff --git a/arch/mips/ath79/Kconfig b/arch/mips/ath79/Kconfig
>> index 2bd35ef..79bb528 100644
>> --- a/arch/mips/ath79/Kconfig
>> +++ b/arch/mips/ath79/Kconfig
>> @@ -28,4 +28,7 @@ config ATH79_DEV_LEDS_GPIO
>>   config ATH79_DEV_UART
>>       def_bool y
>>
>> +config ATH79_DEV_WDT
>> +    def_bool y
> 
>    What's the point of introducing this?

My first thought was that it will be selectable by the board specific config
options. Because the watchdog timer is integrated into the SoC it will be
available on all boards anyway. I will remove the ATH79_DEV_UART and
ATH79_DEV_WDT config options and will change the Makefile accordingly.

>> <...>
>> +void __init ath79_register_wdt(void)
>> +{
>> +    platform_device_register(&ath79_wdt_device);
>> +}
> 
>    I'm not sure creating a separate file for the WDT platfrom device is really
> worth it...

You are right probably. Because it is always used, it can be moved into a common
file instead. I will do that.

>> <..>
>> +#ifndef _ATH_DEV_WDT_H
>> +#define _ATH_DEV_WDT_H
>> +
>> +void ath79_register_wdt(void) __init;
>> +
>> +#endif
> 
>    I think this should better be put into some more common header...

Yes, i will move this too.

>> <...>
>> @@ -259,6 +260,7 @@ static int __init ath79_setup(void)
>>   {
>>       ath79_gpio_init();
>>       ath79_register_uart();
>> +    ath79_register_wdt();
> 
>    Now what if CONFIG_ATH79_DEV_WDT is not enabled? You'll siply get a linker
> error. 

Correct.

> I think you should define an empty inline ath79_register_wdt() in that case.

This won't be needed after the changes proposed above.

Thank you,
Gabor