Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210217083438.37865-1-alexandru.ardelean@analog.com>
 <20210217083438.37865-3-alexandru.ardelean@analog.com> <20210218134240.2dd1feb2@archlinux>
In-Reply-To: <20210218134240.2dd1feb2@archlinux>
From:   Alexandru Ardelean <ardeleanalex@gmail.com>
Date:   Fri, 19 Feb 2021 10:45:39 +0200
Message-ID: <CA+U=DsqhDYgs-HYnC3-T0d0ubvkG8eZQifx1XLzvUa5K-Ru+bg@mail.gmail.com>
Subject: Re: [PATCH v2 2/5] iio: Add output buffer support
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Alexandru Ardelean <alexandru.ardelean@analog.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@analog.com>,
        =?UTF-8?B?TnVubyBTw6E=?= <nuno.sa@analog.com>,
        "Bogdan, Dragos" <dragos.bogdan@analog.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 18, 2021 at 5:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 17 Feb 2021 10:34:35 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> 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>
>
> Just one question on this, otherwise looks good to me.
>
> Jonathan
>
> > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
> > index 5d641f8adfbd..b9970c68005d 100644
> > --- a/drivers/iio/industrialio-buffer.c
> > +++ b/drivers/iio/industrialio-buffer.c
> > @@ -162,6 +162,69 @@ static ssize_t iio_buffer_read(struct file *filp, char __user *buf,
> >       return ret;
> >  }
> >
> ...
>
> >  /**
> >   * iio_buffer_poll() - poll the buffer to find out if it has data
> >   * @filp:    File structure pointer for device access
> > @@ -182,8 +245,19 @@ static __poll_t iio_buffer_poll(struct file *filp,
> >               return 0;
> >
> >       poll_wait(filp, &rb->pollq, wait);
> > -     if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > -             return EPOLLIN | EPOLLRDNORM;
> > +
> > +     switch (rb->direction) {
> > +     case IIO_BUFFER_DIRECTION_IN:
> > +             if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
> > +                     return EPOLLIN | EPOLLRDNORM;
> > +             break;
> > +     case IIO_BUFFER_DIRECTION_OUT:
> > +             if (iio_buffer_space_available(rb) >= rb->watermark)
> > +                     return EPOLLOUT | EPOLLWRNORM;
> > +             break;
> > +     }
> > +
> > +     /* need a way of knowing if there may be enough data... */
>
> Curious on what this comment is referring to?

I'm also a bit curious about what this comment is referring to.
I can remove it.

Maybe Lars can give some insight.

>
> >       return 0;
> >  }
> >
> > @@ -232,6 +306,16 @@ void iio_buffer_wakeup_poll(struct iio_dev *indio_dev)
> >       }
> >  }
> ...