Hi Daniel,
On Tue, Sep 07, 2021 at 11:44:12PM +0100, Daniel Scally wrote:
> 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?
Ah, yes. I agree, that's where you'll need some code to pick the right one.
So you could use a function pointer in the struct and give it the necessary
arguments. I don't think it'd be overkill, things tend to develop over
time. See e.g. the ov8856 driver as a (warning) example.
--
Kind regards,
Sakari Ailus
