On Mon, 8 May 2023 13:39:29 +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>
>
> ---
>
> 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;
> +}
