On 19.12.2024 11:01, David Lechner wrote: > On 12/19/24 10:51 AM, Jonathan Cameron wrote: > > On Thu, 19 Dec 2024 16:42:33 +0000 > > Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > >> On Tue, 17 Dec 2024 11:13:59 +0100 > >> Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > >> > >>> On Mon, 2024-12-16 at 21:36 +0100, Angelo Dureghello wrote: > >>>> From: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> > >>>> > >>>> Add backend support for setting and getting the interface type > >>>> in use. > >>>> > >>>> Link: > >>>> https://lore.kernel.org/linux-iio/20241129153546.63584-1-antoniu.miclaus@xxxxxxxxxx/T/#m6d86939078d780512824f1540145aade38b0990b > >>>> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> > >>>> Co-developed-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > >>>> Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > >>>> --- > >>>> This patch has been picked up from the Antoniu patchset > >>>> still not accepted, and extended with the interface setter, > >>>> fixing also namespace names to be between quotation marks. > >>>> --- > >>>> drivers/iio/industrialio-backend.c | 42 > >>>> ++++++++++++++++++++++++++++++++++++++ > >>>> include/linux/iio/backend.h | 19 +++++++++++++++++ > >>>> 2 files changed, 61 insertions(+) > >>>> > >>>> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- > >>>> backend.c > >>>> index 363281272035..6edc3e685f6a 100644 > >>>> --- a/drivers/iio/industrialio-backend.c > >>>> +++ b/drivers/iio/industrialio-backend.c > >>>> @@ -636,6 +636,48 @@ ssize_t iio_backend_ext_info_set(struct iio_dev > >>>> *indio_dev, uintptr_t private, > >>>> } > >>>> EXPORT_SYMBOL_NS_GPL(iio_backend_ext_info_set, "IIO_BACKEND"); > >>>> > >>>> +/** > >>>> + * iio_backend_interface_type_get - get the interface type used. > >>>> + * @back: Backend device > >>>> + * @type: Interface type > >>>> + * > >>>> + * RETURNS: > >>>> + * 0 on success, negative error number on failure. > >>>> + */ > >>>> +int iio_backend_interface_type_get(struct iio_backend *back, > >>>> + enum iio_backend_interface_type *type) > >>>> +{ > >>>> + int ret; > >>>> + > >>>> + ret = iio_backend_op_call(back, interface_type_get, type); > >>>> + if (ret) > >>>> + return ret; > >>>> + > >>>> + if (*type >= IIO_BACKEND_INTERFACE_MAX) > >>>> + return -EINVAL; > >>>> + > >>>> + return 0; > >>>> +} > >>>> +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, "IIO_BACKEND"); > >>>> + > >>>> +/** > >>>> + * iio_backend_interface_type_set - set the interface type used. > >>>> + * @back: Backend device > >>>> + * @type: Interface type > >>>> + * > >>>> + * RETURNS: > >>>> + * 0 on success, negative error number on failure. > >>>> + */ > >>>> +int iio_backend_interface_type_set(struct iio_backend *back, > >>>> + enum iio_backend_interface_type type) > >>>> +{ > >>>> + if (type >= IIO_BACKEND_INTERFACE_MAX) > >>>> + return -EINVAL; > >>>> + > >>>> + return iio_backend_op_call(back, interface_type_set, type); > >>>> +} > >>>> +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_set, "IIO_BACKEND"); > >>>> + > >>>> /** > >>>> * iio_backend_extend_chan_spec - Extend an IIO channel > >>>> * @back: Backend device > >>>> diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > >>>> index 10be00f3b120..2b7221099d8c 100644 > >>>> --- a/include/linux/iio/backend.h > >>>> +++ b/include/linux/iio/backend.h > >>>> @@ -70,6 +70,15 @@ enum iio_backend_sample_trigger { > >>>> IIO_BACKEND_SAMPLE_TRIGGER_MAX > >>>> }; > >>>> > >>>> +enum iio_backend_interface_type { > >>>> + IIO_BACKEND_INTERFACE_SERIAL_LVDS, > >>>> + IIO_BACKEND_INTERFACE_SERIAL_CMOS, > >>> > >>> The above are apparently not used in the next patch so I would not add them now. > >>>> + IIO_BACKEND_INTERFACE_SERIAL_SPI, > >>>> + IIO_BACKEND_INTERFACE_SERIAL_DSPI, > >>>> + IIO_BACKEND_INTERFACE_SERIAL_QSPI, > >>> > >>> I'll throw my 2 cents but it would be nice to have more feedback on this. I'm > >>> not completely sure about the xSPI stuff in here. We treated the QSPI as a bus > >>> both for control and data in which we also add child devices. And we've been > >>> adding specific bus operations/configurations through the 'struct > >>> ad3552r_hs_platform_data' interface. So, I'm wondering if this should also not > >>> be set through that interface? > >> > >> Maybe - kind of hard to tell until we actually have code. > >> I'd go for kicking them into the long grass for now if they aren't used and > >> just dropping them from this patch. If we ever need them,easy to bring back > >> and then we should have a justification for why! > > > > oops. Misread. Obviously Nuno was saying the ones above aren't used, not the > > SPI ones... I don't feel strongly either way on setting these via > > this generic interface, or via the other path. > > > > Jonathan > > > >> > >> J > >> > >> > >>> > >>> LVDS/CMOS still looks slightly different to me... > >>> > >>> - Nuno Sá > >>> > >>> > >>> > >> > >> > > > > I'm the one that suggested doing it this way to Angelo, so I'll chime in > with my thinking. In Angelo's previous series where we added IIO backend > support for this family of chips, in the devicetree discussion, we > considered the possibility of two SPI controllers, one for register > access and one for the high-speed streaming provided by the backend. > Since the dual and quad SPI here is for IIO backend (the high-speed sample > data interface) is what made me think we should put the control here rather > than putting it in the platform data interface. > > Since this is new HDL, maybe we could avoid this issue altogether and > tweak the implementation in the FPGA a bit. Clearly we had the AD3552R > working without needing to poke this registers, so why can't we do the > same for the new chip? In other words, make these things a compile time > option in the HDL rather than a runtime option? The purpose of the new HDL is to support all the devices of the family, (DSPI/QSPI) just acting on axi MULTI_IO_MODE. Thinking to this a bit better, changing MULTI_IO_MODE is not exactly changing "interface" type so maybe is more appropriarte to use a patform data "bus_mode" function. New ad354x chips do not have QSPI pin, so no chance to access the whole primary + secondary area in DSPI without changes in configuration registers, as done before with ad355xr. Regards, angelo
