Messages in this thread | | | Date | Mon, 15 May 2023 15:31:42 +0300 | From | Matti Vaittinen <> | Subject | Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor |
| |
On 5/13/23 20:52, Jonathan Cameron wrote: > On Mon, 8 May 2023 09:32:28 +0300 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> Hi Jonathan, >> >> On 5/7/23 17:54, Jonathan Cameron wrote: >>> On Wed, 3 May 2023 12:50:14 +0300 >>> Matti Vaittinen <mazziesaccount@gmail.com> 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@gmail.com> >>>> >>> 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! ~~
| |