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