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?
