Messages in this thread |  | | Date | Sat, 6 Dec 2025 19:03:32 +0000 | | From | Jonathan Cameron <> | | Subject | Re: [PATCH v6 2/2] iio: amplifiers: adl8113: add driver support |
| |
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; > +}
|  |