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

[Matti Vaittinen]

On 5/13/23 20:52, Jonathan Cameron wrote:
> On Mon, 8 May 2023 09:32:28 +0300
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
>
> > Hi Jonathan,
> >
> > On 5/7/23 17:54, Jonathan Cameron wrote:
> > > On Wed, 3 May 2023 12:50:14 +0300
> > > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> > >    
> > > > The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> > > > and IR) with four configurable channels. Red and green being always
> > > > available and two out of the rest three (blue, clear, IR) can be
> > > > selected to be simultaneously measured. Typical application is adjusting
> > > > LCD backlight of TVs, mobile phones and tablet PCs.
> > > >
> > > > Add initial support for the ROHM BU27008 color sensor.
> > > >    - raw_read() of RGB and clear channels
> > > >    - triggered buffer w/ DRDY interrtupt
> > > >
> > > > Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
> > > >   
> > > Mostly stuff that you asked about in response to earlier version but
> > > which I hadn't replied to until today.
> > >
> > > Upshot, don't need the manual irq handling in here.
> > >
> > > Whilst you aren't setting IRQF_ONESHOT for the pollfunc side of the trigger
> > > (the downstream IRQ / IRQ thread) the IIO utility functions are.
> >
> > I tried doing:
> >
> > static int bu27008_setup_trigger(struct bu27008_data *data, struct
> > iio_dev *idev)
> > {
> > 	struct iio_trigger *itrig;
> > 	char *name;
> > 	int ret;
> >
> > 	ret = devm_iio_triggered_buffer_setup(data->dev, idev,
> > 					      &iio_pollfunc_store_time,
> > 					      bu27008_trigger_handler,
> > 					      &bu27008_buffer_ops);
> > 	if (ret)
> > 		return dev_err_probe(data->dev, ret,
> > 			     "iio_triggered_buffer_setup_ext FAIL\n");
> >
> > 	itrig = devm_iio_trigger_alloc(data->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(data->dev, GFP_KERNEL, "%s-bu27008",
> > 			      dev_name(data->dev));
> >
> > 	ret = devm_request_irq(data->dev, data->irq,
> > 				/* No IRQ disabling */
> > 			       &iio_trigger_generic_data_rdy_poll,
> > 			       0, name, itrig);
> > 	if (ret)
> > 		return dev_err_probe(data->dev, ret, "Could not request IRQ\n");
> >
> > 	ret = devm_iio_trigger_register(data->dev, itrig);
> > 	if (ret)
> > 		return dev_err_probe(data->dev, ret,
> > 				     "Trigger registration failed\n");
> >
> > 	/* set default trigger */
> > 	idev->trig = iio_trigger_get(itrig);
> >
> > 	return 0;
> > }
> >
> > It seems to me we get IRQ storm out of it, bu27008_trigger_handler never
> > being called. My assumption is that as soon as the IRQ handling code
> > exits the iio_trigger_generic_data_rdy_poll, it re-enables the IRQ - and
> > because we have level active IRQ and because the
> > bu27008_trigger_handler() has not yet had a chance to read the VALID bit
> > which restores the IRQ-line - we will immediately enter back to the IRQ
> > handling.
>
> Ah. I'd miss understood what was going on here. I thought we were talking
> race conditions only - not a level interrupt. Sorry for confusion / being
> half asleep. If it has an Ack like this I'd argue this is really an edge
> interrupt but that would require a guaranteed drop in the signal.

Yes. A failure to detect the edge (and skip acking) would leave the IRQ 
no longer working. I think we have both seen some examples of that in 
the past ;)

> I am assuming the sensor merrily carries on grabbing data, whether or
> not anyone reads it

This is also my understanding.

> and so if we treated this as an edge interrupt then
> the clear to set cycle could be very short (and hence not detected).
> If it instead doesn't read new data until previous has been read, then things
> are much simpler.

I think this is not how BU27008 works.

I think we could probably go on with edge IRQs and cook-up some "re-read 
the VALID-bit again after the IRQ is for sure enabled to ensure the IRQ 
does not go unasserted" - mechanism, which would work on 99.99% of the 
cases. Problem is that some device always handles the 10000th 
measurement ;) To tell the truth, I never really thought of that.

> Hmm. How to make this work cleanly assuming it's case 1. It might be that your
> current approach is the best though it would be nice to do something in the
> IIO code (with risk of breaking everyone  :()

I didn't check if this would be doable.

  I don't think we can though
> as we have no way from the trigger implementation side to know if we might
> get threaded interrupt handling or not on the downstream side.
>
> We have reference counting to reenable a trigger that actually has a hardware
> mask at the device end when all consumers are done - that should be used for
> the reenable, not do it in the pollfunc handler.  As it's a level interrupt
> you avoid need to do a bonus read in there I think (sometimes that's necessary
> because of an edge trigger and a slow read back on a possible unrelated device).
>
> The subtle difference between IRQF_ONESHOT and irq_disable is one uses
> the irq_mask / unmask callbacks on the irq chip and the other is using
> the enable / disable ones.  That may make no practical difference - I'm not
> entirely sure.  A quick glance at some drivers suggests masking is usually
> lighter weight as less state is rewrite on reenable.
>
> So in short, move the irq_enable() into the iio_trig->reenable() callback.

This should be what I did at v5 :) Thanks for the help!

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~