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

[Daniel Scally <djrscally@gmail.com>]

Hi Sakari

On 10/08/2021 22:49, Sakari Ailus wrote:
> On Tue, Aug 10, 2021 at 10:37:35PM +0100, Daniel Scally wrote:
>> Hi Sakari - thanks for all the comments
> You're welcome!
>
> Nice patches btw.


Thanks!

>
>> On 10/08/2021 14:34, Sakari Ailus wrote:
>>> Hi Daniel,
>>>
>>> Thanks for the set.
>>>
>>> On Mon, Aug 09, 2021 at 11:58:37PM +0100, Daniel Scally wrote:
>>>> The ov8865 driver as written expects a 24MHz input clock, but the sensor
>>>> is sometimes found on x86 platforms with a 19.2MHz input clock supplied.
>>>> Add a set of PLL configurations to the driver to support that rate too.
>>>> As ACPI doesn't auto-configure the clock rate, check for a clock-frequency
>>>> during probe and set that rate if one is found.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>>
>>>> 	- Added an enum defining the possible frequency rates to index the
>>>> 	  array (Andy)
>>>>
>>>>  drivers/media/i2c/ov8865.c | 164 +++++++++++++++++++++++++++----------
>>>>  1 file changed, 121 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>>>> index fe700787bfb9..1382b16d1a09 100644
>>>> --- a/drivers/media/i2c/ov8865.c
>>>> +++ b/drivers/media/i2c/ov8865.c
>>>> @@ -21,10 +21,6 @@
>>>>  #include <media/v4l2-image-sizes.h>
>>>>  #include <media/v4l2-mediabus.h>
>>>>  
>>>> -/* Clock rate */
>>>> -
>>>> -#define OV8865_EXTCLK_RATE			24000000
>>>> -
>>>>  /* Register definitions */
>>>>  
>>>>  /* System */
>>>> @@ -567,6 +563,19 @@ struct ov8865_sclk_config {
>>>>  	unsigned int sclk_div;
>>>>  };
>>>>  
>>>> +/* Clock rate */
>>>> +
>>>> +enum extclk_rate {
>>>> +	OV8865_19_2_MHZ,
>>>> +	OV8865_24_MHZ,
>>>> +	OV8865_NUM_SUPPORTED_RATES,
>>>> +};
>>>> +
>>>> +static const unsigned long supported_extclk_rates[] = {
>>>> +	[OV8865_19_2_MHZ] = 19200000,
>>>> +	[OV8865_24_MHZ] = 24000000,
>>>> +};
>>>> +
>>>>  /*
>>>>   * General formulas for (array-centered) mode calculation:
>>>>   * - photo_array_width = 3296
>>>> @@ -665,6 +674,9 @@ struct ov8865_sensor {
>>>>  	struct regulator *avdd;
>>>>  	struct regulator *dvdd;
>>>>  	struct regulator *dovdd;
>>>> +
>>>> +	unsigned long extclk_rate;
>>>> +	enum extclk_rate extclk_rate_idx;
>>>>  	struct clk *extclk;
>>>>  
>>>>  	struct v4l2_fwnode_endpoint endpoint;
>>>> @@ -680,49 +692,83 @@ struct ov8865_sensor {
>>>>  /* Static definitions */
>>>>  
>>>>  /*
>>>> - * EXTCLK = 24 MHz
>>>>   * PHY_SCLK = 720 MHz
>>>>   * MIPI_PCLK = 90 MHz
>>>>   */
>>>> -static const struct ov8865_pll1_config ov8865_pll1_config_native = {
>>>> -	.pll_pre_div_half	= 1,
>>>> -	.pll_pre_div		= 0,
>>>> -	.pll_mul		= 30,
>>>> -	.m_div			= 1,
>>>> -	.mipi_div		= 3,
>>>> -	.pclk_div		= 1,
>>>> -	.sys_pre_div		= 1,
>>>> -	.sys_div		= 2,
>>>> +
>>>> +static const struct ov8865_pll1_config ov8865_pll1_configs_native[] = {
>>>> +	{ /* 19.2 MHz input clock */
>>>> +		.pll_pre_div_half	= 1,
>>>> +		.pll_pre_div		= 2,
>>>> +		.pll_mul		= 75,
>>>> +		.m_div			= 1,
>>>> +		.mipi_div		= 3,
>>>> +		.pclk_div		= 1,
>>>> +		.sys_pre_div		= 1,
>>>> +		.sys_div		= 2,
>>>> +	},
>>>> +	{ /* 24MHz input clock */
>>>> +		.pll_pre_div_half	= 1,
>>>> +		.pll_pre_div		= 0,
>>>> +		.pll_mul		= 30,
>>>> +		.m_div			= 1,
>>>> +		.mipi_div		= 3,
>>>> +		.pclk_div		= 1,
>>>> +		.sys_pre_div		= 1,
>>>> +		.sys_div		= 2,
>>>> +	},
>>> Could you instead add a struct specific to the clock frequency with
>>> pointers to these? See e.g. the ov8856 driver how this could otherwise end
>>> up...I thin
>>
>> You mean something like
>>
>>
>> static struct ov8865_pll_configs_19_2_mhz {
>>
>>     .pll1_config_native = &ov8865_pll1_config_native,
>>
>>     ...
>>
>> };
>>
>>
>>
>> static struct ov8865_pll_configs_24_mhz {
>>
>>     .pll1_config_native = &ov8865_pll1_config_native,
>>
>>     ...
>>
>> };
>>
>>
>> or am I misunderstanding?
> Yes, please --- ov8865_pll1_config_native above is thus the PLL
> configuration for the 24 MHz clock.

I'm not sure about this actually. There's two versions of
ov8865_pll2_config, native and binning, so it becomes something like this:


struct ov8865_pll_configs {
    struct ov8865_pll1_config *pll1_config;
    struct ov8865_pll2_config *pll2_config_native;
    struct ov8865_pll2_config *pll2_config_binning;
};

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,
};


Now because a mode might use either the native or binning version of the
pll2 configs, currently they're actually against the struct for a
particular mode like so:


struct ov8865_mode ov8865_modes[] = {

    {

        <snip>

        .pll1_config            = &ov8865_pll1_config_native,
        .pll2_config            = &ov8865_pll2_config_binning,
        .sclk_config            = &ov8865_sclk_config_native,

    }

};


The problem I'm having is that I can't really see a clean way to store
against the _mode_ whether it should access .pll2_config_native or
.pll2_config_binning, from the new struct ov8865_pll_configs. Do you
have any ideas about a way to do that?