* [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ @ 2016-03-09 5:28 Vlad Banea 2016-03-09 9:22 ` Lars-Peter Clausen 0 siblings, 1 reply; 6+ messages in thread From: Vlad Banea @ 2016-03-09 5:28 UTC (permalink / raw) To: lars, jic23, Michael.Hennerich; +Cc: linux-kernel, Vlad Banea 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. --- 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) -- 2.7.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ 2016-03-09 5:28 [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ Vlad Banea @ 2016-03-09 9:22 ` Lars-Peter Clausen [not found] ` <CACBkv7uBy_eJv5aZRvaHAn6VY0rrjJkWF4mBXvv9_4wFJA9qiQ@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2016-03-09 9:22 UTC (permalink / raw) To: Vlad Banea, jic23, Michael.Hennerich; +Cc: linux-kernel 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) > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CACBkv7uBy_eJv5aZRvaHAn6VY0rrjJkWF4mBXvv9_4wFJA9qiQ@mail.gmail.com>]
* Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ [not found] ` <CACBkv7uBy_eJv5aZRvaHAn6VY0rrjJkWF4mBXvv9_4wFJA9qiQ@mail.gmail.com> @ 2016-03-09 12:21 ` Lars-Peter Clausen [not found] ` <CACBkv7v=D7frj6Uo7c9pd5aohVv_OUJU4jpoHdhP+S5KgmqHAw@mail.gmail.com> 0 siblings, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2016-03-09 12:21 UTC (permalink / raw) To: Vlad Banea; +Cc: Vlad Banea, jic23, Michael.Hennerich, linux-kernel 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>> 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) > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CACBkv7v=D7frj6Uo7c9pd5aohVv_OUJU4jpoHdhP+S5KgmqHAw@mail.gmail.com>]
* Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ [not found] ` <CACBkv7v=D7frj6Uo7c9pd5aohVv_OUJU4jpoHdhP+S5KgmqHAw@mail.gmail.com> @ 2016-03-09 16:41 ` Lars-Peter Clausen 2016-03-09 17:06 ` Vlad Banea 0 siblings, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2016-03-09 16:41 UTC (permalink / raw) To: Vlad Banea; +Cc: Vlad Banea, jic23, Michael.Hennerich, linux-kernel 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) > > > > > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ 2016-03-09 16:41 ` Lars-Peter Clausen @ 2016-03-09 17:06 ` Vlad Banea 2016-03-12 11:14 ` Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Vlad Banea @ 2016-03-09 17:06 UTC (permalink / raw) To: Lars-Peter Clausen; +Cc: Vlad Banea, jic23, Michael.Hennerich, linux-kernel 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@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) >> > > >> > >> > >> >> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ 2016-03-09 17:06 ` Vlad Banea @ 2016-03-12 11:14 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2016-03-12 11:14 UTC (permalink / raw) To: Vlad Banea, Lars-Peter Clausen Cc: Vlad Banea, Michael.Hennerich, linux-kernel 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) >>> > > >>> > >>> > >>> >>> >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-03-12 11:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-09 5:28 [PATCH] iio: adis16480: fix FNCTIO_CTRL corruption when enabling IRQ Vlad Banea
2016-03-09 9:22 ` Lars-Peter Clausen
[not found] ` <CACBkv7uBy_eJv5aZRvaHAn6VY0rrjJkWF4mBXvv9_4wFJA9qiQ@mail.gmail.com>
2016-03-09 12:21 ` Lars-Peter Clausen
[not found] ` <CACBkv7v=D7frj6Uo7c9pd5aohVv_OUJU4jpoHdhP+S5KgmqHAw@mail.gmail.com>
2016-03-09 16:41 ` Lars-Peter Clausen
2016-03-09 17:06 ` Vlad Banea
2016-03-12 11:14 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).