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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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