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

Andy Shevchenko
2021-08-10T13:05:25Z

On Tue, Aug 10, 2021 at 1:59 AM Daniel Scally <djrscally@gmail.com> 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.

...

> +enum extclk_rate {
> + OV8865_19_2_MHZ,
> + OV8865_24_MHZ,
> + OV8865_NUM_SUPPORTED_RATES,

Same here, i.e. no comma,
If these are the only problems during review of v2, you can ignore
them, they are not critical at all.

> +};

...

> +static const struct ov8865_pll2_config ov8865_pll2_configs_native[] = {

> + /* 24MHz input clock */
> + {
> + .pll_pre_div_half = 1,
> + .pll_pre_div = 0,
> + .pll_mul = 30,
> + .dac_div = 2,
> + .sys_pre_div = 5,
> + .sys_div = 0,
> + }

+ comma.

> };

...

> + /* 24MHz input clock */
> + {
> .pll_pre_div_half = 1,
> .pll_pre_div = 0,
> .pll_mul = 30,
> .dac_div = 2,
> .sys_pre_div = 10,
> .sys_div = 0,
> + }

Ditto.

...

> + /*
> + * 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 ACPI case. If the system uses devicetree then the configured

the ACPI case

> + * rate should already be set, so we'll have to check it.
> + */

> +

Redundant blank line.

> + ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> + &rate);
> + if (!ret) {

I'm wondering if something like

... rate = 0;

fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);
if (rate > 0) {

can be used.

> + ret = clk_set_rate(sensor->extclk, rate);
> + if (ret) {
> + dev_err(dev, "failed to set clock rate\n");
> + return ret;
> + }
> + }

--
With Best Regards,
Andy Shevchenko