Hi Evgeniy Polyakov, Thanks for the comments. I will incorporate them and send the patch again. My comments inlined.
Regards, Madhu ----- Original Message ----- From: "Evgeniy Polyakov" <[EMAIL PROTECTED]> To: "Madhusudhan Chikkature" <[EMAIL PROTECTED]> Cc: <[EMAIL PROTECTED]>; <[email protected]> Sent: Monday, September 22, 2008 6:36 PM Subject: Re: [PATCH]OMAP HDQ driver ioremap changes > Hi. > > On Mon, Sep 22, 2008 at 03:45:57PM +0530, Madhusudhan Chikkature ([EMAIL > PROTECTED]) 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? Yes. RMK had given the same comment. I am working on it now. I will submit that as a second patch to use mutex instead of semaphores. > >> @@ -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? I will add a check for that. > >> /* 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? Yes. I will make those changes as well. > > -- > Evgeniy Polyakov > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
