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

[Matti Vaittinen]

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.

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!

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

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~