> From: Jonathan Cameron <jic23@xxxxxxxxxx> > Sent: Monday, April 5, 2021 4:41 PM > To: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx> > Cc: linux-iio@xxxxxxxxxxxxxxx; Jonathan Cameron > <jonathan.cameron@xxxxxxxxxx>; Alexandru Ardelean > <ardeleanalex@xxxxxxxxx>; Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; > tiantao (H) <tiantao6@xxxxxxxxxxxxx> > Subject: Re: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead > of irq request then disable > > On Fri, 2 Apr 2021 20:37:37 +0000 > "Song Bao Hua (Barry Song)" <song.bao.hua@xxxxxxxxxxxxx> wrote: > > > > -----Original Message----- > > > From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] > > > Sent: Saturday, April 3, 2021 7:46 AM > > > To: linux-iio@xxxxxxxxxxxxxxx > > > Cc: Song Bao Hua (Barry Song) <song.bao.hua@xxxxxxxxxxxxx>; > Jonathan Cameron > > > <jonathan.cameron@xxxxxxxxxx>; Alexandru Ardelean > <ardeleanalex@xxxxxxxxx>; > > > Nuno Sa <Nuno.Sa@xxxxxxxxxx> > > > Subject: [PATCH 6/7] iio:imu:adis: Use IRQF_NO_AUTOEN instead > of irq request > > > then disable > > > > > > From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > > > > > This is a bit involved as the adis library code already has some > > > sanity checking of the flags of the requested irq that we need > > > to ensure is happy to pass through the IRQF_NO_AUTOEN flag > untouched. > > > > > > Using this flag avoids us autoenabling the irq in the adis16460 and > > > adis16475 drivers which cover parts that don't have any means of > > > masking the interrupt on the device end. > > > > > > Note, compile tested only! > > > > > > Signed-off-by: Jonathan Cameron > <Jonathan.Cameron@xxxxxxxxxx> > > > Cc: Alexandru Ardelean <ardeleanalex@xxxxxxxxx> > > > Cc: Nuno Sa <Nuno.Sa@xxxxxxxxxx> > > > --- > > > drivers/iio/imu/adis16460.c | 4 ++-- > > > drivers/iio/imu/adis16475.c | 5 +++-- > > > drivers/iio/imu/adis_trigger.c | 11 ++++++----- > > > 3 files changed, 11 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/iio/imu/adis16460.c > b/drivers/iio/imu/adis16460.c > > > index 74a161e39733..73bf45e859b8 100644 > > > --- a/drivers/iio/imu/adis16460.c > > > +++ b/drivers/iio/imu/adis16460.c > > > @@ -403,12 +403,12 @@ static int adis16460_probe(struct > spi_device *spi) > > > if (ret) > > > return ret; > > > > > > + /* We cannot mask the interrupt, so ensure it isn't auto > enabled */ > > > + st->adis.irq_flag |= IRQF_NO_AUTOEN; > > > ret = devm_adis_setup_buffer_and_trigger(&st->adis, > indio_dev, NULL); > > > if (ret) > > > return ret; > > > > > > - adis16460_enable_irq(&st->adis, 0); > > > - > > > ret = __adis_initial_startup(&st->adis); > > > if (ret) > > > return ret; > > > diff --git a/drivers/iio/imu/adis16475.c > b/drivers/iio/imu/adis16475.c > > > index 8f6bea4b6608..1de62fc79e0f 100644 > > > --- a/drivers/iio/imu/adis16475.c > > > +++ b/drivers/iio/imu/adis16475.c > > > @@ -1258,6 +1258,9 @@ static int adis16475_config_irq_pin(struct > adis16475 *st) > > > return -EINVAL; > > > } > > > > > > + /* We cannot mask the interrupt so ensure it's not enabled at > request */ > > > + st->adis.irq_flag |= IRQF_NO_AUTOEN; > > > + > > > val = ADIS16475_MSG_CTRL_DR_POL(polarity); > > > ret = __adis_update_bits(&st->adis, > ADIS16475_REG_MSG_CTRL, > > > > ADIS16475_MSG_CTRL_DR_POL_MASK, val); > > > @@ -1362,8 +1365,6 @@ static int adis16475_probe(struct > spi_device *spi) > > > if (ret) > > > return ret; > > > > > > - adis16475_enable_irq(&st->adis, false); > > > - > > > ret = devm_iio_device_register(&spi->dev, indio_dev); > > > if (ret) > > > return ret; > > > diff --git a/drivers/iio/imu/adis_trigger.c > b/drivers/iio/imu/adis_trigger.c > > > index 0f29e56200af..39b50d6fdb6a 100644 > > > --- a/drivers/iio/imu/adis_trigger.c > > > +++ b/drivers/iio/imu/adis_trigger.c > > > @@ -29,18 +29,19 @@ static const struct iio_trigger_ops > adis_trigger_ops = { > > > > > > static int adis_validate_irq_flag(struct adis *adis) > > > { > > > + unsigned long direction = adis->irq_flag & > IRQF_TRIGGER_MASK; > > > /* > > > * Typically this devices have data ready either on the rising > edge or > > > * on the falling edge of the data ready pin. This checks > enforces that > > > * one of those is set in the drivers... It defaults to > > > - * IRQF_TRIGGER_RISING for backward compatibility wiht > devices that > > > + * IRQF_TRIGGER_RISING for backward compatibility with > devices that > > > * don't support changing the pin polarity. > > > */ > > > - if (!adis->irq_flag) { > > > - adis->irq_flag = IRQF_TRIGGER_RISING; > > > + if (!direction) { > > > > Reviewed-by: Barry Song <song.bao.hua@xxxxxxxxxxxxx> > > > > Though a better way might be: > > > > if (direction == IRQF_TRIGGER_NONE) > Agreed. That makes it a tiny bit more obvious what is going on. > > Applied with that change (seems unlikely to affect other's tags) > to the togreg branch of iio.git and pushed out as testing for the > autobuilders to see if they can find anything we missed. > > As normal, whilst 0-day and friends are working their magic, there > is time for any additional feedback anyone would like to share. If not too late :)... Reviewed-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
