Re: [PATCH v3 14/29] media: ov2680: Add support for more clk setups

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On Tue, Jun 27, 2023 at 03:18:15PM +0200, Hans de Goede wrote:
> On ACPI systems the following 2 scenarios are possible:
> 
> 1. The xvclk is fully controlled by ACPI powermanagement, so there
>    is no "xvclk" for the driver to get (since it is abstracted away).
>    In this case there will be a "clock-frequency" device property
>    to tell the driver the xvclk rate.
> 
> 2. There is a xvclk modelled in the clk framework for the driver,
>    but the clk-generator may not be set to the right frequency
>    yet. In this case there will also be a "clock-frequency" device
>    property and the driver is expected to set the rate of the xvclk
>    through this frequency through the clk framework.
> 
> Handle both these scenarios by switching to devm_clk_get_optional()
> and checking for a "clock-frequency" device property.
> 
> This is modelled after how the same issue was fixed for the ov8865 in
> commit 73dcffeb2ff9 ("media: i2c: Support 19.2MHz input clock in ov8865").
> 
> Acked-by: Rui Miguel Silva <rmfrfs@xxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/media/i2c/ov2680.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
> index b7c23286700e..a6a83f0e53f3 100644
> --- a/drivers/media/i2c/ov2680.c
> +++ b/drivers/media/i2c/ov2680.c
> @@ -698,6 +698,7 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
>  {
>  	struct device *dev = sensor->dev;
>  	struct gpio_desc *gpio;
> +	unsigned int rate = 0;
>  	int ret;
>  
>  	/*
> @@ -718,13 +719,34 @@ static int ov2680_parse_dt(struct ov2680_dev *sensor)
>  
>  	sensor->pwdn_gpio = gpio;
>  
> -	sensor->xvclk = devm_clk_get(dev, "xvclk");
> +	sensor->xvclk = devm_clk_get_optional(dev, "xvclk");
>  	if (IS_ERR(sensor->xvclk)) {
>  		dev_err(dev, "xvclk clock missing or invalid\n");
>  		return PTR_ERR(sensor->xvclk);
>  	}
>  
> -	sensor->xvclk_freq = clk_get_rate(sensor->xvclk);
> +	/*
> +	 * We could have either a 24MHz or 19.2MHz clock rate from either DT or
> +	 * ACPI... but we also need to support the weird IPU3 case which will
> +	 * have an external clock AND a clock-frequency property. Check for the
> +	 * clock-frequency property and if found, set that rate if we managed
> +	 * to acquire a clock. This should cover the ACPI case. If the system
> +	 * uses devicetree then the configured rate should already be set, so
> +	 * we can just read it.
> +	 */

Nice comment :)

> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> +				       &rate);
> +	if (ret && !sensor->xvclk)
> +		return dev_err_probe(dev, ret, "invalid clock config\n");
> +
> +	if (!ret && sensor->xvclk) {
> +		ret = clk_set_rate(sensor->xvclk, rate);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "failed to set clock rate\n");
> +	}
> +
> +	sensor->xvclk_freq = rate ?: clk_get_rate(sensor->xvclk);
>  	if (sensor->xvclk_freq != OV2680_XVCLK_VALUE) {
>  		dev_err(dev, "wrong xvclk frequency %d HZ, expected: %d Hz\n",
>  			sensor->xvclk_freq, OV2680_XVCLK_VALUE);

Looks good to me.
Reviewed-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx>

Regards,
Tommaso


> -- 
> 2.41.0
> 



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux