Re: [RFC][PATCH] watchdog: Convert jz4740_wdt driver to watchdog core

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3096 bytes --]

Hi Axel,

On Fri, Jan 20, 2012 at 09:44:14PM +0800, Axel Lin wrote:
> This patch converts jz4740_wdt driver to use watchdog core APIs.
> Also use devm_* APIs to save a few error handling code.
> 
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
> Hi Paul and Lars,
> I don't have this hardware handy.
> I'd appreciate if you can review and test this patch.
> 
> Thanks,
> Axel
> 
>  drivers/watchdog/Kconfig      |    1 +
>  drivers/watchdog/jz4740_wdt.c |  250 ++++++++++++++---------------------------
>  2 files changed, 86 insertions(+), 165 deletions(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 877b107..32d48b8 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -955,6 +955,7 @@ config INDYDOG
>  config JZ4740_WDT
>  	tristate "Ingenic jz4740 SoC hardware watchdog"
>  	depends on MACH_JZ4740
> +	select WATCHDOG_CORE
>  	help
>  	  Hardware driver for the built-in watchdog timer on Ingenic jz4740 SoCs.
>  
> diff --git a/drivers/watchdog/jz4740_wdt.c b/drivers/watchdog/jz4740_wdt.c
> index 17ef300..2cdb67f 100644
> --- a/drivers/watchdog/jz4740_wdt.c
> +++ b/drivers/watchdog/jz4740_wdt.c
> @@ -17,11 +17,9 @@
>  #include <linux/moduleparam.h>
>  #include <linux/types.h>
>  #include <linux/kernel.h>
> -#include <linux/fs.h>
>  #include <linux/miscdevice.h>
>  #include <linux/watchdog.h>
>  #include <linux/init.h>
> -#include <linux/bitops.h>
>  #include <linux/platform_device.h>
>  #include <linux/spinlock.h>
>  #include <linux/uaccess.h>

I'd think the latter two includes could go. Maybe even more?

>  
> -static void jz4740_wdt_set_heartbeat(int new_heartbeat)
> +static int jz4740_wdt_set_heartbeat(struct watchdog_device *wdt_dev,
> +				    unsigned int new_heartbeat)
>  {
> +	struct jz4740_wdt_drvdata *drvdata = watchdog_get_drvdata(wdt_dev);
>  	unsigned int rtc_clk_rate;
>  	unsigned int timeout_value;
>  	unsigned short clock_div = JZ_WDT_CLOCK_DIV_1;
>  
>  	heartbeat = new_heartbeat;

I'd think it would be better to drop 'heartbeat' usage in this function here,
but simply use 'new_heartbeat' and...

> -static int jz4740_wdt_open(struct inode *inode, struct file *file)
> +static int jz4740_wdt_start(struct watchdog_device *wdt_dev)
>  {
> -	if (test_and_set_bit(WDT_IN_USE, &jz4740_wdt.status))
> -		return -EBUSY;
> -
> -	jz4740_wdt_enable();
> +	jz4740_timer_enable_watchdog();
> +	jz4740_wdt_set_heartbeat(wdt_dev, heartbeat);

... use wdt_dev->timeout here (which would need to get initialized in probe
using the global 'heartbeat')


[...]

> +	drvdata->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (drvdata->mem == NULL) {
> +		dev_err(&pdev->dev, "failed to get memory region resource\n");
> +		return -ENXIO;

You don't need the pointer check here. devm_request_and_ioremap will do that, too.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]