Use of of_parse_phandle()/of_node_put()

Commit Message

Anil Kumar Feb. 13, 2013, 5:02 p.m. UTC

Hi Peter,

On Wed, Feb 13, 2013 at 10:10 PM, Anil Kumar <[e-mail]> wrote:
> Hi,
>
> On Wed, Feb 13, 2013 at 9:22 PM, Peter Ujfalusi <[e-mail]> wrote:
>> On 02/13/2013 07:04 AM, Kumar, Anil wrote:
>>> Hi  Peter,
>>>
>>> Just trying to understand.
>>>
>>> In omap-twl4030.c file probe function :-
>>>
>>> dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
>>>                 if (!dai_node) {
>>>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>>>                         return -EINVAL;
>>>                 }
>>>
>>> Here  “of_parse_phandle()”  is used to get  “of_device” node pointer.
>>> of_parse_phandle() suggest to use of_node_put()  on it when done.
>>>
>>> It looks when code request for an “of_device” node, kernel  maintains
>>> “refcount” for this.
>>> It check “refcount”  before giving pointer of of_device node and  WARN_ON()
>>> in case of refcount  > 0 and increase it on success.
>>>
>>> Should this code need to use of_node_put() on the requested “of_device” when done
>>> so that this can be get  again ?
>>
>> Hrm, one thing or sure we should not call it of_node_put() while we have the
>> card loaded since the node is used runtime by the core.
>>
>> However when we unload the machine driver it might be needed, but not sure
>> about this. None of the existing machine drivers doing it (tegra, samsung,
>> omap, etc).
>> But if it is needed it might be better to be done by the core?
>>

Sorry i forgot to mention.

Yes, I think soc core may have something like[1] to release cpu_of_node.

[1]
------------8---------------
Anil

>
> It looks issue is different here.
> I have done some testing here with patch[1]. In this patch i tried to
> get same "of_device" node pointer
> again and found refcount for this dt node is 1 [Result].
>
> As of_parse_phandle() says it "returns the device_node pointer with
> refcount incremented".
> But why refcount  value is 1 [Result] again ?
>
> -----------------------------8---------------------
> Patch[1]:-
> diff --git a/sound/soc/omap/omap-twl4030.c b/sound/soc/omap/omap-twl4030.c
> index fd98509..0828a5c 100644
> --- a/sound/soc/omap/omap-twl4030.c
> +++ b/sound/soc/omap/omap-twl4030.c
> @@ -297,6 +297,16 @@ static int omap_twl4030_probe(struct platform_device *pdev)
>                         dev_err(&pdev->dev, "McBSP node is not provided\n");
>                         return -EINVAL;
>                 }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +               dai_node = of_parse_phandle(node, "ti,mcbsp", 0);
> +                if (!dai_node) {
> +                        dev_err(&pdev->dev, "McBSP node is not provided\n");
> +                        return -EINVAL;
> +                }
> +               printk(KERN_ERR"refcount 0x%x",
> atomic_read(&dai_node->kref.refcount));
> +
> +
>                 omap_twl4030_dai_links[0].cpu_dai_name  = NULL;
>                 omap_twl4030_dai_links[0].cpu_of_node = dai_node;
>
> [Result]:-
> root@DevKit8000:/# insmod snd-soc-omap-twl4030.ko
> [   95.718109] refcount 0x1
> [   95.720611] refcount 0x1 [   95.818054] omap-twl4030 sound.20:
> twl4030-hifi <-> 49022000.mcbsp mapping ok
>
>
> Thanks,
> Anil
> [...]
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Peter Ujfalusi Feb. 14, 2013, 9:26 a.m. UTC | #1

On 02/13/2013 06:02 PM, Anil Kumar wrote:
> ------------8---------------
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index b7e84a7..9000f4a 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -1044,6 +1044,13 @@ static void soc_remove_dai_links(struct
> snd_soc_card *card)
>                         soc_remove_link_dais(card, dai, order);
>         }
> 
> +       /* release cpu_of_node */
> +       if(card->dai_link) {
> +               int i;
> +               for(i = 0; i < card->num_links; i++)
> +                       of_node_put(card->dai_link[i].cpu_of_node);

and the same for codec_of_node and platform_of_node for that matter.

Mark: what do you think? Does it make sense to do this in the core or should
we let the machine drivers to take care of this?

> +       }
> +
>

Patch

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index b7e84a7..9000f4a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1044,6 +1044,13 @@  static void soc_remove_dai_links(struct
snd_soc_card *card)
                        soc_remove_link_dais(card, dai, order);
        }

+       /* release cpu_of_node */
+       if(card->dai_link) {
+               int i;
+               for(i = 0; i < card->num_links; i++)
+                       of_node_put(card->dai_link[i].cpu_of_node);
+       }
+

Thanks,