Messages in this thread | | | From | Baolin Wang <> | Date | Tue, 31 Oct 2017 19:31:57 +0800 | Subject | Re: [PATCH 1/2] regmap: Add hardware spinlock support |
| |
Hi Mark,
On 31 October 2017 at 18:38, Mark Brown <broonie@kernel.org> 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
| |