Re: [PATCH v4 04/10] memory: ti-aemif: Create aemif_check_cs_timings()

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi Krzysztof,

On 11/21/24 11:28 AM, Krzysztof Kozlowski wrote:
> On 15/11/2024 14:26, Bastien Curutchet wrote:
>> aemif_calc_rate() check the validity of a new computed timing against a
>> 'max' value given as input. This isn't convenient if we want to check
>> the CS timing configuration somewhere else in the code.
>>
>> Wrap the verification of all the chip select's timing configuration into a
>> single function to ease its exportation in upcoming patches.
>> Remove the 'max' input from aemif_calc_rate() as it's no longer used.
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>> ---
>>   drivers/memory/ti-aemif.c | 107 +++++++++++++++++---------------------
>>   1 file changed, 49 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/memory/ti-aemif.c b/drivers/memory/ti-aemif.c
>> index aec6d6464efa..5c1c6f95185f 100644
>> --- a/drivers/memory/ti-aemif.c
>> +++ b/drivers/memory/ti-aemif.c
>> @@ -132,18 +132,48 @@ struct aemif_device {
>>   	struct aemif_cs_data cs_data[NUM_CS];
>>   };
>>   
>> +/**
>> + * aemif_check_cs_timings - Check the validity of a CS timing configuration.
>> + * @timings: timings configuration
>> + *
>> + * @return: 0 if the timing configuration is valid, negative error number otherwise.
>> + */
>> +static int aemif_check_cs_timings(struct aemif_cs_timings *timings)
>> +{
>> +	if (timings->ta > TA_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rhold > RHOLD_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rstrobe > RSTROBE_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->rsetup > RSETUP_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->whold > WHOLD_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->wstrobe > WSTROBE_MAX)
>> +		return -EINVAL;
>> +
>> +	if (timings->wsetup > WSETUP_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   /**
>>    * aemif_calc_rate - calculate timing data.
>>    * @pdev: platform device to calculate for
>>    * @wanted: The cycle time needed in nanoseconds.
>>    * @clk: The input clock rate in kHz.
>> - * @max: The maximum divider value that can be programmed.
>>    *
>>    * On success, returns the calculated timing value minus 1 for easy
>>    * programming into AEMIF timing registers, else negative errno.
>>    */
>> -static int aemif_calc_rate(struct platform_device *pdev, int wanted,
>> -			   unsigned long clk, int max)
>> +static int aemif_calc_rate(struct platform_device *pdev, int wanted, unsigned long clk)
>>   {
>>   	int result;
>>   
>> @@ -156,10 +186,6 @@ static int aemif_calc_rate(struct platform_device *pdev, int wanted,
>>   	if (result < 0)
>>   		result = 0;
>>   
>> -	/* ... But configuring tighter timings is not an option. */
>> -	else if (result > max)
>> -		result = -EINVAL;
>> -
>>   	return result;
>>   }
>>   
>> @@ -249,7 +275,6 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>>   	struct aemif_device *aemif = platform_get_drvdata(pdev);
>>   	unsigned long clk_rate = aemif->clk_rate;
>>   	struct aemif_cs_data *data;
>> -	int ret;
>>   	u32 cs;
>>   	u32 val;
>>   
>> @@ -275,68 +300,34 @@ static int of_aemif_parse_abus_config(struct platform_device *pdev,
>>   	aemif_get_hw_params(pdev, aemif->num_cs++);
>>   
>>   	/* override the values from device node */
>> -	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val)) {
>> -		ret = aemif_calc_rate(pdev, val, clk_rate, TA_MAX);
>> -		if (ret < 0)
>> -			return ret;
>> -
>> -		data->timings.ta = ret;
>> -	}
>> +	if (!of_property_read_u32(np, "ti,cs-min-turnaround-ns", &val))
>> +		data->timings.ta = aemif_calc_rate(pdev, val, clk_rate);
>>   
> 
> You just changed these lines in patch #1. Basically this is partial
> revert of #1.
> 

IMHO this isn't a partial revert of patch #1. Patch #1 moved the call of 
aemif_calc_rate() from aemif_config_abus() to here. Then, this patch 
removes the check of the aemif_calc_rate() as it no longer returns 
negative values. Maybe I should change the aemif_calc_rate() return type 
to u32 by the way.


Best regards,
Bastien