linux-kernel - Re: [PATCH v5 3/4] misc: sram: Add optional clock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 6 Nov 2012 13:28:31 -0500
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	Philipp Zabel <p.zabel@...gutronix.de>
CC:	<linux-kernel@...r.kernel.org>, Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Shawn Guo <shawn.guo@...aro.org>,
	Richard Zhao <richard.zhao@...escale.com>,
	Huang Shijie <shijie8@...il.com>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Matt Porter <mporter@...com>, <kernel@...gutronix.de>,
	<devicetree-discuss@...ts.ozlabs.org>
Subject: Re: [PATCH v5 3/4] misc: sram: Add optional clock

On 12-10-29 08:20 AM, Philipp Zabel wrote:
> 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@...gutronix.de> wrote:
>>> On some platforms the SRAM needs a clock to be enabled explicitly.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@...gutronix.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?

Sorry for delayed reply ; was out last week.
And yes, I like that better than the double call as well.

Thanks,
Paul.
--

> 
>> 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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/