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 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.