Hi, Ok, I'll change the patch to keep the old behaviour as the default one but add a device tree settings for selecting the DIO and polarity as you said.
Vlad On Wed, Mar 9, 2016 at 11:41 AM, Lars-Peter Clausen <l...@metafoo.de> wrote: > Hi, > > The intention was to use DIO1 by default, which the driver does and which > has been tested. With your patch it uses DIO2 by default, which is the > correct setting for your board. > > - Lars > > On 03/09/2016 04:27 PM, Vlad Banea wrote: >> >> Hi, >> >> Since the driver is never setting the FNCTIO_CTRL register, I'd say the >> intention is to use the default settings. >> The clearing of all the other bits as we toggle the interrupt seems >> unintentional, and that's what the proposed patch addresses. The ADIS16485 >> device is not usable without this patch with the most up to date driver, and >> becomes fully functional when the patch is applied.. >> >> I wouldn't mind contributing to allow the driver to fully configure this >> register, but I'm not sure what user interface this should go through. >> >> Thanks >> Vlad >> >> >> >> On Wed, Mar 9, 2016 at 7:21 AM, Lars-Peter Clausen <l...@metafoo.de >> <mailto:l...@metafoo.de>> wrote: >> >> Hi, >> >> Hm, right. But the intention of the driver is to use DIO1. Changing this >> to >> DIO2 by default will break all existing users. >> >> This change should be part of a patch which allows to configure which >> interrupt output to use and also the interrupt polarity. >> >> - Lars >> >> On 03/09/2016 12:39 PM, Vlad Banea wrote: >> > Hi, >> > >> > Thanks for your answer. I'm using this driver with the ADIS16485 >> device and >> > the default value for this register is 0x000D: >> > >> >> http://www.analog.com/media/en/technical-documentation/data-sheets/ADIS16485.pdf >> > (Table 89) >> > >> > When the driver enables the interrupt, the Data Ready Line selection >> and >> > polarity are changed, and I never receive the interrupt. >> > >> > Vlad >> > >> > >> > On Wed, Mar 9, 2016 at 4:22 AM, Lars-Peter Clausen <l...@metafoo.de >> <mailto:l...@metafoo.de> >> > <mailto:l...@metafoo.de <mailto:l...@metafoo.de>>> wrote: >> > >> > On 03/09/2016 06:28 AM, Vlad Banea wrote: >> > > Enabling the IRQ should leave all other settings in the >> FNCTIO_CTRL >> > > register untouched: read the whole register, toggle just the >> enable bit, >> > > before writing it back. >> > >> > Hi, >> > >> > Thanks for the patch. Looks good in general, but it's not a fix. >> The driver >> > does not write this register anywhere else and the reset default >> value is >> > 0x00. So we don't corrupt any other settings since, 0x00 and >> BIT(3) are the >> > only two settings the driver does at the moment. >> > >> > If the patch is in preparation of future changes that are going to >> set/clear >> > other bits of the register this should be noted in the commit >> message. >> > >> > The reason I'm so pedantic here is because fix generally means >> that the >> > patch needs to be backported to older kernel versions, which is >> not the case >> > here. >> > >> > - Lars >> > >> > >> > > --- >> > > drivers/iio/imu/adis16480.c | 15 +++++++++++++-- >> > > 1 file changed, 13 insertions(+), 2 deletions(-) >> > > >> > > diff --git a/drivers/iio/imu/adis16480.c >> b/drivers/iio/imu/adis16480.c >> > > index b94bfd3..8473859 100644 >> > > --- a/drivers/iio/imu/adis16480.c >> > > +++ b/drivers/iio/imu/adis16480.c >> > > @@ -738,8 +738,19 @@ static int adis16480_stop_device(struct >> iio_dev >> > *indio_dev) >> > > >> > > static int adis16480_enable_irq(struct adis *adis, bool enable) >> > > { >> > > - return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, >> > > - enable ? BIT(3) : 0); >> > > + u16 fnctio_ctrl; >> > > + int ret; >> > > + >> > > + ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, >> > &fnctio_ctrl); >> > > + if (ret < 0) >> > > + return ret; >> > > + >> > > + if (enable) >> > > + fnctio_ctrl |= BIT(3); >> > > + else >> > > + fnctio_ctrl &= ~BIT(3); >> > > + >> > > + return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, >> > fnctio_ctrl); >> > > } >> > > >> > > static int adis16480_initial_setup(struct iio_dev *indio_dev) >> > > >> > >> > >> >> >
