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

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

On 12/18/24 9:37 AM, Miquel Raynal wrote:
> On 18/12/2024 at 08:07:24 GMT, Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
> 
>> On 12/13/24 10:46 AM, Miquel Raynal wrote:
>>> Hello Tudor,
>>>
>>
>> Hi!
>>
>>> 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
>>
>> cut
>>
>>>>> +	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.
>>
>> How about introducing a spi_nand_spimem_exec_op() where you call
>> spi_mem_adjust_op_freq() and spi_mem_exec_op()?
> 
> That would work to make the cast disappear but TBH would not be totally
> relevant as adjusting the frequency is typically something that would
> benefit to spi-nor as well (maybe in the future) and therefore would

Right, SPI NOR will benefit of this too.

> fully apply to spi memories as a whole, not just spi-nand. We can think
> about another naming maybe, but I find like spi_mem_exec_op() is the
> right location to do this.
> 

It's not the first time that we adjust spi_mem_op parameters, see:
https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git/tree/drivers/mtd/spi-nor/core.c?h=spi-nor/next#n153

Does SPI NAND need to call spi_mem_adjust_op_size as well? I see it
calls it when using dirmap, but not with a plain spi_mem_exec_op().