LinuxLists.cc - [PATCH v6 0/2] Changes to admfm2000 driver

2024-01-18 08:59:52

Subject: [PATCH v6 0/2] Changes to admfm2000 driver

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

2024-01-18 09:00:09

by Paller, Kim Seer

[permalink] [raw]

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

2024-01-18 09:04:39

by Paller, Kim Seer

[permalink] [raw]

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

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 <[email protected]>
---
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: [email protected]
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 <[email protected]>
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 <[email protected]>");
+MODULE_DESCRIPTION("ADMFM2000 Dual Microwave Down Converter");
+MODULE_LICENSE("GPL");
--
2.34.1

2024-01-18 16:10:03

by Conor Dooley

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>
> +
> +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.

Attachments:

(No filename) (3.15 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-19 00:30:54

by Paller, Kim Seer

[permalink] [raw]

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

> -----Original Message-----
> From: Conor Dooley <[email protected]>
> Sent: Friday, January 19, 2024 12:10 AM
> To: Paller, Kim Seer <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; Jonathan Cameron <[email protected]>; Lars-Peter
> Clausen <[email protected]>; Hennerich, Michael
> <[email protected]>; Rob Herring <[email protected]>;
> Krzysztof Kozlowski <[email protected]>; Conor Dooley
> <[email protected]>; Crt Mori <[email protected]>; Linus Walleij
> <[email protected]>; Bartosz Golaszewski <[email protected]>
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > +
> > +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

2024-01-19 08:26:53

by Nuno Sá

[permalink] [raw]

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

Hi Kim,

On Fri, 2024-01-19 at 00:30 +0000, Paller, Kim Seer wrote:
> > -----Original Message-----
> > From: Conor Dooley <[email protected]>
> > Sent: Friday, January 19, 2024 12:10 AM
> > To: Paller, Kim Seer <[email protected]>
> > Cc: [email protected]; [email protected]; linux-
> > [email protected]; Jonathan Cameron <[email protected]>; Lars-Peter
> > Clausen <[email protected]>; Hennerich, Michael
> > <[email protected]>; Rob Herring <[email protected]>;
> > Krzysztof Kozlowski <[email protected]>; Conor Dooley
> > <[email protected]>; Crt Mori <[email protected]>; Linus Walleij
> > <[email protected]>; Bartosz Golaszewski <[email protected]>
> > 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 <[email protected]>
> > > ---
> > > 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 <[email protected]>
> > > +
> > > +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á

2024-01-19 08:32:43

by Conor Dooley

[permalink] [raw]

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

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 <[email protected]>
> > > Sent: Friday, January 19, 2024 12:10 AM
> > > To: Paller, Kim Seer <[email protected]>
> > > Cc: [email protected]; [email protected]; linux-
> > > [email protected]; Jonathan Cameron <[email protected]>; Lars-Peter
> > > Clausen <[email protected]>; Hennerich, Michael
> > > <[email protected]>; Rob Herring <[email protected]>;
> > > Krzysztof Kozlowski <[email protected]>; Conor Dooley
> > > <[email protected]>; Crt Mori <[email protected]>; Linus Walleij
> > > <[email protected]>; Bartosz Golaszewski <[email protected]>
> > > 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 <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > > > +
> > > > +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?
>

Attachments:

(No filename) (5.53 kB)
signature.asc (235.00 B)
Download all attachments

2024-01-19 09:17:18

by Paller, Kim Seer

[permalink] [raw]

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

> > > > > +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

2024-01-19 09:21:38

by Nuno Sá

[permalink] [raw]

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

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 <[email protected]>
> > > > Sent: Friday, January 19, 2024 12:10 AM
> > > > To: Paller, Kim Seer <[email protected]>
> > > > Cc: [email protected]; [email protected]; linux-
> > > > [email protected]; Jonathan Cameron <[email protected]>; Lars-Peter
> > > > Clausen <[email protected]>; Hennerich, Michael
> > > > <[email protected]>; Rob Herring <[email protected]>;
> > > > Krzysztof Kozlowski <[email protected]>; Conor Dooley
> > > > <[email protected]>; Crt Mori <[email protected]>; Linus Walleij
> > > > <[email protected]>; Bartosz Golaszewski <[email protected]>
> > > > 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 01 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 <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
> > > > > +
> > > > > +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á
> >

2024-01-19 16:54:52

by Conor Dooley

[permalink] [raw]

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

> > > 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.

Attachments:

(No filename) (922.00 B)
signature.asc (235.00 B)
Download all attachments

2024-01-21 16:09:27

by Jonathan Cameron

[permalink] [raw]

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

On Thu, 18 Jan 2024 16:58:56 +0800
Kim Seer Paller <[email protected]> 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 <[email protected]>
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/[email protected]/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: [email protected]
> 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 <[email protected]>
> 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;
> +}