lkml.org 
[lkml]   [2014]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv4 4/7] hwspinlock/core: add common OF helpers
Bjorn,

On 02/07/2014 04:49 PM, Bjorn Andersson wrote:
> On Mon, Jan 13, 2014 at 4:19 PM, Suman Anna <s-anna@ti.com> wrote:
>> This patch adds three new OF helper functions to use/request
>> locks from a hwspinlock device instantiated through a
>> device-tree blob.
>
> Nice, I ran in to the problem of needing a probe deferral on a
> hwspinlock earlier this week so I implemented this yesterday...then I
> got a pointer to your series.
>
> [snip]
>> /**
>> + * of_hwspin_lock_request_specific() - request a OF phandle-based specific lock
>> + * @np: device node from which to request the specific hwlock
>> + * @propname: property name containing hwlock specifier(s)
>> + * @index: index of the hwlock
>> + *
>> + * This function is the OF equivalent of hwspin_lock_request_specific(). This
>> + * function provides a means for users of the hwspinlock module to request a
>> + * specific hwspinlock using the phandle of the hwspinlock device. The requested
>> + * lock number is indexed relative to the hwspinlock device, unlike the
>> + * hwspin_lock_request_specific() which is an absolute lock number.
>> + *
>> + * Returns the address of the assigned hwspinlock, or NULL on error
>> + */
>> +struct hwspinlock *of_hwspin_lock_request_specific(struct device_node *np,
>> + const char *propname, int index)
>> +{
>> + struct hwspinlock_device *bank;
>> + struct of_phandle_args args;
>> + int id;
>> + int ret;
>> +
>> + ret = of_parse_phandle_with_args(np, propname, "#hwlock-cells", index,
>> + &args);
>> + if (ret) {
>> + pr_warn("%s: can't parse hwlocks property of node '%s[%d]' ret = %d\n",
>> + __func__, np->full_name, index, ret);
>> + return NULL;
>> + }
>
> of_parse_phandle_with_args() already does pr_err if it can't find the
> phandle and on some of the issues related to arguments. So please
> remove this pr_warn().

Yes, I will clean this up.

>
> It seems to be standard practice to pass the error value back to the
> consumer, so you should
> return ERR_PTR(ret); here instead of the NULL...

I have modelled the return values in this function based on the return
values in the existing hwspin_lock_request interfaces. I would need to
change those functions as well.

Ohad,
Do you have any objections to the return code convention change? I agree
with Bjorn on the changes. If you are ok, then I will add a separate
patch for the existing functions and revise this patch as well.

>
>> +
>> + mutex_lock(&hwspinlock_tree_lock);
>> + list_for_each_entry(bank, &hwspinlock_devices, list)
>> + if (bank->dev->of_node == args.np)
>> + break;
>> + mutex_unlock(&hwspinlock_tree_lock);
>> + if (&bank->list == &hwspinlock_devices) {
>> + pr_warn("%s: requested hwspinlock device %s is not registered\n",
>> + __func__, args.np->full_name);
>> + return NULL;
>
> ...especially since you want the consumer to have the ability to
> identify this error. Here you should
> return ERR_PTR(-EPROBE_DEFER); so that the consumer knows that this
> lock is not _yet_ registered, but will be in the future.
>
> You should remove this pr_warn as well. The standard use of this
> function would be in a probe() and just returning this error value
> from that probe will give you a line in the log indicating that this
> was in fact the issue.

OK.

>
>> + }
>> +
>> + id = bank->ops->of_xlate(bank, &args);
>> + if (id < 0 || id >= bank->num_locks) {
>> + pr_warn("%s: requested lock %d is either out of range [0, %d] or failed translation\n",
>> + __func__, id, bank->num_locks - 1);
>> + return NULL;
>
> Please return ERR_PTR(-EINVAL); here.

OK, will change this based on Ohad's ack/nack.

>
> Looking forward to your next spin, as I will actually use this interface :)

Thanks for your comments. I will wait to see if there are any additional
comments before I refresh the series later this week.

regards
Suman


\
 
 \ /
  Last update: 2014-02-10 21:01    [W:0.139 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site