Re: [PATCH v4 04/16] media: i2c: Support 19.2MHz input clock in ov8865

[Daniel Scally <djrscally@gmail.com>]

Hi Sakari - sorry, only just coming back to this series.

On 01/11/2021 14:52, Sakari Ailus wrote:
>> +static struct ov8865_pll_configs ov8865_pll_configs_19_2mhz = {
>> +	.pll1_config = &ov8865_pll1_config_native_19_2mhz,
>> +	.pll2_config_native = &ov8865_pll2_config_native_19_2mhz,
>> +	.pll2_config_binning = &ov8865_pll2_config_binning_19_2mhz,
>> +};
>> +
>> +static struct ov8865_pll_configs ov8865_pll_configs_24mhz = {
>> +	.pll1_config = &ov8865_pll1_config_native_24mhz,
>> +	.pll2_config_native = &ov8865_pll2_config_native_24mhz,
>> +	.pll2_config_binning = &ov8865_pll2_config_binning_24mhz,
>> +};
> 
> These should be const.

Done, thank you.


>> @@ -2858,13 +2917,38 @@ static int ov8865_probe(struct i2c_client *client)
>>  		goto error_endpoint;
>>  	}
>>  
>> -	rate = clk_get_rate(sensor->extclk);
>> -	if (rate != OV8865_EXTCLK_RATE) {
>> -		dev_err(dev, "clock rate %lu Hz is unsupported\n", rate);
>> +	/*
>> +	 * We could have either a 24MHz or 19.2MHz clock rate. Check for a
>> +	 * clock-frequency property and if found, set that rate. This should
>> +	 * cover the ACPI case. If the system uses devicetree then the
>> +	 * configured rate should already be set, so we'll have to check it.
>> +	 */
>> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>> +				       &rate);
>> +	if (!ret) {
>> +		ret = clk_set_rate(sensor->extclk, rate);
>> +		if (ret) {
>> +			dev_err(dev, "failed to set clock rate\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	sensor->extclk_rate = clk_get_rate(sensor->extclk);
> 
> clk_get_rate() returns 0 if you don't have a clock. But you can still have
> clock-frequency property that tells the frequency. This is generally the
> case on ACPI based systems apart from some exceptions (which I understand
> you're well aware of).
> 
> See e.g. drivers/media/i2c/ccs/ccs-core.c .

I'm checking the clock-frequency property above...that should be
sufficient here I think right?

>> +
>> +	for (i = 0; i < ARRAY_SIZE(supported_extclk_rates); i++) {
>> +		if (sensor->extclk_rate == supported_extclk_rates[i])
>> +			break;
>> +	}
>> +
>> +	if (i == ARRAY_SIZE(supported_extclk_rates)) {
>> +		dev_err(dev, "clock rate %lu Hz is unsupported\n",
>> +			sensor->extclk_rate);
>>  		ret = -EINVAL;
>>  		goto error_endpoint;
>>  	}
>>  
>> +	sensor->pll_configs = ov8865_pll_configs[i];
>> +
>>  	/* Subdev, entity and pad */
>>  
>>  	subdev = &sensor->subdev;
>