Re: [PATCH 1/2] iio: adc: qcom-spmi-adc5-gen3: Share SDAM0 IRQ with ADC_TM auxiliary driver

[Jishnu Prakash]

Hi Jonathan,

On 5/22/2026 4:17 PM, Jonathan Cameron wrote:
> On Thu, 21 May 2026 16:16:17 +0530
> Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx> wrote:
> 
>> Hi Jonathan,
>>
>> On 5/15/2026 7:24 PM, Jonathan Cameron wrote:
>>> On Fri, 15 May 2026 14:23:44 +0530
>>> Jishnu Prakash <jishnu.prakash@xxxxxxxxxxxxxxxx> wrote:
>>>   

...

>>>> diff --git a/drivers/iio/adc/qcom-spmi-adc5-gen3.c b/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>>>> index f8168a14b907..a819c3e627a0 100644
>>>> --- a/drivers/iio/adc/qcom-spmi-adc5-gen3.c
>>>> +++ b/drivers/iio/adc/qcom-spmi-adc5-gen3.c  
>>>   
>>>>  static int adc5_gen3_probe(struct platform_device *pdev)
>>>>  {
>>>>  	struct device *dev = &pdev->dev;
>>>> @@ -818,7 +782,7 @@ static int adc5_gen3_probe(struct platform_device *pdev)
>>>>  	}
>>>>  
>>>>  	ret = devm_request_irq(dev, adc->dev_data.base[ADC5_GEN3_VADC_SDAM].irq,
>>>> -			       adc5_gen3_isr, 0,
>>>> +			       adc5_gen3_isr, IRQF_ONESHOT | IRQF_SHARED,  
>>>
>>> Sashikio points out that IRQF_ONESHOT is never correct for a non threaded
>>> interrupt.  The point of that flag is to ensure we don't handle another interrupt
>>> until the thread is done. If there isn't a thread then it doesn't do anything
>>> (other than omit a warning!)  
>>
>> I tried at first keeping only the IRQF_SHARED flag here, but it seems that
>> shared interrupts need to agree on the ONESHOT flag configuration, else the
>> second interrupt's IRQ request call fails.
>>
>> And the ADC_TM interrupt needs to be ONESHOT, since we don't want that interrupt to
>> be rearmed before we have notified the thermal framework from the threaded
>> part of the handler. So I had to add the IRQF_ONESHOT here too, though it is
>> not useful here.
> That's an interesting corner case.  Maybe the warning needs to be more refined?
> (I don't think it checks for shared?)

Yes, at the point Sashiko indicated, it looks like there is no check for shared IRQ,
although it is checked later in the same function (__setup_irq).

> 
>>
>> I think it's best to use a threaded IRQ handler in this driver too. I don't really
>> see any meaningful way to split the actions in the interrupt handler here into a primary
>> handler and a threaded handler, so is it fine if I just make the primary handler NULL
>> and move all the ISR functionality into the threaded handler part ?
> 
> That's fine by me.  Just add some comments on why.

Thanks for confirming, I'll push again with a comment explaining this change.

Thanks,
Jishnu

> 
> J
>>
>> Thanks,
>> Jishnu
>>
>>
>>>   
>>>>  			       adc->dev_data.base[ADC5_GEN3_VADC_SDAM].irq_name,
>>>>  			       adc);
>>>>  	if (ret)
>>>> diff --git a/include/linux/iio/adc/qcom-adc5-gen3-common.h b/include/linux/iio/adc/qcom-adc5-gen3-common.h
>>>> index 6303eaa6640b..39cbfcbdb101 100644
>>>> --- a/include/linux/iio/adc/qcom-adc5-gen3-common.h
>>>> +++ b/include/linux/iio/adc/qcom-adc5-gen3-common.h
>>>> @@ -205,7 +205,5 @@ int adc5_gen3_get_scaled_reading(struct device *dev,
>>>>  int adc5_gen3_therm_code_to_temp(struct device *dev,
>>>>  				 struct adc5_channel_common_prop *common_props,
>>>>  				 u16 code, int *val);
>>>> -void adc5_gen3_register_tm_event_notifier(struct device *dev,
>>>> -					  void (*handler)(struct auxiliary_device *));
>>>>  
>>>>  #endif /* QCOM_ADC5_GEN3_COMMON_H */
>>>>  
>>>   
>>
>