Message-ID: <20230513185236.39bbface@jic23-huawei>
Date:   Sat, 13 May 2023 18:52:36 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Matti Vaittinen <mazziesaccount@...il.com>
Cc:     Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 4/5] iio: light: ROHM BU27008 color sensor

On Mon, 8 May 2023 09:32:28 +0300
Matti Vaittinen <mazziesaccount@...il.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@...il.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@...il.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.
I am assuming the sensor merrily carries on grabbing data, whether or
not anyone reads it 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.

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 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 problem does not occur when I use bu27008_data_rdy_poll() (which is 
> the same but disables the IRQ) instead of 
> iio_trigger_generic_data_rdy_poll(), and re-enable the IRQ only after 
> the handler bu27008_trigger_handler() has restored the IRQ line.
> 
> Does the sequence above (bu27008_setup_trigger()) look sane?
> 
> > 
> >   
> >> +static irqreturn_t bu27008_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *idev = pf->indio_dev;
> >> +	struct bu27008_data *data = iio_priv(idev);
> >> +	struct {
> >> +		__le16 chan[BU27008_NUM_HW_CHANS];
> >> +		s64 ts __aligned(8);
> >> +	} raw;
> >> +	int ret, dummy;
> >> +
> >> +	memset(&raw, 0, sizeof(raw));
> >> +
> >> +	/*
> >> +	 * After some measurements, it seems reading the
> >> +	 * BU27008_REG_MODE_CONTROL3 debounces the IRQ line
> >> +	 */
> >> +	ret = regmap_read(data->regmap, BU27008_REG_MODE_CONTROL3, &dummy);
> >> +	if (ret < 0)
> >> +		goto err_read;
> >> +
> >> +	ret = regmap_bulk_read(data->regmap, BU27008_REG_DATA0_LO, &raw.chan,
> >> +			       sizeof(raw.chan));
> >> +	if (ret < 0)
> >> +		goto err_read;
> >> +
> >> +	iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
> >> +err_read:
> >> +	iio_trigger_notify_done(idev->trig);
> >> +
> >> +	enable_irq(data->irq);  
> > 
> > As below. This shouldn't be needed (and if it was it should be in the
> > reenable path that is ultimately a result of that notify_done above and
> > some reference counting fun).  
> 
> I will see the reenable callback, thanks!

Great

> 
> >   
> >> +
> >> +	return IRQ_HANDLED;
> >> +}
> >> +
> >> +static int bu27008_buffer_preenable(struct iio_dev *idev)
> >> +{
> >> +	struct bu27008_data *data = iio_priv(idev);
> >> +	int chan_sel, ret;
> >> +
> >> +	/* Configure channel selection */
> >> +	if (test_bit(BU27008_BLUE, idev->active_scan_mask)) {
> >> +		if (test_bit(BU27008_CLEAR, idev->active_scan_mask))
> >> +			chan_sel = BU27008_BLUE2_CLEAR3;
> >> +		else
> >> +			chan_sel = BU27008_BLUE2_IR3;
> >> +	} else {
> >> +		chan_sel = BU27008_CLEAR2_IR3;
> >> +	}
> >> +
> >> +	chan_sel = FIELD_PREP(BU27008_MASK_CHAN_SEL, chan_sel);
> >> +
> >> +	ret = regmap_update_bits(data->regmap, BU27008_REG_MODE_CONTROL3,
> >> +				 BU27008_MASK_CHAN_SEL, chan_sel);
> >> +	if (ret)
> >> +		return ret;  
> > 
> > Hmm. I'd missed this before but. This is in the wrong place really
> > (though it probably doesn't make much difference), stuff related to
> > enabling particular channels should be in iio_info->update_scan_mode()  
> 
> Oh. I'll check this out as well.
> 
> > 
> > It's arguable that the actual measurement mode setting might come
> > in the postenable callback (after the update_scan_mode() call which
> > in turn follows the preenable callback).
> > 
> > All these callbacks have become a bit blurry over time as we end
> > up with devices that need to do nasty thing in one place.  In this
> > particular case it's pretty simple though, so nicer to move
> > the scan mask stuff to the callback that is given the active_scan
> > mask as a parameter.
> >   
> >> +
> >> +	return bu27008_meas_set(data, BU27008_MEAS_EN);
> >> +}
> >> +
> >> +static int bu27008_buffer_postdisable(struct iio_dev *idev)
> >> +{
> >> +	struct bu27008_data *data = iio_priv(idev);
> >> +
> >> +	return bu27008_meas_set(data, BU27008_MEAS_DIS);
> >> +}
> >> +
> >> +static const struct iio_buffer_setup_ops bu27008_buffer_ops = {
> >> +	.preenable = bu27008_buffer_preenable,
> >> +	.postdisable = bu27008_buffer_postdisable,
> >> +};
> >> +
> >> +static irqreturn_t bu27008_data_rdy_poll(int irq, void *private)
> >> +{
> >> +	/*
> >> +	 * The BU27008 keeps IRQ asserted until we read the VALID bit from
> >> +	 * a register. We need to keep the IRQ disabled until this
> >> +	 */
> >> +	disable_irq_nosync(irq);  
> > 
> > As per my late reply to your question on this, shouldn't be needed
> > as IRQF_ONESHOT is ultimately set for the interrupts nested below this
> > so they'll get the resulting queuing on the threads which is fine.  
> 
> I see an IRQ storm if I omit this. The threaded trigger handler which 
> 'ACKs' the IRQ gets never ran. I'll see the reenable though! Thanks!

As you probably figured, I was wrong. Reenable bit stands though!.

Jonathan

> 
> Yours,
> 	-- Matti
> 

Powered by blists - more mailing lists