Re: [PATCH v2 8/9] iio: dac: ad3552r: add axi platform driver

[Jonathan Cameron]

On Thu, 05 Sep 2024 17:17:38 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:

> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxx>
> Co-developed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> Co-developed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
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, &reg);

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@xxxxxxxxxx>");
> +MODULE_AUTHOR("Angelo Dureghello <adueghello@xxxxxxxxxxxx>");
> +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");
>