| Message ID | 20220225000753.511996-8-djrscally@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support OVTI7251 on Microsoft Surface line | expand |
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"); >> }
On Fri, Feb 25, 2022 at 10:04:29PM +0000, Daniel Scally wrote: > On 25/02/2022 17:09, Andy Shevchenko wrote: > > On Fri, Feb 25, 2022 at 12:07:49AM +0000, Daniel Scally wrote: ... > > > /* 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. So, then the Q is why it's not called the same in the other drivers? ... > Broken ACPI compensated for by the cio2-bridge - it creates the > clock-frequency property which ordinarily wouldn't be there on ACPI systems > AIUI. In the current practice we have CLK priority over property, this means we may do: 1) unconditional reading of the property; 2) trying CLK. Can it be done here?
On 01/03/2022 14:33, Andy Shevchenko wrote: > On Mon, Feb 28, 2022 at 11:11:27PM +0000, Daniel Scally wrote: >> On 28/02/2022 11:55, Andy Shevchenko wrote: >>> On Fri, Feb 25, 2022 at 10:04:29PM +0000, Daniel Scally wrote: >>>> On 25/02/2022 17:09, Andy Shevchenko wrote: > ... > >> Basically it seems better to me to just let it match by device rather than >> have the names. The only advantage I can see for the names is if a device >> has multiple clocks assigned to it...but there are no instances of that in >> media/i2c. > I have heard you, but leave for the judgement done by maintainers. > Fair enough :) > >>>> Broken ACPI compensated for by the cio2-bridge - it creates the >>>> clock-frequency property which ordinarily wouldn't be there on ACPI systems >>>> AIUI. >>> In the current practice we have CLK priority over property, this means we may do: >>> 1) unconditional reading of the property; >>> 2) trying CLK. >>> >>> Can it be done here? >> Er, can you point me to an example? > Something like > > device_read_property_u32("clock-frequency", &rate); > > clk = devm_clk_get_optional(...); > if (IS_ERR(clk)) > return PTR_ERR(clk); > > clk_rate = clk_get_rate(...); > if (clk_rate == 0) > clk_rate = rate; > if (clk_rate == 0) > return dev_err_probe(...); > Gotcha - ok sure, leave that with me.
diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c index 4e88bbf4d828..d4843e797568 100644 --- a/drivers/media/i2c/ov7251.c +++ b/drivers/media/i2c/ov7251.c @@ -96,12 +96,14 @@ struct ov7251_pll_cfgs { }; enum xclk_rate { + OV7251_19_2_MHZ, OV7251_24_MHZ, OV7251_NUM_SUPPORTED_RATES }; enum supported_link_freqs { OV7251_LINK_FREQ_240_MHZ, + OV7251_LINK_FREQ_319_2_MHZ, OV7251_NUM_SUPPORTED_LINK_FREQS }; @@ -147,6 +149,22 @@ static inline struct ov7251 *to_ov7251(struct v4l2_subdev *sd) return container_of(sd, struct ov7251, sd); } +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 +}; + +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 +}; + static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_240_mhz = { .pre_div = 0x03, .mult = 0x64, @@ -155,6 +173,22 @@ static const struct ov7251_pll1_cfg ov7251_pll1_cfg_24_mhz_240_mhz = { .mipi_div = 0x05 }; +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 +}; + +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 +}; + static const struct ov7251_pll2_cfg ov7251_pll2_cfg_24_mhz = { .pre_div = 0x04, .mult = 0x28, @@ -163,14 +197,24 @@ static const struct ov7251_pll2_cfg ov7251_pll2_cfg_24_mhz = { .adc_div = 0x04 }; +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, + } +}; + static const struct ov7251_pll_cfgs ov7251_pll_cfgs_24_mhz = { .pll2 = &ov7251_pll2_cfg_24_mhz, .pll1 = { [OV7251_LINK_FREQ_240_MHZ] = &ov7251_pll1_cfg_24_mhz_240_mhz, + [OV7251_LINK_FREQ_319_2_MHZ] = &ov7251_pll1_cfg_24_mhz_319_2_mhz, } }; static const struct ov7251_pll_cfgs *ov7251_pll_cfgs[] = { + [OV7251_19_2_MHZ] = &ov7251_pll_cfgs_19_2_mhz, [OV7251_24_MHZ] = &ov7251_pll_cfgs_24_mhz }; @@ -564,15 +608,18 @@ static const struct reg_value ov7251_setting_vga_90fps[] = { }; static const unsigned long supported_xclk_rates[] = { + [OV7251_19_2_MHZ] = 19200000, [OV7251_24_MHZ] = 24000000, }; static const s64 link_freq[] = { [OV7251_LINK_FREQ_240_MHZ] = 240000000, + [OV7251_LINK_FREQ_319_2_MHZ] = 319200000, }; static const s64 pixel_rates[] = { [OV7251_LINK_FREQ_240_MHZ] = 48000000, + [OV7251_LINK_FREQ_319_2_MHZ] = 63840000, }; static const struct ov7251_mode_info ov7251_mode_info_data[] = { @@ -1400,6 +1447,7 @@ static int ov7251_probe(struct i2c_client *client) struct device *dev = &client->dev; struct ov7251 *ov7251; u8 chip_id_high, chip_id_low, chip_rev; + unsigned int rate = 0; s64 pixel_rate; int ret; int i; @@ -1416,31 +1464,30 @@ static int ov7251_probe(struct i2c_client *client) return ret; /* get system clock (xclk) */ - ov7251->xclk = devm_clk_get(dev, "xclk"); + ov7251->xclk = devm_clk_get(dev, NULL); if (IS_ERR(ov7251->xclk)) { dev_err(dev, "could not get xclk"); return PTR_ERR(ov7251->xclk); } + /* + * We could have either a 24MHz or 19.2MHz clock rate from either dt or + * ACPI. We also need to support the IPU3 case which will have both an + * external clock AND a clock-frequency property. + */ 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) { + return dev_err_probe(dev, ret, "invalid clock config\n"); } - /* external clock must be 24MHz, allow 1% tolerance */ - if (ov7251->xclk_freq < 23760000 || ov7251->xclk_freq > 24240000) { - dev_err(dev, "external clock frequency %u is not supported\n", - ov7251->xclk_freq); - return -EINVAL; - } + ov7251->xclk_freq = rate ? rate : clk_get_rate(ov7251->xclk); - ret = clk_set_rate(ov7251->xclk, ov7251->xclk_freq); - if (ret) { - dev_err(dev, "could not set xclk frequency\n"); - return ret; - } for (i = 0; i < ARRAY_SIZE(supported_xclk_rates); i++) if (ov7251->xclk_freq == supported_xclk_rates[i]) break;
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. Signed-off-by: Daniel Scally <djrscally@gmail.com> --- Changes in v2: - Added support for 319.2MHz link frequency drivers/media/i2c/ov7251.c | 79 ++++++++++++++++++++++++++++++-------- 1 file changed, 63 insertions(+), 16 deletions(-)