linux-kernel - Re: [PATCH] iio: adis16480: fix FNCTIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CACBkv7tGb8LfLRyCFdOWNJ1zhLmvWSOp9OF8iJxVXdA0fr5qZQ@mail.gmail.com>
Date:	Wed, 9 Mar 2016 12:06:47 -0500
From:	Vlad Banea <vlad@...ea.ca>
To:	Lars-Peter Clausen <lars@...afoo.de>
Cc:	Vlad Banea <vladb@...ux.com>, jic23@...nel.org,
	Michael.Hennerich@...log.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ

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 <lars@...afoo.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@...afoo.de
>> <mailto:lars@...afoo.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@...afoo.de
>>     <mailto:lars@...afoo.de>
>>     > <mailto:lars@...afoo.de <mailto:lars@...afoo.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)
>>     >     >
>>     >
>>     >
>>
>>
>