RE: [PATCH] drivers: iio: adc: add support for ad777x family

[Nechita, Ramona]

>On Wed, May 29, 2024 at 03:01:06PM +0000, Nechita, Ramona wrote:
>
>...
>
>> >> +	/*
>> >> +	 * DMA (thus cache coherency maintenance) requires the
>> >> +	 * transfer buffers to live in their own cache lines.
>> >> +	 */
>> >> +	u8			reg_rx_buf[3] ____cacheline_aligned;
>> >> +	u8			reg_tx_buf[3];
>> >
>> >> +	u8			spidata_rx[32];
>> >> +	u8			spidata_tx[32];
>> >
>> >These will not be cache aligned. Is it a problem?
>> 
>> No, it should be fine without the alignment.
>
>I.o.w. it means that only reg_*x_buf are supposed to be in the different cache lines, correct?

Yes, that was how I intended.

>
>...
>
>> >Btw, can't you use regmap for IO?
>> 
>> Unfortunately, I don't think regmap could be used, because of the crc 
>> and the fact that data is shifted out on the SPI SDO line in the 
>> interrupt. I consider perhaps adding regmap to the mix might complicate things a bit.
>
>Can you add this into the comment area of the patch?

Sure thing, I will wait a little to see if there are any new comments on the v2 of the patch and I will make sure to add this as well in the next version.

>
>...
>
>> >> +	ret = ad777x_spi_write(st, AD777X_REG_SRC_N_LSB, lsb);
>> >> +	if (ret)
>> >> +		return ret;
>> >> +	ret = ad777x_spi_write(st, AD777X_REG_SRC_N_MSB, msb);
>> >> +	if (ret)
>> >> +		return ret;
>> >
>> >Can you use 16-bit writes?
>> >Same Q to all similar LSB/MSB write groups.
>> 
>> I cannot do 16-bit writes due to how the spi functions on the chip and 
>> because the registers for MSB/LSB are at different addresses.
>
>They are supposed to be on the different addresses.
>You mean the distance between them > than stride?

I may be misunderstanding this, but the spi read/write expects HeaderBit+Addr+/-Data+CRC. Writing two 
consecutive registers would mean creating a buffer with this format for each address and
sending all of them at once, correct? I feel like that would overcomplicate the code a bit, but
I can do it if it seems appropriate.

>
>...
>
>> >> +	ret = devm_add_action_or_reset(&spi->dev,
>> >> +				       ad777x_clk_disable,
>> >> +				       st->mclk);
>> >> +	if (ret)
>> >> +		return ret;
>> >
>> >So, what's wrong with the _enabled() API?
>> 
>> Sorry, I am not sure what you mean here by _enabled() API, is there a 
>> different mechanism that can be used for this type of operations?
>
>devm_clk_get_enabled()

I will look into this and update it if it is the case with the review from the v2 patch as well.

>
--
Best Regards,
Ramona Nechita