Message-ID: <mfuzlmbylzjcbepyqyrsgcbzdasjagk3mtu23r7kvzlmmdykyq@2jn4rlksw2dx>
Date: Fri, 27 Dec 2024 10:39:06 +0100
From: Uwe Kleine-König <u.kleine-koenig@...libre.com>
To: Julien Stephan <jstephan@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, 
	Michael Hennerich <Michael.Hennerich@...log.com>, Nuno Sá <nuno.sa@...log.com>, 
	David Lechner <dlechner@...libre.com>, Jonathan Cameron <jic23@...nel.org>, 
	Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH RFC v2 3/4] iio: adc: ad7380: add alert support

Hello Julien,

On Tue, Dec 24, 2024 at 10:34:32AM +0100, Julien Stephan wrote:
> +static void ad7380_enable_irq(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && !atomic_cmpxchg(&st->irq_enabled, 0, 1))
> +		enable_irq(st->spi->irq);
> +}
> +
> +static void ad7380_disable_irq(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0)) {
> +		disable_irq(st->spi->irq);
> +		synchronize_irq(st->spi->irq);

disable_irq() already cares for synchronize_irq().

> +	}
> +}
> +
> +static void ad7380_disable_irq_nosync(struct ad7380_state *st)
> +{
> +	if (st->spi->irq && atomic_cmpxchg(&st->irq_enabled, 1, 0))
> +		disable_irq_nosync(st->spi->irq);
> +}
> +
>  static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
>  				     u32 writeval, u32 *readval)
>  {
> @@ -727,8 +791,8 @@ static int ad7380_regval_to_osr(int regval)
>  
>  static int ad7380_get_osr(struct ad7380_state *st, int *val)
>  {
> -	int tmp;
> -	int ret = 0;
> +	u32 tmp;
> +	int ret;

unrelated change?

>  	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
>  	if (ret)
> @@ -834,6 +898,13 @@ static int ad7380_update_xfers(struct ad7380_state *st,
>  	return 0;
>  }
>  
> +static void ad7380_handle_event_reset_timeout(struct timer_list *t)
> +{
> +	struct ad7380_state *st = from_timer(st, t, alert_timer);
> +
> +	ad7380_enable_irq(st);
> +}
> +
>  static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  {
>  	struct ad7380_state *st = iio_priv(indio_dev);
> @@ -841,6 +912,12 @@ static int ad7380_triggered_buffer_preenable(struct iio_dev *indio_dev)
>  	struct spi_message *msg = &st->normal_msg;
>  	int ret;
>  
> +	timer_setup(&st->alert_timer, ad7380_handle_event_reset_timeout, 0);
> +
> +	ad7380_enable_irq(st);
> +
> +	st->buffered_read_enabled = true;
> +
>  	/*
>  	 * Currently, we always read all channels at the same time. The scan_type
>  	 * is the same for all channels, so we just pass the first channel.
> @@ -894,6 +971,8 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
>  	struct spi_message *msg = &st->normal_msg;
>  	int ret;
>  
> +	st->buffered_read_enabled = false;
> +
>  	if (st->seq) {
>  		ret = regmap_update_bits(st->regmap,
>  					 AD7380_REG_ADDR_CONFIG1,
> @@ -908,6 +987,9 @@ static int ad7380_triggered_buffer_postdisable(struct iio_dev *indio_dev)
>  
>  	spi_unoptimize_message(msg);
>  
> +	ad7380_disable_irq(st);
> +	timer_shutdown_sync(&st->alert_timer);

If the timer triggers between ad7380_disable_irq() and
timer_shutdown_sync() the irq ends up enabled. It's unclear to me why
the irq is enabled in a timer.

> +
>  	return 0;
>  }
>  
> @@ -1002,8 +1084,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		ret = ad7380_read_direct(st, chan->scan_index,
> -					 scan_type, val);
> +		ad7380_enable_irq(st);
> +
> +		ret = ad7380_read_direct(st, chan->scan_index, scan_type, val);
> +
> +		ad7380_disable_irq(st);

Why do you enable the irq only during register reading? Wouldn't it be
natural to have it enabled constantly (assuming a valid limit is
configured)?

>  		iio_device_release_direct_mode(indio_dev);
>  
> @@ -1151,12 +1236,190 @@ static int ad7380_get_current_scan_type(const struct iio_dev *indio_dev,
>  					    : AD7380_SCAN_TYPE_NORMAL;
>  }
>  
> +static int ad7380_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 ad7380_state *st = iio_priv(indio_dev);
> +	int alert_en, tmp, ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD7380_REG_ADDR_CONFIG1, &tmp);
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	alert_en = FIELD_GET(AD7380_CONFIG1_ALERTEN, tmp);
> +
> +	return alert_en;
> +}
> +
> +static int ad7380_write_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir,
> +				     bool state)
> +{
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = iio_device_claim_direct_mode(indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(st->regmap,
> +				 AD7380_REG_ADDR_CONFIG1,
> +				 AD7380_CONFIG1_ALERTEN,
> +				 FIELD_PREP(AD7380_CONFIG1_ALERTEN, state));
> +
> +	iio_device_release_direct_mode(indio_dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	return 0;

The last three code lines are equivalent to a plain

	return ret;

> +}
> +
> [...]
> @@ -1190,6 +1459,50 @@ static int ad7380_init(struct ad7380_state *st, bool external_ref_en)
>  					     AD7380_NUM_SDO_LINES));
>  }
>  
> +static irqreturn_t ad7380_primary_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +
> +	if (!atomic_read(&st->irq_enabled))
> +		return IRQ_NONE;
> +
> +	ad7380_disable_irq_nosync(st);

I wonder why this is needed. The irq is requested with IRQF_ONESHOT, so
the irq is already masked here? And if you drop the masking here, you
can (I think) simplify the irq state tracking and drop the _nosync
variant completely.

> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static irqreturn_t ad7380_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	s64 timestamp = iio_get_time_ns(indio_dev);
> +	struct ad7380_state *st = iio_priv(indio_dev);
> +	const struct iio_chan_spec *chan = &indio_dev->channels[0];
> +	unsigned int i;
> +
> +	for (i = 0; i < st->chip_info->num_channels - 1; i++) {
> +		iio_push_event(indio_dev,
> +			       chan->differential ?
> +			       IIO_DIFF_EVENT_CODE(IIO_VOLTAGE,
> +						   2 * i,
> +						   2 * i + 1,
> +						   IIO_EV_TYPE_THRESH,
> +						   IIO_EV_DIR_EITHER) :
> +			       IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE,
> +						    i,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_EITHER),
> +			       timestamp);
> +	}
> +
> +	if (st->spi->irq && st->buffered_read_enabled)
> +		mod_timer(&st->alert_timer,
> +			  jiffies +
> +			  msecs_to_jiffies(st->alert_reset_timeout_ms));
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int ad7380_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> @@ -1361,6 +1674,39 @@ static int ad7380_probe(struct spi_device *spi)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->available_scan_masks = st->chip_info->available_scan_masks;
>  
> +	if (spi->irq) {
> +		struct iio_chan_spec *chans;
> +		size_t size;
> +		int ret;
> +
> +		ret = devm_request_threaded_irq(dev, spi->irq,
> +						&ad7380_primary_event_handler,
> +						&ad7380_event_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT

I didn't double check, but I think with the explicit
IRQF_TRIGGER_FALLING you overwrite the trigger info from the device
tree.

> +						| IRQF_NO_AUTOEN,
> +						indio_dev->name,
> +						indio_dev);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to request irq\n");
> +
> +		atomic_set(&st->irq_enabled, 0);
> +
> +		/*
> +		 * Copy channels to be able to update the event_spec, since some
> +		 * attributes are visible only when irq are configured
> +		 */
> +		size = indio_dev->num_channels * sizeof(*indio_dev->channels);
> +		chans = devm_kzalloc(dev, size, GFP_KERNEL);
> +		if (!chans)
> +			return -ENOMEM;
> +
> +		memcpy(chans, indio_dev->channels, size);
> +		chans->event_spec = ad7380_events_irq;
> +		chans->num_event_specs = ARRAY_SIZE(ad7380_events_irq);
> +
> +		indio_dev->channels = chans;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
>  					      iio_pollfunc_store_time,
>  					      ad7380_trigger_handler,

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists