From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Cousson, Benoit" Subject: Re: [PATCH] OMAP: HWMOD: Handle opt clocks using clk_add_alias Date: Wed, 25 Aug 2010 16:20:54 +0200 Message-ID: <4C7526C6.7090207@ti.com> References: <1282578269-8835-1-git-send-email-p-basak2@ti.com> <4C72D6AC.3060509@ti.com> <4C74FEEE.2090300@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from comal.ext.ti.com ([198.47.26.152]:59951 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751997Ab0HYOVB (ORCPT ); Wed, 25 Aug 2010 10:21:01 -0400 In-Reply-To: <4C74FEEE.2090300@ti.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: "Basak, Partha" Cc: "Varadarajan, Charulatha" , "Nayak, Rajendra" , Paul Walmsley , Kevin Hilman , linux-omap Re-sent to loml, the original email was sent to lkml... On 8/25/2010 1:30 PM, Cousson, Benoit wrote: > On 8/25/2010 12:45 PM, Basak, Partha wrote: >> >>> From: Cousson, Benoit >>> Sent: Tuesday, August 24, 2010 1:45 AM >>> >>> Hi Partha, >>> >>> On 8/23/2010 5:44 PM, Basak, Partha wrote: >>>> From: Basak, Partha >>>> >>>> For every optional clock present per hwmod per omap-device, this >>> function >>>> adds an entry in the clocks list of the form>> id=role>, >>>> if an entry is already present in the list of the form>> id=role>. >>>> >>>> The function is called from within the framework inside >>> omap_device_build_ss(), >>>> after omap_device_register. >>>> >>>> This allows drivers to get a pointer to its optional clocks based on its >>> role >>>> by calling clk_get(,). >>>> >>>> Link to discussions related to this patch: >>>> http://www.spinics.net/lists/linux-omap/msg34809.html >>>> >>>> Signed-off-by: Charulatha V >>>> Signed-off-by: Basak, Partha >>>> Signed-off-by: Benoit Cousson >>>> Signed-off-by: Rajendra Nayak >>>> >>>> Cc: Paul Walmsley >>>> Cc: Kevin Hilman >>>> --- >>>> arch/arm/plat-omap/omap_device.c | 31 >>> ++++++++++++++++++++++++++++++- >>>> 1 files changed, 30 insertions(+), 1 deletions(-) >>>> >>>> Index: linux-omap-pm/arch/arm/plat-omap/omap_device.c >>>> =================================================================== >>>> --- linux-omap-pm.orig/arch/arm/plat-omap/omap_device.c 2010-08-18 >>> 02:48:30.789079550 +0530 >>>> +++ linux-omap-pm/arch/arm/plat-omap/omap_device.c 2010-08-24 >>> 07:19:43.637080138 +0530 >>>> @@ -82,6 +82,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> #include >>>> #include >>>> @@ -243,7 +244,6 @@ static inline struct omap_device *_find_ >>>> return container_of(pdev, struct omap_device, pdev); >>>> } >>>> >>>> - >>> >>> That fix is not related to the patch subject. >>> >>>> /* Public functions for use by core code */ >>>> >>>> /** >>>> @@ -271,6 +271,47 @@ int omap_device_count_resources(struct o >>>> } >>> >>> This is not a public function, so you should move it before the /* >>> Public functions for use by core code */ >>> >> OK >>>> /** >>>> + * omap_device_add_opt_clk_alias - Add alias for optional clocks in the >>>> + * clocks list. >>>> + * >>>> + * @od: struct omap_device *od >>>> + * >>>> + * For every optional clock present per hwmod per omap-device, this >>> function >>>> + * adds an entry in the clocks list of the form>> id=role> >>>> + * if an entry is already present in it with the form>> id=role> >>>> + * >>>> + * The function is called from inside omap_device_build_ss(), >>>> + * after omap_device_register. >>>> + * >>>> + * This allows drivers to get a pointer to its optional clocks based on >>> its role >>>> + * by calling clk_get(,). >>>> + */ >>>> + >>>> +static void omap_device_add_opt_clk_alias(struct omap_device *od) >>>> +{ >>>> + int i, j; >>>> + struct omap_hwmod_opt_clk *oc; >>>> + struct omap_hwmod *oh; >>>> + >>>> + for (i = 0, oh = *od->hwmods; i< od->hwmods_cnt; i++, oh++) >>> >>> You can avoid that iteration and the extra level of indentation by >>> re-using the iteration done before the call to that function. >>> >>>> + /* Add Clock alias for all optional clocks*/ >>>> + for (j = oh->opt_clks_cnt, oc = oh->opt_clks; >>>> + j> 0; j--, oc++) { >>>> + if ((oc->_clk)&& >>>> + (IS_ERR(clk_get(&od->pdev.dev, oc->role)))) { >>>> + int ret; >>> >> Not clear on your comment, wanted to take care of the scenario where a device is having multiple hwmods > > Which one? the one below or before? These comments are just about the > readability of this patch. > >>> You can avoid the indentation by returning in case of error. >>> You can avoid as well 2 pairs of parens. >>> >>>> + >>>> + ret = clk_add_alias(oc->role, >>>> + dev_name(&od->pdev.dev), >>>> + (char *)oc->clk, NULL); >>> >>> The indentation is not align with the inner parameters... which is not >>> easy in your case due to the too many indentation level you have in this >>> function. >>> >>>> + if (ret) >>>> + dev_err(&od->pdev.dev, "omap_device:\ >>> >>> This is not correct way of splitting long string, use "omap_device:" >>> instead. >>> >> Agreed >> >>> Even if dev_err is usable because you have the dev pointer, it is in >>> fact an error from the omap_device code code, so using pr_err is >>> probably more appropriate (TBC). >>> >>>> + clk_add_alias for %s failed\n", >>>> + oc->role); >>>> + } >>>> + } >>>> +} >>>> +/** >>>> * omap_device_fill_resources - fill in array of struct resource >>>> * @od: struct omap_device * >>>> * @res: pointer to an array of struct resource to be filled in >>>> @@ -424,6 +465,12 @@ struct omap_device *omap_device_build_ss >>>> for (i = 0; i< oh_cnt; i++) >>>> hwmods[i]->od = od; >>> >>> You can call a per hwmod API here in order to avoid the iteration inside >>> the function. >> >> I see your point, but most of the functions in this layer are having omap_device * as the parameter, so I maintained consistency > > Since your function is anyway a private function, I don't see any reason > to do that. > > FYI, I joined an updated version. > > Regards, > Benoit > > --- > From d8a374c679b7f26229cf054520deb0ac416b7f4c Mon Sep 17 00:00:00 2001 > From: Basak, Partha > Date: Mon, 23 Aug 2010 21:14:29 +0530 > Subject: [PATCH] OMAP: hwmod: handle opt clocks node using clk_add_alias > > For every optional clock present per hwmod per omap-device, this function > adds an entry in the clocks list of the form, > if an entry is already present in the list of the form con-id=role>. > > The function is called from within the framework inside > omap_device_build_ss(), > after omap_device_register. > > This allows drivers to get a pointer to its optional clocks based on its > role > by calling clk_get(,). > > Link to discussions related to this patch: > http://www.spinics.net/lists/linux-omap/msg34809.html > > Signed-off-by: Charulatha V > Signed-off-by: Basak, Partha > Signed-off-by: Benoit Cousson > Signed-off-by: Rajendra Nayak > Cc: Paul Walmsley > Cc: Kevin Hilman > --- > arch/arm/plat-omap/omap_device.c | 39 > +++++++++++++++++++++++++++++++++++++- > 1 files changed, 38 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/plat-omap/omap_device.c > b/arch/arm/plat-omap/omap_device.c > index d2b1609..d876cec 100644 > --- a/arch/arm/plat-omap/omap_device.c > +++ b/arch/arm/plat-omap/omap_device.c > @@ -82,6 +82,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -243,6 +244,40 @@ static inline struct omap_device > *_find_by_pdev(struct platform_device *pdev) > return container_of(pdev, struct omap_device, pdev); > } > > +/** > + * _add_optional_clock_alias - Add clock alias for hwmod optional clocks > + * @od: struct omap_device *od > + * > + * For every optional clock present per hwmod per omap_device, this > function > + * adds an entry in the clocks list of the form con-id=role> > + * if an entry is already present in it with the form con-id=role> > + * > + * The function is called from inside omap_device_build_ss(), after > + * omap_device_register. > + * > + * This allows drivers to get a pointer to its optional clocks based on > its role > + * by calling clk_get(,). > + */ > +static void _add_optional_clock_alias(struct omap_device *od, > + struct omap_hwmod *oh) > +{ > + int i; > + struct omap_hwmod_opt_clk *oc; > + > + for (i = oh->opt_clks_cnt, oc = oh->opt_clks; i> 0; i--, oc++) { > + int ret; > + > + if (!oc->_clk || !IS_ERR(clk_get(&od->pdev.dev, oc->role))) > + return; > + > + ret = clk_add_alias(oc->role, dev_name(&od->pdev.dev), > + (char *)oc->clk, NULL); > + if (ret) > + pr_err("omap_device: clk_add_alias for %s failed\n", > + oc->role); > + } > +} > + > > /* Public functions for use by core code */ > > @@ -421,8 +456,10 @@ struct omap_device *omap_device_build_ss(const char > *pdev_name, int pdev_id, > else > ret = omap_device_register(od); > > - for (i = 0; i< oh_cnt; i++) > + for (i = 0; i< oh_cnt; i++) { > hwmods[i]->od = od; > + _add_optional_clock_alias(od, hwmods[i]); > + } > > if (ret) > goto odbs_exit4;