Message-ID: <3370ba6d9a6bb8da5ca1415c354a6076de6f1d79.camel@gmail.com>
Date: Tue, 01 Oct 2024 10:14:45 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Jonathan Cameron
 <jic23@...nel.org>,  Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
	 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Rob Herring
	 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
	 <conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 05/10] iio: backend: extend features

On Mon, 2024-09-30 at 14:25 -0500, David Lechner wrote:
> On 9/29/24 6:05 AM, Jonathan Cameron wrote:
> > On Thu, 19 Sep 2024 11:20:01 +0200
> > Angelo Dureghello <adureghello@...libre.com> wrote:
> > 
> > > From: Angelo Dureghello <adureghello@...libre.com>
> > > 
> > > Extend backend features with new calls needed later on this
> > > patchset from axi version of ad3552r.
> > > 
> > > The follwoing calls are added:
> > > 
> > > iio_backend_ext_sync_enable
> > > 	enable synchronize channels on external trigger
> > > iio_backend_ext_sync_disable
> > > 	disable synchronize channels on external trigger
> > > iio_backend_ddr_enable
> > > 	enable ddr bus transfer
> > > iio_backend_ddr_disable
> > > 	disable ddr bus transfer
> > > iio_backend_set_bus_mode
> > > 	select the type of bus, so that specific read / write
> > > 	operations are performed accordingly
> > > iio_backend_buffer_enable
> > > 	enable buffer
> > > iio_backend_buffer_disable
> > > 	disable buffer
> > > iio_backend_data_transfer_addr
> > > 	define the target register address where the DAC sample
> > > 	will be written.
> > > 
> > > Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> > Hi Angelo,
> > A few trivial comments inline.
> > 
> > > ---
> > >  drivers/iio/industrialio-backend.c | 111
> > > +++++++++++++++++++++++++++++++++++++
> > >  include/linux/iio/backend.h        |  23 ++++++++
> > >  2 files changed, 134 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-backend.c
> > > b/drivers/iio/industrialio-backend.c
> > > index 20b3b5212da7..f4802c422dbf 100644
> > > --- a/drivers/iio/industrialio-backend.c
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -718,6 +718,117 @@ static int __devm_iio_backend_get(struct device
> > > *dev, struct iio_backend *back)
> > ...
> > 
> > > +/**
> > > + * iio_backend_ddr_disable - Disable interface DDR (Double Data Rate)
> > > mode
> > > + * @back: Backend device
> > > + *
> > > + * Disabling DDR data is generated byt the IP at rising or falling front
> > 
> > Spell check your comments.
> > 
> > > + * of the interface clock signal (SDR, Single Data Rate).
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_ddr_disable(struct iio_backend *back)
> > > +{
> > > +	return iio_backend_op_call(back, ddr_disable);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_disable, IIO_BACKEND);
> > 				 struct fwnode_handle *fwnode)
> > >  {
> > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> > > index 37d56914d485..41619b803cd6 100644
> > > --- a/include/linux/iio/backend.h
> > > +++ b/include/linux/iio/backend.h
> > > @@ -14,12 +14,14 @@ struct iio_dev;
> > >  enum iio_backend_data_type {
> > >  	IIO_BACKEND_TWOS_COMPLEMENT,
> > >  	IIO_BACKEND_OFFSET_BINARY,
> > > +	IIO_BACKEND_DATA_UNSIGNED,
> > >  	IIO_BACKEND_DATA_TYPE_MAX
> > >  };
> > >  
> > >  enum iio_backend_data_source {
> > >  	IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE,
> > >  	IIO_BACKEND_EXTERNAL,
> > > +	IIO_BACKEND_INTERNAL_RAMP_16BIT,
> > >  	IIO_BACKEND_DATA_SOURCE_MAX
> > >  };
> > >  
> > > @@ -89,6 +91,13 @@ enum iio_backend_sample_trigger {
> > >   * @read_raw: Read a channel attribute from a backend device
> > >   * @debugfs_print_chan_status: Print channel status into a buffer.
> > >   * @debugfs_reg_access: Read or write register value of backend.
> > > + * @ext_sync_enable: Enable external synchronization.
> > > + * @ext_sync_disable: Disable external synchronization.
> > > + * @ddr_enable: Enable interface DDR (Double Data Rate) mode.
> > > + * @ddr_disable: Disable interface DDR (Double Data Rate) mode.
> > > + * @buffer_enable: Enable data buffer.
> > > + * @buffer_disable: Disable data buffer.
> > 
> > This needs more specific text. What buffer?  I think this came
> > up earlier but it needs to say something about the fact it's enabling
> > or disabling the actual capture of data into the DMA buffers that
> > userspace will read.
> > 
> > > + * @data_transfer_addr: Set data address.
> > >   **/
> > >  struct iio_backend_ops {
> > >  	int (*enable)(struct iio_backend *back);
> > > @@ -129,6 +138,13 @@ struct iio_backend_ops {
> > >  					 size_t len);
> > >  	int (*debugfs_reg_access)(struct iio_backend *back, unsigned int
> > > reg,
> > >  				  unsigned int writeval, unsigned int
> > > *readval);
> > > +	int (*ext_sync_enable)(struct iio_backend *back);
> > I know we've done it this way for existing items, but I wonder if we should
> > squish down the ops slightly and have new enable/disable pairs as
> > single functions.
> > 	int (*ext_sync_set_state)(struct iio_backend *back, bool enable);
> > etc.  If nothing else reduces how many things need documentation ;)
> > 
> > Nuno, what do you think? Worth squashing these pairs into single
> > callbacks?
> 
> I'm not a fan of combining enable and disable functions into one function.
> 
> The implementation will pretty much always be:
> 
> if (enabled) {
>         /* so stuff */
> } else {
>         /* do other stuff */
> }
> 
> Which just adds indent and makes code harder to read.
> 

Hi Jonathan and David,

Yeah, I have this on my todo list and to be fair with Angelo, he already had
something like you're suggesting. I kind of asked him to postpone that so we
don't have mixed styles in the file for now. Then I would convert them all. My
plan would be to squash the .ops into one and then have inline
enable()/disable() helpers (at least for the current users in order to keep
things easier to convert).

As for David's comment, I see your point but one can always improve things a bit

if (enable) {
	/* do stuff */
	return;
}

/* do disable stuff */
return 0

I'm aware the above is always not that straight... but I do think there's always
ways to rearrange things a bit to make it better. Because even with the
enable()/disable() approach, if you start to have a lot of common code, likely
you'll add an helper function. In some cases, one can even add the helper right
away with an 'enable' argument effectively doing what is being suggested in
here. It always depends on the person implementing the ops :)

Anyways, I really don't have a strong feeling about this. I had in my mind to do
something like this. It feels that Jonathan would already be ok with it. If it's
not that awful for David, I'll eventually send the patches (unless Angelo wants
to take care if it in this series).

- Nuno Sá
> 

Powered by blists - more mailing lists