On 1/28/25 13:37, Mark Brown wrote:
> On Tue, Jan 28, 2025 at 09:17:24AM +0100, patrice.chotard@xxxxxxxxxxx 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
