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

From: Nechita, Ramona
Date: Wed May 29 2024 - 11:46:31 EST


>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