The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
line to indicate the completion of a conversion. However, some devices
cannot properly detect the completion of a conversion by an interrupt.
This is for the reason mentioned in the following commit.
commit e9849777d0e2 ("genirq: Add flag to force mask in
disable_irq[_nosync]()")
A read operation is performed by an extra interrupt before the completion
of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
flag.
Signed-off-by: Masahiro Honda <[email protected]>
---
v3:
- Remove the Kconfig option.
v2: https://lore.kernel.org/linux-iio/[email protected]/
- Rework commit message.
- Add a new entry in the Kconfig.
- Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
v1: https://lore.kernel.org/linux-iio/[email protected]/
drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index d8570f620..215ecbedb 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig)
}
EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
+static void ad_sd_free_irq(void *sd)
+{
+ struct ad_sigma_delta *sigma_delta = sd;
+
+ irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
+ free_irq(sigma_delta->spi->irq, sigma_delta);
+}
+
static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
{
struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
@@ -584,11 +592,18 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
init_completion(&sigma_delta->completion);
sigma_delta->irq_dis = true;
- ret = devm_request_irq(dev, sigma_delta->spi->irq,
- ad_sd_data_rdy_trig_poll,
- sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
- indio_dev->name,
- sigma_delta);
+ irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
+ ret = request_irq(sigma_delta->spi->irq,
+ ad_sd_data_rdy_trig_poll,
+ sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
+ indio_dev->name,
+ sigma_delta);
+ if (ret) {
+ irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
+ return ret;
+ }
+
+ ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta);
if (ret)
return ret;
--
2.34.1
On Thu, 2023-04-20 at 19:23 +0900, Masahiro Honda wrote:
> The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> line to indicate the completion of a conversion. However, some devices
> cannot properly detect the completion of a conversion by an interrupt.
> This is for the reason mentioned in the following commit.
>
> commit e9849777d0e2 ("genirq: Add flag to force mask in
> disable_irq[_nosync]()")
>
> A read operation is performed by an extra interrupt before the completion
> of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> flag.
>
> Signed-off-by: Masahiro Honda <[email protected]>
> ---
LGTM:
Reviewed-by: Nuno Sá <[email protected]
On Thu, 20 Apr 2023 19:23:16 +0900
Masahiro Honda <[email protected]> wrote:
> The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> line to indicate the completion of a conversion. However, some devices
> cannot properly detect the completion of a conversion by an interrupt.
> This is for the reason mentioned in the following commit.
>
> commit e9849777d0e2 ("genirq: Add flag to force mask in
> disable_irq[_nosync]()")
>
> A read operation is performed by an extra interrupt before the completion
> of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> flag.
>
> Signed-off-by: Masahiro Honda <[email protected]>
> ---
> v3:
> - Remove the Kconfig option.
> v2: https://lore.kernel.org/linux-iio/[email protected]/
> - Rework commit message.
> - Add a new entry in the Kconfig.
> - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
> v1: https://lore.kernel.org/linux-iio/[email protected]/
>
> drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> index d8570f620..215ecbedb 100644
> --- a/drivers/iio/adc/ad_sigma_delta.c
> +++ b/drivers/iio/adc/ad_sigma_delta.c
> @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig)
> }
> EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
>
> +static void ad_sd_free_irq(void *sd)
> +{
> + struct ad_sigma_delta *sigma_delta = sd;
> +
> + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> + free_irq(sigma_delta->spi->irq, sigma_delta);
> +}
Don't fuse the two operations unwinding like this. Just register a callback that only
does the irq_clear_status_flags immediately after setting them. Then leave
the orginally devm_request_irq call alone. If it fails, the devm cleanup will
deal with the irq_clear_status_flag for you.
It almost never makes sense for a single devm call to unwind more than one function call in
a driver.
Otherwise this looks fine to me,
Thanks,
Jonathan
> +
> static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_dev)
> {
> struct ad_sigma_delta *sigma_delta = iio_device_get_drvdata(indio_dev);
> @@ -584,11 +592,18 @@ static int devm_ad_sd_probe_trigger(struct device *dev, struct iio_dev *indio_de
> init_completion(&sigma_delta->completion);
>
> sigma_delta->irq_dis = true;
> - ret = devm_request_irq(dev, sigma_delta->spi->irq,
> - ad_sd_data_rdy_trig_poll,
> - sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> - indio_dev->name,
> - sigma_delta);
> + irq_set_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> + ret = request_irq(sigma_delta->spi->irq,
> + ad_sd_data_rdy_trig_poll,
> + sigma_delta->info->irq_flags | IRQF_NO_AUTOEN,
> + indio_dev->name,
> + sigma_delta);
> + if (ret) {
> + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> + return ret;
> + }
> +
> + ret = devm_add_action_or_reset(dev, ad_sd_free_irq, sigma_delta);
> if (ret)
> return ret;
>
Hi Jonathan,
On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote:
> On Thu, 20 Apr 2023 19:23:16 +0900
> Masahiro Honda <[email protected]> wrote:
>
> > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> > line to indicate the completion of a conversion. However, some devices
> > cannot properly detect the completion of a conversion by an interrupt.
> > This is for the reason mentioned in the following commit.
> >
> > commit e9849777d0e2 ("genirq: Add flag to force mask in
> > disable_irq[_nosync]()")
> >
> > A read operation is performed by an extra interrupt before the completion
> > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> > flag.
> >
> > Signed-off-by: Masahiro Honda <[email protected]>
> > ---
> > v3:
> > - Remove the Kconfig option.
> > v2:
> > https://lore.kernel.org/linux-iio/[email protected]/
> > - Rework commit message.
> > - Add a new entry in the Kconfig.
> > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > IRQ.
> > v1:
> > https://lore.kernel.org/linux-iio/[email protected]/
> >
> > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > b/drivers/iio/adc/ad_sigma_delta.c
> > index d8570f620..215ecbedb 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev,
> > struct iio_trigger *trig)
> > }
> > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
> >
> > +static void ad_sd_free_irq(void *sd)
> > +{
> > + struct ad_sigma_delta *sigma_delta = sd;
> > +
> > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> > + free_irq(sigma_delta->spi->irq, sigma_delta);
> > +}
>
> Don't fuse the two operations unwinding like this. Just register a callback
> that only
> does the irq_clear_status_flags immediately after setting them. Then leave
I was the one to propose fusing them together because I thought that we could
have issues by clearing the flag after calling free_irq(). After looking again
at the IRQ code, I can see that it is not up to free_irq() to free the allocated
irq_descs (that might only happen when unmapping the virq) which means we should
be fine doing the normal way.
That said, looking at the only users that care to clear this flag, it looks like
they do it before calling free_irq(). Hence, I'm not sure if there's anything
subtle going on. In fact, looking at this line:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909
I'm not so sure we actually need to clear the flag as for these devices, we
should only have one consumer/action per IRQ. Anyways, probably for correctness
we should still explicitly clear it?
- Nuno Sá
Hi Jonathan,
On Sun, Apr 23, 2023 at 7:59 PM Jonathan Cameron <[email protected]> wrote:
>
> On Thu, 20 Apr 2023 19:23:16 +0900
> Masahiro Honda <[email protected]> wrote:
>
> > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> > line to indicate the completion of a conversion. However, some devices
> > cannot properly detect the completion of a conversion by an interrupt.
> > This is for the reason mentioned in the following commit.
> >
> > commit e9849777d0e2 ("genirq: Add flag to force mask in
> > disable_irq[_nosync]()")
> >
> > A read operation is performed by an extra interrupt before the completion
> > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> > flag.
> >
> > Signed-off-by: Masahiro Honda <[email protected]>
> > ---
> > v3:
> > - Remove the Kconfig option.
> > v2: https://lore.kernel.org/linux-iio/[email protected]/
> > - Rework commit message.
> > - Add a new entry in the Kconfig.
> > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the IRQ.
> > v1: https://lore.kernel.org/linux-iio/[email protected]/
> >
> > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> > 1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
> > index d8570f620..215ecbedb 100644
> > --- a/drivers/iio/adc/ad_sigma_delta.c
> > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev, struct iio_trigger *trig)
> > }
> > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
> >
> > +static void ad_sd_free_irq(void *sd)
> > +{
> > + struct ad_sigma_delta *sigma_delta = sd;
> > +
> > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> > + free_irq(sigma_delta->spi->irq, sigma_delta);
> > +}
>
> Don't fuse the two operations unwinding like this. Just register a callback that only
> does the irq_clear_status_flags immediately after setting them. Then leave
> the orginally devm_request_irq call alone. If it fails, the devm cleanup will
> deal with the irq_clear_status_flag for you.
>
> It almost never makes sense for a single devm call to unwind more than one function call in
> a driver.
>
> Otherwise this looks fine to me,
>
> Thanks,
>
> Jonathan
>
I understand. I'll fix it.
Thanks,
Hi Jonathan,
On Wed, Apr 26, 2023 at 9:02 PM Masahiro Honda <[email protected]> wrote:
>
> I understand. I'll fix it.
>
Sorry, I made a mistake.
I would like to wait for a conclusion.
Regards,
On Mon, 24 Apr 2023 11:09:34 +0200
Nuno Sá <[email protected]> wrote:
> Hi Jonathan,
>
> On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote:
> > On Thu, 20 Apr 2023 19:23:16 +0900
> > Masahiro Honda <[email protected]> wrote:
> >
> > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> > > line to indicate the completion of a conversion. However, some devices
> > > cannot properly detect the completion of a conversion by an interrupt.
> > > This is for the reason mentioned in the following commit.
> > >
> > > commit e9849777d0e2 ("genirq: Add flag to force mask in
> > > disable_irq[_nosync]()")
> > >
> > > A read operation is performed by an extra interrupt before the completion
> > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> > > flag.
> > >
> > > Signed-off-by: Masahiro Honda <[email protected]>
> > > ---
> > > v3:
> > > - Remove the Kconfig option.
> > > v2:
> > > https://lore.kernel.org/linux-iio/[email protected]/
> > > - Rework commit message.
> > > - Add a new entry in the Kconfig.
> > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > > IRQ.
> > > v1:
> > > https://lore.kernel.org/linux-iio/[email protected]/
> > >
> > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> > > 1 file changed, 20 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > b/drivers/iio/adc/ad_sigma_delta.c
> > > index d8570f620..215ecbedb 100644
> > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev,
> > > struct iio_trigger *trig)
> > > }
> > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
> > >
> > > +static void ad_sd_free_irq(void *sd)
> > > +{
> > > + struct ad_sigma_delta *sigma_delta = sd;
> > > +
> > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> > > + free_irq(sigma_delta->spi->irq, sigma_delta);
> > > +}
> >
> > Don't fuse the two operations unwinding like this. Just register a callback
> > that only
> > does the irq_clear_status_flags immediately after setting them. Then leave
>
> I was the one to propose fusing them together because I thought that we could
> have issues by clearing the flag after calling free_irq(). After looking again
> at the IRQ code, I can see that it is not up to free_irq() to free the allocated
> irq_descs (that might only happen when unmapping the virq) which means we should
> be fine doing the normal way.
Ah. I'd missed the ordering. If that had been valid (and I think you are correct
that it is not required) then a comment to make that clear would be necessary.
Usual case of: When doing something non obvious with ordering, say why.
>
> That said, looking at the only users that care to clear this flag, it looks like
> they do it before calling free_irq(). Hence, I'm not sure if there's anything
> subtle going on. In fact, looking at this line:
>
> https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909
>
> I'm not so sure we actually need to clear the flag as for these devices, we
> should only have one consumer/action per IRQ. Anyways, probably for correctness
> we should still explicitly clear it?
Good question... Looks to me like a driver shouldn't be clearing this flag
itself, but it's probably harmless in most cases.
I'd drop the clear of the status flag, perhaps adding a comment that
the irq core does it for us.
Jonathan
>
> - Nuno Sá
>
Hi all,
On Mon, May 1, 2023 at 2:27 AM Jonathan Cameron <[email protected]> wrote:
>
> On Mon, 24 Apr 2023 11:09:34 +0200
> Nuno Sá <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > On Sun, 2023-04-23 at 12:15 +0100, Jonathan Cameron wrote:
> > > On Thu, 20 Apr 2023 19:23:16 +0900
> > > Masahiro Honda <[email protected]> wrote:
> > >
> > > > The Sigma-Delta ADCs supported by this driver can use SDO as an interrupt
> > > > line to indicate the completion of a conversion. However, some devices
> > > > cannot properly detect the completion of a conversion by an interrupt.
> > > > This is for the reason mentioned in the following commit.
> > > >
> > > > commit e9849777d0e2 ("genirq: Add flag to force mask in
> > > > disable_irq[_nosync]()")
> > > >
> > > > A read operation is performed by an extra interrupt before the completion
> > > > of a conversion. This patch fixes the issue by setting IRQ_DISABLE_UNLAZY
> > > > flag.
> > > >
> > > > Signed-off-by: Masahiro Honda <[email protected]>
> > > > ---
> > > > v3:
> > > > - Remove the Kconfig option.
> > > > v2:
> > > > https://lore.kernel.org/linux-iio/[email protected]/
> > > > - Rework commit message.
> > > > - Add a new entry in the Kconfig.
> > > > - Call irq_clear_status_flags(irq, IRQ_DISABLE_UNLAZY) when freeing the
> > > > IRQ.
> > > > v1:
> > > > https://lore.kernel.org/linux-iio/[email protected]/
> > > >
> > > > drivers/iio/adc/ad_sigma_delta.c | 25 ++++++++++++++++++++-----
> > > > 1 file changed, 20 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/adc/ad_sigma_delta.c
> > > > b/drivers/iio/adc/ad_sigma_delta.c
> > > > index d8570f620..215ecbedb 100644
> > > > --- a/drivers/iio/adc/ad_sigma_delta.c
> > > > +++ b/drivers/iio/adc/ad_sigma_delta.c
> > > > @@ -565,6 +565,14 @@ int ad_sd_validate_trigger(struct iio_dev *indio_dev,
> > > > struct iio_trigger *trig)
> > > > }
> > > > EXPORT_SYMBOL_NS_GPL(ad_sd_validate_trigger, IIO_AD_SIGMA_DELTA);
> > > >
> > > > +static void ad_sd_free_irq(void *sd)
> > > > +{
> > > > + struct ad_sigma_delta *sigma_delta = sd;
> > > > +
> > > > + irq_clear_status_flags(sigma_delta->spi->irq, IRQ_DISABLE_UNLAZY);
> > > > + free_irq(sigma_delta->spi->irq, sigma_delta);
> > > > +}
> > >
> > > Don't fuse the two operations unwinding like this. Just register a callback
> > > that only
> > > does the irq_clear_status_flags immediately after setting them. Then leave
> >
> > I was the one to propose fusing them together because I thought that we could
> > have issues by clearing the flag after calling free_irq(). After looking again
> > at the IRQ code, I can see that it is not up to free_irq() to free the allocated
> > irq_descs (that might only happen when unmapping the virq) which means we should
> > be fine doing the normal way.
>
> Ah. I'd missed the ordering. If that had been valid (and I think you are correct
> that it is not required) then a comment to make that clear would be necessary.
>
> Usual case of: When doing something non obvious with ordering, say why.
>
> >
> > That said, looking at the only users that care to clear this flag, it looks like
> > they do it before calling free_irq(). Hence, I'm not sure if there's anything
> > subtle going on. In fact, looking at this line:
> >
> > https://elixir.bootlin.com/linux/latest/source/kernel/irq/manage.c#L1909
> >
> > I'm not so sure we actually need to clear the flag as for these devices, we
> > should only have one consumer/action per IRQ. Anyways, probably for correctness
> > we should still explicitly clear it?
>
> Good question... Looks to me like a driver shouldn't be clearing this flag
> itself, but it's probably harmless in most cases.
>
> I'd drop the clear of the status flag, perhaps adding a comment that
> the irq core does it for us.
>
> Jonathan
>
>
> >
> > - Nuno Sá
> >
>
I'll remove the callback and just call irq_set_status_flags() with a comment.
Thanks,
Masahiro