Re: [PATCH v3 4/5] iio: light: ROHM BU27008 color sensor

[Jonathan Cameron <jic23@kernel.org>]

> >   
> >> +
> >> +	if (i2c->irq) {
> >> +		ret = devm_iio_triggered_buffer_setup(dev, idev,
> >> +						      &iio_pollfunc_store_time,
> >> +						      bu27008_trigger_handler,
> >> +						      &bu27008_buffer_ops);
> >> +		if (ret)
> >> +			return dev_err_probe(dev, ret,
> >> +				     "iio_triggered_buffer_setup_ext FAIL\n");
> >> +
> >> +		itrig = devm_iio_trigger_alloc(dev, "%sdata-rdy-dev%d",
> >> +					       idev->name, iio_device_id(idev));
> >> +		if (!itrig)
> >> +			return -ENOMEM;
> >> +
> >> +		data->trig = itrig;
> >> +
> >> +		itrig->ops = &bu27008_trigger_ops;
> >> +		iio_trigger_set_drvdata(itrig, data);
> >> +
> >> +		name = devm_kasprintf(dev, GFP_KERNEL, "%s-bu27008",
> >> +				      dev_name(dev));
> >> +
> >> +		ret = devm_request_threaded_irq(dev, i2c->irq,
> >> +						iio_pollfunc_store_time,  
> > 
> > This is on the wrong irq.   
> 
> Seems like I have some homework to do :)
> 
> Right. I now see I pass the iio_pollfunc_store_time() as top half for 
> both the "real IRQ" generated by the device (here), as well as a 
> top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the 
> idea of taking the timestamp in the top half for the device-generated 
> IRQ as it is closer the moment HW did acquire the sample - but it really 
> would make no difference here (even if I did it correctly).

It's the matter of few function calls later.  So trivial timing wise.

> 
>   iio_pollfunc_store_time is used with the trigger not
> > here.  Basically what happens is the caller of iio_poll_trigger() fires the input
> > to a software irq chip that then signals all the of the downstream irqs (which
> > are the individual consumers of the triggers).  If that's triggered from the
> > top half / non threaded bit of the interrupt the iio_pollfunc_store_time()
> > will be called in that non threaded context before the individual threads
> > for the trigger consumer are started.  
> 
> Oh. So, you mean the iio_pollfunc_store_time() is automatically called 
> already before kicking the SW-IRQ? So we don't need it in 
> devm_iio_triggered_buffer_setup() anymore?

No.  Whatever you register as the top half of the poll_func in the call
to devm_iio_triggered_buffer_setup() will be called as part of the SW irq
chip handling - not this isn't a SW-IRQ at all, it's just some demultiplexing
code.  The top half of an IIO poll func runs in the the trigger interrupt handler
(under iio_trigger_poll) before those in turn start their own interrupt threads
to handle whatever you registered as the threads in devm_iio_trigger_buffer_setup()

> 
> > If there is nothing to do in the actual interrupt as it's a data ready
> > only signal, then you should just call iio_trigger_poll() in the top half and
> > use devm_request_irq() only as there is no thread in this interrupt (though
> > there is one for the interrupt below the software interrupt chip).  
> 
> I haven't tested this yet so please ignore me if I am writing nonsense - 
> but... The BU27008 will keep the IRQ line asserted until a register is 
> read. We can't read the register form HW-IRQ so we need to keep the IRQ 
> disabled until the threaded trigger handler is ran. With the setup we 
> have here, the IRQF_ONESHOT, took care of this. I assume that changing 
> to call the iio_poll_trigger() from top-half means I need to explicitly 
> disable the IRQ and re-enable it at the end of the trigger thread after 
> reading the register which debounces the IRQ line?

Hmm. I'm trying to remember how this works (wrote this a very long time ago).
I'm fairly sure it's not an issue because we use IRQF_ONESHOT down one level
so exercise the same prevention of the threads triggering multiple times etc.
https://elixir.bootlin.com/linux/latest/source/drivers/iio/buffer/industrialio-triggered-buffer.c#L57

It doesn't matter if the device interrupt fires again as it will still be masked
at our software irqchip level and will then get queued up and the thread will
run again.

> 
> > 
> >   
> >> +						&bu27008_irq_thread_handler,
> >> +						IRQF_ONESHOT, name, idev->pollfunc);
> >> +		if (ret)
> >> +			return dev_err_probe(dev, ret,
> >> +					     "Could not request IRQ\n");
> >> +
> >> +
> >> +		ret = devm_iio_trigger_register(dev, itrig);
> >> +		if (ret)
> >> +			return dev_err_probe(dev, ret,
> >> +					     "Trigger registration failed\n");
> >> +	} else {
> >> +		dev_warn(dev, "No IRQ configured\n");  
> > 
> > Why is it a warning?  Either driver works without an IRQ, or it doesn't.
> > dev_dbg() or dev_info() at most.  
> 
> Some of it works. Well, maybe I'll change it to tell that device works 
> in raw_read only mode.
> 
> Thanks again for the help!
> 
> Yours,
> 	-- Matti
>