From: Philipp Zabel <p.zabel@pengutronix.de>
To: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Grant Likely <grant.likely@secretlab.ca>,
Rob Herring <rob.herring@calxeda.com>,
Shawn Guo <shawn.guo@linaro.org>,
Richard Zhao <richard.zhao@freescale.com>,
Huang Shijie <shijie8@gmail.com>,
Dong Aisheng <dong.aisheng@linaro.org>,
Matt Porter <mporter@ti.com>,
kernel@pengutronix.de, devicetree-discuss@lists.ozlabs.org
Subject: Re: [PATCH v5 3/4] misc: sram: Add optional clock
Date: Mon, 29 Oct 2012 13:20:57 +0100 [thread overview]
Message-ID: <1351513257.5872.104.camel@pizza.hi.pengutronix.de> (raw)
In-Reply-To: <CAP=VYLpwahaxVssP1bXmp+ttM_eArF9B2DX1auiOmterdHHUGw@mail.gmail.com>
Am Freitag, den 26.10.2012, 12:17 -0400 schrieb Paul Gortmaker:
> On Thu, Oct 18, 2012 at 10:27 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > On some platforms the SRAM needs a clock to be enabled explicitly.
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > drivers/misc/sram.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
> > index 7a363f2..0cc2e75 100644
> > --- a/drivers/misc/sram.c
> > +++ b/drivers/misc/sram.c
> > @@ -21,6 +21,8 @@
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > #include <linux/io.h>
> > #include <linux/of.h>
> > #include <linux/platform_device.h>
> > @@ -29,6 +31,7 @@
> >
> > struct sram_dev {
> > struct gen_pool *pool;
> > + struct clk *clk;
> > };
>
> I see another field gets added to the struct here. (yet another
> reason to have it folded into the original) But you still
> really don't need to create a sram_dev for this, because...
>
> >
> > static int __devinit sram_probe(struct platform_device *pdev)
> > @@ -53,6 +56,10 @@ static int __devinit sram_probe(struct platform_device *pdev)
> > if (!sram)
> > return -ENOMEM;
> >
> > + sram->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (!IS_ERR(sram->clk))
> > + clk_prepare_enable(sram->clk);
> > +
> > sram->pool = gen_pool_create(PAGE_SHIFT, -1);
> > if (!sram->pool)
> > return -ENOMEM;
> > @@ -80,6 +87,9 @@ static int __devexit sram_remove(struct platform_device *pdev)
> >
> > gen_pool_destroy(sram->pool);
> >
> > + if (!IS_ERR(sram->clk))
> > + clk_disable_unprepare(sram->clk);
> > +
>
> ...here, this looks confusing with the use of IS_ERR on
> an entity that was not recently assigned to.
Right.
How about I set sram->clk = NULL in sram_probe if devm_clk_get returns
an error value?
> Instead, just
> put a "struct clk *clk;" on the stack and do the
>
> clk = devm_clk_get(&pdev->dev, NULL);
>
> in both the init and the teardown. Then the code will be
> more readable.
Calling devm_clk_get on the same clock twice seems a bit weird.
I expect that eventually someone will want to disable clocks during
suspend, so I'd prefer to keep the clk pointer around.
regards
Philipp
next prev parent reply other threads:[~2012-10-29 12:20 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-10-18 14:27 [PATCH v5 0/4] Add generic driver for on-chip SRAM Philipp Zabel
2012-10-18 14:27 ` [PATCH v5 1/4] genalloc: add a global pool list, allow to find pools by phys address Philipp Zabel
2012-10-25 18:56 ` Greg Kroah-Hartman
[not found] ` <1350570453-24546-2-git-send-email-p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2012-10-26 19:46 ` Paul Gortmaker
2012-11-15 13:25 ` Philipp Zabel
[not found] ` <1352985943.2399.198.camel-/rZezPiN1rtR6QfukMTsflXZhhPuCNm+@public.gmane.org>
2012-11-16 1:50 ` Paul Gortmaker
2012-10-18 14:27 ` [PATCH v5 2/4] misc: Generic on-chip SRAM allocation driver Philipp Zabel
2012-10-26 16:07 ` Paul Gortmaker
2012-10-29 12:20 ` Philipp Zabel
[not found] ` <1351513256.5872.103.camel-/rZezPiN1rtR6QfukMTsflXZhhPuCNm+@public.gmane.org>
2012-11-06 18:43 ` Paul Gortmaker
2012-10-18 14:27 ` [PATCH v5 3/4] misc: sram: Add optional clock Philipp Zabel
2012-10-26 15:18 ` Paul Gortmaker
2012-10-29 12:20 ` Philipp Zabel
2012-10-26 16:17 ` Paul Gortmaker
2012-10-29 12:20 ` Philipp Zabel [this message]
[not found] ` <1351513257.5872.104.camel-/rZezPiN1rtR6QfukMTsflXZhhPuCNm+@public.gmane.org>
2012-11-06 18:28 ` Paul Gortmaker
2012-10-18 14:27 ` [PATCH v5 4/4] misc: sram: add support for configurable allocation order Philipp Zabel
2012-11-14 19:15 ` Grant Likely
2012-11-15 13:11 ` Philipp Zabel
2012-11-15 16:52 ` Grant Likely
[not found] ` <1352985095.2399.184.camel-/rZezPiN1rtR6QfukMTsflXZhhPuCNm+@public.gmane.org>
2012-11-16 14:09 ` Matt Porter
2012-11-16 14:11 ` Grant Likely
2012-11-16 15:58 ` Philipp Zabel
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=1351513257.5872.104.camel@pizza.hi.pengutronix.de \
--to=p.zabel@pengutronix.de \
--cc=arnd@arndb.de \
--cc=devicetree-discuss@lists.ozlabs.org \
--cc=dong.aisheng@linaro.org \
--cc=grant.likely@secretlab.ca \
--cc=gregkh@linuxfoundation.org \
--cc=kernel@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
--cc=mporter@ti.com \
--cc=paul.gortmaker@windriver.com \
--cc=richard.zhao@freescale.com \
--cc=rob.herring@calxeda.com \
--cc=shawn.guo@linaro.org \
--cc=shijie8@gmail.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).