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

[Angelo Dureghello <adureghello@baylibre.com>]

On 31/08/24 1:34 PM, Jonathan Cameron wrote:
> 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.
Thanks, fixed.
>
>> +	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.
>
void * was used since data size in the future may vary depending
on the bus physical interface.

Actually, a reg bus write involves several AXI regmap operations.
>
>> +{
>> +	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.
Ok, changed.
>
>> +		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.
>
sure, done.


>> +		}
>> +
>> +		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.

Ok, so initial choice was unsigned int, further thinking of
possible future busses drive the choice to void *.

Let me know, i can switch to unsigned int in case.


>
>
>> +{
>> +	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.

It's a bit difficult to find a different name, physically,
it is a QSPI, 4 lanes + clock + cs, and datasheet is naming it Quad SPI.
But looking the data protocol, it's a bit different.

QSPI has instruction, address and data.
Here we have just ADDR and DATA.

What about ADI_QSPI ?


> Maybe use a switch from the start give it will make sense
> anyway the moment there is a second bus type.

ok, used a switch in the write too.

> 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.
ok done.
>
>> +		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;
>> +}

Thanks,

regards,
Angelo

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