Re: [PATCH RFC 3/8] iio: backend adi-axi-dac: backend features

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 29 Aug 2024 14:32:01 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Extend DAC backend with new features required for the AXI driver
> version for the a3552r DAC.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
Hi Angelo
Minor comments inline.
>  
>  static int axi_dac_enable(struct iio_backend *back)
> @@ -460,7 +493,13 @@ static int axi_dac_data_source_set(struct iio_backend *back, unsigned int chan,
>  	case IIO_BACKEND_EXTERNAL:
>  		return regmap_update_bits(st->regmap,
>  					  AXI_DAC_REG_CHAN_CNTRL_7(chan),
> -					  AXI_DAC_DATA_SEL, AXI_DAC_DATA_DMA);
> +					  AXI_DAC_DATA_SEL,
> +					  AXI_DAC_DATA_DMA);

Unrelated change.   If you want to change this, separate patch.

> +	case IIO_BACKEND_INTERNAL_RAMP_16:
> +		return regmap_update_bits(st->regmap,
> +					  AXI_DAC_REG_CHAN_CNTRL_7(chan),
> +					  AXI_DAC_DATA_SEL,
> +					  AXI_DAC_DATA_INTERNAL_RAMP_16);
>  	default:
>  		return -EINVAL;
>  	}
> @@ -518,9 +557,204 @@ static int axi_dac_reg_access(struct iio_backend *back, unsigned int reg,
>  	return regmap_write(st->regmap, reg, writeval);
>  }
>  

> +
> +static int axi_dac_bus_reg_write(struct iio_backend *back,
> +				 u32 reg, void *val, size_t size)

Maybe just pass an unsigned int for val?
So follow what regmap does? You will still need the size, but it
will just be configuration related rather than affecting the type
of val.



> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> +	if (!st->bus_type)
> +		return -EOPNOTSUPP;
> +
> +	if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {

As below, I'd use a switch and factor out this block as a separate
bus specific function.

> +		int ret;
> +		u32 ival;
> +
> +		if (size != 1 && size != 2)
> +			return -EINVAL;
> +
> +		switch (size) {
> +		case 1:
> +			ival = FIELD_PREP(AXI_DAC_DATA_WR_8, *(u8 *)val);
> +			break;
> +		case 2:
> +			ival =  FIELD_PREP(AXI_DAC_DATA_WR_16, *(u16 *)val);
> +			break;
> +		default:
> +			return  -EINVAL;

Hopefully compiler won't need this and the above. I'd drop the size != 1..
check in favour of just doing it in this switch.

> +		}
> +
> +		ret = regmap_write(st->regmap, AXI_DAC_CNTRL_DATA_WR, ival);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Both REG_CNTRL_2 and AXI_DAC_CNTRL_DATA_WR need to know
> +		 * the data size. So keeping data size control here only,
> +		 * since data size is mandatory for to the current transfer.
> +		 * DDR state handled separately by specific backend calls,
> +		 * generally all raw register writes are SDR.
> +		 */
> +		if (size == 1)
> +			ret = regmap_set_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> +					      AXI_DAC_SYMB_8B);
> +		else
> +			ret = regmap_clear_bits(st->regmap, AXI_DAC_REG_CNTRL_2,
> +						AXI_DAC_SYMB_8B);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> +					 AXI_DAC_ADDRESS,
> +					 FIELD_PREP(AXI_DAC_ADDRESS, reg));
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> +					 AXI_DAC_TRANSFER_DATA,
> +					 AXI_DAC_TRANSFER_DATA);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read_poll_timeout(st->regmap,
> +					       AXI_DAC_REG_CUSTOM_CTRL, ival,
> +					       ival & AXI_DAC_TRANSFER_DATA,
> +					       10, 100 * KILO);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_clear_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> +					  AXI_DAC_TRANSFER_DATA);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int axi_dac_bus_reg_read(struct iio_backend *back,
> +				u32 reg, void *val, size_t size)
As for write, I'd just use an unsigned int * for val like
regmap does.


> +{
> +	struct axi_dac_state *st = iio_backend_get_priv(back);
> +
> +	if (!st->bus_type)
> +		return -EOPNOTSUPP;
> +
> +	if (st->bus_type == AXI_DAC_BUS_TYPE_QSPI) {

It got mentioned in binding review but if this isn't QSPI, even
if similar don't call it that.
Maybe use a switch from the start give it will make sense
anyway the moment there is a second bus type.

I'd be tempted to factor the rest of this block out.
I guess expectation is we'll see more bus types so that factoring
out will be needed soon anyway.


> +		int ret;
> +		u32 bval;
		u32 bval = 0;
> +
> +		if (size != 1 && size != 2)
> +			return -EINVAL;
> +
> +		bval = 0;
> +		ret = axi_dac_bus_reg_write(back,
> +					    AXI_DAC_RD_ADDR(reg), &bval, size);

Ugly wrap.   Move more stuff on to first line.

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read_poll_timeout(st->regmap, AXI_DAC_UI_STATUS,
> +					       bval, bval != AXI_DAC_BUSY,
> +					       10, 100);
> +		if (ret)
> +			return ret;
> +
> +		return regmap_read(st->regmap, AXI_DAC_CNTRL_DATA_RD, val);
> +	}
> +
> +	return -EINVAL;
> +}