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

[Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>]

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()?