Re: [PATCH v7 2/2] iio: proximity: add driver for ST VL53L1X ToF sensor

[Jonathan Cameron]

On Thu, 26 Mar 2026 11:14:25 +0200
Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> wrote:

> On Wed, Mar 25, 2026 at 02:47:19PM +0000, Jonathan Cameron wrote:
> > On Wed, 25 Mar 2026 12:32:23 +0600
> > Siratul Islam <email@xxxxxxxx> wrote:  
> 
> ...
> 
> > > +		trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name,
> > > +					      iio_device_id(indio_dev));
> > > +		if (!trig)
> > > +			return -ENOMEM;
> > > +
> > > +		trig->ops = &vl53l1x_trigger_ops;
> > > +		iio_trigger_set_drvdata(trig, indio_dev);
> > > +		ret = devm_iio_trigger_register(dev, trig);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		indio_dev->trig = iio_trigger_get(trig);  
> > 
> > So Sashiko had a comment on this that has me thinking.  Don't bother fixing it
> > in this driver as it true it's a common bug (and leaks a trigger structure).
> > I'll look into it.  
> 
> This probably can be worked around by introducing a flag devm_allocated and set
> it in devm_iio_trigger_alloc(). If set, the release might automatically put the
> trigger at the error cases. 

(see below as your second point is key to what I'm saying here!)

Approximately what I had in mind, but not there - rather in a new
devm_iio_trigger_get() which would set a flag somewhere to say clean up needed.
The fiddly bit is when to clear it.  I think logically needs to be both:

1) When the trigger is changed as we put the trigger there anyway
2) (maybe implicitly) if the flag is still set on a devm callback registered
by devm_iio_trigger_get().

That devm callback is to ensure correct ordered tear down.  I really dislike
reasoning what happens if we do that stuff out of order.  

> BUT, do all drivers that allocate trigger actually
> bump its reference count?

Not all drivers set the default.  I actively discouraged this for a long time
due to it putting a strong preference for internal triggers when the
right choice is very usecase dependent.

The ones that set the default 'should' all use iio_trigger_get().
We had a whole bunch of fixes for this a long time back.

Anyhow, was complex enough and a common enough problem I wasn't
going to ask this contributor to deal with it.  I should have some time
in the next week or two to put together all the test cases needed to
make sure a solution works for this.

Jonathan

> 
> > > +		ret = vl53l1x_configure_irq(dev, client->irq, indio_dev);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> > > +						      &vl53l1x_trigger_handler,
> > > +						      NULL);
> > > +		if (ret)
> > > +			return ret;  
>