[PATCH 0/7] *** Add support for AD485x DAS Family ***

[PATCH 0/7] *** Add support for AD485x DAS Family ***

[Antoniu Miclaus]

Add support for AD485X fully buffered, 8-channel simultaneous sampling,
16/20-bit, 1 MSPS data acquisition system (DAS) with differential, wide
common-mode range inputs.

Some particularities:
1. softspan - the devices support multiple softspans which are represented in iio
              through offset/scale. The current handling implies changing both
              the scale and the offset separately via IIO, therefore in order to
              properly set the softspan, each time the offset changes the softspan
              is set to the default value. And only after changing also the scale
              the desired softspan is set. This is the approach we are suggesting
              since we need the softspan configurable from userspace and not from
              devicetree.

2. packet format - Data provided on the CMOS and LVDS conversion data output buses
                   are packaged into eight channel packets. This is currently handled
                   as extended info.

Antoniu Miclaus (7):
  iio: backend: add API for interface get
  iio: backend: add support for data size set
  iio: adc: adi-axi-adc: add interface type
  iio: adc: adi-axi-adc: set data format
  dt-bindings: iio: adc: add ad458x
  iio: adc: ad485x: add ad485x driver
  Documentation: ABI: testing: ad485x: add ABI docs

 .../ABI/testing/sysfs-bus-iio-adc-ad485x      |   14 +
 .../bindings/iio/adc/adi,ad485x.yaml          |   82 ++
 drivers/iio/adc/Kconfig                       |   12 +
 drivers/iio/adc/Makefile                      |    1 +
 drivers/iio/adc/ad485x.c                      | 1061 +++++++++++++++++
 drivers/iio/adc/adi-axi-adc.c                 |   44 +
 drivers/iio/industrialio-backend.c            |   45 +
 include/linux/iio/backend.h                   |   13 +
 8 files changed, 1272 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
 create mode 100644 drivers/iio/adc/ad485x.c

-- 
2.46.0

[PATCH 1/7] iio: backend: add API for interface get

[Antoniu Miclaus]

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

Re: [PATCH 1/7] iio: backend: add API for interface get

[David Lechner]

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.

Re: [PATCH 1/7] iio: backend: add API for interface get

[Nuno Sá]

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á

Re: [PATCH 1/7] iio: backend: add API for interface get

[Jonathan Cameron]

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á

Re: [PATCH 1/7] iio: backend: add API for interface get

[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á
> 

[PATCH 2/7] iio: backend: add support for data size set

[Antoniu Miclaus]

Add backend support for setting the data size used.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/industrialio-backend.c | 21 +++++++++++++++++++++
 include/linux/iio/backend.h        |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
index 53ab6bc86a50..a6a6bedce7f1 100644
--- a/drivers/iio/industrialio-backend.c
+++ b/drivers/iio/industrialio-backend.c
@@ -473,6 +473,27 @@ int iio_backend_interface_type_get(struct iio_backend *back,
 }
 EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
 
+/**
+ * iio_backend_data_size_set - set the data width/size in the data bus.
+ * @back: Backend device
+ * @size: Size in bits
+ *
+ * Some frontend devices can dynamically control the word/data size on the
+ * interface/data bus. Hence, the backend device needs to be aware of it so
+ * data can be correctly transferred.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+ssize_t iio_backend_data_size_set(struct iio_backend *back, ssize_t size)
+{
+	if (!size)
+		return -EINVAL;
+
+	return iio_backend_op_call(back, data_size_set, size);
+}
+EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, 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 ba8ad30ac9ba..85b33ed69cc0 100644
--- a/include/linux/iio/backend.h
+++ b/include/linux/iio/backend.h
@@ -87,6 +87,7 @@ enum iio_backend_interface_type {
  * @ext_info_set: Extended info setter.
  * @ext_info_get: Extended info getter.
  * @interface_type_get: Interface type.
+ * @data_size_set: Data size.
  **/
 struct iio_backend_ops {
 	int (*enable)(struct iio_backend *back);
@@ -121,6 +122,7 @@ struct iio_backend_ops {
 			    const struct iio_chan_spec *chan, char *buf);
 	int (*interface_type_get)(struct iio_backend *back,
 				  enum iio_backend_interface_type *type);
+	int (*data_size_set)(struct iio_backend *back, ssize_t size);
 };
 
 int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
@@ -152,6 +154,7 @@ ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
 
 int iio_backend_interface_type_get(struct iio_backend *back,
 				   enum iio_backend_interface_type *type);
+ssize_t iio_backend_data_size_set(struct iio_backend *back, ssize_t size);
 int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
 				 struct iio_backend *back,
 				 struct iio_chan_spec *chan);
-- 
2.46.0

Re: [PATCH 2/7] iio: backend: add support for data size set

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 01:10:19PM +0300, Antoniu Miclaus wrote:
> Add backend support for setting the data size used.

...

> + * RETURNS:

It's main in this file, please add a patch that converts all 'RETURN' and
'RETURNS' to 'Return:'. Or at bare minimum use "Return:" here.

> + */

...

> +	if (!size)
> +		return -EINVAL;

Why 0 can be set? Is it default?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/7] iio: backend: add support for data size set

[David Lechner]

On Mon, Sep 23, 2024 at 12:16 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add backend support for setting the data size used.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  drivers/iio/industrialio-backend.c | 21 +++++++++++++++++++++
>  include/linux/iio/backend.h        |  3 +++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 53ab6bc86a50..a6a6bedce7f1 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -473,6 +473,27 @@ int iio_backend_interface_type_get(struct iio_backend *back,
>  }
>  EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
>
> +/**
> + * iio_backend_data_size_set - set the data width/size in the data bus.
> + * @back: Backend device
> + * @size: Size in bits
> + *
> + * Some frontend devices can dynamically control the word/data size on the
> + * interface/data bus. Hence, the backend device needs to be aware of it so
> + * data can be correctly transferred.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +ssize_t iio_backend_data_size_set(struct iio_backend *back, ssize_t size)

Why signed size? When will it be < 0?

> +{
> +       if (!size)
> +               return -EINVAL;
> +
> +       return iio_backend_op_call(back, data_size_set, size);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, IIO_BACKEND);
> +

Re: [PATCH 2/7] iio: backend: add support for data size set

[Jonathan Cameron]

On Mon, 23 Sep 2024 13:10:19 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add backend support for setting the data size used.

Say 'why' a device might do this.  I assume in general
it's because you haven't wired all the pins, but is there a
reason it might be dynamic?

Jonathan

> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  drivers/iio/industrialio-backend.c | 21 +++++++++++++++++++++
>  include/linux/iio/backend.h        |  3 +++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 53ab6bc86a50..a6a6bedce7f1 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -473,6 +473,27 @@ int iio_backend_interface_type_get(struct iio_backend *back,
>  }
>  EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND);
>  
> +/**
> + * iio_backend_data_size_set - set the data width/size in the data bus.
> + * @back: Backend device
> + * @size: Size in bits
> + *
> + * Some frontend devices can dynamically control the word/data size on the
> + * interface/data bus. Hence, the backend device needs to be aware of it so
> + * data can be correctly transferred.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +ssize_t iio_backend_data_size_set(struct iio_backend *back, ssize_t size)
> +{
> +	if (!size)
> +		return -EINVAL;
> +
> +	return iio_backend_op_call(back, data_size_set, size);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, 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 ba8ad30ac9ba..85b33ed69cc0 100644
> --- a/include/linux/iio/backend.h
> +++ b/include/linux/iio/backend.h
> @@ -87,6 +87,7 @@ enum iio_backend_interface_type {
>   * @ext_info_set: Extended info setter.
>   * @ext_info_get: Extended info getter.
>   * @interface_type_get: Interface type.
> + * @data_size_set: Data size.
>   **/
>  struct iio_backend_ops {
>  	int (*enable)(struct iio_backend *back);
> @@ -121,6 +122,7 @@ struct iio_backend_ops {
>  			    const struct iio_chan_spec *chan, char *buf);
>  	int (*interface_type_get)(struct iio_backend *back,
>  				  enum iio_backend_interface_type *type);
> +	int (*data_size_set)(struct iio_backend *back, ssize_t size);
>  };
>  
>  int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan);
> @@ -152,6 +154,7 @@ ssize_t iio_backend_ext_info_get(struct iio_dev *indio_dev, uintptr_t private,
>  
>  int iio_backend_interface_type_get(struct iio_backend *back,
>  				   enum iio_backend_interface_type *type);
> +ssize_t iio_backend_data_size_set(struct iio_backend *back, ssize_t size);
>  int iio_backend_extend_chan_spec(struct iio_dev *indio_dev,
>  				 struct iio_backend *back,
>  				 struct iio_chan_spec *chan);

[PATCH 3/7] iio: adc: adi-axi-adc: add interface type

[Antoniu Miclaus]

Add support for getting the interface (CMOS or LVDS) used by the AXI ADC
ip.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index 21ce7564e83d..ff48f26e02a3 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -39,6 +39,9 @@
 #define   ADI_AXI_REG_RSTN_MMCM_RSTN		BIT(1)
 #define   ADI_AXI_REG_RSTN_RSTN			BIT(0)
 
+#define ADI_AXI_ADC_REG_CONFIG			0x000c
+#define   ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N	BIT(7)
+
 #define ADI_AXI_ADC_REG_CTRL			0x0044
 #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
 
@@ -249,6 +252,25 @@ static int axi_adc_chan_disable(struct iio_backend *back, unsigned int chan)
 				 ADI_AXI_REG_CHAN_CTRL_ENABLE);
 }
 
+static int axi_adc_interface_type_get(struct iio_backend *back,
+				      enum iio_backend_interface_type *type)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(st->regmap, ADI_AXI_ADC_REG_CONFIG, &val);
+	if (ret)
+		return ret;
+
+	if (val & ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N)
+		*type = IIO_BACKEND_INTERFACE_CMOS;
+	else
+		*type = IIO_BACKEND_INTERFACE_LVDS;
+
+	return 0;
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -285,6 +307,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
 	.iodelay_set = axi_adc_iodelays_set,
 	.test_pattern_set = axi_adc_test_pattern_set,
 	.chan_status = axi_adc_chan_status,
+	.interface_type_get = axi_adc_interface_type_get,
 };
 
 static int adi_axi_adc_probe(struct platform_device *pdev)
-- 
2.46.0

[PATCH 4/7] iio: adc: adi-axi-adc: set data format

[Antoniu Miclaus]

Add support for selecting the data format within the AXI ADC ip.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/adc/adi-axi-adc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
index ff48f26e02a3..926a8902c621 100644
--- a/drivers/iio/adc/adi-axi-adc.c
+++ b/drivers/iio/adc/adi-axi-adc.c
@@ -45,6 +45,8 @@
 #define ADI_AXI_ADC_REG_CTRL			0x0044
 #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK	BIT(1)
 
+#define ADI_AXI_ADC_REG_CNTRL_3			0x004c
+
 #define ADI_AXI_ADC_REG_DRP_STATUS		0x0074
 #define   ADI_AXI_ADC_DRP_LOCKED		BIT(17)
 
@@ -271,6 +273,24 @@ static int axi_adc_interface_type_get(struct iio_backend *back,
 	return 0;
 }
 
+static int axi_adc_data_size_set(struct iio_backend *back,
+				 ssize_t size)
+{
+	struct adi_axi_adc_state *st = iio_backend_get_priv(back);
+	unsigned int val;
+
+	if (size <= 20)
+		val = 0;
+	else if (size <= 24)
+		val = 1;
+	else if (size <= 32)
+		val = 3;
+	else
+		return -EINVAL;
+
+	return regmap_write(st->regmap, ADI_AXI_ADC_REG_CNTRL_3, val);
+}
+
 static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
 						 struct iio_dev *indio_dev)
 {
@@ -308,6 +328,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
 	.test_pattern_set = axi_adc_test_pattern_set,
 	.chan_status = axi_adc_chan_status,
 	.interface_type_get = axi_adc_interface_type_get,
+	.data_size_set = axi_adc_data_size_set,
 };
 
 static int adi_axi_adc_probe(struct platform_device *pdev)
-- 
2.46.0

Re: [PATCH 4/7] iio: adc: adi-axi-adc: set data format

[David Lechner]

On Mon, Sep 23, 2024 at 12:16 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add support for selecting the data format within the AXI ADC ip.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  drivers/iio/adc/adi-axi-adc.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/iio/adc/adi-axi-adc.c b/drivers/iio/adc/adi-axi-adc.c
> index ff48f26e02a3..926a8902c621 100644
> --- a/drivers/iio/adc/adi-axi-adc.c
> +++ b/drivers/iio/adc/adi-axi-adc.c
> @@ -45,6 +45,8 @@
>  #define ADI_AXI_ADC_REG_CTRL                   0x0044
>  #define    ADI_AXI_ADC_CTRL_DDR_EDGESEL_MASK   BIT(1)
>
> +#define ADI_AXI_ADC_REG_CNTRL_3                        0x004c
> +
>  #define ADI_AXI_ADC_REG_DRP_STATUS             0x0074
>  #define   ADI_AXI_ADC_DRP_LOCKED               BIT(17)
>
> @@ -271,6 +273,24 @@ static int axi_adc_interface_type_get(struct iio_backend *back,
>         return 0;
>  }
>
> +static int axi_adc_data_size_set(struct iio_backend *back,
> +                                ssize_t size)
> +{
> +       struct adi_axi_adc_state *st = iio_backend_get_priv(back);
> +       unsigned int val;
> +
> +       if (size <= 20)
> +               val = 0;
> +       else if (size <= 24)
> +               val = 1;
> +       else if (size <= 32)
> +               val = 3;
> +       else
> +               return -EINVAL;
> +
> +       return regmap_write(st->regmap, ADI_AXI_ADC_REG_CNTRL_3, val);

http://analogdevicesinc.github.io/hdl/library/axi_ad485x/index.html
says that bit 8 of this register is CRC_EN and bits 7 to 0 are
CUSTOM_CONTROL so we likely need a mask and regmap_write_bits() here.

Also, since the value being written here is CUSTOM_CONTROL, it makes
me wonder if this callback really belongs to adi_axi_adc_generic or if
we need a copy of that specific to the axi_ad485x project to handle
the custom meaning of this value for this family of chips. The
description of CUSTOM_CONTROL in the link is "Select output format
decode mode.(for ADAQ8092: bit 0 - enables digital output randomizer
decode , bit 1 - enables alternate bit polarity decode)." which
doesn't look like what is being done in the function above.

> +}
> +
>  static struct iio_buffer *axi_adc_request_buffer(struct iio_backend *back,
>                                                  struct iio_dev *indio_dev)
>  {
> @@ -308,6 +328,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = {
>         .test_pattern_set = axi_adc_test_pattern_set,
>         .chan_status = axi_adc_chan_status,
>         .interface_type_get = axi_adc_interface_type_get,
> +       .data_size_set = axi_adc_data_size_set,
>  };
>
>  static int adi_axi_adc_probe(struct platform_device *pdev)
> --
> 2.46.0
>

[PATCH 5/7] dt-bindings: iio: adc: add ad458x

[Antoniu Miclaus]

Add devicetree bindings for ad458x DAS family.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 .../bindings/iio/adc/adi,ad485x.yaml          | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
new file mode 100644
index 000000000000..5f5bdfa9522b
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2022 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad485x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD485X DAS family device driver
+
+maintainers:
+  - Sergiu Cuciurean <sergiu.cuciurean@analog.com>
+  - Dragos Bogdan <dragos.bogdan@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+  Analog Devices AD485X DAS family
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4858.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad4858
+      - adi,ad4857
+      - adi,ad4856
+      - adi,ad4855
+      - adi,ad4854
+      - adi,ad4853
+      - adi,ad4852
+      - adi,ad4851
+      - adi,ad4858i
+
+  reg:
+    maxItems: 1
+
+  vcc-supply: true
+
+  vdd-supply: true
+
+  vddh-supply: true
+
+  vio-supply: true
+
+  pwms:
+    maxItems: 1
+
+  io-backends:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 100000000
+
+required:
+  - compatible
+  - reg
+  - vcc-supply
+  - vdd-supply
+  - vddh-supply
+  - vio-supply
+  - pwms
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0{
+            compatible = "adi,ad4858";
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+            vcc-supply = <&vcc>;
+            vdd-supply = <&vdd>;
+            vddh-supply = <&vddh>;
+            vio-supply = <&vio>;
+            pwms = <&pwm_gen 0 0>;
+            io-backends = <&iio_backend>;
+        };
+    };
+...
-- 
2.46.0

Re: [PATCH 5/7] dt-bindings: iio: adc: add ad458x

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Mon, Sep 23, 2024 at 01:10:22PM +0300, Antoniu Miclaus wrote:
> Add devicetree bindings for ad458x DAS family.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad485x.yaml          | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> new file mode 100644
> index 000000000000..5f5bdfa9522b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad485x.yaml#

The filename should match one of the compatibles.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD485X DAS family device driver
> +
> +maintainers:
> +  - Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> +  - Dragos Bogdan <dragos.bogdan@analog.com>
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +  Analog Devices AD485X DAS family
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4858.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4858
> +      - adi,ad4857
> +      - adi,ad4856
> +      - adi,ad4855
> +      - adi,ad4854
> +      - adi,ad4853
> +      - adi,ad4852
> +      - adi,ad4851
> +      - adi,ad4858i

I take it that all of these devices have some differences between them?

Everything else here seems pretty okay.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 5/7] dt-bindings: iio: adc: add ad458x

[David Lechner]

On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add devicetree bindings for ad458x DAS family.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad485x.yaml          | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
>
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> new file mode 100644
> index 000000000000..5f5bdfa9522b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.
> +%YAML 1.2
> +---

I think we could make the bindings a bit more complete (even if the
driver doesn't use everything yet). Some suggestions below.

> +$id: http://devicetree.org/schemas/iio/adc/adi,ad485x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD485X DAS family device driver
> +
> +maintainers:
> +  - Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> +  - Dragos Bogdan <dragos.bogdan@analog.com>
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +  Analog Devices AD485X DAS family
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4858.pdf

Links for other parts? (I only looked at this datasheet, so some of my
comments below might only apply to this part and not the others.)

Since this is a SPI peripheral, we should add...

$ref: /schemas/spi/spi-peripheral-props.yaml#

> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4858
> +      - adi,ad4857
> +      - adi,ad4856
> +      - adi,ad4855
> +      - adi,ad4854
> +      - adi,ad4853
> +      - adi,ad4852
> +      - adi,ad4851
> +      - adi,ad4858i
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply: true

Do we also need vee-supply (it can be a negative voltage or 0V)? Or is
vcc-supply the voltage between V_CC and V_EE?

> +
> +  vdd-supply: true
> +
> +  vddh-supply: true

Datasheet says V_DDL can be the same or separate supply as V_DDH, so
should probably also have vddl-supply: true.

> +
> +  vio-supply: true

What about reference supplies (REFIO and REFBUF)?

> +
> +  pwms:
> +    maxItems: 1

I suppose this is connected to the CNV pin? Probably worth adding a
description to say that since it isn't so obvious.

> +
> +  io-backends:
> +    maxItems: 1

For PD pin (power down):

pd-gpios:
    maxItems: 1

> +
> +  spi-max-frequency:
> +    maximum: 100000000

Datasheet says that minimum CSCK Period is 40 ns, so that would be
25000000 Hz max frequency.

> +
> +required:
> +  - compatible
> +  - reg
> +  - vcc-supply
> +  - vdd-supply
> +  - vddh-supply

Datasheet says to tie V_DDH to GND to disable LDO, so it sounds like
vddh-supply should be optional, not required.

> +  - vio-supply
> +  - pwms
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0{
> +            compatible = "adi,ad4858";
> +            reg = <0>;
> +            spi-max-frequency = <10000000>;
> +            vcc-supply = <&vcc>;
> +            vdd-supply = <&vdd>;
> +            vddh-supply = <&vddh>;
> +            vio-supply = <&vio>;
> +            pwms = <&pwm_gen 0 0>;
> +            io-backends = <&iio_backend>;
> +        };
> +    };
> +...
> --
> 2.46.0
>

Re: [PATCH 5/7] dt-bindings: iio: adc: add ad458x

[Rob Herring]

On Mon, Sep 23, 2024 at 01:10:22PM +0300, Antoniu Miclaus wrote:
> Add devicetree bindings for ad458x DAS family.

typo: ad485x

Subject too.

> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  .../bindings/iio/adc/adi,ad485x.yaml          | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> new file mode 100644
> index 000000000000..5f5bdfa9522b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad485x.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2022 Analog Devices Inc.

It's 2024

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad485x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD485X DAS family device driver

What's DAS?

This is a binding, not 'device driver'. Just drop.

> +
> +maintainers:
> +  - Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> +  - Dragos Bogdan <dragos.bogdan@analog.com>
> +  - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> +  Analog Devices AD485X DAS family
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad4858.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad4858
> +      - adi,ad4857
> +      - adi,ad4856
> +      - adi,ad4855
> +      - adi,ad4854
> +      - adi,ad4853
> +      - adi,ad4852
> +      - adi,ad4851
> +      - adi,ad4858i
> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply: true
> +
> +  vdd-supply: true
> +
> +  vddh-supply: true
> +
> +  vio-supply: true
> +
> +  pwms:
> +    maxItems: 1
> +
> +  io-backends:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 100000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - vcc-supply
> +  - vdd-supply
> +  - vddh-supply
> +  - vio-supply
> +  - pwms
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0{
> +            compatible = "adi,ad4858";
> +            reg = <0>;
> +            spi-max-frequency = <10000000>;
> +            vcc-supply = <&vcc>;
> +            vdd-supply = <&vdd>;
> +            vddh-supply = <&vddh>;
> +            vio-supply = <&vio>;
> +            pwms = <&pwm_gen 0 0>;
> +            io-backends = <&iio_backend>;
> +        };
> +    };
> +...
> -- 
> 2.46.0
> 

[PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Antoniu Miclaus]

Add support for the AD485X DAS familiy.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 drivers/iio/adc/Kconfig  |   12 +
 drivers/iio/adc/Makefile |    1 +
 drivers/iio/adc/ad485x.c | 1061 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 1074 insertions(+)
 create mode 100644 drivers/iio/adc/ad485x.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index f60fe85a30d5..83f55229d731 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -36,6 +36,18 @@ config AD4130
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad4130.
 
+config AD485X
+	tristate "Analog Device AD485x DAS Driver"
+	depends on SPI
+	select REGMAP_SPI
+	select IIO_BACKEND
+	help
+	  Say yes here to build support for Analog Devices AD485x high speed
+	  data acquisition system (DAS).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called ad485x.
+
 config AD7091R
 	tristate
 
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index d370e066544e..26c1670c8913 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
 obj-$(CONFIG_AD4130) += ad4130.o
+obj-$(CONFIG_AD485X) += ad485x.o
 obj-$(CONFIG_AD7091R) += ad7091r-base.o
 obj-$(CONFIG_AD7091R5) += ad7091r5.o
 obj-$(CONFIG_AD7091R8) += ad7091r8.o
diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
new file mode 100644
index 000000000000..3333cad9ed8f
--- /dev/null
+++ b/drivers/iio/adc/ad485x.c
@@ -0,0 +1,1061 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD485x DAS driver
+ *
+ * Copyright 2024 Analog Devices Inc.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/iio/backend.h>
+#include <linux/iio/iio.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/units.h>
+
+#define AD485X_REG_INTERFACE_CONFIG_A	0x00
+#define AD485X_REG_INTERFACE_CONFIG_B	0x01
+#define AD485X_REG_PRODUCT_ID_L		0x04
+#define AD485X_REG_PRODUCT_ID_H		0x05
+#define AD485X_REG_DEVICE_CTRL		0x25
+#define AD485X_REG_PACKET		0x26
+
+#define AD485X_REG_CH_CONFIG_BASE	0x2A
+#define AD485X_REG_CHX_SOFTSPAN(ch)	((0x12 * (ch)) + AD485X_REG_CH_CONFIG_BASE)
+#define AD485X_REG_CHX_OFFSET(ch)	(AD485X_REG_CHX_SOFTSPAN(ch) + 0x01)
+#define AD485X_REG_CHX_OFFSET_LSB(ch)	AD485X_REG_CHX_OFFSET(ch)
+#define AD485X_REG_CHX_OFFSET_MID(ch)	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
+#define AD485X_REG_CHX_OFFSET_MSB(ch)	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
+#define AD485X_REG_CHX_GAIN(ch)		(AD485X_REG_CHX_OFFSET(ch) + 0x03)
+#define AD485X_REG_CHX_GAIN_LSB(ch)	AD485X_REG_CHX_GAIN(ch)
+#define AD485X_REG_CHX_GAIN_MSB(ch)	(AD485X_REG_CHX_GAIN(ch) + 0x01)
+#define AD485X_REG_CHX_PHASE(ch)	(AD485X_REG_CHX_GAIN(ch) + 0x02)
+#define AD485X_REG_CHX_PHASE_LSB(ch)	AD485X_REG_CHX_PHASE(ch)
+#define AD485X_REG_CHX_PHASE_MSB(ch)	(AD485X_REG_CHX_PHASE_LSB(ch) + 0x01)
+
+#define AD485X_REG_TESTPAT_0(c)		(0x38 + (c) * 0x12)
+#define AD485X_REG_TESTPAT_1(c)		(0x39 + (c) * 0x12)
+#define AD485X_REG_TESTPAT_2(c)		(0x3A + (c) * 0x12)
+#define AD485X_REG_TESTPAT_3(c)		(0x3B + (c) * 0x12)
+
+#define AD485X_SW_RESET			(BIT(7) | BIT(0))
+#define AD485X_SDO_ENABLE		BIT(4)
+#define AD485X_SINGLE_INSTRUCTION	BIT(7)
+#define AD485X_ECHO_CLOCK_MODE		BIT(0)
+
+#define AD485X_PACKET_FORMAT_0		0
+#define AD485X_PACKET_FORMAT_1		1
+#define AD485X_PACKET_FORMAT_MASK	GENMASK(1, 0)
+#define AD485X_OS_EN			BIT(7)
+
+#define AD485X_TEST_PAT			BIT(2)
+
+#define AD4858_PACKET_SIZE_20		0
+#define AD4858_PACKET_SIZE_24		1
+#define AD4858_PACKET_SIZE_32		2
+
+#define AD4857_PACKET_SIZE_16		0
+#define AD4857_PACKET_SIZE_24		1
+
+#define AD485X_TESTPAT_0_DEFAULT	0x2A
+#define AD485X_TESTPAT_1_DEFAULT	0x3C
+#define AD485X_TESTPAT_2_DEFAULT	0xCE
+#define AD485X_TESTPAT_3_DEFAULT(c)	(0x0A + (0x10 * (c)))
+
+#define AD485X_SOFTSPAN_0V_2V5		0
+#define AD485X_SOFTSPAN_N2V5_2V5	1
+#define AD485X_SOFTSPAN_0V_5V		2
+#define AD485X_SOFTSPAN_N5V_5V		3
+#define AD485X_SOFTSPAN_0V_6V25		4
+#define AD485X_SOFTSPAN_N6V25_6V25	5
+#define AD485X_SOFTSPAN_0V_10V		6
+#define AD485X_SOFTSPAN_N10V_10V	7
+#define AD485X_SOFTSPAN_0V_12V5		8
+#define AD485X_SOFTSPAN_N12V5_12V5	9
+#define AD485X_SOFTSPAN_0V_20V		10
+#define AD485X_SOFTSPAN_N20V_20V	11
+#define AD485X_SOFTSPAN_0V_25V		12
+#define AD485X_SOFTSPAN_N25V_25V	13
+#define AD485X_SOFTSPAN_0V_40V		14
+#define AD485X_SOFTSPAN_N40V_40V	15
+
+#define AD485X_MAX_LANES		8
+#define AD485X_MAX_IODELAY		32
+
+#define AD485X_T_CNVH_NS		40
+
+#define AD4858_PROD_ID			0x60
+#define AD4857_PROD_ID			0x61
+#define AD4856_PROD_ID			0x62
+#define AD4855_PROD_ID			0x63
+#define AD4854_PROD_ID			0x64
+#define AD4853_PROD_ID			0x65
+#define AD4852_PROD_ID			0x66
+#define AD4851_PROD_ID			0x67
+#define AD4858I_PROD_ID			0x6F
+
+struct ad485x_chip_info {
+	const char *name;
+	unsigned int product_id;
+	const unsigned int (*scale_table)[2];
+	int num_scales;
+	const int *offset_table;
+	int num_offset;
+	const struct iio_chan_spec *channels;
+	unsigned int num_channels;
+	unsigned long throughput;
+	unsigned int resolution;
+};
+
+struct ad485x_state {
+	struct spi_device *spi;
+	struct pwm_device *cnv;
+	struct iio_backend *back;
+	/*
+	 * Synchronize access to members the of driver state, and ensure
+	 * atomicity of consecutive regmap operations.
+	 */
+	struct mutex lock;
+	struct regmap *regmap;
+	const struct ad485x_chip_info *info;
+	unsigned long sampling_freq;
+	unsigned int (*scales)[2];
+	int *offsets;
+};
+
+static int ad485x_reg_access(struct iio_dev *indio_dev,
+			     unsigned int reg,
+			     unsigned int writeval,
+			     unsigned int *readval)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+
+	if (readval)
+		return regmap_read(st->regmap, reg, readval);
+
+	return regmap_write(st->regmap, reg, writeval);
+}
+
+static int ad485x_set_sampling_freq(struct ad485x_state *st, unsigned int freq)
+{
+	struct pwm_state cnv_state = {
+		.duty_cycle = AD485X_T_CNVH_NS,
+		.enabled = true,
+	};
+	int ret;
+
+	if (freq > st->info->throughput)
+		freq = st->info->throughput;
+
+	cnv_state.period = DIV_ROUND_CLOSEST_ULL(1000000000, freq);
+
+	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
+	if (ret)
+		return ret;
+
+	st->sampling_freq = freq;
+
+	return 0;
+}
+
+static int ad485x_setup(struct ad485x_state *st)
+{
+	unsigned int product_id;
+	int ret;
+
+	ret = ad485x_set_sampling_freq(st, 1000000);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
+			   AD485X_SW_RESET);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
+			   AD485X_SINGLE_INSTRUCTION);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
+			   AD485X_SDO_ENABLE);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
+	if (ret)
+		return ret;
+
+	if (product_id != st->info->product_id)
+		dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
+			 product_id);
+
+	ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
+			   AD485X_ECHO_CLOCK_MODE);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
+}
+
+static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
+{
+	int i, cnt = 0, max_cnt = 0, start, max_start = 0;
+
+	for (i = 0, start = -1; i < size; i++) {
+		if (field[i] == 0) {
+			if (start == -1)
+				start = i;
+			cnt++;
+		} else {
+			if (cnt > max_cnt) {
+				max_cnt = cnt;
+				max_start = start;
+			}
+			start = -1;
+			cnt = 0;
+		}
+	}
+
+	if (cnt > max_cnt) {
+		max_cnt = cnt;
+		max_start = start;
+	}
+
+	if (!max_cnt)
+		return -EIO;
+
+	*ret_start = max_start;
+
+	return max_cnt;
+}
+
+static int ad485x_calibrate(struct ad485x_state *st)
+{
+	int opt_delay, lane_num, delay, i, s, c;
+	enum iio_backend_interface_type interface_type;
+	bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];
+	int ret;
+
+	ret = iio_backend_interface_type_get(st->back, &interface_type);
+	if (ret)
+		return ret;
+
+	if (interface_type == IIO_BACKEND_INTERFACE_CMOS)
+		lane_num = st->info->num_channels;
+	else
+		lane_num = 1;
+
+	if (st->info->resolution == 16) {
+		ret = iio_backend_data_size_set(st->back, 24);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD485X_REG_PACKET,
+				   AD485X_TEST_PAT | AD4857_PACKET_SIZE_24);
+		if (ret)
+			return ret;
+	} else {
+		ret = iio_backend_data_size_set(st->back, 32);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD485X_REG_PACKET,
+				   AD485X_TEST_PAT | AD4858_PACKET_SIZE_32);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < st->info->num_channels; i++) {
+		ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_0(i),
+				   AD485X_TESTPAT_0_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_1(i),
+				   AD485X_TESTPAT_1_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_2(i),
+				   AD485X_TESTPAT_2_DEFAULT);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(st->regmap, AD485X_REG_TESTPAT_3(i),
+				   AD485X_TESTPAT_3_DEFAULT(i));
+		if (ret)
+			return ret;
+
+		ret = iio_backend_chan_enable(st->back, i);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < lane_num; i++) {
+		for (delay = 0; delay < AD485X_MAX_IODELAY; delay++) {
+			ret = iio_backend_iodelay_set(st->back, i, delay);
+			if (ret)
+				return ret;
+			ret = iio_backend_chan_status(st->back, i,
+						      &pn_status[i][delay]);
+			if (ret)
+				return ret;
+		}
+	}
+
+	for (i = 0; i < lane_num; i++) {
+		c = ad485x_find_opt(&pn_status[i][0], AD485X_MAX_IODELAY, &s);
+		if (c < 0)
+			return c;
+
+		opt_delay = s + c / 2;
+		ret = iio_backend_iodelay_set(st->back, i, opt_delay);
+		if (ret)
+			return ret;
+	}
+
+	for (i = 0; i < st->info->num_channels; i++) {
+		ret = iio_backend_chan_disable(st->back, i);
+		if (ret)
+			return ret;
+	}
+
+	ret = iio_backend_data_size_set(st->back, 20);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
+}
+
+static const char *const ad4858_packet_fmts[] = {
+	"20-bit", "24-bit", "32-bit"
+};
+
+static const char *const ad4857_packet_fmts[] = {
+	"16-bit", "24-bit"
+};
+
+static int ad485x_set_packet_format(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    unsigned int format)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (chan->scan_type.realbits == 20)
+		switch (format) {
+		case 0:
+			val = 20;
+			break;
+		case 1:
+			val = 24;
+			break;
+		case 2:
+			val = 32;
+			break;
+		default:
+			return -EINVAL;
+		}
+	else if (chan->scan_type.realbits == 16)
+		switch (format) {
+		case 0:
+			val = 16;
+			break;
+		case 1:
+			val = 24;
+			break;
+		default:
+			return -EINVAL;
+		}
+	else
+		return -EINVAL;
+
+	ret = iio_backend_data_size_set(st->back, val);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(st->regmap, AD485X_REG_PACKET,
+				  AD485X_PACKET_FORMAT_MASK, format);
+}
+
+static int ad485x_get_packet_format(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+	unsigned int format;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
+	if (ret)
+		return ret;
+
+	format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
+
+	return format;
+}
+
+static const struct iio_enum ad4858_packet_fmt = {
+	.items = ad4858_packet_fmts,
+	.num_items = ARRAY_SIZE(ad4858_packet_fmts),
+	.set = ad485x_set_packet_format,
+	.get = ad485x_get_packet_format,
+};
+
+static const struct iio_enum ad4857_packet_fmt = {
+	.items = ad4857_packet_fmts,
+	.num_items = ARRAY_SIZE(ad4857_packet_fmts),
+	.set = ad485x_set_packet_format,
+	.get = ad485x_get_packet_format,
+};
+
+static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
+				 int *val2)
+{
+	unsigned int reg_val;
+	int gain;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
+			  &reg_val);
+	if (ret)
+		return ret;
+
+	gain = (reg_val & 0xFF) << 8;
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
+			  &reg_val);
+	if (ret)
+		return ret;
+
+	gain |= reg_val & 0xFF;
+
+	*val = gain;
+	*val2 = 32768;
+
+	return IIO_VAL_FRACTIONAL;
+}
+
+static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
+				 int val2)
+{
+	unsigned long long gain;
+	unsigned int reg_val;
+	int ret;
+
+	gain = val * 1000000 + val2;
+	gain = gain * 32768;
+	gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
+
+	reg_val = gain;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
+			   reg_val >> 8);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
+			    reg_val & 0xFF);
+}
+
+static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
+				int *val2)
+{
+	unsigned int lsb, mid, msb;
+	int ret;
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
+			  &msb);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
+			  &mid);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
+			  &lsb);
+	if (ret)
+		return ret;
+
+	if (st->info->resolution == 16) {
+		*val = msb << 8;
+		*val |= mid;
+		*val = sign_extend32(*val, 15);
+	} else {
+		*val = msb << 12;
+		*val |= mid << 4;
+		*val |= lsb >> 4;
+		*val = sign_extend32(*val, 19);
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
+				int val2)
+{
+	unsigned int lsb, mid, msb;
+	int ret;
+
+	if (st->info->resolution == 16) {
+		lsb = 0;
+		mid = val & 0xFF;
+		msb = (val >> 8) & 0xFF;
+	} else {
+		lsb = (val << 4) & 0xFF;
+		mid = (val >> 4) & 0xFF;
+		msb = (val >> 12) & 0xFF;
+	}
+
+	guard(mutex)(&st->lock);
+
+	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
+	if (ret)
+		return ret;
+
+	return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
+}
+
+static const unsigned int ad485x_scale_table[][2] = {
+	{2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
+	{12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
+	{20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
+	{80000, 0xF}
+};
+
+static const int ad4857_offset_table[] = {
+	0, -32768
+};
+
+static const int ad4858_offset_table[] = {
+	0, -524288
+};
+
+static const unsigned int ad485x_scale_avail[] = {
+	2500, 5000, 10000, 6250, 12500, 20000, 25000, 40000, 50000, 80000
+};
+
+static void __ad485x_get_scale(struct ad485x_state *st, int scale_tbl,
+			       unsigned int *val, unsigned int *val2)
+{
+	const struct ad485x_chip_info *info = st->info;
+	const struct iio_chan_spec *chan = &info->channels[0];
+	unsigned int tmp;
+
+	tmp = (scale_tbl * 1000000ULL) >> chan->scan_type.realbits;
+	*val = tmp / 1000000;
+	*val2 = tmp % 1000000;
+}
+
+static int ad485x_set_scale(struct ad485x_state *st,
+			    const struct iio_chan_spec *chan, int val, int val2)
+{
+	const struct ad485x_chip_info *info = st->info;
+	unsigned int scale_val[2];
+	unsigned int i, j = 0;
+
+	for (i = 0; i < info->num_scales; i++) {
+		__ad485x_get_scale(st, info->scale_table[i][0],
+				   &scale_val[0], &scale_val[1]);
+		if (scale_val[0] != val || scale_val[1] != val2)
+			continue;
+
+		/*
+		 * Adjust the softspan value (differential or single ended)
+		 * based on the scale value selected and current offset of
+		 * the channel.
+		 *
+		 * If the offset is 0 then continue iterations until finding
+		 * the next matching scale value which always corresponds to
+		 * the single ended mode.
+		 */
+		if (!st->offsets[chan->channel] && !j) {
+			j++;
+			continue;
+		}
+
+		return regmap_write(st->regmap,
+				    AD485X_REG_CHX_SOFTSPAN(chan->channel),
+				    info->scale_table[i][1]);
+	}
+
+	return -EINVAL;
+}
+
+static int ad485x_get_scale(struct ad485x_state *st,
+			    const struct iio_chan_spec *chan, int *val,
+			    int *val2)
+{
+	const struct ad485x_chip_info *info = st->info;
+	unsigned int i, softspan_val;
+	int ret;
+
+	ret = regmap_read(st->regmap, AD485X_REG_CHX_SOFTSPAN(chan->channel),
+			  &softspan_val);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < info->num_scales; i++) {
+		if (softspan_val == info->scale_table[i][1])
+			break;
+	}
+
+	if (i == info->num_scales)
+		return -EIO;
+
+	__ad485x_get_scale(st, info->scale_table[i][0], val, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad485x_set_offset(struct ad485x_state *st,
+			     const struct iio_chan_spec *chan, int val)
+{
+	guard(mutex)(&st->lock);
+
+	if (st->offsets[chan->channel] != val) {
+		st->offsets[chan->channel] = val;
+		/* Restore to the default range if offset changes */
+		if (st->offsets[chan->channel])
+			return regmap_write(st->regmap,
+						AD485X_REG_CHX_SOFTSPAN(chan->channel),
+						AD485X_SOFTSPAN_N40V_40V);
+		return regmap_write(st->regmap,
+					AD485X_REG_CHX_SOFTSPAN(chan->channel),
+					AD485X_SOFTSPAN_0V_40V);
+	}
+
+	return 0;
+}
+
+static int ad485x_scale_offset_fill(struct ad485x_state *st)
+{
+	unsigned int i, val1, val2;
+
+	st->offsets = devm_kcalloc(&st->spi->dev, st->info->num_channels,
+				   sizeof(*st->offsets), GFP_KERNEL);
+	if (!st->offsets)
+		return -ENOMEM;
+
+	st->scales = devm_kmalloc_array(&st->spi->dev, ARRAY_SIZE(ad485x_scale_avail),
+					sizeof(*st->scales), GFP_KERNEL);
+	if (!st->scales)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(ad485x_scale_avail); i++) {
+		__ad485x_get_scale(st, ad485x_scale_avail[i], &val1, &val2);
+		st->scales[i][0] = val1;
+		st->scales[i][1] = val2;
+	}
+
+	return 0;
+}
+
+static int ad485x_read_raw(struct iio_dev *indio_dev,
+			   const struct iio_chan_spec *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*val = st->sampling_freq;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad485x_get_calibscale(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return ad485x_get_scale(st, chan, val, val2);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad485x_get_calibbias(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_OFFSET:
+		scoped_guard(mutex, &st->lock)
+			* val = st->offsets[chan->channel];
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad485x_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		return ad485x_set_sampling_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		return ad485x_set_scale(st, chan, val, val2);
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return ad485x_set_calibscale(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return ad485x_set_calibbias(st, chan->channel, val, val2);
+	case IIO_CHAN_INFO_OFFSET:
+		return ad485x_set_offset(st, chan, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad485x_update_scan_mode(struct iio_dev *indio_dev,
+				   const unsigned long *scan_mask)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+	unsigned int c;
+	int ret;
+
+	for (c = 0; c < st->info->num_channels; c++) {
+		if (test_bit(c, scan_mask))
+			ret = iio_backend_chan_enable(st->back, c);
+		else
+			ret = iio_backend_chan_disable(st->back, c);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int ad485x_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	struct ad485x_state *st = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (const int *)st->scales;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(ad485x_scale_avail) * 2;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_OFFSET:
+		*vals = st->info->offset_table;
+		*type = IIO_VAL_INT;
+		*length = st->info->num_offset;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define AD485X_IIO_CHANNEL(index, real, storage, info)			\
+{									\
+	.type = IIO_VOLTAGE,						\
+	.ext_info = info,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
+		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OFFSET),				\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
+	.info_mask_shared_by_type_available =				\
+		BIT(IIO_CHAN_INFO_SCALE) |				\
+		BIT(IIO_CHAN_INFO_OFFSET),				\
+	.indexed = 1,							\
+	.channel = index,						\
+	.scan_index = index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = real,					\
+		.storagebits = storage,					\
+	},								\
+}
+
+static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
+	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
+	IIO_ENUM_AVAILABLE("packet_format",
+			   IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
+	{},
+};
+
+static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
+	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
+	IIO_ENUM_AVAILABLE("packet_format",
+			   IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
+	{},
+};
+
+static const struct iio_chan_spec ad4858_channels[] = {
+	AD485X_IIO_CHANNEL(0, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(1, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(2, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(3, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(4, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(5, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(6, 20, 32, ad4858_ext_info),
+	AD485X_IIO_CHANNEL(7, 20, 32, ad4858_ext_info),
+};
+
+static const struct iio_chan_spec ad4857_channels[] = {
+	AD485X_IIO_CHANNEL(0, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(1, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(2, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(3, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(4, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(5, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(6, 16, 16, ad4857_ext_info),
+	AD485X_IIO_CHANNEL(7, 16, 16, ad4857_ext_info),
+};
+
+static const struct ad485x_chip_info ad4858_info = {
+	.name = "ad4858",
+	.product_id = AD4858_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4857_info = {
+	.name = "ad4857",
+	.product_id = AD4857_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4856_info = {
+	.name = "ad4856",
+	.product_id = AD4856_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 250 * KILO,
+	.resolution = 20,
+	.resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4855_info = {
+	.name = "ad4855",
+	.product_id = AD4855_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 250 * KILO,
+	.resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4854_info = {
+	.name = "ad4854",
+	.product_id = AD4854_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4853_info = {
+	.name = "ad4853",
+	.product_id = AD4853_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 1 * MEGA,
+	.resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4852_info = {
+	.name = "ad4852",
+	.product_id = AD4852_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 250 * KILO,
+	.resolution = 20,
+};
+
+static const struct ad485x_chip_info ad4851_info = {
+	.name = "ad4851",
+	.product_id = AD4851_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4857_offset_table,
+	.num_offset = ARRAY_SIZE(ad4857_offset_table),
+	.channels = ad4857_channels,
+	.num_channels = ARRAY_SIZE(ad4857_channels),
+	.throughput = 250 * KILO,
+	.resolution = 16,
+};
+
+static const struct ad485x_chip_info ad4858i_info = {
+	.name = "ad4858i",
+	.product_id = AD4858I_PROD_ID,
+	.scale_table = ad485x_scale_table,
+	.num_scales = ARRAY_SIZE(ad485x_scale_table),
+	.offset_table = ad4858_offset_table,
+	.num_offset = ARRAY_SIZE(ad4858_offset_table),
+	.channels = ad4858_channels,
+	.num_channels = ARRAY_SIZE(ad4858_channels),
+	.throughput = 1000000,
+	.resolution = 20,
+};
+
+static const struct iio_info ad485x_info = {
+	.debugfs_reg_access = ad485x_reg_access,
+	.read_raw = ad485x_read_raw,
+	.write_raw = ad485x_write_raw,
+	.update_scan_mode = ad485x_update_scan_mode,
+	.read_avail = ad485x_read_avail,
+};
+
+static const struct regmap_config regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+	.read_flag_mask = BIT(7),
+};
+
+static const char * const ad485x_power_supplies[] = {
+	"vcc",	"vdd",	"vddh", "vio"
+};
+
+static int ad485x_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct ad485x_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+
+	mutex_init(&st->lock);
+
+	ret = devm_regulator_bulk_get_enable(&spi->dev,
+					     ARRAY_SIZE(ad485x_power_supplies),
+					     ad485x_power_supplies);
+	if (ret)
+		return dev_err_probe(&spi->dev, ret,
+				     "failed to get and enable supplies\n");
+
+	st->cnv = devm_pwm_get(&spi->dev, NULL);
+	if (IS_ERR(st->cnv))
+		return PTR_ERR(st->cnv);
+
+	st->info = spi_get_device_match_data(spi);
+	if (!st->info)
+		return -ENODEV;
+
+	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
+	if (IS_ERR(st->regmap))
+		return PTR_ERR(st->regmap);
+
+	ret = ad485x_scale_offset_fill(st);
+	if (ret)
+		return ret;
+
+	ret = ad485x_setup(st);
+	if (ret)
+		return ret;
+
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &ad485x_info;
+
+	st->back = devm_iio_backend_get(&spi->dev, NULL);
+	if (IS_ERR(st->back))
+		return PTR_ERR(st->back);
+
+	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = devm_iio_backend_enable(&spi->dev, st->back);
+	if (ret)
+		return ret;
+
+	ret = ad485x_calibrate(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad485x_of_match[] = {
+	{ .compatible = "adi,ad4858", .data = &ad4858_info, },
+	{ .compatible = "adi,ad4857", .data = &ad4857_info, },
+	{ .compatible = "adi,ad4856", .data = &ad4856_info, },
+	{ .compatible = "adi,ad4855", .data = &ad4855_info, },
+	{ .compatible = "adi,ad4854", .data = &ad4854_info, },
+	{ .compatible = "adi,ad4853", .data = &ad4853_info, },
+	{ .compatible = "adi,ad4852", .data = &ad4852_info, },
+	{ .compatible = "adi,ad4851", .data = &ad4851_info, },
+	{ .compatible = "adi,ad4858i", .data = &ad4858i_info, },
+	{}
+};
+
+static const struct spi_device_id ad485x_spi_id[] = {
+	{ "ad4858", (kernel_ulong_t)&ad4858_info },
+	{ "ad4857", (kernel_ulong_t)&ad4857_info },
+	{ "ad4856", (kernel_ulong_t)&ad4856_info },
+	{ "ad4855", (kernel_ulong_t)&ad4855_info },
+	{ "ad4854", (kernel_ulong_t)&ad4854_info },
+	{ "ad4853", (kernel_ulong_t)&ad4853_info },
+	{ "ad4852", (kernel_ulong_t)&ad4852_info },
+	{ "ad4851", (kernel_ulong_t)&ad4851_info },
+	{ "ad4858i", (kernel_ulong_t)&ad4858i_info },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ad485x_spi_id);
+
+static struct spi_driver ad485x_driver = {
+	.probe = ad485x_probe,
+	.driver = {
+		.name   = "ad485x",
+		.of_match_table = ad485x_of_match,
+	},
+	.id_table = ad485x_spi_id,
+};
+module_spi_driver(ad485x_driver);
+
+MODULE_AUTHOR("Sergiu Cuciurean <sergiu.cuciurean@analog.com>");
+MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>");
+MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD485x DAS driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BACKEND);
-- 
2.46.0

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 01:10:23PM +0300, Antoniu Miclaus wrote:
> Add support for the AD485X DAS familiy.

...

> +#include <asm/unaligned.h>

It's better to move this after linux/*.h

> +#include <linux/delay.h>

> +#include <linux/iio/backend.h>
> +#include <linux/iio/iio.h>

This can be split to a group after the other linux/*.h

> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>

Seems missing headers:

+ array_size.h
+ bits.h
+ device.h
+ err.h
+ mod_devicetable.h
+ module.h
+ mutex.h
+ types.h

...

> +static int ad485x_set_sampling_freq(struct ad485x_state *st, unsigned int freq)
> +{
> +	struct pwm_state cnv_state = {
> +		.duty_cycle = AD485X_T_CNVH_NS,
> +		.enabled = true,
> +	};
> +	int ret;
> +
> +	if (freq > st->info->throughput)
> +		freq = st->info->throughput;

clamp() ? (will need minmax.h)

> +	cnv_state.period = DIV_ROUND_CLOSEST_ULL(1000000000, freq);

We have time / frequency constants, like HZ_PER_..., NSEC_PER_....

> +	ret = pwm_apply_might_sleep(st->cnv, &cnv_state);
> +	if (ret)
> +		return ret;
> +
> +	st->sampling_freq = freq;
> +
> +	return 0;
> +}
> +
> +static int ad485x_setup(struct ad485x_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	ret = ad485x_set_sampling_freq(st, 1000000);

Ditto.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> +			   AD485X_SINGLE_INSTRUCTION);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> +	if (ret)
> +		return ret;
> +
> +	if (product_id != st->info->product_id)
> +		dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> +			 product_id);
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> +			   AD485X_ECHO_CLOCK_MODE);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}

...

> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> +	int i, cnt = 0, max_cnt = 0, start, max_start = 0;

Why are they signed?

> +	for (i = 0, start = -1; i < size; i++) {
> +		if (field[i] == 0) {
> +			if (start == -1)
> +				start = i;
> +			cnt++;
> +		} else {
> +			if (cnt > max_cnt) {
> +				max_cnt = cnt;
> +				max_start = start;
> +			}
> +			start = -1;
> +			cnt = 0;
> +		}
> +	}
> +
> +	if (cnt > max_cnt) {
> +		max_cnt = cnt;
> +		max_start = start;
> +	}
> +
> +	if (!max_cnt)
> +		return -EIO;
> +
> +	*ret_start = max_start;
> +
> +	return max_cnt;
> +}

...

> +static int ad485x_calibrate(struct ad485x_state *st)
> +{
> +	int opt_delay, lane_num, delay, i, s, c;

Why e.g. i is signed?

> +	enum iio_backend_interface_type interface_type;

> +	bool pn_status[AD485X_MAX_LANES][AD485X_MAX_IODELAY];

Why not a bitmap?

> +	int ret;

> +}

...

> +static const char *const ad4858_packet_fmts[] = {
> +	"20-bit", "24-bit", "32-bit"

Leave trailing comma, it may help extending the list in case it becomes a
multi-line.

> +};
> +
> +static const char *const ad4857_packet_fmts[] = {
> +	"16-bit", "24-bit"

Ditto.

> +};

...

> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +	unsigned int format;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> +	if (ret)
> +		return ret;

> +	format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> +	return format;

	return  FIELD_GET(...);

> +}

> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> +				 int *val2)

One line?

...

> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain = (reg_val & 0xFF) << 8;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain |= reg_val & 0xFF;

Why not bulk read and proper __le16/__be16 type?

...

> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> +				 int val2)
> +{
> +	unsigned long long gain;
> +	unsigned int reg_val;
> +	int ret;
> +
> +	gain = val * 1000000 + val2;

MICRO?

> +	gain = gain * 32768;
> +	gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);

Can be combined into one lien.

> +	reg_val = gain;

...

> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> +			   reg_val >> 8);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> +			    reg_val & 0xFF);

Bulk write?

> +}

...

> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> +			  &msb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> +			  &mid);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> +			  &lsb);
> +	if (ret)
> +		return ret;

Bulk read?

> +	if (st->info->resolution == 16) {
> +		*val = msb << 8;
> +		*val |= mid;
> +		*val = sign_extend32(*val, 15);

So, please use respective byteorder conversions...

> +	} else {
> +		*val = msb << 12;
> +		*val |= mid << 4;
> +		*val |= lsb >> 4;
> +		*val = sign_extend32(*val, 19);

...here as well (we have _be14()/_le24 ones).

> +	}

...

> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> +				int val2)
> +{
> +	unsigned int lsb, mid, msb;
> +	int ret;
> +
> +	if (st->info->resolution == 16) {
> +		lsb = 0;
> +		mid = val & 0xFF;
> +		msb = (val >> 8) & 0xFF;
> +	} else {
> +		lsb = (val << 4) & 0xFF;
> +		mid = (val >> 4) & 0xFF;
> +		msb = (val >> 12) & 0xFF;
> +	}

Ditto.

> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);

Bulk write?

> +}

...

> +static const unsigned int ad485x_scale_table[][2] = {
> +	{2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> +	{12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> +	{20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> +	{80000, 0xF}

It's more natural to list them in pow-of-two per line manner.
Moreover the last item is redundant. It's equal to the index. Why do you need it?

> +};

...

> +static const int ad4857_offset_table[] = {
> +	0, -32768

Here, and above and everywhere else in such cases leave a trailing comma.

> +};

...

> +	for (i = 0; i < info->num_scales; i++) {
> +		__ad485x_get_scale(st, info->scale_table[i][0],
> +				   &scale_val[0], &scale_val[1]);



> +		if (scale_val[0] != val || scale_val[1] != val2)
> +			continue;
> +
> +		/*
> +		 * Adjust the softspan value (differential or single ended)
> +		 * based on the scale value selected and current offset of
> +		 * the channel.
> +		 *
> +		 * If the offset is 0 then continue iterations until finding
> +		 * the next matching scale value which always corresponds to
> +		 * the single ended mode.
> +		 */
> +		if (!st->offsets[chan->channel] && !j) {
> +			j++;

So, j may be named properly and be boolean for the sake of the semantic usage.

> +			continue;
> +		}
> +
> +		return regmap_write(st->regmap,
> +				    AD485X_REG_CHX_SOFTSPAN(chan->channel),
> +				    info->scale_table[i][1]);
> +	}

...

> +{
> +	guard(mutex)(&st->lock);
> +
> +	if (st->offsets[chan->channel] != val) {

Invert and drop indentation for the next lines.

> +		st->offsets[chan->channel] = val;
> +		/* Restore to the default range if offset changes */
> +		if (st->offsets[chan->channel])
> +			return regmap_write(st->regmap,
> +						AD485X_REG_CHX_SOFTSPAN(chan->channel),
> +						AD485X_SOFTSPAN_N40V_40V);
> +		return regmap_write(st->regmap,
> +					AD485X_REG_CHX_SOFTSPAN(chan->channel),
> +					AD485X_SOFTSPAN_0V_40V);
> +	}
> +
> +	return 0;
> +}

...

> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->sampling_freq;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad485x_get_calibscale(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad485x_get_scale(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad485x_get_calibbias(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_OFFSET:
> +		scoped_guard(mutex, &st->lock)
> +			* val = st->offsets[chan->channel];

This is interesting white space location...

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +	struct ad485x_state *st = iio_priv(indio_dev);
> +	unsigned int c;
> +	int ret;
> +
> +	for (c = 0; c < st->info->num_channels; c++) {
> +		if (test_bit(c, scan_mask))

Do we have a helper now for this iterator?

> +			ret = iio_backend_chan_enable(st->back, c);
> +		else
> +			ret = iio_backend_chan_disable(st->back, c);
> +		if (ret)
> +			return ret;
> +	}

...

> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> +	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	IIO_ENUM_AVAILABLE("packet_format",
> +			   IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	{},

Please, drop comma in the terminator lines.

> +};

...

> +static const struct ad485x_chip_info ad4858i_info = {
> +	.name = "ad4858i",
> +	.product_id = AD4858I_PROD_ID,
> +	.scale_table = ad485x_scale_table,
> +	.num_scales = ARRAY_SIZE(ad485x_scale_table),
> +	.offset_table = ad4858_offset_table,
> +	.num_offset = ARRAY_SIZE(ad4858_offset_table),
> +	.channels = ad4858_channels,
> +	.num_channels = ARRAY_SIZE(ad4858_channels),
> +	.throughput = 1000000,

How many people wrote this patch? It has inconsistency at least in multipliers
like this all over the code. Who knows what's else also being inconsistent...

> +	.resolution = 20,
> +};

...

> +static const struct regmap_config regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.read_flag_mask = BIT(7),
> +};

Why do you need regmap lock?

...

> +static const char * const ad485x_power_supplies[] = {
> +	"vcc",	"vdd",	"vddh", "vio"

Leave trailing comma.

> +};

...

> +static int ad485x_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad485x_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;

> +	mutex_init(&st->lock);

Why not devm?

> +	ret = devm_regulator_bulk_get_enable(&spi->dev,
> +					     ARRAY_SIZE(ad485x_power_supplies),
> +					     ad485x_power_supplies);
> +	if (ret)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "failed to get and enable supplies\n");
> +
> +	st->cnv = devm_pwm_get(&spi->dev, NULL);
> +	if (IS_ERR(st->cnv))
> +		return PTR_ERR(st->cnv);
> +
> +	st->info = spi_get_device_match_data(spi);
> +	if (!st->info)
> +		return -ENODEV;
> +
> +	st->regmap = devm_regmap_init_spi(spi, &regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	ret = ad485x_scale_offset_fill(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad485x_setup(st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->info->name;
> +	indio_dev->channels = st->info->channels;
> +	indio_dev->num_channels = st->info->num_channels;
> +	indio_dev->info = &ad485x_info;
> +
> +	st->back = devm_iio_backend_get(&spi->dev, NULL);
> +	if (IS_ERR(st->back))
> +		return PTR_ERR(st->back);
> +
> +	ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_iio_backend_enable(&spi->dev, st->back);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad485x_calibrate(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[kernel test robot]

Hi Antoniu,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.11]
[cannot apply to jic23-iio/togreg next-20240924]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Antoniu-Miclaus/iio-backend-add-API-for-interface-get/20240923-182050
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20240923101206.3753-7-antoniu.miclaus%40analog.com
patch subject: [PATCH 6/7] iio: adc: ad485x: add ad485x driver
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240924/202409242353.rDAcuGYR-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240924/202409242353.rDAcuGYR-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409242353.rDAcuGYR-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/iio/adc/ad485x.c: In function 'ad485x_get_packet_format':
>> drivers/iio/adc/ad485x.c:396:18: error: implicit declaration of function 'FIELD_GET' [-Wimplicit-function-declaration]
     396 |         format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
         |                  ^~~~~~~~~
   drivers/iio/adc/ad485x.c: At top level:
   drivers/iio/adc/ad485x.c:854:23: warning: initialized field overwritten [-Woverride-init]
     854 |         .resolution = 16,
         |                       ^~
   drivers/iio/adc/ad485x.c:854:23: note: (near initialization for 'ad4856_info.resolution')


vim +/FIELD_GET +396 drivers/iio/adc/ad485x.c

   384	
   385	static int ad485x_get_packet_format(struct iio_dev *indio_dev,
   386					    const struct iio_chan_spec *chan)
   387	{
   388		struct ad485x_state *st = iio_priv(indio_dev);
   389		unsigned int format;
   390		int ret;
   391	
   392		ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
   393		if (ret)
   394			return ret;
   395	
 > 396		format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
   397	
   398		return format;
   399	}
   400	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[David Lechner]

On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
<antoniu.miclaus@analog.com> wrote:
>
> Add support for the AD485X DAS familiy.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---

...

> +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> +{
> +       int i, cnt = 0, max_cnt = 0, start, max_start = 0;
> +
> +       for (i = 0, start = -1; i < size; i++) {
> +               if (field[i] == 0) {
> +                       if (start == -1)
> +                               start = i;
> +                       cnt++;
> +               } else {
> +                       if (cnt > max_cnt) {
> +                               max_cnt = cnt;
> +                               max_start = start;
> +                       }
> +                       start = -1;
> +                       cnt = 0;
> +               }
> +       }
> +
> +       if (cnt > max_cnt) {
> +               max_cnt = cnt;
> +               max_start = start;
> +       }
> +
> +       if (!max_cnt)
> +               return -EIO;

EIO seems an odd choice since this function doesn't actually do any
I/O. Maybe EINVAL would be better?

> +
> +       *ret_start = max_start;
> +
> +       return max_cnt;
> +}
> +

...

> +static int ad485x_set_calibscale(struct ad485x_state *st, int ch, int val,
> +                                int val2)
> +{
> +       unsigned long long gain;
> +       unsigned int reg_val;
> +       int ret;
> +

Need to check both val and val2 for negative here before converting to unsigned.

if (val < 0 || val2 < 0)
        return -EINVAL;

> +       gain = val * 1000000 + val2;
> +       gain = gain * 32768;
> +       gain = DIV_U64_ROUND_CLOSEST(gain, 1000000);
> +
> +       reg_val = gain;
> +
> +       guard(mutex)(&st->lock);
> +
> +       ret = regmap_write(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> +                          reg_val >> 8);
> +       if (ret)
> +               return ret;
> +
> +       return regmap_write(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> +                           reg_val & 0xFF);
> +}
> +
> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> +                               int *val2)
> +{

val2 is unused and can be removed

> +       unsigned int lsb, mid, msb;
> +       int ret;
> +
> +       guard(mutex)(&st->lock);
> +
> +       ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> +                         &msb);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> +                         &mid);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> +                         &lsb);
> +       if (ret)
> +               return ret;
> +
> +       if (st->info->resolution == 16) {
> +               *val = msb << 8;
> +               *val |= mid;
> +               *val = sign_extend32(*val, 15);
> +       } else {
> +               *val = msb << 12;
> +               *val |= mid << 4;
> +               *val |= lsb >> 4;
> +               *val = sign_extend32(*val, 19);
> +       }
> +
> +       return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> +                               int val2)
> +{

val2 is unused here. It would also be a good idea to implement the
write_raw_get_fmt callback to select IIO_VAL_INT for this attribute to
avoid having to deal with negative val2.

> +       unsigned int lsb, mid, msb;
> +       int ret;

Should check for negative val here before converting to unsigned.

> +
> +       if (st->info->resolution == 16) {
> +               lsb = 0;
> +               mid = val & 0xFF;
> +               msb = (val >> 8) & 0xFF;
> +       } else {
> +               lsb = (val << 4) & 0xFF;
> +               mid = (val >> 4) & 0xFF;
> +               msb = (val >> 12) & 0xFF;
> +       }
> +
> +       guard(mutex)(&st->lock);
> +
> +       ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> +       if (ret)
> +               return ret;
> +
> +       return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);
> +}
> +

...

> +static int ad485x_set_offset(struct ad485x_state *st,
> +                            const struct iio_chan_spec *chan, int val)
> +{
> +       guard(mutex)(&st->lock);
> +
> +       if (st->offsets[chan->channel] != val) {
> +               st->offsets[chan->channel] = val;
> +               /* Restore to the default range if offset changes */
> +               if (st->offsets[chan->channel])
> +                       return regmap_write(st->regmap,
> +                                               AD485X_REG_CHX_SOFTSPAN(chan->channel),
> +                                               AD485X_SOFTSPAN_N40V_40V);
> +               return regmap_write(st->regmap,
> +                                       AD485X_REG_CHX_SOFTSPAN(chan->channel),
> +                                       AD485X_SOFTSPAN_0V_40V);
> +       }
> +
> +       return 0;
> +}

I'm not sure I understand the relationship between softspan and the
offset. A raw value of 0 always means we measured 0V no matter what
the softspan setting is. So it seems like the offset should always be
0.

I'm guessing the intent was to handle bipolar vs. unipolar softspans,
but this doesn't actually work mathematically.

So far, I've only seen inputs that can be bipolar or unipolar
specified in the devicetree. I'm not aware of a way to select this at
runtime.

> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +       IIO_ENUM_AVAILABLE("packet_format",
> +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +       {},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +       IIO_ENUM_AVAILABLE("packet_format",
> +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +       {},
> +};

Usually, something like this packet format would be automatically
selected when buffered reads are enabled based on what other features
it provides are needed, i.e only enable the status bits when events
are enabled.

(For those that didn't read the datasheet, the different packet
formats basically enable extra status bits per sample. And in the case
of oversampling, one of the formats also selects a reduced number of
sample bits.)

We have quite a few parts in the pipline right like this one that have
per-sample status bits. In the past, these were generally handled with
IIO events, but this doesn't really work for these high-speed backends
since the data is being piped directly to DMA and we don't look at
each sample in the ADC driver. So it would be worthwhile to try to
find some general solution here for handling this sort of thing.

> +
> +static const struct iio_chan_spec ad4858_channels[] = {
> +       AD485X_IIO_CHANNEL(0, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(1, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(2, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(3, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(4, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(5, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(6, 20, 32, ad4858_ext_info),
> +       AD485X_IIO_CHANNEL(7, 20, 32, ad4858_ext_info),
> +};
> +
> +static const struct iio_chan_spec ad4857_channels[] = {
> +       AD485X_IIO_CHANNEL(0, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(1, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(2, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(3, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(4, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(5, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(6, 16, 16, ad4857_ext_info),
> +       AD485X_IIO_CHANNEL(7, 16, 16, ad4857_ext_info),
> +};

How does 16-bit storage size work when packet format is 24-bit?

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Andy Shevchenko]

On Thu, Sep 26, 2024 at 04:39:18PM +0200, David Lechner wrote:
> On Mon, Sep 23, 2024 at 12:17 PM Antoniu Miclaus
> <antoniu.miclaus@analog.com> wrote:

...

> > +static int ad485x_find_opt(bool *field, u32 size, u32 *ret_start)
> > +{
> > +       int i, cnt = 0, max_cnt = 0, start, max_start = 0;
> > +
> > +       for (i = 0, start = -1; i < size; i++) {
> > +               if (field[i] == 0) {
> > +                       if (start == -1)
> > +                               start = i;
> > +                       cnt++;
> > +               } else {
> > +                       if (cnt > max_cnt) {
> > +                               max_cnt = cnt;
> > +                               max_start = start;
> > +                       }
> > +                       start = -1;
> > +                       cnt = 0;
> > +               }
> > +       }
> > +
> > +       if (cnt > max_cnt) {
> > +               max_cnt = cnt;
> > +               max_start = start;
> > +       }
> > +
> > +       if (!max_cnt)
> > +               return -EIO;
> 
> EIO seems an odd choice since this function doesn't actually do any
> I/O. Maybe EINVAL would be better?

I would even go with -ENOENT as function called 'find'.

> > +       *ret_start = max_start;
> > +
> > +       return max_cnt;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron]

> 
> > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > +       IIO_ENUM_AVAILABLE("packet_format",
> > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > +       {},
> > +};
> > +
> > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > +       IIO_ENUM_AVAILABLE("packet_format",
> > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > +       {},
> > +};  
> 
> Usually, something like this packet format would be automatically
> selected when buffered reads are enabled based on what other features
> it provides are needed, i.e only enable the status bits when events
> are enabled.
> 
> (For those that didn't read the datasheet, the different packet
> formats basically enable extra status bits per sample. And in the case
> of oversampling, one of the formats also selects a reduced number of
> sample bits.)
> 
> We have quite a few parts in the pipline right like this one that have
> per-sample status bits. In the past, these were generally handled with
> IIO events, but this doesn't really work for these high-speed backends
> since the data is being piped directly to DMA and we don't look at
> each sample in the ADC driver. So it would be worthwhile to try to
> find some general solution here for handling this sort of thing.

We have previously talked about schemes to describe metadata
alongside channels. I guess maybe it's time to actually look at how
that works.  I'm not sure dynamic control of that metadata
is going to be easy to do though or if we even want to
(as opposed to always on or off for a particular device).

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Andy Shevchenko]

On Sat, Sep 28, 2024 at 8:30 PM Jonathan Cameron <jic23@kernel.org> wrote:

...

> > We have quite a few parts in the pipline right like this one that have
> > per-sample status bits. In the past, these were generally handled with
> > IIO events, but this doesn't really work for these high-speed backends
> > since the data is being piped directly to DMA and we don't look at
> > each sample in the ADC driver. So it would be worthwhile to try to
> > find some general solution here for handling this sort of thing.
>
> We have previously talked about schemes to describe metadata
> alongside channels. I guess maybe it's time to actually look at how
> that works.  I'm not sure dynamic control of that metadata
> is going to be easy to do though or if we even want to
> (as opposed to always on or off for a particular device).

Time for the kernel to return JSON on a per channel basis?

Just saying :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Nuno Sá]

On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> 
> > 
> > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > +       {},
> > > +};
> > > +
> > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > +       {},
> > > +};  
> > 
> > Usually, something like this packet format would be automatically
> > selected when buffered reads are enabled based on what other features
> > it provides are needed, i.e only enable the status bits when events
> > are enabled.
> > 
> > (For those that didn't read the datasheet, the different packet
> > formats basically enable extra status bits per sample. And in the case
> > of oversampling, one of the formats also selects a reduced number of
> > sample bits.)
> > 
> > We have quite a few parts in the pipline right like this one that have
> > per-sample status bits. In the past, these were generally handled with
> > IIO events, but this doesn't really work for these high-speed backends
> > since the data is being piped directly to DMA and we don't look at
> > each sample in the ADC driver. So it would be worthwhile to try to
> > find some general solution here for handling this sort of thing.

I did not read the datasheet that extensively but here it goes my 2 cents
(basically my internal feedback on this one):

So this packet format thingy may be a very "funny" discussion if we really need
to support it. I'm not sure how useful it is the 32 bits format rather than
being used in test pattern. I'm not seeing too much benefit on the channel id or
span id information (we can already get that info with other attributes). The
OR/UR is the one that could be more useful but is there someone using it? Do we
really need to have it close to the sample? If not, there's the status register
and... Also, I think this can be implemented using IIO events (likely what we
will be asked). So what comes to mind could be:

For test_pattern (could be implemented as ext_info or an additional channel I
think - not for now I guess) we can easily look at our word side and dynamically
set the proper packet size. So, to me, this is effectively the only place where
32bits would make sense (assuming we don't implement OR/UR for now).
For oversampling we can have both 20/24 bit averaged data. But from the
datasheet:

"Oversampling is useful in applications requiring lower noise and higher dynamic
range per output data-word, which the AD4858 supports with 24-bit output
resolution and reduced average output data rates"

So from the above it looks like it could make sense to default to 24bit packets
if oversampling is enabled.

Now the question is OR/UR. If that is something we can support with events, we
could see when one of OR/UR is being requested to either enable 24 packets (no
oversampling) or 32 bit packets (oversampling on).



> 
> We have previously talked about schemes to describe metadata
> alongside channels. I guess maybe it's time to actually look at how
> that works.  I'm not sure dynamic control of that metadata
> is going to be easy to do though or if we even want to
> (as opposed to always on or off for a particular device).
> 

Indeed this is something we have been discussing and the ability to have status
alongside a buffered samples is starting to be requested more and more. Some
parts do have the status bit alongside the sample (meaning in the same register
read) which means it basically goes with the sample as part of it's
storage_bits. While not ideal, an application caring about those bits still has
access to the complete raw sample and can access them. It gets more complicated
where the status (sometimes a per device status register) is located in another
register. I guess we can have two case:

1) A device status which applies for all channels being sampled;
2) A per channel status (where the .metada approach could make sense).

But I'm not sure how we could define something like this other than assuming
that raw status data is being sent to userspace (given that every device has
it's own custom status bits and quirks).

- Nuno Sá  

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron]

On Mon, 30 Sep 2024 09:05:04 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> >   
> > >   
> > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > +       {},
> > > > +};
> > > > +
> > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > +       {},
> > > > +};    
> > > 
> > > Usually, something like this packet format would be automatically
> > > selected when buffered reads are enabled based on what other features
> > > it provides are needed, i.e only enable the status bits when events
> > > are enabled.
> > > 
> > > (For those that didn't read the datasheet, the different packet
> > > formats basically enable extra status bits per sample. And in the case
> > > of oversampling, one of the formats also selects a reduced number of
> > > sample bits.)
> > > 
> > > We have quite a few parts in the pipline right like this one that have
> > > per-sample status bits. In the past, these were generally handled with
> > > IIO events, but this doesn't really work for these high-speed backends
> > > since the data is being piped directly to DMA and we don't look at
> > > each sample in the ADC driver. So it would be worthwhile to try to
> > > find some general solution here for handling this sort of thing.  
> 
> I did not read the datasheet that extensively but here it goes my 2 cents
> (basically my internal feedback on this one):
> 
> So this packet format thingy may be a very "funny" discussion if we really need
> to support it. I'm not sure how useful it is the 32 bits format rather than
> being used in test pattern. I'm not seeing too much benefit on the channel id or
> span id information (we can already get that info with other attributes). The
> OR/UR is the one that could be more useful but is there someone using it? Do we
> really need to have it close to the sample? If not, there's the status register
> and... Also, I think this can be implemented using IIO events (likely what we
> will be asked). So what comes to mind could be:

Definite preference for using events, but for a device doing DMA I'm not sure
how we can do that without requiring parsing all the data.

So we would need some metadata description to know it is there.

> 
> For test_pattern (could be implemented as ext_info or an additional channel I
> think - not for now I guess) we can easily look at our word side and dynamically
> set the proper packet size. So, to me, this is effectively the only place where
> 32bits would make sense (assuming we don't implement OR/UR for now).
> For oversampling we can have both 20/24 bit averaged data. But from the
> datasheet:
> 
> "Oversampling is useful in applications requiring lower noise and higher dynamic
> range per output data-word, which the AD4858 supports with 24-bit output
> resolution and reduced average output data rates"
> 
> So from the above it looks like it could make sense to default to 24bit packets
> if oversampling is enabled.

That sounds like what we do for the DMA oversampling cases that change
the resolutions.

> 
> Now the question is OR/UR. If that is something we can support with events, we
> could see when one of OR/UR is being requested to either enable 24 packets (no
> oversampling) or 32 bit packets (oversampling on).
> 
> 
> 
> > 
> > We have previously talked about schemes to describe metadata
> > alongside channels. I guess maybe it's time to actually look at how
> > that works.  I'm not sure dynamic control of that metadata
> > is going to be easy to do though or if we even want to
> > (as opposed to always on or off for a particular device).
> >   
> 
> Indeed this is something we have been discussing and the ability to have status
> alongside a buffered samples is starting to be requested more and more. Some
> parts do have the status bit alongside the sample (meaning in the same register
> read) which means it basically goes with the sample as part of it's
> storage_bits. While not ideal, an application caring about those bits still has
> access to the complete raw sample and can access them. 

This has the advantage that if we come along later and define a metadata
in storage bits description it is backwards compatible.  We've been doing
this for years with some devices.

> It gets more complicated
> where the status (sometimes a per device status register) is located in another
> register. I guess we can have two case:
> 
> 1) A device status which applies for all channels being sampled;
> 2) A per channel status (where the .metada approach could make sense).

If it's a separate register per channel and optional, we'd have to treat it as a metadata
channel as no guarantee it would be packed next to the main channel.

If we have a description of metadata additions in main channel storage, I'm not
against having a IIO_METADATA channel type. 

If it's a single channel I'm not sure how we'd make as channel description
general enough easily as we end up with every field possibly needed an association
with a different channel.

> 
> But I'm not sure how we could define something like this other than assuming
> that raw status data is being sent to userspace (given that every device has
> it's own custom status bits and quirks).
That is always fine.

Jonathan
> 
> - Nuno Sá  

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Nuno Sá]

On Sat, 2024-10-05 at 18:26 +0100, Jonathan Cameron wrote:
> On Mon, 30 Sep 2024 09:05:04 +0200
> Nuno Sá <noname.nuno@gmail.com> wrote:
> 
> > On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:
> > >   
> > > >   
> > > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > &ad4858_packet_fmt),
> > > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > > +       {},
> > > > > +};
> > > > > +
> > > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > &ad4857_packet_fmt),
> > > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > > +       {},
> > > > > +};    
> > > > 
> > > > Usually, something like this packet format would be automatically
> > > > selected when buffered reads are enabled based on what other features
> > > > it provides are needed, i.e only enable the status bits when events
> > > > are enabled.
> > > > 
> > > > (For those that didn't read the datasheet, the different packet
> > > > formats basically enable extra status bits per sample. And in the case
> > > > of oversampling, one of the formats also selects a reduced number of
> > > > sample bits.)
> > > > 
> > > > We have quite a few parts in the pipline right like this one that have
> > > > per-sample status bits. In the past, these were generally handled with
> > > > IIO events, but this doesn't really work for these high-speed backends
> > > > since the data is being piped directly to DMA and we don't look at
> > > > each sample in the ADC driver. So it would be worthwhile to try to
> > > > find some general solution here for handling this sort of thing.  
> > 
> > I did not read the datasheet that extensively but here it goes my 2 cents
> > (basically my internal feedback on this one):
> > 
> > So this packet format thingy may be a very "funny" discussion if we really
> > need
> > to support it. I'm not sure how useful it is the 32 bits format rather than
> > being used in test pattern. I'm not seeing too much benefit on the channel
> > id or
> > span id information (we can already get that info with other attributes).
> > The
> > OR/UR is the one that could be more useful but is there someone using it? Do
> > we
> > really need to have it close to the sample? If not, there's the status
> > register
> > and... Also, I think this can be implemented using IIO events (likely what
> > we
> > will be asked). So what comes to mind could be:
> 
> Definite preference for using events, but for a device doing DMA I'm not sure
> how we can do that without requiring parsing all the data.
> 
> So we would need some metadata description to know it is there.
> 
> > 
> > For test_pattern (could be implemented as ext_info or an additional channel
> > I
> > think - not for now I guess) we can easily look at our word side and
> > dynamically
> > set the proper packet size. So, to me, this is effectively the only place
> > where
> > 32bits would make sense (assuming we don't implement OR/UR for now).
> > For oversampling we can have both 20/24 bit averaged data. But from the
> > datasheet:
> > 
> > "Oversampling is useful in applications requiring lower noise and higher
> > dynamic
> > range per output data-word, which the AD4858 supports with 24-bit output
> > resolution and reduced average output data rates"
> > 
> > So from the above it looks like it could make sense to default to 24bit
> > packets
> > if oversampling is enabled.
> 
> That sounds like what we do for the DMA oversampling cases that change
> the resolutions.
> 
> > 
> > Now the question is OR/UR. If that is something we can support with events,
> > we
> > could see when one of OR/UR is being requested to either enable 24 packets
> > (no
> > oversampling) or 32 bit packets (oversampling on).
> > 
> > 
> > 
> > > 
> > > We have previously talked about schemes to describe metadata
> > > alongside channels. I guess maybe it's time to actually look at how
> > > that works.  I'm not sure dynamic control of that metadata
> > > is going to be easy to do though or if we even want to
> > > (as opposed to always on or off for a particular device).
> > >   
> > 
> > Indeed this is something we have been discussing and the ability to have
> > status
> > alongside a buffered samples is starting to be requested more and more. Some
> > parts do have the status bit alongside the sample (meaning in the same
> > register
> > read) which means it basically goes with the sample as part of it's
> > storage_bits. While not ideal, an application caring about those bits still
> > has
> > access to the complete raw sample and can access them. 
> 
> This has the advantage that if we come along later and define a metadata
> in storage bits description it is backwards compatible.  We've been doing
> this for years with some devices.
> 
> > It gets more complicated
> > where the status (sometimes a per device status register) is located in
> > another
> > register. I guess we can have two case:
> > 
> > 1) A device status which applies for all channels being sampled;
> > 2) A per channel status (where the .metada approach could make sense).
> 
> If it's a separate register per channel and optional, we'd have to treat it as
> a metadata
> channel as no guarantee it would be packed next to the main channel.
> 
> If we have a description of metadata additions in main channel storage, I'm
> not
> against having a IIO_METADATA channel type. 
> 

I guess you mean that a complete solution would never only be a IIO_METADATA but
also extending 'struct iio_scan_type'?

> If it's a single channel I'm not sure how we'd make as channel description
> general enough easily as we end up with every field possibly needed an
> association
> with a different channel.

Not sure I followed the above... You mean having a single channel (like one
register) pointing to different channels? 

What I mean with 1) is for example what happens with IMUs (eg: adis16475). They
have a DIAG_STAT register (which is pretty much device wide status/error
information) that's also part of burst transfers (where we get our samples) and
while some bits might not be meaningful for the sampling "session", others might
make sense to reason about data integrity or correctness. 

For the above case, I think the IIO_METADATA channel type would fit.

- Nuno Sá

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron]

On Fri, 11 Oct 2024 12:23:49 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Sat, 2024-10-05 at 18:26 +0100, Jonathan Cameron wrote:
> > On Mon, 30 Sep 2024 09:05:04 +0200
> > Nuno Sá <noname.nuno@gmail.com> wrote:
> >   
> > > On Sat, 2024-09-28 at 18:30 +0100, Jonathan Cameron wrote:  
> > > >     
> > > > >     
> > > > > > +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> > > > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > > &ad4858_packet_fmt),
> > > > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > > > +                          IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> > > > > > +       {},
> > > > > > +};
> > > > > > +
> > > > > > +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> > > > > > +       IIO_ENUM("packet_format", IIO_SHARED_BY_ALL,
> > > > > > &ad4857_packet_fmt),
> > > > > > +       IIO_ENUM_AVAILABLE("packet_format",
> > > > > > +                          IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> > > > > > +       {},
> > > > > > +};      
> > > > > 
> > > > > Usually, something like this packet format would be automatically
> > > > > selected when buffered reads are enabled based on what other features
> > > > > it provides are needed, i.e only enable the status bits when events
> > > > > are enabled.
> > > > > 
> > > > > (For those that didn't read the datasheet, the different packet
> > > > > formats basically enable extra status bits per sample. And in the case
> > > > > of oversampling, one of the formats also selects a reduced number of
> > > > > sample bits.)
> > > > > 
> > > > > We have quite a few parts in the pipline right like this one that have
> > > > > per-sample status bits. In the past, these were generally handled with
> > > > > IIO events, but this doesn't really work for these high-speed backends
> > > > > since the data is being piped directly to DMA and we don't look at
> > > > > each sample in the ADC driver. So it would be worthwhile to try to
> > > > > find some general solution here for handling this sort of thing.    
> > > 
> > > I did not read the datasheet that extensively but here it goes my 2 cents
> > > (basically my internal feedback on this one):
> > > 
> > > So this packet format thingy may be a very "funny" discussion if we really
> > > need
> > > to support it. I'm not sure how useful it is the 32 bits format rather than
> > > being used in test pattern. I'm not seeing too much benefit on the channel
> > > id or
> > > span id information (we can already get that info with other attributes).
> > > The
> > > OR/UR is the one that could be more useful but is there someone using it? Do
> > > we
> > > really need to have it close to the sample? If not, there's the status
> > > register
> > > and... Also, I think this can be implemented using IIO events (likely what
> > > we
> > > will be asked). So what comes to mind could be:  
> > 
> > Definite preference for using events, but for a device doing DMA I'm not sure
> > how we can do that without requiring parsing all the data.
> > 
> > So we would need some metadata description to know it is there.
> >   
> > > 
> > > For test_pattern (could be implemented as ext_info or an additional channel
> > > I
> > > think - not for now I guess) we can easily look at our word side and
> > > dynamically
> > > set the proper packet size. So, to me, this is effectively the only place
> > > where
> > > 32bits would make sense (assuming we don't implement OR/UR for now).
> > > For oversampling we can have both 20/24 bit averaged data. But from the
> > > datasheet:
> > > 
> > > "Oversampling is useful in applications requiring lower noise and higher
> > > dynamic
> > > range per output data-word, which the AD4858 supports with 24-bit output
> > > resolution and reduced average output data rates"
> > > 
> > > So from the above it looks like it could make sense to default to 24bit
> > > packets
> > > if oversampling is enabled.  
> > 
> > That sounds like what we do for the DMA oversampling cases that change
> > the resolutions.
> >   
> > > 
> > > Now the question is OR/UR. If that is something we can support with events,
> > > we
> > > could see when one of OR/UR is being requested to either enable 24 packets
> > > (no
> > > oversampling) or 32 bit packets (oversampling on).
> > > 
> > > 
> > >   
> > > > 
> > > > We have previously talked about schemes to describe metadata
> > > > alongside channels. I guess maybe it's time to actually look at how
> > > > that works.  I'm not sure dynamic control of that metadata
> > > > is going to be easy to do though or if we even want to
> > > > (as opposed to always on or off for a particular device).
> > > >     
> > > 
> > > Indeed this is something we have been discussing and the ability to have
> > > status
> > > alongside a buffered samples is starting to be requested more and more. Some
> > > parts do have the status bit alongside the sample (meaning in the same
> > > register
> > > read) which means it basically goes with the sample as part of it's
> > > storage_bits. While not ideal, an application caring about those bits still
> > > has
> > > access to the complete raw sample and can access them.   
> > 
> > This has the advantage that if we come along later and define a metadata
> > in storage bits description it is backwards compatible.  We've been doing
> > this for years with some devices.
> >   
> > > It gets more complicated
> > > where the status (sometimes a per device status register) is located in
> > > another
> > > register. I guess we can have two case:
> > > 
> > > 1) A device status which applies for all channels being sampled;
> > > 2) A per channel status (where the .metada approach could make sense).  
> > 
> > If it's a separate register per channel and optional, we'd have to treat it as
> > a metadata
> > channel as no guarantee it would be packed next to the main channel.
> > 
> > If we have a description of metadata additions in main channel storage, I'm
> > not
> > against having a IIO_METADATA channel type. 
> >   
> 
> I guess you mean that a complete solution would never only be a IIO_METADATA but
> also extending 'struct iio_scan_type'?

Yes we needs a way to refer to an existing scan element then we could add additional
channels and refer into them as needed.

> 
> > If it's a single channel I'm not sure how we'd make as channel description
> > general enough easily as we end up with every field possibly needed an
> > association
> > with a different channel.  
> 
> Not sure I followed the above... You mean having a single channel (like one
> register) pointing to different channels? 

Both way's around occur.  Multiple channels, some normal, some metadata some
separate pointing to a single storage location and per channel meta data
for different 'signals' shoved in one register.

> 
> What I mean with 1) is for example what happens with IMUs (eg: adis16475). They
> have a DIAG_STAT register (which is pretty much device wide status/error
> information) that's also part of burst transfers (where we get our samples) and
> while some bits might not be meaningful for the sampling "session", others might
> make sense to reason about data integrity or correctness. 
> 
> For the above case, I think the IIO_METADATA channel type would fit.
That one is easy. Fiddly case is where we have say overflow bits for
multiple signals (i.e. pins) in a single dmabuffer element.
To make that work cleanly we need a way to not only describe the contents
but cross reference it to the related data.

We've discussed ways to group actual channel (i.e. current and power on same pin)
in the past but doing this for metadata that is packed in multiple channels
is going to be a real pain.

Basically it all needs to be very flexible and any attempt to support a subset
is likely to wall us into an inflexible ABI.

Jonathan

> 
> - Nuno Sá

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron]

On Mon, 23 Sep 2024 13:10:23 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add support for the AD485X DAS familiy.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

A few additional comments from me.

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ad485x.c b/drivers/iio/adc/ad485x.c
> new file mode 100644
> index 000000000000..3333cad9ed8f
> --- /dev/null
> +++ b/drivers/iio/adc/ad485x.c
> @@ -0,0 +1,1061 @@

> +static int ad485x_setup(struct ad485x_state *st)
> +{
> +	unsigned int product_id;
> +	int ret;
> +
> +	ret = ad485x_set_sampling_freq(st, 1000000);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SW_RESET);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_B,
> +			   AD485X_SINGLE_INSTRUCTION);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_INTERFACE_CONFIG_A,
> +			   AD485X_SDO_ENABLE);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PRODUCT_ID_L, &product_id);
> +	if (ret)
> +		return ret;
> +
> +	if (product_id != st->info->product_id)
> +		dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n",
> +			 product_id);

dev_info() for this is probably better.  Might be fine afterall if
the new part is fully backwards compatible with this one.

> +
> +	ret = regmap_write(st->regmap, AD485X_REG_DEVICE_CTRL,
> +			   AD485X_ECHO_CLOCK_MODE);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_PACKET, 0);
> +}


> +
> +static int ad485x_get_packet_format(struct iio_dev *indio_dev,
> +				    const struct iio_chan_spec *chan)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +	unsigned int format;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_PACKET, &format);
> +	if (ret)
> +		return ret;
> +
> +	format = FIELD_GET(AD485X_PACKET_FORMAT_MASK, format);
> +
> +	return format;
	return FIELD_GET()
> +}

> +
> +static int ad485x_get_calibscale(struct ad485x_state *st, int ch, int *val,
> +				 int *val2)
> +{
> +	unsigned int reg_val;
> +	int gain;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_MSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain = (reg_val & 0xFF) << 8;
Use a byte array into which you write the regval then a get_unalignedb_be16
or similar to get the value.
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch),
> +			  &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	gain |= reg_val & 0xFF;
> +
> +	*val = gain;
> +	*val2 = 32768;
> +
> +	return IIO_VAL_FRACTIONAL;
> +}

> +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> +				int *val2)
> +{
> +	unsigned int lsb, mid, msb;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> +			  &msb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> +			  &mid);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> +			  &lsb);
> +	if (ret)
> +		return ret;
> +
> +	if (st->info->resolution == 16) {
> +		*val = msb << 8;
> +		*val |= mid;
> +		*val = sign_extend32(*val, 15);
> +	} else {
> +		*val = msb << 12;
> +		*val |= mid << 4;
> +		*val |= lsb >> 4;
As below I'd use a byte array then you can do get_unaligned_be24 to
+ a right shift by 4 of the whole thing.

> +		*val = sign_extend32(*val, 19);
> +	}
> +
> +	return IIO_VAL_INT;
> +}
> +
> +static int ad485x_set_calibbias(struct ad485x_state *st, int ch, int val,
> +				int val2)
> +{
> +	unsigned int lsb, mid, msb;
> +	int ret;
> +
> +	if (st->info->resolution == 16) {
> +		lsb = 0;
> +		mid = val & 0xFF;
> +		msb = (val >> 8) & 0xFF;
> +	} else {
> +		lsb = (val << 4) & 0xFF;

Might as well mask by 0xF0 to make it really clear
nothing in bottom few bits.


> +		mid = (val >> 4) & 0xFF;
> +		msb = (val >> 12) & 0xFF;

I'd be tempted to shift the whole thing left 4 bits and
then use a put_unaligned_be24 on it into a byte array then pick
out the bytes from that array.  Will be easier to read.
Above 16 bit case would be a put_unaligned_be16

> +	}
> +
> +	guard(mutex)(&st->lock);
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch), lsb);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch), mid);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb);

I think you already got this question, but consider bulk writes.

> +}
> +
> +static const unsigned int ad485x_scale_table[][2] = {
> +	{2500, 0x0}, {5000, 0x1}, {5000, 0x2}, {10000, 0x3}, {6250, 0x04},
> +	{12500, 0x5}, {10000, 0x6}, {20000, 0x7}, {12500, 0x8}, {25000, 0x9},
> +	{20000, 0xA}, {40000, 0xB}, {25000, 0xC}, {50000, 0xD}, {40000, 0xE},
> +	{80000, 0xF}

Spaces after { and before } preferred.
Maybe just have this as one item per line. I think that is more readable.

> +};

> +
> +static int ad485x_read_raw(struct iio_dev *indio_dev,
> +			   const struct iio_chan_spec *chan,
> +			   int *val, int *val2, long info)
> +{
> +	struct ad485x_state *st = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		*val = st->sampling_freq;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		return ad485x_get_calibscale(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return ad485x_get_scale(st, chan, val, val2);
> +	case IIO_CHAN_INFO_CALIBBIAS:
> +		return ad485x_get_calibbias(st, chan->channel, val, val2);
> +	case IIO_CHAN_INFO_OFFSET:
> +		scoped_guard(mutex, &st->lock)
> +			* val = st->offsets[chan->channel];

			*val = ...

> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +#define AD485X_IIO_CHANNEL(index, real, storage, info)			\
> +{									\
> +	.type = IIO_VOLTAGE,						\
> +	.ext_info = info,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) |		\
> +		BIT(IIO_CHAN_INFO_CALIBBIAS) |				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET),				\
> +	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ),	\
> +	.info_mask_shared_by_type_available =				\
> +		BIT(IIO_CHAN_INFO_SCALE) |				\
> +		BIT(IIO_CHAN_INFO_OFFSET),				\

Maybe add avail for calibscale and calibbias as well.


> +
> +static struct iio_chan_spec_ext_info ad4858_ext_info[] = {
> +	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	IIO_ENUM_AVAILABLE("packet_format",
> +			   IIO_SHARED_BY_ALL, &ad4858_packet_fmt),
> +	{},
> +};
> +
> +static struct iio_chan_spec_ext_info ad4857_ext_info[] = {
> +	IIO_ENUM("packet_format", IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +	IIO_ENUM_AVAILABLE("packet_format",
> +			   IIO_SHARED_BY_ALL, &ad4857_packet_fmt),
> +	{},
I think Andy pointed these out. No trailing comma on terminators.

However, I'm not sure this ABI is practical currently.
Basically I have no idea what it is or how to use it!
> +};

> +
> +static const struct ad485x_chip_info ad4858i_info = {
> +	.name = "ad4858i",
> +	.product_id = AD4858I_PROD_ID,
> +	.scale_table = ad485x_scale_table,
> +	.num_scales = ARRAY_SIZE(ad485x_scale_table),
> +	.offset_table = ad4858_offset_table,
> +	.num_offset = ARRAY_SIZE(ad4858_offset_table),
> +	.channels = ad4858_channels,
> +	.num_channels = ARRAY_SIZE(ad4858_channels),
> +	.throughput = 1000000,

Odd you use MEGA above, but not here.

> +	.resolution = 20,
> +};

> +static int ad485x_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ad485x_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->spi = spi;
> +
> +	mutex_init(&st->lock);

Ever so slight preference for the new option of.
	ret = devm_mutex_init(dev, &st->lock);
	if (ret)
		return ret;

...

> +}
> +
> +static const struct of_device_id ad485x_of_match[] = {
> +	{ .compatible = "adi,ad4858", .data = &ad4858_info, },
> +	{ .compatible = "adi,ad4857", .data = &ad4857_info, },
> +	{ .compatible = "adi,ad4856", .data = &ad4856_info, },
> +	{ .compatible = "adi,ad4855", .data = &ad4855_info, },
> +	{ .compatible = "adi,ad4854", .data = &ad4854_info, },
> +	{ .compatible = "adi,ad4853", .data = &ad4853_info, },
> +	{ .compatible = "adi,ad4852", .data = &ad4852_info, },
> +	{ .compatible = "adi,ad4851", .data = &ad4851_info, },
> +	{ .compatible = "adi,ad4858i", .data = &ad4858i_info, },
> +	{}
	{ }
> +};
> +

RE: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Miclaus, Antoniu]

> > +static int ad485x_get_calibbias(struct ad485x_state *st, int ch, int *val,
> > +				int *val2)
> > +{
> > +	unsigned int lsb, mid, msb;
> > +	int ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +
> > +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch),
> > +			  &msb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_MID(ch),
> > +			  &mid);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(st->regmap, AD485X_REG_CHX_OFFSET_LSB(ch),
> > +			  &lsb);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->info->resolution == 16) {
> > +		*val = msb << 8;
> > +		*val |= mid;
> > +		*val = sign_extend32(*val, 15);
> > +	} else {
> > +		*val = msb << 12;
> > +		*val |= mid << 4;
> > +		*val |= lsb >> 4;
> As below I'd use a byte array then you can do get_unaligned_be24 to
> + a right shift by 4 of the whole thing.
Regarding the bulk writes/reads, the msb/mid/lsb registers need to be read/write in a
specific order and the addresses are not incremental, so I am not sure how the bulk
functions fit. On this matter, we will need unsigned int (not u8) to store the values read via
regmap_read, and in this case we will need extra casts and assignments to use get_unaligned.
> 
> > +		*val = sign_extend32(*val, 19);
> > +	}
> > +
> > +	return IIO_VAL_INT;
> > +}
...

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Andy Shevchenko]

On Tue, Oct 01, 2024 at 11:53:18AM +0000, Miclaus, Antoniu wrote:

> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> read/write in a specific order and the addresses are not incremental,

We have _noinc() variants of regmap accessors.

> so I am not sure how the bulk functions fit. On this matter, we will need
> unsigned int (not u8) to store the values read via regmap_read, and in this
> case we will need extra casts and assignments to use get_unaligned.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Miclaus, Antoniu]

> > Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> > read/write in a specific order and the addresses are not incremental,
> 
> We have _noinc() variants of regmap accessors.
[Miclaus, Antoniu] 
I think _noinc() functions read from the same register address so it doesn't
apply.
I am reading values from multiple register addresses that are not reg_addr,
reg_addr+1, reg_addr+2.

> > so I am not sure how the bulk functions fit. On this matter, we will need
> > unsigned int (not u8) to store the values read via regmap_read, and in this
> > case we will need extra casts and assignments to use get_unaligned.
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[David Lechner]

On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:
>>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
>>> read/write in a specific order and the addresses are not incremental,
>>
>> We have _noinc() variants of regmap accessors.
> [Miclaus, Antoniu] 
> I think _noinc() functions read from the same register address so it doesn't
> apply.
> I am reading values from multiple register addresses that are not reg_addr,
> reg_addr+1, reg_addr+2.

I'm confused by the statement that the registers are not incremental.

For example, this patch defines...

+#define AD485X_REG_CHX_OFFSET_LSB(ch)	AD485X_REG_CHX_OFFSET(ch)
+#define AD485X_REG_CHX_OFFSET_MID(ch)	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
+#define AD485X_REG_CHX_OFFSET_MSB(ch)	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)

This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.

> 
>>> so I am not sure how the bulk functions fit. On this matter, we will need
>>> unsigned int (not u8) to store the values read via regmap_read, and in this
>>> case we will need extra casts and assignments to use get_unaligned.
>>
>> --
>> With Best Regards,
>> Andy Shevchenko
>>
> 

RE: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Miclaus, Antoniu]

> On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:
> >>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> >>> read/write in a specific order and the addresses are not incremental,
> >>
> >> We have _noinc() variants of regmap accessors.
> > [Miclaus, Antoniu]
> > I think _noinc() functions read from the same register address so it doesn't
> > apply.
> > I am reading values from multiple register addresses that are not reg_addr,
> > reg_addr+1, reg_addr+2.
> 
> I'm confused by the statement that the registers are not incremental.
> 
> For example, this patch defines...
> 
> +#define AD485X_REG_CHX_OFFSET_LSB(ch)
> 	AD485X_REG_CHX_OFFSET(ch)
> +#define AD485X_REG_CHX_OFFSET_MID(ch)
> 	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
> +#define AD485X_REG_CHX_OFFSET_MSB(ch)
> 	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
> 
> This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.
Yes you are right. Although I tested with hardware and it seems that the registers
are not properly written when using bulk operations. My guess is that holding CS low during
the entire transaction might be a possible issue. Any suggestions are appreciated.

> >
> >>> so I am not sure how the bulk functions fit. On this matter, we will need
> >>> unsigned int (not u8) to store the values read via regmap_read, and in this
> >>> case we will need extra casts and assignments to use get_unaligned.
> >>
> >> --
> >> With Best Regards,
> >> Andy Shevchenko
> >>
> >

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Andy Shevchenko]

On Thu, Oct 03, 2024 at 10:14:57AM +0000, Miclaus, Antoniu wrote:
> > On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:
> > >>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> > >>> read/write in a specific order and the addresses are not incremental,
> > >>
> > >> We have _noinc() variants of regmap accessors.
> > > [Miclaus, Antoniu]
> > > I think _noinc() functions read from the same register address so it doesn't
> > > apply.
> > > I am reading values from multiple register addresses that are not reg_addr,
> > > reg_addr+1, reg_addr+2.
> > 
> > I'm confused by the statement that the registers are not incremental.
> > 
> > For example, this patch defines...
> > 
> > +#define AD485X_REG_CHX_OFFSET_LSB(ch)
> > 	AD485X_REG_CHX_OFFSET(ch)
> > +#define AD485X_REG_CHX_OFFSET_MID(ch)
> > 	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
> > +#define AD485X_REG_CHX_OFFSET_MSB(ch)
> > 	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
> > 
> > This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.
> Yes you are right. Although I tested with hardware and it seems that the registers
> are not properly written when using bulk operations. My guess is that holding CS low during
> the entire transaction might be a possible issue. Any suggestions are appreciated.

Okay, so each byte has to be written as a separate SPI transfer?
I believe we have already examples of the drivers for such a hardware
in the Linux kernel, but I can't throw any example form top of my head.

> > >>> so I am not sure how the bulk functions fit. On this matter, we will need
> > >>> unsigned int (not u8) to store the values read via regmap_read, and in this
> > >>> case we will need extra casts and assignments to use get_unaligned.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[David Lechner]

On 10/3/24 5:14 AM, Miclaus, Antoniu wrote:
>> On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:
>>>>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
>>>>> read/write in a specific order and the addresses are not incremental,
>>>>
>>>> We have _noinc() variants of regmap accessors.
>>> [Miclaus, Antoniu]
>>> I think _noinc() functions read from the same register address so it doesn't
>>> apply.
>>> I am reading values from multiple register addresses that are not reg_addr,
>>> reg_addr+1, reg_addr+2.
>>
>> I'm confused by the statement that the registers are not incremental.
>>
>> For example, this patch defines...
>>
>> +#define AD485X_REG_CHX_OFFSET_LSB(ch)
>> 	AD485X_REG_CHX_OFFSET(ch)
>> +#define AD485X_REG_CHX_OFFSET_MID(ch)
>> 	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
>> +#define AD485X_REG_CHX_OFFSET_MSB(ch)
>> 	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
>>
>> This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.
> Yes you are right. Although I tested with hardware and it seems that the registers
> are not properly written when using bulk operations. My guess is that holding CS low during
> the entire transaction might be a possible issue. Any suggestions are appreciated.

Is ADDR_DIR in SPI_CONFIG_A set to the correct value to match how
the regmap is configured for bulk writes?

I had to set this bit for AD4695 which has a similar register map
(although on that one I used two regmaps, an 8-bit one and a 16-bit
one, instead of doing bulk operations on the 8-bit one).

> 
>>>
>>>>> so I am not sure how the bulk functions fit. On this matter, we will need
>>>>> unsigned int (not u8) to store the values read via regmap_read, and in this
>>>>> case we will need extra casts and assignments to use get_unaligned.
>>>>
>>>> --
>>>> With Best Regards,
>>>> Andy Shevchenko
>>>>
>>>
> 

RE: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Miclaus, Antoniu]

> On 10/3/24 5:14 AM, Miclaus, Antoniu wrote:
> >> On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:
> >>>>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> >>>>> read/write in a specific order and the addresses are not incremental,
> >>>>
> >>>> We have _noinc() variants of regmap accessors.
> >>> [Miclaus, Antoniu]
> >>> I think _noinc() functions read from the same register address so it doesn't
> >>> apply.
> >>> I am reading values from multiple register addresses that are not reg_addr,
> >>> reg_addr+1, reg_addr+2.
> >>
> >> I'm confused by the statement that the registers are not incremental.
> >>
> >> For example, this patch defines...
> >>
> >> +#define AD485X_REG_CHX_OFFSET_LSB(ch)
> >> 	AD485X_REG_CHX_OFFSET(ch)
> >> +#define AD485X_REG_CHX_OFFSET_MID(ch)
> >> 	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
> >> +#define AD485X_REG_CHX_OFFSET_MSB(ch)
> >> 	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
> >>
> >> This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.
> > Yes you are right. Although I tested with hardware and it seems that the
> registers
> > are not properly written when using bulk operations. My guess is that
> holding CS low during
> > the entire transaction might be a possible issue. Any suggestions are
> appreciated.
> 
> Is ADDR_DIR in SPI_CONFIG_A set to the correct value to match how
> the regmap is configured for bulk writes?
> 
> I had to set this bit for AD4695 which has a similar register map
> (although on that one I used two regmaps, an 8-bit one and a 16-bit
> one, instead of doing bulk operations on the 8-bit one).
> 
Thanks for the input! I tried your suggested approach: set the ADDR_DIR
to 1 during probe. Unfortunately, this did not fix the issue. I am still not able
to perform bulk writes properly to the device.

For now I will keep the only working version in v2, since there will be
most probably  other iterations of the this patch series 😊.

> >
> >>>
> >>>>> so I am not sure how the bulk functions fit. On this matter, we will need
> >>>>> unsigned int (not u8) to store the values read via regmap_read, and in
> this
> >>>>> case we will need extra casts and assignments to use get_unaligned.
> >>>>
> >>>> --
> >>>> With Best Regards,
> >>>> Andy Shevchenko
> >>>>
> >>>
> >

Re: [PATCH 6/7] iio: adc: ad485x: add ad485x driver

[Jonathan Cameron]

On Fri, 4 Oct 2024 14:07:37 +0000
"Miclaus, Antoniu" <Antoniu.Miclaus@analog.com> wrote:

> > On 10/3/24 5:14 AM, Miclaus, Antoniu wrote:  
> > >> On 10/1/24 8:51 AM, Miclaus, Antoniu wrote:  
> > >>>>> Regarding the bulk writes/reads, the msb/mid/lsb registers need to be
> > >>>>> read/write in a specific order and the addresses are not incremental,  
> > >>>>
> > >>>> We have _noinc() variants of regmap accessors.  
> > >>> [Miclaus, Antoniu]
> > >>> I think _noinc() functions read from the same register address so it doesn't
> > >>> apply.
> > >>> I am reading values from multiple register addresses that are not reg_addr,
> > >>> reg_addr+1, reg_addr+2.  
> > >>
> > >> I'm confused by the statement that the registers are not incremental.
> > >>
> > >> For example, this patch defines...
> > >>
> > >> +#define AD485X_REG_CHX_OFFSET_LSB(ch)
> > >> 	AD485X_REG_CHX_OFFSET(ch)
> > >> +#define AD485X_REG_CHX_OFFSET_MID(ch)
> > >> 	(AD485X_REG_CHX_OFFSET_LSB(ch) + 0x01)
> > >> +#define AD485X_REG_CHX_OFFSET_MSB(ch)
> > >> 	(AD485X_REG_CHX_OFFSET_MID(ch) + 0x01)
> > >>
> > >> This looks exactly like reg_addr, reg_addr+1, reg_addr+2 to me.  
> > > Yes you are right. Although I tested with hardware and it seems that the  
> > registers  
> > > are not properly written when using bulk operations. My guess is that  
> > holding CS low during  
> > > the entire transaction might be a possible issue. Any suggestions are  
> > appreciated.
> > 
> > Is ADDR_DIR in SPI_CONFIG_A set to the correct value to match how
> > the regmap is configured for bulk writes?
> > 
> > I had to set this bit for AD4695 which has a similar register map
> > (although on that one I used two regmaps, an 8-bit one and a 16-bit
> > one, instead of doing bulk operations on the 8-bit one).
> >   
> Thanks for the input! I tried your suggested approach: set the ADDR_DIR
> to 1 during probe. Unfortunately, this did not fix the issue. I am still not able
> to perform bulk writes properly to the device.
> 
> For now I will keep the only working version in v2, since there will be
> most probably  other iterations of the this patch series 😊.
I'd definitely like to know what is going on here if you can dig into it.
But if we really get stuck then the code at least needs a comment saying
it is necessary and we aren't sure why.  If we know why and can't change
it then the comment should give that reasoning.

Jonathan

> 
> > >  
> > >>>  
> > >>>>> so I am not sure how the bulk functions fit. On this matter, we will need
> > >>>>> unsigned int (not u8) to store the values read via regmap_read, and in  
> > this  
> > >>>>> case we will need extra casts and assignments to use get_unaligned.  
> > >>>>
> > >>>> --
> > >>>> With Best Regards,
> > >>>> Andy Shevchenko
> > >>>>  
> > >>>  
> > >  
> 

[PATCH 7/7] Documentation: ABI: testing: ad485x: add ABI docs

[Antoniu Miclaus]

Add documentation for the packet size.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x

diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
new file mode 100644
index 000000000000..80aaef4eb750
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
@@ -0,0 +1,14 @@
+What:		/sys/bus/iio/devices/iio:deviceX/packet_format_available
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Packet sizes on the CMOS or LVDS conversion data output bus.
+		Reading this returns the valid values that can be written to the
+		packet_format.
+
+What:		/sys/bus/iio/devices/iio:deviceX/packet_format
+KernelVersion:
+Contact:	linux-iio@vger.kernel.org
+Description:
+		This attribute configures the packet size.
+		Reading returns the actual size used.
-- 
2.46.0

Re: [PATCH 7/7] Documentation: ABI: testing: ad485x: add ABI docs

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 01:10:24PM +0300, Antoniu Miclaus wrote:
> Add documentation for the packet size.

...

> +KernelVersion:

Hmm...
Can't you use https://hansen.beer/~dave/phb/ (for v6.13)?

> +KernelVersion:

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 7/7] Documentation: ABI: testing: ad485x: add ABI docs

[Jonathan Cameron]

On Mon, 23 Sep 2024 13:10:24 +0300
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add documentation for the packet size.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> new file mode 100644
> index 000000000000..80aaef4eb750
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad485x
> @@ -0,0 +1,14 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/packet_format_available
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Packet sizes on the CMOS or LVDS conversion data output bus.
> +		Reading this returns the valid values that can be written to the
> +		packet_format.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/packet_format
> +KernelVersion:
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		This attribute configures the packet size.
> +		Reading returns the actual size used.
This was touched upon by David's review of the driver.
These docs tell us nothing useful unfortunately so a user would have
no idea how to set them...

Re: [PATCH 0/7] *** Add support for AD485x DAS Family ***

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 01:10:17PM +0300, Antoniu Miclaus wrote:
> Add support for AD485X fully buffered, 8-channel simultaneous sampling,
> 16/20-bit, 1 MSPS data acquisition system (DAS) with differential, wide
> common-mode range inputs.
> 
> Some particularities:
> 1. softspan - the devices support multiple softspans which are represented in iio
>               through offset/scale. The current handling implies changing both
>               the scale and the offset separately via IIO, therefore in order to
>               properly set the softspan, each time the offset changes the softspan
>               is set to the default value. And only after changing also the scale
>               the desired softspan is set. This is the approach we are suggesting
>               since we need the softspan configurable from userspace and not from
>               devicetree.
> 
> 2. packet format - Data provided on the CMOS and LVDS conversion data output buses
>                    are packaged into eight channel packets. This is currently handled
>                    as extended info.

Thanks for the series!

Quick note: *** in the Subject was also meant to be removed, but it's fine as
long as it's a cover letter and git send-email does its job.

-- 
With Best Regards,
Andy Shevchenko