On 02/03/2025 06:10, Jonathan Cameron wrote:

On Mon, 24 Feb 2025 20:34:30 +0200
Matti Vaittinen [off-list ref] wrote:

quoted

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@gmail.com>

Some minor stuff inline.

...

quoted

+#define BD79124_INTERVAL_075 0

Can we make these units in these explicit?
#define BD79124_INTERVAL_MS_0_75
maybe?  Nice to avoid need for comments on what the units are where
you use these.

Sure, thanks.

quoted

+#define BD79124_INTERVAL_150 1
+#define BD79124_INTERVAL_300 2
+#define BD79124_INTERVAL_600 3

...

quoted

+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);

No brackets around the ~evbit.
Check for other cases of this.
Otherwise we'll get some script written 'cleanup'.

Sigh. I had a lengthy discussion about this with Andy explaining why I 
like having the parenthesis to avoid any confusion. Well, I suppose I 
have no options if you're strongly opposing them.

...

quoted

+static void bd79124_alm_enable_worker(struct work_struct *work)
+{
+	int i;
+	struct bd79124_data *data = container_of(work, struct bd79124_data,
+						 alm_enable_work.work);
+
+	guard(mutex)(&data->mutex);
+	/*
+	 * We should not re-enable the event if user has disabled it while
+	 * rate-limiting was enabled.
+	 */

Is this comment suggesting something that isn't done or referring to specific
code?  I think it wants to be in the function above where the decision is made.

I have to take another look but it seems it got misplaced during the 
road. Thanks!

Agreeing with all the rest, thanks!

Yours,
	-- Matti