Message ID | 20240905-wip-bl-ad3552r-axi-v0-iio-testing-v2-8-87d669674c00@baylibre.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | iio: add support for the ad3552r AXI DAC IP | expand |
On 9/5/24 10:17 AM, Angelo Dureghello wrote: ... > + > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err, ch = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: { > + int clk_rate; > + > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > + IIO_CHAN_INFO_FREQUENCY); This seems odd to me. How does the backend know what frequency we want? It would make more sense to me if this somehow indicated that we were getting the SPI SCLK rate. > + if (err != IIO_VAL_INT) Would be better to call the variable ret instead of err if it can hold something besides an error code. > + return err; > + > + /* > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > + * Samplerate has sense in DDR only. We should also mention that this assumes QSPI in addtion to DDR enabled. > + */ > + if (st->single_channel) > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > + else > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > + Having the sample rate depend on how many channels are enabled in the buffer seems a bit odd. Sampling frequency is not strictly defined in IIO, so I think it would be fine to always return the same value no matter how many channels are enabled. We will just need to document that the sampling frequency is the rate per sample, not per channel. So if two channels are enabled, the effective sampling rate per channel is 1/2 of the sampling rate reported by the sysfs attribute. > + *val = clk_rate; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: Do we need iio_device_claim_direct_scoped() here to prevent attempting to do register access while a buffered write is in progress? > + err = iio_backend_bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), > + val, 2); > + if (err) > + return err; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int ch = chan->channel; > + > + return iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), val, 2); > + } > + unreachable(); > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) > + goto exit_err; Might want a comment or dev_warn() here. If we put the DAC in DDR mode but can't put the backend in DDR mode, then things are probably going to be a bit broken if we can't get the DAC back out of DDR mode. Not likely to ever get an error here, but it will be helpful to let readers of the code know why the unwinding isn't exactly balanced. > +> + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = AD3552R_STREAM_4BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + return -EINVAL; Move the switch statement before changing to DDR mode or goto exit_err here. > + } > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); > + if (err) > + goto exit_err; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err; > + > + /* > + * The EXT_SYNC is mandatory in the CN0585 project where 2 instances > + * of the IP are in the design and they need to generate the signals > + * synchronized. > + * > + * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0, > + * but EXT_SYMC (ext synch ability) is enabled anyway. EXT_SYNC > + */ > + if (st->synced_transfer == AD3552R_EXT_SYNC_ARM) > + err = iio_backend_ext_sync_enable(st->back); > + else > + err = iio_backend_ext_sync_disable(st->back); > + if (err) > + goto exit_err_sync; goto exit_err; If enabling failed, no need to disable. > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_buffer_enable(st->back); > + if (err) > + goto exit_err_sync; > + > + return 0; > + > +exit_err_sync: > + iio_backend_ext_sync_disable(st->back); > + > +exit_err: > + axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + return err; > +} > + > +static int ad3552r_axi_buffer_predisable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err; > + > + err = iio_backend_buffer_disable(st->back); > + if (err) > + return err; Looks like iio_backend_ext_sync_disable(st->back); should be called here. > + > + /* Inform DAC to set in SDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + if (err) > + return err; > + > + return iio_backend_ddr_disable(st->back); > +} > +
On Thu, 5 Sep 2024 15:40:11 -0500 David Lechner <dlechner@baylibre.com> wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > ... One reply to a comment David made. Jonathan > > > + */ > > + if (st->single_channel) > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > > + else > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > > + > > Having the sample rate depend on how many channels are enabled in > the buffer seems a bit odd. Sampling frequency is not strictly > defined in IIO, so I think it would be fine to always return the > same value no matter how many channels are enabled. > > We will just need to document that the sampling frequency is the > rate per sample, not per channel. So if two channels are enabled, > the effective sampling rate per channel is 1/2 of the sampling > rate reported by the sysfs attribute. There is an oddity around this that we've never cleared up fully. In my head at least if there is a single sampling_frequency it applies to 'scans', not individual channel reads (so would change with the number of channels enabled). If there is a per channel attribute we do have documentation: What: /sys/bus/iio/devices/iio:deviceX/in_voltageX_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_powerY_sampling_frequency What: /sys/bus/iio/devices/iio:deviceX/in_currentZ_sampling_frequency KernelVersion: 5.20 Contact: linux-iio@vger.kernel.org Description: Some devices have separate controls of sampling frequency for individual channels. If multiple channels are enabled in a scan, then the sampling_frequency of the scan may be computed from the per channel sampling frequencies. For many devices the sampling frequency isn't down to each sample taking N microsecs and them running back to back, it is instead a function of a periodic sampling start for samples that take the same time whatever the sampling frequency. Also for simultaneous sampling ADCs it is never channel dependent. So I think if you want to avoid the confusion, make your device fall into the description above and provide a per channel attribute rather than shared_by_all. Or keep it as things stand and have it halve when you double the channels. > > > + *val = clk_rate; > > + > > + return IIO_VAL_INT; > > + } ...
On Thu, 05 Sep 2024 17:17:38 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > From: Angelo Dureghello <adureghello@baylibre.com> > > Add support for ad3552r-axi, where ad3552r has to be controlled > by the custom (fpga-based) ad3552r AXI DAC IP. > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com> > Co-developed-by: David Lechner <dlechner@baylibre.com> > Co-developed-by: Nuno Sá <nuno.sa@analog.com> Comments inline. > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile > index 56a125f56284..cc2af3aa3f52 100644 > --- a/drivers/iio/dac/Makefile > +++ b/drivers/iio/dac/Makefile > @@ -5,6 +5,7 @@ > > # When adding new entries keep the list in alphabetical order > obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o > +obj-$(CONFIG_AD3552R_AXI) += ad3552r-axi.o ad3552r-common.o > obj-$(CONFIG_AD5360) += ad5360.o > obj-$(CONFIG_AD5380) += ad5380.o > obj-$(CONFIG_AD5421) += ad5421.o > diff --git a/drivers/iio/dac/ad3552r-axi.c b/drivers/iio/dac/ad3552r-axi.c > new file mode 100644 > index 000000000000..a9311f29f45d > --- /dev/null > +++ b/drivers/iio/dac/ad3552r-axi.c > @@ -0,0 +1,567 @@ > + > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + int err, ch = chan->channel; > + > + switch (mask) { > + case IIO_CHAN_INFO_SAMP_FREQ: { > + int clk_rate; > + > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > + IIO_CHAN_INFO_FREQUENCY); > + if (err != IIO_VAL_INT) > + return err; If it is another postive value I think you want to return -EINVAL If it's negative return err as here. > + > + /* > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > + * Samplerate has sense in DDR only. > + */ > + if (st->single_channel) > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > + else > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > + > + *val = clk_rate; > + > + return IIO_VAL_INT; > + } > + case IIO_CHAN_INFO_RAW: > + err = iio_backend_bus_reg_read(st->back, > + AD3552R_REG_ADDR_CH_DAC_16B(ch), > + val, 2); > + if (err) > + return err; > + > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > +} > + > +static int ad3552r_axi_set_output_range(struct ad3552r_axi_state *st, > + unsigned int mode) > +{ > + int range_ch_0 = FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode); > + int range_ch_1 = FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode); > + > + return axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, > + AD3552R_MASK_CH_OUTPUT_RANGE, > + range_ch_0 | range_ch_1, 1); return axi3552r_qspi_update_reg_bits(st->back, AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, AD3552R_MASK_CH_OUTPUT_RANGE, FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode) | FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode), 1); looks fine to me from readability point of view and it's shorter. > +} > + > +static int ad3552r_axi_setup(struct ad3552r_axi_state *st) > +{ > + struct fwnode_handle *child __free(fwnode_handle) = NULL; > + u8 gs_p, gs_n; > + s16 goffs; > + u16 id, rfb; > + u16 reg = 0, offset = 0; > + u32 val, range; > + int err; > + > + err = ad3552r_axi_reset(st); > + if (err) > + return err; > + > + err = iio_backend_ddr_disable(st->back); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL1, val); > + return -EIO; > + } > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_SCRATCH_PAD, > + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); > + if (err) > + return err; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, > + &val, 1); > + if (err) > + return err; > + > + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { > + dev_err(st->dev, > + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_SCRATCH_PAD_TEST_VAL2, val); > + return -EIO; > + } This scratch pad test is a separate enough 'thing' maybe pull it out as another function called from here? > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, > + &val, 1); > + if (err) > + return err; > + > + id = val; > + > + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, > + &val, 1); > + if (err) > + return err; > + > + id |= val << 8; > + if (id != st->model_data->chip_id) { > + dev_err(st->dev, "Chip ID mismatch. Expected 0x%x, Read 0x%x\n", > + AD3552R_ID, id); > + } no {} needed here. Also dev_info() to make it slightly less ominous :) > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_REF_INIT, 1); Hmm. This was snuck in during previous patch. Pull new definitions out of that and put them in this patch. I only noticed because I wondered what value it had and was surprised to find it doesn't exist in current driver. I'm not sure a define for write it all to 0 is worth while. Maybe just put a zero here. > + if (err) > + return err; > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_TRANSFER_REGISTER, > + AD3552R_TRANSFER_INIT, 1); Another define that sneaked in during previous patch. Given it's not 'general' and only used here. I'd drop that define and use the various parts that make it up here. Mind you those defines should be introduced in this patch not the previous one. > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); > + if (err) > + return err; > + > + err = ad3552r_get_ref_voltage(st->dev, &val); > + if (err) > + return err; > + > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, > + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, > + val, 1); > + if (err) > + return err; > + > + err = ad3552r_get_drive_strength(st->dev, &val); > + if (!err) { > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SDO_DRIVE_STRENGTH, > + val, 1); > + if (err) > + return err; > + } > + > + child = device_get_named_child_node(st->dev, "channel"); > + if (!child) > + return -EINVAL; > + > + err = ad3552r_get_output_range(st->dev, st->model_data->chip_id, > + child, &range); > + if (!err) > + return ad3552r_axi_set_output_range(st, range); > + > + if (err != -ENOENT) > + return err; > + > + /* Try to get custom range */ > + err = ad3552r_get_custom_gain(st->dev, child, > + &gs_p, &gs_n, &rfb, &goffs); > + if (err) > + return err; > + > + ad3552r_calc_custom_gain(gs_p, gs_n, goffs, ®); I'd return regs > + > + offset = abs((s32)goffs); Hmm. abs(goffs) should use a short I think which will work without the cast and ultimately rely on sign extension of the result. > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_OFFSET(0), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_OFFSET(1), > + offset, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_GAIN(0), > + reg, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + err = iio_backend_bus_reg_write(st->back, > + AD3552R_REG_ADDR_CH_GAIN(1), > + reg, 1); > + if (err) > + return dev_err_probe(st->dev, err, > + "Error writing register\n"); > + > + return 0; > +} ... > + > +static const struct iio_chan_spec_ext_info ad3552r_axi_ext_info[] = { > + IIO_ENUM("synchronous_mode", IIO_SHARED_BY_TYPE, > + &ad3552r_synchronous_mode_enum), > + IIO_ENUM_AVAILABLE("synchronous_mode", IIO_SHARED_BY_TYPE, > + &ad3552r_synchronous_mode_enum), > + {} { } Also see discussion in next patch on this. I'm not sure it makes sense to expose this to userspace but maybe I just don't yet understand the use case. > +}; > + > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .ext_info = ad3552r_axi_ext_info, \ > +} > + > +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>"); > +MODULE_AUTHOR("Angelo Dureghello <adueghello@baylibre.com>"); > +MODULE_DESCRIPTION("AD3552R Driver - AXI IP version"); Maybe relax that description to include all potential backends. As long as they keep to the bindings and interfaces, someone else's completely different backend implementation should work with your front end driver. Mind you I can't immediately think of a better name and module descriptions aren't ABI anyway, so maybe we don't care. > +MODULE_LICENSE("GPL"); >
Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > From: Angelo Dureghello <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > Add support for ad3552r-axi, where ad3552r has to be controlled > by the custom (fpga-based) ad3552r AXI DAC IP. ... > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > +{ > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > + struct iio_backend_data_fmt fmt = { > + .type = IIO_BACKEND_DATA_UNSIGNED > + }; > + int loop_len, val, err; > + > + /* Inform DAC chip to switch into DDR mode */ > + err = axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + AD3552R_MASK_SPI_CONFIG_DDR, 1); > + if (err) > + return err; > + > + /* Inform DAC IP to go for DDR mode from now on */ > + err = iio_backend_ddr_enable(st->back); > + if (err) > + goto exit_err; I don't know if it can be an issue, but iio_backend_ddr_disable() is called if iio_backend_ddr_enable() fails. > + > + switch (*indio_dev->active_scan_mask) { > + case AD3552R_CH0_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(0); > + break; > + case AD3552R_CH1_ACTIVE: > + st->single_channel = true; > + loop_len = AD3552R_STREAM_2BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + case AD3552R_CH0_CH1_ACTIVE: > + st->single_channel = false; > + loop_len = AD3552R_STREAM_4BYTE_LOOP; > + val = AD3552R_REG_ADDR_CH_DAC_16B(1); > + break; > + default: > + return -EINVAL; > + } > + > + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, > + loop_len, 1); > + if (err) > + goto exit_err; > + > + err = iio_backend_data_transfer_addr(st->back, val); > + if (err) > + goto exit_err; > + > + /* > + * The EXT_SYNC is mandatory in the CN0585 project where 2 instances > + * of the IP are in the design and they need to generate the signals > + * synchronized. > + * > + * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0, > + * but EXT_SYMC (ext synch ability) is enabled anyway. > + */ > + if (st->synced_transfer == AD3552R_EXT_SYNC_ARM) > + err = iio_backend_ext_sync_enable(st->back); > + else > + err = iio_backend_ext_sync_disable(st->back); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_data_format_set(st->back, 0, &fmt); > + if (err) > + goto exit_err_sync; > + > + err = iio_backend_buffer_enable(st->back); > + if (err) > + goto exit_err_sync; > + > + return 0; > + > +exit_err_sync: > + iio_backend_ext_sync_disable(st->back); > + > +exit_err: > + axi3552r_qspi_update_reg_bits(st->back, > + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > + AD3552R_MASK_SPI_CONFIG_DDR, > + 0, 1); > + > + iio_backend_ddr_disable(st->back); > + > + return err; > +} ... > +#define AD3552R_CHANNEL(ch) { \ > + .type = IIO_VOLTAGE, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ > + .output = 1, \ > + .indexed = 1, \ > + .channel = (ch), \ > + .scan_index = (ch), \ > + .scan_type = { \ > + .sign = 'u', \ > + .realbits = 16, \ > + .storagebits = 16, \ > + .endianness = IIO_BE, \ > + }, \ > + .ext_info = ad3552r_axi_ext_info, \ > +} > + > +static struct iio_chan_spec ad3552r_axi_channels[] = { I think (but I've not checked :)) that it could be const. CJ > + AD3552R_CHANNEL(0), > + AD3552R_CHANNEL(1), > +}; ...
Hi all, Some comments on top of what David already said... On Thu, 2024-09-05 at 15:40 -0500, David Lechner wrote: > On 9/5/24 10:17 AM, Angelo Dureghello wrote: > > ... > > > + > > +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > > + int err, ch = chan->channel; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_SAMP_FREQ: { > > + int clk_rate; > > + > > + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, > > + IIO_CHAN_INFO_FREQUENCY); > > This seems odd to me. How does the backend know what frequency we want? > It would make more sense to me if this somehow indicated that we were > getting the SPI SCLK rate. > Yes, this sampling frequency bit seems very wrong atm. And the thing is, we're not even getting SCLK. According to [1], the /4 and /8 is for clk_in which is not the same as SCLK (unless I'm missing something). OTOH, if in the backend patch, that clk_get() is somehow getting sclk, that's wrong because sclk is an output clk of the IP. So we need to get clk_in which should be (typically) 133MHz. > > + if (err != IIO_VAL_INT) > > Would be better to call the variable ret instead of err if it can hold > something besides an error code. > > > + return err; > > + > > + /* > > + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). > > + * Samplerate has sense in DDR only. > > We should also mention that this assumes QSPI in addtion to DDR enabled. > I understand the QSPI bit but why the DDR part? I just don't understand the comment "Samplerate has sense in DDR only.". It needs way more explanation if that is true... > > + */ > > + if (st->single_channel) > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); > > + else > > + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); > > + > This division also looks to be very backend dependent. So it's far from ideal being in here... To me, the way we need to get this done is for the backend to effectively report back SCLK (in a correct way). Then, depending on the number of bits per clk (4 for QSPI), the word size and DDR vs SDR we get the device sample rate. With it, we then choose one of Jonathan's suggestion (a per channel attr might be less confusing). All the above said, I probably need to catch up on the above. It might happen that David and Angelo already got some more info from the hdl guys while I was on vacation. [1]: https://analogdevicesinc.github.io/hdl/library/axi_ad3552r/index.html - Nuno Sá
On Sun, 2024-09-08 at 18:28 +0200, Christophe JAILLET wrote: > Le 05/09/2024 à 17:17, Angelo Dureghello a écrit : > > From: Angelo Dureghello > > <adureghello-rdvid1DuHRBWk0Htik3J/w@public.gmane.org> > > > > Add support for ad3552r-axi, where ad3552r has to be controlled > > by the custom (fpga-based) ad3552r AXI DAC IP. > > ... > > > +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) > > +{ > > + struct ad3552r_axi_state *st = iio_priv(indio_dev); > > + struct iio_backend_data_fmt fmt = { > > + .type = IIO_BACKEND_DATA_UNSIGNED > > + }; > > + int loop_len, val, err; > > + > > + /* Inform DAC chip to switch into DDR mode */ > > + err = axi3552r_qspi_update_reg_bits(st->back, > > + > > AD3552R_REG_ADDR_INTERFACE_CONFIG_D, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > + AD3552R_MASK_SPI_CONFIG_DDR, > > 1); > > + if (err) > > + return err; > > + > > + /* Inform DAC IP to go for DDR mode from now on */ > > + err = iio_backend_ddr_enable(st->back); > > + if (err) > > + goto exit_err; > > I don't know if it can be an issue, but iio_backend_ddr_disable() is > called if iio_backend_ddr_enable() fails. > > I don't think it would be an issue but conceptually it does not really make sense. Yeah, it should be fixed... - Nuno Sá
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index 1cfd7e2a622f..030af7702a3c 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -16,6 +16,17 @@ config AD3552R To compile this driver as a module, choose M here: the module will be called ad3552r. +config AD3552R_AXI + tristate "Analog Devices AD3552R DAC driver, AXI version" + select IIO_BACKEND + help + Say yes here to build support for Analog Devices AD3552R + Digital to Analog Converter, connected through the Xilinx + fpga AXI interface. + + To compile this driver as a module, choose M here: the + module will be called ad3552r-axi. + config AD5064 tristate "Analog Devices AD5064 and similar multi-channel DAC driver" depends on (SPI_MASTER && I2C!=m) || I2C diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index 56a125f56284..cc2af3aa3f52 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -5,6 +5,7 @@ # When adding new entries keep the list in alphabetical order obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o +obj-$(CONFIG_AD3552R_AXI) += ad3552r-axi.o ad3552r-common.o obj-$(CONFIG_AD5360) += ad5360.o obj-$(CONFIG_AD5380) += ad5380.o obj-$(CONFIG_AD5421) += ad5421.o diff --git a/drivers/iio/dac/ad3552r-axi.c b/drivers/iio/dac/ad3552r-axi.c new file mode 100644 index 000000000000..a9311f29f45d --- /dev/null +++ b/drivers/iio/dac/ad3552r-axi.c @@ -0,0 +1,567 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Analog Devices AD3552R + * Digital to Analog converter driver, AXI DAC backend version + * + * Copyright 2024 Analog Devices Inc. + */ + +#include <linux/bitfield.h> +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/iio/backend.h> +#include <linux/iio/buffer.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/property.h> +#include <linux/units.h> + +#include "ad3552r.h" + +enum ad3552r_synchronous_mode_status { + AD3552R_NO_SYNC, + AD3552R_EXT_SYNC_ARM, +}; + +struct ad3552r_axi_model_data { + const char *model_name; + enum ad3552r_id chip_id; + unsigned int num_hw_channels; +}; + +struct ad3552r_axi_state { + const struct ad3552r_axi_model_data *model_data; + struct gpio_desc *reset_gpio; + struct device *dev; + struct iio_backend *back; + bool single_channel; + bool synced_transfer; +}; + +static int axi3552r_qspi_update_reg_bits(struct iio_backend *back, + u32 reg, u32 mask, u32 val, + size_t xfer_size) +{ + u32 rval; + int err; + + err = iio_backend_bus_reg_read(back, reg, &rval, xfer_size); + if (err) + return err; + + rval &= ~mask; + rval |= val; + + return iio_backend_bus_reg_write(back, reg, rval, xfer_size); +} + +static int ad3552r_axi_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + struct ad3552r_axi_state *st = iio_priv(indio_dev); + int err, ch = chan->channel; + + switch (mask) { + case IIO_CHAN_INFO_SAMP_FREQ: { + int clk_rate; + + err = iio_backend_read_raw(st->back, chan, &clk_rate, 0, + IIO_CHAN_INFO_FREQUENCY); + if (err != IIO_VAL_INT) + return err; + + /* + * Data stream SDR/DDR (clk_in/8 or clk_in/4 update rate). + * Samplerate has sense in DDR only. + */ + if (st->single_channel) + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 4); + else + clk_rate = DIV_ROUND_CLOSEST(clk_rate, 8); + + *val = clk_rate; + + return IIO_VAL_INT; + } + case IIO_CHAN_INFO_RAW: + err = iio_backend_bus_reg_read(st->back, + AD3552R_REG_ADDR_CH_DAC_16B(ch), + val, 2); + if (err) + return err; + + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int ad3552r_axi_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int val, int val2, long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_RAW: + iio_device_claim_direct_scoped(return -EBUSY, indio_dev) { + struct ad3552r_axi_state *st = iio_priv(indio_dev); + int ch = chan->channel; + + return iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_DAC_16B(ch), val, 2); + } + unreachable(); + default: + return -EINVAL; + } +} + +static int ad3552r_axi_buffer_postenable(struct iio_dev *indio_dev) +{ + struct ad3552r_axi_state *st = iio_priv(indio_dev); + struct iio_backend_data_fmt fmt = { + .type = IIO_BACKEND_DATA_UNSIGNED + }; + int loop_len, val, err; + + /* Inform DAC chip to switch into DDR mode */ + err = axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + AD3552R_MASK_SPI_CONFIG_DDR, 1); + if (err) + return err; + + /* Inform DAC IP to go for DDR mode from now on */ + err = iio_backend_ddr_enable(st->back); + if (err) + goto exit_err; + + switch (*indio_dev->active_scan_mask) { + case AD3552R_CH0_ACTIVE: + st->single_channel = true; + loop_len = AD3552R_STREAM_2BYTE_LOOP; + val = AD3552R_REG_ADDR_CH_DAC_16B(0); + break; + case AD3552R_CH1_ACTIVE: + st->single_channel = true; + loop_len = AD3552R_STREAM_2BYTE_LOOP; + val = AD3552R_REG_ADDR_CH_DAC_16B(1); + break; + case AD3552R_CH0_CH1_ACTIVE: + st->single_channel = false; + loop_len = AD3552R_STREAM_4BYTE_LOOP; + val = AD3552R_REG_ADDR_CH_DAC_16B(1); + break; + default: + return -EINVAL; + } + + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_STREAM_MODE, + loop_len, 1); + if (err) + goto exit_err; + + err = iio_backend_data_transfer_addr(st->back, val); + if (err) + goto exit_err; + + /* + * The EXT_SYNC is mandatory in the CN0585 project where 2 instances + * of the IP are in the design and they need to generate the signals + * synchronized. + * + * Note: in first IP implementations CONFIG EXT_SYNC (RO) can be 0, + * but EXT_SYMC (ext synch ability) is enabled anyway. + */ + if (st->synced_transfer == AD3552R_EXT_SYNC_ARM) + err = iio_backend_ext_sync_enable(st->back); + else + err = iio_backend_ext_sync_disable(st->back); + if (err) + goto exit_err_sync; + + err = iio_backend_data_format_set(st->back, 0, &fmt); + if (err) + goto exit_err_sync; + + err = iio_backend_buffer_enable(st->back); + if (err) + goto exit_err_sync; + + return 0; + +exit_err_sync: + iio_backend_ext_sync_disable(st->back); + +exit_err: + axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + 0, 1); + + iio_backend_ddr_disable(st->back); + + return err; +} + +static int ad3552r_axi_buffer_predisable(struct iio_dev *indio_dev) +{ + struct ad3552r_axi_state *st = iio_priv(indio_dev); + int err; + + err = iio_backend_buffer_disable(st->back); + if (err) + return err; + + /* Inform DAC to set in SDR mode */ + err = axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SPI_CONFIG_DDR, + 0, 1); + if (err) + return err; + + return iio_backend_ddr_disable(st->back); +} + +static int ad3552r_axi_set_output_range(struct ad3552r_axi_state *st, + unsigned int mode) +{ + int range_ch_0 = FIELD_PREP(AD3552R_MASK_CH0_RANGE, mode); + int range_ch_1 = FIELD_PREP(AD3552R_MASK_CH1_RANGE, mode); + + return axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_CH0_CH1_OUTPUT_RANGE, + AD3552R_MASK_CH_OUTPUT_RANGE, + range_ch_0 | range_ch_1, 1); +} + +static int ad3552r_axi_reset(struct ad3552r_axi_state *st) +{ + int err; + + st->reset_gpio = devm_gpiod_get_optional(st->dev, + "reset", GPIOD_OUT_LOW); + if (IS_ERR(st->reset_gpio)) + return PTR_ERR(st->reset_gpio); + + if (st->reset_gpio) { + gpiod_set_value_cansleep(st->reset_gpio, 1); + fsleep(10); + gpiod_set_value_cansleep(st->reset_gpio, 0); + } else { + err = axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_A, + AD3552R_MASK_SOFTWARE_RESET, + AD3552R_MASK_SOFTWARE_RESET, 1); + if (err) + return err; + } + msleep(100); + + return 0; +} + +static int ad3552r_axi_setup(struct ad3552r_axi_state *st) +{ + struct fwnode_handle *child __free(fwnode_handle) = NULL; + u8 gs_p, gs_n; + s16 goffs; + u16 id, rfb; + u16 reg = 0, offset = 0; + u32 val, range; + int err; + + err = ad3552r_axi_reset(st); + if (err) + return err; + + err = iio_backend_ddr_disable(st->back); + if (err) + return err; + + err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + AD3552R_SCRATCH_PAD_TEST_VAL1, 1); + if (err) + return err; + + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + &val, 1); + if (err) + return err; + + if (val != AD3552R_SCRATCH_PAD_TEST_VAL1) { + dev_err(st->dev, + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", + AD3552R_SCRATCH_PAD_TEST_VAL1, val); + return -EIO; + } + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_SCRATCH_PAD, + AD3552R_SCRATCH_PAD_TEST_VAL2, 1); + if (err) + return err; + + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_SCRATCH_PAD, + &val, 1); + if (err) + return err; + + if (val != AD3552R_SCRATCH_PAD_TEST_VAL2) { + dev_err(st->dev, + "SCRATCH_PAD_TEST mismatch. Expected 0x%x, Read 0x%x\n", + AD3552R_SCRATCH_PAD_TEST_VAL2, val); + return -EIO; + } + + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_L, + &val, 1); + if (err) + return err; + + id = val; + + err = iio_backend_bus_reg_read(st->back, AD3552R_REG_ADDR_PRODUCT_ID_H, + &val, 1); + if (err) + return err; + + id |= val << 8; + if (id != st->model_data->chip_id) { + dev_err(st->dev, "Chip ID mismatch. Expected 0x%x, Read 0x%x\n", + AD3552R_ID, id); + } + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, + AD3552R_REF_INIT, 1); + if (err) + return err; + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_TRANSFER_REGISTER, + AD3552R_TRANSFER_INIT, 1); + if (err) + return err; + + err = iio_backend_data_source_set(st->back, 0, IIO_BACKEND_EXTERNAL); + if (err) + return err; + + err = iio_backend_data_source_set(st->back, 1, IIO_BACKEND_EXTERNAL); + if (err) + return err; + + err = ad3552r_get_ref_voltage(st->dev, &val); + if (err) + return err; + + err = axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_SH_REFERENCE_CONFIG, + AD3552R_MASK_REFERENCE_VOLTAGE_SEL, + val, 1); + if (err) + return err; + + err = ad3552r_get_drive_strength(st->dev, &val); + if (!err) { + err = axi3552r_qspi_update_reg_bits(st->back, + AD3552R_REG_ADDR_INTERFACE_CONFIG_D, + AD3552R_MASK_SDO_DRIVE_STRENGTH, + val, 1); + if (err) + return err; + } + + child = device_get_named_child_node(st->dev, "channel"); + if (!child) + return -EINVAL; + + err = ad3552r_get_output_range(st->dev, st->model_data->chip_id, + child, &range); + if (!err) + return ad3552r_axi_set_output_range(st, range); + + if (err != -ENOENT) + return err; + + /* Try to get custom range */ + err = ad3552r_get_custom_gain(st->dev, child, + &gs_p, &gs_n, &rfb, &goffs); + if (err) + return err; + + ad3552r_calc_custom_gain(gs_p, gs_n, goffs, ®); + + offset = abs((s32)goffs); + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_OFFSET(0), + offset, 1); + if (err) + return dev_err_probe(st->dev, err, + "Error writing register\n"); + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_OFFSET(1), + offset, 1); + if (err) + return dev_err_probe(st->dev, err, + "Error writing register\n"); + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_GAIN(0), + reg, 1); + if (err) + return dev_err_probe(st->dev, err, + "Error writing register\n"); + + err = iio_backend_bus_reg_write(st->back, + AD3552R_REG_ADDR_CH_GAIN(1), + reg, 1); + if (err) + return dev_err_probe(st->dev, err, + "Error writing register\n"); + + return 0; +} + +static const struct iio_buffer_setup_ops ad3552r_axi_buffer_setup_ops = { + .postenable = ad3552r_axi_buffer_postenable, + .predisable = ad3552r_axi_buffer_predisable, +}; + +static int ad3552r_set_synchronous_mode_status(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + unsigned int status) +{ + struct ad3552r_axi_state *st = iio_priv(indio_dev); + + st->synced_transfer = status; + + return 0; +} + +static int ad3552r_get_synchronous_mode_status(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan) +{ + struct ad3552r_axi_state *st = iio_priv(indio_dev); + + return st->synced_transfer; +} + +static const char *const synchronous_mode_status[] = { + [AD3552R_NO_SYNC] = "no_sync", + [AD3552R_EXT_SYNC_ARM] = "ext_sync_arm", +}; + +static const struct iio_enum ad3552r_synchronous_mode_enum = { + .items = synchronous_mode_status, + .num_items = ARRAY_SIZE(synchronous_mode_status), + .get = ad3552r_get_synchronous_mode_status, + .set = ad3552r_set_synchronous_mode_status, +}; + +static const struct iio_chan_spec_ext_info ad3552r_axi_ext_info[] = { + IIO_ENUM("synchronous_mode", IIO_SHARED_BY_TYPE, + &ad3552r_synchronous_mode_enum), + IIO_ENUM_AVAILABLE("synchronous_mode", IIO_SHARED_BY_TYPE, + &ad3552r_synchronous_mode_enum), + {} +}; + +#define AD3552R_CHANNEL(ch) { \ + .type = IIO_VOLTAGE, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ), \ + .output = 1, \ + .indexed = 1, \ + .channel = (ch), \ + .scan_index = (ch), \ + .scan_type = { \ + .sign = 'u', \ + .realbits = 16, \ + .storagebits = 16, \ + .endianness = IIO_BE, \ + }, \ + .ext_info = ad3552r_axi_ext_info, \ +} + +static struct iio_chan_spec ad3552r_axi_channels[] = { + AD3552R_CHANNEL(0), + AD3552R_CHANNEL(1), +}; + +static const struct iio_info ad3552r_axi_info = { + .read_raw = &ad3552r_axi_read_raw, + .write_raw = &ad3552r_axi_write_raw, +}; + +static int ad3552r_axi_probe(struct platform_device *pdev) +{ + struct ad3552r_axi_state *st; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->dev = &pdev->dev; + + st->back = devm_iio_backend_get(&pdev->dev, NULL); + if (IS_ERR(st->back)) + return PTR_ERR(st->back); + + ret = devm_iio_backend_enable(&pdev->dev, st->back); + if (ret) + return ret; + + st->model_data = device_get_match_data(&pdev->dev); + + indio_dev->name = "ad3552r"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->setup_ops = &ad3552r_axi_buffer_setup_ops; + indio_dev->channels = ad3552r_axi_channels; + indio_dev->num_channels = ARRAY_SIZE(ad3552r_axi_channels); + indio_dev->info = &ad3552r_axi_info; + + ret = devm_iio_backend_request_buffer(&pdev->dev, st->back, indio_dev); + if (ret) + return ret; + + ret = ad3552r_axi_setup(st); + if (ret) + return ret; + + return devm_iio_device_register(&pdev->dev, indio_dev); +} + +static const struct ad3552r_axi_model_data ad3552r_model_data = { + .model_name = "ad3552r", + .chip_id = AD3552R_ID, + .num_hw_channels = 2, +}; + +static const struct of_device_id ad3552r_axi_of_id[] = { + { .compatible = "adi,ad3552r", .data = &ad3552r_model_data }, + { } +}; +MODULE_DEVICE_TABLE(of, ad3552r_axi_of_id); + +static struct platform_driver axi_ad3552r_driver = { + .driver = { + .name = "ad3552r-axi", + .of_match_table = ad3552r_axi_of_id, + }, + .probe = ad3552r_axi_probe, +}; +module_platform_driver(axi_ad3552r_driver); + +MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>"); +MODULE_AUTHOR("Angelo Dureghello <adueghello@baylibre.com>"); +MODULE_DESCRIPTION("AD3552R Driver - AXI IP version"); +MODULE_LICENSE("GPL");