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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 29 May 2024 15:45:58 +0000
From: "Nechita, Ramona" <Ramona.Nechita@...log.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
CC: "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        Jonathan Cameron
	<jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Hennerich,
 Michael" <Michael.Hennerich@...log.com>,
        Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
        "Sa, Nuno" <Nuno.Sa@...log.com>,
        Marius
 Cristea <marius.cristea@...rochip.com>,
        "Schmitt, Marcelo"
	<Marcelo.Schmitt@...log.com>,
        Maksim Kiselev <bigunclemax@...il.com>,
        Ivan
 Mikhaylov <fr0st61te@...il.com>,
        Marcus Folkesson
	<marcus.folkesson@...il.com>,
        Liam Beguin <liambeguin@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] drivers: iio: adc: add support for ad777x family

>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