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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hello,

On 18/12/2024 at 10:13:39 GMT, Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:

> On 12/18/24 10:03 AM, Tudor Ambarus wrote:
>> 
>> 
>> 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().
>> 
>
> I ask because I'm thinking of adding in the SPIMEM core a prepare()
> method, and maybe rename exec_op() to exec(). And then introduce a
> prepare_exec() method that the upper layers would call? Similar to
> clk_prepare_enable.

Do you have something else in mind you would like to put in the prepare
step? I am not at all opposed to it, but I feel like for now the
spi_mem_exec_op() is a fine path for that, especially since there are
very little things to "prepare" (for now).

Do you mind if I keep the cast (not the one from the series, I cleaned
that up to have an adjust_op function as discussed) for now, and if you
ever go the prepare/exec path we would get this converted to use the new
API? I'd like to make progresses on other topics in the spi-nand core
and avoid being blocked by a bigger task that we need to discuss first.

Cheers,
Miquèl