| Message ID | 20220225000753.511996-8-djrscally@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support OVTI7251 on Microsoft Surface line | expand |
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"); > }
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?
Hi Andy 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: >>> 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? If I'm honest this is one of those areas I'm still sketchy on, in drivers/media/i2c I see calls to clk_get with clock names like so: eclk extclk mclk refclk xclk xti xvclk But those only work if the firmware (I.E. dt) for the device defines that name for the clock which means they need to define the dt in a particular way. So if I make the int3472-discrete driver (which is sort of playing the part of dt here) register the clocks called "xclk" that means the drivers all need to have that name too, which breaks the dt entries if they weren't originally called that. On the other hand, if the driver has no name but relies on device matching it will work for both the original dt and also for the int3472-discrete driver's clock 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. > >> 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? >
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. ... > > > 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(...);
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.
On Mon, Feb 28, 2022 at 01:55:31PM +0200, 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: > > > 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? FWIW, most sensor drivers use NULL for the name as there's just a single clock. This one is rather an exception. Existing DT should continue to just work as well as not specifying the name gives you the first one. The name could be removed frem the bindings IMO.
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(-)