Re: [PATCH] iio: xilinx-ams: Unmask interrupts after updating alarms

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Jul 2025 20:28:47 -0400
Sean Anderson <sean.anderson@linux.dev> wrote:

> To convert level-triggered alarms into edge-triggered IIO events, alarms
> are masked when they are triggered. To ensure we catch subsequent
> alarms, we then periodically poll to see if the alarm is still active.
> If it isn't, we unmask it. Active but masked alarms are stored in
> current_masked_alarm.
> 
> If an active alarm is disabled, it will remain set in
> current_masked_alarm until ams_unmask_worker clears it. If the alarm is
> re-enabled before ams_unmask_worker runs, then it will never be cleared
> from current_masked_alarm. This will prevent the alarm event from being
> pushed even if the alarm is still active.
> 
> Fix this by recalculating current_masked_alarm immediately when enabling
> or disabling alarms.
> 
> Fixes: d5c70627a794 ("iio: adc: Add Xilinx AMS driver")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
Anand?  

This seems fine to me, but I'm not that familiar with the hardware or driver.

> 
>  drivers/iio/adc/xilinx-ams.c | 45 ++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c
> index 76dd0343f5f7..180d4140993d 100644
> --- a/drivers/iio/adc/xilinx-ams.c
> +++ b/drivers/iio/adc/xilinx-ams.c
> @@ -389,6 +389,29 @@ static void ams_update_pl_alarm(struct ams *ams, unsigned long alarm_mask)
>  	ams_pl_update_reg(ams, AMS_REG_CONFIG3, AMS_REGCFG3_ALARM_MASK, cfg);
>  }
>  
> +static void ams_unmask(struct ams *ams)
> +{
> +	unsigned int status, unmask;
> +
> +	status = readl(ams->base + AMS_ISR_0);
> +
> +	/* Clear those bits which are not active anymore */
> +	unmask = (ams->current_masked_alarm ^ status) & ams->current_masked_alarm;
> +
> +	/* Clear status of disabled alarm */
> +	unmask |= ams->intr_mask;
> +
> +	ams->current_masked_alarm &= status;
> +
> +	/* Also clear those which are masked out anyway */
> +	ams->current_masked_alarm &= ~ams->intr_mask;
> +
> +	/* Clear the interrupts before we unmask them */
> +	writel(unmask, ams->base + AMS_ISR_0);
> +
> +	ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
> +}
> +
>  static void ams_update_alarm(struct ams *ams, unsigned long alarm_mask)
>  {
>  	unsigned long flags;
> @@ -401,6 +424,7 @@ static void ams_update_alarm(struct ams *ams, unsigned long alarm_mask)
>  
>  	spin_lock_irqsave(&ams->intr_lock, flags);
>  	ams_update_intrmask(ams, AMS_ISR0_ALARM_MASK, ~alarm_mask);
> +	ams_unmask(ams);
>  	spin_unlock_irqrestore(&ams->intr_lock, flags);
>  }
>  
> @@ -1035,28 +1059,9 @@ static void ams_handle_events(struct iio_dev *indio_dev, unsigned long events)
>  static void ams_unmask_worker(struct work_struct *work)
>  {
>  	struct ams *ams = container_of(work, struct ams, ams_unmask_work.work);
> -	unsigned int status, unmask;
>  
>  	spin_lock_irq(&ams->intr_lock);
> -
> -	status = readl(ams->base + AMS_ISR_0);
> -
> -	/* Clear those bits which are not active anymore */
> -	unmask = (ams->current_masked_alarm ^ status) & ams->current_masked_alarm;
> -
> -	/* Clear status of disabled alarm */
> -	unmask |= ams->intr_mask;
> -
> -	ams->current_masked_alarm &= status;
> -
> -	/* Also clear those which are masked out anyway */
> -	ams->current_masked_alarm &= ~ams->intr_mask;
> -
> -	/* Clear the interrupts before we unmask them */
> -	writel(unmask, ams->base + AMS_ISR_0);
> -
> -	ams_update_intrmask(ams, ~AMS_ALARM_MASK, ~AMS_ALARM_MASK);
> -
> +	ams_unmask(ams);
>  	spin_unlock_irq(&ams->intr_lock);
>  
>  	/* If still pending some alarm re-trigger the timer */