Re: [PATCH v4 2/6] media: mt9m114: Bypass PLL if required

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:36AM +0100, Mathis Foerst wrote:
> The MT9M114 sensor has an internal PLL that generates the required SYSCLK
> from EXTCLK. It also has the option to bypass the PLL and use EXTCLK
> directly as SYSCLK.
> The current driver implementation uses a hardcoded PLL configuration that
> requires a specific EXTCLK frequency. Depending on the available clocks,
> it can be desirable to use a different PLL configuration or to bypass it.
> 
> The link-frequency of the output bus (Parallel or MIPI-CSI) is configured
> in the device tree.
> 
> Check if EXTCLK can be used as SYSCLK to achieve this link-frequency. If
> yes, bypass the PLL.
> Otherwise, (as before) check if EXTCLK and the default PLL configuration
> provide the required SYSCLK to achieve the link-frequency. If yes, use the
> PLL. If no, throw an error.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@mt.com>
> ---
>  drivers/media/i2c/mt9m114.c | 62 ++++++++++++++++++++++++++-----------
>  1 file changed, 44 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 5f0b0ad8f885..b06003b69f6f 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -261,6 +261,7 @@
>  #define MT9M114_CAM_PGA_PGA_CONTROL			CCI_REG16(0xc95e)
>  #define MT9M114_CAM_SYSCTL_PLL_ENABLE			CCI_REG8(0xc97e)
>  #define MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE			BIT(0)
> +#define MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE			0x00
>  #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N		CCI_REG16(0xc980)
>  #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(m, n)		(((n) << 8) | (m))
>  #define MT9M114_CAM_SYSCTL_PLL_DIVIDER_P		CCI_REG16(0xc982)
> @@ -377,6 +378,7 @@ struct mt9m114 {
>  	struct gpio_desc *reset;
>  	struct regulator_bulk_data supplies[3];
>  	struct v4l2_fwnode_endpoint bus_cfg;
> +	bool bypass_pll;
>  
>  	struct {
>  		unsigned int m;
> @@ -743,14 +745,20 @@ static int mt9m114_initialize(struct mt9m114 *sensor)
>  	}
>  
>  	/* Configure the PLL. */
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> -		  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> -		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> -						       sensor->pll.n),
> -		  &ret);
> -	cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
> -		  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p), &ret);
> +	if (sensor->bypass_pll) {
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> +			  MT9M114_CAM_SYSCTL_PLL_DISABLE_VALUE, &ret);
> +	} else {
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_ENABLE,
> +			  MT9M114_CAM_SYSCTL_PLL_ENABLE_VALUE, &ret);
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_M_N,
> +			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_VALUE(sensor->pll.m,
> +							       sensor->pll.n),
> +			  &ret);
> +		cci_write(sensor->regmap, MT9M114_CAM_SYSCTL_PLL_DIVIDER_P,
> +			  MT9M114_CAM_SYSCTL_PLL_DIVIDER_P_VALUE(sensor->pll.p),
> +			  &ret);
> +	}

You can add a blank line here.

>  	cci_write(sensor->regmap, MT9M114_CAM_SENSOR_CFG_PIXCLK,
>  		  sensor->pixrate, &ret);
>  
> @@ -2235,9 +2243,19 @@ static const struct dev_pm_ops mt9m114_pm_ops = {
>   * Probe & Remove
>   */
>  
> +static int mt9m114_verify_link_frequency(struct mt9m114 *sensor)
> +{
> +	unsigned int link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> +				? sensor->pixrate * 8 : sensor->pixrate * 2;

			       ? sensor->pixrate * 8 : sensor->pixrate * 2;

And missing blank line.

> +	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> +	    sensor->bus_cfg.link_frequencies[0] != link_freq)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int mt9m114_clk_init(struct mt9m114 *sensor)
>  {
> -	unsigned int link_freq;
>  
>  	/* Hardcode the PLL multiplier and dividers to default settings. */
>  	sensor->pll.m = 32;
> @@ -2249,19 +2267,27 @@ static int mt9m114_clk_init(struct mt9m114 *sensor)
>  	 * for 16-bit per pixel, transmitted in DDR over a single lane. For
>  	 * parallel mode, the sensor ouputs one pixel in two PIXCLK cycles.
>  	 */
> -	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> -			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
>  
> -	link_freq = sensor->bus_cfg.bus_type == V4L2_MBUS_CSI2_DPHY
> -		  ? sensor->pixrate * 8 : sensor->pixrate * 2;
> +	/*
> +	 * Check if EXTCLK fits the configured link frequency. Bypass the PLL
> +	 * in this case.
> +	 */
> +	sensor->pixrate = clk_get_rate(sensor->clk) / 2;
> +	if (mt9m114_verify_link_frequency(sensor) == 0) {

I would be cleaner to pass the pixel rate as a parameter to the
function:

	unsigned int pixrate;

	...

	pixrate = clk_get_rate(sensor->clk) / 2;
	if (mt9m114_verify_link_frequency(sensor, pixrate) == 0) {
		sensor->pixrate = pixrate;
		sensor->bypass_pll = true;
		return 0;
	}

> +		sensor->bypass_pll = true;
> +		return 0;
> +	}
>  
> -	if (sensor->bus_cfg.nr_of_link_frequencies != 1 ||
> -	    sensor->bus_cfg.link_frequencies[0] != link_freq) {
> -		dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> -		return -EINVAL;
> +	/* Check if the PLL configuration fits the configured link frequency */

s/frequency/frequency./

With those small issues addressed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	sensor->pixrate = clk_get_rate(sensor->clk) * sensor->pll.m
> +			/ ((sensor->pll.n + 1) * (sensor->pll.p + 1));
> +	if (mt9m114_verify_link_frequency(sensor) == 0) {
> +		sensor->bypass_pll = false;
> +		return 0;
>  	}
>  
> -	return 0;
> +	dev_err(&sensor->client->dev, "Unsupported DT link-frequencies\n");
> +	return -EINVAL;
>  }
>  
>  static int mt9m114_identify(struct mt9m114 *sensor)

-- 
Regards,

Laurent Pinchart