lkml.org 
[lkml]   [2010]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] OMAP: HWMOD: Handle opt clocks using clk_add_alias
    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<p-basak2@ti.com>
    >>>
    >>> For every optional clock present per hwmod per omap-device, this
    >> function
    >>> adds an entry in the clocks list of the form<dev-id=dev_name, con-
    >> id=role>,
    >>> if an entry is already present in the list of the form<dev-id=NULL, 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(<dev*>,<role>).
    >>>
    >>> Link to discussions related to this patch:
    >>> http://www.spinics.net/lists/linux-omap/msg34809.html
    >>>
    >>> Signed-off-by: Charulatha V<charu@ti.com>
    >>> Signed-off-by: Basak, Partha<p-basak2@ti.com>
    >>> Signed-off-by: Benoit Cousson<b-cousson@ti.com>
    >>> Signed-off-by: Rajendra Nayak<rnayak@ti.com>
    >>>
    >>> Cc: Paul Walmsley<paul@pwsan.com>
    >>> Cc: Kevin Hilman<khilman@deeprootsystems.com>
    >>> ---
    >>> 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<linux/slab.h>
    >>> #include<linux/err.h>
    >>> #include<linux/io.h>
    >>> +#include<linux/clk.h>
    >>>
    >>> #include<plat/omap_device.h>
    >>> #include<plat/omap_hwmod.h>
    >>> @@ -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<dev-id=dev_name, con-
    >> id=role>
    >>> + * if an entry is already present in it with the form<dev-id=NULL,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(<dev*>,<role>).
    >>> + */
    >>> +
    >>> +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 <p-basak2@ti.com>
    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 <dev-id=dev_name, con-id=role>,
    if an entry is already present in the list of the form <dev-id=NULL,
    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(<dev*>, <role>).

    Link to discussions related to this patch:
    http://www.spinics.net/lists/linux-omap/msg34809.html

    Signed-off-by: Charulatha V <charu@ti.com>
    Signed-off-by: Basak, Partha <p-basak2@ti.com>
    Signed-off-by: Benoit Cousson <b-cousson@ti.com>
    Signed-off-by: Rajendra Nayak <rnayak@ti.com>
    Cc: Paul Walmsley <paul@pwsan.com>
    Cc: Kevin Hilman <khilman@deeprootsystems.com>
    ---
    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 <linux/slab.h>
    #include <linux/err.h>
    #include <linux/io.h>
    +#include <linux/clk.h>

    #include <plat/omap_device.h>
    #include <plat/omap_hwmod.h>
    @@ -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 <dev-id=dev_name,
    con-id=role>
    + * if an entry is already present in it with the form <dev-id=NULL,
    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(<dev*>, <role>).
    + */
    +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;
    --
    1.6.0.4





    \
     
     \ /
      Last update: 2010-08-25 13:33    [W:0.042 / U:29.724 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site