Hi David, On Fri, Apr 25, 2025 at 06:13:48PM -0500, David Lechner wrote: > On 4/22/25 6:34 AM, Jorge Marques wrote: > > The AD4052/AD4058/AD4050/AD4056 are versatile, 16-bit/12-bit, > > successive approximation register (SAR) analog-to-digital converter (ADC) > > that enables low-power, high-density data acquisition solutions without > > sacrificing precision. > > This ADC offers a unique balance of performance and power efficiency, > > plus innovative features for seamlessly switching between high-resolution > > and low-power modes tailored to the immediate needs of the system. > > The AD4052/AD4058/AD4050/AD4056 are ideal for battery-powered, > > compact data acquisition and edge sensing applications. > > > > Signed-off-by: Jorge Marques <jorge.marques@xxxxxxxxxx> > > --- > > MAINTAINERS | 1 + > > drivers/iio/adc/Kconfig | 14 + > > drivers/iio/adc/Makefile | 1 + > > drivers/iio/adc/ad4052.c | 1425 ++++++++++++++++++++++++++++++++++++++++++++++ > > This patch is way too big, so I didn't review most of it yet. But time to call > it quits for today. In the future, it would be a lot easier for reviewers if > you can split things into multiple patches instead of implementing all of the > features at once. E.g. start with just a basic driver, then a patch to add > oversampling support, then another patch to add SPI offload support. 500 lines > is a more manageable size for review. > Ack. I split v3 into three commits: base, offload and iio events. My playground is here: https://github.com/gastmaier/adi-linux/pull/9 > ... > > > +static int ad4052_update_xfer_offload(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan) > > +{ > > + struct ad4052_state *st = iio_priv(indio_dev); > > + const struct iio_scan_type *scan_type; > > + struct spi_transfer *xfer = &st->xfer; > > + > > + scan_type = iio_get_current_scan_type(indio_dev, chan); > > + > > + if (IS_ERR(scan_type)) > > + return PTR_ERR(scan_type); > > + > > + xfer = &st->offload_xfer; > > + xfer->bits_per_word = scan_type->realbits; > > + xfer->len = BITS_TO_BYTES(scan_type->storagebits); > > This doesn't work for oversampling. realbits may be 16 while storagebits is 32. > But the SPI controller needs to know how many realbits-sized words to read. > > So this should be > > xfer->len = BITS_TO_BYTES(scan_type->realbits); > Agreed, but due to spi message optimization needs to be: xfer->len = scan_type->realbits == 24 ? 4 : 2; Because 3 bytes cannot be optimized. > > + > > + spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1); > > + st->offload_msg.offload = st->offload; > > + > > + return spi_optimize_message(st->spi, &st->offload_msg); > > I know it is like this in a few other drivers already, but I don't like having > spi_optimize_message() in this funtion because it makes it really easy to > forget to do have balanced calls to spi_unoptimize_message(). > Ack. > > +} > > + > > ... > > > +static const struct iio_buffer_setup_ops ad4052_buffer_setup_ops = { > > + .postenable = &ad4052_buffer_postenable, > > + .predisable = &ad4052_buffer_predisable, > > +}; > > Would be nice to add "offload" to the name of this struct and the callbacks > to make it clear that these are only for the SPI offload use case. > Ack. > ... > > > + > > +static bool ad4052_offload_trigger_match(struct spi_offload_trigger *trigger, > > + enum spi_offload_trigger_type type, > > + u64 *args, u32 nargs) > > +{ > > We should be checking the args here according to what I suggested in my reply > to the devicetree bindings patch. Right now it is assuming that we are only > using this for SPI offload and that the pin used is GP1 and the event is data > read. We should at least verify that the args match those assumptions. > > For bonus points, we could implement allowing GPO as well. > Yes, it is assuming as you mentioned. I'm okay with "at least verifying". but then I need to look-up at the parent node first, since it resides at the spi-controller node, if that's ok. > > + return type == SPI_OFFLOAD_TRIGGER_DATA_READY; > > +} > > + > > +static const struct spi_offload_trigger_ops ad4052_offload_trigger_ops = { > > + .match = ad4052_offload_trigger_match, > > +}; > > + > > +static int ad4052_request_offload(struct iio_dev *indio_dev) > > +{ > > + struct ad4052_state *st = iio_priv(indio_dev); > > + struct device *dev = &st->spi->dev; > > + struct dma_chan *rx_dma; > > + struct spi_offload_trigger_info trigger_info = { > > + .fwnode = dev_fwnode(dev), > > + .ops = &ad4052_offload_trigger_ops, > > + .priv = st, > > + }; > > + struct pwm_state pwm_st; > > + int ret; > > + > > + indio_dev->setup_ops = &ad4052_buffer_setup_ops; > > + > > + ret = devm_spi_offload_trigger_register(dev, &trigger_info); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "failed to register offload trigger\n"); > > Strictly speaking, the trigger-source provider is indendant of using it for > SPI offload. I guess this is fine here for now though. > Ok. > > + > > + st->offload_trigger = devm_spi_offload_trigger_get(dev, st->offload, > > + SPI_OFFLOAD_TRIGGER_DATA_READY); > > + if (IS_ERR(st->offload_trigger)) > > + return PTR_ERR(st->offload_trigger); > > + > > + st->cnv_pwm = devm_pwm_get(dev, NULL); > > + if (IS_ERR(st->cnv_pwm)) > > + return dev_err_probe(dev, PTR_ERR(st->cnv_pwm), > > + "failed to get CNV PWM\n"); > > + > > + pwm_init_state(st->cnv_pwm, &pwm_st); > > + > > + pwm_st.enabled = false; > > + pwm_st.duty_cycle = AD4052_T_CNVH_NS * 2; > > + pwm_st.period = DIV_ROUND_UP_ULL(NSEC_PER_SEC, > > + AD4052_MAX_RATE(st->grade)); > > + > > + ret = pwm_apply_might_sleep(st->cnv_pwm, &pwm_st); > > + if (ret) > > + return dev_err_probe(dev, ret, "failed to apply CNV PWM\n"); > > + > > + ret = devm_add_action_or_reset(dev, ad4052_pwm_disable, st->cnv_pwm); > > + if (ret) > > + return ret; > > + > > + rx_dma = devm_spi_offload_rx_stream_request_dma_chan(dev, st->offload); > > + if (IS_ERR(rx_dma)) > > + return PTR_ERR(rx_dma); > > + > > + return devm_iio_dmaengine_buffer_setup_with_handle(dev, indio_dev, rx_dma, > > + IIO_BUFFER_DIRECTION_IN); > > +} > > + > > +static int ad4052_probe(struct spi_device *spi) > > +{ > > + const struct ad4052_chip_info *chip; > > + struct device *dev = &spi->dev; > > + struct iio_dev *indio_dev; > > + struct ad4052_state *st; > > + int ret = 0; > > + > > + chip = spi_get_device_match_data(spi); > > + if (!chip) > > + return dev_err_probe(dev, -ENODEV, > > + "Could not find chip info data\n"); > > + > > + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); > > + if (!indio_dev) > > + return -ENOMEM; > > + > > + st = iio_priv(indio_dev); > > + st->spi = spi; > > + spi_set_drvdata(spi, st); > > + init_completion(&st->completion); > > + > > + st->regmap = devm_regmap_init_spi(spi, &ad4052_regmap_config); > > + if (IS_ERR(st->regmap)) > > + return dev_err_probe(dev, PTR_ERR(st->regmap), > > + "Failed to initialize regmap\n"); > > + > > + st->mode = AD4052_SAMPLE_MODE; > > + st->wait_event = false; > > + st->chip = chip; > > + st->grade = chip->prod_id <= 0x75 ? AD4052_2MSPS : AD4052_500KSPS; > > + st->oversampling_frequency = AD4052_FS_OFFSET(st->grade); > > + st->events_frequency = AD4052_FS_OFFSET(st->grade); > > Somewhere around here, we should be turning on the power supplies. Also, it > looks like we need some special handling to get the reference volage. If there > is a supply connected to REF, use that, if not, use VDD which requires writing > to a register to let the chip know. > Yes, v3 will add regulators. Vref can be sourced from either REF (default) or VDD. So the idea is, if REF node not provided, set VDD as REF? > > + > > + st->cnv_gp = devm_gpiod_get_optional(dev, "cnv", GPIOD_OUT_LOW); > > + if (IS_ERR(st->cnv_gp)) > > + return dev_err_probe(dev, PTR_ERR(st->cnv_gp), > > + "Failed to get cnv gpio\n"); > > + > > + indio_dev->modes = INDIO_BUFFER_HARDWARE | INDIO_DIRECT_MODE; > > INDIO_BUFFER_HARDWARE should not be set here. If using SPI offload, > devm_iio_dmaengine_buffer_setup_with_handle() will add it automatically. > For non-SPI-offload operation, it should not be set. > Ack. > > + indio_dev->num_channels = 1; > > + indio_dev->info = &ad4052_info; > > + indio_dev->name = chip->name; > > + > > + st->offload = devm_spi_offload_get(dev, spi, &ad4052_offload_config); > > This > > > + if (IS_ERR(st->offload)) > > + return PTR_ERR(st->offload); > > should be > > ret = PTR_ERR_OR_ZERO(st->offload); > Ack. > > + > > + if (ret && ret != -ENODEV) > > + return dev_err_probe(dev, ret, "Failed to get offload\n"); > > + > > + if (ret == -ENODEV) { > > + st->offload_trigger = NULL; > > + indio_dev->channels = chip->channels; > > + } else { > > + indio_dev->channels = chip->offload_channels; > > + ret = ad4052_request_offload(indio_dev); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to configure offload\n"); > > + } > > + > > + st->xfer.rx_buf = &st->d32; > > I don't think we want this set globally. I.e. it doesn't make sense for SPI > offload xfers. > Ack. > > + > > + ret = ad4052_soft_reset(st); > > + if (ret) > > + return dev_err_probe(dev, ret, "AD4052 failed to soft reset\n"); > > + > > + ret = ad4052_check_ids(st); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "AD4052 fields assertions failed\n"); > > + > > + ret = ad4052_setup(indio_dev, indio_dev->channels); > > + if (ret) > > + return ret; > > + > > + ret = regmap_write(st->regmap, AD4052_REG_DEVICE_STATUS, > > + AD4052_DEVICE_STATUS_DEVICE_RESET); > > Why not include this in ad4052_setup() or even ad4052_soft_reset()? > Ack. But on setup to not write registers before doing the sanity test. > > + if (ret) > > + return ret; > > + > > + ret = ad4052_request_irq(indio_dev); > > + if (ret) > > + return ret; > > + > > + ad4052_update_xfer_raw(indio_dev, indio_dev->channels); > > + > > + pm_runtime_set_active(dev); > > + ret = devm_pm_runtime_enable(dev); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to enable pm_runtime\n"); > > + > > + pm_runtime_set_autosuspend_delay(dev, 1000); > > + pm_runtime_use_autosuspend(dev); > > + > > + return devm_iio_device_register(dev, indio_dev); > > +} > > + > > +static int ad4052_runtime_suspend(struct device *dev) > > +{ > > + struct ad4052_state *st = dev_get_drvdata(dev); > > + > > + return regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, > > + FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, > > + AD4052_DEVICE_CONFIG_LOW_POWER_MODE)); > > +} > > + > > +static int ad4052_runtime_resume(struct device *dev) > > +{ > > + struct ad4052_state *st = dev_get_drvdata(dev); > > + int ret; > > + > > + ret = regmap_write(st->regmap, AD4052_REG_DEVICE_CONFIG, > > + FIELD_PREP(AD4052_DEVICE_CONFIG_POWER_MODE_MSK, 0)); > > regmap_clear_bits() would be shorter if there isn't going to be a macro to > explain the meaning of 0. > Ack. > > + return ret; > > +} > > + Regards, Jorge
