From: Stefan Roese <sr@denx.de>
To: dedekind1@gmail.com
Cc: Shiraz Hashim <shiraz.hashim@st.com>,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
Julia Lawall <Julia.Lawall@lip6.fr>,
linux-mtd@lists.infradead.org,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently
Date: Fri, 24 Aug 2012 13:14:32 +0000 [thread overview]
Message-ID: <50377E38.1040701@denx.de> (raw)
In-Reply-To: <1345808104.2848.285.camel@sauron.fi.intel.com>
On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
>> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
>> ret = mtd_device_unregister(&flash->mtd);
>> if (ret)
>> dev_err(&pdev->dev, "error removing mtd\n");
>> -
>> - iounmap(flash->base_addr);
>> - kfree(flash);
>> }
>>
>> irq = platform_get_irq(pdev, 0);
>> - free_irq(irq, dev);
>
> I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> strange code - we get irq, without checking for error, and then free it?
> What is the rationale?
Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().
>> clk_disable_unprepare(dev->clk);
>> - clk_put(dev->clk);
>> - iounmap(dev->io_base);
>> - kfree(dev);
>>
>> smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - release_mem_region(smi_base->start, resource_size(smi_base));
>> platform_set_drvdata(pdev, NULL);
>
> Why do we set platform data to NULL, is this needed?
It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.
Thanks,
Stefan
WARNING: multiple messages have this Message-ID (diff)
From: Stefan Roese <sr@denx.de>
To: dedekind1@gmail.com
Cc: Shiraz Hashim <shiraz.hashim@st.com>,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
Julia Lawall <Julia.Lawall@lip6.fr>,
linux-mtd@lists.infradead.org,
David Woodhouse <dwmw2@infradead.org>
Subject: Re: [PATCH] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently
Date: Fri, 24 Aug 2012 15:14:32 +0200 [thread overview]
Message-ID: <50377E38.1040701@denx.de> (raw)
In-Reply-To: <1345808104.2848.285.camel@sauron.fi.intel.com>
On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
>> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
>> ret = mtd_device_unregister(&flash->mtd);
>> if (ret)
>> dev_err(&pdev->dev, "error removing mtd\n");
>> -
>> - iounmap(flash->base_addr);
>> - kfree(flash);
>> }
>>
>> irq = platform_get_irq(pdev, 0);
>> - free_irq(irq, dev);
>
> I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> strange code - we get irq, without checking for error, and then free it?
> What is the rationale?
Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().
>> clk_disable_unprepare(dev->clk);
>> - clk_put(dev->clk);
>> - iounmap(dev->io_base);
>> - kfree(dev);
>>
>> smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - release_mem_region(smi_base->start, resource_size(smi_base));
>> platform_set_drvdata(pdev, NULL);
>
> Why do we set platform data to NULL, is this needed?
It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.
Thanks,
Stefan
WARNING: multiple messages have this Message-ID (diff)
From: Stefan Roese <sr@denx.de>
To: dedekind1@gmail.com
Cc: Julia Lawall <Julia.Lawall@lip6.fr>,
Shiraz Hashim <shiraz.hashim@st.com>,
David Woodhouse <dwmw2@infradead.org>,
kernel-janitors@vger.kernel.org, linux-mtd@lists.infradead.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently
Date: Fri, 24 Aug 2012 15:14:32 +0200 [thread overview]
Message-ID: <50377E38.1040701@denx.de> (raw)
In-Reply-To: <1345808104.2848.285.camel@sauron.fi.intel.com>
On 08/24/2012 01:35 PM, Artem Bityutskiy wrote:
>> @@ -1073,21 +1043,13 @@ static int __devexit spear_smi_remove(struct platform_device *pdev)
>> ret = mtd_device_unregister(&flash->mtd);
>> if (ret)
>> dev_err(&pdev->dev, "error removing mtd\n");
>> -
>> - iounmap(flash->base_addr);
>> - kfree(flash);
>> }
>>
>> irq = platform_get_irq(pdev, 0);
>> - free_irq(irq, dev);
>
> I guess 'platform_get_irq()' should be killed as well? Stefan, this is
> strange code - we get irq, without checking for error, and then free it?
> What is the rationale?
Yes, this seems bogus. platform_get_irq() definitely should be removed
from spear_smi_remove().
>> clk_disable_unprepare(dev->clk);
>> - clk_put(dev->clk);
>> - iounmap(dev->io_base);
>> - kfree(dev);
>>
>> smi_base = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> - release_mem_region(smi_base->start, resource_size(smi_base));
>> platform_set_drvdata(pdev, NULL);
>
> Why do we set platform data to NULL, is this needed?
It seems to be common practice to use this call to clear the drvdata in
the driver remove function. I have to admit, that I'm not sure if its
really needed though.
Thanks,
Stefan
next prev parent reply other threads:[~2012-08-24 13:14 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-11 8:58 [PATCH] drivers/mtd/devices/spear_smi.c: failure test for null rather than negative integer Julia Lawall
2012-07-11 8:58 ` Julia Lawall
2012-07-11 8:58 ` Julia Lawall
2012-07-11 9:01 ` viresh kumar
2012-07-11 9:01 ` viresh kumar
2012-07-11 9:01 ` viresh kumar
2012-07-16 9:38 ` Stefan Roese
2012-07-16 9:38 ` Stefan Roese
2012-07-16 9:38 ` Stefan Roese
2012-08-04 20:36 ` [PATCH] drivers/mtd/devices/spear_smi.c: use devm_ functions consistently Julia Lawall
2012-08-04 20:36 ` Julia Lawall
2012-08-04 20:36 ` Julia Lawall
2012-08-24 11:35 ` Artem Bityutskiy
2012-08-24 11:35 ` Artem Bityutskiy
2012-08-24 11:35 ` Artem Bityutskiy
2012-08-24 13:14 ` Stefan Roese [this message]
2012-08-24 13:14 ` Stefan Roese
2012-08-24 13:14 ` Stefan Roese
2012-08-24 14:43 ` Artem Bityutskiy
2012-08-24 14:43 ` Artem Bityutskiy
2012-08-24 14:43 ` Artem Bityutskiy
2012-08-17 9:59 ` [PATCH] drivers/mtd/devices/spear_smi.c: failure test for null rather than negative integer Artem Bityutskiy
2012-08-17 10:03 ` Artem Bityutskiy
2012-08-17 10:03 ` Artem Bityutskiy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50377E38.1040701@denx.de \
--to=sr@denx.de \
--cc=Julia.Lawall@lip6.fr \
--cc=dedekind1@gmail.com \
--cc=dwmw2@infradead.org \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=shiraz.hashim@st.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.