Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC

[David Lechner]

On 7/11/24 4:20 AM, Nuno Sá wrote:
> On Mon, 2024-07-08 at 17:05 +0100, Jonathan Cameron wrote:
>> On Mon, 8 Jul 2024 05:17:55 +0000
>> "Tinaco, Mariel" <Mariel.Tinaco@xxxxxxxxxx> wrote:
>>
>>>> -----Original Message-----
>>>> From: Jonathan Cameron <jic23@xxxxxxxxxx>
>>>> Sent: Saturday, June 29, 2024 2:46 AM
>>>> To: Tinaco, Mariel <Mariel.Tinaco@xxxxxxxxxx>
>>>> Cc: linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>>>> kernel@xxxxxxxxxxxxxxx; Lars-Peter Clausen <lars@xxxxxxxxxx>; Rob Herring
>>>> <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
>>>> <conor+dt@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
>>>> <broonie@xxxxxxxxxx>; Hennerich, Michael <Michael.Hennerich@xxxxxxxxxx>;
>>>> Marcelo Schmitt <marcelo.schmitt1@xxxxxxxxx>; Dimitri Fedrau
>>>> <dima.fedrau@xxxxxxxxx>; Guenter Roeck <linux@xxxxxxxxxxxx>
>>>> Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
>>>>
>>>> [External]
>>>>   
>>>>>>> +};
>>>>>>> +
>>>>>>> +static int ad8460_get_powerdown_mode(struct iio_dev *indio_dev,
>>>>>>> +				     const struct iio_chan_spec *chan) {
>>>>>>> +	return 0;  
>>>>>>
>>>>>> Why have the stubs in here?  
>>>>>
>>>>> Should I move the stubs to a different place in the code or remove
>>>>> them altogether since there is only a single powerdown mode available  
>>>> Ah. I'd not really understood what was going on here.  This is fine as is.
>>>>   
>>>>>> AD8460_HVDAC_DATA_WORD_HIGH(index),  
>>>>>>> +			    ((val >> 8) & 0xFF));  
>>>>>>
>>>>>> bulk write? or do these need to be ordered?  
>>>>>
>>>>> For this I used bulk read/write this way.
>>>>>
>>>>> static int ad8460_set_hvdac_word(struct ad8460_state *state,
>>>>> 				 int index,
>>>>> 				 int val)
>>>>> {
>>>>> 	u8 regvals[AD8460_DATA_BYTE_WORD_LENGTH];  
>>>> regmap bulk accesses (when spi anyway) should be provided with DMA safe
>>>> buffers.
>>>> Easiest way to do that is add one with __aligned(IIO_DMA_MINALIGN) to the
>>>> end of the ad8460_state structure.  Possibly you'll need a lock to protect it -
>>>> I
>>>> haven't checked.  
>>>>>
>>>>> 	regvals[0] = val & 0xFF;
>>>>> 	regvals[1] = (val >> 8) & 0xFF;  
>>>>
>>>> That is an endian conversion so use appropriate endian function to fill it
>>>> efficiently and document clearly what is going on.
>>>>
>>>>
>>>> 	put_unaligned_le16()
>>>>   
>>>>>
>>>>> 	return regmap_bulk_write(state->regmap,  
>>>> AD8460_HVDAC_DATA_WORD_LOW(index),  
>>>>> 				 regvals,  
>>>> AD8460_DATA_BYTE_WORD_LENGTH); }  
>>>>>
>>>>>  
>>>>>>> +}  
>>>>   
>>>>>>> +	state->regmap = devm_regmap_init_spi(spi,
>>>>>>> &ad8460_regmap_config);
>>>>>>> +	if (IS_ERR(state->regmap))
>>>>>>> +		return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
>>>>>>> +				     "Failed to initialize regmap");
>>>>>>> +
>>>>>>> +	ret = devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,
>>>>>>> +"tx",
>>>>>>> +  
>>>>>> IIO_BUFFER_DIRECTION_OUT);
>>>>>>
>>>>>> Ah. I take back my binding comment. I assume this is mapping some
>>>>>> non standard interface for the parallel data flow?  
>>>>>
>>>>> Yes, the HDL side doesn't follow yet the standard IIO backend from
>>>>> which this driver was tested  
>>>>
>>>> Hmm. I'd like to see this brought inline with the other iio backend drivers if
>>>> possible.  
>>>
>>> Does this mean that we would need to implement an AXI IP core on the
>>> FPGA side to be able to test this?
>>
>> Don't think so.  That framework is meant to support any equivalent IP.
>> So whatever you have should be supportable. Maybe it's somewhat of a stub
>> driver though if there isn't anything controllable.
>>
>> It's Nuno's area of expertise though +CC.
>>
> 
> Hi Jonathan,
> 
> Yeah, I did reply David (IIRC) about the very same question. In the design/HW Mariel
> is working on the DAC is directly connected to the DMA core which is handled already
> by a proper dma controller driver. So in this case I'm really not seeing the backend
> need right now (maybe in the future we may have another design for this device that
> could justify for a backend device but no idea on that).
> 
> As you mention, we could very well do a stub platform driver so we can use the
> backend framework (like dma-backend or something) that could pretty much be a stub
> for the DMA controller. But is it worth it though? We'd actually be "lying" in terms
> of HW description as the DMA is a property of the actual converter.
> 
> - Nuno Sá
> 
> 

I'm a bit inclined to agree with Jonathan here. I could see someone in the future,
wanting to, e.g., use DMA + a GPIO controller for the parallel interface if they
didn't have an FPGA. So it seems a bit more future-proof to just always use the
IIO backend framework for the parallel interface.

FWIW, I don't think it would be "lying" since the io-backend DT node would be
representing physical parallel bus between the DMA controller and the ADC
chip.

But if DT maintainers are OK with the idea that a DMA channel can be directly
wired to an external chip, I guess I won't complain. :-)