linux-iio.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Nuno Sa <Nuno.Sa@analog.com>
Subject: Re: [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs
Date: Sun, 26 Sep 2021 15:38:41 +0100	[thread overview]
Message-ID: <20210926153841.48b60e5b@jic23-huawei> (raw)
In-Reply-To: <20210921115408.66711-1-miquel.raynal@bootlin.com>

On Tue, 21 Sep 2021 13:53:52 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Until now the max1027.c driver, which handles 10-bit devices (max10xx)
> and 12-bit devices (max12xx), only supported internal triggers. When the
> hardware trigger is not wired it is very convenient to use external
> triggers. Overall, when several values are needed at the same time,
> using triggers and buffers improves quite a lot the performances.
> 
> This series does a bit of cleaning/code reorganization before actually
> bringing more flexibility to the driver, up to the point where it is
> possible to use an external trigger, even without the IRQ line wired.
> 
> This series is currently based on a v5.15-rc1 kernel and the external
> triggering mechanism has been tested on a custom board where the IRQ and
> the EOC lines have not been populated.
> 
> How to test sysfs triggers:
>     echo 0 > /sys/bus/iio/devices/iio_sysfs_trigger/add_trigger
>     cat /sys/bus/iio/devices/iio_sysfs_trigger/trigger0/name > \
>         /sys/bus/iio/devices/iio:device0/trigger/current_trigger
>     echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltageX_en
>     echo 1 > /sys/bus/iio/devices/iio:device0/scan_elements/in_voltageY_en
>     echo 1 > /sys/bus/iio/devices/iio:device0/buffer/enable
>     cat /dev/iio\:device0 > /tmp/data &
>     echo 1 > /sys/bus/iio/devices/trigger0/trigger_now
>     od -t x1 /tmp/data
> 

Series applied to the togreg branch of iio.git and pushed out as testing
to see if 0-day can find anything we missed.

Thanks for persisting on this one.

Jonathan

> Cheers,
> Miquèl
> 
> Changes in v4:
> * Full rework again of the last few patches bringing external triggers
>   support according to Jonathan's reviews: trigger handling (internal
>   or external) should be in a dedicated helper to keep the exchanges
>   between the IIO core and the drivers standards. This also improves
>   reusability and now the max1027 trigger can also be used to trigger
>   other IIO devices.
> 
> Changes in v3:
> * Rebased on top of v5.15-rc1.
> * Dropped the useless change from devm_kmalloc to kmalloc because I
>   thought devm_kmalloc allocations were still not suitable for DMA
>   purposes.
> * Added a comment explaining the use of the available and active masks
>   in the code as suggested by Jonathan.
> * Released the lock used in iio_device_claim_direct_mode() in the two
>   error paths.
> * Did not move the call to reinit_completion before
>   wait_for_completion_timeout() as advised by Nuno because the
>   triggering is done before entering the waiting thread, so there is a
>   world were we reinit a completion object right before waiting for it
>   (which would lead to a timeout).
> * Deeply rewored the various handlers (see my answer to
>   "[PATCH v2 15/16] iio: adc: max1027: Add support for external  triggers"
> 
> Changes in v2:
> [All]
> * Overall quite a few changes, I'll try to list them here but I made
>   significant changes on the last few commits so it's hard to have an
>   exhaustive and detailed list.
> * Simplified the return statements as advised by Nuno.
> * Dropped useless debug messages.
> * Used iio_trigger_validate_own_device() instead of an internal
>   variable when possible.
> * Added Nuno's Reviewed-by's when relevant.
> [Created a new patch to fix the style]
> [Created a new patch to ensure st->buffer is DMA-safe]
> [Push only the requested samples]
> * Dropped a useless check over active_scan_mask mask in
>   ->set_trigger_state().  
> * Dropped the st->buffer indirection with a missing __be16 type.
> * Do not push only the requested samples in the IIO buffers, rely on the
>   core to handle this by providing additional 'available_scan_masks'
>   instead of dropping this entry from the initial setup.
> [Create a helper to configure the trigger]
> * Avoided messing with new lines.
> * Dropped cnvst_trigger, used a function parameter instead.
> [Prevent single channel accesses during buffer reads]
> * Used iio_device_claim_direct_mode() when relevant.
> * Dropped the extra iio_buffer_enabled() call.
> * Prevented returning with a mutex held.
> [Introduce an end of conversion helper]
> * Moved the check against active scan mask to the very end of the series
>   where we actually make use of it.
> * Moved the Queue declaration to another patch.
> [Dropped the patch: Prepare re-using the EOC interrupt]
> [Consolidate the end of conversion helper]
> * Used a dynamic completion object instead of a static queue.
> * Reworded the commit message to actually describe what this commit
>   does.
> [Support software triggers]
> * Dropped the patch and replaced it with something hopefully close to
>   what Jonathan and Nuno described in their reviews.
> [Enable software triggers to be  used without IRQ]
> * Wrote a more generic commit message, not focusing on software
>   triggers.
> 
> Miquel Raynal (16):
>   iio: adc: max1027: Fix style
>   iio: adc: max1027: Drop extra warning message
>   iio: adc: max1027: Drop useless debug messages
>   iio: adc: max1027: Minimize the number of converted channels
>   iio: adc: max1027: Rename a helper
>   iio: adc: max1027: Create a helper to enable/disable the cnvst trigger
>   iio: adc: max1027: Simplify the _set_trigger_state() helper
>   iio: adc: max1027: Ensure a default cnvst trigger configuration
>   iio: adc: max1027: Create a helper to configure the channels to scan
>   iio: adc: max1027: Prevent single channel accesses during buffer reads
>   iio: adc: max1027: Separate the IRQ handler from the read logic
>   iio: adc: max1027: Introduce an end of conversion helper
>   iio: adc: max1027: Stop requesting a threaded IRQ
>   iio: adc: max1027: Use the EOC IRQ when populated for single reads
>   iio: adc: max1027: Allow all kind of triggers to be used
>   iio: adc: max1027: Don't reject external triggers when there is no IRQ
> 
>  drivers/iio/adc/max1027.c | 286 +++++++++++++++++++++++++++-----------
>  1 file changed, 205 insertions(+), 81 deletions(-)
> 


      parent reply	other threads:[~2021-09-26 14:34 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 11:53 Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 01/16] iio: adc: max1027: Fix style Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 02/16] iio: adc: max1027: Drop extra warning message Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 03/16] iio: adc: max1027: Drop useless debug messages Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 04/16] iio: adc: max1027: Minimize the number of converted channels Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 05/16] iio: adc: max1027: Rename a helper Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 06/16] iio: adc: max1027: Create a helper to enable/disable the cnvst trigger Miquel Raynal
2021-09-21 11:53 ` [PATCH v4 07/16] iio: adc: max1027: Simplify the _set_trigger_state() helper Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 08/16] iio: adc: max1027: Ensure a default cnvst trigger configuration Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 09/16] iio: adc: max1027: Create a helper to configure the channels to scan Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 10/16] iio: adc: max1027: Prevent single channel accesses during buffer reads Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 11/16] iio: adc: max1027: Separate the IRQ handler from the read logic Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 12/16] iio: adc: max1027: Introduce an end of conversion helper Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 13/16] iio: adc: max1027: Stop requesting a threaded IRQ Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 14/16] iio: adc: max1027: Use the EOC IRQ when populated for single reads Miquel Raynal
2021-09-26 14:36   ` Jonathan Cameron
2021-09-27 14:30     ` Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 15/16] iio: adc: max1027: Allow all kind of triggers to be used Miquel Raynal
2021-09-21 11:54 ` [PATCH v4 16/16] iio: adc: max1027: Don't reject external triggers when there is no IRQ Miquel Raynal
2021-09-26 14:38 ` Jonathan Cameron [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210926153841.48b60e5b@jic23-huawei \
    --to=jic23@kernel.org \
    --cc=Nuno.Sa@analog.com \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=miquel.raynal@bootlin.com \
    --cc=thomas.petazzoni@bootlin.com \
    --subject='Re: [PATCH v4 00/16] Bring external triggers support to MAX1027-like ADCs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).