[PATCH 0/6] media: ov6650: Master and pixel clock handling fixes

[PATCH 0/6] media: ov6650: Master and pixel clock handling fixes

[Janusz Krzysztofik]

Fix issues with handling of master and pixel clock, mostly those
introduced as temporary workarounds when the driver was converted form
soc_camera sensor to a standalone V4L2 subdevice driver.

Janusz Krzysztofik (6):
  media: ov6650: Fix stored frame interval not in sync with hardware
  media: ov6650: Drop obsolete .pclk_limit attribute
  media: ov6650: Simplify clock divisor calculation
  media: ov6650: Don't reapply pixel clock divisor on format change
  media: ov6650: Drop unused .pclk_max field
  media: ov6650: Fix arbitrary selection of master clock rate

 drivers/media/i2c/ov6650.c | 129 +++++++++++++++++++++----------------
 1 file changed, 72 insertions(+), 57 deletions(-)

-- 
2.21.0

[PATCH 1/6] media: ov6650: Fix stored frame interval not in sync with hardware

[Janusz Krzysztofik]

The driver stores a frame interval value supposed to be in line with
hardware state in a device private structure.  Since the driver initial
submission, the respective field of the structure has never been
initialised on device probe.  Moreover, if updated from
.s_frame_interval(), a new value is stored before it is applied on
hardware.  If an error occurs during device update, the stored value
may no longer reflect hardware state and consecutive calls to
.g_frame_interval() may return incorrect information.

Assuming a failed update of the device means its actual state hasn't
changed, update the frame interval field of the device private
structure with a new value only after it is successfully applied on
hardware so it always reflects actual hardware state to the extent
possible.  Also, initialise the field with hardware default frame
interval on device probe.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 16887049f0cd..f60aeb1f7813 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -130,6 +130,7 @@
 #define CLKRC_24MHz		0xc0
 #define CLKRC_DIV_MASK		0x3f
 #define GET_CLKRC_DIV(x)	(((x) & CLKRC_DIV_MASK) + 1)
+#define DEF_CLKRC		0x00
 
 #define COMA_RESET		BIT(7)
 #define COMA_QCIF		BIT(5)
@@ -783,19 +784,17 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
 	else if (div > GET_CLKRC_DIV(CLKRC_DIV_MASK))
 		div = GET_CLKRC_DIV(CLKRC_DIV_MASK);
 
-	/*
-	 * Keep result to be used as tpf limit
-	 * for subsequent clock divider calculations
-	 */
-	priv->tpf.numerator = div;
-	priv->tpf.denominator = FRAME_RATE_MAX;
+	tpf->numerator = div;
+	tpf->denominator = FRAME_RATE_MAX;
 
-	clkrc = to_clkrc(&priv->tpf, priv->pclk_limit, priv->pclk_max);
+	clkrc = to_clkrc(tpf, priv->pclk_limit, priv->pclk_max);
 
 	ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
 	if (!ret) {
-		tpf->numerator = GET_CLKRC_DIV(clkrc);
-		tpf->denominator = FRAME_RATE_MAX;
+		priv->tpf.numerator = GET_CLKRC_DIV(clkrc);
+		priv->tpf.denominator = FRAME_RATE_MAX;
+
+		*tpf = priv->tpf;
 	}
 
 	return ret;
@@ -1038,6 +1037,10 @@ static int ov6650_probe(struct i2c_client *client,
 	priv->rect.width  = W_CIF;
 	priv->rect.height = H_CIF;
 
+	/* Hardware default frame interval */
+	priv->tpf.numerator   = GET_CLKRC_DIV(DEF_CLKRC);
+	priv->tpf.denominator = FRAME_RATE_MAX;
+
 	priv->subdev.internal_ops = &ov6650_internal_ops;
 
 	ret = v4l2_async_register_subdev(&priv->subdev);
-- 
2.21.0

[PATCH 2/6] media: ov6650: Drop obsolete .pclk_limit attribute

[Janusz Krzysztofik]

That attribute used to be obtained from platform data by a soc_camera
host interface and passed to the sensor driver for .s_mbus_fmt() video
operation callback, later reused as .set_fmt() pad operation callback,
to be able to limit frame rate.  The driver stored that value in its
private structure for further use from .g/s_parm(), later converted to
g/s_frame_interval().

On conversion of the driver from soc_camera sensor to a standalone V4L2
subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
v4l2 subdevice"), that attribute had been replaced by a constant and
hardcoded to an arbitrarily chosen pixel clock limit.  Drop it.  Host
interfaces can limit frame rate if needed by calling
.s_frame_interval().

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f60aeb1f7813..a50244401491 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -197,7 +197,6 @@ struct ov6650 {
 	struct v4l2_clk		*clk;
 	bool			half_scale;	/* scale down output by 2 */
 	struct v4l2_rect	rect;		/* sensor cropping window */
-	unsigned long		pclk_limit;	/* from host */
 	unsigned long		pclk_max;	/* from resolution and format */
 	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
 	u32 code;
@@ -546,8 +545,7 @@ static bool is_unscaled_ok(int width, int height, struct v4l2_rect *rect)
 	return width > rect->width >> 1 || height > rect->height >> 1;
 }
 
-static u8 to_clkrc(struct v4l2_fract *timeperframe,
-		unsigned long pclk_limit, unsigned long pclk_max)
+static u8 to_clkrc(struct v4l2_fract *timeperframe, unsigned long pclk_max)
 {
 	unsigned long pclk;
 
@@ -557,9 +555,6 @@ static u8 to_clkrc(struct v4l2_fract *timeperframe,
 	else
 		pclk = pclk_max;
 
-	if (pclk_limit && pclk_limit < pclk)
-		pclk = pclk_limit;
-
 	return (pclk_max - 1) / pclk;
 }
 
@@ -653,10 +648,9 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 
 	clkrc = CLKRC_12MHz;
 	mclk = 12000000;
-	priv->pclk_limit = 1334000;
 	dev_dbg(&client->dev, "using 12MHz input clock\n");
 
-	clkrc |= to_clkrc(&priv->tpf, priv->pclk_limit, priv->pclk_max);
+	clkrc |= to_clkrc(&priv->tpf, priv->pclk_max);
 
 	pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
 	dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
@@ -756,7 +750,7 @@ static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
 	struct ov6650 *priv = to_ov6650(client);
 
 	ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
-			priv->pclk_limit, priv->pclk_max));
+							  priv->pclk_max));
 	ival->interval.denominator = FRAME_RATE_MAX;
 
 	dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
@@ -787,7 +781,7 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
 	tpf->numerator = div;
 	tpf->denominator = FRAME_RATE_MAX;
 
-	clkrc = to_clkrc(tpf, priv->pclk_limit, priv->pclk_max);
+	clkrc = to_clkrc(tpf, priv->pclk_max);
 
 	ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
 	if (!ret) {
-- 
2.21.0

[PATCH 3/6] media: ov6650: Simplify clock divisor calculation

[Janusz Krzysztofik]

As appears from an analysis of to_clkrc() helper code after its
pclk_limit argument has been dropped, its result no longer depends on
another argument - pclk_max.  Moreover, assuming that a constant value
of FRAME_RATE_MAX is always used as a denominator of the only
significant argument left - a struct v4l2_fract, the result in fact
depends only on the numerator value of that argument.  As a further
consequence, it no longer makes sense to recalculate frame intervals by
converting them forth and back with a GET_CLKRC_DIV(to_clkrc(tpf))
construct.

Drop use of GET_CLKRC_DIV() on results of to_clkrc() where possible -
use the frame interval value directly.  Furthermore, replace the
to_clkrc() helper function with a simple macro and update its users to
always use FRAME_RATE_MAX as frame interval denominator and pass only
its numerator as an argument.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index a50244401491..61ddd4ea4c26 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -545,18 +545,7 @@ static bool is_unscaled_ok(int width, int height, struct v4l2_rect *rect)
 	return width > rect->width >> 1 || height > rect->height >> 1;
 }
 
-static u8 to_clkrc(struct v4l2_fract *timeperframe, unsigned long pclk_max)
-{
-	unsigned long pclk;
-
-	if (timeperframe->numerator && timeperframe->denominator)
-		pclk = pclk_max * timeperframe->denominator /
-				(FRAME_RATE_MAX * timeperframe->numerator);
-	else
-		pclk = pclk_max;
-
-	return (pclk_max - 1) / pclk;
-}
+#define to_clkrc(div)	((div) - 1)
 
 /* set the format we will capture in */
 static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
@@ -650,7 +639,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 	mclk = 12000000;
 	dev_dbg(&client->dev, "using 12MHz input clock\n");
 
-	clkrc |= to_clkrc(&priv->tpf, priv->pclk_max);
+	clkrc |= to_clkrc(priv->tpf.numerator);
 
 	pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
 	dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
@@ -749,9 +738,7 @@ static int ov6650_g_frame_interval(struct v4l2_subdev *sd,
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
 
-	ival->interval.numerator = GET_CLKRC_DIV(to_clkrc(&priv->tpf,
-							  priv->pclk_max));
-	ival->interval.denominator = FRAME_RATE_MAX;
+	ival->interval = priv->tpf;
 
 	dev_dbg(&client->dev, "Frame interval: %u/%u s\n",
 		ival->interval.numerator, ival->interval.denominator);
@@ -766,7 +753,6 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
 	struct ov6650 *priv = to_ov6650(client);
 	struct v4l2_fract *tpf = &ival->interval;
 	int div, ret;
-	u8 clkrc;
 
 	if (tpf->numerator == 0 || tpf->denominator == 0)
 		div = 1;  /* Reset to full rate */
@@ -778,14 +764,9 @@ static int ov6650_s_frame_interval(struct v4l2_subdev *sd,
 	else if (div > GET_CLKRC_DIV(CLKRC_DIV_MASK))
 		div = GET_CLKRC_DIV(CLKRC_DIV_MASK);
 
-	tpf->numerator = div;
-	tpf->denominator = FRAME_RATE_MAX;
-
-	clkrc = to_clkrc(tpf, priv->pclk_max);
-
-	ret = ov6650_reg_rmw(client, REG_CLKRC, clkrc, CLKRC_DIV_MASK);
+	ret = ov6650_reg_rmw(client, REG_CLKRC, to_clkrc(div), CLKRC_DIV_MASK);
 	if (!ret) {
-		priv->tpf.numerator = GET_CLKRC_DIV(clkrc);
+		priv->tpf.numerator = div;
 		priv->tpf.denominator = FRAME_RATE_MAX;
 
 		*tpf = priv->tpf;
-- 
2.21.0

[PATCH 4/6] media: ov6650: Don't reapply pixel clock divisor on format change

[Janusz Krzysztofik]

As calculation of pixel clock hardware divisor no longer depends on
mbus format specific maximum pixel clock, there is no need to reapply
the divisor on format change.  Drop related code from ov6650_s_fmt()
helper.

Since a master clock hardware divisor, so far applied only together
with the pixel clock divisor in a single operation, will no longer be
applied from ov6650_s_fmt(), apply it, still using a hardcoded value
for now, from  ov6650_prog_dflt() helper so hardware is still
initialised correctly on device probe.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index 61ddd4ea4c26..e95ea132ef06 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -564,8 +564,7 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 		.r.height = mf->height << half_scale,
 	};
 	u32 code = mf->code;
-	unsigned long mclk, pclk;
-	u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask, clkrc;
+	u8 coma_set = 0, coma_mask = 0, coml_set, coml_mask;
 	int ret;
 
 	/* select color matrix configuration for given color encoding */
@@ -635,21 +634,9 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 		coma_mask |= COMA_QCIF;
 	}
 
-	clkrc = CLKRC_12MHz;
-	mclk = 12000000;
-	dev_dbg(&client->dev, "using 12MHz input clock\n");
-
-	clkrc |= to_clkrc(priv->tpf.numerator);
-
-	pclk = priv->pclk_max / GET_CLKRC_DIV(clkrc);
-	dev_dbg(&client->dev, "pixel clock divider: %ld.%ld\n",
-			mclk / pclk, 10 * mclk % pclk / pclk);
-
 	ret = ov6650_set_selection(sd, NULL, &sel);
 	if (!ret)
 		ret = ov6650_reg_rmw(client, REG_COMA, coma_set, coma_mask);
-	if (!ret)
-		ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
 	if (!ret) {
 		priv->half_scale = half_scale;
 
@@ -798,6 +785,8 @@ static int ov6650_prog_dflt(struct i2c_client *client)
 	dev_dbg(&client->dev, "initializing\n");
 
 	ret = ov6650_reg_write(client, REG_COMA, 0);	/* ~COMA_RESET */
+	if (!ret)
+		ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
 	if (!ret)
 		ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
 
-- 
2.21.0

[PATCH 5/6] media: ov6650: Drop unused .pclk_max field

[Janusz Krzysztofik]

This field of the driver private structure is no longer used, drop it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index e95ea132ef06..f4723c0b5c70 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -197,7 +197,6 @@ struct ov6650 {
 	struct v4l2_clk		*clk;
 	bool			half_scale;	/* scale down output by 2 */
 	struct v4l2_rect	rect;		/* sensor cropping window */
-	unsigned long		pclk_max;	/* from resolution and format */
 	struct v4l2_fract	tpf;		/* as requested with s_frame_interval */
 	u32 code;
 };
@@ -618,17 +617,14 @@ static int ov6650_s_fmt(struct v4l2_subdev *sd, struct v4l2_mbus_framefmt *mf)
 			code == MEDIA_BUS_FMT_SBGGR8_1X8) {
 		coml_mask = COML_ONE_CHANNEL;
 		coml_set = 0;
-		priv->pclk_max = 4000000;
 	} else {
 		coml_mask = 0;
 		coml_set = COML_ONE_CHANNEL;
-		priv->pclk_max = 8000000;
 	}
 
 	if (half_scale) {
 		dev_dbg(&client->dev, "max resolution: QCIF\n");
 		coma_set |= COMA_QCIF;
-		priv->pclk_max /= 2;
 	} else {
 		dev_dbg(&client->dev, "max resolution: CIF\n");
 		coma_mask |= COMA_QCIF;
-- 
2.21.0

[PATCH 6/6] media: ov6650: Fix arbitrary selection of master clock rate

[Janusz Krzysztofik]

A hardcoded 12 MHz master clock frequency has been assumed since
conversion of the driver from soc_camera sensor to a standalone V4L2
subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
v4l2 subdevice").  Fix it.

Define a static table of supported master clock rates (fix misnamed
symbol while being at it), then use v4l2_clk_get/set_rate() to obtain
a clock rate matching one of those supported.  On success, apply
respective master clock hardware divisor provided by the matching
element of the table.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/media/i2c/ov6650.c | 64 ++++++++++++++++++++++++++++++++++----
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
index f4723c0b5c70..13fd7c4699b2 100644
--- a/drivers/media/i2c/ov6650.c
+++ b/drivers/media/i2c/ov6650.c
@@ -124,7 +124,7 @@
 
 #define DEF_AECH		0x4D
 
-#define CLKRC_6MHz		0x00
+#define CLKRC_8MHz		0x00
 #define CLKRC_12MHz		0x40
 #define CLKRC_16MHz		0x80
 #define CLKRC_24MHz		0xc0
@@ -201,6 +201,29 @@ struct ov6650 {
 	u32 code;
 };
 
+struct ov6650_xclk {
+	unsigned long	rate;
+	u8		clkrc;
+};
+
+static const struct ov6650_xclk ov6650_xclk[] = {
+{
+	.rate	= 8000000,
+	.clkrc	= CLKRC_8MHz,
+},
+{
+	.rate	= 12000000,
+	.clkrc	= CLKRC_12MHz,
+},
+{
+	.rate	= 16000000,
+	.clkrc	= CLKRC_16MHz,
+},
+{
+	.rate	= 24000000,
+	.clkrc	= CLKRC_24MHz,
+},
+};
 
 static u32 ov6650_codes[] = {
 	MEDIA_BUS_FMT_YUYV8_2X8,
@@ -774,7 +797,7 @@ static int ov6650_reset(struct i2c_client *client)
 }
 
 /* program default register values */
-static int ov6650_prog_dflt(struct i2c_client *client)
+static int ov6650_prog_dflt(struct i2c_client *client, u8 clkrc)
 {
 	int ret;
 
@@ -782,7 +805,7 @@ static int ov6650_prog_dflt(struct i2c_client *client)
 
 	ret = ov6650_reg_write(client, REG_COMA, 0);	/* ~COMA_RESET */
 	if (!ret)
-		ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
+		ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
 	if (!ret)
 		ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
 
@@ -793,8 +816,10 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct ov6650 *priv = to_ov6650(client);
-	u8		pidh, pidl, midh, midl;
-	int		ret;
+	const struct ov6650_xclk *xclk;
+	unsigned long rate;
+	u8 pidh, pidl, midh, midl;
+	int i, ret;
 
 	priv->clk = v4l2_clk_get(&client->dev, NULL);
 	if (IS_ERR(priv->clk)) {
@@ -803,6 +828,33 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 		return ret;
 	}
 
+	rate = v4l2_clk_get_rate(priv->clk);
+	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
+		if (rate != ov6650_xclk[i].rate)
+			continue;
+
+		xclk = &ov6650_xclk[i];
+		dev_info(&client->dev, "using host default clock rate %lukHz\n",
+			 rate / 1000);
+		break;
+	}
+	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
+		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
+		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
+			continue;
+
+		xclk = &ov6650_xclk[i];
+		dev_info(&client->dev, "using negotiated clock rate %lukHz\n",
+			 xclk->rate / 1000);
+		break;
+	}
+	if (!xclk) {
+		dev_err(&client->dev, "unable to get supported clock rate\n");
+		if (!ret)
+			ret = -EINVAL;
+		goto eclkput;
+	}
+
 	ret = ov6650_s_power(sd, 1);
 	if (ret < 0)
 		goto eclkput;
@@ -836,7 +888,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
 
 	ret = ov6650_reset(client);
 	if (!ret)
-		ret = ov6650_prog_dflt(client);
+		ret = ov6650_prog_dflt(client, xclk->clkrc);
 	if (!ret) {
 		struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
 
-- 
2.21.0

Re: [PATCH 6/6] media: ov6650: Fix arbitrary selection of master clock rate

[Sakari Ailus]

Hi Janusz,

On Sun, Oct 13, 2019 at 02:50:50PM +0200, Janusz Krzysztofik wrote:
> A hardcoded 12 MHz master clock frequency has been assumed since
> conversion of the driver from soc_camera sensor to a standalone V4L2
> subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
> v4l2 subdevice").  Fix it.
> 
> Define a static table of supported master clock rates (fix misnamed
> symbol while being at it), then use v4l2_clk_get/set_rate() to obtain
> a clock rate matching one of those supported.  On success, apply
> respective master clock hardware divisor provided by the matching
> element of the table.
> 
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
>  drivers/media/i2c/ov6650.c | 64 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> index f4723c0b5c70..13fd7c4699b2 100644
> --- a/drivers/media/i2c/ov6650.c
> +++ b/drivers/media/i2c/ov6650.c
> @@ -124,7 +124,7 @@
>  
>  #define DEF_AECH		0x4D
>  
> -#define CLKRC_6MHz		0x00
> +#define CLKRC_8MHz		0x00
>  #define CLKRC_12MHz		0x40
>  #define CLKRC_16MHz		0x80
>  #define CLKRC_24MHz		0xc0
> @@ -201,6 +201,29 @@ struct ov6650 {
>  	u32 code;
>  };
>  
> +struct ov6650_xclk {
> +	unsigned long	rate;
> +	u8		clkrc;
> +};
> +
> +static const struct ov6650_xclk ov6650_xclk[] = {
> +{
> +	.rate	= 8000000,
> +	.clkrc	= CLKRC_8MHz,
> +},
> +{
> +	.rate	= 12000000,
> +	.clkrc	= CLKRC_12MHz,
> +},
> +{
> +	.rate	= 16000000,
> +	.clkrc	= CLKRC_16MHz,
> +},
> +{
> +	.rate	= 24000000,
> +	.clkrc	= CLKRC_24MHz,
> +},
> +};
>  
>  static u32 ov6650_codes[] = {
>  	MEDIA_BUS_FMT_YUYV8_2X8,
> @@ -774,7 +797,7 @@ static int ov6650_reset(struct i2c_client *client)
>  }
>  
>  /* program default register values */
> -static int ov6650_prog_dflt(struct i2c_client *client)
> +static int ov6650_prog_dflt(struct i2c_client *client, u8 clkrc)
>  {
>  	int ret;
>  
> @@ -782,7 +805,7 @@ static int ov6650_prog_dflt(struct i2c_client *client)
>  
>  	ret = ov6650_reg_write(client, REG_COMA, 0);	/* ~COMA_RESET */
>  	if (!ret)
> -		ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
> +		ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
>  	if (!ret)
>  		ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
>  
> @@ -793,8 +816,10 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	struct ov6650 *priv = to_ov6650(client);
> -	u8		pidh, pidl, midh, midl;
> -	int		ret;
> +	const struct ov6650_xclk *xclk;
> +	unsigned long rate;
> +	u8 pidh, pidl, midh, midl;
> +	int i, ret;
>  
>  	priv->clk = v4l2_clk_get(&client->dev, NULL);
>  	if (IS_ERR(priv->clk)) {
> @@ -803,6 +828,33 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  		return ret;
>  	}
>  
> +	rate = v4l2_clk_get_rate(priv->clk);
> +	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
> +		if (rate != ov6650_xclk[i].rate)
> +			continue;
> +
> +		xclk = &ov6650_xclk[i];
> +		dev_info(&client->dev, "using host default clock rate %lukHz\n",
> +			 rate / 1000);
> +		break;
> +	}

xclk is undefined unless it was set in the previous loop. Please initialise
to to NULL. I can fix that, too, while applying the patch.

> +	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
> +		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> +		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
> +			continue;
> +
> +		xclk = &ov6650_xclk[i];
> +		dev_info(&client->dev, "using negotiated clock rate %lukHz\n",
> +			 xclk->rate / 1000);
> +		break;
> +	}
> +	if (!xclk) {
> +		dev_err(&client->dev, "unable to get supported clock rate\n");
> +		if (!ret)
> +			ret = -EINVAL;
> +		goto eclkput;
> +	}
> +
>  	ret = ov6650_s_power(sd, 1);
>  	if (ret < 0)
>  		goto eclkput;
> @@ -836,7 +888,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
>  
>  	ret = ov6650_reset(client);
>  	if (!ret)
> -		ret = ov6650_prog_dflt(client);
> +		ret = ov6650_prog_dflt(client, xclk->clkrc);
>  	if (!ret) {
>  		struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
>  

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 6/6] media: ov6650: Fix arbitrary selection of master clock rate

[Janusz Krzysztofik]

Hi Sakari,

On Friday, October 18, 2019 8:54:19 P.M. CEST Sakari Ailus wrote:
> Hi Janusz,
> 
> On Sun, Oct 13, 2019 at 02:50:50PM +0200, Janusz Krzysztofik wrote:
> > A hardcoded 12 MHz master clock frequency has been assumed since
> > conversion of the driver from soc_camera sensor to a standalone V4L2
> > subdevice by commit 23a52386fabe ("media: ov6650: convert to standalone
> > v4l2 subdevice").  Fix it.
> > 
> > Define a static table of supported master clock rates (fix misnamed
> > symbol while being at it), then use v4l2_clk_get/set_rate() to obtain
> > a clock rate matching one of those supported.  On success, apply
> > respective master clock hardware divisor provided by the matching
> > element of the table.
> > 
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > ---
> >  drivers/media/i2c/ov6650.c | 64 ++++++++++++++++++++++++++++++++++----
> >  1 file changed, 58 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov6650.c b/drivers/media/i2c/ov6650.c
> > index f4723c0b5c70..13fd7c4699b2 100644
> > --- a/drivers/media/i2c/ov6650.c
> > +++ b/drivers/media/i2c/ov6650.c
> > @@ -124,7 +124,7 @@
> >  
> >  #define DEF_AECH		0x4D
> >  
> > -#define CLKRC_6MHz		0x00
> > +#define CLKRC_8MHz		0x00
> >  #define CLKRC_12MHz		0x40
> >  #define CLKRC_16MHz		0x80
> >  #define CLKRC_24MHz		0xc0
> > @@ -201,6 +201,29 @@ struct ov6650 {
> >  	u32 code;
> >  };
> >  
> > +struct ov6650_xclk {
> > +	unsigned long	rate;
> > +	u8		clkrc;
> > +};
> > +
> > +static const struct ov6650_xclk ov6650_xclk[] = {
> > +{
> > +	.rate	= 8000000,
> > +	.clkrc	= CLKRC_8MHz,
> > +},
> > +{
> > +	.rate	= 12000000,
> > +	.clkrc	= CLKRC_12MHz,
> > +},
> > +{
> > +	.rate	= 16000000,
> > +	.clkrc	= CLKRC_16MHz,
> > +},
> > +{
> > +	.rate	= 24000000,
> > +	.clkrc	= CLKRC_24MHz,
> > +},
> > +};
> >  
> >  static u32 ov6650_codes[] = {
> >  	MEDIA_BUS_FMT_YUYV8_2X8,
> > @@ -774,7 +797,7 @@ static int ov6650_reset(struct i2c_client *client)
> >  }
> >  
> >  /* program default register values */
> > -static int ov6650_prog_dflt(struct i2c_client *client)
> > +static int ov6650_prog_dflt(struct i2c_client *client, u8 clkrc)
> >  {
> >  	int ret;
> >  
> > @@ -782,7 +805,7 @@ static int ov6650_prog_dflt(struct i2c_client *client)
> >  
> >  	ret = ov6650_reg_write(client, REG_COMA, 0);	/* ~COMA_RESET */
> >  	if (!ret)
> > -		ret = ov6650_reg_write(client, REG_CLKRC, CLKRC_12MHz);
> > +		ret = ov6650_reg_write(client, REG_CLKRC, clkrc);
> >  	if (!ret)
> >  		ret = ov6650_reg_rmw(client, REG_COMB, 0, COMB_BAND_FILTER);
> >  
> > @@ -793,8 +816,10 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
> >  {
> >  	struct i2c_client *client = v4l2_get_subdevdata(sd);
> >  	struct ov6650 *priv = to_ov6650(client);
> > -	u8		pidh, pidl, midh, midl;
> > -	int		ret;
> > +	const struct ov6650_xclk *xclk;
> > +	unsigned long rate;
> > +	u8 pidh, pidl, midh, midl;
> > +	int i, ret;
> >  
> >  	priv->clk = v4l2_clk_get(&client->dev, NULL);
> >  	if (IS_ERR(priv->clk)) {
> > @@ -803,6 +828,33 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
> >  		return ret;
> >  	}
> >  
> > +	rate = v4l2_clk_get_rate(priv->clk);
> > +	for (i = 0; rate && i < ARRAY_SIZE(ov6650_xclk); i++) {
> > +		if (rate != ov6650_xclk[i].rate)
> > +			continue;
> > +
> > +		xclk = &ov6650_xclk[i];
> > +		dev_info(&client->dev, "using host default clock rate %lukHz\n",
> > +			 rate / 1000);
> > +		break;
> > +	}
> 
> xclk is undefined unless it was set in the previous loop. 

Indeed, sorry for that.

> Please initialise
> to to NULL. I can fix that, too, while applying the patch.

OK, please do if you can.

Thanks,
Janusz


> > +	for (i = 0; !xclk && i < ARRAY_SIZE(ov6650_xclk); i++) {
> > +		ret = v4l2_clk_set_rate(priv->clk, ov6650_xclk[i].rate);
> > +		if (ret || v4l2_clk_get_rate(priv->clk) != ov6650_xclk[i].rate)
> > +			continue;
> > +
> > +		xclk = &ov6650_xclk[i];
> > +		dev_info(&client->dev, "using negotiated clock rate %lukHz\n",
> > +			 xclk->rate / 1000);
> > +		break;
> > +	}
> > +	if (!xclk) {
> > +		dev_err(&client->dev, "unable to get supported clock rate\n");
> > +		if (!ret)
> > +			ret = -EINVAL;
> > +		goto eclkput;
> > +	}
> > +
> >  	ret = ov6650_s_power(sd, 1);
> >  	if (ret < 0)
> >  		goto eclkput;
> > @@ -836,7 +888,7 @@ static int ov6650_video_probe(struct v4l2_subdev *sd)
> >  
> >  	ret = ov6650_reset(client);
> >  	if (!ret)
> > -		ret = ov6650_prog_dflt(client);
> > +		ret = ov6650_prog_dflt(client, xclk->clkrc);
> >  	if (!ret) {
> >  		struct v4l2_mbus_framefmt mf = ov6650_def_fmt;
> >  
> 
>