Messages in this thread | | | Subject | Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ | From | Jonathan Cameron <> | Date | Sat, 12 Mar 2016 11:14:28 +0000 |
| |
On 09/03/16 17:06, Vlad Banea wrote: > 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. Cool. As a process related aside, please make sure to cc the list linux-iio@vger.kernel.org
Jonathan > > Vlad > > On Wed, Mar 9, 2016 at 11:41 AM, Lars-Peter Clausen <lars@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 <lars@metafoo.de >>> <mailto:lars@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 <lars@metafoo.de >>> <mailto:lars@metafoo.de> >>> > <mailto:lars@metafoo.de <mailto:lars@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) >>> > > >>> > >>> > >>> >>> >>
| |