Re: [PATCH 03/24] spi: amd: Support per spi-mem operation frequency switches

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Tudor,

On 11/11/2024 at 13:36:15 GMT, Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:

> On 10/25/24 5:14 PM, Miquel Raynal wrote:
>> Every ->exec_op() call correctly configures the spi bus speed to the
>> maximum allowed frequency for the memory using the constant spi default
>> parameter. Since we can now have per-operation constraints, let's use
>> the value that comes from the spi-mem operation structure instead. In
>> case there is no specific limitation for this operation, the default spi
>> device value will be given anyway.
>> 
>> This controller however performed a frequency check, which is also
>> observed during the ->check_op() phase.
>> 
>> The per-operation frequency capability is thus advertised to the spi-mem
>> core.
>> 
>> Cc: Sanjay R Mehta <sanju.mehta@xxxxxxx>
>> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
>> ---
>>  drivers/spi/spi-amd.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
>> index 2245ad54b03a..f58dc6375582 100644
>> --- a/drivers/spi/spi-amd.c
>> +++ b/drivers/spi/spi-amd.c
>> @@ -368,6 +368,9 @@ static bool amd_spi_supports_op(struct spi_mem *mem,
>>  	    op->data.buswidth > 1 || op->data.nbytes > AMD_SPI_MAX_DATA)
>>  		return false;
>>  
>> +	if (op->max_freq < AMD_SPI_MIN_HZ)
>
> How about using mem->spi->controller->min_speed_hz intead?

Good idea. I think I used AMD_SPI_MIN_HZ to follow what was done
somewhere else, but that's fine.

>
>> +		return false;
>
> I find the check fine here, but I see however that amd_set_spi_freq()
> duplicates the same, returning -EINVAL when speed_hz < AMD_SPI_MIN_HZ

This one is useless, the spi core already takes care of this check, I'll
drop it in a separate patch.

>> +
>>  	return spi_mem_default_supports_op(mem, op);
>>  }
>>  
>> @@ -443,7 +446,7 @@ static int amd_spi_exec_mem_op(struct spi_mem *mem,
>>  
>>  	amd_spi = spi_controller_get_devdata(mem->spi->controller);
>>  
>> -	ret = amd_set_spi_freq(amd_spi, mem->spi->max_speed_hz);
>> +	ret = amd_set_spi_freq(amd_spi, op->max_freq);
>>  	if (ret)
>>  		return ret;
>
> however the return code is checked just on this call, and completely
> ignored in the 2 other calls. I find the code a bit ugly for the non
> SPIMEM case, but maybe something for the amd owner to address.

Once the above check removed (it does not make much sense there), the
function can return void.

Cheers,
Miquèl