On Fri, Feb 25, 2022 at 12:07:49AM +0000, Daniel Scally wrote:
> The OV7251 sensor is used as the IR camera sensor on the Microsoft
> Surface line of tablets; this provides a 19.2MHz external clock, and
> the Windows driver for this sensor configures a 319.2MHz link freq to
> the CSI-2 receiver. Add the ability to support those rate to the
> driver by defining a new set of PLL configs.
> +static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_240_mhz = {
> + .pre_div = 0x03,
> + .mult = 0x4b,
> + .div = 0x01,
> + .pix_div = 0x0a,
> + .mipi_div = 0x05
+ Comma.
> +};
> +
> +static const struct ov7251_pll1_cfg ov7251_pll1_cfg_19_2_mhz_319_2_mhz = {
> + .pre_div = 0x01,
> + .mult = 0x85,
> + .div = 0x04,
> + .pix_div = 0x0a,
> + .mipi_div = 0x05
Ditto.
> +};
...
> +static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_319_2_mhz = {
> + .pre_div = 0x05,
> + .mult = 0x85,
> + .div = 0x02,
> + .pix_div = 0x0a,
> + .mipi_div = 0x05
Ditto.
> +};
> +
> +static const struct ov7251_pll2_cfg ov7251_pll2_cfg_19_2_mhz = {
> + .pre_div = 0x04,
> + .mult = 0x32,
> + .div = 0x00,
> + .sys_div = 0x05,
> + .adc_div = 0x04
Ditto.
> +};
...
> +static const struct ov7251_pll_cfgs ov7251_pll_cfgs_19_2_mhz = {
> + .pll2 = &ov7251_pll2_cfg_19_2_mhz,
> + .pll1 = {
> + [OV7251_LINK_FREQ_240_MHZ] = &ov7251_pll1_cfg_19_2_mhz_240_mhz,
> + [OV7251_LINK_FREQ_319_2_MHZ] = &ov7251_pll1_cfg_19_2_mhz_319_2_mhz,
> + }
Ditto.
> +};
...
> /* get system clock (xclk) */
> - ov7251->xclk = devm_clk_get(dev, "xclk");
> + ov7251->xclk = devm_clk_get(dev, NULL);
Why a clock doesn't have a name anymore?
Shouldn't you rather switch to _optional()?
> if (IS_ERR(ov7251->xclk)) {
> dev_err(dev, "could not get xclk");
> return PTR_ERR(ov7251->xclk);
This should be dev_err_probe().
> }
...
> + /*
> + * We could have either a 24MHz or 19.2MHz clock rate from either dt or
DT
> + * ACPI. We also need to support the IPU3 case which will have both an
> + * external clock AND a clock-frequency property.
Why is that? Broken table?
> + */
> ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> - &ov7251->xclk_freq);
> - if (ret) {
> - dev_err(dev, "could not get xclk frequency\n");
> - return ret;
> + &rate);
> + if (!ret && ov7251->xclk) {
> + ret = clk_set_rate(ov7251->xclk, rate);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to set clock rate\n");
> + } else if (ret && !ov7251->xclk) {
Redundant 'else' if you test for error condition first.
> + return dev_err_probe(dev, ret, "invalid clock config\n");
> }
--
With Best Regards,
Andy Shevchenko
