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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 8 May 2023 13:39:29 +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>
> 
> ---
> 
> Please, check the way trigger handling is now implemented. I've verified
> it works for one user-space user, but I am no longer confident on how
> the triggers are intended to be used.
> 
> When testing the trigger passing iio_trigger_generic_data_rdy_poll() as
> a handler in devm_request_irq() I saw an IRQ storm. The threaded handler
> given in devm_iio_triggered_buffer_setup() [bu27008_trigger_handler()] got
> never called and system hung just looping in
> iio_trigger_generic_data_rdy_poll(). Hence, this version disables the
> IRQ in the handler registered at devm_request_irq(), before cascading
> into iio_trigger_poll(). IRQ is now re-enabled in trigger's .reenable
> callback. Feedback is appreciated!

What you have makes sense to me.

A few trivial things inline I'll tidy up whilst applying.


> +static int bu27008_trigger_set_state(struct iio_trigger *trig,
> +				     bool state)
> +{
> +	struct bu27008_data *data = iio_trigger_get_drvdata(trig);
> +	int ret = 0;

No need to initialize ret.

> +
> +	if (state)
> +		ret = bu27008_set_drdy_irq(data, BU27008_INT_EN);
> +	else
> +		ret = bu27008_set_drdy_irq(data, BU27008_INT_DIS);
> +	if (ret)
> +		dev_err(data->dev, "Failed to set trigger state\n");
> +
> +	return ret;
> +}
> +
> +static void bu27008_trigger_reenable(struct iio_trigger *trig)
> +{
> +	struct bu27008_data *data = iio_trigger_get_drvdata(trig);
> +
> +	enable_irq(data->irq);
> +}


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

Half sentence.   If this is all that comes up I'll change it to.

We need to keep the IRQ disable until then.

> +	 */
> +	disable_irq_nosync(irq);
> +	iio_trigger_poll(private);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +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,
> +			       &bu27008_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;
> +}