Re: [PATCH 2/2] iio: light: vcnl4000: add regulator support

[Jonathan Cameron]

On Wed, 11 Mar 2026 14:22:36 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Wed, Mar 11, 2026 at 01:38:03PM +0200, Erikas Bitovtas wrote:
> > Add supply, I2C and cathode voltage regulators to the sensor and enable
> > them. This keeps the sensor powered on even after its only supply shared
> > by another device shuts down.  
> 
> > Signed-off-by: Erikas Bitovtas <xerikasxx@xxxxxxxxx>
> > Reported-by: Raymond Hackley <raymondhackley@xxxxxxxxxxxxxx>  
> 
> Where was it reported? Do you need Closes tag?
> 
> ...
> 
> > +#include "linux/array_size.h"
> > +#include "linux/regulator/consumer.h"  
> 
> Double quotes, huh?!
> 
> >  #include <linux/bitfield.h>
> >  #include <linux/module.h>
> >  #include <linux/i2c.h>  
> 
> Also, please keep the list ordered.
> 
> ...
> 
> >  	mutex_init(&data->vcnl4000_lock);
> > +	ret = devm_regulator_bulk_get_enable(&client->dev,
> > +				      ARRAY_SIZE(regulator_names),
> > +				      regulator_names);
> > +	if (ret < 0)
> > +		return ret;  
> 
> You can't add devm_ after non-devm calls.

This one happens to be fine because there is no cleanup of the
mutex_init(), so it is sort of not mixing devm and non devm.
That is kind of a historical thing where I for one wasn't convinced
it was worth the annoyance of mutex_destroy() until the devm
easy way of doing it came along.

Now, as the code is being touched anyway, I would
like that moved to
ret = devm_mutex_init();
if (ret)
	return ret;
as a precursor patch both as it makes it obvious we are still devm and
to get the advantage when lock debugging is turned on.

Thanks

Jonathan

> Also it would help you to have
> 
> 	struct device *dev = &client->dev;
> 
> at the top of the function.
> 
> ...
> 
> With the above being said, I expect a series out of two patches at least.
>