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); > +}