On Thu, 28 Mar 2024 14:22:32 +0100
Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@xxxxxxxxxx> wrote:
> From: Nuno Sa <nuno.sa@xxxxxxxxxx>
>
> This adds the needed backend ops for supporting a backend inerfacing
> with an high speed dac. The new ops are:
>
> * data_source_set();
> * set_sampling_freq();
> * extend_chan_spec();
> * ext_info_set();
> * ext_info_get().
>
> Also to note the new helpers that are meant to be used by the backends
> when extending an IIO channel (adding extended info):
>
> * iio_backend_ext_info_set();
> * iio_backend_ext_info_get().
>
> Signed-off-by: Nuno Sa <nuno.sa@xxxxxxxxxx>
I'm pretty flexible on this so far as I think we are still learning how front
ends and backends should interact. Maybe we'll figure that out in the medium
term and rework this stuff. For now it looks fine. A few minor things inline.
>
> +/**
> + * iio_backend_ext_info_get - IIO ext_info read callback
> + * @indio_dev: IIO device
> + * @private: Data private to the driver
> + * @chan: IIO channel
> + * @buf: Buffer where to place the attribute data
> + *
> + * This helper is intended to be used by backends that extend an IIO channel
> + * (trough iio_backend_extend_chan_spec()) with extended info. In that case,
> + * backends are not supposed to give their own callbacks (as they would not
> + * a way to get te backend from indio_dev). This is the getter.
te->the?
> +/**
> + * iio_backend_extend_chan_spec - Extend an IIO channel
> + * @indio_dev: IIO device
> + * @back: Backend device
> + * @chan: IIO channel
> + *
> + * Some backends may have their own functionalities and hence capable of
> + * extending a frontend's channel.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
> + struct iio_backend *back,
> + struct iio_chan_spec *chan)
> +{
> + const struct iio_chan_spec_ext_info *ext_info = chan->ext_info;
This is getting confusing. So this one is the front end value of ext_info?
Name it as such frontend_ext_info
> + int ret;
> +
> + ret = iio_backend_op_call(back, extend_chan_spec, chan);
> + if (ret)
> + return ret;
> + /*
> + * Let's keep things simple for now. Don't allow to overwrite the
> + * frontend's extended info. If ever needed, we can support appending
> + * it.
> + */
> + if (ext_info && chan->ext_info != ext_info)
> + return -EOPNOTSUPP;
> + if (!chan->ext_info)
This is checking if the backend added anything? Perhaps a comment on that
as we don't need a backend_ext_info local variable...
> + return 0;
> + /*
> + * !\NOTE: this will break as soon as we have multiple backends on one
> + * frontend and all of them extend channels. In that case, the core
> + * backend code has no way to get the correct backend given the
> + * iio device.
> + *
> + * One solution for this could be introducing a new backend
> + * dedicated callback in struct iio_info so we can callback into the
> + * frontend so it can give us the right backend given a chan_spec.
> + */
Hmm. This is indeed messy. Could we associate it with the buffer as presuably
a front end with multiple backends is using multiple IIO buffers?
As you say a dance via the front end would work fine.
> + iio_device_set_drvdata(indio_dev, back);
> +
> + /* Don't allow backends to get creative and force their own handlers */
> + for (ext_info = chan->ext_info; ext_info->name; ext_info++) {
> + if (ext_info->read != iio_backend_ext_info_get)
> + return -EINVAL;
> + if (ext_info->write != iio_backend_ext_info_set)
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_extend_chan_spec, IIO_BACKEND);
> diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> index a6d79381866e..09ff2f8f9fd8 100644
> --- a/include/linux/iio/backend.h
> +++ b/include/linux/iio/backend.h
> @@ -4,6 +4,7 @@
>
> #include <linux/types.h>
>
> +struct iio_chan_spec;
> struct fwnode_handle;
> struct iio_backend;
> struct device;
> @@ -15,6 +16,26 @@ enum iio_backend_data_type {
> IIO_BACKEND_DATA_TYPE_MAX
> };
>
> +enum iio_backend_data_source {
> + IIO_BACKEND_INTERNAL_CW,
CW? Either expand out what ever that is in definition of add a comment
at least.
> + IIO_BACKEND_EXTERNAL,
What does external mean in this case?
> + IIO_BACKEND_DATA_SOURCE_MAX
> +};
> +
> +/**
> + * IIO_BACKEND_EX_INFO - Helper for an IIO extended channel attribute
> + * @_name: Attribute name
> + * @_shared: Whether the attribute is shared between all channels
> + * @_what: Data private to the driver
> + */
> +#define IIO_BACKEND_EX_INFO(_name, _shared, _what) { \
> + .name = (_name), \
> + .shared = (_shared), \
> + .read = iio_backend_ext_info_get, \
> + .write = iio_backend_ext_info_set, \
> + .private = (_what), \
> +}
> +
> /**
> * struct iio_backend_data_fmt - Backend data format
> * @type: Data type.
> @@ -35,8 +56,13 @@ struct iio_backend_data_fmt {
> * @chan_enable: Enable one channel.
> * @chan_disable: Disable one channel.
> * @data_format_set: Configure the data format for a specific channel.
> + * @data_source_set: Configure the data source for a specific channel.
> + * @set_sample_rate: Configure the sampling rate for a specific channel.
> * @request_buffer: Request an IIO buffer.
> * @free_buffer: Free an IIO buffer.
> + * @extend_chan_spec: Extend an IIO channel.
> + * @ext_info_set: Extended info setter.
> + * @ext_info_get: Extended info getter.
> **/
> struct iio_backend_ops {
> int (*enable)(struct iio_backend *back);
> @@ -45,10 +71,21 @@ struct iio_backend_ops {
> int (*chan_disable)(struct iio_backend *back, unsigned int chan);
> int (*data_format_set)(struct iio_backend *back, unsigned int chan,
> const struct iio_backend_data_fmt *data);
> + int (*data_source_set)(struct iio_backend *back, unsigned int chan,
> + enum iio_backend_data_source data);
> + int (*set_sample_rate)(struct iio_backend *back, unsigned int chan,
> + u64 sample_rate);
Name the parameter that so we know the units. _hz?
