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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

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

Thanks,
Miquèl