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

[Matti Vaittinen <mazziesaccount@gmail.com>]

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.

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