diff mbox series

[v7,1/2] dt-bindings: iio: adc: add max14001

Message ID 20230620132641.256307-1-kimseer.paller@analog.com
State Not Applicable, archived
Headers show
Series [v7,1/2] dt-bindings: iio: adc: add max14001 | expand

Checks

Context Check Description
robh/checkpatch success
robh/patch-applied success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Paller, Kim Seer June 20, 2023, 1:26 p.m. UTC 
The MAX14001 is a configurable, isolated 10-bit ADC for multi-range
binary inputs.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
V6 -> V7: No changes.
V5 -> V6: Removed tags.
V3 -> V5: Added spaces between prefixes in subject. Fixed MAINTAINERS reference.

 .../bindings/iio/adc/adi,max14001.yaml        | 54 +++++++++++++++++++
 MAINTAINERS                                   |  7 +++
 2 files changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml


base-commit: 6f449d52b90fdd927fcf9df0388701de6d5381c6

Comments

Krzysztof Kozlowski June 20, 2023, 1:29 p.m. UTC | #1 
On 20/06/2023 15:26, Kim Seer Paller wrote:
> The MAX14001 is a configurable, isolated 10-bit ADC for multi-range
> binary inputs.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
> V6 -> V7: No changes.

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions. However, there's no need to repost patches *only* to add the
tags. The upstream maintainer will do that for acks received on the
version they apply.

https://elixir.bootlin.com/linux/v5.17/source/Documentation/process/submitting-patches.rst#L540

If a tag was not added on purpose, please state why and what changed.\


Best regards,
Krzysztof
Andy Shevchenko June 20, 2023, 3:15 p.m. UTC | #2 
On Tue, Jun 20, 2023 at 4:27 PM Kim Seer Paller
<kimseer.paller@analog.com> wrote:
>
> The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> binary inputs.

...

> +       /*
> +        * Align received data from the receive buffer, reversing and reordering
> +        * it to match the expected MSB-first format.
> +        */
> +       *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> +                                                       MAX14001_DATA_MASK;

Using __force in the C files is somehow stinky.

...

> +       /*
> +        * Convert transmit buffer to big-endian format and reverse transmit
> +        * buffer to align with the LSB-first input on SDI port.
> +        */
> +       st->spi_tx_buffer = (__force u16)(cpu_to_be16(bitrev16(

You have a different type of spi_tx_buffer than u16, don't you?

> +                               FIELD_PREP(MAX14001_ADDR_MASK, reg_addr) |
> +                               FIELD_PREP(MAX14001_SET_WRITE_BIT, 1) |
> +                               FIELD_PREP(MAX14001_DATA_MASK, data))));

...

> +       vref = devm_regulator_get_optional(dev, "vref");
> +       if (IS_ERR(vref)) {
> +               if (PTR_ERR(vref) != -ENODEV)
> +                       return dev_err_probe(dev, PTR_ERR(vref),
> +                                            "Failed to get vref regulator");
> +
> +               /* internal reference */
> +               st->vref_mv = 1250;
> +       } else {
> +               ret = regulator_enable(vref);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                       "Failed to enable vref regulators\n");
> +
> +               ret = devm_add_action_or_reset(dev, max14001_regulator_disable,
> +                                              vref);
> +               if (ret)
> +                       return ret;
> +
> +               ret = regulator_get_voltage(vref);
> +               if (ret < 0)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to get vref\n");
> +
> +               st->vref_mv = ret / 1000;
> +
> +               /* select external voltage reference source for the ADC */
> +               ret = max14001_reg_update(st, MAX14001_CFG,
> +                                         MAX14001_CFG_EXRF, 1);
> +               if (ret < 0)
> +                       return ret;
> +       }

Now, looking at this code I'm wondering if we may have something like
devm_regulator_get_enable_and_voltage_optional()

But it's another story.
Paller, Kim Seer June 21, 2023, 12:38 a.m. UTC | #3 
> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Tuesday, June 20, 2023 11:15 PM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: jic23@kernel.org; lars@metafoo.de; lgirdwood@gmail.com;
> broonie@kernel.org; Hennerich, Michael <Michael.Hennerich@analog.com>;
> robh@kernel.org; krzysztof.kozlowski@linaro.org; conor+dt@kernel.org; linux-
> iio@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
> Subject: Re: [PATCH v7 2/2] iio: adc: max14001: New driver
> 
> [External]
> 
> On Tue, Jun 20, 2023 at 4:27 PM Kim Seer Paller
> <kimseer.paller@analog.com> wrote:
> >
> > The MAX14001 is configurable, isolated 10-bit ADCs for multi-range
> > binary inputs.
> 
> ...
> 
> > +       /*
> > +        * Align received data from the receive buffer, reversing and reordering
> > +        * it to match the expected MSB-first format.
> > +        */
> > +       *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > +                                                       MAX14001_DATA_MASK;
> 
> Using __force in the C files is somehow stinky.
> 
> ...
> 
> > +       /*
> > +        * Convert transmit buffer to big-endian format and reverse transmit
> > +        * buffer to align with the LSB-first input on SDI port.
> > +        */
> > +       st->spi_tx_buffer = (__force u16)(cpu_to_be16(bitrev16(
> 
> You have a different type of spi_tx_buffer than u16, don't you?

I have the same type of spi_tx_buffer as u16. 

Other than using force cast, is there any way to resolve the endian warning? I have 
actually swapped the order of bitrev16() and cpu_to_be16/be16_to_cpu() functions. 
I have tested and they also work fine.

Best regards,
Kim Seer Paller
Andy Shevchenko June 21, 2023, 8:21 a.m. UTC | #4 
On Wed, Jun 21, 2023 at 3:38 AM Paller, Kim Seer
<KimSeer.Paller@analog.com> wrote:
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Tuesday, June 20, 2023 11:15 PM
> > On Tue, Jun 20, 2023 at 4:27 PM Kim Seer Paller
> > <kimseer.paller@analog.com> wrote:

...

> > > +       /*
> > > +        * Align received data from the receive buffer, reversing and reordering
> > > +        * it to match the expected MSB-first format.
> > > +        */
> > > +       *data = (__force u16)(be16_to_cpu(bitrev16(st->spi_rx_buffer))) &
> > > +                                                       MAX14001_DATA_MASK;
> >
> > Using __force in the C files is somehow stinky.

...

> > > +       /*
> > > +        * Convert transmit buffer to big-endian format and reverse transmit
> > > +        * buffer to align with the LSB-first input on SDI port.
> > > +        */
> > > +       st->spi_tx_buffer = (__force u16)(cpu_to_be16(bitrev16(
> >
> > You have a different type of spi_tx_buffer than u16, don't you?
>
> I have the same type of spi_tx_buffer as u16.

And you should have __be16.

> Other than using force cast, is there any way to resolve the endian warning? I have
> actually swapped the order of bitrev16() and cpu_to_be16/be16_to_cpu() functions.
> I have tested and they also work fine.

You really have to get it correct on both LE and BE architectures.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
new file mode 100644
index 000000000000..9d03c611fca3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
@@ -0,0 +1,54 @@ 
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,max14001.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices MAX14001 ADC
+
+maintainers:
+  - Kim Seer Paller <kimseer.paller@analog.com>
+
+description: |
+    Single channel 10 bit ADC with SPI interface. Datasheet
+    can be found here:
+      https://www.analog.com/media/en/technical-documentation/data-sheets/MAX14001-MAX14002.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,max14001
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 5000000
+
+  vref-supply:
+    description: Voltage reference to establish input scaling.
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        adc@0 {
+            compatible = "adi,max14001";
+            reg = <0>;
+            spi-max-frequency = <5000000>;
+            vref-supply = <&vref_reg>;
+        };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index f794002a192e..dcea2c31f920 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12670,6 +12670,13 @@  S:	Maintained
 F:	Documentation/devicetree/bindings/sound/max9860.txt
 F:	sound/soc/codecs/max9860.*
 
+MAX14001 IIO ADC DRIVER
+M:	Kim Seer Paller <kimseer.paller@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/adc/adi,max14001.yaml
+
 MAXBOTIX ULTRASONIC RANGER IIO DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 L:	linux-iio@vger.kernel.org