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
Attachment:
signature.asc
Description: PGP signature
