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@xxxxxxxxx>
>>>> ---
>>>> 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?
