linux-kernel - Re: [PATCH v2 2/9] spi: stm32: Add OSPI driver

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 for Android: free password hash cracker in your pocket

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

Message-ID: <4ea55395-e4e2-425e-9711-3c99f30a9fa9@foss.st.com>
Date: Thu, 30 Jan 2025 09:55:46 +0100
From: Patrice CHOTARD <patrice.chotard@...s.st.com>
To: Mark Brown <broonie@...nel.org>
CC: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Alexandre Torgue
	<alexandre.torgue@...s.st.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Catalin Marinas
	<catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <christophe.kerello@...s.st.com>
Subject: Re: [PATCH v2 2/9] spi: stm32: Add OSPI driver



On 1/28/25 13:37, Mark Brown wrote:
> On Tue, Jan 28, 2025 at 09:17:24AM +0100, patrice.chotard@...s.st.com wrote:
> 
>> +static int stm32_ospi_tx_poll(struct stm32_ospi *ospi, u8 *buf, u32 len, bool read)
>> +{
> 
>> +	if (read)
>> +		tx_fifo = stm32_ospi_read_fifo;
>> +	else
>> +		tx_fifo = stm32_ospi_write_fifo;
> 
>> +		tx_fifo(buf++, regs_base + OSPI_DR);
> 
> It feels like the _tx_poll and tx_fifo naming is a landmine waiting to
> surprise people in the future.  The code sharing makes sense but the
> naming is just looking to cause surprises, especially with it just being
> a bool selecting read or write.

Agree, i will replace "tx_fifo" to a more neutral name as "fifo" for example

> 
>> +static int stm32_ospi_tx(struct stm32_ospi *ospi, const struct spi_mem_op *op)
>> +{
> 
>> +       return stm32_ospi_tx_poll(ospi, buf, op->data.nbytes,
>> +                                op->data.dir == SPI_MEM_DATA_IN);
> 
> Though the one caller is also using _tx only naming, it's a bit more
> tied in with the op sending though.

I will replace stm32_ospi_tx_poll() by stm32_ospi_poll()

> 
>> +	ctrl->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
>> +			  SPI_TX_DUAL | SPI_TX_QUAD |
>> +			  SPI_TX_OCTAL | SPI_RX_OCTAL;
>> +	ctrl->setup = stm32_ospi_setup;
>> +	ctrl->bus_num = -1;
>> +	ctrl->mem_ops = &stm32_ospi_mem_ops;
>> +	ctrl->use_gpio_descriptors = true;
>> +	ctrl->transfer_one_message = stm32_ospi_transfer_one_message;
>> +	ctrl->num_chipselect = STM32_OSPI_MAX_NORCHIP;
>> +	ctrl->dev.of_node = dev->of_node;
> 
> It looks like the controller only does half duplex as well so it should
> set SPI_CONTROLLER_HALF_DUPLEX.

Right, i will add it.

Thanks
Patrice