On 08/02/2025 18:52, Jonathan Cameron wrote:
On Wed, 5 Feb 2025 15:38:16 +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.
The I2C protocol for manual start of the measurement and data reading is
somewhat peculiar. It requires the master to do clock stretching after
sending the I2C slave-address until the slave has captured the data.
Needless to say this is not well suopported by the I2C controllers.
Thus the driver does not support the BD79124's manual measurement mode
but implements the measurements using automatic measurement mode relying
on the BD79124's ability of storing latest measurements into register.
The driver does also support configuring the threshold events for
detecting the out-of-window events.
The BD79124 keeps asserting IRQ for as long as the measured voltage is
out of the configured window. Thus the driver masks the received event
for a fixed duration (1 second) when an event is handled. This prevents
the user-space from choking on the events
The ADC input pins can be also configured as general purpose outputs.
Those pins which don't have corresponding ADC channel node in the
device-tree will be controllable as GPO.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Hi Matti,
Just a few really trivial comments though this wasn't my most thorough
of reviews as ran out of time / energy today!
Thanks :) I appreciate the time and energy invested here :)
diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
new file mode 100644
index 000000000000..ea93762a24cc
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79124.c
@@ -0,0 +1,1149 @@
+static int bd79124_write_event_value(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info, int val,
+ int val2)
+{
+ struct bd79124_data *data = iio_priv(iio_dev);
+ int reg;
+
+ if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+ return -EINVAL;
+
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ if (dir == IIO_EV_DIR_RISING) {
+ guard(mutex)(&data->mutex);
+
+ data->alarm_r_limit[chan->channel] = val;
+ reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
+ } else if (dir == IIO_EV_DIR_FALLING) {
+ guard(mutex)(&data->mutex);
+
+ data->alarm_f_limit[chan->channel] = val;
+ reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
+ } else {
+ return -EINVAL;
+ }
+ /*
+ * We don't want to enable the alarm if it is not enabled or
+ * if it is suppressed. In that case skip writing to the
+ * register.
+ */
+ if (!(data->alarm_monitored[chan->channel] & BIT(dir)) ||
+ data->alarm_suppressed[chan->channel] & BIT(dir))
+ return 0;
+
+ return bd79124_write_int_to_reg(data, reg, val);
+
+ case IIO_EV_INFO_HYSTERESIS:
+ reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+ val >>= 3;
Odd indent.
Oh, indeed. Thanks!
+
+ return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
+ val);
+ default:
+ return -EINVAL;
+ }
+}
+static void bd79124_re_enable_lo(struct bd79124_data *data, unsigned int channel)
+{
+ int ret, evbit = BIT(IIO_EV_DIR_FALLING);
+
+ if (!(data->alarm_suppressed[channel] & evbit))
+ return;
+
+ data->alarm_suppressed[channel] &= (~evbit);
+
+ if (!(data->alarm_monitored[channel] & evbit))
+ return;
+
+ ret = bd79124_write_int_to_reg(data, BD79124_GET_LOW_LIMIT_REG(channel),
+ data->alarm_f_limit[channel]);
+ if (ret)
+ dev_warn(data->dev, "Low limit enabling failed for channel%d\n",
+ channel);
+}
+
+static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
+{
+ int ret, evbit = BIT(IIO_EV_DIR_RISING);
+
+ if (!(data->alarm_suppressed[channel] & evbit))
+ return;
+
+ data->alarm_suppressed[channel] &= (~evbit);
+
+ if (!(data->alarm_monitored[channel] & evbit))
+ return;
This lot is very similar to the lo variant. Can we combine them or
use some helper for both?
Initially I did this.
But the code looked a bit dull because the evbitm, alarm-limit array and
prints depend on the direction. Furthermore, the caller already knows
the direction (as the caller does also handle directions separately), so
doing:
foo(dir)
{
if (dir == bar)
...
else
...
}
...
if (dir == bar)
foo(dir);
else
foo(dir);
started to feel just a bit, meh. Hence I separated the stuff to own _lo
and _hi functions.
+
+ ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel),
+ data->alarm_r_limit[channel]);
+ if (ret)
+ dev_warn(data->dev, "High limit enabling failed for channel%d\n",
+ channel);
+}
Yours,
-- Matti
