On Fri, 1 Nov 2024 13:23:58 +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.
Various things inline to add to David's much more thorough review.
Jonathan
> +enum {
> + AD4851_SCAN_TYPE_NORMAL,
> + AD4851_SCAN_TYPE_RESOLUTION_BOOST,
> +};
> +
> +struct ad4851_state {
> + struct spi_device *spi;
> + struct pwm_device *cnv;
> + struct iio_backend *back;
> + /*
> + * Synchronize access to members the of driver state, and ensure
> + * atomicity of consecutive regmap operations.
> + */
> + struct mutex lock;
> + struct regmap *regmap;
> + struct regulator *vrefbuf;
> + struct regulator *vrefio;
> + const struct ad4851_chip_info *info;
> + struct gpio_desc *pd_gpio;
> + bool resolution_boost_enabled;
> + unsigned long sampling_freq;
> + unsigned int (*scales)[2];
As described below consider clearer type definition by using
a structure.
> + int *offsets;
> +};
>
> +static int ad4851_setup(struct ad4851_state *st)
> +{
> + unsigned int product_id;
> + int ret;
> +
> + if (st->pd_gpio) {
Not obvious why you are pulsing twice. Add a comment.
> + gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> + fsleep(1);
> + gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> + fsleep(1);
> + gpiod_set_value(st->pd_gpio, GPIOD_OUT_HIGH);
> + fsleep(1);
> + gpiod_set_value(st->pd_gpio, GPIOD_OUT_LOW);
> + fsleep(1000);
> + }
...
> +static int ad4851_calibrate(struct ad4851_state *st)
> +{
> + unsigned int opt_delay, lane_num, delay, i, s, c;
> + enum iio_backend_interface_type interface_type;
> + DECLARE_BITMAP(pn_status, AD4851_MAX_LANES * AD4851_MAX_IODELAY);
> + bool status;
> + int ret;
> +
> + ret = iio_backend_interface_type_get(st->back, &interface_type);
> + if (ret)
> + return ret;
> +
> + switch (interface_type) {
> + case IIO_BACKEND_INTERFACE_SERIAL_CMOS:
> + lane_num = st->info->num_channels;
num_lanes is probably a better name as lane_num sounds like the number
of a particular lane rather than now many there are.
> + break;
> + case IIO_BACKEND_INTERFACE_SERIAL_LVDS:
> + lane_num = 1;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (st->info->resolution == 16) {
> + ret = iio_backend_data_size_set(st->back, 24);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> + AD4851_TEST_PAT | AD4857_PACKET_SIZE_24);
> + if (ret)
> + return ret;
> + } else {
> + ret = iio_backend_data_size_set(st->back, 32);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4851_REG_PACKET,
> + AD4851_TEST_PAT | AD4858_PACKET_SIZE_32);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < st->info->num_channels; i++) {
> + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_0(i),
> + AD4851_TESTPAT_0_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_1(i),
> + AD4851_TESTPAT_1_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_2(i),
> + AD4851_TESTPAT_2_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, AD4851_REG_TESTPAT_3(i),
> + AD4851_TESTPAT_3_DEFAULT(i));
> + if (ret)
> + return ret;
> +
> + ret = iio_backend_chan_enable(st->back, i);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < lane_num; i++) {
> + for (delay = 0; delay < AD4851_MAX_IODELAY; delay++) {
> + ret = iio_backend_iodelay_set(st->back, i, delay);
> + if (ret)
> + return ret;
Blank line here is more consistent with surounding code (which looks better
than this block does).
> + ret = iio_backend_chan_status(st->back, i, &status);
> + if (ret)
> + return ret;
> +
> + if (status)
> + set_bit(i * AD4851_MAX_IODELAY + delay, pn_status);
> + else
> + clear_bit(i * AD4851_MAX_IODELAY + delay, pn_status);
> + }
> + }
> +
> + for (i = 0; i < lane_num; i++) {
> + status = test_bit(i * AD4851_MAX_IODELAY, pn_status);
> + c = ad4851_find_opt(&status, AD4851_MAX_IODELAY, &s);
> + if (c < 0)
> + return c;
> +
> + opt_delay = s + c / 2;
> + ret = iio_backend_iodelay_set(st->back, i, opt_delay);
> + if (ret)
> + return ret;
> + }
> +
> + for (i = 0; i < st->info->num_channels; i++) {
> + ret = iio_backend_chan_disable(st->back, i);
> + if (ret)
> + return ret;
> + }
> +
> + ret = iio_backend_data_size_set(st->back, 20);
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD4851_REG_PACKET, 0);
> +}
> +
> +static int ad4851_set_calibscale(struct ad4851_state *st, int ch, int val,
> + int val2)
> +{
> + u64 gain;
> + u8 buf[0];
As per my very brief review the other day, 0 length is a problem.
> + int ret;
> +
> + if (val < 0 || val2 < 0)
> + return -EINVAL;
> +
> + gain = val * MICRO + val2;
> + gain = DIV_U64_ROUND_CLOSEST(gain * 32768, MICRO);
> +
> + put_unaligned_be16(gain, buf);
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_write(st->regmap, AD4851_REG_CHX_GAIN_MSB(ch),
> + buf[0]);
Don't wrap so early. It fits under 80 chars on oneline.
> + if (ret)
> + return ret;
> +
> + return regmap_write(st->regmap, AD4851_REG_CHX_GAIN_LSB(ch),
> + buf[1]);
> +}
> +
> +static const unsigned int ad4851_scale_table[][2] = {
I'd suggest a structure for each entry. Then you can do
ad4851_scale_table[i].reg_val etc
rather than opaque indexing to 0 or 1.
> + { 2500, 0x0 },
> + { 5000, 0x1 },
> + { 5000, 0x2 },
> + { 10000, 0x3 },
> + { 6250, 0x04 },
> + { 12500, 0x5 },
> + { 10000, 0x6 },
> + { 20000, 0x7 },
> + { 12500, 0x8 },
> + { 25000, 0x9 },
> + { 20000, 0xA },
> + { 40000, 0xB },
> + { 25000, 0xC },
> + { 50000, 0xD },
> + { 40000, 0xE },
> + { 80000, 0xF },
> +};
...
> +
> +static int ad4851_get_scale(struct ad4851_state *st,
> + const struct iio_chan_spec *chan, int *val,
> + int *val2)
> +{
> + int i, softspan_val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, AD4851_REG_CHX_SOFTSPAN(chan->channel),
> + &softspan_val);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < ARRAY_SIZE(ad4851_scale_table); i++) {
> + if (softspan_val == ad4851_scale_table[i][1])
See above - I'd use a structure for the scale_table entries
so this and the [0] below can be named structure elements.
> + break;
> + }
> +
> + if (i == ARRAY_SIZE(ad4851_scale_table))
> + return -EIO;
> +
> + __ad4851_get_scale(st, ad4851_scale_table[i][0], val, val2);
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad4851_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
struct device *dev = *spi->dev;
will shorten a lot of code in here so probably worth doing.
> + struct ad4851_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_mutex_init(&spi->dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_bulk_get_enable(&spi->dev,
> + ARRAY_SIZE(ad4851_power_supplies),
> + ad4851_power_supplies);
> + if (ret)
> + return dev_err_probe(&spi->dev, ret,
> + "failed to get and enable supplies\n");
> +
> + ret = devm_regulator_get_enable_optional(&spi->dev, "vddh");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(&spi->dev, ret, "failed to get vddh voltage\n");
You aren't trying to get the voltage, just turn it on. So update the commit
message to "failed to enable vddh\n"
> +
> + ret = devm_regulator_get_enable_optional(&spi->dev, "vddl");
> + if (ret < 0 && ret != -ENODEV)
> + return dev_err_probe(&spi->dev, ret, "failed to get vddl voltage\n");
Same here.
> +
> + st->vrefbuf = devm_regulator_get_optional(&spi->dev, "vrefbuf");
> + if (IS_ERR(st->vrefbuf)) {
> + if (PTR_ERR(st->vrefbuf) != -ENODEV)
> + return dev_err_probe(&spi->dev, PTR_ERR(st->vrefbuf),
> + "Failed to get vrefbuf regulator\n");
> + }
> +
> + st->vrefio = devm_regulator_get_optional(&spi->dev, "vrefio");
> + if (IS_ERR(st->vrefio)) {
> + if (PTR_ERR(st->vrefio) != -ENODEV)
> + return dev_err_probe(&spi->dev, PTR_ERR(st->vrefio),
> + "Failed to get vrefbuf regulator\n");
wrong regulator in the message.
> + }
> +
> + st->pd_gpio = devm_gpiod_get_optional(&spi->dev, "pd", GPIOD_OUT_LOW);
> + if (IS_ERR(st->pd_gpio))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->pd_gpio),
> + "Error on requesting pd GPIO\n");
> +
> + st->cnv = devm_pwm_get(&spi->dev, NULL);
> + if (IS_ERR(st->cnv))
> + return dev_err_probe(&spi->dev, PTR_ERR(st->cnv),
> + "Error on requesting pwm\n");
> +
> + 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_scale_fill(st);
> + if (ret)
> + return ret;
> +
> + ret = ad4851_setup(st);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = st->info->name;
> + indio_dev->channels = st->info->channels;
> + indio_dev->num_channels = st->info->num_channels;
> + indio_dev->info = &ad4851_iio_info;
Whilst it's not actively used, current best practice is to set
indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + st->back = devm_iio_backend_get(&spi->dev, NULL);
> + if (IS_ERR(st->back))
> + return PTR_ERR(st->back);
> +
> + ret = devm_iio_backend_request_buffer(&spi->dev, st->back, indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_backend_enable(&spi->dev, st->back);
> + if (ret)
> + return ret;
> +
> + ret = ad4851_calibrate(st);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
