[PATCH 0/3] iio: adc: palmas_gpadc: add iio events

[PATCH 0/3] iio: adc: palmas_gpadc: add iio events

[Patrik Dahlström]

These changes are based on [1] and [2].

The palmas gpadc block has support for monitoring up to 2 ADC channels
and issue an interrupt if they reach past a set threshold. This can be
configured statically with device tree todayi, but it only gets enabled
when reaching sleep mode. Also, it doesn't look like anyone is using it.

Instead of this one special case, change the code so userspace can
configure the ADC channels to their own needs through the iio events
subsystem.

Thresholds and events were tested on omap5-uevm board. It should still
be possible to wake up from sleep mode on events, but my board don't
like sleep. A userspace tool for monitoring events and adjusting
thresholds can be found at [3].

[1] https://patchwork.kernel.org/project/linux-iio/patch/20230318163039.56115-1-jic23@kernel.org/
[2] https://patchwork.kernel.org/project/linux-iio/patch/20230313205029.1881745-1-risca@dalakolonin.se/
[3] https://github.com/Risca/pyra_vol_mon


Patrik Dahlström (3):
  iio: adc: palmas_gpadc: add support for iio threshold events
  iio: adc: palmas_gpadc: remove adc_wakeupX_data
  iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property

 drivers/iio/adc/palmas_gpadc.c | 527 +++++++++++++++++++++++++++------
 include/linux/mfd/palmas.h     |   8 -
 2 files changed, 434 insertions(+), 101 deletions(-)


base-commit: 37fd83916da2e4cae03d350015c82a67b1b334c4
prerequisite-patch-id: 9b1f55610800b91b721d042bf7f33b58179237d1
prerequisite-patch-id: b0418c707db13f514400956596e9ebe91c25bba0
-- 
2.25.1

[PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[Patrik Dahlström]

The palmas gpadc block has support for monitoring up to 2 ADC channels
and issue an interrupt if they reach past a set threshold. The gpadc
driver had limited support for this through the adc_wakeup{1,2}_data
platform data. This however only allow a fixed threshold to be set at
boot, and would only enable it when entering sleep mode.

This change hooks into the IIO events system and exposes to userspace
the ability to configure threshold values for each channel individually,
but only allow up to 2 such thresholds to be enabled at any given time.

The logic around suspend and resume had to be adjusted so that user
space configuration don't get reset on resume. Instead, any configured
adc auto wakeup gets enabled during probe.

Enabling a threshold from userspace will overwrite the adc wakeup
configuration set during probe. Depending on how you look at it, this
could also mean we allow userspace to update the adc wakeup thresholds.

Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
---
 drivers/iio/adc/palmas_gpadc.c | 495 +++++++++++++++++++++++++++++----
 1 file changed, 438 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 24d7c096e4b8..84c6e3b66205 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -20,6 +20,7 @@
 #include <linux/completion.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/iio/events.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
@@ -76,6 +77,16 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = {
 	PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true),
 };
 
+struct palmas_gpadc_thresholds {
+	int high_thresh;
+	int low_thresh;
+};
+
+struct palmas_adc_event {
+	int channel;
+	enum iio_event_direction direction;
+};
+
 /*
  * struct palmas_gpadc - the palmas_gpadc structure
  * @ch0_current:	channel 0 current source setting
@@ -117,8 +128,30 @@ struct palmas_gpadc {
 	bool				wakeup2_enable;
 	int				auto_conversion_period;
 	struct mutex			lock;
+	struct palmas_adc_event		event0;
+	struct palmas_adc_event		event1;
+	struct palmas_gpadc_thresholds	thresh_data[PALMAS_ADC_CH_MAX];
 };
 
+static struct palmas_adc_event *palmas_gpadc_get_event_channel(
+	struct palmas_gpadc *adc, int adc_chan, enum iio_event_direction dir)
+{
+	if (adc_chan == adc->event0.channel && dir == adc->event0.direction)
+		return &adc->event0;
+
+	if (adc_chan == adc->event1.channel && dir == adc->event1.direction)
+		return &adc->event1;
+
+	return NULL;
+}
+
+static bool palmas_gpadc_channel_is_freerunning(struct palmas_gpadc *adc,
+						int adc_chan)
+{
+	return palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_RISING) ||
+		palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_FALLING);
+}
+
 /*
  * GPADC lock issue in AUTO mode.
  * Impact: In AUTO mode, GPADC conversion can be locked after disabling AUTO
@@ -188,11 +221,24 @@ static irqreturn_t palmas_gpadc_irq(int irq, void *data)
 
 static irqreturn_t palmas_gpadc_irq_auto(int irq, void *data)
 {
-	struct palmas_gpadc *adc = data;
+	struct iio_dev *indio_dev = data;
+	struct palmas_gpadc *adc = iio_priv(indio_dev);
+	struct palmas_adc_event *ev;
 
 	dev_dbg(adc->dev, "Threshold interrupt %d occurs\n", irq);
 	palmas_disable_auto_conversion(adc);
 
+	ev = (irq == adc->irq_auto_0) ? &adc->event0 : &adc->event1;
+	if (ev->channel != -1) {
+		enum iio_event_direction dir;
+		u64 code;
+
+		dir = ev->direction;
+		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, ev->channel,
+					    IIO_EV_TYPE_THRESH, dir);
+		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
+	}
+
 	return IRQ_HANDLED;
 }
 
@@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
 {
 	int ret;
 
+	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
+		return 0; // ADC already running
+
 	ret = palmas_gpadc_enable(adc, adc_chan, true);
 	if (ret < 0)
 		return ret;
@@ -339,28 +388,44 @@ static int palmas_gpadc_start_conversion(struct palmas_gpadc *adc, int adc_chan)
 	unsigned int val;
 	int ret;
 
-	init_completion(&adc->conv_completion);
-	ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
-				PALMAS_GPADC_SW_SELECT,
-				PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
-				PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
-	if (ret < 0) {
-		dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
-		return ret;
-	}
+	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan)) {
+		int event = (adc_chan == adc->event0.channel) ? 0 : 1;
+		unsigned int reg = (event == 0) ?
+			PALMAS_GPADC_AUTO_CONV0_LSB :
+			PALMAS_GPADC_AUTO_CONV1_LSB;
 
-	ret = wait_for_completion_timeout(&adc->conv_completion,
-				PALMAS_ADC_CONVERSION_TIMEOUT);
-	if (ret == 0) {
-		dev_err(adc->dev, "conversion not completed\n");
-		return -ETIMEDOUT;
+		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
+					reg, &val, 2);
+		if (ret < 0) {
+			dev_err(adc->dev, "AUTO_CONV%x_LSB read failed: %d\n",
+				event, ret);
+			return ret;
+		}
 	}
+	else {
+		init_completion(&adc->conv_completion);
+		ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
+					PALMAS_GPADC_SW_SELECT,
+					PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
+					PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
+		if (ret < 0) {
+			dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
+			return ret;
+		}
 
-	ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
-				PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
-	if (ret < 0) {
-		dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
-		return ret;
+		ret = wait_for_completion_timeout(&adc->conv_completion,
+					PALMAS_ADC_CONVERSION_TIMEOUT);
+		if (ret == 0) {
+			dev_err(adc->dev, "conversion not completed\n");
+			return -ETIMEDOUT;
+		}
+
+		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
+					PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
+		if (ret < 0) {
+			dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
+			return ret;
+		}
 	}
 
 	ret = val & 0xFFF;
@@ -385,6 +450,80 @@ static int palmas_gpadc_get_calibrated_code(struct palmas_gpadc *adc,
 	return val;
 }
 
+static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
+					       struct palmas_adc_event *ev)
+{
+	const int INL = 2;
+	const int adc_chan = ev->channel;
+	const int orig = adc->thresh_data[adc_chan].high_thresh;
+	int val = orig;
+	int gain_drift;
+	int offset_drift;
+
+	if (!val)
+		return 0;
+
+	val = (val * 1000) / adc->adc_info[adc_chan].gain;
+
+	if (!adc->adc_info[adc_chan].is_uncalibrated) {
+		val = (val * adc->adc_info[adc_chan].gain_error +
+		       adc->adc_info[adc_chan].offset) /
+			1000;
+		gain_drift = 1002;
+		offset_drift = 2;
+	}
+	else {
+		gain_drift = 1022;
+		offset_drift = 36;
+	}
+
+	// add tolerance to threshold
+	val = ((val + INL) * gain_drift) / 1000 + offset_drift;
+
+	// clamp to max possible value
+	if (val > 0xFFF)
+		val = 0xFFF;
+
+	return val;
+}
+
+static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
+					      struct palmas_adc_event *ev)
+{
+	const int INL = 2;
+	const int adc_chan = ev->channel;
+	const int orig = adc->thresh_data[adc_chan].low_thresh;
+	int val = orig;
+	int gain_drift;
+	int offset_drift;
+
+	if (!val)
+		return val;
+
+	val = (val * 1000) / adc->adc_info[adc_chan].gain;
+
+        if (!adc->adc_info[adc_chan].is_uncalibrated) {
+            val = (val * adc->adc_info[adc_chan].gain_error -
+		   adc->adc_info[adc_chan].offset) /
+		    1000;
+            gain_drift = 998;
+            offset_drift = 2;
+        }
+        else {
+            gain_drift = 978;
+            offset_drift = 36;
+        }
+
+	// calculate tolerances
+	val = ((val - INL) * gain_drift) / 1000 - offset_drift;
+
+	// clamp to minimum 0
+	if (val < 0)
+		val = 0;
+
+	return val;
+}
+
 static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
 	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
 {
@@ -431,8 +570,239 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
 	return ret;
 }
 
+static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir)
+{
+	struct palmas_gpadc *adc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	mutex_lock(&adc->lock);
+
+	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir)) {
+		ret = 1;
+	}
+
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
+static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
+				       struct palmas_adc_event *ev,
+				       struct palmas_adc_wakeup_property *wakeup)
+{
+	wakeup->adc_channel_number = ev->channel;
+	if (ev->direction == IIO_EV_DIR_RISING) {
+		wakeup->adc_low_threshold = 0;
+		wakeup->adc_high_threshold =
+			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
+	}
+	else {
+		wakeup->adc_low_threshold =
+			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
+		wakeup->adc_high_threshold = 0;
+	}
+}
+
+static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
+static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
+
+static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
+{
+	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
+	bool enable;
+
+	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
+	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
+
+	enable = adc->wakeup1_enable || adc->wakeup2_enable;
+	if (!was_enabled && enable)
+		device_wakeup_enable(adc->dev);
+	else if (was_enabled && !enable)
+		device_wakeup_disable(adc->dev);
+
+	if (!enable)
+		return palmas_adc_wakeup_reset(adc);
+
+	// adjust levels
+	if (adc->wakeup1_enable)
+		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
+	if (adc->wakeup2_enable)
+		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
+
+	return palmas_adc_wakeup_configure(adc);
+}
+
+static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
+	const struct iio_chan_spec *chan, enum iio_event_direction dir)
+{
+	struct palmas_adc_event *ev;
+	int adc_chan = chan->channel;
+
+	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
+		/* already enabled */
+		return 0;
+
+	if (adc->event0.channel == -1)
+		ev = &adc->event0;
+	else if (adc->event1.channel == -1) {
+		/* event0 has to be the lowest channel */
+		if (adc_chan < adc->event0.channel) {
+			adc->event1 = adc->event0;
+			ev = &adc->event0;
+		}
+		else
+			ev = &adc->event1;
+	}
+	else /* both AUTO channels already in use */ {
+		dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
+			 adc->event0.channel, adc->event1.channel);
+		return -EBUSY;
+	}
+
+	ev->channel = adc_chan;
+	ev->direction = dir;
+
+	return palmas_gpadc_reconfigure_event_channels(adc);
+}
+
+static int palmas_gpadc_disable_event_config(struct palmas_gpadc *adc,
+	const struct iio_chan_spec *chan, enum iio_event_direction dir)
+{
+	int adc_chan = chan->channel;
+	struct palmas_adc_event *ev =
+		palmas_gpadc_get_event_channel(adc, adc_chan, dir);
+
+	if (!ev)
+		return 0;
+
+	if (ev == &adc->event0) {
+		adc->event0 = adc->event1;
+		ev = &adc->event1;
+	}
+
+	ev->channel = -1;
+	ev->direction = IIO_EV_DIR_NONE;
+
+	return palmas_gpadc_reconfigure_event_channels(adc);
+}
+
+static int palmas_gpadc_write_event_config(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, enum iio_event_type type,
+	enum iio_event_direction dir, int state)
+{
+	struct palmas_gpadc *adc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	mutex_lock(&adc->lock);
+
+	if (state)
+		ret = palmas_gpadc_enable_event_config(adc, chan, dir);
+	else
+		ret = palmas_gpadc_disable_event_config(adc, chan, dir);
+
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
+static int palmas_gpadc_read_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	mutex_lock(&adc->lock);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		*val = (dir == IIO_EV_DIR_RISING) ?
+			adc->thresh_data[adc_chan].high_thresh :
+			adc->thresh_data[adc_chan].low_thresh;
+		ret = IIO_VAL_INT;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
+static int palmas_gpadc_write_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
+	int adc_chan = chan->channel;
+	int ret = 0;
+
+	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	mutex_lock(&adc->lock);
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (val < 0 || val > 0xFFF) {
+			ret = -EINVAL;
+			break;
+		}
+		if (dir == IIO_EV_DIR_RISING)
+			adc->thresh_data[adc_chan].high_thresh = val;
+		else
+			adc->thresh_data[adc_chan].low_thresh = val;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
+		ret = palmas_gpadc_reconfigure_event_channels(adc);
+
+	mutex_unlock(&adc->lock);
+
+	return ret;
+}
+
 static const struct iio_info palmas_gpadc_iio_info = {
 	.read_raw = palmas_gpadc_read_raw,
+	.read_event_config = palmas_gpadc_read_event_config,
+	.write_event_config = palmas_gpadc_write_event_config,
+	.read_event_value = palmas_gpadc_read_event_value,
+	.write_event_value = palmas_gpadc_write_event_value,
+};
+
+static const struct iio_event_spec palmas_gpadc_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				BIT(IIO_EV_INFO_ENABLE),
+	},
 };
 
 #define PALMAS_ADC_CHAN_IIO(chan, _type, chan_info)	\
@@ -443,6 +813,8 @@ static const struct iio_info palmas_gpadc_iio_info = {
 			BIT(chan_info),			\
 	.indexed = 1,					\
 	.channel = PALMAS_ADC_CH_##chan,		\
+	.event_spec = palmas_gpadc_events,		\
+	.num_event_specs = ARRAY_SIZE(palmas_gpadc_events)	\
 }
 
 static const struct iio_chan_spec palmas_gpadc_iio_channel[] = {
@@ -492,9 +864,12 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
 	return 0;
 }
 
-static void palmas_disable_wakeup(void *dev)
+static void palmas_disable_wakeup(void *data)
 {
-	device_wakeup_disable(dev);
+	struct palmas_gpadc *adc = data;
+
+	if (adc->wakeup1_enable || adc->wakeup2_enable)
+		device_wakeup_disable(adc->dev);
 }
 
 static int palmas_gpadc_probe(struct platform_device *pdev)
@@ -547,36 +922,49 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 		return dev_err_probe(adc->dev, ret,
 				     "request irq %d failed\n", adc->irq);
 
+	adc->irq_auto_0 = platform_get_irq(pdev, 1);
+	if (adc->irq_auto_0 < 0)
+		return dev_err_probe(adc->dev, adc->irq_auto_0,
+				     "get auto0 irq failed\n");
+
+	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, NULL,
+					palmas_gpadc_irq_auto, IRQF_ONESHOT,
+					"palmas-adc-auto-0", indio_dev);
+	if (ret < 0)
+		return dev_err_probe(adc->dev, ret,
+				     "request auto0 irq %d failed\n",
+				     adc->irq_auto_0);
+
+	adc->irq_auto_1 = platform_get_irq(pdev, 2);
+	if (adc->irq_auto_1 < 0)
+		return dev_err_probe(adc->dev, adc->irq_auto_1,
+				     "get auto1 irq failed\n");
+
+	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, NULL,
+					palmas_gpadc_irq_auto, IRQF_ONESHOT,
+					"palmas-adc-auto-1", indio_dev);
+	if (ret < 0)
+		return dev_err_probe(adc->dev, ret,
+				     "request auto1 irq %d failed\n",
+				     adc->irq_auto_1);
+
 	if (gpadc_pdata->adc_wakeup1_data) {
 		memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data,
 			sizeof(adc->wakeup1_data));
 		adc->wakeup1_enable = true;
-		adc->irq_auto_0 =  platform_get_irq(pdev, 1);
-		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0,
-						NULL, palmas_gpadc_irq_auto,
-						IRQF_ONESHOT,
-						"palmas-adc-auto-0", adc);
-		if (ret < 0)
-			return dev_err_probe(adc->dev, ret,
-					     "request auto0 irq %d failed\n",
-					     adc->irq_auto_0);
 	}
 
 	if (gpadc_pdata->adc_wakeup2_data) {
 		memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data,
 				sizeof(adc->wakeup2_data));
 		adc->wakeup2_enable = true;
-		adc->irq_auto_1 =  platform_get_irq(pdev, 2);
-		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1,
-						NULL, palmas_gpadc_irq_auto,
-						IRQF_ONESHOT,
-						"palmas-adc-auto-1", adc);
-		if (ret < 0)
-			return dev_err_probe(adc->dev, ret,
-					     "request auto1 irq %d failed\n",
-					     adc->irq_auto_1);
 	}
 
+	adc->event0.channel = -1;
+	adc->event0.direction = IIO_EV_DIR_NONE;
+	adc->event1.channel = -1;
+	adc->event1.direction = IIO_EV_DIR_NONE;
+
 	/* set the current source 0 (value 0/5/15/20 uA => 0..3) */
 	if (gpadc_pdata->ch0_current <= 1)
 		adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
@@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 		return dev_err_probe(adc->dev, ret,
 				     "iio_device_register() failed\n");
 
-	device_set_wakeup_capable(&pdev->dev, 1);
 	for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
 		if (!(adc->adc_info[i].is_uncalibrated))
 			palmas_gpadc_calibrate(adc, i);
 	}
 
+	device_set_wakeup_capable(&pdev->dev, 1);
 	if (adc->wakeup1_enable || adc->wakeup2_enable) {
-		device_wakeup_enable(&pdev->dev);
-		ret = devm_add_action_or_reset(&pdev->dev,
-					       palmas_disable_wakeup,
-					       &pdev->dev);
+		ret = palmas_adc_wakeup_configure(adc);
 		if (ret)
 			return ret;
+		device_wakeup_enable(&pdev->dev);
 	}
+	ret = devm_add_action_or_reset(&pdev->dev,
+				       palmas_disable_wakeup,
+				       adc);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -755,16 +1146,11 @@ static int palmas_gpadc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct palmas_gpadc *adc = iio_priv(indio_dev);
-	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
 	int ret;
 
-	if (!device_may_wakeup(dev) || !wakeup)
+	if (!device_may_wakeup(dev))
 		return 0;
 
-	ret = palmas_adc_wakeup_configure(adc);
-	if (ret < 0)
-		return ret;
-
 	if (adc->wakeup1_enable)
 		enable_irq_wake(adc->irq_auto_0);
 
@@ -778,16 +1164,11 @@ static int palmas_gpadc_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct palmas_gpadc *adc = iio_priv(indio_dev);
-	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
 	int ret;
 
-	if (!device_may_wakeup(dev) || !wakeup)
+	if (!device_may_wakeup(dev))
 		return 0;
 
-	ret = palmas_adc_wakeup_reset(adc);
-	if (ret < 0)
-		return ret;
-
 	if (adc->wakeup1_enable)
 		disable_irq_wake(adc->irq_auto_0);
 
-- 
2.25.1

[PATCH 2/3] iio: adc: palmas_gpadc: remove adc_wakeupX_data

[Patrik Dahlström]

It does not seem to be used by anyone and having two ways to configure
the same thing but for different use cases is bound to introduce bugs.
This removes the special use case and leaves the generic one. It's still
possible to achieve the same thing as before from userspace.

Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
---
 drivers/iio/adc/palmas_gpadc.c | 18 ------------------
 include/linux/mfd/palmas.h     |  2 --
 2 files changed, 20 deletions(-)

diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 84c6e3b66205..419d7db51345 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -948,18 +948,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 				     "request auto1 irq %d failed\n",
 				     adc->irq_auto_1);
 
-	if (gpadc_pdata->adc_wakeup1_data) {
-		memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data,
-			sizeof(adc->wakeup1_data));
-		adc->wakeup1_enable = true;
-	}
-
-	if (gpadc_pdata->adc_wakeup2_data) {
-		memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data,
-				sizeof(adc->wakeup2_data));
-		adc->wakeup2_enable = true;
-	}
-
 	adc->event0.channel = -1;
 	adc->event0.direction = IIO_EV_DIR_NONE;
 	adc->event1.channel = -1;
@@ -1004,12 +992,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 	}
 
 	device_set_wakeup_capable(&pdev->dev, 1);
-	if (adc->wakeup1_enable || adc->wakeup2_enable) {
-		ret = palmas_adc_wakeup_configure(adc);
-		if (ret)
-			return ret;
-		device_wakeup_enable(&pdev->dev);
-	}
 	ret = devm_add_action_or_reset(&pdev->dev,
 				       palmas_disable_wakeup,
 				       adc);
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 1e61c7e9f50d..dc79d5e2d680 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -153,8 +153,6 @@ struct palmas_gpadc_platform_data {
 	int start_polarity;
 
 	int auto_conversion_period_ms;
-	struct palmas_adc_wakeup_property *adc_wakeup1_data;
-	struct palmas_adc_wakeup_property *adc_wakeup2_data;
 };
 
 struct palmas_reg_init {
-- 
2.25.1

[PATCH 3/3] iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property

[Patrik Dahlström]

This struct contain essentially the same information as
palmas_adc_event and palmas_gpadc_thresholds combined, but with more
ambiguity: the code decided whether to trigger on rising or falling edge
based on the high threshold being non-zero.

Since its use in platform data has now been removed, we can remove it
entirely.

Lastly, the use case for waking up the cpu from sleep mode when a
threshold has been passed is no longer the primary use for events so all
code is changed to say "event" instead of "wakeup".

Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
---
 drivers/iio/adc/palmas_gpadc.c | 94 +++++++++++++---------------------
 include/linux/mfd/palmas.h     |  6 ---
 2 files changed, 36 insertions(+), 64 deletions(-)

diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
index 419d7db51345..042b68f29444 100644
--- a/drivers/iio/adc/palmas_gpadc.c
+++ b/drivers/iio/adc/palmas_gpadc.c
@@ -122,10 +122,8 @@ struct palmas_gpadc {
 	int				irq_auto_1;
 	struct palmas_gpadc_info	*adc_info;
 	struct completion		conv_completion;
-	struct palmas_adc_wakeup_property wakeup1_data;
-	struct palmas_adc_wakeup_property wakeup2_data;
-	bool				wakeup1_enable;
-	bool				wakeup2_enable;
+	bool				event0_enable;
+	bool				event1_enable;
 	int				auto_conversion_period;
 	struct mutex			lock;
 	struct palmas_adc_event		event0;
@@ -592,50 +590,26 @@ static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
-				       struct palmas_adc_event *ev,
-				       struct palmas_adc_wakeup_property *wakeup)
-{
-	wakeup->adc_channel_number = ev->channel;
-	if (ev->direction == IIO_EV_DIR_RISING) {
-		wakeup->adc_low_threshold = 0;
-		wakeup->adc_high_threshold =
-			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
-	}
-	else {
-		wakeup->adc_low_threshold =
-			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
-		wakeup->adc_high_threshold = 0;
-	}
-}
-
-static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
-static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
+static int palmas_adc_events_configure(struct palmas_gpadc *adc);
+static int palmas_adc_events_reset(struct palmas_gpadc *adc);
 
 static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
 {
-	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
+	bool was_enabled = adc->event0_enable || adc->event1_enable;
 	bool enable;
 
-	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
-	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
+	adc->event0_enable = adc->event0.channel == -1 ? false : true;
+	adc->event1_enable = adc->event1.channel == -1 ? false : true;
 
-	enable = adc->wakeup1_enable || adc->wakeup2_enable;
+	enable = adc->event0_enable || adc->event1_enable;
 	if (!was_enabled && enable)
 		device_wakeup_enable(adc->dev);
 	else if (was_enabled && !enable)
 		device_wakeup_disable(adc->dev);
 
-	if (!enable)
-		return palmas_adc_wakeup_reset(adc);
-
-	// adjust levels
-	if (adc->wakeup1_enable)
-		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
-	if (adc->wakeup2_enable)
-		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
-
-	return palmas_adc_wakeup_configure(adc);
+	return enable ?
+		palmas_adc_events_configure(adc) :
+		palmas_adc_events_reset(adc);
 }
 
 static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
@@ -864,12 +838,14 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
 	return 0;
 }
 
-static void palmas_disable_wakeup(void *data)
+static void palmas_disable_events(void *data)
 {
 	struct palmas_gpadc *adc = data;
 
-	if (adc->wakeup1_enable || adc->wakeup2_enable)
+	if (adc->event0_enable || adc->event1_enable) {
+		palmas_adc_events_reset(adc);
 		device_wakeup_disable(adc->dev);
+	}
 }
 
 static int palmas_gpadc_probe(struct platform_device *pdev)
@@ -993,7 +969,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 
 	device_set_wakeup_capable(&pdev->dev, 1);
 	ret = devm_add_action_or_reset(&pdev->dev,
-				       palmas_disable_wakeup,
+				       palmas_disable_events,
 				       adc);
 	if (ret)
 		return ret;
@@ -1001,7 +977,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
+static int palmas_adc_events_configure(struct palmas_gpadc *adc)
 {
 	int adc_period, conv;
 	int i;
@@ -1027,16 +1003,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
 	}
 
 	conv = 0;
-	if (adc->wakeup1_enable) {
+	if (adc->event0_enable) {
+		struct palmas_adc_event *ev = &adc->event0;
 		int polarity;
 
-		ch0 = adc->wakeup1_data.adc_channel_number;
+		ch0 = ev->channel;
 		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN;
-		if (adc->wakeup1_data.adc_high_threshold > 0) {
-			thres = adc->wakeup1_data.adc_high_threshold;
+
+		if (ev->direction == IIO_EV_DIR_RISING) {
+			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
 			polarity = 0;
 		} else {
-			thres = adc->wakeup1_data.adc_low_threshold;
+			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
 			polarity = PALMAS_GPADC_THRES_CONV0_MSB_THRES_CONV0_POL;
 		}
 
@@ -1058,16 +1036,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
 		}
 	}
 
-	if (adc->wakeup2_enable) {
+	if (adc->event1_enable) {
+		struct palmas_adc_event *ev = &adc->event1;
 		int polarity;
 
-		ch1 = adc->wakeup2_data.adc_channel_number;
+		ch1 = ev->channel;
 		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN;
-		if (adc->wakeup2_data.adc_high_threshold > 0) {
-			thres = adc->wakeup2_data.adc_high_threshold;
+
+		if (ev->direction == IIO_EV_DIR_RISING) {
+			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
 			polarity = 0;
 		} else {
-			thres = adc->wakeup2_data.adc_low_threshold;
+			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
 			polarity = PALMAS_GPADC_THRES_CONV1_MSB_THRES_CONV1_POL;
 		}
 
@@ -1106,7 +1086,7 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
 	return ret;
 }
 
-static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
+static int palmas_adc_events_reset(struct palmas_gpadc *adc)
 {
 	int ret;
 
@@ -1128,15 +1108,14 @@ static int palmas_gpadc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct palmas_gpadc *adc = iio_priv(indio_dev);
-	int ret;
 
 	if (!device_may_wakeup(dev))
 		return 0;
 
-	if (adc->wakeup1_enable)
+	if (adc->event0_enable)
 		enable_irq_wake(adc->irq_auto_0);
 
-	if (adc->wakeup2_enable)
+	if (adc->event1_enable)
 		enable_irq_wake(adc->irq_auto_1);
 
 	return 0;
@@ -1146,15 +1125,14 @@ static int palmas_gpadc_resume(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct palmas_gpadc *adc = iio_priv(indio_dev);
-	int ret;
 
 	if (!device_may_wakeup(dev))
 		return 0;
 
-	if (adc->wakeup1_enable)
+	if (adc->event0_enable)
 		disable_irq_wake(adc->irq_auto_0);
 
-	if (adc->wakeup2_enable)
+	if (adc->event1_enable)
 		disable_irq_wake(adc->irq_auto_1);
 
 	return 0;
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index dc79d5e2d680..55f22adb1a9e 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -129,12 +129,6 @@ struct palmas_pmic_driver_data {
 			    struct regulator_config config);
 };
 
-struct palmas_adc_wakeup_property {
-	int adc_channel_number;
-	int adc_high_threshold;
-	int adc_low_threshold;
-};
-
 struct palmas_gpadc_platform_data {
 	/* Channel 3 current source is only enabled during conversion */
 	int ch3_current;	/* 0: off; 1: 10uA; 2: 400uA; 3: 800 uA */
-- 
2.25.1

Re: [PATCH 0/3] iio: adc: palmas_gpadc: add iio events

[H. Nikolaus Schaller]

Hi Patrik,

> Am 19.03.2023 um 23:39 schrieb Patrik Dahlström <risca@dalakolonin.se>:
> 
> These changes are based on [1] and [2].
> 
> The palmas gpadc block has support for monitoring up to 2 ADC channels
> and issue an interrupt if they reach past a set threshold. This can be
> configured statically with device tree todayi, but it only gets enabled
> when reaching sleep mode. Also, it doesn't look like anyone is using it.
> 
> Instead of this one special case, change the code so userspace can
> configure the ADC channels to their own needs through the iio events
> subsystem.
> 
> Thresholds and events were tested on omap5-uevm board. It should still
> be possible to wake up from sleep mode on events, but my board don't
> like sleep. A userspace tool for monitoring events and adjusting
> thresholds can be found at [3].
> 
> [1] https://patchwork.kernel.org/project/linux-iio/patch/20230318163039.56115-1-jic23@kernel.org/
> [2] https://patchwork.kernel.org/project/linux-iio/patch/20230313205029.1881745-1-risca@dalakolonin.se/
> [3] https://github.com/Risca/pyra_vol_mon

nice to see the code here!

I haven't tested it yet and will just comment from code inspection.

Maybe it could be good to add linux-omap@vger.kernel.org because the Palmas is mostly used for OMAP devices.

BR,
Nikolaus

> 
> 
> Patrik Dahlström (3):
>  iio: adc: palmas_gpadc: add support for iio threshold events
>  iio: adc: palmas_gpadc: remove adc_wakeupX_data
>  iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property
> 
> drivers/iio/adc/palmas_gpadc.c | 527 +++++++++++++++++++++++++++------
> include/linux/mfd/palmas.h     |   8 -
> 2 files changed, 434 insertions(+), 101 deletions(-)
> 
> 
> base-commit: 37fd83916da2e4cae03d350015c82a67b1b334c4
> prerequisite-patch-id: 9b1f55610800b91b721d042bf7f33b58179237d1
> prerequisite-patch-id: b0418c707db13f514400956596e9ebe91c25bba0
> -- 
> 2.25.1
> 

Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[H. Nikolaus Schaller]

Hi Patrik,

> Am 19.03.2023 um 23:39 schrieb Patrik Dahlström <risca@dalakolonin.se>:
> 
> The palmas gpadc block has support for monitoring up to 2 ADC channels
> and issue an interrupt if they reach past a set threshold. The gpadc
> driver had limited support for this through the adc_wakeup{1,2}_data
> platform data. This however only allow a fixed threshold to be set at
> boot, and would only enable it when entering sleep mode.
> 
> This change hooks into the IIO events system and exposes to userspace
> the ability to configure threshold values for each channel individually,
> but only allow up to 2 such thresholds to be enabled at any given time.
> 
> The logic around suspend and resume had to be adjusted so that user
> space configuration don't get reset on resume. Instead, any configured
> adc auto wakeup gets enabled during probe.
> 
> Enabling a threshold from userspace will overwrite the adc wakeup
> configuration set during probe. Depending on how you look at it, this
> could also mean we allow userspace to update the adc wakeup thresholds.
> 
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> ---
> drivers/iio/adc/palmas_gpadc.c | 495 +++++++++++++++++++++++++++++----
> 1 file changed, 438 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> index 24d7c096e4b8..84c6e3b66205 100644
> --- a/drivers/iio/adc/palmas_gpadc.c
> +++ b/drivers/iio/adc/palmas_gpadc.c
> @@ -20,6 +20,7 @@
> #include <linux/completion.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/machine.h>
> #include <linux/iio/driver.h>
> @@ -76,6 +77,16 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = {
> 	PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true),
> };
> 
> +struct palmas_gpadc_thresholds {
> +	int high_thresh;
> +	int low_thresh;
> +};
> +
> +struct palmas_adc_event {
> +	int channel;
> +	enum iio_event_direction direction;
> +};
> +
> /*
>  * struct palmas_gpadc - the palmas_gpadc structure
>  * @ch0_current:	channel 0 current source setting
> @@ -117,8 +128,30 @@ struct palmas_gpadc {
> 	bool				wakeup2_enable;
> 	int				auto_conversion_period;
> 	struct mutex			lock;
> +	struct palmas_adc_event		event0;
> +	struct palmas_adc_event		event1;
> +	struct palmas_gpadc_thresholds	thresh_data[PALMAS_ADC_CH_MAX];
> };
> 
> +static struct palmas_adc_event *palmas_gpadc_get_event_channel(
> +	struct palmas_gpadc *adc, int adc_chan, enum iio_event_direction dir)
> +{
> +	if (adc_chan == adc->event0.channel && dir == adc->event0.direction)
> +		return &adc->event0;
> +
> +	if (adc_chan == adc->event1.channel && dir == adc->event1.direction)
> +		return &adc->event1;
> +
> +	return NULL;
> +}
> +
> +static bool palmas_gpadc_channel_is_freerunning(struct palmas_gpadc *adc,
> +						int adc_chan)
> +{
> +	return palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_RISING) ||
> +		palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_FALLING);
> +}
> +
> /*
>  * GPADC lock issue in AUTO mode.
>  * Impact: In AUTO mode, GPADC conversion can be locked after disabling AUTO
> @@ -188,11 +221,24 @@ static irqreturn_t palmas_gpadc_irq(int irq, void *data)
> 
> static irqreturn_t palmas_gpadc_irq_auto(int irq, void *data)
> {
> -	struct palmas_gpadc *adc = data;
> +	struct iio_dev *indio_dev = data;
> +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> +	struct palmas_adc_event *ev;
> 
> 	dev_dbg(adc->dev, "Threshold interrupt %d occurs\n", irq);
> 	palmas_disable_auto_conversion(adc);
> 
> +	ev = (irq == adc->irq_auto_0) ? &adc->event0 : &adc->event1;
> +	if (ev->channel != -1) {
> +		enum iio_event_direction dir;
> +		u64 code;
> +
> +		dir = ev->direction;
> +		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, ev->channel,
> +					    IIO_EV_TYPE_THRESH, dir);
> +		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
> +	}
> +
> 	return IRQ_HANDLED;
> }
> 
> @@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
> {
> 	int ret;
> 
> +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
> +		return 0; // ADC already running
> +
> 	ret = palmas_gpadc_enable(adc, adc_chan, true);
> 	if (ret < 0)
> 		return ret;
> @@ -339,28 +388,44 @@ static int palmas_gpadc_start_conversion(struct palmas_gpadc *adc, int adc_chan)
> 	unsigned int val;
> 	int ret;
> 
> -	init_completion(&adc->conv_completion);
> -	ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
> -				PALMAS_GPADC_SW_SELECT,
> -				PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
> -				PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
> -	if (ret < 0) {
> -		dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
> -		return ret;
> -	}
> +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan)) {
> +		int event = (adc_chan == adc->event0.channel) ? 0 : 1;
> +		unsigned int reg = (event == 0) ?
> +			PALMAS_GPADC_AUTO_CONV0_LSB :
> +			PALMAS_GPADC_AUTO_CONV1_LSB;
> 
> -	ret = wait_for_completion_timeout(&adc->conv_completion,
> -				PALMAS_ADC_CONVERSION_TIMEOUT);
> -	if (ret == 0) {
> -		dev_err(adc->dev, "conversion not completed\n");
> -		return -ETIMEDOUT;
> +		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> +					reg, &val, 2);
> +		if (ret < 0) {
> +			dev_err(adc->dev, "AUTO_CONV%x_LSB read failed: %d\n",
> +				event, ret);
> +			return ret;
> +		}
> 	}
> +	else {
> +		init_completion(&adc->conv_completion);
> +		ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
> +					PALMAS_GPADC_SW_SELECT,
> +					PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
> +					PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
> +		if (ret < 0) {
> +			dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
> +			return ret;
> +		}
> 
> -	ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> -				PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
> -	if (ret < 0) {
> -		dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
> -		return ret;
> +		ret = wait_for_completion_timeout(&adc->conv_completion,
> +					PALMAS_ADC_CONVERSION_TIMEOUT);
> +		if (ret == 0) {
> +			dev_err(adc->dev, "conversion not completed\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> +					PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
> +		if (ret < 0) {
> +			dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
> +			return ret;
> +		}
> 	}
> 
> 	ret = val & 0xFFF;
> @@ -385,6 +450,80 @@ static int palmas_gpadc_get_calibrated_code(struct palmas_gpadc *adc,
> 	return val;
> }
> 
> +static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
> +					       struct palmas_adc_event *ev)
> +{
> +	const int INL = 2;
> +	const int adc_chan = ev->channel;
> +	const int orig = adc->thresh_data[adc_chan].high_thresh;
> +	int val = orig;
> +	int gain_drift;
> +	int offset_drift;
> +
> +	if (!val)
> +		return 0;

What is the reason to make val = 0 a special handling?
IMHO this makes the threshold levels discontinuous.

> +
> +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> +
> +	if (!adc->adc_info[adc_chan].is_uncalibrated) {
> +		val = (val * adc->adc_info[adc_chan].gain_error +
> +		       adc->adc_info[adc_chan].offset) /

here it would make a difference for val == 0 and offset != 0

> +			1000;
> +		gain_drift = 1002;
> +		offset_drift = 2;

where do these magic constants come from?

> +	}
> +	else {
> +		gain_drift = 1022;
> +		offset_drift = 36;

same here.

> +	}
> +
> +	// add tolerance to threshold
> +	val = ((val + INL) * gain_drift) / 1000 + offset_drift;

here it would make a difference for val == 0.

> +
> +	// clamp to max possible value
> +	if (val > 0xFFF)
> +		val = 0xFFF;
> +
> +	return val;
> +}
> +
> +static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
> +					      struct palmas_adc_event *ev)
> +{
> +	const int INL = 2;
> +	const int adc_chan = ev->channel;
> +	const int orig = adc->thresh_data[adc_chan].low_thresh;
> +	int val = orig;
> +	int gain_drift;
> +	int offset_drift;
> +
> +	if (!val)
> +		return val;

same here. And why return val and not 0 as above?

> +
> +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> +
> +        if (!adc->adc_info[adc_chan].is_uncalibrated) {
> +            val = (val * adc->adc_info[adc_chan].gain_error -
> +		   adc->adc_info[adc_chan].offset) /
> +		    1000;
> +            gain_drift = 998;
> +            offset_drift = 2;
> +        }
> +        else {
> +            gain_drift = 978;
> +            offset_drift = 36;

same here - how are they related to the constants in 
palmas_gpadc_get_high_threshold_raw() ?

> +        }
> +
> +	// calculate tolerances
> +	val = ((val - INL) * gain_drift) / 1000 - offset_drift;
> +
> +	// clamp to minimum 0
> +	if (val < 0)
> +		val = 0;
> +
> +	return val;
> +}
> +
> static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> 	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> {
> @@ -431,8 +570,239 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> 	return ret;
> }
> 
> +static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir)
> +{
> +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +
> +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir)) {
> +		ret = 1;
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> +static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
> +				       struct palmas_adc_event *ev,
> +				       struct palmas_adc_wakeup_property *wakeup)
> +{
> +	wakeup->adc_channel_number = ev->channel;
> +	if (ev->direction == IIO_EV_DIR_RISING) {
> +		wakeup->adc_low_threshold = 0;
> +		wakeup->adc_high_threshold =
> +			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> +	}
> +	else {
> +		wakeup->adc_low_threshold =
> +			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> +		wakeup->adc_high_threshold = 0;
> +	}
> +}
> +
> +static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
> +static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
> +
> +static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> +{
> +	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> +	bool enable;
> +
> +	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> +	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> +
> +	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> +	if (!was_enabled && enable)
> +		device_wakeup_enable(adc->dev);
> +	else if (was_enabled && !enable)
> +		device_wakeup_disable(adc->dev);
> +
> +	if (!enable)
> +		return palmas_adc_wakeup_reset(adc);
> +
> +	// adjust levels
> +	if (adc->wakeup1_enable)
> +		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> +	if (adc->wakeup2_enable)
> +		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> +
> +	return palmas_adc_wakeup_configure(adc);
> +}
> +
> +static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> +{
> +	struct palmas_adc_event *ev;
> +	int adc_chan = chan->channel;
> +
> +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> +		/* already enabled */
> +		return 0;
> +
> +	if (adc->event0.channel == -1)
> +		ev = &adc->event0;
> +	else if (adc->event1.channel == -1) {
> +		/* event0 has to be the lowest channel */
> +		if (adc_chan < adc->event0.channel) {
> +			adc->event1 = adc->event0;
> +			ev = &adc->event0;
> +		}
> +		else
> +			ev = &adc->event1;
> +	}
> +	else /* both AUTO channels already in use */ {
> +		dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
> +			 adc->event0.channel, adc->event1.channel);
> +		return -EBUSY;
> +	}
> +
> +	ev->channel = adc_chan;
> +	ev->direction = dir;
> +
> +	return palmas_gpadc_reconfigure_event_channels(adc);
> +}
> +
> +static int palmas_gpadc_disable_event_config(struct palmas_gpadc *adc,
> +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> +{
> +	int adc_chan = chan->channel;
> +	struct palmas_adc_event *ev =
> +		palmas_gpadc_get_event_channel(adc, adc_chan, dir);
> +
> +	if (!ev)
> +		return 0;
> +
> +	if (ev == &adc->event0) {
> +		adc->event0 = adc->event1;
> +		ev = &adc->event1;
> +	}
> +
> +	ev->channel = -1;
> +	ev->direction = IIO_EV_DIR_NONE;
> +
> +	return palmas_gpadc_reconfigure_event_channels(adc);
> +}
> +
> +static int palmas_gpadc_write_event_config(struct iio_dev *indio_dev,
> +	const struct iio_chan_spec *chan, enum iio_event_type type,
> +	enum iio_event_direction dir, int state)
> +{
> +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +
> +	if (state)
> +		ret = palmas_gpadc_enable_event_config(adc, chan, dir);
> +	else
> +		ret = palmas_gpadc_disable_event_config(adc, chan, dir);
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> +static int palmas_gpadc_read_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		*val = (dir == IIO_EV_DIR_RISING) ?
> +			adc->thresh_data[adc_chan].high_thresh :
> +			adc->thresh_data[adc_chan].low_thresh;
> +		ret = IIO_VAL_INT;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> +static int palmas_gpadc_write_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
> +	int adc_chan = chan->channel;
> +	int ret = 0;
> +
> +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> +		return -EINVAL;
> +
> +	mutex_lock(&adc->lock);
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (val < 0 || val > 0xFFF) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +		if (dir == IIO_EV_DIR_RISING)
> +			adc->thresh_data[adc_chan].high_thresh = val;
> +		else
> +			adc->thresh_data[adc_chan].low_thresh = val;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> +		ret = palmas_gpadc_reconfigure_event_channels(adc);
> +
> +	mutex_unlock(&adc->lock);
> +
> +	return ret;
> +}
> +
> static const struct iio_info palmas_gpadc_iio_info = {
> 	.read_raw = palmas_gpadc_read_raw,
> +	.read_event_config = palmas_gpadc_read_event_config,
> +	.write_event_config = palmas_gpadc_write_event_config,
> +	.read_event_value = palmas_gpadc_read_event_value,
> +	.write_event_value = palmas_gpadc_write_event_value,
> +};
> +
> +static const struct iio_event_spec palmas_gpadc_events[] = {
> +	{
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_RISING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				BIT(IIO_EV_INFO_ENABLE),
> +	}, {
> +		.type = IIO_EV_TYPE_THRESH,
> +		.dir = IIO_EV_DIR_FALLING,
> +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> +				BIT(IIO_EV_INFO_ENABLE),
> +	},
> };
> 
> #define PALMAS_ADC_CHAN_IIO(chan, _type, chan_info)	\
> @@ -443,6 +813,8 @@ static const struct iio_info palmas_gpadc_iio_info = {
> 			BIT(chan_info),			\
> 	.indexed = 1,					\
> 	.channel = PALMAS_ADC_CH_##chan,		\
> +	.event_spec = palmas_gpadc_events,		\
> +	.num_event_specs = ARRAY_SIZE(palmas_gpadc_events)	\
> }
> 
> static const struct iio_chan_spec palmas_gpadc_iio_channel[] = {
> @@ -492,9 +864,12 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
> 	return 0;
> }
> 
> -static void palmas_disable_wakeup(void *dev)
> +static void palmas_disable_wakeup(void *data)
> {
> -	device_wakeup_disable(dev);
> +	struct palmas_gpadc *adc = data;
> +
> +	if (adc->wakeup1_enable || adc->wakeup2_enable)
> +		device_wakeup_disable(adc->dev);
> }
> 
> static int palmas_gpadc_probe(struct platform_device *pdev)
> @@ -547,36 +922,49 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> 		return dev_err_probe(adc->dev, ret,
> 				     "request irq %d failed\n", adc->irq);
> 
> +	adc->irq_auto_0 = platform_get_irq(pdev, 1);
> +	if (adc->irq_auto_0 < 0)
> +		return dev_err_probe(adc->dev, adc->irq_auto_0,
> +				     "get auto0 irq failed\n");
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, NULL,
> +					palmas_gpadc_irq_auto, IRQF_ONESHOT,
> +					"palmas-adc-auto-0", indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(adc->dev, ret,
> +				     "request auto0 irq %d failed\n",
> +				     adc->irq_auto_0);
> +
> +	adc->irq_auto_1 = platform_get_irq(pdev, 2);
> +	if (adc->irq_auto_1 < 0)
> +		return dev_err_probe(adc->dev, adc->irq_auto_1,
> +				     "get auto1 irq failed\n");
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, NULL,
> +					palmas_gpadc_irq_auto, IRQF_ONESHOT,
> +					"palmas-adc-auto-1", indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(adc->dev, ret,
> +				     "request auto1 irq %d failed\n",
> +				     adc->irq_auto_1);
> +
> 	if (gpadc_pdata->adc_wakeup1_data) {
> 		memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data,
> 			sizeof(adc->wakeup1_data));
> 		adc->wakeup1_enable = true;
> -		adc->irq_auto_0 =  platform_get_irq(pdev, 1);
> -		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0,
> -						NULL, palmas_gpadc_irq_auto,
> -						IRQF_ONESHOT,
> -						"palmas-adc-auto-0", adc);
> -		if (ret < 0)
> -			return dev_err_probe(adc->dev, ret,
> -					     "request auto0 irq %d failed\n",
> -					     adc->irq_auto_0);
> 	}
> 
> 	if (gpadc_pdata->adc_wakeup2_data) {
> 		memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data,
> 				sizeof(adc->wakeup2_data));
> 		adc->wakeup2_enable = true;
> -		adc->irq_auto_1 =  platform_get_irq(pdev, 2);
> -		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1,
> -						NULL, palmas_gpadc_irq_auto,
> -						IRQF_ONESHOT,
> -						"palmas-adc-auto-1", adc);
> -		if (ret < 0)
> -			return dev_err_probe(adc->dev, ret,
> -					     "request auto1 irq %d failed\n",
> -					     adc->irq_auto_1);
> 	}
> 
> +	adc->event0.channel = -1;
> +	adc->event0.direction = IIO_EV_DIR_NONE;
> +	adc->event1.channel = -1;
> +	adc->event1.direction = IIO_EV_DIR_NONE;
> +
> 	/* set the current source 0 (value 0/5/15/20 uA => 0..3) */
> 	if (gpadc_pdata->ch0_current <= 1)
> 		adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
> @@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> 		return dev_err_probe(adc->dev, ret,
> 				     "iio_device_register() failed\n");
> 
> -	device_set_wakeup_capable(&pdev->dev, 1);
> 	for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
> 		if (!(adc->adc_info[i].is_uncalibrated))
> 			palmas_gpadc_calibrate(adc, i);
> 	}
> 
> +	device_set_wakeup_capable(&pdev->dev, 1);
> 	if (adc->wakeup1_enable || adc->wakeup2_enable) {
> -		device_wakeup_enable(&pdev->dev);
> -		ret = devm_add_action_or_reset(&pdev->dev,
> -					       palmas_disable_wakeup,
> -					       &pdev->dev);
> +		ret = palmas_adc_wakeup_configure(adc);
> 		if (ret)
> 			return ret;
> +		device_wakeup_enable(&pdev->dev);
> 	}
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       palmas_disable_wakeup,
> +				       adc);
> +	if (ret)
> +		return ret;
> 
> 	return 0;
> }
> @@ -755,16 +1146,11 @@ static int palmas_gpadc_suspend(struct device *dev)
> {
> 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> 	struct palmas_gpadc *adc = iio_priv(indio_dev);
> -	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
> 	int ret;
> 
> -	if (!device_may_wakeup(dev) || !wakeup)
> +	if (!device_may_wakeup(dev))
> 		return 0;
> 
> -	ret = palmas_adc_wakeup_configure(adc);
> -	if (ret < 0)
> -		return ret;
> -
> 	if (adc->wakeup1_enable)
> 		enable_irq_wake(adc->irq_auto_0);
> 
> @@ -778,16 +1164,11 @@ static int palmas_gpadc_resume(struct device *dev)
> {
> 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> 	struct palmas_gpadc *adc = iio_priv(indio_dev);
> -	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
> 	int ret;
> 
> -	if (!device_may_wakeup(dev) || !wakeup)
> +	if (!device_may_wakeup(dev))
> 		return 0;
> 
> -	ret = palmas_adc_wakeup_reset(adc);
> -	if (ret < 0)
> -		return ret;
> -
> 	if (adc->wakeup1_enable)
> 		disable_irq_wake(adc->irq_auto_0);
> 
> -- 
> 2.25.1
> 

Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[Jonathan Cameron]

On Sun, 19 Mar 2023 23:39:06 +0100
Patrik Dahlström <risca@dalakolonin.se> wrote:

> The palmas gpadc block has support for monitoring up to 2 ADC channels
> and issue an interrupt if they reach past a set threshold. The gpadc
> driver had limited support for this through the adc_wakeup{1,2}_data
> platform data. This however only allow a fixed threshold to be set at
> boot, and would only enable it when entering sleep mode.
> 
> This change hooks into the IIO events system and exposes to userspace
> the ability to configure threshold values for each channel individually,
> but only allow up to 2 such thresholds to be enabled at any given time.

Add a comment here on what happens if userspace tries to set more than two.
It's not as obvious as you'd think as we have some drivers that use a fifo
approach so on setting the third event they push the oldest one out.

> 
> The logic around suspend and resume had to be adjusted so that user
> space configuration don't get reset on resume. Instead, any configured
> adc auto wakeup gets enabled during probe.
> 
> Enabling a threshold from userspace will overwrite the adc wakeup
> configuration set during probe. Depending on how you look at it, this
> could also mean we allow userspace to update the adc wakeup thresholds.

I'm not sure I read the code right, but can you end up enabling a wakeup
that wasn't previously present?  That seems likely something we should
not be doing after boot.

One option here would be to make it either wakeup is supported, or events
are supported.  I suspect no one uses the wakeup anyway so that shouldn't
matter much (+ you remove it in next patch - do that first and this code
becomes more obvious).


A few trivial comments inline.
> 
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>

>  
> @@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
>  {
>  	int ret;
>  
> +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
> +		return 0; // ADC already running

/* */

...

>  
> +static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
> +					       struct palmas_adc_event *ev)
> +{
> +	const int INL = 2;
> +	const int adc_chan = ev->channel;
> +	const int orig = adc->thresh_data[adc_chan].high_thresh;
> +	int val = orig;
> +	int gain_drift;
> +	int offset_drift;
> +
> +	if (!val)
> +		return 0;
> +
> +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> +
> +	if (!adc->adc_info[adc_chan].is_uncalibrated) {
> +		val = (val * adc->adc_info[adc_chan].gain_error +
> +		       adc->adc_info[adc_chan].offset) /
> +			1000;
> +		gain_drift = 1002;
> +		offset_drift = 2;
> +	}
> +	else {
> +		gain_drift = 1022;
> +		offset_drift = 36;
> +	}
> +
> +	// add tolerance to threshold
> +	val = ((val + INL) * gain_drift) / 1000 + offset_drift;
> +
> +	// clamp to max possible value
/* clamp .. */
val = min(val, 0xFFF);


> +	if (val > 0xFFF)
> +		val = 0xFFF;
> +
> +	return val;
> +}
> +
> +static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
> +					      struct palmas_adc_event *ev)
> +{
> +	const int INL = 2;
> +	const int adc_chan = ev->channel;
> +	const int orig = adc->thresh_data[adc_chan].low_thresh;
> +	int val = orig;
> +	int gain_drift;
> +	int offset_drift;
> +
> +	if (!val)
> +		return val;
> +
> +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> +
> +        if (!adc->adc_info[adc_chan].is_uncalibrated) {
> +            val = (val * adc->adc_info[adc_chan].gain_error -
> +		   adc->adc_info[adc_chan].offset) /
> +		    1000;
> +            gain_drift = 998;
> +            offset_drift = 2;
> +        }
> +        else {
> +            gain_drift = 978;
> +            offset_drift = 36;
> +        }
> +
> +	// calculate tolerances
/* */

+ I think this could do with more information on why a tweak is needed.

> +	val = ((val - INL) * gain_drift) / 1000 - offset_drift;
> +
> +	// clamp to minimum 0

/* */ for all comments. 

val = max(0, val); then comment may not be needed.

> +	if (val < 0)
> +		val = 0;
> +
> +	return val;
> +}

> +static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> +{
> +	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> +	bool enable;
> +
> +	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> +	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> +
> +	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> +	if (!was_enabled && enable)
> +		device_wakeup_enable(adc->dev);
> +	else if (was_enabled && !enable)
> +		device_wakeup_disable(adc->dev);
> +
> +	if (!enable)
> +		return palmas_adc_wakeup_reset(adc);
> +
> +	// adjust levels

/* adjust levels */ 

> +	if (adc->wakeup1_enable)
> +		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> +	if (adc->wakeup2_enable)
> +		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> +
> +	return palmas_adc_wakeup_configure(adc);
> +}
> +
> +static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> +{
> +	struct palmas_adc_event *ev;
> +	int adc_chan = chan->channel;
> +
> +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> +		/* already enabled */
> +		return 0;
> +
> +	if (adc->event0.channel == -1)

I'd add brackets for all legs of this if / else once one of them needs
it. Tends to end up more readable.

> +		ev = &adc->event0;
> +	else if (adc->event1.channel == -1) {
> +		/* event0 has to be the lowest channel */
> +		if (adc_chan < adc->event0.channel) {
> +			adc->event1 = adc->event0;
> +			ev = &adc->event0;
> +		}
> +		else
> +			ev = &adc->event1;
> +	}

} else /*...

> +	else /* both AUTO channels already in use */ {
> +		dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
> +			 adc->event0.channel, adc->event1.channel);
> +		return -EBUSY;
> +	}
> +
> +	ev->channel = adc_chan;
> +	ev->direction = dir;
> +
> +	return palmas_gpadc_reconfigure_event_channels(adc);
> +}

> +
> +static int palmas_gpadc_write_event_value(struct iio_dev *indio_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)

Prefer parameters aligned just after (

> +{
...


>  
>  static int palmas_gpadc_probe(struct platform_device *pdev)

...

>  	/* set the current source 0 (value 0/5/15/20 uA => 0..3) */
>  	if (gpadc_pdata->ch0_current <= 1)
>  		adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
> @@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
>  		return dev_err_probe(adc->dev, ret,
>  				     "iio_device_register() failed\n");
>  
> -	device_set_wakeup_capable(&pdev->dev, 1);
>  	for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
>  		if (!(adc->adc_info[i].is_uncalibrated))
>  			palmas_gpadc_calibrate(adc, i);
>  	}
>  
> +	device_set_wakeup_capable(&pdev->dev, 1);
>  	if (adc->wakeup1_enable || adc->wakeup2_enable) {
> -		device_wakeup_enable(&pdev->dev);
> -		ret = devm_add_action_or_reset(&pdev->dev,
> -					       palmas_disable_wakeup,
> -					       &pdev->dev);
> +		ret = palmas_adc_wakeup_configure(adc);
>  		if (ret)
>  			return ret;
> +		device_wakeup_enable(&pdev->dev);

>  	}
> +	ret = devm_add_action_or_reset(&pdev->dev,

Add a comment for this to explain why it might need disabling even if
it wasn't enabled above.  I think if you just drop wakeup support in
general that will be fine given we have no known users.


> +				       palmas_disable_wakeup,
> +				       adc);
> +	if (ret)
> +		return ret;
>  
>  	return 0;
>  }

Re: [PATCH 2/3] iio: adc: palmas_gpadc: remove adc_wakeupX_data

[Jonathan Cameron]

On Sun, 19 Mar 2023 23:39:07 +0100
Patrik Dahlström <risca@dalakolonin.se> wrote:

> It does not seem to be used by anyone and having two ways to configure
> the same thing but for different use cases is bound to introduce bugs.
> This removes the special use case and leaves the generic one. It's still
> possible to achieve the same thing as before from userspace.

It wasn't possible to do it from userspace previously and there
is no way to turn it off without userspace having to explicitly disable events
before suspending.

We could look at adding a userspace control on whether wakeup is enabled.
However, I'd raise the question of does anyone care for this particular device
anymore.

As mentioned in patch 1 review, alternative is support either events or wakeup
configured at boot (if clever maybe allow for one of each combination as well).
That would keep to the old flows working and enable what you want.

Jonathan


> 
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> ---
>  drivers/iio/adc/palmas_gpadc.c | 18 ------------------
>  include/linux/mfd/palmas.h     |  2 --
>  2 files changed, 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> index 84c6e3b66205..419d7db51345 100644
> --- a/drivers/iio/adc/palmas_gpadc.c
> +++ b/drivers/iio/adc/palmas_gpadc.c
> @@ -948,18 +948,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
>  				     "request auto1 irq %d failed\n",
>  				     adc->irq_auto_1);
>  
> -	if (gpadc_pdata->adc_wakeup1_data) {
> -		memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data,
> -			sizeof(adc->wakeup1_data));
> -		adc->wakeup1_enable = true;
> -	}
> -
> -	if (gpadc_pdata->adc_wakeup2_data) {
> -		memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data,
> -				sizeof(adc->wakeup2_data));
> -		adc->wakeup2_enable = true;
> -	}
> -
>  	adc->event0.channel = -1;
>  	adc->event0.direction = IIO_EV_DIR_NONE;
>  	adc->event1.channel = -1;
> @@ -1004,12 +992,6 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
>  	}
>  
>  	device_set_wakeup_capable(&pdev->dev, 1);
> -	if (adc->wakeup1_enable || adc->wakeup2_enable) {
> -		ret = palmas_adc_wakeup_configure(adc);
> -		if (ret)
> -			return ret;
> -		device_wakeup_enable(&pdev->dev);
> -	}
>  	ret = devm_add_action_or_reset(&pdev->dev,
>  				       palmas_disable_wakeup,
>  				       adc);
> diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
> index 1e61c7e9f50d..dc79d5e2d680 100644
> --- a/include/linux/mfd/palmas.h
> +++ b/include/linux/mfd/palmas.h
> @@ -153,8 +153,6 @@ struct palmas_gpadc_platform_data {
>  	int start_polarity;
>  
>  	int auto_conversion_period_ms;
> -	struct palmas_adc_wakeup_property *adc_wakeup1_data;
> -	struct palmas_adc_wakeup_property *adc_wakeup2_data;
>  };
>  
>  struct palmas_reg_init {

Re: [PATCH 3/3] iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property

[Jonathan Cameron]

On Sun, 19 Mar 2023 23:39:08 +0100
Patrik Dahlström <risca@dalakolonin.se> wrote:

> This struct contain essentially the same information as
> palmas_adc_event and palmas_gpadc_thresholds combined, but with more
> ambiguity: the code decided whether to trigger on rising or falling edge
> based on the high threshold being non-zero.
> 
> Since its use in platform data has now been removed, we can remove it
> entirely.
> 
> Lastly, the use case for waking up the cpu from sleep mode when a
> threshold has been passed is no longer the primary use for events so all
> code is changed to say "event" instead of "wakeup".
Good. I nearly pointed this out in the earlier patch.  The wakeup naming
was confusing. However, I'd prefer that was done in a separate patch to
any other changes.  It's hard to spot the meaningful stuff when there
is a lot of renaming going on.

A few questions / comments inline.

Jonathan

> 
> Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> ---
>  drivers/iio/adc/palmas_gpadc.c | 94 +++++++++++++---------------------
>  include/linux/mfd/palmas.h     |  6 ---
>  2 files changed, 36 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> index 419d7db51345..042b68f29444 100644
> --- a/drivers/iio/adc/palmas_gpadc.c
> +++ b/drivers/iio/adc/palmas_gpadc.c
> @@ -122,10 +122,8 @@ struct palmas_gpadc {
>  	int				irq_auto_1;
>  	struct palmas_gpadc_info	*adc_info;
>  	struct completion		conv_completion;
> -	struct palmas_adc_wakeup_property wakeup1_data;
> -	struct palmas_adc_wakeup_property wakeup2_data;
> -	bool				wakeup1_enable;
> -	bool				wakeup2_enable;
> +	bool				event0_enable;
> +	bool				event1_enable;
>  	int				auto_conversion_period;
>  	struct mutex			lock;
>  	struct palmas_adc_event		event0;
> @@ -592,50 +590,26 @@ static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
> -				       struct palmas_adc_event *ev,
> -				       struct palmas_adc_wakeup_property *wakeup)
> -{
> -	wakeup->adc_channel_number = ev->channel;
> -	if (ev->direction == IIO_EV_DIR_RISING) {
> -		wakeup->adc_low_threshold = 0;
> -		wakeup->adc_high_threshold =
> -			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> -	}
> -	else {
> -		wakeup->adc_low_threshold =
> -			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> -		wakeup->adc_high_threshold = 0;
> -	}
> -}
> -
> -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
> -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
> +static int palmas_adc_events_configure(struct palmas_gpadc *adc);
> +static int palmas_adc_events_reset(struct palmas_gpadc *adc);
>  
>  static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
>  {
> -	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> +	bool was_enabled = adc->event0_enable || adc->event1_enable;
>  	bool enable;
>  
> -	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> -	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> +	adc->event0_enable = adc->event0.channel == -1 ? false : true;
> +	adc->event1_enable = adc->event1.channel == -1 ? false : true;
>  
> -	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> +	enable = adc->event0_enable || adc->event1_enable;
>  	if (!was_enabled && enable)
>  		device_wakeup_enable(adc->dev);
>  	else if (was_enabled && !enable)
>  		device_wakeup_disable(adc->dev);
>  
> -	if (!enable)
> -		return palmas_adc_wakeup_reset(adc);
> -
> -	// adjust levels
> -	if (adc->wakeup1_enable)
> -		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> -	if (adc->wakeup2_enable)
> -		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> -
> -	return palmas_adc_wakeup_configure(adc);
> +	return enable ?
> +		palmas_adc_events_configure(adc) :
> +		palmas_adc_events_reset(adc);
>  }
>  
>  static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> @@ -864,12 +838,14 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
>  	return 0;
>  }
>  
> -static void palmas_disable_wakeup(void *data)
> +static void palmas_disable_events(void *data)
>  {
>  	struct palmas_gpadc *adc = data;
>  
> -	if (adc->wakeup1_enable || adc->wakeup2_enable)
> +	if (adc->event0_enable || adc->event1_enable) {
> +		palmas_adc_events_reset(adc);

I can't immediately follow why this reset is needed when it wasn't before.
Perhaps that will be clearer once the renames aren't in the same patch.

>  		device_wakeup_disable(adc->dev);
> +	}
>  }
>  
>  static int palmas_gpadc_probe(struct platform_device *pdev)
> @@ -993,7 +969,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
>  
>  	device_set_wakeup_capable(&pdev->dev, 1);
>  	ret = devm_add_action_or_reset(&pdev->dev,
> -				       palmas_disable_wakeup,
> +				       palmas_disable_events,
>  				       adc);
>  	if (ret)
>  		return ret;
> @@ -1001,7 +977,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> +static int palmas_adc_events_configure(struct palmas_gpadc *adc)
>  {
>  	int adc_period, conv;
>  	int i;
> @@ -1027,16 +1003,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
>  	}
>  
>  	conv = 0;
> -	if (adc->wakeup1_enable) {
> +	if (adc->event0_enable) {
> +		struct palmas_adc_event *ev = &adc->event0;
>  		int polarity;
>  
> -		ch0 = adc->wakeup1_data.adc_channel_number;
> +		ch0 = ev->channel;
>  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN;
> -		if (adc->wakeup1_data.adc_high_threshold > 0) {
> -			thres = adc->wakeup1_data.adc_high_threshold;
> +
> +		if (ev->direction == IIO_EV_DIR_RISING) {
> +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
>  			polarity = 0;
>  		} else {
> -			thres = adc->wakeup1_data.adc_low_threshold;
> +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
>  			polarity = PALMAS_GPADC_THRES_CONV0_MSB_THRES_CONV0_POL;
>  		}
>  
> @@ -1058,16 +1036,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
>  		}
>  	}
>  
> -	if (adc->wakeup2_enable) {
> +	if (adc->event1_enable) {
> +		struct palmas_adc_event *ev = &adc->event1;
>  		int polarity;
>  
> -		ch1 = adc->wakeup2_data.adc_channel_number;
> +		ch1 = ev->channel;
>  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN;
> -		if (adc->wakeup2_data.adc_high_threshold > 0) {
> -			thres = adc->wakeup2_data.adc_high_threshold;
> +
> +		if (ev->direction == IIO_EV_DIR_RISING) {
> +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
>  			polarity = 0;
>  		} else {
> -			thres = adc->wakeup2_data.adc_low_threshold;
> +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
>  			polarity = PALMAS_GPADC_THRES_CONV1_MSB_THRES_CONV1_POL;
>  		}
>  
> @@ -1106,7 +1086,7 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
>  	return ret;
>  }
>  
> -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
> +static int palmas_adc_events_reset(struct palmas_gpadc *adc)
>  {
>  	int ret;
>  
> @@ -1128,15 +1108,14 @@ static int palmas_gpadc_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> -	int ret;

?  Seems unrelated - perhaps should be in earlier patch.

>  
>  	if (!device_may_wakeup(dev))
>  		return 0;
>  
> -	if (adc->wakeup1_enable)
> +	if (adc->event0_enable)
>  		enable_irq_wake(adc->irq_auto_0);
>  
> -	if (adc->wakeup2_enable)
> +	if (adc->event1_enable)
>  		enable_irq_wake(adc->irq_auto_1);
>  
>  	return 0;
> @@ -1146,15 +1125,14 @@ static int palmas_gpadc_resume(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> -	int ret;

?

>  
>  	if (!device_may_wakeup(dev))
>  		return 0;
>  
> -	if (adc->wakeup1_enable)
> +	if (adc->event0_enable)
>  		disable_irq_wake(adc->irq_auto_0);
>  
> -	if (adc->wakeup2_enable)
> +	if (adc->event1_enable)
>  		disable_irq_wake(adc->irq_auto_1);
>  
>  	return 0;

Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[Patrik Dahlström]

On Tue, Mar 21, 2023 at 09:40:07PM +0100, H. Nikolaus Schaller wrote:
> Hi Patrik,
> 
> > Am 19.03.2023 um 23:39 schrieb Patrik Dahlström <risca@dalakolonin.se>:
> > 
> > The palmas gpadc block has support for monitoring up to 2 ADC channels
> > and issue an interrupt if they reach past a set threshold. The gpadc
> > driver had limited support for this through the adc_wakeup{1,2}_data
> > platform data. This however only allow a fixed threshold to be set at
> > boot, and would only enable it when entering sleep mode.
> > 
> > This change hooks into the IIO events system and exposes to userspace
> > the ability to configure threshold values for each channel individually,
> > but only allow up to 2 such thresholds to be enabled at any given time.
> > 
> > The logic around suspend and resume had to be adjusted so that user
> > space configuration don't get reset on resume. Instead, any configured
> > adc auto wakeup gets enabled during probe.
> > 
> > Enabling a threshold from userspace will overwrite the adc wakeup
> > configuration set during probe. Depending on how you look at it, this
> > could also mean we allow userspace to update the adc wakeup thresholds.
> > 
> > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> > ---
> > drivers/iio/adc/palmas_gpadc.c | 495 +++++++++++++++++++++++++++++----
> > 1 file changed, 438 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> > index 24d7c096e4b8..84c6e3b66205 100644
> > --- a/drivers/iio/adc/palmas_gpadc.c
> > +++ b/drivers/iio/adc/palmas_gpadc.c
> > @@ -20,6 +20,7 @@
> > #include <linux/completion.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > +#include <linux/iio/events.h>
> > #include <linux/iio/iio.h>
> > #include <linux/iio/machine.h>
> > #include <linux/iio/driver.h>
> > @@ -76,6 +77,16 @@ static struct palmas_gpadc_info palmas_gpadc_info[] = {
> > 	PALMAS_ADC_INFO(IN15, 0, 0, 0, 0, INVALID, INVALID, true),
> > };
> > 
> > +struct palmas_gpadc_thresholds {
> > +	int high_thresh;
> > +	int low_thresh;
> > +};
> > +
> > +struct palmas_adc_event {
> > +	int channel;
> > +	enum iio_event_direction direction;
> > +};
> > +
> > /*
> >  * struct palmas_gpadc - the palmas_gpadc structure
> >  * @ch0_current:	channel 0 current source setting
> > @@ -117,8 +128,30 @@ struct palmas_gpadc {
> > 	bool				wakeup2_enable;
> > 	int				auto_conversion_period;
> > 	struct mutex			lock;
> > +	struct palmas_adc_event		event0;
> > +	struct palmas_adc_event		event1;
> > +	struct palmas_gpadc_thresholds	thresh_data[PALMAS_ADC_CH_MAX];
> > };
> > 
> > +static struct palmas_adc_event *palmas_gpadc_get_event_channel(
> > +	struct palmas_gpadc *adc, int adc_chan, enum iio_event_direction dir)
> > +{
> > +	if (adc_chan == adc->event0.channel && dir == adc->event0.direction)
> > +		return &adc->event0;
> > +
> > +	if (adc_chan == adc->event1.channel && dir == adc->event1.direction)
> > +		return &adc->event1;
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool palmas_gpadc_channel_is_freerunning(struct palmas_gpadc *adc,
> > +						int adc_chan)
> > +{
> > +	return palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_RISING) ||
> > +		palmas_gpadc_get_event_channel(adc, adc_chan, IIO_EV_DIR_FALLING);
> > +}
> > +
> > /*
> >  * GPADC lock issue in AUTO mode.
> >  * Impact: In AUTO mode, GPADC conversion can be locked after disabling AUTO
> > @@ -188,11 +221,24 @@ static irqreturn_t palmas_gpadc_irq(int irq, void *data)
> > 
> > static irqreturn_t palmas_gpadc_irq_auto(int irq, void *data)
> > {
> > -	struct palmas_gpadc *adc = data;
> > +	struct iio_dev *indio_dev = data;
> > +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > +	struct palmas_adc_event *ev;
> > 
> > 	dev_dbg(adc->dev, "Threshold interrupt %d occurs\n", irq);
> > 	palmas_disable_auto_conversion(adc);
> > 
> > +	ev = (irq == adc->irq_auto_0) ? &adc->event0 : &adc->event1;
> > +	if (ev->channel != -1) {
> > +		enum iio_event_direction dir;
> > +		u64 code;
> > +
> > +		dir = ev->direction;
> > +		code = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, ev->channel,
> > +					    IIO_EV_TYPE_THRESH, dir);
> > +		iio_push_event(indio_dev, code, iio_get_time_ns(indio_dev));
> > +	}
> > +
> > 	return IRQ_HANDLED;
> > }
> > 
> > @@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
> > {
> > 	int ret;
> > 
> > +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
> > +		return 0; // ADC already running
> > +
> > 	ret = palmas_gpadc_enable(adc, adc_chan, true);
> > 	if (ret < 0)
> > 		return ret;
> > @@ -339,28 +388,44 @@ static int palmas_gpadc_start_conversion(struct palmas_gpadc *adc, int adc_chan)
> > 	unsigned int val;
> > 	int ret;
> > 
> > -	init_completion(&adc->conv_completion);
> > -	ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
> > -				PALMAS_GPADC_SW_SELECT,
> > -				PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
> > -				PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
> > -	if (ret < 0) {
> > -		dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
> > -		return ret;
> > -	}
> > +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan)) {
> > +		int event = (adc_chan == adc->event0.channel) ? 0 : 1;
> > +		unsigned int reg = (event == 0) ?
> > +			PALMAS_GPADC_AUTO_CONV0_LSB :
> > +			PALMAS_GPADC_AUTO_CONV1_LSB;
> > 
> > -	ret = wait_for_completion_timeout(&adc->conv_completion,
> > -				PALMAS_ADC_CONVERSION_TIMEOUT);
> > -	if (ret == 0) {
> > -		dev_err(adc->dev, "conversion not completed\n");
> > -		return -ETIMEDOUT;
> > +		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> > +					reg, &val, 2);
> > +		if (ret < 0) {
> > +			dev_err(adc->dev, "AUTO_CONV%x_LSB read failed: %d\n",
> > +				event, ret);
> > +			return ret;
> > +		}
> > 	}
> > +	else {
> > +		init_completion(&adc->conv_completion);
> > +		ret = palmas_update_bits(adc->palmas, PALMAS_GPADC_BASE,
> > +					PALMAS_GPADC_SW_SELECT,
> > +					PALMAS_GPADC_SW_SELECT_SW_START_CONV0,
> > +					PALMAS_GPADC_SW_SELECT_SW_START_CONV0);
> > +		if (ret < 0) {
> > +			dev_err(adc->dev, "SELECT_SW_START write failed: %d\n", ret);
> > +			return ret;
> > +		}
> > 
> > -	ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> > -				PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
> > -	if (ret < 0) {
> > -		dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
> > -		return ret;
> > +		ret = wait_for_completion_timeout(&adc->conv_completion,
> > +					PALMAS_ADC_CONVERSION_TIMEOUT);
> > +		if (ret == 0) {
> > +			dev_err(adc->dev, "conversion not completed\n");
> > +			return -ETIMEDOUT;
> > +		}
> > +
> > +		ret = palmas_bulk_read(adc->palmas, PALMAS_GPADC_BASE,
> > +					PALMAS_GPADC_SW_CONV0_LSB, &val, 2);
> > +		if (ret < 0) {
> > +			dev_err(adc->dev, "SW_CONV0_LSB read failed: %d\n", ret);
> > +			return ret;
> > +		}
> > 	}
> > 
> > 	ret = val & 0xFFF;
> > @@ -385,6 +450,80 @@ static int palmas_gpadc_get_calibrated_code(struct palmas_gpadc *adc,
> > 	return val;
> > }
> > 
> > +static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
> > +					       struct palmas_adc_event *ev)
> > +{
> > +	const int INL = 2;
> > +	const int adc_chan = ev->channel;
> > +	const int orig = adc->thresh_data[adc_chan].high_thresh;
> > +	int val = orig;
> > +	int gain_drift;
> > +	int offset_drift;
> > +
> > +	if (!val)
> > +		return 0;
> 
> What is the reason to make val = 0 a special handling?
> IMHO this makes the threshold levels discontinuous.

These patches has gone through a couple of internal revisions already, and
one of the early versions used threshold == 0 to mean that the event was
disabled.

I will re-visit the threshold calculations and update for v2.
> 
> > +
> > +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > +	if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > +		val = (val * adc->adc_info[adc_chan].gain_error +
> > +		       adc->adc_info[adc_chan].offset) /
> 
> here it would make a difference for val == 0 and offset != 0
> 
> > +			1000;
> > +		gain_drift = 1002;
> > +		offset_drift = 2;
> 
> where do these magic constants come from?

If I remember correctly, they where taken from one of the many datasheets
that had fractions of information available for this chip. I will document
the variables and add references in v2.

> 
> > +	}
> > +	else {
> > +		gain_drift = 1022;
> > +		offset_drift = 36;
> 
> same here.
> 
> > +	}
> > +
> > +	// add tolerance to threshold
> > +	val = ((val + INL) * gain_drift) / 1000 + offset_drift;
> 
> here it would make a difference for val == 0.
> 
> > +
> > +	// clamp to max possible value
> > +	if (val > 0xFFF)
> > +		val = 0xFFF;
> > +
> > +	return val;
> > +}
> > +
> > +static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
> > +					      struct palmas_adc_event *ev)
> > +{
> > +	const int INL = 2;
> > +	const int adc_chan = ev->channel;
> > +	const int orig = adc->thresh_data[adc_chan].low_thresh;
> > +	int val = orig;
> > +	int gain_drift;
> > +	int offset_drift;
> > +
> > +	if (!val)
> > +		return val;
> 
> same here. And why return val and not 0 as above?

Yes, that indeed seem odd.

> 
> > +
> > +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > +        if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > +            val = (val * adc->adc_info[adc_chan].gain_error -
> > +		   adc->adc_info[adc_chan].offset) /
> > +		    1000;
> > +            gain_drift = 998;
> > +            offset_drift = 2;
> > +        }
> > +        else {
> > +            gain_drift = 978;
> > +            offset_drift = 36;
> 
> same here - how are they related to the constants in 
> palmas_gpadc_get_high_threshold_raw() ?
> 
> > +        }
> > +
> > +	// calculate tolerances
> > +	val = ((val - INL) * gain_drift) / 1000 - offset_drift;
> > +
> > +	// clamp to minimum 0
> > +	if (val < 0)
> > +		val = 0;
> > +
> > +	return val;
> > +}
> > +
> > static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> > 	struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> > {
> > @@ -431,8 +570,239 @@ static int palmas_gpadc_read_raw(struct iio_dev *indio_dev,
> > 	return ret;
> > }
> > 
> > +static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
> > +	const struct iio_chan_spec *chan, enum iio_event_type type,
> > +	enum iio_event_direction dir)
> > +{
> > +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > +	int adc_chan = chan->channel;
> > +	int ret = 0;
> > +
> > +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +
> > +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir)) {
> > +		ret = 1;
> > +	}
> > +
> > +	mutex_unlock(&adc->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
> > +				       struct palmas_adc_event *ev,
> > +				       struct palmas_adc_wakeup_property *wakeup)
> > +{
> > +	wakeup->adc_channel_number = ev->channel;
> > +	if (ev->direction == IIO_EV_DIR_RISING) {
> > +		wakeup->adc_low_threshold = 0;
> > +		wakeup->adc_high_threshold =
> > +			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> > +	}
> > +	else {
> > +		wakeup->adc_low_threshold =
> > +			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> > +		wakeup->adc_high_threshold = 0;
> > +	}
> > +}
> > +
> > +static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
> > +static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
> > +
> > +static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> > +{
> > +	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	bool enable;
> > +
> > +	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> > +	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> > +
> > +	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	if (!was_enabled && enable)
> > +		device_wakeup_enable(adc->dev);
> > +	else if (was_enabled && !enable)
> > +		device_wakeup_disable(adc->dev);
> > +
> > +	if (!enable)
> > +		return palmas_adc_wakeup_reset(adc);
> > +
> > +	// adjust levels
> > +	if (adc->wakeup1_enable)
> > +		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> > +	if (adc->wakeup2_enable)
> > +		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> > +
> > +	return palmas_adc_wakeup_configure(adc);
> > +}
> > +
> > +static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> > +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> > +{
> > +	struct palmas_adc_event *ev;
> > +	int adc_chan = chan->channel;
> > +
> > +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> > +		/* already enabled */
> > +		return 0;
> > +
> > +	if (adc->event0.channel == -1)
> > +		ev = &adc->event0;
> > +	else if (adc->event1.channel == -1) {
> > +		/* event0 has to be the lowest channel */
> > +		if (adc_chan < adc->event0.channel) {
> > +			adc->event1 = adc->event0;
> > +			ev = &adc->event0;
> > +		}
> > +		else
> > +			ev = &adc->event1;
> > +	}
> > +	else /* both AUTO channels already in use */ {
> > +		dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
> > +			 adc->event0.channel, adc->event1.channel);
> > +		return -EBUSY;
> > +	}
> > +
> > +	ev->channel = adc_chan;
> > +	ev->direction = dir;
> > +
> > +	return palmas_gpadc_reconfigure_event_channels(adc);
> > +}
> > +
> > +static int palmas_gpadc_disable_event_config(struct palmas_gpadc *adc,
> > +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> > +{
> > +	int adc_chan = chan->channel;
> > +	struct palmas_adc_event *ev =
> > +		palmas_gpadc_get_event_channel(adc, adc_chan, dir);
> > +
> > +	if (!ev)
> > +		return 0;
> > +
> > +	if (ev == &adc->event0) {
> > +		adc->event0 = adc->event1;
> > +		ev = &adc->event1;
> > +	}
> > +
> > +	ev->channel = -1;
> > +	ev->direction = IIO_EV_DIR_NONE;
> > +
> > +	return palmas_gpadc_reconfigure_event_channels(adc);
> > +}
> > +
> > +static int palmas_gpadc_write_event_config(struct iio_dev *indio_dev,
> > +	const struct iio_chan_spec *chan, enum iio_event_type type,
> > +	enum iio_event_direction dir, int state)
> > +{
> > +	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > +	int adc_chan = chan->channel;
> > +	int ret = 0;
> > +
> > +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +
> > +	if (state)
> > +		ret = palmas_gpadc_enable_event_config(adc, chan, dir);
> > +	else
> > +		ret = palmas_gpadc_disable_event_config(adc, chan, dir);
> > +
> > +	mutex_unlock(&adc->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int palmas_gpadc_read_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
> > +	int adc_chan = chan->channel;
> > +	int ret = 0;
> > +
> > +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		*val = (dir == IIO_EV_DIR_RISING) ?
> > +			adc->thresh_data[adc_chan].high_thresh :
> > +			adc->thresh_data[adc_chan].low_thresh;
> > +		ret = IIO_VAL_INT;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&adc->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int palmas_gpadc_write_event_value(struct iio_dev *indio_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 palmas_gpadc *adc = iio_priv(indio_dev);
> > +	int adc_chan = chan->channel;
> > +	int ret = 0;
> > +
> > +	if (adc_chan > PALMAS_ADC_CH_MAX || type != IIO_EV_TYPE_THRESH)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adc->lock);
> > +	switch (info) {
> > +	case IIO_EV_INFO_VALUE:
> > +		if (val < 0 || val > 0xFFF) {
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +		if (dir == IIO_EV_DIR_RISING)
> > +			adc->thresh_data[adc_chan].high_thresh = val;
> > +		else
> > +			adc->thresh_data[adc_chan].low_thresh = val;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> > +		ret = palmas_gpadc_reconfigure_event_channels(adc);
> > +
> > +	mutex_unlock(&adc->lock);
> > +
> > +	return ret;
> > +}
> > +
> > static const struct iio_info palmas_gpadc_iio_info = {
> > 	.read_raw = palmas_gpadc_read_raw,
> > +	.read_event_config = palmas_gpadc_read_event_config,
> > +	.write_event_config = palmas_gpadc_write_event_config,
> > +	.read_event_value = palmas_gpadc_read_event_value,
> > +	.write_event_value = palmas_gpadc_write_event_value,
> > +};
> > +
> > +static const struct iio_event_spec palmas_gpadc_events[] = {
> > +	{
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_RISING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +				BIT(IIO_EV_INFO_ENABLE),
> > +	}, {
> > +		.type = IIO_EV_TYPE_THRESH,
> > +		.dir = IIO_EV_DIR_FALLING,
> > +		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
> > +				BIT(IIO_EV_INFO_ENABLE),
> > +	},
> > };
> > 
> > #define PALMAS_ADC_CHAN_IIO(chan, _type, chan_info)	\
> > @@ -443,6 +813,8 @@ static const struct iio_info palmas_gpadc_iio_info = {
> > 			BIT(chan_info),			\
> > 	.indexed = 1,					\
> > 	.channel = PALMAS_ADC_CH_##chan,		\
> > +	.event_spec = palmas_gpadc_events,		\
> > +	.num_event_specs = ARRAY_SIZE(palmas_gpadc_events)	\
> > }
> > 
> > static const struct iio_chan_spec palmas_gpadc_iio_channel[] = {
> > @@ -492,9 +864,12 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
> > 	return 0;
> > }
> > 
> > -static void palmas_disable_wakeup(void *dev)
> > +static void palmas_disable_wakeup(void *data)
> > {
> > -	device_wakeup_disable(dev);
> > +	struct palmas_gpadc *adc = data;
> > +
> > +	if (adc->wakeup1_enable || adc->wakeup2_enable)
> > +		device_wakeup_disable(adc->dev);
> > }
> > 
> > static int palmas_gpadc_probe(struct platform_device *pdev)
> > @@ -547,36 +922,49 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> > 		return dev_err_probe(adc->dev, ret,
> > 				     "request irq %d failed\n", adc->irq);
> > 
> > +	adc->irq_auto_0 = platform_get_irq(pdev, 1);
> > +	if (adc->irq_auto_0 < 0)
> > +		return dev_err_probe(adc->dev, adc->irq_auto_0,
> > +				     "get auto0 irq failed\n");
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0, NULL,
> > +					palmas_gpadc_irq_auto, IRQF_ONESHOT,
> > +					"palmas-adc-auto-0", indio_dev);
> > +	if (ret < 0)
> > +		return dev_err_probe(adc->dev, ret,
> > +				     "request auto0 irq %d failed\n",
> > +				     adc->irq_auto_0);
> > +
> > +	adc->irq_auto_1 = platform_get_irq(pdev, 2);
> > +	if (adc->irq_auto_1 < 0)
> > +		return dev_err_probe(adc->dev, adc->irq_auto_1,
> > +				     "get auto1 irq failed\n");
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1, NULL,
> > +					palmas_gpadc_irq_auto, IRQF_ONESHOT,
> > +					"palmas-adc-auto-1", indio_dev);
> > +	if (ret < 0)
> > +		return dev_err_probe(adc->dev, ret,
> > +				     "request auto1 irq %d failed\n",
> > +				     adc->irq_auto_1);
> > +
> > 	if (gpadc_pdata->adc_wakeup1_data) {
> > 		memcpy(&adc->wakeup1_data, gpadc_pdata->adc_wakeup1_data,
> > 			sizeof(adc->wakeup1_data));
> > 		adc->wakeup1_enable = true;
> > -		adc->irq_auto_0 =  platform_get_irq(pdev, 1);
> > -		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_0,
> > -						NULL, palmas_gpadc_irq_auto,
> > -						IRQF_ONESHOT,
> > -						"palmas-adc-auto-0", adc);
> > -		if (ret < 0)
> > -			return dev_err_probe(adc->dev, ret,
> > -					     "request auto0 irq %d failed\n",
> > -					     adc->irq_auto_0);
> > 	}
> > 
> > 	if (gpadc_pdata->adc_wakeup2_data) {
> > 		memcpy(&adc->wakeup2_data, gpadc_pdata->adc_wakeup2_data,
> > 				sizeof(adc->wakeup2_data));
> > 		adc->wakeup2_enable = true;
> > -		adc->irq_auto_1 =  platform_get_irq(pdev, 2);
> > -		ret = devm_request_threaded_irq(&pdev->dev, adc->irq_auto_1,
> > -						NULL, palmas_gpadc_irq_auto,
> > -						IRQF_ONESHOT,
> > -						"palmas-adc-auto-1", adc);
> > -		if (ret < 0)
> > -			return dev_err_probe(adc->dev, ret,
> > -					     "request auto1 irq %d failed\n",
> > -					     adc->irq_auto_1);
> > 	}
> > 
> > +	adc->event0.channel = -1;
> > +	adc->event0.direction = IIO_EV_DIR_NONE;
> > +	adc->event1.channel = -1;
> > +	adc->event1.direction = IIO_EV_DIR_NONE;
> > +
> > 	/* set the current source 0 (value 0/5/15/20 uA => 0..3) */
> > 	if (gpadc_pdata->ch0_current <= 1)
> > 		adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
> > @@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> > 		return dev_err_probe(adc->dev, ret,
> > 				     "iio_device_register() failed\n");
> > 
> > -	device_set_wakeup_capable(&pdev->dev, 1);
> > 	for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
> > 		if (!(adc->adc_info[i].is_uncalibrated))
> > 			palmas_gpadc_calibrate(adc, i);
> > 	}
> > 
> > +	device_set_wakeup_capable(&pdev->dev, 1);
> > 	if (adc->wakeup1_enable || adc->wakeup2_enable) {
> > -		device_wakeup_enable(&pdev->dev);
> > -		ret = devm_add_action_or_reset(&pdev->dev,
> > -					       palmas_disable_wakeup,
> > -					       &pdev->dev);
> > +		ret = palmas_adc_wakeup_configure(adc);
> > 		if (ret)
> > 			return ret;
> > +		device_wakeup_enable(&pdev->dev);
> > 	}
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> > +				       palmas_disable_wakeup,
> > +				       adc);
> > +	if (ret)
> > +		return ret;
> > 
> > 	return 0;
> > }
> > @@ -755,16 +1146,11 @@ static int palmas_gpadc_suspend(struct device *dev)
> > {
> > 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > 	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
> > 	int ret;
> > 
> > -	if (!device_may_wakeup(dev) || !wakeup)
> > +	if (!device_may_wakeup(dev))
> > 		return 0;
> > 
> > -	ret = palmas_adc_wakeup_configure(adc);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > 	if (adc->wakeup1_enable)
> > 		enable_irq_wake(adc->irq_auto_0);
> > 
> > @@ -778,16 +1164,11 @@ static int palmas_gpadc_resume(struct device *dev)
> > {
> > 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > 	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int wakeup = adc->wakeup1_enable || adc->wakeup2_enable;
> > 	int ret;
> > 
> > -	if (!device_may_wakeup(dev) || !wakeup)
> > +	if (!device_may_wakeup(dev))
> > 		return 0;
> > 
> > -	ret = palmas_adc_wakeup_reset(adc);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > 	if (adc->wakeup1_enable)
> > 		disable_irq_wake(adc->irq_auto_0);
> > 
> > -- 
> > 2.25.1
> > 
> 
> 
> 

Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[Patrik Dahlström]

On Sun, Mar 26, 2023 at 05:51:01PM +0100, Jonathan Cameron wrote:
> On Sun, 19 Mar 2023 23:39:06 +0100
> Patrik Dahlström <risca@dalakolonin.se> wrote:
> 
> > The palmas gpadc block has support for monitoring up to 2 ADC channels
> > and issue an interrupt if they reach past a set threshold. The gpadc
> > driver had limited support for this through the adc_wakeup{1,2}_data
> > platform data. This however only allow a fixed threshold to be set at
> > boot, and would only enable it when entering sleep mode.
> > 
> > This change hooks into the IIO events system and exposes to userspace
> > the ability to configure threshold values for each channel individually,
> > but only allow up to 2 such thresholds to be enabled at any given time.
> 
> Add a comment here on what happens if userspace tries to set more than two.
> It's not as obvious as you'd think as we have some drivers that use a fifo
> approach so on setting the third event they push the oldest one out.

Will do!

Is there any preference to any one approach?

> 
> > 
> > The logic around suspend and resume had to be adjusted so that user
> > space configuration don't get reset on resume. Instead, any configured
> > adc auto wakeup gets enabled during probe.
> > 
> > Enabling a threshold from userspace will overwrite the adc wakeup
> > configuration set during probe. Depending on how you look at it, this
> > could also mean we allow userspace to update the adc wakeup thresholds.
> 
> I'm not sure I read the code right, but can you end up enabling a wakeup
> that wasn't previously present?  That seems likely something we should
> not be doing after boot.
> 
> One option here would be to make it either wakeup is supported, or events
> are supported.  I suspect no one uses the wakeup anyway so that shouldn't
> matter much (+ you remove it in next patch - do that first and this code
> becomes more obvious).
> 

My use case is for monitoring a volume wheel connected to one of the ADC
inputs of the palmas chip. By off-loading the monitoring to a separate
chip, the SoC can go to sleep and only wake up when the wheel is moved.

It made sense for my use case, but I see your point. IIO events and wakeup
triggers should be treated as separate things. I will look into defining
the dev_pm_info of the device. Then userspace should be able to control
wakeup from /sys/devices/.../power/wakeup.

However, suspend and resume is a bit flaky on my board so testing might be
too. If the board reacts and at least tries to resume should indicate that
the code works, no?

In any case, I will remove the old wakeup code first in v2.

> 
> A few trivial comments inline.

I will adress them in v2. They all made perfect sense.

> > 
> > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> 
> >  
> > @@ -280,6 +326,9 @@ static int palmas_gpadc_read_prepare(struct palmas_gpadc *adc, int adc_chan)
> >  {
> >  	int ret;
> >  
> > +	if (palmas_gpadc_channel_is_freerunning(adc, adc_chan))
> > +		return 0; // ADC already running
> 
> /* */
> 
> ...
> 
> >  
> > +static int palmas_gpadc_get_high_threshold_raw(struct palmas_gpadc *adc,
> > +					       struct palmas_adc_event *ev)
> > +{
> > +	const int INL = 2;
> > +	const int adc_chan = ev->channel;
> > +	const int orig = adc->thresh_data[adc_chan].high_thresh;
> > +	int val = orig;
> > +	int gain_drift;
> > +	int offset_drift;
> > +
> > +	if (!val)
> > +		return 0;
> > +
> > +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > +	if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > +		val = (val * adc->adc_info[adc_chan].gain_error +
> > +		       adc->adc_info[adc_chan].offset) /
> > +			1000;
> > +		gain_drift = 1002;
> > +		offset_drift = 2;
> > +	}
> > +	else {
> > +		gain_drift = 1022;
> > +		offset_drift = 36;
> > +	}
> > +
> > +	// add tolerance to threshold
> > +	val = ((val + INL) * gain_drift) / 1000 + offset_drift;
> > +
> > +	// clamp to max possible value
> /* clamp .. */
> val = min(val, 0xFFF);
> 
> 
> > +	if (val > 0xFFF)
> > +		val = 0xFFF;
> > +
> > +	return val;
> > +}
> > +
> > +static int palmas_gpadc_get_low_threshold_raw(struct palmas_gpadc *adc,
> > +					      struct palmas_adc_event *ev)
> > +{
> > +	const int INL = 2;
> > +	const int adc_chan = ev->channel;
> > +	const int orig = adc->thresh_data[adc_chan].low_thresh;
> > +	int val = orig;
> > +	int gain_drift;
> > +	int offset_drift;
> > +
> > +	if (!val)
> > +		return val;
> > +
> > +	val = (val * 1000) / adc->adc_info[adc_chan].gain;
> > +
> > +        if (!adc->adc_info[adc_chan].is_uncalibrated) {
> > +            val = (val * adc->adc_info[adc_chan].gain_error -
> > +		   adc->adc_info[adc_chan].offset) /
> > +		    1000;
> > +            gain_drift = 998;
> > +            offset_drift = 2;
> > +        }
> > +        else {
> > +            gain_drift = 978;
> > +            offset_drift = 36;
> > +        }
> > +
> > +	// calculate tolerances
> /* */
> 
> + I think this could do with more information on why a tweak is needed.
> 
> > +	val = ((val - INL) * gain_drift) / 1000 - offset_drift;
> > +
> > +	// clamp to minimum 0
> 
> /* */ for all comments. 
> 
> val = max(0, val); then comment may not be needed.
> 
> > +	if (val < 0)
> > +		val = 0;
> > +
> > +	return val;
> > +}
> 
> > +static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> > +{
> > +	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	bool enable;
> > +
> > +	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> > +	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> > +
> > +	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	if (!was_enabled && enable)
> > +		device_wakeup_enable(adc->dev);
> > +	else if (was_enabled && !enable)
> > +		device_wakeup_disable(adc->dev);
> > +
> > +	if (!enable)
> > +		return palmas_adc_wakeup_reset(adc);
> > +
> > +	// adjust levels
> 
> /* adjust levels */ 
> 
> > +	if (adc->wakeup1_enable)
> > +		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> > +	if (adc->wakeup2_enable)
> > +		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> > +
> > +	return palmas_adc_wakeup_configure(adc);
> > +}
> > +
> > +static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> > +	const struct iio_chan_spec *chan, enum iio_event_direction dir)
> > +{
> > +	struct palmas_adc_event *ev;
> > +	int adc_chan = chan->channel;
> > +
> > +	if (palmas_gpadc_get_event_channel(adc, adc_chan, dir))
> > +		/* already enabled */
> > +		return 0;
> > +
> > +	if (adc->event0.channel == -1)
> 
> I'd add brackets for all legs of this if / else once one of them needs
> it. Tends to end up more readable.
> 
> > +		ev = &adc->event0;
> > +	else if (adc->event1.channel == -1) {
> > +		/* event0 has to be the lowest channel */
> > +		if (adc_chan < adc->event0.channel) {
> > +			adc->event1 = adc->event0;
> > +			ev = &adc->event0;
> > +		}
> > +		else
> > +			ev = &adc->event1;
> > +	}
> 
> } else /*...
> 
> > +	else /* both AUTO channels already in use */ {
> > +		dev_warn(adc->dev, "event0 - %d, event1 - %d\n",
> > +			 adc->event0.channel, adc->event1.channel);
> > +		return -EBUSY;
> > +	}
> > +
> > +	ev->channel = adc_chan;
> > +	ev->direction = dir;
> > +
> > +	return palmas_gpadc_reconfigure_event_channels(adc);
> > +}
> 
> > +
> > +static int palmas_gpadc_write_event_value(struct iio_dev *indio_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)
> 
> Prefer parameters aligned just after (
> 
> > +{
> ...
> 
> 
> >  
> >  static int palmas_gpadc_probe(struct platform_device *pdev)
> 
> ...
> 
> >  	/* set the current source 0 (value 0/5/15/20 uA => 0..3) */
> >  	if (gpadc_pdata->ch0_current <= 1)
> >  		adc->ch0_current = PALMAS_ADC_CH0_CURRENT_SRC_0;
> > @@ -610,20 +998,23 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> >  		return dev_err_probe(adc->dev, ret,
> >  				     "iio_device_register() failed\n");
> >  
> > -	device_set_wakeup_capable(&pdev->dev, 1);
> >  	for (i = 0; i < PALMAS_ADC_CH_MAX; i++) {
> >  		if (!(adc->adc_info[i].is_uncalibrated))
> >  			palmas_gpadc_calibrate(adc, i);
> >  	}
> >  
> > +	device_set_wakeup_capable(&pdev->dev, 1);
> >  	if (adc->wakeup1_enable || adc->wakeup2_enable) {
> > -		device_wakeup_enable(&pdev->dev);
> > -		ret = devm_add_action_or_reset(&pdev->dev,
> > -					       palmas_disable_wakeup,
> > -					       &pdev->dev);
> > +		ret = palmas_adc_wakeup_configure(adc);
> >  		if (ret)
> >  			return ret;
> > +		device_wakeup_enable(&pdev->dev);
> 
> >  	}
> > +	ret = devm_add_action_or_reset(&pdev->dev,
> 
> Add a comment for this to explain why it might need disabling even if
> it wasn't enabled above.  I think if you just drop wakeup support in
> general that will be fine given we have no known users.
> 

I'm one such user.

> 
> > +				       palmas_disable_wakeup,
> > +				       adc);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return 0;
> >  }
> 
> 
> 

Re: [PATCH 3/3] iio: adc: palmas_gpadc: remove palmas_adc_wakeup_property

[Patrik Dahlström]

On Sun, Mar 26, 2023 at 05:59:28PM +0100, Jonathan Cameron wrote:
> On Sun, 19 Mar 2023 23:39:08 +0100
> Patrik Dahlström <risca@dalakolonin.se> wrote:
> 
> > This struct contain essentially the same information as
> > palmas_adc_event and palmas_gpadc_thresholds combined, but with more
> > ambiguity: the code decided whether to trigger on rising or falling edge
> > based on the high threshold being non-zero.
> > 
> > Since its use in platform data has now been removed, we can remove it
> > entirely.
> > 
> > Lastly, the use case for waking up the cpu from sleep mode when a
> > threshold has been passed is no longer the primary use for events so all
> > code is changed to say "event" instead of "wakeup".
> Good. I nearly pointed this out in the earlier patch.  The wakeup naming
> was confusing. However, I'd prefer that was done in a separate patch to
> any other changes.  It's hard to spot the meaningful stuff when there
> is a lot of renaming going on.

Since I was doing this patch last, it made little sense to keep the wakeup
naming when removing the wakeup property. However, as you pointed out in
your review of the previous patches, it would be better to first remove the
wakeup property and then add iio events support.
> 
> A few questions / comments inline.
> 
> Jonathan
> 
> > 
> > Signed-off-by: Patrik Dahlström <risca@dalakolonin.se>
> > ---
> >  drivers/iio/adc/palmas_gpadc.c | 94 +++++++++++++---------------------
> >  include/linux/mfd/palmas.h     |  6 ---
> >  2 files changed, 36 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/palmas_gpadc.c b/drivers/iio/adc/palmas_gpadc.c
> > index 419d7db51345..042b68f29444 100644
> > --- a/drivers/iio/adc/palmas_gpadc.c
> > +++ b/drivers/iio/adc/palmas_gpadc.c
> > @@ -122,10 +122,8 @@ struct palmas_gpadc {
> >  	int				irq_auto_1;
> >  	struct palmas_gpadc_info	*adc_info;
> >  	struct completion		conv_completion;
> > -	struct palmas_adc_wakeup_property wakeup1_data;
> > -	struct palmas_adc_wakeup_property wakeup2_data;
> > -	bool				wakeup1_enable;
> > -	bool				wakeup2_enable;
> > +	bool				event0_enable;
> > +	bool				event1_enable;
> >  	int				auto_conversion_period;
> >  	struct mutex			lock;
> >  	struct palmas_adc_event		event0;
> > @@ -592,50 +590,26 @@ static int palmas_gpadc_read_event_config(struct iio_dev *indio_dev,
> >  	return ret;
> >  }
> >  
> > -static void palmas_adc_event_to_wakeup(struct palmas_gpadc *adc,
> > -				       struct palmas_adc_event *ev,
> > -				       struct palmas_adc_wakeup_property *wakeup)
> > -{
> > -	wakeup->adc_channel_number = ev->channel;
> > -	if (ev->direction == IIO_EV_DIR_RISING) {
> > -		wakeup->adc_low_threshold = 0;
> > -		wakeup->adc_high_threshold =
> > -			palmas_gpadc_get_high_threshold_raw(adc, &adc->event0);
> > -	}
> > -	else {
> > -		wakeup->adc_low_threshold =
> > -			palmas_gpadc_get_low_threshold_raw(adc, &adc->event0);
> > -		wakeup->adc_high_threshold = 0;
> > -	}
> > -}
> > -
> > -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc);
> > -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc);
> > +static int palmas_adc_events_configure(struct palmas_gpadc *adc);
> > +static int palmas_adc_events_reset(struct palmas_gpadc *adc);
> >  
> >  static int palmas_gpadc_reconfigure_event_channels(struct palmas_gpadc *adc)
> >  {
> > -	bool was_enabled = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	bool was_enabled = adc->event0_enable || adc->event1_enable;
> >  	bool enable;
> >  
> > -	adc->wakeup1_enable = adc->event0.channel == -1 ? false : true;
> > -	adc->wakeup2_enable = adc->event1.channel == -1 ? false : true;
> > +	adc->event0_enable = adc->event0.channel == -1 ? false : true;
> > +	adc->event1_enable = adc->event1.channel == -1 ? false : true;
> >  
> > -	enable = adc->wakeup1_enable || adc->wakeup2_enable;
> > +	enable = adc->event0_enable || adc->event1_enable;
> >  	if (!was_enabled && enable)
> >  		device_wakeup_enable(adc->dev);
> >  	else if (was_enabled && !enable)
> >  		device_wakeup_disable(adc->dev);
> >  
> > -	if (!enable)
> > -		return palmas_adc_wakeup_reset(adc);
> > -
> > -	// adjust levels
> > -	if (adc->wakeup1_enable)
> > -		palmas_adc_event_to_wakeup(adc, &adc->event0, &adc->wakeup1_data);
> > -	if (adc->wakeup2_enable)
> > -		palmas_adc_event_to_wakeup(adc, &adc->event1, &adc->wakeup2_data);
> > -
> > -	return palmas_adc_wakeup_configure(adc);
> > +	return enable ?
> > +		palmas_adc_events_configure(adc) :
> > +		palmas_adc_events_reset(adc);
> >  }
> >  
> >  static int palmas_gpadc_enable_event_config(struct palmas_gpadc *adc,
> > @@ -864,12 +838,14 @@ static int palmas_gpadc_get_adc_dt_data(struct platform_device *pdev,
> >  	return 0;
> >  }
> >  
> > -static void palmas_disable_wakeup(void *data)
> > +static void palmas_disable_events(void *data)
> >  {
> >  	struct palmas_gpadc *adc = data;
> >  
> > -	if (adc->wakeup1_enable || adc->wakeup2_enable)
> > +	if (adc->event0_enable || adc->event1_enable) {
> > +		palmas_adc_events_reset(adc);
> 
> I can't immediately follow why this reset is needed when it wasn't before.
> Perhaps that will be clearer once the renames aren't in the same patch.

The original code would only enable adc events when entering any kind of
sleep mode and then reset when resuming, hence the name wakeupX_enable. The
new code allow adc events to be enabled at any time. palmas_disable_events
is run when unloading the module and as such it makes sense to also reset
the adc.

> 
> >  		device_wakeup_disable(adc->dev);
> > +	}
> >  }
> >  
> >  static int palmas_gpadc_probe(struct platform_device *pdev)
> > @@ -993,7 +969,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> >  
> >  	device_set_wakeup_capable(&pdev->dev, 1);
> >  	ret = devm_add_action_or_reset(&pdev->dev,
> > -				       palmas_disable_wakeup,
> > +				       palmas_disable_events,
> >  				       adc);
> >  	if (ret)
> >  		return ret;
> > @@ -1001,7 +977,7 @@ static int palmas_gpadc_probe(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> > +static int palmas_adc_events_configure(struct palmas_gpadc *adc)
> >  {
> >  	int adc_period, conv;
> >  	int i;
> > @@ -1027,16 +1003,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  	}
> >  
> >  	conv = 0;
> > -	if (adc->wakeup1_enable) {
> > +	if (adc->event0_enable) {
> > +		struct palmas_adc_event *ev = &adc->event0;
> >  		int polarity;
> >  
> > -		ch0 = adc->wakeup1_data.adc_channel_number;
> > +		ch0 = ev->channel;
> >  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV0_EN;
> > -		if (adc->wakeup1_data.adc_high_threshold > 0) {
> > -			thres = adc->wakeup1_data.adc_high_threshold;
> > +
> > +		if (ev->direction == IIO_EV_DIR_RISING) {
> > +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
> >  			polarity = 0;
> >  		} else {
> > -			thres = adc->wakeup1_data.adc_low_threshold;
> > +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
> >  			polarity = PALMAS_GPADC_THRES_CONV0_MSB_THRES_CONV0_POL;
> >  		}
> >  
> > @@ -1058,16 +1036,18 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  		}
> >  	}
> >  
> > -	if (adc->wakeup2_enable) {
> > +	if (adc->event1_enable) {
> > +		struct palmas_adc_event *ev = &adc->event1;
> >  		int polarity;
> >  
> > -		ch1 = adc->wakeup2_data.adc_channel_number;
> > +		ch1 = ev->channel;
> >  		conv |= PALMAS_GPADC_AUTO_CTRL_AUTO_CONV1_EN;
> > -		if (adc->wakeup2_data.adc_high_threshold > 0) {
> > -			thres = adc->wakeup2_data.adc_high_threshold;
> > +
> > +		if (ev->direction == IIO_EV_DIR_RISING) {
> > +			thres = palmas_gpadc_get_high_threshold_raw(adc, ev);
> >  			polarity = 0;
> >  		} else {
> > -			thres = adc->wakeup2_data.adc_low_threshold;
> > +			thres = palmas_gpadc_get_low_threshold_raw(adc, ev);
> >  			polarity = PALMAS_GPADC_THRES_CONV1_MSB_THRES_CONV1_POL;
> >  		}
> >  
> > @@ -1106,7 +1086,7 @@ static int palmas_adc_wakeup_configure(struct palmas_gpadc *adc)
> >  	return ret;
> >  }
> >  
> > -static int palmas_adc_wakeup_reset(struct palmas_gpadc *adc)
> > +static int palmas_adc_events_reset(struct palmas_gpadc *adc)
> >  {
> >  	int ret;
> >  
> > @@ -1128,15 +1108,14 @@ static int palmas_gpadc_suspend(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int ret;
> 
> ?  Seems unrelated - perhaps should be in earlier patch.

You're right. I'll look into it.

> 
> >  
> >  	if (!device_may_wakeup(dev))
> >  		return 0;
> >  
> > -	if (adc->wakeup1_enable)
> > +	if (adc->event0_enable)
> >  		enable_irq_wake(adc->irq_auto_0);
> >  
> > -	if (adc->wakeup2_enable)
> > +	if (adc->event1_enable)
> >  		enable_irq_wake(adc->irq_auto_1);
> >  
> >  	return 0;
> > @@ -1146,15 +1125,14 @@ static int palmas_gpadc_resume(struct device *dev)
> >  {
> >  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> >  	struct palmas_gpadc *adc = iio_priv(indio_dev);
> > -	int ret;
> 
> ?
> 
> >  
> >  	if (!device_may_wakeup(dev))
> >  		return 0;
> >  
> > -	if (adc->wakeup1_enable)
> > +	if (adc->event0_enable)
> >  		disable_irq_wake(adc->irq_auto_0);
> >  
> > -	if (adc->wakeup2_enable)
> > +	if (adc->event1_enable)
> >  		disable_irq_wake(adc->irq_auto_1);
> >  
> >  	return 0;
> 
> 

Re: [PATCH 1/3] iio: adc: palmas_gpadc: add support for iio threshold events

[Jonathan Cameron]

On Sat, 1 Apr 2023 20:45:07 +0200
Patrik Dahlström <risca@dalakolonin.se> wrote:

> On Sun, Mar 26, 2023 at 05:51:01PM +0100, Jonathan Cameron wrote:
> > On Sun, 19 Mar 2023 23:39:06 +0100
> > Patrik Dahlström <risca@dalakolonin.se> wrote:
> >   
> > > The palmas gpadc block has support for monitoring up to 2 ADC channels
> > > and issue an interrupt if they reach past a set threshold. The gpadc
> > > driver had limited support for this through the adc_wakeup{1,2}_data
> > > platform data. This however only allow a fixed threshold to be set at
> > > boot, and would only enable it when entering sleep mode.
> > > 
> > > This change hooks into the IIO events system and exposes to userspace
> > > the ability to configure threshold values for each channel individually,
> > > but only allow up to 2 such thresholds to be enabled at any given time.  
> > 
> > Add a comment here on what happens if userspace tries to set more than two.
> > It's not as obvious as you'd think as we have some drivers that use a fifo
> > approach so on setting the third event they push the oldest one out.  
> 
> Will do!
> 
> Is there any preference to any one approach?

Not really.  Userspace should be checking either for an error, or that
the setup matches what it thinks it configured (after finishing configuring)
so it should cope with either.

> 
> >   
> > > 
> > > The logic around suspend and resume had to be adjusted so that user
> > > space configuration don't get reset on resume. Instead, any configured
> > > adc auto wakeup gets enabled during probe.
> > > 
> > > Enabling a threshold from userspace will overwrite the adc wakeup
> > > configuration set during probe. Depending on how you look at it, this
> > > could also mean we allow userspace to update the adc wakeup thresholds.  
> > 
> > I'm not sure I read the code right, but can you end up enabling a wakeup
> > that wasn't previously present?  That seems likely something we should
> > not be doing after boot.
> > 
> > One option here would be to make it either wakeup is supported, or events
> > are supported.  I suspect no one uses the wakeup anyway so that shouldn't
> > matter much (+ you remove it in next patch - do that first and this code
> > becomes more obvious).
> >   
> 
> My use case is for monitoring a volume wheel connected to one of the ADC
> inputs of the palmas chip. By off-loading the monitoring to a separate
> chip, the SoC can go to sleep and only wake up when the wheel is moved.
> 
> It made sense for my use case, but I see your point. IIO events and wakeup
> triggers should be treated as separate things. I will look into defining
> the dev_pm_info of the device. Then userspace should be able to control
> wakeup from /sys/devices/.../power/wakeup.
> 
> However, suspend and resume is a bit flaky on my board so testing might be
> too. If the board reacts and at least tries to resume should indicate that
> the code works, no?

That will be fine. Obviously good to debug the other issues with resume though!

> 
> In any case, I will remove the old wakeup code first in v2.
> 
> > 
> > A few trivial comments inline.  
> 
> I will adress them in v2. They all made perfect sense.
>> > > +	ret = devm_add_action_or_reset(&pdev->dev,  
> > 
> > Add a comment for this to explain why it might need disabling even if
> > it wasn't enabled above.  I think if you just drop wakeup support in
> > general that will be fine given we have no known users.
> >   
> 
> I'm one such user.
Fair enough :)

Jonathan

> 
> >   
> > > +				       palmas_disable_wakeup,
> > > +				       adc);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	return 0;
> > >  }  
> > 
> > 
> >