linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Nuno Sá" <noname.nuno@gmail.com>
To: Mihail Chindris <mihail.chindris@analog.com>,
	linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Cc: lars@metafoo.de, Michael.Hennerich@analog.com, jic23@kernel.org,
	nuno.sa@analog.com, dragos.bogdan@analog.com,
	alexandru.ardelean@analog.com
Subject: Re: [PATCH v4 1/6] iio: Add output buffer support
Date: Mon, 23 Aug 2021 15:50:14 +0200	[thread overview]
Message-ID: <2a67cb8ee77cb96bc899d82348cb0eb0bf44925c.camel@gmail.com> (raw)
In-Reply-To: <20210820165927.4524-2-mihail.chindris@analog.com>

On Fri, 2021-08-20 at 16:59 +0000, Mihail Chindris wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Currently IIO only supports buffer mode for capture devices like
> ADCs. Add
> support for buffered mode for output devices like DACs.
> 
> The output buffer implementation is analogous to the input buffer
> implementation. Instead of using read() to get data from the buffer
> write()
> is used to copy data into the buffer.
> 
> poll() with POLLOUT will wakeup if there is space available for more
> or
> equal to the configured watermark of samples.
> 
> Drivers can remove data from a buffer using
> iio_buffer_remove_sample(), the
> function can e.g. called from a trigger handler to write the data to
> hardware.
> 
> A buffer can only be either a output buffer or an input, but not
> both. So,
> for a device that has an ADC and DAC path, this will mean 2 IIO
> buffers
> (one for each direction).
> 
> The direction of the buffer is decided by the new direction field of
> the
> iio_buffer struct and should be set after allocating and before
> registering
> it.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> Signed-off-by: Mihail Chindris <mihail.chindris@analog.com>
> ---
>  drivers/iio/iio_core.h            |   4 +
>  drivers/iio/industrialio-buffer.c | 133
> +++++++++++++++++++++++++++++-
>  drivers/iio/industrialio-core.c   |   1 +
>  include/linux/iio/buffer.h        |   7 ++
>  include/linux/iio/buffer_impl.h   |  11 +++
>  5 files changed, 154 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
> index 8f4a9b264962..61e318431de9 100644
> --- a/drivers/iio/iio_core.h
> +++ b/drivers/iio/iio_core.h
> @@ -68,12 +68,15 @@ __poll_t iio_buffer_poll_wrapper(struct file
> *filp,
>  				 struct poll_table_struct *wait);
>  ssize_t iio_buffer_read_wrapper(struct file *filp, char __user *buf,
>  				size_t n, loff_t *f_ps);
> +ssize_t iio_buffer_write_wrapper(struct file *filp, const char
> __user *buf,
> +				 size_t n, loff_t *f_ps);
>  
>  int iio_buffers_alloc_sysfs_and_mask(struct iio_dev *indio_dev);
>  void iio_buffers_free_sysfs_and_mask(struct iio_dev *indio_dev);
>  
>  #define iio_buffer_poll_addr (&iio_buffer_poll_wrapper)
>  #define iio_buffer_read_outer_addr (&iio_buffer_read_wrapper)
> +#define iio_buffer_write_outer_addr (&iio_buffer_write_wrapper)
>  
>  void iio_disable_all_buffers(struct iio_dev *indio_dev);
>  void iio_buffer_wakeup_poll(struct iio_dev *indio_dev);
> @@ -83,6 +86,7 @@ void iio_device_detach_buffers(struct iio_dev
> *indio_dev);
>  
>  #define iio_buffer_poll_addr NULL
>  #define iio_buffer_read_outer_addr NULL
> +#define iio_buffer_write_outer_addr NULL
>  
>  static inline int iio_buffers_alloc_sysfs_and_mask(struct iio_dev
> *indio_dev)
>  {
> diff --git a/drivers/iio/industrialio-buffer.c
> b/drivers/iio/industrialio-buffer.c
> index a95cc2da56be..73d4451a0572 100644
> --- a/drivers/iio/industrialio-buffer.c
> +++ b/drivers/iio/industrialio-buffer.c
> @@ -161,6 +161,69 @@ static ssize_t iio_buffer_read(struct file
> *filp, char __user *buf,
>  	return ret;
>  }
>  
> +static size_t iio_buffer_space_available(struct iio_buffer *buf)
> +{
> +	if (buf->access->space_available)
> +		return buf->access->space_available(buf);
> +
> +	return SIZE_MAX;
> +}
> +
> +static ssize_t iio_buffer_write(struct file *filp, const char __user
> *buf,
> +				size_t n, loff_t *f_ps)
> +{
> +	struct iio_dev_buffer_pair *ib = filp->private_data;
> +	struct iio_buffer *rb = ib->buffer;
> +	struct iio_dev *indio_dev = ib->indio_dev;
> +	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> +	size_t datum_size;
> +	size_t to_wait;
> +	int ret;
> +

Even though I do not agree that this is suficient, we should have the
same check as we have for input buffer:


if (!indio_dev->info)
	return -ENODEV;

> +	if (!rb || !rb->access->write)
> +		return -EINVAL;
> +
> +	datum_size = rb->bytes_per_datum;
> +
> +	/*
> +	 * If datum_size is 0 there will never be anything to read from
> the
> +	 * buffer, so signal end of file now.
> +	 */
> +	if (!datum_size)
> +		return 0;
> +
> +	if (filp->f_flags & O_NONBLOCK)
> +		to_wait = 0;
> +	else
> +		to_wait = min_t(size_t, n / datum_size, rb->watermark);
> +

I had a bit of a crazy thought... Typically, output devices do not
really have a typical trigger as we are used to have in input devices.
Hence, typically we just use a hrtimer (maybe a pwm-trigger can also be
added) trigger where we kind of poll the buffer for new data to send to
the device. So, I was wondering if we could just optionally bypass the
kbuf path in output devices (via some optional buffer option)? At this
point, we pretty much already know that we have data to consume :).
This would be kind of a synchronous interface. One issue I see with
this is that we cannot really have a deterministic (or close) sampling
frequency as we have for example with a pwm based trigger.

Anyways, just me throwing some ideas. This is not the most important
thing for now...

- Nuno Sá 
> 


  parent reply	other threads:[~2021-08-23 13:50 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 16:59 [PATCH v4 0/6] iio: Add output buffer support and DAC example Mihail Chindris
2021-08-20 16:59 ` [PATCH v4 1/6] iio: Add output buffer support Mihail Chindris
2021-08-21  0:24   ` kernel test robot
2021-08-23  2:23   ` kernel test robot
2021-08-23 13:50   ` Nuno Sá [this message]
2021-08-30 15:42     ` Jonathan Cameron
2021-09-01  8:50       ` Sa, Nuno
2021-09-05  9:54         ` Jonathan Cameron
2021-08-25  8:35   ` Alexandru Ardelean
2021-08-30 16:05   ` Jonathan Cameron
2021-09-01  8:54     ` Sa, Nuno
2021-09-05  9:55       ` Jonathan Cameron
2021-09-16 10:57     ` Chindris, Mihail
2021-08-20 16:59 ` [PATCH v4 2/6] iio: kfifo-buffer: " Mihail Chindris
2021-08-21 14:15   ` kernel test robot
2021-08-23 13:51   ` Nuno Sá
2021-08-20 16:59 ` [PATCH v4 3/6] iio: triggered-buffer: extend support to configure output buffers Mihail Chindris
2021-08-21  3:28   ` kernel test robot
2021-08-20 16:59 ` [PATCH v4 4/6] Documentation:ABI:testing:add doc for AD3552R ABI Mihail Chindris
2021-08-30 15:22   ` Jonathan Cameron
2021-08-20 16:59 ` [PATCH v4 5/6] dt-bindings: iio: dac: Add adi,ad3552r.yaml Mihail Chindris
2021-08-30 15:37   ` Jonathan Cameron
2021-08-20 16:59 ` [PATCH v4 6/6] drivers:iio:dac: Add AD3552R driver support Mihail Chindris
2021-08-20 19:55   ` kernel test robot
2021-08-20 22:15   ` kernel test robot
2021-08-23 14:01   ` Nuno Sá
2021-08-30 16:54   ` Jonathan Cameron
2021-08-25  7:35 ` [PATCH v4 0/6] iio: Add output buffer support and DAC example Alexandru Ardelean
2021-08-30 15:17   ` Jonathan Cameron
     [not found] <202108222341.6RtluiRt-lkp@intel.com>
2021-08-25 15:31 ` [PATCH v4 1/6] iio: Add output buffer support kernel test robot

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=2a67cb8ee77cb96bc899d82348cb0eb0bf44925c.camel@gmail.com \
    --to=noname.nuno@gmail.com \
    --cc=Michael.Hennerich@analog.com \
    --cc=alexandru.ardelean@analog.com \
    --cc=dragos.bogdan@analog.com \
    --cc=jic23@kernel.org \
    --cc=lars@metafoo.de \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mihail.chindris@analog.com \
    --cc=nuno.sa@analog.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).