Re: [PATCH 2/2] iio: light: veml3328: add support for new device

[Jonathan Cameron <jic23@kernel.org>]

> > > +
> > > +struct veml3328_data {
> > > +     struct regmap *regmap;
> > > +     struct device *dev;  
> > The use of the one embedded in regmap got mentioned already in another review.  
> > > +     struct mutex lock;  
> > All locks need a comment saying what data they are protecting
> > (might well be in the device).
> >
> > Mind you - I'm seeing quite a bit of locking around simple regmap calls.
> > Given there are locks in regmap, you may need to call out if there
> > is a particular readout sequence that must not be interrupted.
> >
> > I'm not immediately seeing one and as such you might not need a local
> > lock.  
> 
> I was on the fence with this one - my understanding was that the locking
> in regmap was just for i2c bus interactions, not actual value read/writes.
> I've no problem with removing it though if I am mistaken.
> 
The regmap calls themselves are all safe against races.
E.g.
 */
int regmap_update_bits_base(struct regmap *map, unsigned int reg,
			    unsigned int mask, unsigned int val,
			    bool *change, bool async, bool force)
{
	int ret;

	map->lock(map->lock_arg);

	map->async = async;

	ret = _regmap_update_bits(map, reg, mask, val, change, force);

	map->async = false;

	map->unlock(map->lock_arg);

	return ret;
}
EXPORT_SYMBOL_GPL(regmap_update_bits_base);

is taking the lock - under the hood probably uses regmap_lock_mutex() but
I haven't checked this specific config.

So you only tend to need your own locking if:
1) read modify write cycles occur that aren't handled by the simple regmap calls.
2) need to maintain consistency between some internal state and a register.
3) need to ensure a sequence of regmap accesses aren't interrupted.

Jonathan