[v5,1/2] dt-bindings: iio: frequency: add admfm2000

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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
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 | 154 ++++++++++++++++++
 MAINTAINERS                                   |   7 +
 2 files changed, 161 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml


base-commit: c2d5304e6c648ebcf653bace7e51e0e6742e46c8

On Fri, 24 Nov 2023 18:51:15 +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>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
 
Hi,

Sorry I'm late to the party.

Long term we might want to support cases where some of the pins are hard wired,
but that can happen when someone comes along with such a board.

Only thing I wonder is if the gpios could be moved under the child nodes
as I think they only apply to specific channels?  Would make the
driver a little more complex but the binding cleaner.

Thanks Krzysztof for all your reviews btw
(in general, rather than just this!)

Follow on comments inline...


> 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 000000000..037438737
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,admfm2000.yaml
> @@ -0,0 +1,154 @@
> +# 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
> +
> +  switch1-gpios:
> +    items:
> +      - description: B15 GPIO, when high (and B16 low) channel 1 is in
> +          Direct IF mode.
> +      - description: B16 GPIO, when high (and B15 low) channel 1 is in
> +          Mixer mode.
> +
> +  switch2-gpios:
> +    items:
> +      - description: K14 GPIO, when high (and L14 low) channel 2 is in
> +          Mixer mode.
> +      - description: L14 GPIO, when high (and K14 low) channel 2 is in
> +          Direct IF mode.
> +
> +  attenuation1-gpios:
> +    description: |
> +      Choice of attenuation:
> +      D15 D14 C16 C15 C14
I don't think there is a useful public data sheet, but normally I'd expect
these to have friendly names rather than pin coords.
chan0-att0, chan0-att1 or something like that.
Hopefully with something like that we could combine the docs if we can push
the GPIOs down into the child nodes.

> +      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: C14 GPIO
> +      - description: C15 GPIO
> +      - description: C16 GPIO
> +      - description: D14 GPIO
> +      - description: D15 GPIO
> +
> +  attenuation2-gpios:
> +    description: |
> +      Choice of attenuation:
> +      M16 M15 M14 L16 L15
> +      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: L15 GPIO
> +      - description: L16 GPIO
> +      - description: M14 GPIO
> +      - description: M15 GPIO
> +      - description: M16 GPIO
> +
> +  '#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]
> +
> +    required:
> +      - reg
> +      - adi,mode
> +
> +required:
> +  - compatible
> +  - switch1-gpios
> +  - switch2-gpios
> +  - attenuation1-gpios
> +  - attenuation2-gpios

On Fri, 24 Nov 2023 18:51:16 +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>
I've +CC Linus and Bartosz for the question of GPIOs under the channel child
nodes in DT.

Some background for them.  This device has two separate channels and each of them
has a mirrored set of attentuation and configuration controls via arrays of GPIOS.

Currently they are
switch1-gpios, switch2-gpios etc.

I suggested we might be able to move those into the existing channel child nodes
that are used for describing other per channel stuff.

      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>;
      };

I think there are suitable interfaces to do this in the GPIO firmware handling code
but wanted your opinion on whether it is worth the effort.

Relevant code is towards the end.

A few trivial other comments. In general this looks very clean to me.

Thanks,

Jonathan

> ---
> 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 | 310 ++++++++++++++++++++++++++++++
>  4 files changed, 322 insertions(+)
>  create mode 100644 drivers/iio/frequency/admfm2000.c
> 
..

> +
> +static int admfm2000_mode(struct iio_dev *indio_dev, u32 reg, u32 mode)
> +{
> +	struct admfm2000_state *st = iio_priv(indio_dev);
> +	DECLARE_BITMAP(values, 2);
> +
> +	switch (mode) {
> +	case ADMFM2000_MIXER_MODE:
> +		values[0] = (reg == 0) ? 1 : 2;
> +		gpiod_set_array_value_cansleep(st->sw_ch[reg]->ndescs,
> +					       st->sw_ch[reg]->desc,
> +					       NULL, values);
> +		break;
> +	case ADMFM2000_DIRECT_IF_MODE:
> +		values[0] = (reg == 0) ? 2 : 1;
> +		gpiod_set_array_value_cansleep(st->sw_ch[reg]->ndescs,
> +					       st->sw_ch[reg]->desc,
> +					       NULL, values);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;

I'd return in the good paths above as nothing useful to do down here.

> +}

> +
> +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 > ADMF20000_MAX_GAIN || gain < ADMF20000_MIN_GAIN)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_HARDWAREGAIN:
> +		mutex_lock(&st->lock);
guard(mutex)(&st->lock); 
would tidy this up a tiny bit by allow a direct return.
You will need to add {} around the whole case statement though.

> +		st->gain[chan->channel] = ~((abs(gain) / 1000) & 0x1F);
> +
> +		ret = admfm2000_attenuation(indio_dev, chan->channel,
> +					    st->gain[chan->channel]);
> +
> +		mutex_unlock(&st->lock);
> +		if (ret)
> +			return ret;
return here.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...

> +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;
> +
> +	device_for_each_child_node(dev, child) {

If the below handling of gpios suggestion works, that would become per channel
and move in here.

> +		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");
> +		}
> +
> +		ret = admfm2000_mode(indio_dev, reg, mode);
> +		if (ret) {
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int admfm2000_setup(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;
> +
Looking at this and considering if we can move the description into the channel
child fwnodes of the main one, the interfaces exposed are a bit limited, but I think
we can do it with devm_fwnode_gpiod_get_index() or potentially adding similar for
the array forms.


> +	st->sw_ch[0] = devm_gpiod_get_array(dev, "switch1", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sw_ch[0]))
> +		return dev_err_probe(dev, PTR_ERR(st->sw_ch[0]),
> +				     "Failed to get gpios\n");
> +
> +	if (st->sw_ch[0]->ndescs != ADMF20000_MODE_GPIOS) {
> +		dev_err_probe(dev, -ENODEV, "%d GPIOs needed to operate\n",
> +			      ADMF20000_MODE_GPIOS);
> +		return -ENODEV;
> +	}
> +
> +	st->sw_ch[1] = devm_gpiod_get_array(dev, "switch2", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->sw_ch[1]))
> +		return dev_err_probe(dev, PTR_ERR(st->sw_ch[1]),
> +				     "Failed to get gpios\n");
> +
> +	if (st->sw_ch[1]->ndescs != ADMF20000_MODE_GPIOS) {
> +		dev_err_probe(dev, -ENODEV, "%d GPIOs needed to operate\n",
> +			      ADMF20000_MODE_GPIOS);
> +		return -ENODEV;
> +	}
> +
> +	st->dsa_gpios[0] = devm_gpiod_get_array(dev, "attenuation1",
> +						GPIOD_OUT_LOW);
> +	if (IS_ERR(st->dsa_gpios[0]))
> +		return dev_err_probe(dev, PTR_ERR(st->dsa_gpios[0]),
> +				     "Failed to get gpios\n");
> +
> +	if (st->dsa_gpios[0]->ndescs != ADMF20000_DSA_GPIOS) {
> +		dev_err_probe(dev, -ENODEV, "%d GPIOs needed to operate\n",
> +			      ADMF20000_DSA_GPIOS);
> +		return -ENODEV;
> +	}
> +
> +	st->dsa_gpios[1] = devm_gpiod_get_array(dev, "attenuation2",
> +						GPIOD_OUT_LOW);
> +	if (IS_ERR(st->dsa_gpios[1]))
> +		return dev_err_probe(dev, PTR_ERR(st->dsa_gpios[1]),
> +				     "Failed to get gpios\n");
> +
> +	if (st->dsa_gpios[1]->ndescs != ADMF20000_DSA_GPIOS) {
> +		dev_err_probe(dev, -ENODEV, "%d GPIOs needed to operate\n",
> +			      ADMF20000_DSA_GPIOS);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}