Re: [PATCH]OMAP HDQ driver ioremap changes

[Evgeniy Polyakov <johnpol@2ka.mipt.ru>]

Hi.

On Mon, Sep 22, 2008 at 03:45:57PM +0530, Madhusudhan Chikkature (madhu.cr@ti.com) wrote:
> This patch provides the HDQ driver modifications to use ioremap for the base
> address.

Looks good.
Couple of small comments inline.

> --- linux-omap-2.6.orig/drivers/w1/masters/omap_hdq.c	2008-08-18
> 14:48:26.000000000 +0530
> +++ linux-omap-2.6/drivers/w1/masters/omap_hdq.c	2008-09-22 14:56:28.000000000
> +0530
> @@ -53,7 +53,7 @@ DECLARE_WAIT_QUEUE_HEAD(hdq_wait_queue);
>  int W1_ID;
> 
>  struct hdq_data {
> -	resource_size_t		hdq_base;
> +	void __iomem		*hdq_base;
>  	struct	semaphore	hdq_semlock;

Shouldn't it use mutex or it does counting?

> @@ -577,7 +577,7 @@ static int __init omap_hdq_probe(struct
>  		return -ENXIO;
>  	}
> 
> -	hdq_data->hdq_base = res->start;
> +	hdq_data->hdq_base = ioremap(res->start, SZ_4K);

Suppose it does not fail on this arch?

>  	/* get interface & functional clock objects */
>  	hdq_data->hdq_ick = clk_get(&pdev->dev, "hdq_ick");
> @@ -588,12 +588,14 @@ static int __init omap_hdq_probe(struct
>  		if (IS_ERR(hdq_data->hdq_ick)) {
>  			ret = PTR_ERR(hdq_data->hdq_ick);
>  			platform_set_drvdata(pdev, NULL);
> +			iounmap(hdq_data->hdq_base);
>  			kfree(hdq_data);
>  			return ret;
>  		}
>  		if (IS_ERR(hdq_data->hdq_fck)) {
>  			ret = PTR_ERR(hdq_data->hdq_fck);
>  			platform_set_drvdata(pdev, NULL);
> +			iounmap(hdq_data->hdq_base);
>  			kfree(hdq_data);
>  			return ret;
>  		}

Don't you want to use goto and single exit path here and in other places?

-- 
	Evgeniy Polyakov