From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753279AbcCLLOj (ORCPT ); Sat, 12 Mar 2016 06:14:39 -0500 Received: from saturn.retrosnub.co.uk ([178.18.118.26]:43704 "EHLO saturn.retrosnub.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752354AbcCLLOb (ORCPT ); Sat, 12 Mar 2016 06:14:31 -0500 Subject: Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ To: Vlad Banea , Lars-Peter Clausen References: <1457501295-8258-1-git-send-email-vladb@linux.com> <56DFEB6D.90604@metafoo.de> <56E01556.2030107@metafoo.de> <56E05223.6090502@metafoo.de> Cc: Vlad Banea , Michael.Hennerich@analog.com, linux-kernel@vger.kernel.org From: Jonathan Cameron Message-ID: <56E3FA14.4030706@kernel.org> Date: Sat, 12 Mar 2016 11:14:28 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 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 >> > 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 >> >>> > >> 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) >>> > > >>> > >>> > >>> >>> >>