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

[Krzysztof Kozlowski <krzk@kernel.org>]

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.

Best regards,
Krzysztof