* [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 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-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 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 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 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-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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.