lkml.org 
[lkml]   [2023]   [May]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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! ~~

\
 
 \ /
  Last update: 2023-05-15 14:32    [W:0.057 / U:0.664 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site