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! ~~
