On Fri, 13 Dec 2024 18:44:45 +0200
Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote:
> Add support for the AD485X a fully buffered, 8-channel simultaneous
> sampling, 16/20-bit, 1 MSPS data acquisition system (DAS) with
> differential, wide common-mode range inputs.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
Hi Antoniu
Whilst David's question needs a clear answer I took a look at this
version and noticed a few more minor things.
One is a left over from a previous review that got missed.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/ad4851.c b/drivers/iio/adc/ad4851.c
> new file mode 100644
> index 000000000000..a5dbf464a4b0
> --- /dev/null
> +++ b/drivers/iio/adc/ad4851.c
> @@ -0,0 +1,1300 @@
...
> +static int ad4851_get_calibbias(struct ad4851_state *st, int ch, int *val)
> +{
> + unsigned int lsb, mid, msb;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MSB(ch),
> + &msb);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_MID(ch),
> + &mid);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(st->regmap, AD4851_REG_CHX_OFFSET_LSB(ch),
> + &lsb);
Fits under 80 chars so should be on oneline.
Check the whole driver for similar cases. As code evolves and indents
change it's common to have a few of these but a final check by the author
before posting should tidy them up.
> + if (ret)
> + return ret;
> +
> + if (st->info->resolution == 16) {
> + *val = msb << 8;
> + *val |= mid;
> + *val = sign_extend32(*val, 15);
> + } else {
> + *val = msb << 12;
> + *val |= mid << 4;
> + *val |= lsb >> 4;
> + *val = sign_extend32(*val, 19);
> + }
> +
> + return IIO_VAL_INT;
> +}
...
> +
> +static int ad4851_parse_channels(struct iio_dev *indio_dev, struct iio_chan_spec **ad4851_channels,
> + const struct iio_chan_spec ad4851_chan,
> + const struct iio_chan_spec ad4851_chan_diff)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct ad4851_state *st = iio_priv(indio_dev);
See below. I'd get the device via st->spi.dev
> + struct iio_chan_spec *channels;
> + unsigned int num_channels, index = 0, reg;
Trivial but I'd prefer splitting the elements that are assigned from those that aren't.
unsigned int num_channels, reg;
unsigned int index = 0;
It can sometimes be a little hard to spot which are assigned and which not if
they are all in one long line.
> + int ret;
> +
> + num_channels = device_get_child_node_count(dev);
> + if (num_channels > AD4851_MAX_CH_NR)
> + return dev_err_probe(dev, -EINVAL, "Too many channels: %u\n",
> + num_channels);
> +
> + channels = devm_kcalloc(dev, num_channels,
> + sizeof(*channels), GFP_KERNEL);
> + if (!channels)
> + return -ENOMEM;
> +
> + indio_dev->channels = channels;
> + indio_dev->num_channels = num_channels;
> + st->num_channels = num_channels;
Trivial. Maybe worth avoiding that duplication.
> +
> + device_for_each_child_node_scoped(dev, child) {
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Missing channel number\n");
> + if (fwnode_property_present(child, "bipolar")) {
Bipolar and differential aren't necessarily linked. You can have
a differential channel that is not bipolar. Maybe in this case this
connection is there. Anyhow this really comes back to David's question
on why these aren't differential channels in DT.
> + *channels = ad4851_chan_diff;
> + channels->scan_index = index++;
> + channels->channel = reg;
> + channels->channel2 = reg + AD4851_MAX_CH_NR;
> +
> + } else {
> + *channels = ad4851_chan;
> + channels->scan_index = index++;
> + channels->channel = reg;
> + ret = regmap_write(st->regmap, AD4851_REG_CHX_SOFTSPAN(reg),
> + AD4851_SOFTSPAN_0V_40V);
> + if (ret)
> + return ret;
> + }
> + channels++;
> + }
> +
> + *ad4851_channels = channels;
> +
> + return 0;
> +}
> +
> +static int ad4857_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct iio_chan_spec *ad4851_channels;
> + const struct iio_chan_spec ad4851_chan = AD4857_IIO_CHANNEL(0, 0, 0);
> + const struct iio_chan_spec ad4851_chan_diff = AD4857_IIO_CHANNEL(0, 0, 1);
> +
> + return ad4851_parse_channels(indio_dev, &ad4851_channels, ad4851_chan, ad4851_chan_diff);
> +}
> +
> +static int ad4858_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
Whilst true that it's that device, that is kind of an internal detail of IIO.
struct ad4851_state *state = iio_priv(indio_dev);
struct device *dev = &state->spi.dev;
is contained entirely in this driver code so perhaps a more resilient path to
that device.
Same for other places where you need to get to that device.
> + struct iio_chan_spec *ad4851_channels;
> + const struct iio_chan_spec ad4851_chan = AD4858_IIO_CHANNEL(0, 0, 0);
> + const struct iio_chan_spec ad4851_chan_diff = AD4858_IIO_CHANNEL(0, 0, 1);
> + unsigned int reg;
> + int ret;
> +
> + ret = ad4851_parse_channels(indio_dev, &ad4851_channels, ad4851_chan, ad4851_chan_diff);
> + if (ret)
> + return ret;
> +
> + device_for_each_child_node_scoped(dev, child) {
> + ret = fwnode_property_read_u32(child, "reg", ®);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Missing channel number\n");
> + if (fwnode_property_present(child, "bipolar")) {
> + ad4851_channels->ext_scan_type = ad4851_scan_type_20_1;
> + ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_1);
> +
> + } else {
> + ad4851_channels->ext_scan_type = ad4851_scan_type_20_0;
> + ad4851_channels->num_ext_scan_type = ARRAY_SIZE(ad4851_scan_type_20_0);
> + }
> + ad4851_channels++;
> + }
> +
> + return 0;
> +}
> +
> +/* parse_channel() function populates of the rest of the chip_info fields */
This comment is confusing. Whilst there are other fields that are filled
by parsing the channels, they aren't in anything called chip_info.
> +static const struct ad4851_chip_info ad4851_info = {
> + .name = "ad4851",
> + .product_id = 0x67,
> + .max_sample_rate_hz = 250 * KILO,
> + .resolution = 16,
> + .parse_channels = ad4857_parse_channels,
> +};
> +static int ad4851_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct device *dev = &spi->dev;
> + struct ad4851_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_mutex_init(dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_bulk_get_enable(dev,
> + ARRAY_SIZE(ad4851_power_supplies),
> + ad4851_power_supplies);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to get and enable supplies\n");
> +
> + ret = devm_regulator_get_enable_optional(dev, "vddh");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "failed to enable vddh voltage\n");
> +
> + ret = devm_regulator_get_enable_optional(dev, "vddl");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(dev, ret, "failed to enable vddl voltage\n");
> +
> + st->vrefbuf = devm_regulator_get_optional(dev, "vrefbuf");
Please add a comment here and for vrefio below to say why you don't enable
these yet. I assume that's to do with setting device state before enabling
the regulator. If that ordering isn't actually required, then enabling them
here probably makes more sense. Then just store a flag/bool to say if they were
enodev or not rather than keeping the regulator pointer around.
In many cases I'd expect these to be provided precision references that are always
on so I hope the device doesn't need them to be off until it is configured.
> + if (IS_ERR(st->vrefbuf)) {
> + if (PTR_ERR(st->vrefbuf) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(st->vrefbuf),
> + "Failed to get vrefbuf regulator\n");
> + }
> +
> + st->vrefio = devm_regulator_get_optional(dev, "vrefio");
> + if (IS_ERR(st->vrefio)) {
> + if (PTR_ERR(st->vrefio) != -ENODEV)
> + return dev_err_probe(dev, PTR_ERR(st->vrefio),
> + "Failed to get vrefio regulator\n");
> + }
> +
> + st->pd_gpio = devm_gpiod_get_optional(dev, "pd", GPIOD_OUT_LOW);
> + if (IS_ERR(st->pd_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->pd_gpio),
> + "Error on requesting pd GPIO\n");
> +
> + st->cnv = devm_pwm_get(dev, NULL);
> + if (IS_ERR(st->cnv))
> + return dev_err_probe(dev, PTR_ERR(st->cnv),
> + "Error on requesting pwm\n");
> +
> + ret = devm_add_action_or_reset(&st->spi->dev, ad4851_pwm_disable,
> + st->cnv);
At this point it's not turned on. So not appropriate to turn it off automatically
just yet.
> + if (ret)
> + return ret;
> +
> + st->info = spi_get_device_match_data(spi);
> + if (!st->info)
> + return -ENODEV;
> +
> + st->regmap = devm_regmap_init_spi(spi, ®map_config);
> + if (IS_ERR(st->regmap))
> + return PTR_ERR(st->regmap);
> +
> + ret = ad4851_set_sampling_freq(st, HZ_PER_MHZ);
> + if (ret)
> + return ret;
The call above is what enables the PWM I think. So should have
the devm_add_action_or_reset() to turn it off down here.
Note I gave this feedback on an earlier version. Please make sure to
either address all comments in next posting or reply to the review to
given a reason they are not appropriate / correct etc.
> +
> + ret = ad4851_setup(st);
> + if (ret)
> + return ret;
> +}
