of_clk_get() / devm_clk_get()

of_clk_get() / devm_clk_get()

[Tony Prisk]

Currently we have devm_clk_get() which gives a managed-resource clk (by
name), or of_clk_get() which gives an unmanaged resource clk (by id).

I just wanted to sound out everyone as to whether there is a need for a
managed version of the of_clk_get.

My personal concern about devm_clk_get is that it requires (if I
understand correctly) that the DT node have the clock-names property
(which is optional). If the optional name is not supplied, it fails.
This basically makes it 'not optional' when a driver uses devm_clk_get.
(Please correct me if I'm wrong about this).

Everything below here is irrelevant if the above is wrong :)


If the above IS correct, then couldn't we have a devm_of_clk_get which
accepts a *id (as per devm_clk_get) AND an index (as per of_clk_get),
with the *id given priority.

Something along the lines of...

struct clk *devm_of_clk_get(struct device *dev,
			    const char *id,
			    int index)
{
	struct clk **ptr, *clk = NULL;

	if (id)
		clk = devm_clk_get(dev, id);

	if (!clk || IS_ERR(clk)) {
		clk = of_clk_get(dev->of_node, index);
		if (!IS_ERR(clk)) {
			ptr = devres_alloc(devm_clk_release,
					   sizeof(*ptr),
					   GFP_KERNEL)
			if (!ptr)
				return ERR_PTR(-ENOMEM);

			*ptr = clk;
			devres_add(dev, ptr);
		}
	}

	return clk;
}

This would make clock-names optional (as intended) and give a fallback
to the index when clock-names isn't specified.

One issue (which I don't know how much of an issue it is) is that this
doesn't follow the normal pattern of normal_func -> devm_func parameter
layout.


Regards
Tony Prisk

of_clk_get() / devm_clk_get()

[Stephen Warren]

On 02/13/2013 09:17 PM, Tony Prisk wrote:
> Currently we have devm_clk_get() which gives a managed-resource clk (by
> name), or of_clk_get() which gives an unmanaged resource clk (by id).
> 
> I just wanted to sound out everyone as to whether there is a need for a
> managed version of the of_clk_get.
> 
> My personal concern about devm_clk_get is that it requires (if I
> understand correctly) that the DT node have the clock-names property
> (which is optional). If the optional name is not supplied, it fails.
> This basically makes it 'not optional' when a driver uses devm_clk_get.
> (Please correct me if I'm wrong about this).

I thought supplying NULL for the name/ID simply gave you the first clock
in the list (clocks property)? I'm sure I've seen plenty of Tegra
drivers that end up doing that.

of_clk_get() / devm_clk_get()

[Tony Prisk]

On Wed, 2013-02-13 at 21:40 -0700, Stephen Warren wrote:
> On 02/13/2013 09:17 PM, Tony Prisk wrote:
> > Currently we have devm_clk_get() which gives a managed-resource clk (by
> > name), or of_clk_get() which gives an unmanaged resource clk (by id).
> > 
> > I just wanted to sound out everyone as to whether there is a need for a
> > managed version of the of_clk_get.
> > 
> > My personal concern about devm_clk_get is that it requires (if I
> > understand correctly) that the DT node have the clock-names property
> > (which is optional). If the optional name is not supplied, it fails.
> > This basically makes it 'not optional' when a driver uses devm_clk_get.
> > (Please correct me if I'm wrong about this).
> 
> I thought supplying NULL for the name/ID simply gave you the first clock
> in the list (clocks property)? I'm sure I've seen plenty of Tegra
> drivers that end up doing that.
> 

I think someone else told me that once before so it is probably correct
- but it doesn't solve the problem of requesting a second clock without
a clock-names property.

That leaves us in the situation of only being able to use managed clocks
when there is only one, or forcing dts files to include the 'optional'
property.

Regards
Tony Prisk

of_clk_get() / devm_clk_get()

[Stephen Warren]

On 02/13/2013 11:42 PM, Tony Prisk wrote:
> On Wed, 2013-02-13 at 21:40 -0700, Stephen Warren wrote:
>> On 02/13/2013 09:17 PM, Tony Prisk wrote:
>>> Currently we have devm_clk_get() which gives a managed-resource clk (by
>>> name), or of_clk_get() which gives an unmanaged resource clk (by id).
>>>
>>> I just wanted to sound out everyone as to whether there is a need for a
>>> managed version of the of_clk_get.
>>>
>>> My personal concern about devm_clk_get is that it requires (if I
>>> understand correctly) that the DT node have the clock-names property
>>> (which is optional). If the optional name is not supplied, it fails.
>>> This basically makes it 'not optional' when a driver uses devm_clk_get.
>>> (Please correct me if I'm wrong about this).
>>
>> I thought supplying NULL for the name/ID simply gave you the first clock
>> in the list (clocks property)? I'm sure I've seen plenty of Tegra
>> drivers that end up doing that.
>>
> 
> I think someone else told me that once before so it is probably correct
> - but it doesn't solve the problem of requesting a second clock without
> a clock-names property.
> 
> That leaves us in the situation of only being able to use managed clocks
> when there is only one, or forcing dts files to include the 'optional'
> property.

True. Writing a devm_ wrapper around of_clk_get() should be pretty
trivial though.