From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932346AbcCIMVw (ORCPT ); Wed, 9 Mar 2016 07:21:52 -0500 Received: from smtp-out-116.synserver.de ([212.40.185.116]:1079 "EHLO smtp-out-116.synserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750937AbcCIMVp (ORCPT ); Wed, 9 Mar 2016 07:21:45 -0500 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 13732 Subject: Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ To: Vlad Banea References: <1457501295-8258-1-git-send-email-vladb@linux.com> <56DFEB6D.90604@metafoo.de> Cc: Vlad Banea , jic23@kernel.org, Michael.Hennerich@analog.com, linux-kernel@vger.kernel.org From: Lars-Peter Clausen Message-ID: <56E01556.2030107@metafoo.de> Date: Wed, 9 Mar 2016 13:21:42 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/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 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) > > > >