diff mbox series

[v2,07/11] media: i2c: Add support for new frequencies to ov7251

Message ID 20220225000753.511996-8-djrscally@gmail.com
State Superseded
Headers show
Series Support OVTI7251 on Microsoft Surface line | expand

Commit Message

Daniel Scally Feb. 25, 2022, 12:07 a.m. UTC
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(-)

Comments

Daniel Scally Feb. 25, 2022, 10:04 p.m. UTC | #1
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");
>>   	}
Andy Shevchenko Feb. 28, 2022, 11:55 a.m. UTC | #2
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?
Daniel Scally March 1, 2022, 11:27 p.m. UTC | #3
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 mbox series

Patch

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;