lkml.org 
[lkml]   [2016]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ
From
Date
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)
>>> > >
>>> >
>>> >
>>>
>>>
>>

\
 
 \ /
  Last update: 2016-03-12 13:01    [W:0.599 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site