| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20240121160900.308055c7@jic23-huawei>
Date: Sun, 21 Jan 2024 16:09:00 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Kim Seer Paller <kimseer.paller@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Lars-Peter Clausen <lars@...afoo.de>,
Michael Hennerich <Michael.Hennerich@...log.com>, Rob Herring
<robh+dt@...nel.org>, Krzysztof Kozlowski
<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
Crt Mori <cmo@...exis.com>, Linus Walleij <linus.walleij@...aro.org>,
Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v6 2/2] iio: frequency: admfm2000: New driver
On Thu, 18 Jan 2024 16:58:56 +0800
Kim Seer Paller <kimseer.paller@...log.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@...log.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@...r.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@...log.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", ®);
> + 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;
> +}
Powered by blists - more mailing lists