Re: [PATCH 01/24] spi: spi-mem: Extend spi-mem operations with a per-operation maximum frequency

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hello Tudor,

On 11/11/2024 at 13:07:09 GMT, Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>
> cut
>
>> 
>> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
>> index 17b8baf749e6..ab650ae953bb 100644
>> --- a/drivers/spi/spi-mem.c
>> +++ b/drivers/spi/spi-mem.c
>> @@ -356,6 +356,7 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  {
>>  	unsigned int tmpbufsize, xferpos = 0, totalxferlen = 0;
>>  	struct spi_controller *ctlr = mem->spi->controller;
>> +	unsigned int xfer_speed = op->max_freq;
>
> be aware that for controllers that don't support SPIMEM ops, you pass
> the frequency from the upper layers, without adjusting it with
> spi->max_speed_hz. Was this intentional?

That is exactly the opposite of what I wanted to achieve
initially. That's a very good catch.

>>  	struct spi_transfer xfers[4] = { };
>>  	struct spi_message msg;
>>  	u8 *tmpbuf;
>> @@ -368,6 +369,9 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
>>  	if (!spi_mem_internal_supports_op(mem, op))
>>  		return -EOPNOTSUPP;
>>  
>> +	if (!op->max_freq || op->max_freq > mem->spi->max_speed_hz)
>> +		((struct spi_mem_op *)op)->max_freq = mem->spi->max_speed_hz;
>
> not a big fan of casting the const out. How about introducing a
> spi_mem_adjust_op_freq()? The upper layers will use that were needed,
> and you'll still be able to pass a const op to spi_mem_exec_op()

I know it is not ideal so to follow your idea I drafted the use of
spi_mem_adjust_op_freq(). In order to avoid the cast, we actually need
to call this function everywhere in the core and the drivers to make
sure we never get out of bounds, but here is the problem:

    $ git grep -w spi_mem_exec_op -- drivers/ | wc -l
    42

This approach requires to add a call to spi_mem_adjust_op_freq() before
*every* spi_mem_exec_op(). Yes I can do that but that means to be very
attentive to the fact that these two functions are always called
together. I am not sure it is a good idea.

What about doing the following once in spi_mem_exec_op() instead?

    spi_mem_adjust_op_freq(desc->mem, (struct spi_mem_op *)op);

I know we still have a cast, but it feels more acceptable than the one I
initially proposed and covers all cases. I would not accept that in a
driver, but here we are in the core, so that sounds acceptable.

Another possibility otherwise would be to drop the const from the
spi_mem_op structure entirely. But I prefer the above function call.

>> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
>> index f866d5c8ed32..8963f236911b 100644
>> --- a/include/linux/spi/spi-mem.h
>> +++ b/include/linux/spi/spi-mem.h
>> @@ -68,6 +68,9 @@ enum spi_mem_data_dir {
>>  	SPI_MEM_DATA_OUT,
>>  };
>>  
>> +#define SPI_MEM_OP_MAX_FREQ(__freq)				\
>> +	.max_freq = __freq
>> +
>>  /**
>>   * struct spi_mem_op - describes a SPI memory operation
>>   * @cmd.nbytes: number of opcode bytes (only 1 or 2 are valid). The opcode is
>> @@ -95,6 +98,9 @@ enum spi_mem_data_dir {
>>   *		 operation does not involve transferring data
>>   * @data.buf.in: input buffer (must be DMA-able)
>>   * @data.buf.out: output buffer (must be DMA-able)
>> + * @max_freq: frequency limitation wrt this operation. 0 means there is no
>> + *	      specific constraint and the highest achievable frequency can be
>> + *	      attempted).
>
> nit: you close a parenthesis without opening one

Corrected!

Thanks for this very useful feedback!
Miquèl