Hi.
On Mon, Sep 22, 2008 at 03:45:57PM +0530, Madhusudhan Chikkature (madhu.cr@xxxxxx) 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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
