On 5/1/23 17:20, Jonathan Cameron wrote:
> On Wed, 26 Apr 2023 11:08:17 +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>
>
> Hi Matti,
>
> Mostly trivial stuff, but some confusion has occurred with respect to the
> two interrupts involve in an IIO trigger. Specifically the pollfunc stuff
> occurs on the downward side of the irqchip that hides on the consumer side
> of an IIO trigger) and is set up by devm_iio_trigered_buffer_setup() not
> of the interrupt that calls iio_trigger_poll[_nested]()
>
>
>> +
>> +static int bu27008_try_find_new_time_gain(struct bu27008_data *data, int val,
>> + int val2, int *gain_sel)
>> +{
>> + /* Could not support new scale with existing int-time */
>
> I'd move that above the function and change it to
> /* Called if the new scale could not be supported with existing int-time */
> Down here it is not clear that this applies to the whole funciton.
>
>
>> + int i, ret, new_time_sel;
>> +
>> + for (i = 0; i < data->gts.num_itime; i++) {
>> + new_time_sel = data->gts.itime_table[i].sel;
>> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
>> + new_time_sel, val, val2 * 1000, gain_sel);
>> + if (!ret)
>> + break;
>> + }
>> + if (i == data->gts.num_itime) {
>> + dev_err(data->dev, "Can't support scale %u %u\n", val,
>> + val2);
>
> Line wrapping inconsistent. I like short lines with appropriate flexibility where
> longer ones are more readable. However, I am fairly sure this one fits under 80
> chars as a single line.
Thanks! Both the comment and the line-wrapping were just left like this
when I pulled this part of a functionality into this new function. Well
spotted!
>
>> +
>> + return -EINVAL;
>> + }
>> +
>> + return bu27008_set_int_time_sel(data, new_time_sel);
>> +}
>> +
>> +static int bu27008_set_scale(struct bu27008_data *data,
>> + struct iio_chan_spec const *chan,
>> + int val, int val2)
>> +{
>> + int ret, gain_sel, time_sel;
>> +
>> + if (chan->scan_index == BU27008_IR)
>> + return -EINVAL;
>> +
>> + mutex_lock(&data->mutex);
>> +
>> + ret = bu27008_get_int_time_sel(data, &time_sel);
>> + if (ret < 0)
>> + goto unlock_out;
>> +
>> + ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts, time_sel,
>> + val, val2 * 1000, &gain_sel);
>> + if (ret)
>> + ret = bu27008_try_find_new_time_gain(data, val, val2, &gain_sel);
>
> Obviously it is code that doesn't make any functional difference, but I'd prefer to see
> if (ret) {
> ret = bu27....
> if (ret)
> goto unlock_out;
> }
> ret = bu27008_write_gain_sel();
>
> so that each error path is out of line, but the good path is the linear flow.
... Yuck! The difference of tastes ... ;)
Well, not worth fighting I guess.
>
>> +
>> + if (!ret)
>> + ret = bu27008_write_gain_sel(data, gain_sel);
>> +
>> +unlock_out:
>> + mutex_unlock(&data->mutex);
>> +
>> + return ret;
>> +}
>
>
>> +static irqreturn_t bu27008_irq_thread_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);
>> +
>> + iio_trigger_poll_nested(data->trig);
>
> See below but this is what alerted me to something unusual.
> It never makes sense to have iio_trigger_poll_nested() called unless
> there is a check on whether it should be called! If there isn't
> iio_trigger_poll() in the top half is the right thing to do. >
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>
>
>> +static int bu27008_probe(struct i2c_client *i2c)
>> +{
>
> ...
>
>> +
>> + if (i2c->irq) {
>> + ret = devm_iio_triggered_buffer_setup(dev, idev,
>> + &iio_pollfunc_store_time,
>> + bu27008_trigger_handler,
>> + &bu27008_buffer_ops);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "iio_triggered_buffer_setup_ext FAIL\n");
>> +
>> + itrig = devm_iio_trigger_alloc(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(dev, GFP_KERNEL, "%s-bu27008",
>> + dev_name(dev));
>> +
>> + ret = devm_request_threaded_irq(dev, i2c->irq,
>> + iio_pollfunc_store_time,
>
> This is on the wrong irq.
Seems like I have some homework to do :)
Right. I now see I pass the iio_pollfunc_store_time() as top half for
both the "real IRQ" generated by the device (here), as well as a
top-half for the devm_iio_triggered_buffer_setup(). Ideally I like the
idea of taking the timestamp in the top half for the device-generated
IRQ as it is closer the moment HW did acquire the sample - but it really
would make no difference here (even if I did it correctly).
iio_pollfunc_store_time is used with the trigger not
> here. Basically what happens is the caller of iio_poll_trigger() fires the input
> to a software irq chip that then signals all the of the downstream irqs (which
> are the individual consumers of the triggers). If that's triggered from the
> top half / non threaded bit of the interrupt the iio_pollfunc_store_time()
> will be called in that non threaded context before the individual threads
> for the trigger consumer are started.
Oh. So, you mean the iio_pollfunc_store_time() is automatically called
already before kicking the SW-IRQ? So we don't need it in
devm_iio_triggered_buffer_setup() anymore?
> If there is nothing to do in the actual interrupt as it's a data ready
> only signal, then you should just call iio_trigger_poll() in the top half and
> use devm_request_irq() only as there is no thread in this interrupt (though
> there is one for the interrupt below the software interrupt chip).
I haven't tested this yet so please ignore me if I am writing nonsense -
but... The BU27008 will keep the IRQ line asserted until a register is
read. We can't read the register form HW-IRQ so we need to keep the IRQ
disabled until the threaded trigger handler is ran. With the setup we
have here, the IRQF_ONESHOT, took care of this. I assume that changing
to call the iio_poll_trigger() from top-half means I need to explicitly
disable the IRQ and re-enable it at the end of the trigger thread after
reading the register which debounces the IRQ line?
>
>
>> + &bu27008_irq_thread_handler,
>> + IRQF_ONESHOT, name, idev->pollfunc);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Could not request IRQ\n");
>> +
>> +
>> + ret = devm_iio_trigger_register(dev, itrig);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Trigger registration failed\n");
>> + } else {
>> + dev_warn(dev, "No IRQ configured\n");
>
> Why is it a warning? Either driver works without an IRQ, or it doesn't.
> dev_dbg() or dev_info() at most.
Some of it works. Well, maybe I'll change it to tell that device works
in raw_read only mode.
Thanks again for the help!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
