| 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: <CAHp75VcxYGskVpQ5HKiFUUnNSj-9qpdXeBjz9-oEHc9eumE0fg@mail.gmail.com>
Date: Thu, 4 Dec 2025 14:47:51 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoniu Miclaus <antoniu.miclaus@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] iio: amplifiers: adl8113: add driver support
On Fri, Nov 28, 2025 at 4:45 PM Antoniu Miclaus
<antoniu.miclaus@...log.com> wrote:
>
> Add support for adl8113 10MHz to 12GHz Low Noise Amplifier with
> 10MHz to 14GHz bypass switches.
Almost there, it's the 5th version and looks good (no major issues),
so if you address my comments below in v6, I'll give a tag.
...
> +#include <linux/array_size.h>
+ bitmap.h // bitmap_*(), BIT(), GENMASK(), etc.
> +#include <linux/device.h>
Is it really used? I found
+ dev_printk.h // dev_err_probe() et al.
+ device/devres.h // devm_k*alloc*()
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
+ limits.h // INT_MIN
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
Are those two being used? (device/devres.h should provide what
devm_k*alloc*() needs)
+ types.h // DECLARE_BITMAP()
...
> +struct adl8113_state {
> + struct gpio_descs *gpios;
> + struct adl8113_gain_config *gain_configs;
> + int num_gain_configs;
Do you need this to be signed?
> + enum adl8113_signal_path current_path;
> +};
...
> +static const char * const adl8113_supply_names[] = {
> + "vdd1",
> + "vss2",
> + "vdd2"
Please, keep the trailing comma.
> +};
...
> +static int adl8113_set_path(struct adl8113_state *st,
> + enum adl8113_signal_path path)
> +{
> + DECLARE_BITMAP(values, 2);
> + int ret;
> + bitmap_zero(values, 2);
Move this to the switch case...
> + /* Determine GPIO values based on signal path */
> + switch (path) {
> + case ADL8113_INTERNAL_AMP:
> + /* va=0, vb=0 - already zero */
Unneeded comment here and below, just put a number to the bitmap
bitmap_write(..., 0x00);
> + break;
> + case ADL8113_INTERNAL_BYPASS:
> + /* va=1, vb=1 */
> + __set_bit(0, values);
> + __set_bit(1, values);
bitmap_write(..., 0x03);
> + break;
> + case ADL8113_EXTERNAL_A:
> + /* va=0, vb=1 */
> + __set_bit(1, values);
bitmap_write(..., 0x02);
> + break;
> + case ADL8113_EXTERNAL_B:
> + /* va=1, vb=0 */
> + __set_bit(0, values);
bitmap_write(..., 0x01);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = gpiod_set_array_value_cansleep(st->gpios->ndescs, st->gpios->desc,
> + st->gpios->info, values);
> + if (ret)
> + return ret;
> +
> + st->current_path = path;
> + return 0;
> +}
...
> +static int adl8113_find_gain_config(struct adl8113_state *st, int gain_db)
> +{
> + int i;
Does it need to be signed?
> + for (i = 0; i < st->num_gain_configs; i++) {
> + if (st->gain_configs[i].gain_db == gain_db)
> + return i;
> + }
> + return -EINVAL;
> +}
...
> +static int adl8113_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct adl8113_state *st = iio_priv(indio_dev);
> + int i;
unsigned int i;
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + /* Find current gain configuration */
> + for (i = 0; i < st->num_gain_configs; i++) {
> + if (st->gain_configs[i].path == st->current_path) {
> + *val = st->gain_configs[i].gain_db;
> + *val2 = 0;
> + return IIO_VAL_INT_PLUS_MICRO_DB;
> + }
> + }
> + return -EINVAL;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int adl8113_init_gain_configs(struct device *dev, struct adl8113_state *st)
> +{
> + int external_a_gain, external_b_gain, i = 0, j;
unsigned int i, j;
And it's better to decouple assignment
> + /*
> + * Allocate for all 4 possible paths:
> + * - Internal amp and bypass (always present)
> + * - External bypass A and B (optional, or INT_MIN for testing)
> + */
> + st->gain_configs = devm_kcalloc(dev, 4,
> + sizeof(*st->gain_configs), GFP_KERNEL);
> + if (!st->gain_configs)
> + return -ENOMEM;
/* Start filling the gain configurations with data */
i = 0;
> + /* Always include internal amplifier (14dB) */
> + st->gain_configs[i].path = ADL8113_INTERNAL_AMP;
> + st->gain_configs[i].gain_db = 14;
> + i++;
> +
> + /* Always include internal bypass (-2dB insertion loss) */
> + st->gain_configs[i].path = ADL8113_INTERNAL_BYPASS;
> + st->gain_configs[i].gain_db = -2;
> + i++;
> +
> + /* Add external bypass A if configured */
> + if (!device_property_read_u32(dev, "adi,external-bypass-a-gain-db",
> + &external_a_gain)) {
> + st->gain_configs[i].path = ADL8113_EXTERNAL_A;
> + st->gain_configs[i].gain_db = external_a_gain;
> + i++;
> + }
> +
> + /* Add external bypass B if configured */
> + if (!device_property_read_u32(dev, "adi,external-bypass-b-gain-db",
> + &external_b_gain)) {
> + st->gain_configs[i].path = ADL8113_EXTERNAL_B;
> + st->gain_configs[i].gain_db = external_b_gain;
> + i++;
> + }
> +
> + /*
> + * If there's a free external bypass path, add one with INT_MIN gain
> + * to represent "nothing connected" for testing purposes
Missing period at the end.
> + */
> + if (!device_property_present(dev, "adi,external-bypass-a-gain-db")) {
> + st->gain_configs[i].path = ADL8113_EXTERNAL_A;
> + st->gain_configs[i].gain_db = INT_MIN;
> + i++;
> + } else if (!device_property_present(dev, "adi,external-bypass-b-gain-db")) {
> + st->gain_configs[i].path = ADL8113_EXTERNAL_B;
> + st->gain_configs[i].gain_db = INT_MIN;
> + i++;
> + }
> +
> + st->num_gain_configs = i;
> +
> + /* Check for duplicate gain values */
> + for (i = 0; i < st->num_gain_configs - 1; i++) {
> + for (j = i + 1; j < st->num_gain_configs; j++) {
> + if (st->gain_configs[i].gain_db == st->gain_configs[j].gain_db)
> + return dev_err_probe(dev, -EINVAL,
> + "Duplicate gain values not allowed: %d dB\n",
> + st->gain_configs[i].gain_db);
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int adl8113_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct adl8113_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);
> +
> + st->gpios = devm_gpiod_get_array(dev, "ctrl", GPIOD_OUT_LOW);
> + if (IS_ERR(st->gpios))
> + return dev_err_probe(dev, PTR_ERR(st->gpios),
> + "failed to get control GPIOs\n");
> +
> + if (st->gpios->ndescs != 2)
> + return dev_err_probe(dev, -EINVAL,
> + "expected 2 control GPIOs, got %d\n",
%u
> + st->gpios->ndescs);
> +
> + ret = devm_regulator_bulk_get_enable(dev,
> + ARRAY_SIZE(adl8113_supply_names),
> + adl8113_supply_names);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to get and enable supplies\n");
> +
> + /* Initialize gain configurations from devicetree */
> + ret = adl8113_init_gain_configs(dev, st);
> + if (ret)
> + return ret;
> +
> + /* Initialize to internal amplifier path (14dB) */
> + ret = adl8113_set_path(st, ADL8113_INTERNAL_AMP);
> + if (ret)
> + return ret;
> +
> + indio_dev->info = &adl8113_info;
> + indio_dev->name = "adl8113";
> + indio_dev->channels = adl8113_channels;
> + indio_dev->num_channels = ARRAY_SIZE(adl8113_channels);
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
...
> +static struct platform_driver adl8113_driver = {
> + .driver = {
> + .name = "adl8113",
> + .of_match_table = adl8113_of_match,
> + },
> + .probe = adl8113_probe,
> +};
> +
Redundant blank line.
> +module_platform_driver(adl8113_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists