> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Mittwoch, 1. April 2020 12:27
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; devicetree
> <devicetree@xxxxxxxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> Hartmut Knaack <knaack.h@xxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>;
> Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Ardelean,
> Alexandru <alexandru.Ardelean@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>
> Subject: Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable
>
> On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@xxxxxxxxxx> wrote:
> > > From: linux-iio-owner@xxxxxxxxxxxxxxx <linux-iio-owner@xxxxxxxxxxxxxxx>
> On
> > > Behalf Of Andy Shevchenko
> > > Sent: Dienstag, 31. März 2020 19:40
> > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
> > > >
> > > > There are some ADIS devices that can configure the data ready pin
> > > > polarity. Hence, we cannot hardcode our IRQ mask as
> > > IRQF_TRIGGER_RISING
> > > > since we might want to have it as IRQF_TRIGGER_FALLING.
>
> ...
>
> > > > +static int adis_validate_irq_mask(struct adis *adis)
> > > > +{
> > > > + if (!adis->irq_mask) {
> > > > + adis->irq_mask = IRQF_TRIGGER_RISING;
> > > > + return 0;
> > >
> > > > + } else if (adis->irq_mask != IRQF_TRIGGER_RISING &&
> > >
> > > 'else' is redundant.
> > >
> > > > + adis->irq_mask != IRQF_TRIGGER_FALLING) {
> > >
> > > But this condition rises questions. Why i can't configure both?
> > > Why I can't configure other flags there?
> >
> > Both you can't because it is just how these type of devices work. Data is
> ready either
> > on the rising edge or on the falling edge (if supported by the device)...
> > I agree this could check if only one of the flags are set instead of directly
> comparing the
> > values (invalidating other flags) but I would prefer to keep this simple for
> now...
> >
> > >
> > > > + dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> > > > + adis->irq_mask);
> > > > + return -EINVAL;
> > > > + }
> > >
> > > > + return 0;
> > > > +}
> > >
> > > And actually name of the function is not exactly what it does. It
> > > validates *or* initializes.
> >
> > Well, yes. It just sets the mask to the default value to keep backward
> compatibility
> > with all the other devices that don't support/use this variable...
>
> Perhaps documentation in a comment form should be added.
> Moreover, I realized that you added to variable and function mask
> suffix while it's actually flag.
Will change the suffix to flag...
- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko
