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

  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(-)
  

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
>
  

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;
>  }
  

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

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;
> >  }
> 
> 
>
  

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;
> > >  }  
> > 
> > 
> >