Re: [PATCH 1/3] rtc: stmp3xxx: add wdt-accessor function

[Andrew Morton <akpm@linux-foundation.org>]

On Mon, 23 Jan 2012 20:33:03 +0100
Wolfram Sang <w.sang@pengutronix.de> wrote:

> This RTC also includes a watchdog timer. Provide an accessor function
> for it to be picked up by a watchdog driver. Also register the
> platform_device here to get the boot-time dependencies right.
> 
>
> ...
>
>  struct stmp3xxx_rtc_data {
>  	struct rtc_device *rtc;
>  	void __iomem *io;
>  	int irq_alarm;
>  };
>  
> +#if defined(CONFIG_STMP3XXX_RTC_WATCHDOG) || defined(CONFIG_STMP3XXX_RTC_WATCHDOG_MODULE)

You can use IS_ENABLED() here.

> +/**
> + * stmp3xxx_wdt_set_timeout - configure the watchdog inside the STMP3xxx RTC
> + * @dev: the parent device of the watchdog (= the RTC)
> + * @timeout: the desired value for the timeout register of the watchdog.
> + *           0 disables the watchdog
> + *
> + * The watchdog needs one register and two bits which are in the RTC domain.
> + * To handle the resource conflict, the RTC driver will create another
> + * platform_device for the watchdog driver as a child of the RTC device.
> + * The watchdog driver is passed the below accessor function via platform_data
> + * to configure the watchdog. Locking is not needed because accessing SET/CLR
> + * registers is atomic.
> + */
> +
> +void stmp3xxx_wdt_set_timeout(struct device *dev, u32 timeout)
> +{
> +	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
> +	void __iomem *base;
> +
> +	if (timeout) {
> +		writel(timeout, rtc_data->io + STMP3XXX_RTC_WATCHDOG);
> +		base = rtc_data->io + MXS_SET_ADDR;
> +	} else {
> +		base = rtc_data->io + MXS_CLR_ADDR;
> +	}
> +	writel(STMP3XXX_RTC_CTRL_WATCHDOGEN, base + STMP3XXX_RTC_CTRL);
> +	writel(STMP3XXX_RTC_PERSISTENT1_FORCE_UPDATER,
> +		base + STMP3XXX_RTC_PERSISTENT1);
> +}

This wants EXPORT_SYMBOL(), surely?

Which points suspicious fingers at your testing ;) Please test all
combinations of y, n and m on both config symbols.

> +static struct stmp3xxx_wdt_pdata wdt_pdata = {
> +	.wdt_set_timeout = stmp3xxx_wdt_set_timeout,
> +};
> +
> +static struct platform_device stmp3xxx_wdt_pdev = {
> +	.name = "stmp3xxx_rtc_wdt",
> +	.dev = {
> +		.platform_data = &wdt_pdata,
> +	},
> +};
> +
> +static void stmp3xxx_wdt_register(struct platform_device *rtc_pdev)
> +{
> +	stmp3xxx_wdt_pdev.dev.parent = &rtc_pdev->dev;
> +	stmp3xxx_wdt_pdev.id = rtc_pdev->id;
> +	platform_device_register(&stmp3xxx_wdt_pdev);
> +}
> +#else
> +static void stmp3xxx_wdt_register(struct device *dev)
> +{
> +}

This stub is unfortunate.  If
!IS_ENABLED(CONFIG_STMP3XXX_RTC_WATCHDOG), the user still must load
this module just to get a silly empty function.  If the
stmp3xxx_wdt_register() stub were made static inline in a header file,
the user won't need to compile or load this module at all.  There may
be other (initialisation?) reasons why the user must still load this
module?