Messages in this thread | | | Date | Tue, 31 Oct 2017 10:38:30 +0000 | From | Mark Brown <> | Subject | Re: [PATCH 1/2] regmap: Add hardware spinlock support |
| |
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...
> + 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.
> --- 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. [unhandled content-type:application/pgp-signature] | |