* [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper @ 2020-03-04 8:42 Alexandru Ardelean 2020-03-04 8:42 ` [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions Alexandru Ardelean 2020-04-13 17:05 ` [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Jonathan Cameron 0 siblings, 2 replies; 17+ messages in thread From: Alexandru Ardelean @ 2020-03-04 8:42 UTC (permalink / raw) To: linux-iio, linux-kernel Cc: jic23, eugen.hristev, ludovic.desroches, Alexandru Ardelean This change moves the logic to check if the current channel is the touchscreen channel to a separate helper. This reduces some code duplication, but the main intent is to re-use this in the next patches. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- This patchset continues discussion: https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ Apologies for the delay. Changelog v1 -> v2: * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper' * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() - at91_adc_buffer_postenable() - now just calls iio_triggered_buffer_postenable() if the channel isn't the touchscreen channel * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() - at91_adc_buffer_predisable() - now just calls iio_triggered_buffer_predisable() if the channel isn't the touchscreen channel drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c index a5c7771227d5..f2a74c47c768 100644 --- a/drivers/iio/adc/at91-sama5d2_adc.c +++ b/drivers/iio/adc/at91-sama5d2_adc.c @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) return 0; } +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) +{ + struct at91_adc_state *st = iio_priv(indio_dev); + + return !!bitmap_subset(indio_dev->active_scan_mask, + &st->touch_st.channels_bitmask, + AT91_SAMA5D2_MAX_CHAN_IDX + 1); +} + static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) { int ret; struct at91_adc_state *st = iio_priv(indio_dev); /* check if we are enabling triggered buffer or the touchscreen */ - if (bitmap_subset(indio_dev->active_scan_mask, - &st->touch_st.channels_bitmask, - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { - /* touchscreen enabling */ + if (at91_adc_current_chan_is_touch(indio_dev)) return at91_adc_configure_touch(st, true); - } + /* if we are not in triggered mode, we cannot enable the buffer. */ if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) return -EINVAL; @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) u8 bit; /* check if we are disabling triggered buffer or the touchscreen */ - if (bitmap_subset(indio_dev->active_scan_mask, - &st->touch_st.channels_bitmask, - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { - /* touchscreen disable */ + if (at91_adc_current_chan_is_touch(indio_dev)) return at91_adc_configure_touch(st, false); - } + /* if we are not in triggered mode, nothing to do here */ if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) return -EINVAL; @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) return 0; /* check if we are enabling triggered buffer or the touchscreen */ - if (bitmap_subset(indio_dev->active_scan_mask, - &st->touch_st.channels_bitmask, - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { - /* touchscreen enabling */ + if (at91_adc_current_chan_is_touch(indio_dev)) return at91_adc_configure_touch(st, true); - } else { + else return at91_adc_configure_trigger(st->trig, true); - } /* not needed but more explicit */ return 0; -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions 2020-03-04 8:42 [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Alexandru Ardelean @ 2020-03-04 8:42 ` Alexandru Ardelean 2020-04-19 10:04 ` ludovic.desroches 2020-04-13 17:05 ` [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Jonathan Cameron 1 sibling, 1 reply; 17+ messages in thread From: Alexandru Ardelean @ 2020-03-04 8:42 UTC (permalink / raw) To: linux-iio, linux-kernel Cc: jic23, eugen.hristev, ludovic.desroches, Alexandru Ardelean The iio_triggered_buffer_{predisable,postenable} functions attach/detach poll functions. In most cases the iio_triggered_buffer_postenable() should be called first to attach the poll function, and then the driver can init the data to be triggered. In this case it's the other way around: the DMA code should be initialized before the attaching the poll function and the reverse should be done when un-initializing. To make things easier when removing the iio_triggered_buffer_postenable() & iio_triggered_buffer_predisable() functions from the IIO core API, the DMA code has been moved into preenable() for init, and postdisable() for uninit. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/adc/at91-sama5d2_adc.c | 32 ++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c index f2a74c47c768..2e01073d401d 100644 --- a/drivers/iio/adc/at91-sama5d2_adc.c +++ b/drivers/iio/adc/at91-sama5d2_adc.c @@ -882,7 +882,7 @@ static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) AT91_SAMA5D2_MAX_CHAN_IDX + 1); } -static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev) { int ret; struct at91_adc_state *st = iio_priv(indio_dev); @@ -902,13 +902,20 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) return ret; } + return 0; +} + +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) +{ + if (at91_adc_current_chan_is_touch(indio_dev)) + return 0; + return iio_triggered_buffer_postenable(indio_dev); } -static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev) { struct at91_adc_state *st = iio_priv(indio_dev); - int ret; u8 bit; /* check if we are disabling triggered buffer or the touchscreen */ @@ -919,13 +926,8 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) return -EINVAL; - /* continue with the triggered buffer */ - ret = iio_triggered_buffer_predisable(indio_dev); - if (ret < 0) - dev_err(&indio_dev->dev, "buffer predisable failed\n"); - if (!st->dma_st.dma_chan) - return ret; + return 0; /* if we are using DMA we must clear registers and end DMA */ dmaengine_terminate_sync(st->dma_st.dma_chan); @@ -952,10 +954,20 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) /* read overflow register to clear possible overflow status */ at91_adc_readl(st, AT91_SAMA5D2_OVER); - return ret; + return 0; +} + +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) +{ + if (at91_adc_current_chan_is_touch(indio_dev)) + return 0; + + return iio_triggered_buffer_predisable(indio_dev); } static const struct iio_buffer_setup_ops at91_buffer_setup_ops = { + .preenable = &at91_adc_buffer_preenable, + .postdisable = &at91_adc_buffer_postdisable, .postenable = &at91_adc_buffer_postenable, .predisable = &at91_adc_buffer_predisable, }; -- 2.20.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions 2020-03-04 8:42 ` [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions Alexandru Ardelean @ 2020-04-19 10:04 ` ludovic.desroches 2020-04-23 10:56 ` Eugen.Hristev 0 siblings, 1 reply; 17+ messages in thread From: ludovic.desroches @ 2020-04-19 10:04 UTC (permalink / raw) To: Alexandru Ardelean; +Cc: linux-iio, linux-kernel, jic23, eugen.hristev On Wed, Mar 04, 2020 at 10:42:19AM +0200, Alexandru Ardelean wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > The iio_triggered_buffer_{predisable,postenable} functions attach/detach > poll functions. > > In most cases the iio_triggered_buffer_postenable() should be called first > to attach the poll function, and then the driver can init the data to be > triggered. > In this case it's the other way around: the DMA code should be initialized > before the attaching the poll function and the reverse should be done when > un-initializing. > > To make things easier when removing the iio_triggered_buffer_postenable() & > iio_triggered_buffer_predisable() functions from the IIO core API, the DMA > code has been moved into preenable() for init, and postdisable() for > uninit. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> > --- > drivers/iio/adc/at91-sama5d2_adc.c | 32 ++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > index f2a74c47c768..2e01073d401d 100644 > --- a/drivers/iio/adc/at91-sama5d2_adc.c > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > @@ -882,7 +882,7 @@ static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > AT91_SAMA5D2_MAX_CHAN_IDX + 1); > } > > -static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev) > { > int ret; > struct at91_adc_state *st = iio_priv(indio_dev); > @@ -902,13 +902,20 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > return ret; > } > > + return 0; > +} > + > +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > +{ > + if (at91_adc_current_chan_is_touch(indio_dev)) > + return 0; > + > return iio_triggered_buffer_postenable(indio_dev); > } > > -static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev) > { > struct at91_adc_state *st = iio_priv(indio_dev); > - int ret; > u8 bit; > > /* check if we are disabling triggered buffer or the touchscreen */ > @@ -919,13 +926,8 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > return -EINVAL; > > - /* continue with the triggered buffer */ > - ret = iio_triggered_buffer_predisable(indio_dev); > - if (ret < 0) > - dev_err(&indio_dev->dev, "buffer predisable failed\n"); > - > if (!st->dma_st.dma_chan) > - return ret; > + return 0; > > /* if we are using DMA we must clear registers and end DMA */ > dmaengine_terminate_sync(st->dma_st.dma_chan); > @@ -952,10 +954,20 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > > /* read overflow register to clear possible overflow status */ > at91_adc_readl(st, AT91_SAMA5D2_OVER); > - return ret; > + return 0; > +} > + > +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > +{ > + if (at91_adc_current_chan_is_touch(indio_dev)) > + return 0; > + > + return iio_triggered_buffer_predisable(indio_dev); > } > > static const struct iio_buffer_setup_ops at91_buffer_setup_ops = { > + .preenable = &at91_adc_buffer_preenable, > + .postdisable = &at91_adc_buffer_postdisable, > .postenable = &at91_adc_buffer_postenable, > .predisable = &at91_adc_buffer_predisable, > }; > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions 2020-04-19 10:04 ` ludovic.desroches @ 2020-04-23 10:56 ` Eugen.Hristev 0 siblings, 0 replies; 17+ messages in thread From: Eugen.Hristev @ 2020-04-23 10:56 UTC (permalink / raw) To: Ludovic.Desroches, alexandru.ardelean; +Cc: linux-iio, linux-kernel, jic23 On 19.04.2020 13:04, ludovic.desroches@microchip.com wrote: > On Wed, Mar 04, 2020 at 10:42:19AM +0200, Alexandru Ardelean wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> The iio_triggered_buffer_{predisable,postenable} functions attach/detach >> poll functions. >> >> In most cases the iio_triggered_buffer_postenable() should be called first >> to attach the poll function, and then the driver can init the data to be >> triggered. >> In this case it's the other way around: the DMA code should be initialized >> before the attaching the poll function and the reverse should be done when >> un-initializing. >> >> To make things easier when removing the iio_triggered_buffer_postenable() & >> iio_triggered_buffer_predisable() functions from the IIO core API, the DMA >> code has been moved into preenable() for init, and postdisable() for >> uninit. >> >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> Hi Jonathan, Please don't forget my other 2 patches on this driver, they've been patiently waiting for this merge window Thanks! > >> --- >> drivers/iio/adc/at91-sama5d2_adc.c | 32 ++++++++++++++++++++---------- >> 1 file changed, 22 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c >> index f2a74c47c768..2e01073d401d 100644 >> --- a/drivers/iio/adc/at91-sama5d2_adc.c >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c >> @@ -882,7 +882,7 @@ static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) >> AT91_SAMA5D2_MAX_CHAN_IDX + 1); >> } >> >> -static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) >> +static int at91_adc_buffer_preenable(struct iio_dev *indio_dev) >> { >> int ret; >> struct at91_adc_state *st = iio_priv(indio_dev); >> @@ -902,13 +902,20 @@ static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) >> return ret; >> } >> >> + return 0; >> +} >> + >> +static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) >> +{ >> + if (at91_adc_current_chan_is_touch(indio_dev)) >> + return 0; >> + >> return iio_triggered_buffer_postenable(indio_dev); >> } >> >> -static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) >> +static int at91_adc_buffer_postdisable(struct iio_dev *indio_dev) >> { >> struct at91_adc_state *st = iio_priv(indio_dev); >> - int ret; >> u8 bit; >> >> /* check if we are disabling triggered buffer or the touchscreen */ >> @@ -919,13 +926,8 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) >> return -EINVAL; >> >> - /* continue with the triggered buffer */ >> - ret = iio_triggered_buffer_predisable(indio_dev); >> - if (ret < 0) >> - dev_err(&indio_dev->dev, "buffer predisable failed\n"); >> - >> if (!st->dma_st.dma_chan) >> - return ret; >> + return 0; >> >> /* if we are using DMA we must clear registers and end DMA */ >> dmaengine_terminate_sync(st->dma_st.dma_chan); >> @@ -952,10 +954,20 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) >> >> /* read overflow register to clear possible overflow status */ >> at91_adc_readl(st, AT91_SAMA5D2_OVER); >> - return ret; >> + return 0; >> +} >> + >> +static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) >> +{ >> + if (at91_adc_current_chan_is_touch(indio_dev)) >> + return 0; >> + >> + return iio_triggered_buffer_predisable(indio_dev); >> } >> >> static const struct iio_buffer_setup_ops at91_buffer_setup_ops = { >> + .preenable = &at91_adc_buffer_preenable, >> + .postdisable = &at91_adc_buffer_postdisable, >> .postenable = &at91_adc_buffer_postenable, >> .predisable = &at91_adc_buffer_predisable, >> }; >> -- >> 2.20.1 >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-03-04 8:42 [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Alexandru Ardelean 2020-03-04 8:42 ` [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions Alexandru Ardelean @ 2020-04-13 17:05 ` Jonathan Cameron 2020-04-14 12:22 ` Eugen.Hristev 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2020-04-13 17:05 UTC (permalink / raw) To: Alexandru Ardelean Cc: linux-iio, linux-kernel, eugen.hristev, ludovic.desroches On Wed, 4 Mar 2020 10:42:18 +0200 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This change moves the logic to check if the current channel is the > touchscreen channel to a separate helper. > This reduces some code duplication, but the main intent is to re-use this > in the next patches. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Eugen / Ludovic, Have you had a chance to look at this series? Thanks, Jonathan > --- > > This patchset continues discussion: > https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > Apologies for the delay. > > Changelog v1 -> v2: > * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > helper' > * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > - at91_adc_buffer_postenable() - now just calls > iio_triggered_buffer_postenable() if the channel isn't the touchscreen > channel > * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > - at91_adc_buffer_predisable() - now just calls > iio_triggered_buffer_predisable() if the channel isn't the touchscreen > channel > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > index a5c7771227d5..f2a74c47c768 100644 > --- a/drivers/iio/adc/at91-sama5d2_adc.c > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > return 0; > } > > +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > +{ > + struct at91_adc_state *st = iio_priv(indio_dev); > + > + return !!bitmap_subset(indio_dev->active_scan_mask, > + &st->touch_st.channels_bitmask, > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > +} > + > static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > { > int ret; > struct at91_adc_state *st = iio_priv(indio_dev); > > /* check if we are enabling triggered buffer or the touchscreen */ > - if (bitmap_subset(indio_dev->active_scan_mask, > - &st->touch_st.channels_bitmask, > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > - /* touchscreen enabling */ > + if (at91_adc_current_chan_is_touch(indio_dev)) > return at91_adc_configure_touch(st, true); > - } > + > /* if we are not in triggered mode, we cannot enable the buffer. */ > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > return -EINVAL; > @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > u8 bit; > > /* check if we are disabling triggered buffer or the touchscreen */ > - if (bitmap_subset(indio_dev->active_scan_mask, > - &st->touch_st.channels_bitmask, > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > - /* touchscreen disable */ > + if (at91_adc_current_chan_is_touch(indio_dev)) > return at91_adc_configure_touch(st, false); > - } > + > /* if we are not in triggered mode, nothing to do here */ > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > return -EINVAL; > @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > return 0; > > /* check if we are enabling triggered buffer or the touchscreen */ > - if (bitmap_subset(indio_dev->active_scan_mask, > - &st->touch_st.channels_bitmask, > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > - /* touchscreen enabling */ > + if (at91_adc_current_chan_is_touch(indio_dev)) > return at91_adc_configure_touch(st, true); > - } else { > + else > return at91_adc_configure_trigger(st->trig, true); > - } > > /* not needed but more explicit */ > return 0; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-13 17:05 ` [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Jonathan Cameron @ 2020-04-14 12:22 ` Eugen.Hristev 2020-04-14 17:45 ` Jonathan Cameron 2020-04-15 6:43 ` ludovic.desroches 0 siblings, 2 replies; 17+ messages in thread From: Eugen.Hristev @ 2020-04-14 12:22 UTC (permalink / raw) To: jic23, alexandru.ardelean; +Cc: linux-iio, linux-kernel, Ludovic.Desroches On 13.04.2020 20:05, Jonathan Cameron wrote: > On Wed, 4 Mar 2020 10:42:18 +0200 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > >> This change moves the logic to check if the current channel is the >> touchscreen channel to a separate helper. >> This reduces some code duplication, but the main intent is to re-use this >> in the next patches. >> >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > Eugen / Ludovic, > > Have you had a chance to look at this series? Hi Jonathan, Does the patch apply correctly for you ? I will try to test it , if I manage to apply it. I can only test the ADC though because at this moment I do not have a touchscreen at disposal. Meanwhile, the code looks good for me, Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> By the way, I do not know if my two pending patches on this driver will conflict or not. Eugen > > Thanks, > > Jonathan > >> --- >> >> This patchset continues discussion: >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ >> Apologies for the delay. >> >> Changelog v1 -> v2: >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() >> helper' >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() >> - at91_adc_buffer_postenable() - now just calls >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen >> channel >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() >> - at91_adc_buffer_predisable() - now just calls >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen >> channel >> >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- >> 1 file changed, 15 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c >> index a5c7771227d5..f2a74c47c768 100644 >> --- a/drivers/iio/adc/at91-sama5d2_adc.c >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) >> return 0; >> } >> >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) >> +{ >> + struct at91_adc_state *st = iio_priv(indio_dev); >> + >> + return !!bitmap_subset(indio_dev->active_scan_mask, >> + &st->touch_st.channels_bitmask, >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); >> +} >> + >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) >> { >> int ret; >> struct at91_adc_state *st = iio_priv(indio_dev); >> >> /* check if we are enabling triggered buffer or the touchscreen */ >> - if (bitmap_subset(indio_dev->active_scan_mask, >> - &st->touch_st.channels_bitmask, >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >> - /* touchscreen enabling */ >> + if (at91_adc_current_chan_is_touch(indio_dev)) >> return at91_adc_configure_touch(st, true); >> - } >> + >> /* if we are not in triggered mode, we cannot enable the buffer. */ >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) >> return -EINVAL; >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) >> u8 bit; >> >> /* check if we are disabling triggered buffer or the touchscreen */ >> - if (bitmap_subset(indio_dev->active_scan_mask, >> - &st->touch_st.channels_bitmask, >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >> - /* touchscreen disable */ >> + if (at91_adc_current_chan_is_touch(indio_dev)) >> return at91_adc_configure_touch(st, false); >> - } >> + >> /* if we are not in triggered mode, nothing to do here */ >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) >> return -EINVAL; >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) >> return 0; >> >> /* check if we are enabling triggered buffer or the touchscreen */ >> - if (bitmap_subset(indio_dev->active_scan_mask, >> - &st->touch_st.channels_bitmask, >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >> - /* touchscreen enabling */ >> + if (at91_adc_current_chan_is_touch(indio_dev)) >> return at91_adc_configure_touch(st, true); >> - } else { >> + else >> return at91_adc_configure_trigger(st->trig, true); >> - } >> >> /* not needed but more explicit */ >> return 0; > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-14 12:22 ` Eugen.Hristev @ 2020-04-14 17:45 ` Jonathan Cameron 2020-04-15 6:33 ` Ardelean, Alexandru 2020-04-15 6:43 ` ludovic.desroches 1 sibling, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2020-04-14 17:45 UTC (permalink / raw) To: Eugen.Hristev Cc: alexandru.ardelean, linux-iio, linux-kernel, Ludovic.Desroches On Tue, 14 Apr 2020 12:22:45 +0000 <Eugen.Hristev@microchip.com> wrote: > On 13.04.2020 20:05, Jonathan Cameron wrote: > > On Wed, 4 Mar 2020 10:42:18 +0200 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > >> This change moves the logic to check if the current channel is the > >> touchscreen channel to a separate helper. > >> This reduces some code duplication, but the main intent is to re-use this > >> in the next patches. > >> > >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Eugen / Ludovic, > > > > Have you had a chance to look at this series? > > Hi Jonathan, > > Does the patch apply correctly for you ? I haven't tried yet :) > I will try to test it , if I manage to apply it. > I can only test the ADC though because at this moment I do not have a > touchscreen at disposal. > > Meanwhile, the code looks good for me, > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > By the way, I do not know if my two pending patches on this driver will > conflict or not. As this is a long term rework patch at heart, there isn't any particular rush as long as we don't loose it forever! Thanks, Jonathan > > Eugen > > > > > Thanks, > > > > Jonathan > > > >> --- > >> > >> This patchset continues discussion: > >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > >> Apologies for the delay. > >> > >> Changelog v1 -> v2: > >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > >> helper' > >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > >> - at91_adc_buffer_postenable() - now just calls > >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen > >> channel > >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > >> - at91_adc_buffer_predisable() - now just calls > >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen > >> channel > >> > >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > >> 1 file changed, 15 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > >> index a5c7771227d5..f2a74c47c768 100644 > >> --- a/drivers/iio/adc/at91-sama5d2_adc.c > >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c > >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > >> return 0; > >> } > >> > >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > >> +{ > >> + struct at91_adc_state *st = iio_priv(indio_dev); > >> + > >> + return !!bitmap_subset(indio_dev->active_scan_mask, > >> + &st->touch_st.channels_bitmask, > >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > >> +} > >> + > >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > >> { > >> int ret; > >> struct at91_adc_state *st = iio_priv(indio_dev); > >> > >> /* check if we are enabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen enabling */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, true); > >> - } > >> + > >> /* if we are not in triggered mode, we cannot enable the buffer. */ > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > >> return -EINVAL; > >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > >> u8 bit; > >> > >> /* check if we are disabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen disable */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, false); > >> - } > >> + > >> /* if we are not in triggered mode, nothing to do here */ > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > >> return -EINVAL; > >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > >> return 0; > >> > >> /* check if we are enabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen enabling */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, true); > >> - } else { > >> + else > >> return at91_adc_configure_trigger(st->trig, true); > >> - } > >> > >> /* not needed but more explicit */ > >> return 0; > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-14 17:45 ` Jonathan Cameron @ 2020-04-15 6:33 ` Ardelean, Alexandru 2020-04-27 12:20 ` Eugen.Hristev 0 siblings, 1 reply; 17+ messages in thread From: Ardelean, Alexandru @ 2020-04-15 6:33 UTC (permalink / raw) To: Eugen.Hristev, jic23; +Cc: Ludovic.Desroches, linux-kernel, linux-iio On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > On Tue, 14 Apr 2020 12:22:45 +0000 > <Eugen.Hristev@microchip.com> wrote: > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > This change moves the logic to check if the current channel is the > > > > touchscreen channel to a separate helper. > > > > This reduces some code duplication, but the main intent is to re-use > > > > this > > > > in the next patches. > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > Eugen / Ludovic, > > > > > > Have you had a chance to look at this series? > > > > Hi Jonathan, > > > > Does the patch apply correctly for you ? > > I haven't tried yet :) > I've rebased this patchset on top of current iio/testing and it still applies. > > I will try to test it , if I manage to apply it. > > I can only test the ADC though because at this moment I do not have a > > touchscreen at disposal. > > > > Meanwhile, the code looks good for me, > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > By the way, I do not know if my two pending patches on this driver will > > conflict or not. > > As this is a long term rework patch at heart, there isn't any particular > rush as long as we don't loose it forever! > > Thanks, > > Jonathan > > > Eugen > > > > > Thanks, > > > > > > Jonathan > > > > > > > --- > > > > > > > > This patchset continues discussion: > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ > > > > > > > > Apologies for the delay. > > > > > > > > Changelog v1 -> v2: > > > > * added patch 'iio: at91-sama5d2_adc: split > > > > at91_adc_current_chan_is_touch() > > > > helper' > > > > * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > > > > - at91_adc_buffer_postenable() - now just calls > > > > iio_triggered_buffer_postenable() if the channel isn't the > > > > touchscreen > > > > channel > > > > * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > > > > - at91_adc_buffer_predisable() - now just calls > > > > iio_triggered_buffer_predisable() if the channel isn't the > > > > touchscreen > > > > channel > > > > > > > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91- > > > > sama5d2_adc.c > > > > index a5c7771227d5..f2a74c47c768 100644 > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev > > > > *indio_dev) > > > > return 0; > > > > } > > > > > > > > +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > > > > +{ > > > > + struct at91_adc_state *st = iio_priv(indio_dev); > > > > + > > > > + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > + &st->touch_st.channels_bitmask, > > > > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > +} > > > > + > > > > static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > > > > { > > > > int ret; > > > > struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > /* check if we are enabling triggered buffer or the touchscreen > > > > */ > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > - &st->touch_st.channels_bitmask, > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > - /* touchscreen enabling */ > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > return at91_adc_configure_touch(st, true); > > > > - } > > > > + > > > > /* if we are not in triggered mode, we cannot enable the buffer. > > > > */ > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > return -EINVAL; > > > > @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct > > > > iio_dev *indio_dev) > > > > u8 bit; > > > > > > > > /* check if we are disabling triggered buffer or the touchscreen > > > > */ > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > - &st->touch_st.channels_bitmask, > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > - /* touchscreen disable */ > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > return at91_adc_configure_touch(st, false); > > > > - } > > > > + > > > > /* if we are not in triggered mode, nothing to do here */ > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > return -EINVAL; > > > > @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct > > > > device *dev) > > > > return 0; > > > > > > > > /* check if we are enabling triggered buffer or the touchscreen > > > > */ > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > - &st->touch_st.channels_bitmask, > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > - /* touchscreen enabling */ > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > return at91_adc_configure_touch(st, true); > > > > - } else { > > > > + else > > > > return at91_adc_configure_trigger(st->trig, true); > > > > - } > > > > > > > > /* not needed but more explicit */ > > > > return 0; > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-15 6:33 ` Ardelean, Alexandru @ 2020-04-27 12:20 ` Eugen.Hristev 2020-04-27 13:00 ` Ardelean, Alexandru 0 siblings, 1 reply; 17+ messages in thread From: Eugen.Hristev @ 2020-04-27 12:20 UTC (permalink / raw) To: alexandru.Ardelean, jic23; +Cc: Ludovic.Desroches, linux-kernel, linux-iio On 15.04.2020 09:33, Ardelean, Alexandru wrote: > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: >> On Tue, 14 Apr 2020 12:22:45 +0000 >> <Eugen.Hristev@microchip.com> wrote: >> >>> On 13.04.2020 20:05, Jonathan Cameron wrote: >>>> On Wed, 4 Mar 2020 10:42:18 +0200 >>>> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: >>>> >>>>> This change moves the logic to check if the current channel is the >>>>> touchscreen channel to a separate helper. >>>>> This reduces some code duplication, but the main intent is to re-use >>>>> this >>>>> in the next patches. >>>>> >>>>> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> >>>> Eugen / Ludovic, >>>> >>>> Have you had a chance to look at this series? >>> >>> Hi Jonathan, >>> >>> Does the patch apply correctly for you ? >> >> I haven't tried yet :) >> > > I've rebased this patchset on top of current iio/testing and it still applies. > Hi Alex, I tried this patch on top of my tree (however I am testing with an older kernel 5.4) , and I have issues starting the buffer after you moved my code to the preenable callback. Namely, on the line: if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) return -EINVAL; And with this , the preenable fails on my side, because the current mode is not yet switched to triggered. I do remember adding this line with a specific reason. It may be related to touchscreen operations, but I have to retest the touch with and without this line and your patch. Meanwhile, maybe you have any suggestions on how to fix the buffer ? This check here makes any sense to you ? Thanks, Eugen > >>> I will try to test it , if I manage to apply it. >>> I can only test the ADC though because at this moment I do not have a >>> touchscreen at disposal. >>> >>> Meanwhile, the code looks good for me, >>> >>> Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> >>> >>> By the way, I do not know if my two pending patches on this driver will >>> conflict or not. >> >> As this is a long term rework patch at heart, there isn't any particular >> rush as long as we don't loose it forever! >> >> Thanks, >> >> Jonathan >> >>> Eugen >>> >>>> Thanks, >>>> >>>> Jonathan >>>> >>>>> --- >>>>> >>>>> This patchset continues discussion: >>>>> >>>>> https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ >>>>> >>>>> Apologies for the delay. >>>>> >>>>> Changelog v1 -> v2: >>>>> * added patch 'iio: at91-sama5d2_adc: split >>>>> at91_adc_current_chan_is_touch() >>>>> helper' >>>>> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() >>>>> - at91_adc_buffer_postenable() - now just calls >>>>> iio_triggered_buffer_postenable() if the channel isn't the >>>>> touchscreen >>>>> channel >>>>> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() >>>>> - at91_adc_buffer_predisable() - now just calls >>>>> iio_triggered_buffer_predisable() if the channel isn't the >>>>> touchscreen >>>>> channel >>>>> >>>>> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- >>>>> 1 file changed, 15 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91- >>>>> sama5d2_adc.c >>>>> index a5c7771227d5..f2a74c47c768 100644 >>>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c >>>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c >>>>> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev >>>>> *indio_dev) >>>>> return 0; >>>>> } >>>>> >>>>> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) >>>>> +{ >>>>> + struct at91_adc_state *st = iio_priv(indio_dev); >>>>> + >>>>> + return !!bitmap_subset(indio_dev->active_scan_mask, >>>>> + &st->touch_st.channels_bitmask, >>>>> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); >>>>> +} >>>>> + >>>>> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) >>>>> { >>>>> int ret; >>>>> struct at91_adc_state *st = iio_priv(indio_dev); >>>>> >>>>> /* check if we are enabling triggered buffer or the touchscreen >>>>> */ >>>>> - if (bitmap_subset(indio_dev->active_scan_mask, >>>>> - &st->touch_st.channels_bitmask, >>>>> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >>>>> - /* touchscreen enabling */ >>>>> + if (at91_adc_current_chan_is_touch(indio_dev)) >>>>> return at91_adc_configure_touch(st, true); >>>>> - } >>>>> + >>>>> /* if we are not in triggered mode, we cannot enable the buffer. >>>>> */ >>>>> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) >>>>> return -EINVAL; >>>>> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct >>>>> iio_dev *indio_dev) >>>>> u8 bit; >>>>> >>>>> /* check if we are disabling triggered buffer or the touchscreen >>>>> */ >>>>> - if (bitmap_subset(indio_dev->active_scan_mask, >>>>> - &st->touch_st.channels_bitmask, >>>>> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >>>>> - /* touchscreen disable */ >>>>> + if (at91_adc_current_chan_is_touch(indio_dev)) >>>>> return at91_adc_configure_touch(st, false); >>>>> - } >>>>> + >>>>> /* if we are not in triggered mode, nothing to do here */ >>>>> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) >>>>> return -EINVAL; >>>>> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct >>>>> device *dev) >>>>> return 0; >>>>> >>>>> /* check if we are enabling triggered buffer or the touchscreen >>>>> */ >>>>> - if (bitmap_subset(indio_dev->active_scan_mask, >>>>> - &st->touch_st.channels_bitmask, >>>>> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { >>>>> - /* touchscreen enabling */ >>>>> + if (at91_adc_current_chan_is_touch(indio_dev)) >>>>> return at91_adc_configure_touch(st, true); >>>>> - } else { >>>>> + else >>>>> return at91_adc_configure_trigger(st->trig, true); >>>>> - } >>>>> >>>>> /* not needed but more explicit */ >>>>> return 0; >>>> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-27 12:20 ` Eugen.Hristev @ 2020-04-27 13:00 ` Ardelean, Alexandru 2020-04-30 7:30 ` Ardelean, Alexandru 0 siblings, 1 reply; 17+ messages in thread From: Ardelean, Alexandru @ 2020-04-27 13:00 UTC (permalink / raw) To: Eugen.Hristev, jic23; +Cc: Ludovic.Desroches, linux-kernel, linux-iio On Mon, 2020-04-27 at 12:20 +0000, Eugen.Hristev@microchip.com wrote: > [External] > > On 15.04.2020 09:33, Ardelean, Alexandru wrote: > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > > > On Tue, 14 Apr 2020 12:22:45 +0000 > > > <Eugen.Hristev@microchip.com> wrote: > > > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > > > > > This change moves the logic to check if the current channel is the > > > > > > touchscreen channel to a separate helper. > > > > > > This reduces some code duplication, but the main intent is to re-use > > > > > > this > > > > > > in the next patches. > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > Eugen / Ludovic, > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > Hi Jonathan, > > > > > > > > Does the patch apply correctly for you ? > > > > > > I haven't tried yet :) > > > > > > > I've rebased this patchset on top of current iio/testing and it still > > applies. > > > > Hi Alex, > > I tried this patch on top of my tree (however I am testing with an older > kernel 5.4) , and I have issues starting the buffer after you moved my > code to the preenable callback. > > Namely, on the line: > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > return -EINVAL; Apologies for the breakage. For the touch-part I don't see that code being executed. But a question is: does the driver need to check for the currentmode? Or is that something that the IIO core should do? > > And with this , the preenable fails on my side, because the current mode > is not yet switched to triggered. > > I do remember adding this line with a specific reason. It may be related > to touchscreen operations, but I have to retest the touch with and > without this line and your patch. > > Meanwhile, maybe you have any suggestions on how to fix the buffer ? Well, there was the question of whether iio_triggered_buffer_postenable() [to attach the pollfunc] makes sense to be called first/last in the old at91_adc_buffer_postenable(), and the answer was 'last'; so then one solution was to move things to preenable(). Going back to the old patch isn't ideal, as the idea was to make the position of iio_triggered_buffer_postenable() consistent across all drivers, so that it can be removed [and moved to the IIO core]. But if we need revert the patch, then I guess it's fine. The only solution I see right now [for going forward], is to remove that check for 'currrentmode' > This check here makes any sense to you ? I think Jonathan may have to add some input here, but I think that in this current situation, checking 'currentmode' looks like is re-validating how the device was configured via the IIO framework. I am not sure if it's needed or not. > > Thanks, > Eugen > > > > > I will try to test it , if I manage to apply it. > > > > I can only test the ADC though because at this moment I do not have a > > > > touchscreen at disposal. > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > > > By the way, I do not know if my two pending patches on this driver will > > > > conflict or not. > > > > > > As this is a long term rework patch at heart, there isn't any particular > > > rush as long as we don't loose it forever! > > > > > > Thanks, > > > > > > Jonathan > > > > > > > Eugen > > > > > > > > > Thanks, > > > > > > > > > > Jonathan > > > > > > > > > > > --- > > > > > > > > > > > > This patchset continues discussion: > > > > > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ > > > > > > > > > > > > Apologies for the delay. > > > > > > > > > > > > Changelog v1 -> v2: > > > > > > * added patch 'iio: at91-sama5d2_adc: split > > > > > > at91_adc_current_chan_is_touch() > > > > > > helper' > > > > > > * renamed at91_adc_buffer_postenable() -> > > > > > > at91_adc_buffer_preenable() > > > > > > - at91_adc_buffer_postenable() - now just calls > > > > > > iio_triggered_buffer_postenable() if the channel isn't the > > > > > > touchscreen > > > > > > channel > > > > > > * renamed at91_adc_buffer_predisable() -> > > > > > > at91_adc_buffer_postdisable() > > > > > > - at91_adc_buffer_predisable() - now just calls > > > > > > iio_triggered_buffer_predisable() if the channel isn't the > > > > > > touchscreen > > > > > > channel > > > > > > > > > > > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++-------- > > > > > > ------- > > > > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > b/drivers/iio/adc/at91- > > > > > > sama5d2_adc.c > > > > > > index a5c7771227d5..f2a74c47c768 100644 > > > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev > > > > > > *indio_dev) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > +static bool at91_adc_current_chan_is_touch(struct iio_dev > > > > > > *indio_dev) > > > > > > +{ > > > > > > + struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > + > > > > > > + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > > > + &st->touch_st.channels_bitmask, > > > > > > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > > > +} > > > > > > + > > > > > > static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > > > > > > { > > > > > > int ret; > > > > > > struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > touchscreen > > > > > > */ > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > - &st->touch_st.channels_bitmask, > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > - /* touchscreen enabling */ > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > return at91_adc_configure_touch(st, true); > > > > > > - } > > > > > > + > > > > > > /* if we are not in triggered mode, we cannot enable the > > > > > > buffer. > > > > > > */ > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > > > return -EINVAL; > > > > > > @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct > > > > > > iio_dev *indio_dev) > > > > > > u8 bit; > > > > > > > > > > > > /* check if we are disabling triggered buffer or the > > > > > > touchscreen > > > > > > */ > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > - &st->touch_st.channels_bitmask, > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > - /* touchscreen disable */ > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > return at91_adc_configure_touch(st, false); > > > > > > - } > > > > > > + > > > > > > /* if we are not in triggered mode, nothing to do here */ > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > > > return -EINVAL; > > > > > > @@ -1886,14 +1889,10 @@ static __maybe_unused int > > > > > > at91_adc_resume(struct > > > > > > device *dev) > > > > > > return 0; > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > touchscreen > > > > > > */ > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > - &st->touch_st.channels_bitmask, > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > - /* touchscreen enabling */ > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > return at91_adc_configure_touch(st, true); > > > > > > - } else { > > > > > > + else > > > > > > return at91_adc_configure_trigger(st->trig, true); > > > > > > - } > > > > > > > > > > > > /* not needed but more explicit */ > > > > > > return 0; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-27 13:00 ` Ardelean, Alexandru @ 2020-04-30 7:30 ` Ardelean, Alexandru 2020-04-30 8:10 ` Ardelean, Alexandru 0 siblings, 1 reply; 17+ messages in thread From: Ardelean, Alexandru @ 2020-04-30 7:30 UTC (permalink / raw) To: Eugen.Hristev, jic23; +Cc: Ludovic.Desroches, linux-kernel, linux-iio On Mon, 2020-04-27 at 13:00 +0000, Ardelean, Alexandru wrote: > [External] > > On Mon, 2020-04-27 at 12:20 +0000, Eugen.Hristev@microchip.com wrote: > > [External] > > > > On 15.04.2020 09:33, Ardelean, Alexandru wrote: > > > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > > > > On Tue, 14 Apr 2020 12:22:45 +0000 > > > > <Eugen.Hristev@microchip.com> wrote: > > > > > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > > > > > > > This change moves the logic to check if the current channel is the > > > > > > > touchscreen channel to a separate helper. > > > > > > > This reduces some code duplication, but the main intent is to re- > > > > > > > use > > > > > > > this > > > > > > > in the next patches. > > > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > > Eugen / Ludovic, > > > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > > > Hi Jonathan, > > > > > > > > > > Does the patch apply correctly for you ? > > > > > > > > I haven't tried yet :) > > > > > > > > > > I've rebased this patchset on top of current iio/testing and it still > > > applies. > > > > > > > Hi Alex, > > > > I tried this patch on top of my tree (however I am testing with an older > > kernel 5.4) , and I have issues starting the buffer after you moved my > > code to the preenable callback. > > > > Namely, on the line: > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > return -EINVAL; > In the meantime I found this patch: https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-lars@metafoo.de/ from about ~5 years ago; if this patch is a valid proposal, it could fix this case as well; well, it might break others, so applying it [now] would need some general review of all pre/post enable/disable hooks > Apologies for the breakage. > > For the touch-part I don't see that code being executed. > > But a question is: does the driver need to check for the currentmode? > Or is that something that the IIO core should do? > > > And with this , the preenable fails on my side, because the current mode > > is not yet switched to triggered. > > > > I do remember adding this line with a specific reason. It may be related > > to touchscreen operations, but I have to retest the touch with and > > without this line and your patch. > > > > Meanwhile, maybe you have any suggestions on how to fix the buffer ? > > Well, there was the question of whether iio_triggered_buffer_postenable() [to > attach the pollfunc] makes sense to be called first/last in the old > at91_adc_buffer_postenable(), and the answer was 'last'; so then one solution > was to move things to preenable(). > > Going back to the old patch isn't ideal, as the idea was to make the position > of > iio_triggered_buffer_postenable() consistent across all drivers, so that it > can > be removed [and moved to the IIO core]. > > But if we need revert the patch, then I guess it's fine. > The only solution I see right now [for going forward], is to remove that check > for 'currrentmode' > > > This check here makes any sense to you ? > > I think Jonathan may have to add some input here, but I think that in this > current situation, checking 'currentmode' looks like is re-validating how the > device was configured via the IIO framework. > I am not sure if it's needed or not. > > > Thanks, > > Eugen > > > > > > > I will try to test it , if I manage to apply it. > > > > > I can only test the ADC though because at this moment I do not have a > > > > > touchscreen at disposal. > > > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > > > > > By the way, I do not know if my two pending patches on this driver > > > > > will > > > > > conflict or not. > > > > > > > > As this is a long term rework patch at heart, there isn't any particular > > > > rush as long as we don't loose it forever! > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > > > Eugen > > > > > > > > > > > Thanks, > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > --- > > > > > > > > > > > > > > This patchset continues discussion: > > > > > > > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ > > > > > > > > > > > > > > Apologies for the delay. > > > > > > > > > > > > > > Changelog v1 -> v2: > > > > > > > * added patch 'iio: at91-sama5d2_adc: split > > > > > > > at91_adc_current_chan_is_touch() > > > > > > > helper' > > > > > > > * renamed at91_adc_buffer_postenable() -> > > > > > > > at91_adc_buffer_preenable() > > > > > > > - at91_adc_buffer_postenable() - now just calls > > > > > > > iio_triggered_buffer_postenable() if the channel isn't the > > > > > > > touchscreen > > > > > > > channel > > > > > > > * renamed at91_adc_buffer_predisable() -> > > > > > > > at91_adc_buffer_postdisable() > > > > > > > - at91_adc_buffer_predisable() - now just calls > > > > > > > iio_triggered_buffer_predisable() if the channel isn't the > > > > > > > touchscreen > > > > > > > channel > > > > > > > > > > > > > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++-------- > > > > > > > ------- > > > > > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > b/drivers/iio/adc/at91- > > > > > > > sama5d2_adc.c > > > > > > > index a5c7771227d5..f2a74c47c768 100644 > > > > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev > > > > > > > *indio_dev) > > > > > > > return 0; > > > > > > > } > > > > > > > > > > > > > > +static bool at91_adc_current_chan_is_touch(struct iio_dev > > > > > > > *indio_dev) > > > > > > > +{ > > > > > > > + struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > + > > > > > > > + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > > > > + &st->touch_st.channels_bitmask, > > > > > > > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > > > > +} > > > > > > > + > > > > > > > static int at91_adc_buffer_postenable(struct iio_dev > > > > > > > *indio_dev) > > > > > > > { > > > > > > > int ret; > > > > > > > struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > touchscreen > > > > > > > */ > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > - /* touchscreen enabling */ > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > - } > > > > > > > + > > > > > > > /* if we are not in triggered mode, we cannot enable the > > > > > > > buffer. > > > > > > > */ > > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > > > > return -EINVAL; > > > > > > > @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct > > > > > > > iio_dev *indio_dev) > > > > > > > u8 bit; > > > > > > > > > > > > > > /* check if we are disabling triggered buffer or the > > > > > > > touchscreen > > > > > > > */ > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > - /* touchscreen disable */ > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > return at91_adc_configure_touch(st, false); > > > > > > > - } > > > > > > > + > > > > > > > /* if we are not in triggered mode, nothing to do here */ > > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > > > > return -EINVAL; > > > > > > > @@ -1886,14 +1889,10 @@ static __maybe_unused int > > > > > > > at91_adc_resume(struct > > > > > > > device *dev) > > > > > > > return 0; > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > touchscreen > > > > > > > */ > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > - /* touchscreen enabling */ > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > - } else { > > > > > > > + else > > > > > > > return at91_adc_configure_trigger(st->trig, true); > > > > > > > - } > > > > > > > > > > > > > > /* not needed but more explicit */ > > > > > > > return 0; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-30 7:30 ` Ardelean, Alexandru @ 2020-04-30 8:10 ` Ardelean, Alexandru 2020-05-02 17:31 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: Ardelean, Alexandru @ 2020-04-30 8:10 UTC (permalink / raw) To: Eugen.Hristev, jic23; +Cc: Ludovic.Desroches, linux-kernel, linux-iio On Thu, 2020-04-30 at 07:30 +0000, Ardelean, Alexandru wrote: > On Mon, 2020-04-27 at 13:00 +0000, Ardelean, Alexandru wrote: > > [External] > > > > On Mon, 2020-04-27 at 12:20 +0000, Eugen.Hristev@microchip.com wrote: > > > [External] > > > > > > On 15.04.2020 09:33, Ardelean, Alexandru wrote: > > > > > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > > > > > On Tue, 14 Apr 2020 12:22:45 +0000 > > > > > <Eugen.Hristev@microchip.com> wrote: > > > > > > > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > > > > > > > > > This change moves the logic to check if the current channel is > > > > > > > > the > > > > > > > > touchscreen channel to a separate helper. > > > > > > > > This reduces some code duplication, but the main intent is to > > > > > > > > re- > > > > > > > > use > > > > > > > > this > > > > > > > > in the next patches. > > > > > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com > > > > > > > > > > > > > > > > Eugen / Ludovic, > > > > > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > Does the patch apply correctly for you ? > > > > > > > > > > I haven't tried yet :) > > > > > > > > > > > > > I've rebased this patchset on top of current iio/testing and it still > > > > applies. > > > > > > > > > > Hi Alex, > > > > > > I tried this patch on top of my tree (however I am testing with an older > > > kernel 5.4) , and I have issues starting the buffer after you moved my > > > code to the preenable callback. > > > > > > Namely, on the line: > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > return -EINVAL; > > In the meantime I found this patch: > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-lars@metafoo.de/__;!!A3Ni8CS0y2Y!ocQuNvFF_8rd-cCvMNXU0cTk9mLezpCPyzelQyhbxMGdKgFo0_JTgTD1q1VU-kj10aqxxA$ > > from about ~5 years ago; > > if this patch is a valid proposal, it could fix this case as well; > well, it might break others, so applying it [now] would need some general > review > of all pre/post enable/disable hooks > So, apologies if this will start to seem like spamming. I decided to do a bit of shell magic for this: get_files() { git grep -w iio_buffer_setup_ops | grep drivers | cut -d: -f1 | sort | uniq } for file in $(get_files) ; do if grep -q currentmode $file ; then echo $file fi done It finds 4 drivers. Though, `get_files()` will return 56 files. drivers/iio/accel/bmc150-accel-core.c drivers/iio/adc/at91-sama5d2_adc.c drivers/iio/adc/stm32-dfsdm-adc.c drivers/iio/magnetometer/rm3100-core.c The rm3100 driver doesn't do any checks in the setup_ops for 'currentmode' as far as I could see. So, Lars' patch could work nicely to fix this current case and not break others. Semantically though, it would sound nicer to have a 'nextmode' parameter somewhere; maybe on the setup_ops(indio_dev, nextmode)? Though, only those 3 drivers would really ever use it; so doing it like that sounds like overkill. So, we're left with Lars' patch or we could add an 'indio_dev->nextmode' field, that may be used in just these 3 drivers [which again: sounds overkill at this point in time]. Alternatively, this 'indio_dev->currentmode' could be removed from all these 3 drivers somehow. But that needs testing and a thorough understanding of all 3 drivers and what they're doing, to do properly. @Jonathan: what do you think? In any case, pending a reply, I'll send Lars' patch. Even if we come to a different conclusion we have something to start with. But, if the conclusion is that Lars' patch is a good solution now, it can be applied. > > Apologies for the breakage. > > > > For the touch-part I don't see that code being executed. > > > > But a question is: does the driver need to check for the currentmode? > > Or is that something that the IIO core should do? > > > > > And with this , the preenable fails on my side, because the current mode > > > is not yet switched to triggered. > > > > > > I do remember adding this line with a specific reason. It may be related > > > to touchscreen operations, but I have to retest the touch with and > > > without this line and your patch. > > > > > > Meanwhile, maybe you have any suggestions on how to fix the buffer ? > > > > Well, there was the question of whether iio_triggered_buffer_postenable() > > [to > > attach the pollfunc] makes sense to be called first/last in the old > > at91_adc_buffer_postenable(), and the answer was 'last'; so then one > > solution > > was to move things to preenable(). > > > > Going back to the old patch isn't ideal, as the idea was to make the > > position > > of > > iio_triggered_buffer_postenable() consistent across all drivers, so that it > > can > > be removed [and moved to the IIO core]. > > > > But if we need revert the patch, then I guess it's fine. > > The only solution I see right now [for going forward], is to remove that > > check > > for 'currrentmode' > > > > > This check here makes any sense to you ? > > > > I think Jonathan may have to add some input here, but I think that in this > > current situation, checking 'currentmode' looks like is re-validating how > > the > > device was configured via the IIO framework. > > I am not sure if it's needed or not. > > > > > Thanks, > > > Eugen > > > > > > > > > I will try to test it , if I manage to apply it. > > > > > > I can only test the ADC though because at this moment I do not have > > > > > > a > > > > > > touchscreen at disposal. > > > > > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > > > > > > > By the way, I do not know if my two pending patches on this driver > > > > > > will > > > > > > conflict or not. > > > > > > > > > > As this is a long term rework patch at heart, there isn't any > > > > > particular > > > > > rush as long as we don't loose it forever! > > > > > > > > > > Thanks, > > > > > > > > > > Jonathan > > > > > > > > > > > Eugen > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > This patchset continues discussion: > > > > > > > > > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ > > > > > > > > > > > > > > > > Apologies for the delay. > > > > > > > > > > > > > > > > Changelog v1 -> v2: > > > > > > > > * added patch 'iio: at91-sama5d2_adc: split > > > > > > > > at91_adc_current_chan_is_touch() > > > > > > > > helper' > > > > > > > > * renamed at91_adc_buffer_postenable() -> > > > > > > > > at91_adc_buffer_preenable() > > > > > > > > - at91_adc_buffer_postenable() - now just calls > > > > > > > > iio_triggered_buffer_postenable() if the channel isn't the > > > > > > > > touchscreen > > > > > > > > channel > > > > > > > > * renamed at91_adc_buffer_predisable() -> > > > > > > > > at91_adc_buffer_postdisable() > > > > > > > > - at91_adc_buffer_predisable() - now just calls > > > > > > > > iio_triggered_buffer_predisable() if the channel isn't the > > > > > > > > touchscreen > > > > > > > > channel > > > > > > > > > > > > > > > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++---- > > > > > > > > ---- > > > > > > > > ------- > > > > > > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > b/drivers/iio/adc/at91- > > > > > > > > sama5d2_adc.c > > > > > > > > index a5c7771227d5..f2a74c47c768 100644 > > > > > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct > > > > > > > > iio_dev > > > > > > > > *indio_dev) > > > > > > > > return 0; > > > > > > > > } > > > > > > > > > > > > > > > > +static bool at91_adc_current_chan_is_touch(struct iio_dev > > > > > > > > *indio_dev) > > > > > > > > +{ > > > > > > > > + struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > + > > > > > > > > + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > + &st->touch_st.channels_bitmask, > > > > > > > > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > > > > > +} > > > > > > > > + > > > > > > > > static int at91_adc_buffer_postenable(struct iio_dev > > > > > > > > *indio_dev) > > > > > > > > { > > > > > > > > int ret; > > > > > > > > struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > > touchscreen > > > > > > > > */ > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > - /* touchscreen enabling */ > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > > - } > > > > > > > > + > > > > > > > > /* if we are not in triggered mode, we cannot enable the > > > > > > > > buffer. > > > > > > > > */ > > > > > > > > if (!(indio_dev->currentmode & > > > > > > > > INDIO_ALL_TRIGGERED_MODES)) > > > > > > > > return -EINVAL; > > > > > > > > @@ -906,12 +912,9 @@ static int > > > > > > > > at91_adc_buffer_predisable(struct > > > > > > > > iio_dev *indio_dev) > > > > > > > > u8 bit; > > > > > > > > > > > > > > > > /* check if we are disabling triggered buffer or the > > > > > > > > touchscreen > > > > > > > > */ > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > - /* touchscreen disable */ > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > return at91_adc_configure_touch(st, false); > > > > > > > > - } > > > > > > > > + > > > > > > > > /* if we are not in triggered mode, nothing to do here > > > > > > > > */ > > > > > > > > if (!(indio_dev->currentmode & > > > > > > > > INDIO_ALL_TRIGGERED_MODES)) > > > > > > > > return -EINVAL; > > > > > > > > @@ -1886,14 +1889,10 @@ static __maybe_unused int > > > > > > > > at91_adc_resume(struct > > > > > > > > device *dev) > > > > > > > > return 0; > > > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > > touchscreen > > > > > > > > */ > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > - /* touchscreen enabling */ > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > > - } else { > > > > > > > > + else > > > > > > > > return at91_adc_configure_trigger(st->trig, > > > > > > > > true); > > > > > > > > - } > > > > > > > > > > > > > > > > /* not needed but more explicit */ > > > > > > > > return 0; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-30 8:10 ` Ardelean, Alexandru @ 2020-05-02 17:31 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2020-05-02 17:31 UTC (permalink / raw) To: Ardelean, Alexandru Cc: Eugen.Hristev, Ludovic.Desroches, linux-kernel, linux-iio On Thu, 30 Apr 2020 08:10:29 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Thu, 2020-04-30 at 07:30 +0000, Ardelean, Alexandru wrote: > > On Mon, 2020-04-27 at 13:00 +0000, Ardelean, Alexandru wrote: > > > [External] > > > > > > On Mon, 2020-04-27 at 12:20 +0000, Eugen.Hristev@microchip.com wrote: > > > > [External] > > > > > > > > On 15.04.2020 09:33, Ardelean, Alexandru wrote: > > > > > > > > > On Tue, 2020-04-14 at 18:45 +0100, Jonathan Cameron wrote: > > > > > > On Tue, 14 Apr 2020 12:22:45 +0000 > > > > > > <Eugen.Hristev@microchip.com> wrote: > > > > > > > > > > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > > > > > > > > > > > This change moves the logic to check if the current channel is > > > > > > > > > the > > > > > > > > > touchscreen channel to a separate helper. > > > > > > > > > This reduces some code duplication, but the main intent is to > > > > > > > > > re- > > > > > > > > > use > > > > > > > > > this > > > > > > > > > in the next patches. > > > > > > > > > > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com > > > > > > > > > > > > > > > > > > Eugen / Ludovic, > > > > > > > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > > > Does the patch apply correctly for you ? > > > > > > > > > > > > I haven't tried yet :) > > > > > > > > > > > > > > > > I've rebased this patchset on top of current iio/testing and it still > > > > > applies. > > > > > > > > > > > > > Hi Alex, > > > > > > > > I tried this patch on top of my tree (however I am testing with an older > > > > kernel 5.4) , and I have issues starting the buffer after you moved my > > > > code to the preenable callback. > > > > > > > > Namely, on the line: > > > > > > > > if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > return -EINVAL; > > > > In the meantime I found this patch: > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/1431525891-19285-5-git-send-email-lars@metafoo.de/__;!!A3Ni8CS0y2Y!ocQuNvFF_8rd-cCvMNXU0cTk9mLezpCPyzelQyhbxMGdKgFo0_JTgTD1q1VU-kj10aqxxA$ > > > > from about ~5 years ago; Yup. That started the transition to claim_direct_mode to avoid the locking issues. There are probably a few drivers that never got there but in theory moving the setting of this parameter earlier should now be fine as the original race condition is closed (subject to a few drivers that still have that race condition) > > > > if this patch is a valid proposal, it could fix this case as well; > > well, it might break others, so applying it [now] would need some general > > review > > of all pre/post enable/disable hooks > > > > So, apologies if this will start to seem like spamming. > I decided to do a bit of shell magic for this: > > get_files() { > git grep -w iio_buffer_setup_ops | grep drivers | cut -d: -f1 | sort | uniq > } > > for file in $(get_files) ; do > if grep -q currentmode $file ; then > echo $file > fi > done > > It finds 4 drivers. > Though, `get_files()` will return 56 files. > > drivers/iio/accel/bmc150-accel-core.c > drivers/iio/adc/at91-sama5d2_adc.c > drivers/iio/adc/stm32-dfsdm-adc.c > drivers/iio/magnetometer/rm3100-core.c > > The rm3100 driver doesn't do any checks in the setup_ops for 'currentmode' as > far as I could see. > > So, Lars' patch could work nicely to fix this current case and not break others. > > Semantically though, it would sound nicer to have a 'nextmode' parameter > somewhere; maybe on the setup_ops(indio_dev, nextmode)? > Though, only those 3 drivers would really ever use it; so doing it like that > sounds like overkill. > > So, we're left with Lars' patch or we could add an 'indio_dev->nextmode' field, > that may be used in just these 3 drivers [which again: sounds overkill at this > point in time]. > > Alternatively, this 'indio_dev->currentmode' could be removed from all these 3 > drivers somehow. But that needs testing and a thorough understanding of all 3 > drivers and what they're doing, to do properly. > > @Jonathan: what do you think? I think we are 'now' fine to apply what Lars originally suggested to solve this issue. There are still some difference between pre and post functions but for many usecases they no longer matter (if curious just look at what remains being called between them). > > In any case, pending a reply, I'll send Lars' patch. > Even if we come to a different conclusion we have something to start with. > But, if the conclusion is that Lars' patch is a good solution now, it can be > applied. > > > > > Apologies for the breakage. > > > > > > For the touch-part I don't see that code being executed. > > > > > > But a question is: does the driver need to check for the currentmode? > > > Or is that something that the IIO core should do? > > > > > > > And with this , the preenable fails on my side, because the current mode > > > > is not yet switched to triggered. > > > > > > > > I do remember adding this line with a specific reason. It may be related > > > > to touchscreen operations, but I have to retest the touch with and > > > > without this line and your patch. > > > > > > > > Meanwhile, maybe you have any suggestions on how to fix the buffer ? > > > > > > Well, there was the question of whether iio_triggered_buffer_postenable() > > > [to > > > attach the pollfunc] makes sense to be called first/last in the old > > > at91_adc_buffer_postenable(), and the answer was 'last'; so then one > > > solution > > > was to move things to preenable(). > > > > > > Going back to the old patch isn't ideal, as the idea was to make the > > > position > > > of > > > iio_triggered_buffer_postenable() consistent across all drivers, so that it > > > can > > > be removed [and moved to the IIO core]. > > > > > > But if we need revert the patch, then I guess it's fine. > > > The only solution I see right now [for going forward], is to remove that > > > check > > > for 'currrentmode' > > > > > > > This check here makes any sense to you ? > > > > > > I think Jonathan may have to add some input here, but I think that in this > > > current situation, checking 'currentmode' looks like is re-validating how > > > the > > > device was configured via the IIO framework. > > > I am not sure if it's needed or not. > > > > > > > Thanks, > > > > Eugen > > > > > > > > > > > I will try to test it , if I manage to apply it. > > > > > > > I can only test the ADC though because at this moment I do not have > > > > > > > a > > > > > > > touchscreen at disposal. > > > > > > > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > > > > > > > > > By the way, I do not know if my two pending patches on this driver > > > > > > > will > > > > > > > conflict or not. > > > > > > > > > > > > As this is a long term rework patch at heart, there isn't any > > > > > > particular > > > > > > rush as long as we don't loose it forever! > > > > > > > > > > > > Thanks, > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > Eugen > > > > > > > > > > > > > > > Thanks, > > > > > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > > > > > > This patchset continues discussion: > > > > > > > > > > > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/__;!!A3Ni8CS0y2Y!ql1bYiNMPFlz1twnCCAQpiEBvpzxR_VHAPL712rWFfwy2TSKjZ2UhGBoV7-29Syny6z0yg$ > > > > > > > > > > > > > > > > > > Apologies for the delay. > > > > > > > > > > > > > > > > > > Changelog v1 -> v2: > > > > > > > > > * added patch 'iio: at91-sama5d2_adc: split > > > > > > > > > at91_adc_current_chan_is_touch() > > > > > > > > > helper' > > > > > > > > > * renamed at91_adc_buffer_postenable() -> > > > > > > > > > at91_adc_buffer_preenable() > > > > > > > > > - at91_adc_buffer_postenable() - now just calls > > > > > > > > > iio_triggered_buffer_postenable() if the channel isn't the > > > > > > > > > touchscreen > > > > > > > > > channel > > > > > > > > > * renamed at91_adc_buffer_predisable() -> > > > > > > > > > at91_adc_buffer_postdisable() > > > > > > > > > - at91_adc_buffer_predisable() - now just calls > > > > > > > > > iio_triggered_buffer_predisable() if the channel isn't the > > > > > > > > > touchscreen > > > > > > > > > channel > > > > > > > > > > > > > > > > > > drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++---- > > > > > > > > > ---- > > > > > > > > > ------- > > > > > > > > > 1 file changed, 15 insertions(+), 16 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > > b/drivers/iio/adc/at91- > > > > > > > > > sama5d2_adc.c > > > > > > > > > index a5c7771227d5..f2a74c47c768 100644 > > > > > > > > > --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > > +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > > > > > > @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct > > > > > > > > > iio_dev > > > > > > > > > *indio_dev) > > > > > > > > > return 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static bool at91_adc_current_chan_is_touch(struct iio_dev > > > > > > > > > *indio_dev) > > > > > > > > > +{ > > > > > > > > > + struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > > + > > > > > > > > > + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > > + &st->touch_st.channels_bitmask, > > > > > > > > > + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > static int at91_adc_buffer_postenable(struct iio_dev > > > > > > > > > *indio_dev) > > > > > > > > > { > > > > > > > > > int ret; > > > > > > > > > struct at91_adc_state *st = iio_priv(indio_dev); > > > > > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > > > touchscreen > > > > > > > > > */ > > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > > - /* touchscreen enabling */ > > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > > > - } > > > > > > > > > + > > > > > > > > > /* if we are not in triggered mode, we cannot enable the > > > > > > > > > buffer. > > > > > > > > > */ > > > > > > > > > if (!(indio_dev->currentmode & > > > > > > > > > INDIO_ALL_TRIGGERED_MODES)) > > > > > > > > > return -EINVAL; > > > > > > > > > @@ -906,12 +912,9 @@ static int > > > > > > > > > at91_adc_buffer_predisable(struct > > > > > > > > > iio_dev *indio_dev) > > > > > > > > > u8 bit; > > > > > > > > > > > > > > > > > > /* check if we are disabling triggered buffer or the > > > > > > > > > touchscreen > > > > > > > > > */ > > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > > - /* touchscreen disable */ > > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > > return at91_adc_configure_touch(st, false); > > > > > > > > > - } > > > > > > > > > + > > > > > > > > > /* if we are not in triggered mode, nothing to do here > > > > > > > > > */ > > > > > > > > > if (!(indio_dev->currentmode & > > > > > > > > > INDIO_ALL_TRIGGERED_MODES)) > > > > > > > > > return -EINVAL; > > > > > > > > > @@ -1886,14 +1889,10 @@ static __maybe_unused int > > > > > > > > > at91_adc_resume(struct > > > > > > > > > device *dev) > > > > > > > > > return 0; > > > > > > > > > > > > > > > > > > /* check if we are enabling triggered buffer or the > > > > > > > > > touchscreen > > > > > > > > > */ > > > > > > > > > - if (bitmap_subset(indio_dev->active_scan_mask, > > > > > > > > > - &st->touch_st.channels_bitmask, > > > > > > > > > - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > > > > > > - /* touchscreen enabling */ > > > > > > > > > + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > > > > > > return at91_adc_configure_touch(st, true); > > > > > > > > > - } else { > > > > > > > > > + else > > > > > > > > > return at91_adc_configure_trigger(st->trig, > > > > > > > > > true); > > > > > > > > > - } > > > > > > > > > > > > > > > > > > /* not needed but more explicit */ > > > > > > > > > return 0; ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-14 12:22 ` Eugen.Hristev 2020-04-14 17:45 ` Jonathan Cameron @ 2020-04-15 6:43 ` ludovic.desroches 2020-04-18 17:58 ` Jonathan Cameron 1 sibling, 1 reply; 17+ messages in thread From: ludovic.desroches @ 2020-04-15 6:43 UTC (permalink / raw) To: Eugen Hristev - M18282 Cc: Jonathan Cameron, Alexandru Ardelean, linux-iio, linux-kernel On Tue, Apr 14, 2020 at 12:22:45PM +0000, Eugen Hristev - M18282 wrote: > On 13.04.2020 20:05, Jonathan Cameron wrote: > > On Wed, 4 Mar 2020 10:42:18 +0200 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > >> This change moves the logic to check if the current channel is the > >> touchscreen channel to a separate helper. > >> This reduces some code duplication, but the main intent is to re-use this > >> in the next patches. > >> > >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Eugen / Ludovic, > > > > Have you had a chance to look at this series? > > Hi Jonathan, > > Does the patch apply correctly for you ? No issue on my side to apply them (v5.7-rc1 and next). > I will try to test it , if I manage to apply it. > I can only test the ADC though because at this moment I do not have a > touchscreen at disposal. Same here, not able to test the touchscreen but it doesn't seem very risky. > > Meanwhile, the code looks good for me, > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> You can add mine as well: Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> Regards Ludovic > > By the way, I do not know if my two pending patches on this driver will > conflict or not. > > Eugen > > > > > Thanks, > > > > Jonathan > > > >> --- > >> > >> This patchset continues discussion: > >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > >> Apologies for the delay. > >> > >> Changelog v1 -> v2: > >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > >> helper' > >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > >> - at91_adc_buffer_postenable() - now just calls > >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen > >> channel > >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > >> - at91_adc_buffer_predisable() - now just calls > >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen > >> channel > >> > >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > >> 1 file changed, 15 insertions(+), 16 deletions(-) > >> > >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > >> index a5c7771227d5..f2a74c47c768 100644 > >> --- a/drivers/iio/adc/at91-sama5d2_adc.c > >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c > >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > >> return 0; > >> } > >> > >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > >> +{ > >> + struct at91_adc_state *st = iio_priv(indio_dev); > >> + > >> + return !!bitmap_subset(indio_dev->active_scan_mask, > >> + &st->touch_st.channels_bitmask, > >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > >> +} > >> + > >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > >> { > >> int ret; > >> struct at91_adc_state *st = iio_priv(indio_dev); > >> > >> /* check if we are enabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen enabling */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, true); > >> - } > >> + > >> /* if we are not in triggered mode, we cannot enable the buffer. */ > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > >> return -EINVAL; > >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > >> u8 bit; > >> > >> /* check if we are disabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen disable */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, false); > >> - } > >> + > >> /* if we are not in triggered mode, nothing to do here */ > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > >> return -EINVAL; > >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > >> return 0; > >> > >> /* check if we are enabling triggered buffer or the touchscreen */ > >> - if (bitmap_subset(indio_dev->active_scan_mask, > >> - &st->touch_st.channels_bitmask, > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > >> - /* touchscreen enabling */ > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > >> return at91_adc_configure_touch(st, true); > >> - } else { > >> + else > >> return at91_adc_configure_trigger(st->trig, true); > >> - } > >> > >> /* not needed but more explicit */ > >> return 0; > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-15 6:43 ` ludovic.desroches @ 2020-04-18 17:58 ` Jonathan Cameron 2020-04-19 10:09 ` ludovic.desroches 0 siblings, 1 reply; 17+ messages in thread From: Jonathan Cameron @ 2020-04-18 17:58 UTC (permalink / raw) To: ludovic.desroches Cc: Eugen Hristev - M18282, Alexandru Ardelean, linux-iio, linux-kernel On Wed, 15 Apr 2020 08:43:52 +0200 ludovic.desroches@microchip.com wrote: > On Tue, Apr 14, 2020 at 12:22:45PM +0000, Eugen Hristev - M18282 wrote: > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > >> This change moves the logic to check if the current channel is the > > >> touchscreen channel to a separate helper. > > >> This reduces some code duplication, but the main intent is to re-use this > > >> in the next patches. > > >> > > >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > Eugen / Ludovic, > > > > > > Have you had a chance to look at this series? > > > > Hi Jonathan, > > > > Does the patch apply correctly for you ? > > No issue on my side to apply them (v5.7-rc1 and next). > > > I will try to test it , if I manage to apply it. > > I can only test the ADC though because at this moment I do not have a > > touchscreen at disposal. > > Same here, not able to test the touchscreen but it doesn't seem very risky. > > > > > Meanwhile, the code looks good for me, > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > You can add mine as well: > > Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> For both of you - tags for both patches or just this one? Thanks, Jonathan > > Regards > > Ludovic > > > > > By the way, I do not know if my two pending patches on this driver will > > conflict or not. > > > > Eugen > > > > > > > > Thanks, > > > > > > Jonathan > > > > > >> --- > > >> > > >> This patchset continues discussion: > > >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > > >> Apologies for the delay. > > >> > > >> Changelog v1 -> v2: > > >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > > >> helper' > > >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > > >> - at91_adc_buffer_postenable() - now just calls > > >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen > > >> channel > > >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > > >> - at91_adc_buffer_predisable() - now just calls > > >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen > > >> channel > > >> > > >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > > >> 1 file changed, 15 insertions(+), 16 deletions(-) > > >> > > >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > > >> index a5c7771227d5..f2a74c47c768 100644 > > >> --- a/drivers/iio/adc/at91-sama5d2_adc.c > > >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > > >> return 0; > > >> } > > >> > > >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > > >> +{ > > >> + struct at91_adc_state *st = iio_priv(indio_dev); > > >> + > > >> + return !!bitmap_subset(indio_dev->active_scan_mask, > > >> + &st->touch_st.channels_bitmask, > > >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > >> +} > > >> + > > >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > > >> { > > >> int ret; > > >> struct at91_adc_state *st = iio_priv(indio_dev); > > >> > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > >> - &st->touch_st.channels_bitmask, > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > >> - /* touchscreen enabling */ > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > >> return at91_adc_configure_touch(st, true); > > >> - } > > >> + > > >> /* if we are not in triggered mode, we cannot enable the buffer. */ > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > >> return -EINVAL; > > >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > > >> u8 bit; > > >> > > >> /* check if we are disabling triggered buffer or the touchscreen */ > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > >> - &st->touch_st.channels_bitmask, > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > >> - /* touchscreen disable */ > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > >> return at91_adc_configure_touch(st, false); > > >> - } > > >> + > > >> /* if we are not in triggered mode, nothing to do here */ > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > >> return -EINVAL; > > >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > > >> return 0; > > >> > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > >> - &st->touch_st.channels_bitmask, > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > >> - /* touchscreen enabling */ > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > >> return at91_adc_configure_touch(st, true); > > >> - } else { > > >> + else > > >> return at91_adc_configure_trigger(st->trig, true); > > >> - } > > >> > > >> /* not needed but more explicit */ > > >> return 0; > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-18 17:58 ` Jonathan Cameron @ 2020-04-19 10:09 ` ludovic.desroches 2020-04-25 15:06 ` Jonathan Cameron 0 siblings, 1 reply; 17+ messages in thread From: ludovic.desroches @ 2020-04-19 10:09 UTC (permalink / raw) To: Jonathan Cameron Cc: Eugen Hristev - M18282, Alexandru Ardelean, linux-iio, linux-kernel On Sat, Apr 18, 2020 at 06:58:53PM +0100, Jonathan Cameron wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 15 Apr 2020 08:43:52 +0200 > ludovic.desroches@microchip.com wrote: > > > On Tue, Apr 14, 2020 at 12:22:45PM +0000, Eugen Hristev - M18282 wrote: > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > >> This change moves the logic to check if the current channel is the > > > >> touchscreen channel to a separate helper. > > > >> This reduces some code duplication, but the main intent is to re-use this > > > >> in the next patches. > > > >> > > > >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > Eugen / Ludovic, > > > > > > > > Have you had a chance to look at this series? > > > > > > Hi Jonathan, > > > > > > Does the patch apply correctly for you ? > > > > No issue on my side to apply them (v5.7-rc1 and next). > > > > > I will try to test it , if I manage to apply it. > > > I can only test the ADC though because at this moment I do not have a > > > touchscreen at disposal. > > > > Same here, not able to test the touchscreen but it doesn't seem very risky. > > > > > > > > Meanwhile, the code looks good for me, > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > You can add mine as well: > > > > Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > For both of you - tags for both patches or just this one? Sorry both patches for me, I just replied to patch 2/2 to ease the collection of tags. Ludovic > > Thanks, > > Jonathan > > > > > Regards > > > > Ludovic > > > > > > > > By the way, I do not know if my two pending patches on this driver will > > > conflict or not. > > > > > > Eugen > > > > > > > > > > > Thanks, > > > > > > > > Jonathan > > > > > > > >> --- > > > >> > > > >> This patchset continues discussion: > > > >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > > > >> Apologies for the delay. > > > >> > > > >> Changelog v1 -> v2: > > > >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > > > >> helper' > > > >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > > > >> - at91_adc_buffer_postenable() - now just calls > > > >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen > > > >> channel > > > >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > > > >> - at91_adc_buffer_predisable() - now just calls > > > >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen > > > >> channel > > > >> > > > >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > > > >> 1 file changed, 15 insertions(+), 16 deletions(-) > > > >> > > > >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > > > >> index a5c7771227d5..f2a74c47c768 100644 > > > >> --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > > > >> return 0; > > > >> } > > > >> > > > >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > > > >> +{ > > > >> + struct at91_adc_state *st = iio_priv(indio_dev); > > > >> + > > > >> + return !!bitmap_subset(indio_dev->active_scan_mask, > > > >> + &st->touch_st.channels_bitmask, > > > >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > >> +} > > > >> + > > > >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > > > >> { > > > >> int ret; > > > >> struct at91_adc_state *st = iio_priv(indio_dev); > > > >> > > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > >> - &st->touch_st.channels_bitmask, > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > >> - /* touchscreen enabling */ > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > >> return at91_adc_configure_touch(st, true); > > > >> - } > > > >> + > > > >> /* if we are not in triggered mode, we cannot enable the buffer. */ > > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > >> return -EINVAL; > > > >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > > > >> u8 bit; > > > >> > > > >> /* check if we are disabling triggered buffer or the touchscreen */ > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > >> - &st->touch_st.channels_bitmask, > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > >> - /* touchscreen disable */ > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > >> return at91_adc_configure_touch(st, false); > > > >> - } > > > >> + > > > >> /* if we are not in triggered mode, nothing to do here */ > > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > >> return -EINVAL; > > > >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > > > >> return 0; > > > >> > > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > >> - &st->touch_st.channels_bitmask, > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > >> - /* touchscreen enabling */ > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > >> return at91_adc_configure_touch(st, true); > > > >> - } else { > > > >> + else > > > >> return at91_adc_configure_trigger(st->trig, true); > > > >> - } > > > >> > > > >> /* not needed but more explicit */ > > > >> return 0; > > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper 2020-04-19 10:09 ` ludovic.desroches @ 2020-04-25 15:06 ` Jonathan Cameron 0 siblings, 0 replies; 17+ messages in thread From: Jonathan Cameron @ 2020-04-25 15:06 UTC (permalink / raw) To: ludovic.desroches Cc: Eugen Hristev - M18282, Alexandru Ardelean, linux-iio, linux-kernel On Sun, 19 Apr 2020 12:09:43 +0200 ludovic.desroches@microchip.com wrote: > On Sat, Apr 18, 2020 at 06:58:53PM +0100, Jonathan Cameron wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Wed, 15 Apr 2020 08:43:52 +0200 > > ludovic.desroches@microchip.com wrote: > > > > > On Tue, Apr 14, 2020 at 12:22:45PM +0000, Eugen Hristev - M18282 wrote: > > > > On 13.04.2020 20:05, Jonathan Cameron wrote: > > > > > On Wed, 4 Mar 2020 10:42:18 +0200 > > > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > > > >> This change moves the logic to check if the current channel is the > > > > >> touchscreen channel to a separate helper. > > > > >> This reduces some code duplication, but the main intent is to re-use this > > > > >> in the next patches. > > > > >> > > > > >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > Eugen / Ludovic, > > > > > > > > > > Have you had a chance to look at this series? > > > > > > > > Hi Jonathan, > > > > > > > > Does the patch apply correctly for you ? > > > > > > No issue on my side to apply them (v5.7-rc1 and next). > > > > > > > I will try to test it , if I manage to apply it. > > > > I can only test the ADC though because at this moment I do not have a > > > > touchscreen at disposal. > > > > > > Same here, not able to test the touchscreen but it doesn't seem very risky. > > > > > > > > > > > Meanwhile, the code looks good for me, > > > > > > > > Reviewed-by: Eugen Hristev <eugen.hristev@microchip.com> > > > > > > You can add mine as well: > > > > > > Reviewed-by: Ludovic Desroches <ludovic.desroches@microchip.com> > > > > For both of you - tags for both patches or just this one? > > Sorry both patches for me, I just replied to patch 2/2 to ease the collection > of tags. > > Ludovic Hi All, Some considerable fuzz due to Engen's patches going in first but the resulting diff looks right etc so I think I fixed it up correctly. Both patches applied to the togreg branch of iio.git and pushed out as testing for the autobuilders to play with it. Thanks, Jonathan > > > > > Thanks, > > > > Jonathan > > > > > > > > Regards > > > > > > Ludovic > > > > > > > > > > > By the way, I do not know if my two pending patches on this driver will > > > > conflict or not. > > > > > > > > Eugen > > > > > > > > > > > > > > Thanks, > > > > > > > > > > Jonathan > > > > > > > > > >> --- > > > > >> > > > > >> This patchset continues discussion: > > > > >> https://lore.kernel.org/linux-iio/20191023082508.17583-1-alexandru.ardelean@analog.com/ > > > > >> Apologies for the delay. > > > > >> > > > > >> Changelog v1 -> v2: > > > > >> * added patch 'iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() > > > > >> helper' > > > > >> * renamed at91_adc_buffer_postenable() -> at91_adc_buffer_preenable() > > > > >> - at91_adc_buffer_postenable() - now just calls > > > > >> iio_triggered_buffer_postenable() if the channel isn't the touchscreen > > > > >> channel > > > > >> * renamed at91_adc_buffer_predisable() -> at91_adc_buffer_postdisable() > > > > >> - at91_adc_buffer_predisable() - now just calls > > > > >> iio_triggered_buffer_predisable() if the channel isn't the touchscreen > > > > >> channel > > > > >> > > > > >> drivers/iio/adc/at91-sama5d2_adc.c | 31 +++++++++++++++--------------- > > > > >> 1 file changed, 15 insertions(+), 16 deletions(-) > > > > >> > > > > >> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c > > > > >> index a5c7771227d5..f2a74c47c768 100644 > > > > >> --- a/drivers/iio/adc/at91-sama5d2_adc.c > > > > >> +++ b/drivers/iio/adc/at91-sama5d2_adc.c > > > > >> @@ -873,18 +873,24 @@ static int at91_adc_dma_start(struct iio_dev *indio_dev) > > > > >> return 0; > > > > >> } > > > > >> > > > > >> +static bool at91_adc_current_chan_is_touch(struct iio_dev *indio_dev) > > > > >> +{ > > > > >> + struct at91_adc_state *st = iio_priv(indio_dev); > > > > >> + > > > > >> + return !!bitmap_subset(indio_dev->active_scan_mask, > > > > >> + &st->touch_st.channels_bitmask, > > > > >> + AT91_SAMA5D2_MAX_CHAN_IDX + 1); > > > > >> +} > > > > >> + > > > > >> static int at91_adc_buffer_postenable(struct iio_dev *indio_dev) > > > > >> { > > > > >> int ret; > > > > >> struct at91_adc_state *st = iio_priv(indio_dev); > > > > >> > > > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > > >> - &st->touch_st.channels_bitmask, > > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > >> - /* touchscreen enabling */ > > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > >> return at91_adc_configure_touch(st, true); > > > > >> - } > > > > >> + > > > > >> /* if we are not in triggered mode, we cannot enable the buffer. */ > > > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > >> return -EINVAL; > > > > >> @@ -906,12 +912,9 @@ static int at91_adc_buffer_predisable(struct iio_dev *indio_dev) > > > > >> u8 bit; > > > > >> > > > > >> /* check if we are disabling triggered buffer or the touchscreen */ > > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > > >> - &st->touch_st.channels_bitmask, > > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > >> - /* touchscreen disable */ > > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > >> return at91_adc_configure_touch(st, false); > > > > >> - } > > > > >> + > > > > >> /* if we are not in triggered mode, nothing to do here */ > > > > >> if (!(indio_dev->currentmode & INDIO_ALL_TRIGGERED_MODES)) > > > > >> return -EINVAL; > > > > >> @@ -1886,14 +1889,10 @@ static __maybe_unused int at91_adc_resume(struct device *dev) > > > > >> return 0; > > > > >> > > > > >> /* check if we are enabling triggered buffer or the touchscreen */ > > > > >> - if (bitmap_subset(indio_dev->active_scan_mask, > > > > >> - &st->touch_st.channels_bitmask, > > > > >> - AT91_SAMA5D2_MAX_CHAN_IDX + 1)) { > > > > >> - /* touchscreen enabling */ > > > > >> + if (at91_adc_current_chan_is_touch(indio_dev)) > > > > >> return at91_adc_configure_touch(st, true); > > > > >> - } else { > > > > >> + else > > > > >> return at91_adc_configure_trigger(st->trig, true); > > > > >> - } > > > > >> > > > > >> /* not needed but more explicit */ > > > > >> return 0; > > > > > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-02 17:31 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-04 8:42 [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Alexandru Ardelean 2020-03-04 8:42 ` [PATCH v2 2/2] iio: at91-sama5d2_adc: adjust iio_triggered_buffer_{predisable,postenable} positions Alexandru Ardelean 2020-04-19 10:04 ` ludovic.desroches 2020-04-23 10:56 ` Eugen.Hristev 2020-04-13 17:05 ` [PATCH v2 1/2] iio: at91-sama5d2_adc: split at91_adc_current_chan_is_touch() helper Jonathan Cameron 2020-04-14 12:22 ` Eugen.Hristev 2020-04-14 17:45 ` Jonathan Cameron 2020-04-15 6:33 ` Ardelean, Alexandru 2020-04-27 12:20 ` Eugen.Hristev 2020-04-27 13:00 ` Ardelean, Alexandru 2020-04-30 7:30 ` Ardelean, Alexandru 2020-04-30 8:10 ` Ardelean, Alexandru 2020-05-02 17:31 ` Jonathan Cameron 2020-04-15 6:43 ` ludovic.desroches 2020-04-18 17:58 ` Jonathan Cameron 2020-04-19 10:09 ` ludovic.desroches 2020-04-25 15:06 ` 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).