[PATCH v6 0/2] Changes to admfm2000 driver

[PATCH v6 0/2] Changes to admfm2000 driver

[Kim Seer Paller]

Hi all,

Apologies for taking a long time to follow up on this series. I took some time to
test and try out the suggested changes. As for the major change in the bindings
and driver, the array of switch and attenuation GPIOs were moved under child nodes
utilizing the devm_fwnode_gpiod_get_index() for GPIO parsing.

I have just an inquiry regarding the difficulty of implementing the use of
guard(mutex)(&st->lock) for my current controller. Is it okay to simply use the
mutex_lock function instead?

Best regards,
Kim Seer Paller

Kim Seer Paller (2):
  dt-bindings: iio: frequency: add admfm2000
  iio: frequency: admfm2000: New driver

 .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++
 MAINTAINERS                                   |   8 +
 drivers/iio/frequency/Kconfig                 |  10 +
 drivers/iio/frequency/Makefile                |   1 +
 drivers/iio/frequency/admfm2000.c             | 307 ++++++++++++++++++
 5 files changed, 455 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
 create mode 100644 drivers/iio/frequency/admfm2000.c


base-commit: 296455ade1fdcf5f8f8c033201633b60946c589a
-- 
2.34.1

[PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Kim Seer Paller]

Dual microwave down converter module with input RF and LO frequency
ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
for each down conversion path.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
          Changed pin coords with friendly names. Removed Reviewed-by tag.
V4 -> V5: Added Reviewed-by tag.
V3 -> V4: Updated the description of the properties with multiple entries and
          defined the order.
V2 -> V3: Adjusted indentation to resolve wrong indentation warning. 
          Changed node name to converter. Updated the descriptions to clarify
          the properties.
V1 -> V2: Removed '|' after description. Specified the pins connected to
          the GPIOs. Added additionalProperties: false. Changed node name to gpio.
          Aligned < syntax with the previous syntax in the examples.

 .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 136 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml

diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
new file mode 100644
index 000000000000..6f2c91c38666
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
@@ -0,0 +1,129 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2023 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADMFM2000 Dual Microwave Down Converter
+
+maintainers:
+  - Kim Seer Paller <kimseer.paller@analog.com>
+
+description:
+  Dual microwave down converter module with input RF and LO frequency ranges
+  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
+  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each down
+  conversion path.
+
+properties:
+  compatible:
+    enum:
+      - adi,admfm2000
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@[0-1]$":
+    type: object
+    description: Represents a channel of the device.
+
+    additionalProperties: false
+
+    properties:
+      reg:
+        description:
+          The channel number.
+        minimum: 0
+        maximum: 1
+
+      adi,mode:
+        description:
+          RF path selected for the channel.
+            0 - Direct IF mode
+            1 - Mixer mode
+        $ref: /schemas/types.yaml#/definitions/uint32
+        enum: [0, 1]
+
+      switch-gpios:
+        description: |
+          GPIOs to select the RF path for the channel.
+          SW-CH1   CTRL-A   CTRL-B
+          SW-CH2   CTRL-A   CTRL-B    CH1 Status        CH2 Status
+                   1        0         Direct IF mode    Mixer mode
+                   0        1         Mixer mode        Direct IF mode
+
+        items:
+          - description: SW-CH-CTRL-A GPIO
+          - description: SW-CH-CTRL-B GPIO
+
+      attenuation-gpios:
+        description: |
+          Choice of attenuation:
+          DSA-V4  DSA-V3  DSA-V2  DSA-V1  DSA-V0
+          1       1       1       1       1        0 dB
+          1       1       1       1       0        -1 dB
+          1       1       1       0       1        -2 dB
+          1       1       0       1       1        -4 dB
+          1       0       1       1       1        -8 dB
+          0       1       1       1       1        -16 dB
+          0       0       0       0       0        -31 dB
+
+        items:
+          - description: DSA-V0 GPIO
+          - description: DSA-V1 GPIO
+          - description: DSA-V2 GPIO
+          - description: DSA-V3 GPIO
+          - description: DSA-V4 GPIO
+
+    required:
+      - reg
+      - adi,mode
+      - switch-gpios
+      - attenuation-gpios
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    converter {
+      compatible = "adi,admfm2000";
+
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      channel@0 {
+        reg = <0>;
+        adi,mode = <1>;
+        switch-gpios = <&gpio 1 GPIO_ACTIVE_LOW>,
+                       <&gpio 2 GPIO_ACTIVE_HIGH>;
+
+        attenuation-gpios = <&gpio 17 GPIO_ACTIVE_LOW>,
+                            <&gpio 22 GPIO_ACTIVE_LOW>,
+                            <&gpio 23 GPIO_ACTIVE_LOW>,
+                            <&gpio 24 GPIO_ACTIVE_LOW>,
+                            <&gpio 25 GPIO_ACTIVE_LOW>;
+      };
+
+      channel@1 {
+        reg = <1>;
+        adi,mode = <1>;
+        switch-gpios = <&gpio 3 GPIO_ACTIVE_LOW>,
+                       <&gpio 4 GPIO_ACTIVE_HIGH>;
+
+        attenuation-gpios = <&gpio 0 GPIO_ACTIVE_LOW>,
+                            <&gpio 5 GPIO_ACTIVE_LOW>,
+                            <&gpio 6 GPIO_ACTIVE_LOW>,
+                            <&gpio 16 GPIO_ACTIVE_LOW>,
+                            <&gpio 26 GPIO_ACTIVE_LOW>;
+      };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 2d94c72c3742..3a86f9d6cb98 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1260,6 +1260,13 @@ W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/hwmon/adi,adm1177.yaml
 F:	drivers/hwmon/adm1177.c
 
+ANALOG DEVICES INC ADMFM2000 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/frequency/adi,admfm2000.yaml
+
 ANALOG DEVICES INC ADMV1013 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
-- 
2.34.1

Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Conor Dooley]

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

Hey,

On Thu, Jan 18, 2024 at 04:58:55PM +0800, Kim Seer Paller wrote:
> Dual microwave down converter module with input RF and LO frequency
> ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> for each down conversion path.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> ---
> V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
>           Changed pin coords with friendly names. Removed Reviewed-by tag.
> V4 -> V5: Added Reviewed-by tag.
> V3 -> V4: Updated the description of the properties with multiple entries and
>           defined the order.
> V2 -> V3: Adjusted indentation to resolve wrong indentation warning. 
>           Changed node name to converter. Updated the descriptions to clarify
>           the properties.
> V1 -> V2: Removed '|' after description. Specified the pins connected to
>           the GPIOs. Added additionalProperties: false. Changed node name to gpio.
>           Aligned < syntax with the previous syntax in the examples.
> 
>  .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 136 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> new file mode 100644
> index 000000000000..6f2c91c38666
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> @@ -0,0 +1,129 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADMFM2000 Dual Microwave Down Converter
> +
> +maintainers:
> +  - Kim Seer Paller <kimseer.paller@analog.com>
> +
> +description:
> +  Dual microwave down converter module with input RF and LO frequency ranges
> +  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
> +  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each down
> +  conversion path.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,admfm2000
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@[0-1]$":
> +    type: object
> +    description: Represents a channel of the device.
> +
> +    additionalProperties: false
> +
> +    properties:
> +      reg:
> +        description:
> +          The channel number.
> +        minimum: 0
> +        maximum: 1
> +
> +      adi,mode:
> +        description:
> +          RF path selected for the channel.
> +            0 - Direct IF mode
> +            1 - Mixer mode
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]

How come this is an enum, rather than a boolean property such as
"adi,mixer-mode"?

Cheers,
Conor.

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

RE: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Paller, Kim Seer]

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Friday, January 19, 2024 12:10 AM
> To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>; Lars-Peter
> Clausen <lars@metafoo.de>; Hennerich, Michael
> <Michael.Hennerich@analog.com>; Rob Herring <robh+dt@kernel.org>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> <conor+dt@kernel.org>; Crt Mori <cmo@melexis.com>; Linus Walleij
> <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>
> Subject: Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000
> 
> [External]
> 
> Hey,
> 
> On Thu, Jan 18, 2024 at 04:58:55PM +0800, Kim Seer Paller wrote:
> > Dual microwave down converter module with input RF and LO frequency
> > ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> > 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> > for each down conversion path.
> >
> > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > ---
> > V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
> >           Changed pin coords with friendly names. Removed Reviewed-by tag.
> > V4 -> V5: Added Reviewed-by tag.
> > V3 -> V4: Updated the description of the properties with multiple entries and
> >           defined the order.
> > V2 -> V3: Adjusted indentation to resolve wrong indentation warning.
> >           Changed node name to converter. Updated the descriptions to clarify
> >           the properties.
> > V1 -> V2: Removed '|' after description. Specified the pins connected to
> >           the GPIOs. Added additionalProperties: false. Changed node name to
> gpio.
> >           Aligned < syntax with the previous syntax in the examples.
> >
> >  .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
> >  MAINTAINERS                                   |   7 +
> >  2 files changed, 136 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > new file mode 100644
> > index 000000000000..6f2c91c38666
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > @@ -0,0 +1,129 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2023 Analog Devices Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADMFM2000 Dual Microwave Down Converter
> > +
> > +maintainers:
> > +  - Kim Seer Paller <kimseer.paller@analog.com>
> > +
> > +description:
> > +  Dual microwave down converter module with input RF and LO frequency
> ranges
> > +  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
> > +  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each down
> > +  conversion path.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,admfm2000
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^channel@[0-1]$":
> > +    type: object
> > +    description: Represents a channel of the device.
> > +
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description:
> > +          The channel number.
> > +        minimum: 0
> > +        maximum: 1
> > +
> > +      adi,mode:
> > +        description:
> > +          RF path selected for the channel.
> > +            0 - Direct IF mode
> > +            1 - Mixer mode
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        enum: [0, 1]
> 
> How come this is an enum, rather than a boolean property such as
> "adi,mixer-mode"?

I used an enum, perhaps because it was easier to implement. However, this
could be changed if a boolean property might be more suitable in this case.
Is that the preferred option?

Best regards,
Kim Seer Paller

Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Nuno Sá]

Hi Kim,

On Fri, 2024-01-19 at 00:30 +0000, Paller, Kim Seer wrote:
> > -----Original Message-----
> > From: Conor Dooley <conor@kernel.org>
> > Sent: Friday, January 19, 2024 12:10 AM
> > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>; Lars-Peter
> > Clausen <lars@metafoo.de>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>; Rob Herring <robh+dt@kernel.org>;
> > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > <conor+dt@kernel.org>; Crt Mori <cmo@melexis.com>; Linus Walleij
> > <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>
> > Subject: Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000
> > 
> > [External]
> > 
> > Hey,
> > 
> > On Thu, Jan 18, 2024 at 04:58:55PM +0800, Kim Seer Paller wrote:
> > > Dual microwave down converter module with input RF and LO frequency
> > > ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> > > 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> > > for each down conversion path.
> > > 
> > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > > ---
> > > V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
> > >           Changed pin coords with friendly names. Removed Reviewed-by tag.
> > > V4 -> V5: Added Reviewed-by tag.
> > > V3 -> V4: Updated the description of the properties with multiple entries and
> > >           defined the order.
> > > V2 -> V3: Adjusted indentation to resolve wrong indentation warning.
> > >           Changed node name to converter. Updated the descriptions to clarify
> > >           the properties.
> > > V1 -> V2: Removed '|' after description. Specified the pins connected to
> > >           the GPIOs. Added additionalProperties: false. Changed node name to
> > gpio.
> > >           Aligned < syntax with the previous syntax in the examples.
> > > 
> > >  .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
> > >  MAINTAINERS                                   |   7 +
> > >  2 files changed, 136 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > 
> > > diff --git
> > a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > new file mode 100644
> > > index 000000000000..6f2c91c38666
> > > --- /dev/null
> > > +++
> > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > @@ -0,0 +1,129 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +# Copyright 2023 Analog Devices Inc.
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ADMFM2000 Dual Microwave Down Converter
> > > +
> > > +maintainers:
> > > +  - Kim Seer Paller <kimseer.paller@analog.com>
> > > +
> > > +description:
> > > +  Dual microwave down converter module with input RF and LO frequency
> > ranges
> > > +  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
> > > +  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each down
> > > +  conversion path.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,admfm2000
> > > +
> > > +  '#address-cells':
> > > +    const: 1
> > > +
> > > +  '#size-cells':
> > > +    const: 0
> > > +
> > > +patternProperties:
> > > +  "^channel@[0-1]$":
> > > +    type: object
> > > +    description: Represents a channel of the device.
> > > +
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      reg:
> > > +        description:
> > > +          The channel number.
> > > +        minimum: 0
> > > +        maximum: 1
> > > +
> > > +      adi,mode:
> > > +        description:
> > > +          RF path selected for the channel.
> > > +            0 - Direct IF mode
> > > +            1 - Mixer mode
> > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > +        enum: [0, 1]
> > 
> > How come this is an enum, rather than a boolean property such as
> > "adi,mixer-mode"?
> 
> I used an enum, perhaps because it was easier to implement. However, this
> could be changed if a boolean property might be more suitable in this case.
> Is that the preferred option?
> 

Hmmm, How is the enum easier than a boolean property :)? I guess the device has a
default mode. So, if it is Direct IF mode you have 'adi,mixer-mode' to enable that
mode and that's it. So the code is pretty much just:

if (device_property_read_bool()) {
	/* enable mixer mode */
}

Also remember that from a bindings point of view being easier to implement or not in
the driver does not matter much. Take for an example, properties with well know units
like 'microamp'. It would be much easier to just have an enum with the device
register values but that's not how we should do it since it wouldn't be intuitive at
all for people reading devicetrees.

- Nuno Sá

Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Conor Dooley]

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

On Fri, Jan 19, 2024 at 09:20:01AM +0100, Nuno Sá wrote:
> Hi Kim,
> 
> On Fri, 2024-01-19 at 00:30 +0000, Paller, Kim Seer wrote:
> > > -----Original Message-----
> > > From: Conor Dooley <conor@kernel.org>
> > > Sent: Friday, January 19, 2024 12:10 AM
> > > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > kernel@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>; Lars-Peter
> > > Clausen <lars@metafoo.de>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>; Rob Herring <robh+dt@kernel.org>;
> > > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > > <conor+dt@kernel.org>; Crt Mori <cmo@melexis.com>; Linus Walleij
> > > <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>
> > > Subject: Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000
> > > 
> > > [External]
> > > 
> > > Hey,
> > > 
> > > On Thu, Jan 18, 2024 at 04:58:55PM +0800, Kim Seer Paller wrote:
> > > > Dual microwave down converter module with input RF and LO frequency
> > > > ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> > > > 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> > > > for each down conversion path.
> > > > 
> > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > > > ---
> > > > V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
> > > >           Changed pin coords with friendly names. Removed Reviewed-by tag.
> > > > V4 -> V5: Added Reviewed-by tag.
> > > > V3 -> V4: Updated the description of the properties with multiple entries and
> > > >           defined the order.
> > > > V2 -> V3: Adjusted indentation to resolve wrong indentation warning.
> > > >           Changed node name to converter. Updated the descriptions to clarify
> > > >           the properties.
> > > > V1 -> V2: Removed '|' after description. Specified the pins connected to
> > > >           the GPIOs. Added additionalProperties: false. Changed node name to
> > > gpio.
> > > >           Aligned < syntax with the previous syntax in the examples.
> > > > 
> > > >  .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
> > > >  MAINTAINERS                                   |   7 +
> > > >  2 files changed, 136 insertions(+)
> > > >  create mode 100644
> > > Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > 
> > > > diff --git
> > > a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > new file mode 100644
> > > > index 000000000000..6f2c91c38666
> > > > --- /dev/null
> > > > +++
> > > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > @@ -0,0 +1,129 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +# Copyright 2023 Analog Devices Inc.
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: ADMFM2000 Dual Microwave Down Converter
> > > > +
> > > > +maintainers:
> > > > +  - Kim Seer Paller <kimseer.paller@analog.com>
> > > > +
> > > > +description:
> > > > +  Dual microwave down converter module with input RF and LO frequency
> > > ranges
> > > > +  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
> > > > +  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each down
> > > > +  conversion path.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - adi,admfm2000
> > > > +
> > > > +  '#address-cells':
> > > > +    const: 1
> > > > +
> > > > +  '#size-cells':
> > > > +    const: 0
> > > > +
> > > > +patternProperties:
> > > > +  "^channel@[0-1]$":
> > > > +    type: object
> > > > +    description: Represents a channel of the device.
> > > > +
> > > > +    additionalProperties: false
> > > > +
> > > > +    properties:
> > > > +      reg:
> > > > +        description:
> > > > +          The channel number.
> > > > +        minimum: 0
> > > > +        maximum: 1
> > > > +
> > > > +      adi,mode:
> > > > +        description:
> > > > +          RF path selected for the channel.
> > > > +            0 - Direct IF mode
> > > > +            1 - Mixer mode
> > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > +        enum: [0, 1]
> > > 
> > > How come this is an enum, rather than a boolean property such as
> > > "adi,mixer-mode"?
> > 
> > I used an enum, perhaps because it was easier to implement. However, this
> > could be changed if a boolean property might be more suitable in this case.
> > Is that the preferred option?
> > 
> 
> Hmmm, How is the enum easier than a boolean property :)? I guess the device has a
> default mode. So, if it is Direct IF mode you have 'adi,mixer-mode' to enable that
> mode and that's it. So the code is pretty much just:
> 
> if (device_property_read_bool()) {

device_property_present() is preferred I think.

> 	/* enable mixer mode */
> }
> 
> Also remember that from a bindings point of view being easier to implement or not in
> the driver does not matter much. Take for an example, properties with well know units
> like 'microamp'. It would be much easier to just have an enum with the device
> register values but that's not how we should do it since it wouldn't be intuitive at
> all for people reading devicetrees.
> 
> - Nuno Sá
> 

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

RE: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Paller, Kim Seer]

> > > > > +patternProperties:
> > > > > +  "^channel@[0-1]$":
> > > > > +    type: object
> > > > > +    description: Represents a channel of the device.
> > > > > +
> > > > > +    additionalProperties: false
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description:
> > > > > +          The channel number.
> > > > > +        minimum: 0
> > > > > +        maximum: 1
> > > > > +
> > > > > +      adi,mode:
> > > > > +        description:
> > > > > +          RF path selected for the channel.
> > > > > +            0 - Direct IF mode
> > > > > +            1 - Mixer mode
> > > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +        enum: [0, 1]
> > > >
> > > > How come this is an enum, rather than a boolean property such as
> > > > "adi,mixer-mode"?
> > >
> > > I used an enum, perhaps because it was easier to implement. However,
> > > this could be changed if a boolean property might be more suitable in this
> case.
> > > Is that the preferred option?
> > >
> >
> > Hmmm, How is the enum easier than a boolean property :)? I guess the
> > device has a default mode. So, if it is Direct IF mode you have
> > 'adi,mixer-mode' to enable that mode and that's it. So the code is pretty
> much just:
> >
> > if (device_property_read_bool()) {
> 
> device_property_present() is preferred I think.
> 
> > 	/* enable mixer mode */
> > }
> >
> > Also remember that from a bindings point of view being easier to
> > implement or not in the driver does not matter much. Take for an
> > example, properties with well know units like 'microamp'. It would be
> > much easier to just have an enum with the device register values but
> > that's not how we should do it since it wouldn't be intuitive at all for people
> reading devicetrees.

Hi Conor, Nuno,

Thanks for the input, I'll take note of that.

Best,
Kim

Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Nuno Sá]

On Fri, 2024-01-19 at 08:31 +0000, Conor Dooley wrote:
> On Fri, Jan 19, 2024 at 09:20:01AM +0100, Nuno Sá wrote:
> > Hi Kim,
> > 
> > On Fri, 2024-01-19 at 00:30 +0000, Paller, Kim Seer wrote:
> > > > -----Original Message-----
> > > > From: Conor Dooley <conor@kernel.org>
> > > > Sent: Friday, January 19, 2024 12:10 AM
> > > > To: Paller, Kim Seer <KimSeer.Paller@analog.com>
> > > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux-
> > > > kernel@vger.kernel.org; Jonathan Cameron <jic23@kernel.org>; Lars-Peter
> > > > Clausen <lars@metafoo.de>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>; Rob Herring <robh+dt@kernel.org>;
> > > > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley
> > > > <conor+dt@kernel.org>; Crt Mori <cmo@melexis.com>; Linus Walleij
> > > > <linus.walleij@linaro.org>; Bartosz Golaszewski <brgl@bgdev.pl>
> > > > Subject: Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000
> > > > 
> > > > [External]
> > > > 
> > > > Hey,
> > > > 
> > > > On Thu, Jan 18, 2024 at 04:58:55PM +0800, Kim Seer Paller wrote:
> > > > > Dual microwave down converter module with input RF and LO frequency
> > > > > ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> > > > > 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> > > > > for each down conversion path.
> > > > > 
> > > > > Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
> > > > > ---
> > > > > V5 -> V6: Moved array of switch and attenuation GPIOs to the channel node.
> > > > >           Changed pin coords with friendly names. Removed Reviewed-by tag.
> > > > > V4 -> V5: Added Reviewed-by tag.
> > > > > V3 -> V4: Updated the description of the properties with multiple entries
> > > > > and
> > > > >           defined the order.
> > > > > V2 -> V3: Adjusted indentation to resolve wrong indentation warning.
> > > > >           Changed node name to converter. Updated the descriptions to
> > > > > clarify
> > > > >           the properties.
> > > > > V1 -> V2: Removed '|' after description. Specified the pins connected to
> > > > >           the GPIOs. Added additionalProperties: false. Changed node name
> > > > > to
> > > > gpio.
> > > > >           Aligned < syntax with the previous syntax in the examples.
> > > > > 
> > > > >  .../bindings/iio/frequency/adi,admfm2000.yaml | 129 ++++++++++++++++++
> > > > >  MAINTAINERS                                   |   7 +
> > > > >  2 files changed, 136 insertions(+)
> > > > >  create mode 100644
> > > > Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > > 
> > > > > diff --git
> > > > a/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..6f2c91c38666
> > > > > --- /dev/null
> > > > > +++
> > > > b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> > > > > @@ -0,0 +1,129 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +# Copyright 2023 Analog Devices Inc.
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/iio/frequency/adi,admfm2000.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: ADMFM2000 Dual Microwave Down Converter
> > > > > +
> > > > > +maintainers:
> > > > > +  - Kim Seer Paller <kimseer.paller@analog.com>
> > > > > +
> > > > > +description:
> > > > > +  Dual microwave down converter module with input RF and LO frequency
> > > > ranges
> > > > > +  from 0.5 to 32 GHz and an output IF frequency range from 0.1 to 8 GHz.
> > > > > +  It consists of a LNA, mixer, IF filter, DSA, and IF amplifier for each
> > > > > down
> > > > > +  conversion path.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    enum:
> > > > > +      - adi,admfm2000
> > > > > +
> > > > > +  '#address-cells':
> > > > > +    const: 1
> > > > > +
> > > > > +  '#size-cells':
> > > > > +    const: 0
> > > > > +
> > > > > +patternProperties:
> > > > > +  "^channel@[0-1]$":
> > > > > +    type: object
> > > > > +    description: Represents a channel of the device.
> > > > > +
> > > > > +    additionalProperties: false
> > > > > +
> > > > > +    properties:
> > > > > +      reg:
> > > > > +        description:
> > > > > +          The channel number.
> > > > > +        minimum: 0
> > > > > +        maximum: 1
> > > > > +
> > > > > +      adi,mode:
> > > > > +        description:
> > > > > +          RF path selected for the channel.
> > > > > +            0 - Direct IF mode
> > > > > +            1 - Mixer mode
> > > > > +        $ref: /schemas/types.yaml#/definitions/uint32
> > > > > +        enum: [0, 1]
> > > > 
> > > > How come this is an enum, rather than a boolean property such as
> > > > "adi,mixer-mode"?
> > > 
> > > I used an enum, perhaps because it was easier to implement. However, this
> > > could be changed if a boolean property might be more suitable in this case.
> > > Is that the preferred option?
> > > 
> > 
> > Hmmm, How is the enum easier than a boolean property :)? I guess the device has a
> > default mode. So, if it is Direct IF mode you have 'adi,mixer-mode' to enable
> > that
> > mode and that's it. So the code is pretty much just:
> > 
> > if (device_property_read_bool()) {
> 
> device_property_present() is preferred I think.
> 

Hmm, don't want to start an argument but I'm not sure either :). I would argue that
device_property_read_bool() has more users (according to git grep - and if I did not
mess the grep) and it pretty much wraps device_property_present(). So, if there was
no value in it's "meaning" we would/should stop using it and eventually drop it...
Anyways, not really a big deal.

- Nuno Sá
> > 

Re: [PATCH v6 1/2] dt-bindings: iio: frequency: add admfm2000

[Conor Dooley]

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

> > > Hmmm, How is the enum easier than a boolean property :)? I guess the device has a
> > > default mode. So, if it is Direct IF mode you have 'adi,mixer-mode' to enable
> > > that
> > > mode and that's it. So the code is pretty much just:
> > > 
> > > if (device_property_read_bool()) {
> > 
> > device_property_present() is preferred I think.
> > 
> 
> Hmm, don't want to start an argument but I'm not sure either :). I would argue that
> device_property_read_bool() has more users (according to git grep - and if I did not
> mess the grep) and it pretty much wraps device_property_present(). So, if there was
> no value in it's "meaning" we would/should stop using it and eventually drop it...
> Anyways, not really a big deal.

If there's an actually boolean property that can have a true/false
value, but testing for presence alone device_property_present() is
the more accurate function to use.

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

[PATCH v6 2/2] iio: frequency: admfm2000: New driver

[Kim Seer Paller]

Dual microwave down converter module with input RF and LO frequency
ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
for each down conversion path.

Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
---
V5 -> V6: Used devm_fwnode_gpiod_get_index for getting array of gpios.
V4 -> V5: Added missing return -ENODEV in setup function. Reordered variable
	  declarations in probe function.
V1 -> V4: No changes.

 MAINTAINERS                       |   1 +
 drivers/iio/frequency/Kconfig     |  10 +
 drivers/iio/frequency/Makefile    |   1 +
 drivers/iio/frequency/admfm2000.c | 307 ++++++++++++++++++++++++++++++
 4 files changed, 319 insertions(+)
 create mode 100644 drivers/iio/frequency/admfm2000.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 3a86f9d6cb98..49d320535105 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1266,6 +1266,7 @@ L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
 F:	Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
+F:	drivers/iio/frequency/admfm2000.c
 
 ANALOG DEVICES INC ADMV1013 DRIVER
 M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 9e85dfa58508..c455be7d4a1c 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -60,6 +60,16 @@ config ADF4377
 	  To compile this driver as a module, choose M here: the
 	  module will be called adf4377.
 
+config ADMFM2000
+	tristate "Analog Devices ADMFM2000 Dual Microwave Down Converter"
+	depends on GPIOLIB
+	help
+	  Say yes here to build support for Analog Devices ADMFM2000 Dual
+	  Microwave Down Converter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called admfm2000.
+
 config ADMV1013
 	tristate "Analog Devices ADMV1013 Microwave Upconverter"
 	depends on SPI && COMMON_CLK
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index b616c29b4a08..70d0e0b70e80 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_AD9523) += ad9523.o
 obj-$(CONFIG_ADF4350) += adf4350.o
 obj-$(CONFIG_ADF4371) += adf4371.o
 obj-$(CONFIG_ADF4377) += adf4377.o
+obj-$(CONFIG_ADMFM2000) += admfm2000.o
 obj-$(CONFIG_ADMV1013) += admv1013.o
 obj-$(CONFIG_ADMV1014) += admv1014.o
 obj-$(CONFIG_ADMV4420) += admv4420.o
diff --git a/drivers/iio/frequency/admfm2000.c b/drivers/iio/frequency/admfm2000.c
new file mode 100644
index 000000000000..a09ec38fad22
--- /dev/null
+++ b/drivers/iio/frequency/admfm2000.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADMFM2000 Dual Microwave Down Converter
+ *
+ * Copyright 2023 Analog Devices Inc.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define ADMFM2000_MIXER_MODE		0
+#define ADMFM2000_DIRECT_IF_MODE	1
+#define ADMFM2000_DSA_GPIOS		5
+#define ADMFM2000_MODE_GPIOS		2
+#define ADMFM2000_MAX_GAIN		0
+#define ADMFM2000_MIN_GAIN		-31000
+#define ADMFM2000_DEFAULT_GAIN		-0x20
+
+struct admfm2000_state {
+	struct mutex			lock; /* protect sensor state */
+	struct gpio_desc		*sw1_ch[2];
+	struct gpio_desc		*sw2_ch[2];
+	struct gpio_desc		*dsa1_gpios[5];
+	struct gpio_desc		*dsa2_gpios[5];
+	u32				gain[2];
+};
+
+static int admfm2000_mode(struct iio_dev *indio_dev, u32 chan, u32 mode)
+{
+	struct admfm2000_state *st = iio_priv(indio_dev);
+	int i;
+
+	switch (mode) {
+	case ADMFM2000_MIXER_MODE:
+		for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
+			gpiod_set_value_cansleep(st->sw1_ch[i], (chan == 0) ? 1 : 0);
+			gpiod_set_value_cansleep(st->sw2_ch[i], (chan == 0) ? 0 : 1);
+		}
+		return 0;
+	case ADMFM2000_DIRECT_IF_MODE:
+		for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
+			gpiod_set_value_cansleep(st->sw1_ch[i], (chan == 0) ? 0 : 1);
+			gpiod_set_value_cansleep(st->sw2_ch[i], (chan == 0) ? 1 : 0);
+		}
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int admfm2000_attenuation(struct iio_dev *indio_dev, u32 chan, u32 value)
+{
+	struct admfm2000_state *st = iio_priv(indio_dev);
+	int i;
+
+	switch (chan) {
+	case 0:
+		for (i = 0; i < ADMFM2000_DSA_GPIOS; i++)
+			gpiod_set_value_cansleep(st->dsa1_gpios[i], value & (1 << i));
+		return 0;
+	case 1:
+		for (i = 0; i < ADMFM2000_DSA_GPIOS; i++)
+			gpiod_set_value_cansleep(st->dsa2_gpios[i], value & (1 << i));
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int admfm2000_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int *val,
+			      int *val2, long mask)
+{
+	struct admfm2000_state *st = iio_priv(indio_dev);
+	int gain;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		mutex_lock(&st->lock);
+		gain = ~(st->gain[chan->channel]) * -1000;
+		*val = gain / 1000;
+		*val2 = (gain % 1000) * 1000;
+		mutex_unlock(&st->lock);
+
+		return  IIO_VAL_INT_PLUS_MICRO_DB;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int admfm2000_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int val,
+			     int val2, long mask)
+{
+	struct admfm2000_state *st = iio_priv(indio_dev);
+	int gain, ret;
+
+	if (val < 0)
+		gain = (val * 1000) - (val2 / 1000);
+	else
+		gain = (val * 1000) + (val2 / 1000);
+
+	if (gain > ADMFM2000_MAX_GAIN || gain < ADMFM2000_MIN_GAIN)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		mutex_lock(&st->lock);
+		st->gain[chan->channel] = ~((abs(gain) / 1000) & 0x1F);
+
+		ret = admfm2000_attenuation(indio_dev, chan->channel,
+					     st->gain[chan->channel]);
+		mutex_unlock(&st->lock);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int admfm2000_write_raw_get_fmt(struct iio_dev *indio_dev,
+				       struct iio_chan_spec const *chan,
+				       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		return IIO_VAL_INT_PLUS_MICRO_DB;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info admfm2000_info = {
+	.read_raw = &admfm2000_read_raw,
+	.write_raw = &admfm2000_write_raw,
+	.write_raw_get_fmt = &admfm2000_write_raw_get_fmt,
+};
+
+#define ADMFM2000_CHAN(_channel) {					\
+	.type = IIO_VOLTAGE,						\
+	.output = 1,							\
+	.indexed = 1,							\
+	.channel = _channel,						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_HARDWAREGAIN),		\
+}
+
+static const struct iio_chan_spec admfm2000_channels[] = {
+	ADMFM2000_CHAN(0),
+	ADMFM2000_CHAN(1),
+};
+
+static int admfm2000_channel_config(struct admfm2000_state *st,
+				    struct iio_dev *indio_dev)
+{
+	struct platform_device *pdev = to_platform_device(indio_dev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct fwnode_handle *child;
+	u32 reg, mode;
+	int ret, i;
+
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &reg);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, ret,
+					     "Failed to get reg property\n");
+		}
+
+		if (reg >= indio_dev->num_channels) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL, "reg bigger than: %d\n",
+					     indio_dev->num_channels);
+		}
+
+		ret = fwnode_property_read_u32(child, "adi,mode", &mode);
+		if (ret) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, ret,
+					     "Failed to get mode property\n");
+		}
+
+		if (mode >= 2) {
+			fwnode_handle_put(child);
+			return dev_err_probe(dev, -EINVAL, "mode bigger than: 1\n");
+		}
+
+		switch (reg) {
+		case 0:
+			for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
+				st->sw1_ch[i] = devm_fwnode_gpiod_get_index(dev, child,
+									    "switch", i,
+									    GPIOD_OUT_LOW,
+									    NULL);
+				if (IS_ERR(st->sw1_ch[i])) {
+					fwnode_handle_put(child);
+					return dev_err_probe(dev, PTR_ERR(st->sw1_ch[i]),
+							     "Failed to get gpios\n");
+				}
+			}
+
+			for (i = 0; i < ADMFM2000_DSA_GPIOS; i++) {
+				st->dsa1_gpios[i] = devm_fwnode_gpiod_get_index(dev, child,
+										"attenuation", i,
+										GPIOD_OUT_LOW,
+										NULL);
+				if (IS_ERR(st->dsa1_gpios[i])) {
+					fwnode_handle_put(child);
+					return dev_err_probe(dev, PTR_ERR(st->dsa1_gpios[i]),
+							     "Failed to get gpios\n");
+				}
+			}
+			break;
+		case 1:
+			for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
+				st->sw2_ch[i] = devm_fwnode_gpiod_get_index(dev, child,
+									    "switch", i,
+									    GPIOD_OUT_LOW,
+									    NULL);
+				if (IS_ERR(st->sw2_ch[i])) {
+					fwnode_handle_put(child);
+					return dev_err_probe(dev, PTR_ERR(st->sw2_ch[i]),
+							     "Failed to get gpios\n");
+				}
+			}
+
+			for (i = 0; i < ADMFM2000_DSA_GPIOS; i++) {
+				st->dsa2_gpios[i] = devm_fwnode_gpiod_get_index(dev, child,
+										"attenuation", i,
+										GPIOD_OUT_LOW,
+										NULL);
+				if (IS_ERR(st->dsa2_gpios[i])) {
+					fwnode_handle_put(child);
+					return dev_err_probe(dev, PTR_ERR(st->dsa2_gpios[i]),
+							     "Failed to get gpios\n");
+				}
+			}
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		ret = admfm2000_mode(indio_dev, reg, mode);
+		if (ret) {
+			fwnode_handle_put(child);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int admfm2000_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct admfm2000_state *st;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	indio_dev->name = "admfm2000";
+	indio_dev->num_channels = ARRAY_SIZE(admfm2000_channels);
+	indio_dev->channels = admfm2000_channels;
+	indio_dev->info = &admfm2000_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	st->gain[0] = ADMFM2000_DEFAULT_GAIN;
+	st->gain[1] = ADMFM2000_DEFAULT_GAIN;
+
+	mutex_init(&st->lock);
+
+	ret = admfm2000_channel_config(st, indio_dev);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct of_device_id admfm2000_of_match[] = {
+	{ .compatible = "adi,admfm2000" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, admfm2000_of_match);
+
+static struct platform_driver admfm2000_driver = {
+	.driver = {
+		.name = "admfm2000",
+		.of_match_table = admfm2000_of_match,
+	},
+	.probe = admfm2000_probe,
+};
+module_platform_driver(admfm2000_driver);
+
+MODULE_AUTHOR("Kim Seer Paller <kimseer.paller@analog.com>");
+MODULE_DESCRIPTION("ADMFM2000 Dual Microwave Down Converter");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v6 2/2] iio: frequency: admfm2000: New driver

[Jonathan Cameron]

On Thu, 18 Jan 2024 16:58:56 +0800
Kim Seer Paller <kimseer.paller@analog.com> wrote:

> Dual microwave down converter module with input RF and LO frequency
> ranges from 0.5 to 32 GHz and an output IF frequency range from 0.1 to
> 8 GHz. It consists of a LNA, mixer, IF filter, DSA, and IF amplifier
> for each down conversion path.
> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@analog.com>
Hi.

A few comments inline.  Mainly about reducing some code duplication.
The note about autocleanup of fwnode_handle_put is a reference to:

https://lore.kernel.org/linux-iio/Za0NxrgBb0ve233b@smile.fi.intel.com/T/

Though I'm not sure that will land exactly as currently implemented, so
don't base anything on it yet.

> ---
> V5 -> V6: Used devm_fwnode_gpiod_get_index for getting array of gpios.
> V4 -> V5: Added missing return -ENODEV in setup function. Reordered variable
> 	  declarations in probe function.
> V1 -> V4: No changes.
> 
>  MAINTAINERS                       |   1 +
>  drivers/iio/frequency/Kconfig     |  10 +
>  drivers/iio/frequency/Makefile    |   1 +
>  drivers/iio/frequency/admfm2000.c | 307 ++++++++++++++++++++++++++++++
>  4 files changed, 319 insertions(+)
>  create mode 100644 drivers/iio/frequency/admfm2000.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3a86f9d6cb98..49d320535105 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1266,6 +1266,7 @@ L:	linux-iio@vger.kernel.org
>  S:	Supported
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> +F:	drivers/iio/frequency/admfm2000.c
>  
>  ANALOG DEVICES INC ADMV1013 DRIVER
>  M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 9e85dfa58508..c455be7d4a1c 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -60,6 +60,16 @@ config ADF4377
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adf4377.
>  
> +config ADMFM2000
> +	tristate "Analog Devices ADMFM2000 Dual Microwave Down Converter"
> +	depends on GPIOLIB
> +	help
> +	  Say yes here to build support for Analog Devices ADMFM2000 Dual
> +	  Microwave Down Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called admfm2000.
> +
>  config ADMV1013
>  	tristate "Analog Devices ADMV1013 Microwave Upconverter"
>  	depends on SPI && COMMON_CLK
> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index b616c29b4a08..70d0e0b70e80 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_AD9523) += ad9523.o
>  obj-$(CONFIG_ADF4350) += adf4350.o
>  obj-$(CONFIG_ADF4371) += adf4371.o
>  obj-$(CONFIG_ADF4377) += adf4377.o
> +obj-$(CONFIG_ADMFM2000) += admfm2000.o
>  obj-$(CONFIG_ADMV1013) += admv1013.o
>  obj-$(CONFIG_ADMV1014) += admv1014.o
>  obj-$(CONFIG_ADMV4420) += admv4420.o
> diff --git a/drivers/iio/frequency/admfm2000.c b/drivers/iio/frequency/admfm2000.c
> new file mode 100644
> index 000000000000..a09ec38fad22
> --- /dev/null
> +++ b/drivers/iio/frequency/admfm2000.c
> @@ -0,0 +1,307 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADMFM2000 Dual Microwave Down Converter
> + *
> + * Copyright 2023 Analog Devices Inc.
As you are updating in 2024, this might want an update to include this year.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>

Why this include?
You are using stuff from property.h not the of specific stuff.
You should have mod_devicetable.h for the of_device_id definition though.

> +#include <linux/platform_device.h>


> +
> +static int admfm2000_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
> +{
> +	struct admfm2000_state *st = iio_priv(indio_dev);
> +	int gain;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		mutex_lock(&st->lock);
> +		gain = ~(st->gain[chan->channel]) * -1000;
> +		*val = gain / 1000;
> +		*val2 = (gain % 1000) * 1000;
> +		mutex_unlock(&st->lock);
> +
> +		return  IIO_VAL_INT_PLUS_MICRO_DB;

Odd extra space after return.

> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int admfm2000_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct admfm2000_state *st = iio_priv(indio_dev);
> +	int gain, ret;
> +
> +	if (val < 0)
> +		gain = (val * 1000) - (val2 / 1000);
> +	else
> +		gain = (val * 1000) + (val2 / 1000);
> +
> +	if (gain > ADMFM2000_MAX_GAIN || gain < ADMFM2000_MIN_GAIN)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		mutex_lock(&st->lock);
> +		st->gain[chan->channel] = ~((abs(gain) / 1000) & 0x1F);
> +
> +		ret = admfm2000_attenuation(indio_dev, chan->channel,
> +					     st->gain[chan->channel]);
> +		mutex_unlock(&st->lock);
> +		break;
		return ret;

> +	default:
> +		ret = -EINVAL;
		return -EINVAL;

> +	}
> +
> +	return ret;

Not needed with direct returns above.  Returning early reduces the code
a reviewer needs to consider for a given flow, which is nice to do!

> +}

> +
> +static int admfm2000_channel_config(struct admfm2000_state *st,
> +				    struct iio_dev *indio_dev)
> +{
> +	struct platform_device *pdev = to_platform_device(indio_dev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *child;
> +	u32 reg, mode;
> +	int ret, i;
> +
> +	device_for_each_child_node(dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &reg);
> +		if (ret) {
> +			fwnode_handle_put(child);

Once a proposed auto cleanup solution for fwnode_handle_put() lands
we an tidy this up a lot as then we can get rid of all these manual
reference drops.

> +			return dev_err_probe(dev, ret,
> +					     "Failed to get reg property\n");
> +		}
> +
> +		if (reg >= indio_dev->num_channels) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL, "reg bigger than: %d\n",
> +					     indio_dev->num_channels);
> +		}
> +
> +		ret = fwnode_property_read_u32(child, "adi,mode", &mode);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, ret,
> +					     "Failed to get mode property\n");
> +		}
> +
> +		if (mode >= 2) {
> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL, "mode bigger than: 1\n");
> +		}
> +
> +		switch (reg) {
> +		case 0:

Use a couple of local variables to avoid the code duplication.

		struct gpio_desc *sw;
		struct gpio_desc *dsa;
		switch (ret) {
		case 0:
			sw = st->sw1_ch;
			dsa = st->dsa1_gpios;
			break;
		case 1:
			sw = st->sw2_ch;
			dsa = st->dsa2_gpios; /* or maybe make these arrays of pointers */
			break;
		default:
			fwnode_handle_put()
			return;
		}

		for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
			sw[i] = devm_fdnode...
etc



> +			for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
> +				st->sw1_ch[i] = devm_fwnode_gpiod_get_index(dev, child,
> +									    "switch", i,
> +									    GPIOD_OUT_LOW,
> +									    NULL);
> +				if (IS_ERR(st->sw1_ch[i])) {
> +					fwnode_handle_put(child);
> +					return dev_err_probe(dev, PTR_ERR(st->sw1_ch[i]),
> +							     "Failed to get gpios\n");
> +				}
> +			}
> +
> +			for (i = 0; i < ADMFM2000_DSA_GPIOS; i++) {
> +				st->dsa1_gpios[i] = devm_fwnode_gpiod_get_index(dev, child,
> +										"attenuation", i,
> +										GPIOD_OUT_LOW,
> +										NULL);
> +				if (IS_ERR(st->dsa1_gpios[i])) {
> +					fwnode_handle_put(child);
> +					return dev_err_probe(dev, PTR_ERR(st->dsa1_gpios[i]),
> +							     "Failed to get gpios\n");
> +				}
> +			}
> +			break;
> +		case 1:
> +			for (i = 0; i < ADMFM2000_MODE_GPIOS; i++) {
> +				st->sw2_ch[i] = devm_fwnode_gpiod_get_index(dev, child,
> +									    "switch", i,
> +									    GPIOD_OUT_LOW,
> +									    NULL);
> +				if (IS_ERR(st->sw2_ch[i])) {
> +					fwnode_handle_put(child);
> +					return dev_err_probe(dev, PTR_ERR(st->sw2_ch[i]),
> +							     "Failed to get gpios\n");
> +				}
> +			}
> +
> +			for (i = 0; i < ADMFM2000_DSA_GPIOS; i++) {
> +				st->dsa2_gpios[i] = devm_fwnode_gpiod_get_index(dev, child,
> +										"attenuation", i,
> +										GPIOD_OUT_LOW,
> +										NULL);
> +				if (IS_ERR(st->dsa2_gpios[i])) {
> +					fwnode_handle_put(child);
> +					return dev_err_probe(dev, PTR_ERR(st->dsa2_gpios[i]),
> +							     "Failed to get gpios\n");
> +				}
> +			}
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		ret = admfm2000_mode(indio_dev, reg, mode);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}