Re: [PATCH 1/2] regmap: Add hardware spinlock support

[Baolin Wang]

Hi Mark,

On 31 October 2017 at 18:38, Mark Brown <broonie@xxxxxxxxxx> wrote:
> On Tue, Oct 31, 2017 at 02:35:55PM +0800, Baolin Wang wrote:
>
> This looks mostly good, a few small things:
>
>> +static void regmap_lock_hwlock(void *__map)
>> +{
>> +     struct regmap *map = __map;
>> +     int ret;
>> +
>> +     if (map->hwlock_timeout)
>> +             ret = hwspin_lock_timeout(map->hwlock, map->hwlock_timeout);
>> +     else
>> +             ret = hwspin_trylock(map->hwlock);
>> +
>> +     if (ret)
>> +             dev_err(map->dev, "Failed to get hwlock %d\n", ret);
>> +}
>
> Given that we have no error handling path on the locks should we be
> supporting timeout mode at all?  Otherwise we should probably add a
> set of error handling paths whenever we take the lock...

It will be more helpful to use the timeout to try more times to get
the hwlock, and we usually do not use hwspin_trylock_xxx(), so we can
remove hwspin_trylock_xxx() support and set timeout as MAX value as
default to avoid adding 'hwlock_timeout' config,
is this OK for you?

>
>> +             if (config->hwlock_mode == HWLOCK_IRQSTATE) {
>> +                     map->lock = regmap_lock_hwlock_irqsave;
>> +                     map->unlock = regmap_unlock_hwlock_irqrestore;
>> +             } else if (config->hwlock_mode == HWLOCK_IRQ) {
>> +                     map->lock = regmap_lock_hwlock_irq;
>> +                     map->unlock = regmap_unlock_hwlock_irq;
>> +             } else {
>> +                     map->lock = regmap_lock_hwlock;
>> +                     map->unlock = regmap_unlock_hwlock;
>> +             }
>
> This should be a switch statement.

Yes, will fix in next version.

>
>> --- a/include/linux/regmap.h
>> +++ b/include/linux/regmap.h
>> @@ -19,6 +19,7 @@
>>  #include <linux/err.h>
>>  #include <linux/bug.h>
>>  #include <linux/lockdep.h>
>> +#include <linux/hwspinlock.h>
>
> We don't actually use hwspinlock.h in the header (the config options are
> all just unsigned ints) so this could be moved to regmap.c with a
> forward declaration of the struct in internal.h.  That way we don't
> force a rebuild of every regmap user when hwspinlock changes.

You are right. I will more the hwspinlock.h to regmap.c file. Very
appreciated for your comments.

-- 
Baolin.wang
Best Regards