Re: [PATCH RFC 7/8] iio: dac: ad3552r: add axi platform driver

[Angelo Dureghello]

Hi Jonathan,

On 31/08/24 2:13 PM, Jonathan Cameron wrote:
> On Thu, 29 Aug 2024 14:32:05 +0200
> Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
>
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> >
> > Add support for ad3552r AXI DAC IP version.
> >
> > Signed-off-by: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> Hi Angelo
>
> To me this feels like the interface is much closer to SPI + SPI offload
> than to a conventional IIO backend on the basis it carries the configuration
> path as well.
>
> Can we see if it can be fitted into that model?  You will need to define
> a new bus type etc for it but should be fairly simple given constrained
> setup (at least today!)
>
> That will resolve a bunch of questions around the binding as well.

thanks for all the feedbacks.

I see, spi offload may have more sense but as of now looks like moving to
AXI SPI engine instead of AXI DAC would require quite a lot of work from the
ADI HDL guys and also then, for me some work reworking all this stuff.
From an initial discussion with Nuno and David, we was oriented to use the
iio backend for the current HDL, so at least for this chip at this stage 
would
be good, if possible, to stay this way.


> Jonathan
>
> > diff --git a/drivers/iio/dac/ad3552r-axi.c b/drivers/iio/dac/ad3552r-axi.c
> > new file mode 100644
> > index 000000000000..98e5da08c973
> > --- /dev/null
> > +++ b/drivers/iio/dac/ad3552r-axi.c
> > @@ -0,0 +1,572 @@
> > +// 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/buffer.h>
> > +#include <linux/iio/backend.h>
> > +#include <linux/of.h>
> Why?  Probably want mod_devicetable.h


with mod_devicetable.h in place of of.h i get

drivers/iio/dac/ad3552r-axi.c:272:9: error: cleanup argument not a function
  struct fwnode_handle *child __free(fwnode_handle) = NULL;
         ^~~~~~~~~~~~~


> > +#include <linux/platform_device.h>
> > +#include <linux/units.h>
>
> > +static int ad3552r_axi_update_scan_mode(struct iio_dev *indio_dev,
> > +					const unsigned long *active_scan_mask)
> > +{
> > +	struct ad3552r_axi_state *st = iio_priv(indio_dev);
> > +
> > +	st->active_scan_mask = *active_scan_mask;
> We probably want another accessor for this, but for now that variable
> can still be read from iio_dev->active_scan_mask so no need
> for the copy here (and hence no need for this callback).
thanks a lot, removed the accessor, not needed
> > +
> > +	return 0;
> > +}
> > +
> > +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 (st->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;
> > +
> > +	iio_backend_data_transfer_addr(st->back, val);
> err = ?
thanks, fixed.
> > +	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 is anabled 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;
> > +
> > +	err =  iio_backend_buffer_enable(st->back);
> > +	if (!err)
> > +		return 0;
> Keep the good path inline as that's more idiomatic and what a reviewers
> eyes expect to see.
>
> 	if (err)
> 		goto exit_err_sync;
>
> 	return 0;
ok, fixed
> > +
> > +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 DDR mode */
> You set the DAC to ddr mode whilst disabling it? That seems backwards.
Thanks, wrong comment, i am setting back to SDR here, comment fixed.
> > +	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_reset(struct ad3552r_axi_state *st)
> > +{
> > +	int err;
> > +
> > +	/* AXI reset performed by backend enable() */
> This comment is confusing given it's in a function called
> axi_reset and you don't do the backend enable() here.
> So how is this resetting the AXI bus (or is that name
> referring to the fpga IP?)

Ok, i removed the comment.

Actually, the IP reset is performed by the backend "enable"
while this function resets the target chip.

> > +
> > +	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, reg = 0, offset = 0;
> Generally don't mix assignment and non assignment stuff on online.
> Fine to have them all not assigned or all assigned, but a mix
> tends to lead to people missing one in the middle that is
> different.
>
> 	u16 id, rfb,
> 	u16 reg = 0, offset = 0;
Ok, done.
> > +	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;
> > +
> > +	val = AD3552R_SCRATCH_PAD_TEST_VAL1;
> > +	err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD,
> > +					&val, 1);
> as per earlier review, I'd pass an unsigned int instead of a void *
> Then you can avoid the dance with a local variable.
void * was chosen thinking to future busses, please let me know if
it can stay this way.
> > +	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;
> > +	}
> > +
> > +	val = AD3552R_SCRATCH_PAD_TEST_VAL2;
> > +	err = iio_backend_bus_reg_write(st->back,
> > +					AD3552R_REG_ADDR_SCRATCH_PAD,
> > +					&val, 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;
> > +	mdelay(100);
> Document this delay as it's odd to need a gap whilst reading ID registers.
I ported that delay from a previous testing driver, but i verified
it is not needed, so i removed it.
> > +
> > +	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 != AD3552R_ID) {
> > +		dev_err(st->dev, "Chip ID mismatch. Expected 0x%x, Read 0x%x\n",
> > +			AD3552R_ID, id);
> Print an message only on this. We want to enable fallback dt compatibles for
> future devices on old kernels, so we can't require a match on a WHOAMI type register.
> We can put a message in the log though to give us a hint if that fallback
> compatible is wrong.
ok, done.
> > +		return -ENODEV;
> > +	}
> > +
> > +	st->chip_id = id;
> This is usually a bad sign.  It is much more extensible for a driver to at
> this point 'pick' between a set of per device type structures that encode
> all the difference between device variants.  So good to do that from
> the start.  Lots of old drivers do it this way, but we've learnt over the years
> that it becomes steadily more messy over time as a driver supports more and
> more devices.
>
> I guess the existing driver is doing it this way though so maybe that's
> a refactor for another day.

Thanks, ok, i reworked the same way as ad3552r.c (spi) since i'll have 
to add
other variants soon.


> > +
> > +	val = AD3552R_REF_INIT;
> > +	err = iio_backend_bus_reg_write(st->back,
> > +					AD3552R_REG_ADDR_SH_REFERENCE_CONFIG,
> > +					&val, 1);
> > +	if (err)
> > +		return err;
> > +
> > +	val = AD3552R_TRANSFER_INIT;
> > +	err = iio_backend_bus_reg_write(st->back,
> > +					AD3552R_REG_ADDR_TRANSFER_REGISTER,
> > +					&val, 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->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, &reg);
> > +
> > +	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 const char *const synchronous_mode_status[] = {
> > +	[AD3552R_NO_SYNC] = "no_sync",
> > +	[AD3552R_EXT_SYNC_ARM] = "ext_sync_arm",
> I'll comment on this in the ABI docs patch.
>
> > +};
> > +
> > +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),
> > +	{},
> 	{ }
>
> I'm not blanket fixing this case yet (unlikely the ID ones) but
> generally it's nice to not have a comma after a 'null' terminator
> entry as adding anything after it would be a bug.
ok, done
> > +};
> > +
> > +#define AD3552R_CHANNEL(ch) { \
> > +	.type = IIO_VOLTAGE, \
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> > +	.info_mask_shared_by_all = (((ch) == 0) ? \
> > +		BIT(IIO_CHAN_INFO_SAMP_FREQ) : 0), \
> If it's shared by all it should be set for all.
> The core code will only create one attr as a result.
>
> Technically it's not a 'bug' to not do this but the semantics
> are wrong if you set something that is for all channels on only
> one of them, so if there are other drivers doing this that I've
> missed we should clean that up.

Ok, i set it for all so.


> > +	.output = 1, \
> > +	.indexed = 1, \
> > +	.channel = (ch), \
> > +	.scan_index = (ch), \
> > +	.scan_type = { \
> > +		.sign = 'u', \
> > +		.realbits = 16, \
> > +		.storagebits = 16, \
> > +		.shift = 0, \
> Zero shift is the 'obvious' default, so need to specify it in this
> case.
ok, fixed
> > +		.endianness = IIO_BE, \
> > +	}, \
> > +	.ext_info = ad3552r_axi_ext_info, \
> > +}
> > +
> > +static const struct of_device_id ad3552r_axi_of_id[] = {
> > +	{ .compatible = "adi,ad3552r" },
> > +	{}
> Trivial, but I'm trying to standardize formats of these in IIO on
> 	{ }
ok, done
> > +};


Thanks a lot,

regards,
Angelo

--
 ,,,      Angelo Dureghello
:: :.     BayLibre -runtime team- Developer
:`___:
 `____: