Re: [PATCH v6 2/2] iio: amplifiers: adl8113: add driver support

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 5 Dec 2025 16:40:41 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add support for adl8113 10MHz to 12GHz Low Noise Amplifier with
> 10MHz to 14GHz bypass switches.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
Hi Antoniu,

> no changes in v6.
Given I assume you just missed Andy's prior review I'll take
a look at this version with assumption both sets of comments will be
sorted for v7.

A few things inline.

> diff --git a/drivers/iio/amplifiers/adl8113.c b/drivers/iio/amplifiers/adl8113.c
> new file mode 100644
> index 000000000000..eed5fe69280b
> --- /dev/null
> +++ b/drivers/iio/amplifiers/adl8113.c

> +struct adl8113_gain_config {
> +	enum adl8113_signal_path path;
> +	int gain_db;
> +	int va;
> +	int vb;

What are va and vb for?  Currently seem unused because you derive
them from path at use point.

> +};



> +
> +static const struct iio_info adl8113_info = {
> +	.read_raw = adl8113_read_raw,
> +	.write_raw = adl8113_write_raw,
> +};
> +
> +static int adl8113_init_gain_configs(struct device *dev, struct adl8113_state *st)
> +{
> +	int external_a_gain, external_b_gain, i = 0, j;

Preference for not hiding initializations in a list where only one is initialized.

	int external_a_gain, external_b_gain, j;
	int i = 0;

makes it easier to spot.

> +
> +	/*
> +	 * 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);

Slightly odd wrap. I'd move the sizeof() up a line.

> +	if (!st->gain_configs)
> +		return -ENOMEM;
> +
> +	/* Always include internal amplifier (14dB) */
> +	st->gain_configs[i].path = ADL8113_INTERNAL_AMP;
> +	st->gain_configs[i].gain_db = 14;

Could do this as something like:

	st->gain_configs[i++] = (struct adl8113_gain_config) {
		.path = ADL8113_INTERNAL_AMP,
		.gain_db = 14,
	};

	st->gain_configs[i++] = (struct adl8113_gain_config) {
		.path = ADL8113_INTERNAL_BYPASS,
		.gain_db = -2,
	};

etc.

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

I don't follow this one.  What sort of testing purpose?  Something we want
in a real system?

> +	 */
> +	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);

What happens if we just don't bother enforcing this? I assume the second of the duplicates
can't be selected?  Do we care beyond it being silly?

> +		}
> +	}
> +
> +	return 0;
> +}