On 25/02/2022 17:09, Andy Shevchenko wrote:
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()?
The problem is since we could have a the clock coming from some dt file
with it named xclk, as this driver is obviously designed for, but it
also can be created by the int3472-tps68470 or int3472-discrete drivers
which don't use that name. Without knowing what it's called, best thing
I could think to do was remove the name and rely on device matching.
if (IS_ERR(ov7251->xclk)) {
dev_err(dev, "could not get xclk");
return PTR_ERR(ov7251->xclk);
This should be dev_err_probe().
Yep
}
...
+ /*
+ * 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?
Broken ACPI compensated for by the cio2-bridge - it creates the
clock-frequency property which ordinarily wouldn't be there on ACPI
systems AIUI.
+ */
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.
Ah yes, ok thanks.
+ return dev_err_probe(dev, ret, "invalid clock config\n");
}
