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@xxxxxxxxx>
> ---
> 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:
signature.asc
Description: Digital signature
