>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
