Re: [PATCH v2 08/12] media: imx-mipi-csis: Only set clock rate when specified in DT

[Frank Li <Frank.li@nxp.com>]

On Thu, Aug 21, 2025 at 03:09:40AM +0300, Laurent Pinchart wrote:
> The imx-mipi-csis driver sets the rate of the wrap clock to the value
> specified in the device tree's "clock-frequency" property, and defaults
> to 166 MHz otherwise. This is a historical mistake, as clock rate
> selection should have been left to the assigned-clock-rates property.
>
> Honouring the clock-frequency property can't be removed without breaking
> backwards compatibility, and the corresponding code isn't very
> intrusive. The 166 MHz default, on the other hand, prevents
> configuration of the clock rate through assigned-clock-rates, as the
> driver immediately overwrites the rate. This behaviour is confusing and
> has cost debugging time.
>
> There is little value in a 166 MHz default. All mainline device tree
> sources that enable the CSIS specify a clock-frequency explicitly, and
> the default wrap clock configuration on supported platforms is at least
> as high as 166 MHz. Drop the default, and only set the clock rate
> manually when the clock-frequency property is specified.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/media/platform/nxp/imx-mipi-csis.c | 23 +++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/nxp/imx-mipi-csis.c b/drivers/media/platform/nxp/imx-mipi-csis.c
> index 67da8326540b..83ba68a20bd1 100644
> --- a/drivers/media/platform/nxp/imx-mipi-csis.c
> +++ b/drivers/media/platform/nxp/imx-mipi-csis.c
> @@ -230,8 +230,6 @@
>  #define MIPI_CSIS_PKTDATA_EVEN			0x3000
>  #define MIPI_CSIS_PKTDATA_SIZE			SZ_4K
>
> -#define DEFAULT_SCLK_CSIS_FREQ			166000000UL
> -
>  struct mipi_csis_event {
>  	bool debug;
>  	u32 mask;
> @@ -708,12 +706,17 @@ static int mipi_csis_clk_get(struct mipi_csis_device *csis)
>  	if (ret < 0)
>  		return ret;
>
> -	/* Set clock rate */
> -	ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
> -			   csis->clk_frequency);
> -	if (ret < 0)
> -		dev_err(csis->dev, "set rate=%d failed: %d\n",
> -			csis->clk_frequency, ret);
> +	if (csis->clk_frequency) {
> +		/*
> +		 * Set the clock rate. This is deprecated, for backward
> +		 * compatibility with old device trees.
> +		 */
> +		ret = clk_set_rate(csis->clks[MIPI_CSIS_CLK_WRAP].clk,
> +				   csis->clk_frequency);
> +		if (ret < 0)
> +			dev_err(csis->dev, "set rate=%d failed: %d\n",
> +				csis->clk_frequency, ret);
> +	}
>
>  	return ret;
>  }
> @@ -1417,9 +1420,7 @@ static int mipi_csis_parse_dt(struct mipi_csis_device *csis)
>  {
>  	struct device_node *node = csis->dev->of_node;
>
> -	if (of_property_read_u32(node, "clock-frequency",
> -				 &csis->clk_frequency))
> -		csis->clk_frequency = DEFAULT_SCLK_CSIS_FREQ;
> +	of_property_read_u32(node, "clock-frequency", &csis->clk_frequency);
>
>  	return 0;
>  }
> --
> Regards,
>
> Laurent Pinchart
>