Re: [PATCH 2/2] watchdog:OMAP3:Enable support for IVA2 and SECURE

[Kevin Hilman <khilman@deeprootsystems.com>]

Ulrik Bech Hald <ubh@ti.com> writes:

> This patch enables the IVA2 and SECURE WDTs in the omap_wdt
> driver for omap34xx family. SECURE will be available as
> /dev/watchdog_secure on HS/EMU devices and IVA2 will be available
> as /dev/watchdog_iva2. MPU will still be available as /dev/watchdog
>
> Signed-off-by: Ulrik Bech Hald <ubh@ti.com>
> ---
> This patch has a dependency on:
> [PATCH 1/1] watchdog: OMAP fixes: enable clock in probe, trigger timer reload
>
>  drivers/watchdog/omap_wdt.c |   51 ++++++++++++++++++++++++++++++++++++------
>  1 files changed, 43 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index f271385..85a92de 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -47,7 +47,15 @@
>  
>  #include "omap_wdt.h"
>  
> -static struct platform_device *omap_wdt_dev;
> +#define NUM_WDTS	3
> +
> +enum {
> +	SECURE_WDT = 1,
> +	MPU_WDT,
> +	IVA2_WDT,
> +};
> +
> +static struct platform_device *omap_wdt_dev[NUM_WDTS];

If you're going to have shared numbers here, they should be shared and
also used by the SoC init code.  IOW, the id's you're using in the
devices.c file affects the numbering here.

Also, why not use zero-based numbering here and in devices.c, then
you can avoide the 'pdev->id - 1' below.

>  static unsigned timer_margin;
>  module_param(timer_margin, uint, 0);
> @@ -139,8 +147,23 @@ static void omap_wdt_set_timeout(struct omap_wdt_dev *wdev)
>   */
>  static int omap_wdt_open(struct inode *inode, struct file *file)
>  {
> -	struct omap_wdt_dev *wdev = platform_get_drvdata(omap_wdt_dev);
> -	void __iomem *base = wdev->base;
> +	struct omap_wdt_dev *wdev;
> +	void __iomem *base;
> +
> +	/* by default MPU wdt is present across omap family */
> +	wdev = platform_get_drvdata(omap_wdt_dev[1]);

Drop this and just use the inode match.

> +	/* Find match between device node and wdt device */
> +	int i;
> +	for (i = 0; i < NUM_WDTS; i++) {
> +		if (omap_wdt_dev[i]) {
> +			wdev = platform_get_drvdata(omap_wdt_dev[i]);
> +			if (iminor(inode) == wdev->omap_wdt_miscdev.minor)
> +				break;
> +		}
> +	}

You should check for a valid match here.

> +	base = wdev->base;
>  
>  	if (test_and_set_bit(1, (unsigned long *)&(wdev->omap_wdt_users)))
>  		return -EBUSY;
> @@ -271,7 +294,7 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
>  		goto err_get_resource;
>  	}
>  
> -	if (omap_wdt_dev) {
> +	if (omap_wdt_dev[pdev->id-1]) {

With zero-based numbering, you can drop the -1.

>  		ret = -EBUSY;
>  		goto err_busy;
>  	}
> @@ -317,10 +340,21 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
>  	omap_wdt_adjust_timeout(timer_margin);
>  
>  	wdev->omap_wdt_miscdev.parent = &pdev->dev;
> -	wdev->omap_wdt_miscdev.minor = WATCHDOG_MINOR;
> -	wdev->omap_wdt_miscdev.name = "watchdog";
> +	wdev->omap_wdt_miscdev.minor = MISC_DYNAMIC_MINOR;
>  	wdev->omap_wdt_miscdev.fops = &omap_wdt_fops;
>  
> +	switch (pdev->id) {
> +	case SECURE_WDT:
> +		wdev->omap_wdt_miscdev.name = "watchdog_secure";
> +		break;
> +	case IVA2_WDT:
> +		wdev->omap_wdt_miscdev.name = "watchdog_iva2";
> +		break;
> +	case MPU_WDT:
> +	default:
> +		wdev->omap_wdt_miscdev.name = "watchdog";
> +	}
> +

The more think about this, the more I don't like this pdev->id
switching in the driver.  The only thing it is needed for
is to set the name of the node.

Instead, why not set the name in devices.c and pass it in
using platform_data.

Then you can drop the enum and the pdev->id switching.

>  	ret = misc_register(&(wdev->omap_wdt_miscdev));
>  	if (ret)
>  		goto err_misc;
> @@ -332,7 +366,8 @@ static int __devinit omap_wdt_probe(struct platform_device *pdev)
>  	/* autogate OCP interface clock */
>  	__raw_writel(0x01, wdev->base + OMAP_WATCHDOG_SYS_CONFIG);
>  
> -	omap_wdt_dev = pdev;
> +	/* keep track of the wdt platform devices in local device array */
> +	omap_wdt_dev[pdev->id - 1] = pdev;
>  
>  	return 0;
>  
> @@ -384,7 +419,7 @@ static int __devexit omap_wdt_remove(struct platform_device *pdev)
>  	iounmap(wdev->base);
>  
>  	kfree(wdev);
> -	omap_wdt_dev = NULL;
> +	omap_wdt_dev[pdev->id-1] = NULL;
>  
>  	return 0;
>  }
> -- 
> 1.5.4.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html