On Thu, 18 Jan 2024 16:58:56 +0800 Kim Seer Paller <kimseer.paller@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxx > 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@xxxxxxxxxx> > 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", ®); > + 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; > +}
