| Message ID | 20240923101206.3753-2-antoniu.miclaus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | *** Add support for AD485x DAS Family *** | expand |
On Mon, Sep 23, 2024 at 12:15 PM Antoniu Miclaus <antoniu.miclaus@analog.com> wrote: > > Add backend support for obtaining the interface type used. > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > --- > drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++ > include/linux/iio/backend.h | 10 ++++++++++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c > index efe05be284b6..53ab6bc86a50 100644 > --- a/drivers/iio/industrialio-backend.c > +++ b/drivers/iio/industrialio-backend.c > @@ -449,6 +449,30 @@ 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 interace 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_CMOS) > + return -EINVAL; > + > + return 0; > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); > + > /** > * iio_backend_extend_chan_spec - Extend an IIO channel > * @indio_dev: IIO device > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > index 8099759d7242..ba8ad30ac9ba 100644 > --- a/include/linux/iio/backend.h > +++ b/include/linux/iio/backend.h > @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger { > IIO_BACKEND_SAMPLE_TRIGGER_MAX > }; > > +enum iio_backend_interface_type { > + IIO_BACKEND_INTERFACE_LVDS, > + IIO_BACKEND_INTERFACE_CMOS > +}; > + > /** > * struct iio_backend_ops - operations structure for an iio_backend > * @enable: Enable backend. > @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger { > * @extend_chan_spec: Extend an IIO channel. > * @ext_info_set: Extended info setter. > * @ext_info_get: Extended info getter. > + * @interface_type_get: Interface type. > **/ > struct iio_backend_ops { > int (*enable)(struct iio_backend *back); > @@ -113,6 +119,8 @@ struct iio_backend_ops { > const char *buf, size_t len); > int (*ext_info_get)(struct iio_backend *back, uintptr_t private, > const struct iio_chan_spec *chan, char *buf); > + int (*interface_type_get)(struct iio_backend *back, > + enum iio_backend_interface_type *type); > }; > > int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); > @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private, > ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private, > const struct iio_chan_spec *chan, char *buf); > > +int iio_backend_interface_type_get(struct iio_backend *back, > + enum iio_backend_interface_type *type); > int iio_backend_extend_chan_spec(struct iio_dev *indio_dev, > struct iio_backend *back, > struct iio_chan_spec *chan); > -- > 2.46.0 > This seems very specific to the AD485x chips and the AXI ADC backend. Since it is describing how the chip is wired to the AXI DAC IP block, I would be tempted to use the devicetree for this info instead of adding a new backend function.
On Thu, 2024-09-26 at 10:40 +0200, David Lechner wrote: > On Mon, Sep 23, 2024 at 12:15 PM Antoniu Miclaus > <antoniu.miclaus@analog.com> wrote: > > > > Add backend support for obtaining the interface type used. > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > > --- > > drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++ > > include/linux/iio/backend.h | 10 ++++++++++ > > 2 files changed, 34 insertions(+) > > > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- > > backend.c > > index efe05be284b6..53ab6bc86a50 100644 > > --- a/drivers/iio/industrialio-backend.c > > +++ b/drivers/iio/industrialio-backend.c > > @@ -449,6 +449,30 @@ 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 interace 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_CMOS) > > + return -EINVAL; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); > > + > > /** > > * iio_backend_extend_chan_spec - Extend an IIO channel > > * @indio_dev: IIO device > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > > index 8099759d7242..ba8ad30ac9ba 100644 > > --- a/include/linux/iio/backend.h > > +++ b/include/linux/iio/backend.h > > @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger { > > IIO_BACKEND_SAMPLE_TRIGGER_MAX > > }; > > > > +enum iio_backend_interface_type { > > + IIO_BACKEND_INTERFACE_LVDS, > > + IIO_BACKEND_INTERFACE_CMOS > > +}; > > + > > /** > > * struct iio_backend_ops - operations structure for an iio_backend > > * @enable: Enable backend. > > @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger { > > * @extend_chan_spec: Extend an IIO channel. > > * @ext_info_set: Extended info setter. > > * @ext_info_get: Extended info getter. > > + * @interface_type_get: Interface type. > > **/ > > struct iio_backend_ops { > > int (*enable)(struct iio_backend *back); > > @@ -113,6 +119,8 @@ struct iio_backend_ops { > > const char *buf, size_t len); > > int (*ext_info_get)(struct iio_backend *back, uintptr_t private, > > const struct iio_chan_spec *chan, char *buf); > > + int (*interface_type_get)(struct iio_backend *back, > > + enum iio_backend_interface_type *type); > > }; > > > > int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); > > @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, > > uintptr_t private, > > ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private, > > const struct iio_chan_spec *chan, char *buf); > > > > +int iio_backend_interface_type_get(struct iio_backend *back, > > + enum iio_backend_interface_type *type); > > int iio_backend_extend_chan_spec(struct iio_dev *indio_dev, > > struct iio_backend *back, > > struct iio_chan_spec *chan); > > -- > > 2.46.0 > > > > This seems very specific to the AD485x chips and the AXI ADC backend. > Since it is describing how the chip is wired to the AXI DAC IP block, > I would be tempted to use the devicetree for this info instead of > adding a new backend function. Not sure If I'm following your point but I think this is the typical case where the chip (being it a DAC or ADC) supports both CMOS and LVDS interfaces. Naturally you only use one on your system and this is a synthesis parameter on the FPGA IP core. Therefore, it makes sense for the frontend to have way to ask for this information to the backend. That said, we could also have a DT parameter but, ideally, we would then need a way to match the parameter with the backend otherwise we could have DT stating LVDS and the backend built with CMOS. Other thing that we could think about is a new devm_iio_backend_get_with_info() where the frontend would get some constant and static info about the backend (the interface type would an ideal match for something like this). But I feel it's still early days for something like this :) - Nuno Sá
On Thu, 26 Sep 2024 12:52:39 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Thu, 2024-09-26 at 10:40 +0200, David Lechner wrote: > > On Mon, Sep 23, 2024 at 12:15 PM Antoniu Miclaus > > <antoniu.miclaus@analog.com> wrote: > > > > > > Add backend support for obtaining the interface type used. > > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > > > --- > > > drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++ > > > include/linux/iio/backend.h | 10 ++++++++++ > > > 2 files changed, 34 insertions(+) > > > > > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- > > > backend.c > > > index efe05be284b6..53ab6bc86a50 100644 > > > --- a/drivers/iio/industrialio-backend.c > > > +++ b/drivers/iio/industrialio-backend.c > > > @@ -449,6 +449,30 @@ 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 interace 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_CMOS) Put a COUNT entry or similar on the end of the enum so this doesn't need updating for more types. > > > + return -EINVAL; > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); > > > + > > > /** > > > * iio_backend_extend_chan_spec - Extend an IIO channel > > > * @indio_dev: IIO device > > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > > > index 8099759d7242..ba8ad30ac9ba 100644 > > > --- a/include/linux/iio/backend.h > > > +++ b/include/linux/iio/backend.h > > > @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger { > > > IIO_BACKEND_SAMPLE_TRIGGER_MAX > > > }; > > > > > > +enum iio_backend_interface_type { > > > + IIO_BACKEND_INTERFACE_LVDS, > > > + IIO_BACKEND_INTERFACE_CMOS trailing comma. This is going to get bigger! > > > +}; > > > + > > > /** > > > * struct iio_backend_ops - operations structure for an iio_backend > > > * @enable: Enable backend. > > > @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger { > > > * @extend_chan_spec: Extend an IIO channel. > > > * @ext_info_set: Extended info setter. > > > * @ext_info_get: Extended info getter. > > > + * @interface_type_get: Interface type. > > > **/ > > > struct iio_backend_ops { > > > int (*enable)(struct iio_backend *back); > > > @@ -113,6 +119,8 @@ struct iio_backend_ops { > > > const char *buf, size_t len); > > > int (*ext_info_get)(struct iio_backend *back, uintptr_t private, > > > const struct iio_chan_spec *chan, char *buf); > > > + int (*interface_type_get)(struct iio_backend *back, > > > + enum iio_backend_interface_type *type); > > > }; > > > > > > int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); > > > @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, > > > uintptr_t private, > > > ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private, > > > const struct iio_chan_spec *chan, char *buf); > > > > > > +int iio_backend_interface_type_get(struct iio_backend *back, > > > + enum iio_backend_interface_type *type); > > > int iio_backend_extend_chan_spec(struct iio_dev *indio_dev, > > > struct iio_backend *back, > > > struct iio_chan_spec *chan); > > > -- > > > 2.46.0 > > > > > > > This seems very specific to the AD485x chips and the AXI ADC backend. > > Since it is describing how the chip is wired to the AXI DAC IP block, > > I would be tempted to use the devicetree for this info instead of > > adding a new backend function. > > Not sure If I'm following your point but I think this is the typical case where the > chip (being it a DAC or ADC) supports both CMOS and LVDS interfaces. Naturally you > only use one on your system and this is a synthesis parameter on the FPGA IP core. > Therefore, it makes sense for the frontend to have way to ask for this information to > the backend. > > That said, we could also have a DT parameter but, ideally, we would then need a way > to match the parameter with the backend otherwise we could have DT stating LVDS and > the backend built with CMOS. That would be a DTS bug that you should fix :) For this to make sense you are relying on an FPGA that also has pins flexible enough to support LVDS and CMOS so it's only a firmware thing. Been a while since I last messed with FPGAs, but that seems unlikely to be true in general. So far I'm with David on this, feels like something we shouldn't be discovering at runtime though maybe that's a convenience that we do want to enable. > > Other thing that we could think about is a new devm_iio_backend_get_with_info() where > the frontend would get some constant and static info about the backend (the interface > type would an ideal match for something like this). But I feel it's still early days > for something like this :) > > - Nuno Sá
On Sat, 2024-09-28 at 18:23 +0100, Jonathan Cameron wrote: > On Thu, 26 Sep 2024 12:52:39 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Thu, 2024-09-26 at 10:40 +0200, David Lechner wrote: > > > On Mon, Sep 23, 2024 at 12:15 PM Antoniu Miclaus > > > <antoniu.miclaus@analog.com> wrote: > > > > > > > > Add backend support for obtaining the interface type used. > > > > > > > > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> > > > > --- > > > > drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++ > > > > include/linux/iio/backend.h | 10 ++++++++++ > > > > 2 files changed, 34 insertions(+) > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > > b/drivers/iio/industrialio- > > > > backend.c > > > > index efe05be284b6..53ab6bc86a50 100644 > > > > --- a/drivers/iio/industrialio-backend.c > > > > +++ b/drivers/iio/industrialio-backend.c > > > > @@ -449,6 +449,30 @@ 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 interace 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_CMOS) > Put a COUNT entry or similar on the end of the enum so this doesn't need > updating for more types. > > > > > + return -EINVAL; > > > > + > > > > + return 0; > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); > > > > + > > > > /** > > > > * iio_backend_extend_chan_spec - Extend an IIO channel > > > > * @indio_dev: IIO device > > > > diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h > > > > index 8099759d7242..ba8ad30ac9ba 100644 > > > > --- a/include/linux/iio/backend.h > > > > +++ b/include/linux/iio/backend.h > > > > @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger { > > > > IIO_BACKEND_SAMPLE_TRIGGER_MAX > > > > }; > > > > > > > > +enum iio_backend_interface_type { > > > > + IIO_BACKEND_INTERFACE_LVDS, > > > > + IIO_BACKEND_INTERFACE_CMOS > > trailing comma. > > This is going to get bigger! > > > > > +}; > > > > + > > > > /** > > > > * struct iio_backend_ops - operations structure for an iio_backend > > > > * @enable: Enable backend. > > > > @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger { > > > > * @extend_chan_spec: Extend an IIO channel. > > > > * @ext_info_set: Extended info setter. > > > > * @ext_info_get: Extended info getter. > > > > + * @interface_type_get: Interface type. > > > > **/ > > > > struct iio_backend_ops { > > > > int (*enable)(struct iio_backend *back); > > > > @@ -113,6 +119,8 @@ struct iio_backend_ops { > > > > const char *buf, size_t len); > > > > int (*ext_info_get)(struct iio_backend *back, uintptr_t private, > > > > const struct iio_chan_spec *chan, char > > > > *buf); > > > > + int (*interface_type_get)(struct iio_backend *back, > > > > + enum iio_backend_interface_type > > > > *type); > > > > }; > > > > > > > > int iio_backend_chan_enable(struct iio_backend *back, unsigned int > > > > chan); > > > > @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev > > > > *indio_dev, > > > > uintptr_t private, > > > > ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t > > > > private, > > > > const struct iio_chan_spec *chan, char > > > > *buf); > > > > > > > > +int iio_backend_interface_type_get(struct iio_backend *back, > > > > + enum iio_backend_interface_type > > > > *type); > > > > int iio_backend_extend_chan_spec(struct iio_dev *indio_dev, > > > > struct iio_backend *back, > > > > struct iio_chan_spec *chan); > > > > -- > > > > 2.46.0 > > > > > > > > > > This seems very specific to the AD485x chips and the AXI ADC backend. > > > Since it is describing how the chip is wired to the AXI DAC IP block, > > > I would be tempted to use the devicetree for this info instead of > > > adding a new backend function. > > > > Not sure If I'm following your point but I think this is the typical case > > where the > > chip (being it a DAC or ADC) supports both CMOS and LVDS interfaces. > > Naturally you > > only use one on your system and this is a synthesis parameter on the FPGA IP > > core. > > Therefore, it makes sense for the frontend to have way to ask for this > > information to > > the backend. > > > > That said, we could also have a DT parameter but, ideally, we would then > > need a way > > to match the parameter with the backend otherwise we could have DT stating > > LVDS and > > the backend built with CMOS. > > That would be a DTS bug that you should fix :) For this to make sense you are > relying on an FPGA that also has pins flexible enough to support LVDS and CMOS > so it's only a firmware thing. Been a while since I last messed with FPGAs, > but that seems unlikely to be true in general. > Sure, but if this is something the FPGA can give us as part of it's register map, it makes sense to me to have an interface like this... > So far I'm with David on this, feels like something we shouldn't be > discovering > at runtime though maybe that's a convenience that we do want to enable. > To be clear, I'm not against a DT parameter as it indeed describes how the HW is being used (even though we could get it done solely with the interface_get()) and while I agree with you that having a mismatch in interface types would be a DT bug, it's always better to be able to detect and catch it early on (and fail early) then going against the wall until we realize the issue. So, I do see value in an interface like this even if only to match and validate against a DT parameter. - Nuno Sá >
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c index efe05be284b6..53ab6bc86a50 100644 --- a/drivers/iio/industrialio-backend.c +++ b/drivers/iio/industrialio-backend.c @@ -449,6 +449,30 @@ 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 interace 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_CMOS) + return -EINVAL; + + return 0; +} +EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); + /** * iio_backend_extend_chan_spec - Extend an IIO channel * @indio_dev: IIO device diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h index 8099759d7242..ba8ad30ac9ba 100644 --- a/include/linux/iio/backend.h +++ b/include/linux/iio/backend.h @@ -63,6 +63,11 @@ enum iio_backend_sample_trigger { IIO_BACKEND_SAMPLE_TRIGGER_MAX }; +enum iio_backend_interface_type { + IIO_BACKEND_INTERFACE_LVDS, + IIO_BACKEND_INTERFACE_CMOS +}; + /** * struct iio_backend_ops - operations structure for an iio_backend * @enable: Enable backend. @@ -81,6 +86,7 @@ enum iio_backend_sample_trigger { * @extend_chan_spec: Extend an IIO channel. * @ext_info_set: Extended info setter. * @ext_info_get: Extended info getter. + * @interface_type_get: Interface type. **/ struct iio_backend_ops { int (*enable)(struct iio_backend *back); @@ -113,6 +119,8 @@ struct iio_backend_ops { const char *buf, size_t len); int (*ext_info_get)(struct iio_backend *back, uintptr_t private, const struct iio_chan_spec *chan, char *buf); + int (*interface_type_get)(struct iio_backend *back, + enum iio_backend_interface_type *type); }; int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); @@ -142,6 +150,8 @@ ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private, ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private, const struct iio_chan_spec *chan, char *buf); +int iio_backend_interface_type_get(struct iio_backend *back, + enum iio_backend_interface_type *type); int iio_backend_extend_chan_spec(struct iio_dev *indio_dev, struct iio_backend *back, struct iio_chan_spec *chan);
Add backend support for obtaining the interface type used. Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com> --- drivers/iio/industrialio-backend.c | 24 ++++++++++++++++++++++++ include/linux/iio/backend.h | 10 ++++++++++ 2 files changed, 34 insertions(+)