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

 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 */
-- 
2.35.1.1320.gc452695387.dirty

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

On 7/24/25 17:32, Jonathan Cameron wrote:
> 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.

Anand left some time ago. Salih or Conall should be able to provide some input.

Thanks,
Michal

On Fri, 25 Jul 2025 06:47:16 +0200
Michal Simek <michal.simek@amd.com> wrote:

> On 7/24/25 17:32, Jonathan Cameron wrote:
> > 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.  
> 
> Anand left some time ago. Salih or Conall should be able to provide some input.
> 
> Thanks,
> Michal
> 

Hi Michal,

Thanks for letting me know.  Would be good to have a MAINTAINERS update patch
to remove Anand + ideally add someone else.  If not to mark it orphaned
(will still be covered by the top level IIO entry).

On 7/27/25 17:43, Jonathan Cameron wrote:
> On Fri, 25 Jul 2025 06:47:16 +0200
> Michal Simek <michal.simek@amd.com> wrote:
> 
>> On 7/24/25 17:32, Jonathan Cameron wrote:
>>> 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.
>>
>> Anand left some time ago. Salih or Conall should be able to provide some input.
>>
>> Thanks,
>> Michal
>>
> 
> Hi Michal,
> 
> Thanks for letting me know.  Would be good to have a MAINTAINERS update patch
> to remove Anand + ideally add someone else.  If not to mark it orphaned
> (will still be covered by the top level IIO entry).

Salih: Can you please send a patch for it?

It is also covered my fragment that's why you don't need to explicitly add me there.

Thanks,
Michal