| Message ID | 20240829-wip-bl-ad3552r-axi-v0-v1-2-b6da6015327a@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: dac: introducing ad3552r-axi | expand |
On Thu, 29 Aug 2024 14:32:00 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Extend backend features with new calls needed later on this > patchset from axi version of ad3552r. > > A bus type property has been added to the devicetree to > inform the backend about the type of bus (interface) in use > bu the IP. > > 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. > iio_backend_bus_reg_read > generic bus read, bus-type dependent > iio_backend_bus_read_write > generic bus write, bus-type dependent > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > --- > drivers/iio/industrialio-backend.c | 151 +++++++++++++++++++++++++++++++++++++ > include/linux/iio/backend.h | 24 ++++++ > 2 files changed, 175 insertions(+) > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c > index a52a6b61c8b5..1f60c8626be7 100644 > --- a/drivers/iio/industrialio-backend.c > +++ b/drivers/iio/industrialio-backend.c > @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) > return 0; > } > + > +/** > + * iio_backend_buffer_enable - Enable data buffering Data buffering is a very vague term. Perhaps some more detail on what this means? > + * @back: Backend device > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_buffer_enable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, buffer_enable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > + > +/** > + * iio_backend_set_buffer_disable - Disable data buffering > + * @back: Backend device > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_buffer_disable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, buffer_disable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); > + > +/** > + * iio_backend_buffer_transfer_addr - Set data address. > + * @back: Backend device > + * @chan_address: Channel register address Run scripts/kernel-doc on this and fix the errors (parameter name is wrong). W=1 builds might also point the simpler ones out. > + * > + * Some devices may need to inform the backend about an address/location > + * where to read or write the data. I'd drop the 'location' part unless this gets used later because you are referring register address above. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address) > +{ > + return iio_backend_op_call(back, data_transfer_addr, address); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_data_transfer_addr, IIO_BACKEND); > + > +/** > + * iio_backend_bus_reg_read - Read from the interface bus > + * @back: Backend device > + * @reg: Register valule > + * @val: Pointer to register value > + * @size: Size, in bytes > + * > + * A backend may operate on a specific interface with a related bus. > + * Read from the interface bus. So this is effectively routing control plane data through the offloaded bus? That sounds a lot more like a conventional bus than IIO backend. Perhaps it should be presented as that with the IIO device attached to that bus? I don't fully understand what is wired up here. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_bus_reg_read(struct iio_backend *back, > + u32 reg, void *val, size_t size) > +{ > + if (!size) > + return -EINVAL; > + > + return iio_backend_op_call(back, bus_reg_read, reg, val, size); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_bus_reg_read, IIO_BACKEND); > +
Hi Jonathan, thanks for the feedbacks, On 31/08/24 1:23 PM, Jonathan Cameron wrote: > On Thu, 29 Aug 2024 14:32:00 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > >> From: Angelo Dureghello <adureghello@baylibre.com> >> >> Extend backend features with new calls needed later on this >> patchset from axi version of ad3552r. >> >> A bus type property has been added to the devicetree to >> inform the backend about the type of bus (interface) in use >> bu the IP. >> >> 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. >> iio_backend_bus_reg_read >> generic bus read, bus-type dependent >> iio_backend_bus_read_write >> generic bus write, bus-type dependent >> >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >> --- >> drivers/iio/industrialio-backend.c | 151 +++++++++++++++++++++++++++++++++++++ >> include/linux/iio/backend.h | 24 ++++++ >> 2 files changed, 175 insertions(+) >> >> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c >> index a52a6b61c8b5..1f60c8626be7 100644 >> --- a/drivers/iio/industrialio-backend.c >> +++ b/drivers/iio/industrialio-backend.c >> @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) >> return 0; >> } > >> + >> +/** >> + * iio_backend_buffer_enable - Enable data buffering > Data buffering is a very vague term. Perhaps some more detail on what > this means? for this DAC IP, it is the dma buffer where i write the samples, for other non-dac frontends may be something different, so i kept it generic. Not sure what a proper name may be, maybe "Enable optional data buffer" ? >> + * @back: Backend device >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_buffer_enable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, buffer_enable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); >> + >> +/** >> + * iio_backend_set_buffer_disable - Disable data buffering >> + * @back: Backend device >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_buffer_disable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, buffer_disable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); >> + >> +/** >> + * iio_backend_buffer_transfer_addr - Set data address. >> + * @back: Backend device >> + * @chan_address: Channel register address > Run scripts/kernel-doc on this and fix the errors (parameter name is > wrong). W=1 builds might also point the simpler ones out. ack, done >> + * >> + * Some devices may need to inform the backend about an address/location >> + * where to read or write the data. > I'd drop the 'location' part unless this gets used later because you > are referring register address above. ack, done >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address) >> +{ >> + return iio_backend_op_call(back, data_transfer_addr, address); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_data_transfer_addr, IIO_BACKEND); >> + >> +/** >> + * iio_backend_bus_reg_read - Read from the interface bus >> + * @back: Backend device >> + * @reg: Register valule >> + * @val: Pointer to register value >> + * @size: Size, in bytes >> + * >> + * A backend may operate on a specific interface with a related bus. >> + * Read from the interface bus. > So this is effectively routing control plane data through the offloaded > bus? That sounds a lot more like a conventional bus than IIO backend. > Perhaps it should be presented as that with the IIO device attached > to that bus? I don't fully understand what is wired up here. > Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar to QSPI as in my case (ad3552r). In particular, the bus is physically as a QSPI bus, but the data format over it is a bit different. So ad3552r needs this 5 lanes bus + double data rate to reach 33MUPS. https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_bus_reg_read(struct iio_backend *back, >> + u32 reg, void *val, size_t size) >> +{ >> + if (!size) >> + return -EINVAL; >> + >> + return iio_backend_op_call(back, bus_reg_read, reg, val, size); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_bus_reg_read, IIO_BACKEND); >> + Thanks a lot, regards, Angelo
On Mon, 2 Sep 2024 16:03:22 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > Hi Jonathan, > > thanks for the feedbacks, > > On 31/08/24 1:23 PM, Jonathan Cameron wrote: > > On Thu, 29 Aug 2024 14:32:00 +0200 > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > >> From: Angelo Dureghello <adureghello@baylibre.com> > >> > >> Extend backend features with new calls needed later on this > >> patchset from axi version of ad3552r. > >> > >> A bus type property has been added to the devicetree to > >> inform the backend about the type of bus (interface) in use > >> bu the IP. > >> > >> 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. > >> iio_backend_bus_reg_read > >> generic bus read, bus-type dependent > >> iio_backend_bus_read_write > >> generic bus write, bus-type dependent > >> > >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > >> --- > >> drivers/iio/industrialio-backend.c | 151 +++++++++++++++++++++++++++++++++++++ > >> include/linux/iio/backend.h | 24 ++++++ > >> 2 files changed, 175 insertions(+) > >> > >> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c > >> index a52a6b61c8b5..1f60c8626be7 100644 > >> --- a/drivers/iio/industrialio-backend.c > >> +++ b/drivers/iio/industrialio-backend.c > >> @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) > >> return 0; > >> } > > > >> + > >> +/** > >> + * iio_backend_buffer_enable - Enable data buffering > > Data buffering is a very vague term. Perhaps some more detail on what > > this means? > > for this DAC IP, it is the dma buffer where i write the samples, > for other non-dac frontends may be something different, so i kept it > generic. Not sure what a proper name may be, maybe > > "Enable optional data buffer" ? How do you 'enable' a buffer? Enable writing into it maybe? > > > >> + * @back: Backend device > >> + * > >> + * RETURNS: > >> + * 0 on success, negative error number on failure. > >> + */ > >> +int iio_backend_buffer_enable(struct iio_backend *back) > >> +{ > >> + return iio_backend_op_call(back, buffer_enable); > >> +} > >> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > >> + > >> +/** > > >> +/** > >> + * iio_backend_bus_reg_read - Read from the interface bus > >> + * @back: Backend device > >> + * @reg: Register valule > >> + * @val: Pointer to register value > >> + * @size: Size, in bytes > >> + * > >> + * A backend may operate on a specific interface with a related bus. > >> + * Read from the interface bus. > > So this is effectively routing control plane data through the offloaded > > bus? That sounds a lot more like a conventional bus than IIO backend. > > Perhaps it should be presented as that with the IIO device attached > > to that bus? I don't fully understand what is wired up here. > > > Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar > to QSPI as in my case (ad3552r). ok. If this is a bus used for both control and dataplane, then we should really be presenting it as a bus (+ offload) similar to do for spi + offload. > > In particular, the bus is physically as a QSPI bus, but the data format > over it is a bit different. > > So ad3552r needs this 5 lanes bus + double data rate to reach 33MUPS. > > https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html > Jonathan
Hi Jonathan, On 03/09/24 9:11 PM, Jonathan Cameron wrote: > On Mon, 2 Sep 2024 16:03:22 +0200 > Angelo Dureghello <adureghello@baylibre.com> wrote: > >> Hi Jonathan, >> >> thanks for the feedbacks, >> >> On 31/08/24 1:23 PM, Jonathan Cameron wrote: >>> On Thu, 29 Aug 2024 14:32:00 +0200 >>> Angelo Dureghello <adureghello@baylibre.com> wrote: >>> >>>> From: Angelo Dureghello <adureghello@baylibre.com> >>>> >>>> Extend backend features with new calls needed later on this >>>> patchset from axi version of ad3552r. >>>> >>>> A bus type property has been added to the devicetree to >>>> inform the backend about the type of bus (interface) in use >>>> bu the IP. >>>> >>>> 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. >>>> iio_backend_bus_reg_read >>>> generic bus read, bus-type dependent >>>> iio_backend_bus_read_write >>>> generic bus write, bus-type dependent >>>> >>>> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> >>>> --- >>>> drivers/iio/industrialio-backend.c | 151 +++++++++++++++++++++++++++++++++++++ >>>> include/linux/iio/backend.h | 24 ++++++ >>>> 2 files changed, 175 insertions(+) >>>> >>>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c >>>> index a52a6b61c8b5..1f60c8626be7 100644 >>>> --- a/drivers/iio/industrialio-backend.c >>>> +++ b/drivers/iio/industrialio-backend.c >>>> @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) >>>> return 0; >>>> } >>> >>>> + >>>> +/** >>>> + * iio_backend_buffer_enable - Enable data buffering >>> Data buffering is a very vague term. Perhaps some more detail on what >>> this means? >> for this DAC IP, it is the dma buffer where i write the samples, >> for other non-dac frontends may be something different, so i kept it >> generic. Not sure what a proper name may be, maybe >> >> "Enable optional data buffer" ? > How do you 'enable' a buffer? Enable writing into it maybe? for the current case, this is done using the custom register of the AXI IP, enabling a "stream". return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, AXI_DAC_STREAM_ENABLE); Functionally, looks like dma data is processed (sent over qspi) when the stream is enabled. Maybe a name as "stream_enable" would me more appropriate ? "Stream" seems less generic btw. >> >>>> + * @back: Backend device >>>> + * >>>> + * RETURNS: >>>> + * 0 on success, negative error number on failure. >>>> + */ >>>> +int iio_backend_buffer_enable(struct iio_backend *back) >>>> +{ >>>> + return iio_backend_op_call(back, buffer_enable); >>>> +} >>>> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); >>>> + >>>> +/** >>>> +/** >>>> + * iio_backend_bus_reg_read - Read from the interface bus >>>> + * @back: Backend device >>>> + * @reg: Register valule >>>> + * @val: Pointer to register value >>>> + * @size: Size, in bytes >>>> + * >>>> + * A backend may operate on a specific interface with a related bus. >>>> + * Read from the interface bus. >>> So this is effectively routing control plane data through the offloaded >>> bus? That sounds a lot more like a conventional bus than IIO backend. >>> Perhaps it should be presented as that with the IIO device attached >>> to that bus? I don't fully understand what is wired up here. >>> >> Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar >> to QSPI as in my case (ad3552r). > ok. > > If this is a bus used for both control and dataplane, then we should really > be presenting it as a bus (+ offload) similar to do for spi + offload. > >> In particular, the bus is physically as a QSPI bus, but the data format >> over it is a bit different. >> >> So ad3552r needs this 5 lanes bus + double data rate to reach 33MUPS. >> >> https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html >> > Jonathan >
On Wed, 2024-09-04 at 14:01 +0200, Angelo Dureghello wrote: > Hi Jonathan, > > On 03/09/24 9:11 PM, Jonathan Cameron wrote: > > On Mon, 2 Sep 2024 16:03:22 +0200 > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > Hi Jonathan, > > > > > > thanks for the feedbacks, > > > > > > On 31/08/24 1:23 PM, Jonathan Cameron wrote: > > > > On Thu, 29 Aug 2024 14:32:00 +0200 > > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > Extend backend features with new calls needed later on this > > > > > patchset from axi version of ad3552r. > > > > > > > > > > A bus type property has been added to the devicetree to > > > > > inform the backend about the type of bus (interface) in use > > > > > bu the IP. > > > > > > > > > > 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. > > > > > iio_backend_bus_reg_read > > > > > generic bus read, bus-type dependent > > > > > iio_backend_bus_read_write > > > > > generic bus write, bus-type dependent > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > --- > > > > > drivers/iio/industrialio-backend.c | 151 > > > > > +++++++++++++++++++++++++++++++++++++ > > > > > include/linux/iio/backend.h | 24 ++++++ > > > > > 2 files changed, 175 insertions(+) > > > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > > > b/drivers/iio/industrialio-backend.c > > > > > index a52a6b61c8b5..1f60c8626be7 100644 > > > > > --- a/drivers/iio/industrialio-backend.c > > > > > +++ b/drivers/iio/industrialio-backend.c > > > > > @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device > > > > > *dev, struct iio_backend *back) > > > > > return 0; > > > > > } > > > > > > > > > + > > > > > +/** > > > > > + * iio_backend_buffer_enable - Enable data buffering > > > > Data buffering is a very vague term. Perhaps some more detail on what > > > > this means? > > > for this DAC IP, it is the dma buffer where i write the samples, > > > for other non-dac frontends may be something different, so i kept it > > > generic. Not sure what a proper name may be, maybe > > > > > > "Enable optional data buffer" ? > > How do you 'enable' a buffer? Enable writing into it maybe? > > for the current case, this is done using the custom register > of the AXI IP, enabling a "stream". > > return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, > AXI_DAC_STREAM_ENABLE); > > Functionally, looks like dma data is processed (sent over qspi) > when the stream is enabled. > > Maybe a name as "stream_enable" would me more appropriate ? > "Stream" seems less generic btw. > Yes, stream enable is very specific for this usecase. This is basically connected to typical IIO buffering. So maybe we could either: 1) Embed struct iio_buffer_setup_ops in the backend ops struct; 2) Or just define directly the ones we need now in backend ops. > > > > > > > > + * @back: Backend device > > > > > + * > > > > > + * RETURNS: > > > > > + * 0 on success, negative error number on failure. > > > > > + */ > > > > > +int iio_backend_buffer_enable(struct iio_backend *back) > > > > > +{ > > > > > + return iio_backend_op_call(back, buffer_enable); > > > > > +} > > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > > > > > + > > > > > +/** > > > > > +/** > > > > > + * iio_backend_bus_reg_read - Read from the interface bus > > > > > + * @back: Backend device > > > > > + * @reg: Register valule > > > > > + * @val: Pointer to register value > > > > > + * @size: Size, in bytes > > > > > + * > > > > > + * A backend may operate on a specific interface with a related bus. > > > > > + * Read from the interface bus. > > > > So this is effectively routing control plane data through the offloaded > > > > bus? That sounds a lot more like a conventional bus than IIO backend. > > > > Perhaps it should be presented as that with the IIO device attached > > > > to that bus? I don't fully understand what is wired up here. > > > > > > > Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar > > > to QSPI as in my case (ad3552r). > > ok. > > > > If this is a bus used for both control and dataplane, then we should really > > be presenting it as a bus (+ offload) similar to do for spi + offload. > > Yes, indeed. In this case we also use the axi-dac core for controlling the frontend device (accessing it's register) which is fairly weird. But not sure how we can do it differently. For the spi_engine that is really a spi controller with the extra offloading capability. For this one, it's now "acting" as a spi controller but in the future it may also "act" as a parallel controller (the axi-adc already is in works for that with the ad7606 series). I was also very skeptical when I first saw these new functions but I'm not really sure how to do it differently. I mean, it also does not make much sense to have an additional bus driver as the register maps are the same. Not sure if turning it in a MFD device, helps... FWIW, I still don't fully understand why can't we have this supported by the spi_engine core. My guess is that we need features from the axi-dac (for the dataplane) so we are incorporating the controlplane on it instead of going spi_engine + axi-dac. Also want to leave a quick note about LVDS (that was mentioned). That interface is typically only used for data so I'm not seeing any special handling like this for that interface. - Nuno Sá > >
On Thu, 05 Sep 2024 12:28:51 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2024-09-04 at 14:01 +0200, Angelo Dureghello wrote: > > Hi Jonathan, > > > > On 03/09/24 9:11 PM, Jonathan Cameron wrote: > > > On Mon, 2 Sep 2024 16:03:22 +0200 > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > Hi Jonathan, > > > > > > > > thanks for the feedbacks, > > > > > > > > On 31/08/24 1:23 PM, Jonathan Cameron wrote: > > > > > On Thu, 29 Aug 2024 14:32:00 +0200 > > > > > Angelo Dureghello <adureghello@baylibre.com> wrote: > > > > > > > > > > > From: Angelo Dureghello <adureghello@baylibre.com> > > > > > > > > > > > > Extend backend features with new calls needed later on this > > > > > > patchset from axi version of ad3552r. > > > > > > > > > > > > A bus type property has been added to the devicetree to > > > > > > inform the backend about the type of bus (interface) in use > > > > > > bu the IP. > > > > > > > > > > > > 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. > > > > > > iio_backend_bus_reg_read > > > > > > generic bus read, bus-type dependent > > > > > > iio_backend_bus_read_write > > > > > > generic bus write, bus-type dependent > > > > > > > > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > > > > > > --- > > > > > > drivers/iio/industrialio-backend.c | 151 > > > > > > +++++++++++++++++++++++++++++++++++++ > > > > > > include/linux/iio/backend.h | 24 ++++++ > > > > > > 2 files changed, 175 insertions(+) > > > > > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > > > > b/drivers/iio/industrialio-backend.c > > > > > > index a52a6b61c8b5..1f60c8626be7 100644 > > > > > > --- a/drivers/iio/industrialio-backend.c > > > > > > +++ b/drivers/iio/industrialio-backend.c > > > > > > @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device > > > > > > *dev, struct iio_backend *back) > > > > > > return 0; > > > > > > } > > > > > > > > > > > + > > > > > > +/** > > > > > > + * iio_backend_buffer_enable - Enable data buffering > > > > > Data buffering is a very vague term. Perhaps some more detail on what > > > > > this means? > > > > for this DAC IP, it is the dma buffer where i write the samples, > > > > for other non-dac frontends may be something different, so i kept it > > > > generic. Not sure what a proper name may be, maybe > > > > > > > > "Enable optional data buffer" ? > > > How do you 'enable' a buffer? Enable writing into it maybe? > > > > for the current case, this is done using the custom register > > of the AXI IP, enabling a "stream". > > > > return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL, > > AXI_DAC_STREAM_ENABLE); > > > > Functionally, looks like dma data is processed (sent over qspi) > > when the stream is enabled. > > > > Maybe a name as "stream_enable" would me more appropriate ? > > "Stream" seems less generic btw. > > Ok. Maybe "enable buffer filling" or something like that? > > Yes, stream enable is very specific for this usecase. This is basically > connected to typical IIO buffering. So maybe we could either: > > 1) Embed struct iio_buffer_setup_ops in the backend ops struct; > 2) Or just define directly the ones we need now in backend ops. Structurally whatever makes sense - I was just quibbling over the documentation ;) > > > > > > > > > > > + * @back: Backend device > > > > > > + * > > > > > > + * RETURNS: > > > > > > + * 0 on success, negative error number on failure. > > > > > > + */ > > > > > > +int iio_backend_buffer_enable(struct iio_backend *back) > > > > > > +{ > > > > > > + return iio_backend_op_call(back, buffer_enable); > > > > > > +} > > > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > > > > > > + > > > > > > +/** > > > > > > +/** > > > > > > + * iio_backend_bus_reg_read - Read from the interface bus > > > > > > + * @back: Backend device > > > > > > + * @reg: Register valule > > > > > > + * @val: Pointer to register value > > > > > > + * @size: Size, in bytes > > > > > > + * > > > > > > + * A backend may operate on a specific interface with a related bus. > > > > > > + * Read from the interface bus. > > > > > So this is effectively routing control plane data through the offloaded > > > > > bus? That sounds a lot more like a conventional bus than IIO backend. > > > > > Perhaps it should be presented as that with the IIO device attached > > > > > to that bus? I don't fully understand what is wired up here. > > > > > > > > > Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar > > > > to QSPI as in my case (ad3552r). > > > ok. > > > > > > If this is a bus used for both control and dataplane, then we should really > > > be presenting it as a bus (+ offload) similar to do for spi + offload. > > > > > Yes, indeed. In this case we also use the axi-dac core for controlling the > frontend device (accessing it's register) which is fairly weird. But not sure > how we can do it differently. For the spi_engine that is really a spi controller > with the extra offloading capability. For this one, it's now "acting" as a spi > controller but in the future it may also "act" as a parallel controller (the > axi-adc already is in works for that with the ad7606 series). > > I was also very skeptical when I first saw these new functions but I'm not > really sure how to do it differently. I mean, it also does not make much sense > to have an additional bus driver as the register maps are the same. Not sure if > turning it in a MFD device, helps... Hmm. A given adi-axi-adc interface is going to be one of (or something else) 1) SPI(ish) controller + offloads like this one. 2) Parallel bus - data only 3) Parallel bus with control. Maybe we argue these are tightly coupled enough that we don't care but it feels like a direction that might bite us in the long run, particularly if we end up dt bindings that are hard to work with if we change how this fits together - imagine an SPI engine with a mode that does work for this + an SPI offload engine that works with that. Then the binding here will be hard to deal with. > > FWIW, I still don't fully understand why can't we have this supported by the > spi_engine core. My guess is that we need features from the axi-dac (for the > dataplane) so we are incorporating the controlplane on it instead of going > spi_engine + axi-dac. > > Also want to leave a quick note about LVDS (that was mentioned). That interface > is typically only used for data so I'm not seeing any special handling like this > for that interface. Makes sense. I'm a bit surprised that the parallel bus being used for control is on the list given that is also a bit messy to do (need some signalling that direction is changing or a lot of wires). Jonathan > > - Nuno Sá > > >
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c index a52a6b61c8b5..1f60c8626be7 100644 --- a/drivers/iio/industrialio-backend.c +++ b/drivers/iio/industrialio-backend.c @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back) return 0; } +/** + * iio_backend_ext_sync_enable - Enable external synchronization + * @back: Backend device + * + * Enable synchronization by external signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ext_sync_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ext_sync_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); + +/** + * iio_backend_ext_sync_disable - Disable external synchronization + * @back: Backend device + * + * Disable synchronization by external signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ext_sync_disable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ext_sync_disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); + +/** + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode + * @back: Backend device + * + * Enabling DDR, data is generated by the IP at each front + * (raising and falling) of the bus clock signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ddr_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ddr_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); + +/** + * 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 + * 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); + +/** + * iio_backend_buffer_enable - Enable data buffering + * @back: Backend device + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_buffer_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, buffer_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); + +/** + * iio_backend_set_buffer_disable - Disable data buffering + * @back: Backend device + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_buffer_disable(struct iio_backend *back) +{ + return iio_backend_op_call(back, buffer_disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); + +/** + * iio_backend_buffer_transfer_addr - Set data address. + * @back: Backend device + * @chan_address: Channel register address + * + * Some devices may need to inform the backend about an address/location + * where to read or write the data. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address) +{ + return iio_backend_op_call(back, data_transfer_addr, address); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_data_transfer_addr, IIO_BACKEND); + +/** + * iio_backend_bus_reg_read - Read from the interface bus + * @back: Backend device + * @reg: Register valule + * @val: Pointer to register value + * @size: Size, in bytes + * + * A backend may operate on a specific interface with a related bus. + * Read from the interface bus. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_bus_reg_read(struct iio_backend *back, + u32 reg, void *val, size_t size) +{ + if (!size) + return -EINVAL; + + return iio_backend_op_call(back, bus_reg_read, reg, val, size); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_bus_reg_read, IIO_BACKEND); + +/** + * iio_backend_bus_reg_write - Write on the interface bus + * @back: Backend device + * @reg: Register value + * @val: Register Value + * @size: Size in bytes + * + * A backend may operate on a specific interface with a related bus. + * Write to the interface bus. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_bus_reg_write(struct iio_backend *back, + u32 reg, void *val, size_t size) +{ + if (!size) + return -EINVAL; + + return iio_backend_op_call(back, bus_reg_write, reg, val, size); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_bus_reg_write, IIO_BACKEND); + static struct iio_backend *__devm_iio_backend_fwnode_get(struct device *dev, const char *name, struct fwnode_handle *fwnode) { diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h index 37d56914d485..6f56bbb9e391 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_16, IIO_BACKEND_DATA_SOURCE_MAX }; @@ -129,6 +131,17 @@ 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); + int (*ext_sync_disable)(struct iio_backend *back); + int (*ddr_enable)(struct iio_backend *back); + int (*ddr_disable)(struct iio_backend *back); + int (*buffer_enable)(struct iio_backend *back); + int (*buffer_disable)(struct iio_backend *back); + int (*data_transfer_addr)(struct iio_backend *back, u32 address); + int (*bus_reg_read)(struct iio_backend *back, u32 reg, void *val, + size_t size); + int (*bus_reg_write)(struct iio_backend *back, u32 reg, void *val, + size_t size); }; /** @@ -164,6 +177,17 @@ int iio_backend_data_sample_trigger(struct iio_backend *back, int devm_iio_backend_request_buffer(struct device *dev, struct iio_backend *back, struct iio_dev *indio_dev); +int iio_backend_ext_sync_enable(struct iio_backend *back); +int iio_backend_ext_sync_disable(struct iio_backend *back); +int iio_backend_ddr_enable(struct iio_backend *back); +int iio_backend_ddr_disable(struct iio_backend *back); +int iio_backend_buffer_enable(struct iio_backend *back); +int iio_backend_buffer_disable(struct iio_backend *back); +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address); +int iio_backend_bus_reg_read(struct iio_backend *back, + u32 reg, void *val, size_t size); +int iio_backend_bus_reg_write(struct iio_backend *back, + u32 reg, void *val, size_t size); ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private, const struct iio_chan_spec *chan, const char *buf, size_t len);