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.
--
With Best Regards,
Andy Shevchenko
