On 31/01/2025 19:41, Jonathan Cameron wrote:
On Fri, 31 Jan 2025 15:37:48 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
an automatic measurement mode, with an alarm interrupt for out-of-window
measurements. The window is configurable for each channel.
Hi Jonathan,
I just sent the v2, where I (think I) addressed all comments except ones
below. Just wanted to point out what was not changed and why :)
...
+struct bd79124_raw {
+ u8 bit0_3; /* Is set in high bits of the byte */
+ u8 bit4_11;
+};
+#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
You could do this as an endian conversion and a single shift I think.
Might be slightly simpler.
I kept this struct with bytes matching the register spec. Doing the
endian conversion and then shifting would probably have worked, but my
head hurts when I try thinking how the bits settle there. Especially if
this is done on a big-endian machine. I can rework this for v3 if you
feel very strongly about this.
...
+static irqreturn_t bd79124_event_handler(int irq, void *priv)
+{
+ int ret, i_hi, i_lo, i;
+ struct iio_dev *idev = priv;
+ struct bd79124_data *d = iio_priv(idev);
+
+ /*
+ * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
+ * subsystem to disable the offending IRQ line if we get a hardware
+ * problem. This behaviour has saved my poor bottom a few times in the
+ * past as, instead of getting unusably unresponsive, the system has
+ * spilled out the magic words "...nobody cared".
*laughs*. Maybe the comment isn't strictly necessary but it cheered
up my Friday.
+ */
+ ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
+ if (ret)
+ return IRQ_NONE;
+
+ ret = regmap_read(d->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
+ if (ret)
+ return IRQ_NONE;
+
+ if (!i_lo && !i_hi)
+ return IRQ_NONE;
+
+ for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+ u64 ecode;
+
+ if (BIT(i) & i_hi) {
Maybe cleaner as a pair of
for_each_set_bit() loops.
I kept the original for 2 reasons.
1. the main reason is that the for_each_set_bit() would want the value
read from a register to be in long. Regmap wants to use int. Solving
this produced (in my 'humblish' opinion) less readable code.
2. The current implementation has only one loop, which should perhaps be
a tiny bit more efficient.
+ ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+ IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
+
+ iio_push_event(idev, ecode, d->timestamp);
+ /*
+ * The BD79124 keeps the IRQ asserted for as long as
+ * the voltage exceeds the threshold. It may not serve
+ * the purpose to keep the IRQ firing and events
+ * generated in a loop because it may yield the
+ * userspace to have some problems when event handling
+ * there is slow.
+ *
+ * Thus, we disable the event for the channel. Userspace
+ * needs to re-enable the event.
That's not pretty. So I'd prefer a timeout and autoreenable if we can.
And I did this, but with constant 1 sec 'grace time' instead of
modifiable time-out. I believe this suffices and keeps it simpler.
Yours,
-- Matti
