[PATCH 00/17] smiapp and smiapp-pll: more robust parameter handling, cleanups

[PATCH 00/17] smiapp and smiapp-pll: more robust parameter handling, cleanups

[Sakari Ailus]

Hi,

This set brings several improvements to the smiapp driver and the smiapp PLL
calculator. Especially:

- Several cleanups and small fixes,
- only calculate the VT clock tree for sensors that have no OP tree (the OP
  constraits in pixel clock divider were not taken into account for VT
  clocks),
- fix a (harmless) lockdep warning in sensor initialisation,
- the PLL update is done if there has been a change in output BPP and
- maintain information on valid combinations of link frequencies and
  formats. What could happen was that if one chose a link frequency that was
  not possible for a given media bus format (or the other way around), the
  user was simply given an error with no hints on what could be wrong. The
  format is considered more important and the link rate is adjusted to suit
  the format if needed.

-- 
Kind regards,
Sakari

[PATCH 01/17] smiapp-pll: Correct clock debug prints

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The PLL flags were not used correctly.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 2335529..ab5d9a3 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -67,7 +67,7 @@ static void print_pll(struct device *dev, struct smiapp_pll *pll)
 {
 	dev_dbg(dev, "pre_pll_clk_div\t%d\n",  pll->pre_pll_clk_div);
 	dev_dbg(dev, "pll_multiplier \t%d\n",  pll->pll_multiplier);
-	if (pll->flags != SMIAPP_PLL_FLAG_NO_OP_CLOCKS) {
+	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
 		dev_dbg(dev, "op_sys_clk_div \t%d\n", pll->op_sys_clk_div);
 		dev_dbg(dev, "op_pix_clk_div \t%d\n", pll->op_pix_clk_div);
 	}
@@ -77,7 +77,7 @@ static void print_pll(struct device *dev, struct smiapp_pll *pll)
 	dev_dbg(dev, "ext_clk_freq_hz \t%d\n", pll->ext_clk_freq_hz);
 	dev_dbg(dev, "pll_ip_clk_freq_hz \t%d\n", pll->pll_ip_clk_freq_hz);
 	dev_dbg(dev, "pll_op_clk_freq_hz \t%d\n", pll->pll_op_clk_freq_hz);
-	if (pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS) {
+	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
 		dev_dbg(dev, "op_sys_clk_freq_hz \t%d\n",
 			pll->op_sys_clk_freq_hz);
 		dev_dbg(dev, "op_pix_clk_freq_hz \t%d\n",
-- 
1.7.10.4

[PATCH 02/17] smiapp-pll: The clock tree values are unsigned --- fix debug prints

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

These values are unsigned, so use %u instead of %d.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |   94 ++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 47 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index ab5d9a3..d14af5c 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -65,26 +65,26 @@ static int bounds_check(struct device *dev, uint32_t val,
 
 static void print_pll(struct device *dev, struct smiapp_pll *pll)
 {
-	dev_dbg(dev, "pre_pll_clk_div\t%d\n",  pll->pre_pll_clk_div);
-	dev_dbg(dev, "pll_multiplier \t%d\n",  pll->pll_multiplier);
+	dev_dbg(dev, "pre_pll_clk_div\t%u\n",  pll->pre_pll_clk_div);
+	dev_dbg(dev, "pll_multiplier \t%u\n",  pll->pll_multiplier);
 	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
-		dev_dbg(dev, "op_sys_clk_div \t%d\n", pll->op_sys_clk_div);
-		dev_dbg(dev, "op_pix_clk_div \t%d\n", pll->op_pix_clk_div);
+		dev_dbg(dev, "op_sys_clk_div \t%u\n", pll->op_sys_clk_div);
+		dev_dbg(dev, "op_pix_clk_div \t%u\n", pll->op_pix_clk_div);
 	}
-	dev_dbg(dev, "vt_sys_clk_div \t%d\n",  pll->vt_sys_clk_div);
-	dev_dbg(dev, "vt_pix_clk_div \t%d\n",  pll->vt_pix_clk_div);
+	dev_dbg(dev, "vt_sys_clk_div \t%u\n",  pll->vt_sys_clk_div);
+	dev_dbg(dev, "vt_pix_clk_div \t%u\n",  pll->vt_pix_clk_div);
 
-	dev_dbg(dev, "ext_clk_freq_hz \t%d\n", pll->ext_clk_freq_hz);
-	dev_dbg(dev, "pll_ip_clk_freq_hz \t%d\n", pll->pll_ip_clk_freq_hz);
-	dev_dbg(dev, "pll_op_clk_freq_hz \t%d\n", pll->pll_op_clk_freq_hz);
+	dev_dbg(dev, "ext_clk_freq_hz \t%u\n", pll->ext_clk_freq_hz);
+	dev_dbg(dev, "pll_ip_clk_freq_hz \t%u\n", pll->pll_ip_clk_freq_hz);
+	dev_dbg(dev, "pll_op_clk_freq_hz \t%u\n", pll->pll_op_clk_freq_hz);
 	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
-		dev_dbg(dev, "op_sys_clk_freq_hz \t%d\n",
+		dev_dbg(dev, "op_sys_clk_freq_hz \t%u\n",
 			pll->op_sys_clk_freq_hz);
-		dev_dbg(dev, "op_pix_clk_freq_hz \t%d\n",
+		dev_dbg(dev, "op_pix_clk_freq_hz \t%u\n",
 			pll->op_pix_clk_freq_hz);
 	}
-	dev_dbg(dev, "vt_sys_clk_freq_hz \t%d\n", pll->vt_sys_clk_freq_hz);
-	dev_dbg(dev, "vt_pix_clk_freq_hz \t%d\n", pll->vt_pix_clk_freq_hz);
+	dev_dbg(dev, "vt_sys_clk_freq_hz \t%u\n", pll->vt_sys_clk_freq_hz);
+	dev_dbg(dev, "vt_pix_clk_freq_hz \t%u\n", pll->vt_pix_clk_freq_hz);
 }
 
 /*
@@ -123,11 +123,11 @@ static int __smiapp_pll_calculate(struct device *dev,
 	 * Get pre_pll_clk_div so that our pll_op_clk_freq_hz won't be
 	 * too high.
 	 */
-	dev_dbg(dev, "pre_pll_clk_div %d\n", pll->pre_pll_clk_div);
+	dev_dbg(dev, "pre_pll_clk_div %u\n", pll->pre_pll_clk_div);
 
 	/* Don't go above max pll multiplier. */
 	more_mul_max = limits->max_pll_multiplier / mul;
-	dev_dbg(dev, "more_mul_max: max_pll_multiplier check: %d\n",
+	dev_dbg(dev, "more_mul_max: max_pll_multiplier check: %u\n",
 		more_mul_max);
 	/* Don't go above max pll op frequency. */
 	more_mul_max =
@@ -135,30 +135,30 @@ static int __smiapp_pll_calculate(struct device *dev,
 		      more_mul_max,
 		      limits->max_pll_op_freq_hz
 		      / (pll->ext_clk_freq_hz / pll->pre_pll_clk_div * mul));
-	dev_dbg(dev, "more_mul_max: max_pll_op_freq_hz check: %d\n",
+	dev_dbg(dev, "more_mul_max: max_pll_op_freq_hz check: %u\n",
 		more_mul_max);
 	/* Don't go above the division capability of op sys clock divider. */
 	more_mul_max = min(more_mul_max,
 			   limits->op.max_sys_clk_div * pll->pre_pll_clk_div
 			   / div);
-	dev_dbg(dev, "more_mul_max: max_op_sys_clk_div check: %d\n",
+	dev_dbg(dev, "more_mul_max: max_op_sys_clk_div check: %u\n",
 		more_mul_max);
 	/* Ensure we won't go above min_pll_multiplier. */
 	more_mul_max = min(more_mul_max,
 			   DIV_ROUND_UP(limits->max_pll_multiplier, mul));
-	dev_dbg(dev, "more_mul_max: min_pll_multiplier check: %d\n",
+	dev_dbg(dev, "more_mul_max: min_pll_multiplier check: %u\n",
 		more_mul_max);
 
 	/* Ensure we won't go below min_pll_op_freq_hz. */
 	more_mul_min = DIV_ROUND_UP(limits->min_pll_op_freq_hz,
 				    pll->ext_clk_freq_hz / pll->pre_pll_clk_div
 				    * mul);
-	dev_dbg(dev, "more_mul_min: min_pll_op_freq_hz check: %d\n",
+	dev_dbg(dev, "more_mul_min: min_pll_op_freq_hz check: %u\n",
 		more_mul_min);
 	/* Ensure we won't go below min_pll_multiplier. */
 	more_mul_min = max(more_mul_min,
 			   DIV_ROUND_UP(limits->min_pll_multiplier, mul));
-	dev_dbg(dev, "more_mul_min: min_pll_multiplier check: %d\n",
+	dev_dbg(dev, "more_mul_min: min_pll_multiplier check: %u\n",
 		more_mul_min);
 
 	if (more_mul_min > more_mul_max) {
@@ -168,23 +168,23 @@ static int __smiapp_pll_calculate(struct device *dev,
 	}
 
 	more_mul_factor = lcm(div, pll->pre_pll_clk_div) / div;
-	dev_dbg(dev, "more_mul_factor: %d\n", more_mul_factor);
+	dev_dbg(dev, "more_mul_factor: %u\n", more_mul_factor);
 	more_mul_factor = lcm(more_mul_factor, limits->op.min_sys_clk_div);
-	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %d\n",
+	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %u\n",
 		more_mul_factor);
 	i = roundup(more_mul_min, more_mul_factor);
 	if (!is_one_or_even(i))
 		i <<= 1;
 
-	dev_dbg(dev, "final more_mul: %d\n", i);
+	dev_dbg(dev, "final more_mul: %u\n", i);
 	if (i > more_mul_max) {
-		dev_dbg(dev, "final more_mul is bad, max %d\n", more_mul_max);
+		dev_dbg(dev, "final more_mul is bad, max %u\n", more_mul_max);
 		return -EINVAL;
 	}
 
 	pll->pll_multiplier = mul * i;
 	pll->op_sys_clk_div = div * i / pll->pre_pll_clk_div;
-	dev_dbg(dev, "op_sys_clk_div: %d\n", pll->op_sys_clk_div);
+	dev_dbg(dev, "op_sys_clk_div: %u\n", pll->op_sys_clk_div);
 
 	pll->pll_ip_clk_freq_hz = pll->ext_clk_freq_hz
 		/ pll->pre_pll_clk_div;
@@ -197,7 +197,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 		pll->pll_op_clk_freq_hz / pll->op_sys_clk_div;
 
 	pll->op_pix_clk_div = pll->bits_per_pixel;
-	dev_dbg(dev, "op_pix_clk_div: %d\n", pll->op_pix_clk_div);
+	dev_dbg(dev, "op_pix_clk_div: %u\n", pll->op_pix_clk_div);
 
 	pll->op_pix_clk_freq_hz =
 		pll->op_sys_clk_freq_hz / pll->op_pix_clk_div;
@@ -214,7 +214,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 		vt_op_binning_div = pll->binning_horizontal;
 	else
 		vt_op_binning_div = 1;
-	dev_dbg(dev, "vt_op_binning_div: %d\n", vt_op_binning_div);
+	dev_dbg(dev, "vt_op_binning_div: %u\n", vt_op_binning_div);
 
 	/*
 	 * Profile 2 supports vt_pix_clk_div E [4, 10]
@@ -227,30 +227,30 @@ static int __smiapp_pll_calculate(struct device *dev,
 	 *
 	 * Find absolute limits for the factor of vt divider.
 	 */
-	dev_dbg(dev, "scale_m: %d\n", pll->scale_m);
+	dev_dbg(dev, "scale_m: %u\n", pll->scale_m);
 	min_vt_div = DIV_ROUND_UP(pll->op_pix_clk_div * pll->op_sys_clk_div
 				  * pll->scale_n,
 				  lane_op_clock_ratio * vt_op_binning_div
 				  * pll->scale_m);
 
 	/* Find smallest and biggest allowed vt divisor. */
-	dev_dbg(dev, "min_vt_div: %d\n", min_vt_div);
+	dev_dbg(dev, "min_vt_div: %u\n", min_vt_div);
 	min_vt_div = max(min_vt_div,
 			 DIV_ROUND_UP(pll->pll_op_clk_freq_hz,
 				      limits->vt.max_pix_clk_freq_hz));
-	dev_dbg(dev, "min_vt_div: max_vt_pix_clk_freq_hz: %d\n",
+	dev_dbg(dev, "min_vt_div: max_vt_pix_clk_freq_hz: %u\n",
 		min_vt_div);
 	min_vt_div = max_t(uint32_t, min_vt_div,
 			   limits->vt.min_pix_clk_div
 			   * limits->vt.min_sys_clk_div);
-	dev_dbg(dev, "min_vt_div: min_vt_clk_div: %d\n", min_vt_div);
+	dev_dbg(dev, "min_vt_div: min_vt_clk_div: %u\n", min_vt_div);
 
 	max_vt_div = limits->vt.max_sys_clk_div * limits->vt.max_pix_clk_div;
-	dev_dbg(dev, "max_vt_div: %d\n", max_vt_div);
+	dev_dbg(dev, "max_vt_div: %u\n", max_vt_div);
 	max_vt_div = min(max_vt_div,
 			 DIV_ROUND_UP(pll->pll_op_clk_freq_hz,
 				      limits->vt.min_pix_clk_freq_hz));
-	dev_dbg(dev, "max_vt_div: min_vt_pix_clk_freq_hz: %d\n",
+	dev_dbg(dev, "max_vt_div: min_vt_pix_clk_freq_hz: %u\n",
 		max_vt_div);
 
 	/*
@@ -258,28 +258,28 @@ static int __smiapp_pll_calculate(struct device *dev,
 	 * with all values of pix_clk_div.
 	 */
 	min_sys_div = limits->vt.min_sys_clk_div;
-	dev_dbg(dev, "min_sys_div: %d\n", min_sys_div);
+	dev_dbg(dev, "min_sys_div: %u\n", min_sys_div);
 	min_sys_div = max(min_sys_div,
 			  DIV_ROUND_UP(min_vt_div,
 				       limits->vt.max_pix_clk_div));
-	dev_dbg(dev, "min_sys_div: max_vt_pix_clk_div: %d\n", min_sys_div);
+	dev_dbg(dev, "min_sys_div: max_vt_pix_clk_div: %u\n", min_sys_div);
 	min_sys_div = max(min_sys_div,
 			  pll->pll_op_clk_freq_hz
 			  / limits->vt.max_sys_clk_freq_hz);
-	dev_dbg(dev, "min_sys_div: max_pll_op_clk_freq_hz: %d\n", min_sys_div);
+	dev_dbg(dev, "min_sys_div: max_pll_op_clk_freq_hz: %u\n", min_sys_div);
 	min_sys_div = clk_div_even_up(min_sys_div);
-	dev_dbg(dev, "min_sys_div: one or even: %d\n", min_sys_div);
+	dev_dbg(dev, "min_sys_div: one or even: %u\n", min_sys_div);
 
 	max_sys_div = limits->vt.max_sys_clk_div;
-	dev_dbg(dev, "max_sys_div: %d\n", max_sys_div);
+	dev_dbg(dev, "max_sys_div: %u\n", max_sys_div);
 	max_sys_div = min(max_sys_div,
 			  DIV_ROUND_UP(max_vt_div,
 				       limits->vt.min_pix_clk_div));
-	dev_dbg(dev, "max_sys_div: min_vt_pix_clk_div: %d\n", max_sys_div);
+	dev_dbg(dev, "max_sys_div: min_vt_pix_clk_div: %u\n", max_sys_div);
 	max_sys_div = min(max_sys_div,
 			  DIV_ROUND_UP(pll->pll_op_clk_freq_hz,
 				       limits->vt.min_pix_clk_freq_hz));
-	dev_dbg(dev, "max_sys_div: min_vt_pix_clk_freq_hz: %d\n", max_sys_div);
+	dev_dbg(dev, "max_sys_div: min_vt_pix_clk_freq_hz: %u\n", max_sys_div);
 
 	/*
 	 * Find pix_div such that a legal pix_div * sys_div results
@@ -296,7 +296,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 			if (pix_div < limits->vt.min_pix_clk_div
 			    || pix_div > limits->vt.max_pix_clk_div) {
 				dev_dbg(dev,
-					"pix_div %d too small or too big (%d--%d)\n",
+					"pix_div %u too small or too big (%u--%u)\n",
 					pix_div,
 					limits->vt.min_pix_clk_div,
 					limits->vt.max_pix_clk_div);
@@ -390,9 +390,9 @@ int smiapp_pll_calculate(struct device *dev,
 		lane_op_clock_ratio = pll->csi2.lanes;
 	else
 		lane_op_clock_ratio = 1;
-	dev_dbg(dev, "lane_op_clock_ratio: %d\n", lane_op_clock_ratio);
+	dev_dbg(dev, "lane_op_clock_ratio: %u\n", lane_op_clock_ratio);
 
-	dev_dbg(dev, "binning: %dx%d\n", pll->binning_horizontal,
+	dev_dbg(dev, "binning: %ux%u\n", pll->binning_horizontal,
 		pll->binning_vertical);
 
 	switch (pll->bus_type) {
@@ -411,7 +411,7 @@ int smiapp_pll_calculate(struct device *dev,
 	}
 
 	/* Figure out limits for pre-pll divider based on extclk */
-	dev_dbg(dev, "min / max pre_pll_clk_div: %d / %d\n",
+	dev_dbg(dev, "min / max pre_pll_clk_div: %u / %u\n",
 		limits->min_pre_pll_clk_div, limits->max_pre_pll_clk_div);
 	max_pre_pll_clk_div =
 		min_t(uint16_t, limits->max_pre_pll_clk_div,
@@ -422,20 +422,20 @@ int smiapp_pll_calculate(struct device *dev,
 		      clk_div_even_up(
 			      DIV_ROUND_UP(pll->ext_clk_freq_hz,
 					   limits->max_pll_ip_freq_hz)));
-	dev_dbg(dev, "pre-pll check: min / max pre_pll_clk_div: %d / %d\n",
+	dev_dbg(dev, "pre-pll check: min / max pre_pll_clk_div: %u / %u\n",
 		min_pre_pll_clk_div, max_pre_pll_clk_div);
 
 	i = gcd(pll->pll_op_clk_freq_hz, pll->ext_clk_freq_hz);
 	mul = div_u64(pll->pll_op_clk_freq_hz, i);
 	div = pll->ext_clk_freq_hz / i;
-	dev_dbg(dev, "mul %d / div %d\n", mul, div);
+	dev_dbg(dev, "mul %u / div %u\n", mul, div);
 
 	min_pre_pll_clk_div =
 		max_t(uint16_t, min_pre_pll_clk_div,
 		      clk_div_even_up(
 			      DIV_ROUND_UP(mul * pll->ext_clk_freq_hz,
 					   limits->max_pll_op_freq_hz)));
-	dev_dbg(dev, "pll_op check: min / max pre_pll_clk_div: %d / %d\n",
+	dev_dbg(dev, "pll_op check: min / max pre_pll_clk_div: %u / %u\n",
 		min_pre_pll_clk_div, max_pre_pll_clk_div);
 
 	for (pll->pre_pll_clk_div = min_pre_pll_clk_div;
-- 
1.7.10.4

[PATCH 03/17] smiapp-pll: Separate bounds checking into a separate function

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Enough work for this function already.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |  110 +++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index d14af5c..bde8eb8 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -87,6 +87,64 @@ static void print_pll(struct device *dev, struct smiapp_pll *pll)
 	dev_dbg(dev, "vt_pix_clk_freq_hz \t%u\n", pll->vt_pix_clk_freq_hz);
 }
 
+static int check_all_bounds(struct device *dev,
+			    const struct smiapp_pll_limits *limits,
+			    struct smiapp_pll *pll)
+{
+	int rval;
+
+	rval = bounds_check(dev, pll->pll_ip_clk_freq_hz,
+			    limits->min_pll_ip_freq_hz,
+			    limits->max_pll_ip_freq_hz,
+			    "pll_ip_clk_freq_hz");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->pll_multiplier,
+			limits->min_pll_multiplier, limits->max_pll_multiplier,
+			"pll_multiplier");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->pll_op_clk_freq_hz,
+			limits->min_pll_op_freq_hz, limits->max_pll_op_freq_hz,
+			"pll_op_clk_freq_hz");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->op_sys_clk_div,
+			limits->op.min_sys_clk_div, limits->op.max_sys_clk_div,
+			"op_sys_clk_div");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->op_pix_clk_div,
+			limits->op.min_pix_clk_div, limits->op.max_pix_clk_div,
+			"op_pix_clk_div");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->op_sys_clk_freq_hz,
+			limits->op.min_sys_clk_freq_hz,
+			limits->op.max_sys_clk_freq_hz,
+			"op_sys_clk_freq_hz");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->op_pix_clk_freq_hz,
+			limits->op.min_pix_clk_freq_hz,
+			limits->op.max_pix_clk_freq_hz,
+			"op_pix_clk_freq_hz");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->vt_sys_clk_freq_hz,
+			limits->vt.min_sys_clk_freq_hz,
+			limits->vt.max_sys_clk_freq_hz,
+			"vt_sys_clk_freq_hz");
+	if (!rval)
+		rval = bounds_check(
+			dev, pll->vt_pix_clk_freq_hz,
+			limits->vt.min_pix_clk_freq_hz,
+			limits->vt.max_pix_clk_freq_hz,
+			"vt_pix_clk_freq_hz");
+
+	return rval;
+}
+
 /*
  * Heuristically guess the PLL tree for a given common multiplier and
  * divisor. Begin with the operational timing and continue to video
@@ -117,7 +175,6 @@ static int __smiapp_pll_calculate(struct device *dev,
 	uint32_t min_vt_div, max_vt_div, vt_div;
 	uint32_t min_sys_div, max_sys_div;
 	unsigned int i;
-	int rval;
 
 	/*
 	 * Get pre_pll_clk_div so that our pll_op_clk_freq_hz won't be
@@ -323,56 +380,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 	pll->pixel_rate_csi =
 		pll->op_pix_clk_freq_hz * lane_op_clock_ratio;
 
-	rval = bounds_check(dev, pll->pll_ip_clk_freq_hz,
-			    limits->min_pll_ip_freq_hz,
-			    limits->max_pll_ip_freq_hz,
-			    "pll_ip_clk_freq_hz");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->pll_multiplier,
-			limits->min_pll_multiplier, limits->max_pll_multiplier,
-			"pll_multiplier");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->pll_op_clk_freq_hz,
-			limits->min_pll_op_freq_hz, limits->max_pll_op_freq_hz,
-			"pll_op_clk_freq_hz");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->op_sys_clk_div,
-			limits->op.min_sys_clk_div, limits->op.max_sys_clk_div,
-			"op_sys_clk_div");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->op_pix_clk_div,
-			limits->op.min_pix_clk_div, limits->op.max_pix_clk_div,
-			"op_pix_clk_div");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->op_sys_clk_freq_hz,
-			limits->op.min_sys_clk_freq_hz,
-			limits->op.max_sys_clk_freq_hz,
-			"op_sys_clk_freq_hz");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->op_pix_clk_freq_hz,
-			limits->op.min_pix_clk_freq_hz,
-			limits->op.max_pix_clk_freq_hz,
-			"op_pix_clk_freq_hz");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->vt_sys_clk_freq_hz,
-			limits->vt.min_sys_clk_freq_hz,
-			limits->vt.max_sys_clk_freq_hz,
-			"vt_sys_clk_freq_hz");
-	if (!rval)
-		rval = bounds_check(
-			dev, pll->vt_pix_clk_freq_hz,
-			limits->vt.min_pix_clk_freq_hz,
-			limits->vt.max_pix_clk_freq_hz,
-			"vt_pix_clk_freq_hz");
-
-	return rval;
+	return check_all_bounds(dev, limits, pll);
 }
 
 int smiapp_pll_calculate(struct device *dev,
-- 
1.7.10.4

[PATCH 04/17] smiapp-pll: External clock frequency isn't an output value

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

It's input. Move it elsewhere in the struct.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/smiapp-pll.h b/drivers/media/i2c/smiapp-pll.h
index 5ce2b61..2885cd7 100644
--- a/drivers/media/i2c/smiapp-pll.h
+++ b/drivers/media/i2c/smiapp-pll.h
@@ -53,6 +53,7 @@ struct smiapp_pll {
 	uint8_t scale_n;
 	uint8_t bits_per_pixel;
 	uint32_t link_freq;
+	uint32_t ext_clk_freq_hz;
 
 	/* output values */
 	uint16_t pre_pll_clk_div;
@@ -62,7 +63,6 @@ struct smiapp_pll {
 	uint16_t vt_sys_clk_div;
 	uint16_t vt_pix_clk_div;
 
-	uint32_t ext_clk_freq_hz;
 	uint32_t pll_ip_clk_freq_hz;
 	uint32_t pll_op_clk_freq_hz;
 	uint32_t op_sys_clk_freq_hz;
-- 
1.7.10.4

[PATCH 05/17] smiapp-pll: Unify OP and VT PLL structs

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Uniform representation for VT and OP clocks.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c         |   60 ++++++++++++++++----------------
 drivers/media/i2c/smiapp-pll.h         |   18 +++++-----
 drivers/media/i2c/smiapp/smiapp-core.c |   14 ++++----
 3 files changed, 46 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index bde8eb8..40a18ba 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -68,23 +68,23 @@ static void print_pll(struct device *dev, struct smiapp_pll *pll)
 	dev_dbg(dev, "pre_pll_clk_div\t%u\n",  pll->pre_pll_clk_div);
 	dev_dbg(dev, "pll_multiplier \t%u\n",  pll->pll_multiplier);
 	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
-		dev_dbg(dev, "op_sys_clk_div \t%u\n", pll->op_sys_clk_div);
-		dev_dbg(dev, "op_pix_clk_div \t%u\n", pll->op_pix_clk_div);
+		dev_dbg(dev, "op_sys_clk_div \t%u\n", pll->op.sys_clk_div);
+		dev_dbg(dev, "op_pix_clk_div \t%u\n", pll->op.pix_clk_div);
 	}
-	dev_dbg(dev, "vt_sys_clk_div \t%u\n",  pll->vt_sys_clk_div);
-	dev_dbg(dev, "vt_pix_clk_div \t%u\n",  pll->vt_pix_clk_div);
+	dev_dbg(dev, "vt_sys_clk_div \t%u\n",  pll->vt.sys_clk_div);
+	dev_dbg(dev, "vt_pix_clk_div \t%u\n",  pll->vt.pix_clk_div);
 
 	dev_dbg(dev, "ext_clk_freq_hz \t%u\n", pll->ext_clk_freq_hz);
 	dev_dbg(dev, "pll_ip_clk_freq_hz \t%u\n", pll->pll_ip_clk_freq_hz);
 	dev_dbg(dev, "pll_op_clk_freq_hz \t%u\n", pll->pll_op_clk_freq_hz);
 	if (!(pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)) {
 		dev_dbg(dev, "op_sys_clk_freq_hz \t%u\n",
-			pll->op_sys_clk_freq_hz);
+			pll->op.sys_clk_freq_hz);
 		dev_dbg(dev, "op_pix_clk_freq_hz \t%u\n",
-			pll->op_pix_clk_freq_hz);
+			pll->op.pix_clk_freq_hz);
 	}
-	dev_dbg(dev, "vt_sys_clk_freq_hz \t%u\n", pll->vt_sys_clk_freq_hz);
-	dev_dbg(dev, "vt_pix_clk_freq_hz \t%u\n", pll->vt_pix_clk_freq_hz);
+	dev_dbg(dev, "vt_sys_clk_freq_hz \t%u\n", pll->vt.sys_clk_freq_hz);
+	dev_dbg(dev, "vt_pix_clk_freq_hz \t%u\n", pll->vt.pix_clk_freq_hz);
 }
 
 static int check_all_bounds(struct device *dev,
@@ -109,35 +109,35 @@ static int check_all_bounds(struct device *dev,
 			"pll_op_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op_sys_clk_div,
+			dev, pll->op.sys_clk_div,
 			limits->op.min_sys_clk_div, limits->op.max_sys_clk_div,
 			"op_sys_clk_div");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op_pix_clk_div,
+			dev, pll->op.pix_clk_div,
 			limits->op.min_pix_clk_div, limits->op.max_pix_clk_div,
 			"op_pix_clk_div");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op_sys_clk_freq_hz,
+			dev, pll->op.sys_clk_freq_hz,
 			limits->op.min_sys_clk_freq_hz,
 			limits->op.max_sys_clk_freq_hz,
 			"op_sys_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op_pix_clk_freq_hz,
+			dev, pll->op.pix_clk_freq_hz,
 			limits->op.min_pix_clk_freq_hz,
 			limits->op.max_pix_clk_freq_hz,
 			"op_pix_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->vt_sys_clk_freq_hz,
+			dev, pll->vt.sys_clk_freq_hz,
 			limits->vt.min_sys_clk_freq_hz,
 			limits->vt.max_sys_clk_freq_hz,
 			"vt_sys_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->vt_pix_clk_freq_hz,
+			dev, pll->vt.pix_clk_freq_hz,
 			limits->vt.min_pix_clk_freq_hz,
 			limits->vt.max_pix_clk_freq_hz,
 			"vt_pix_clk_freq_hz");
@@ -240,8 +240,8 @@ static int __smiapp_pll_calculate(struct device *dev,
 	}
 
 	pll->pll_multiplier = mul * i;
-	pll->op_sys_clk_div = div * i / pll->pre_pll_clk_div;
-	dev_dbg(dev, "op_sys_clk_div: %u\n", pll->op_sys_clk_div);
+	pll->op.sys_clk_div = div * i / pll->pre_pll_clk_div;
+	dev_dbg(dev, "op_sys_clk_div: %u\n", pll->op.sys_clk_div);
 
 	pll->pll_ip_clk_freq_hz = pll->ext_clk_freq_hz
 		/ pll->pre_pll_clk_div;
@@ -250,14 +250,14 @@ static int __smiapp_pll_calculate(struct device *dev,
 		* pll->pll_multiplier;
 
 	/* Derive pll_op_clk_freq_hz. */
-	pll->op_sys_clk_freq_hz =
-		pll->pll_op_clk_freq_hz / pll->op_sys_clk_div;
+	pll->op.sys_clk_freq_hz =
+		pll->pll_op_clk_freq_hz / pll->op.sys_clk_div;
 
-	pll->op_pix_clk_div = pll->bits_per_pixel;
-	dev_dbg(dev, "op_pix_clk_div: %u\n", pll->op_pix_clk_div);
+	pll->op.pix_clk_div = pll->bits_per_pixel;
+	dev_dbg(dev, "op_pix_clk_div: %u\n", pll->op.pix_clk_div);
 
-	pll->op_pix_clk_freq_hz =
-		pll->op_sys_clk_freq_hz / pll->op_pix_clk_div;
+	pll->op.pix_clk_freq_hz =
+		pll->op.sys_clk_freq_hz / pll->op.pix_clk_div;
 
 	/*
 	 * Some sensors perform analogue binning and some do this
@@ -285,7 +285,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 	 * Find absolute limits for the factor of vt divider.
 	 */
 	dev_dbg(dev, "scale_m: %u\n", pll->scale_m);
-	min_vt_div = DIV_ROUND_UP(pll->op_pix_clk_div * pll->op_sys_clk_div
+	min_vt_div = DIV_ROUND_UP(pll->op.pix_clk_div * pll->op.sys_clk_div
 				  * pll->scale_n,
 				  lane_op_clock_ratio * vt_op_binning_div
 				  * pll->scale_m);
@@ -369,16 +369,16 @@ static int __smiapp_pll_calculate(struct device *dev,
 			break;
 	}
 
-	pll->vt_sys_clk_div = DIV_ROUND_UP(min_vt_div, best_pix_div);
-	pll->vt_pix_clk_div = best_pix_div;
+	pll->vt.sys_clk_div = DIV_ROUND_UP(min_vt_div, best_pix_div);
+	pll->vt.pix_clk_div = best_pix_div;
 
-	pll->vt_sys_clk_freq_hz =
-		pll->pll_op_clk_freq_hz / pll->vt_sys_clk_div;
-	pll->vt_pix_clk_freq_hz =
-		pll->vt_sys_clk_freq_hz / pll->vt_pix_clk_div;
+	pll->vt.sys_clk_freq_hz =
+		pll->pll_op_clk_freq_hz / pll->vt.sys_clk_div;
+	pll->vt.pix_clk_freq_hz =
+		pll->vt.sys_clk_freq_hz / pll->vt.pix_clk_div;
 
 	pll->pixel_rate_csi =
-		pll->op_pix_clk_freq_hz * lane_op_clock_ratio;
+		pll->op.pix_clk_freq_hz * lane_op_clock_ratio;
 
 	return check_all_bounds(dev, limits, pll);
 }
diff --git a/drivers/media/i2c/smiapp-pll.h b/drivers/media/i2c/smiapp-pll.h
index 2885cd7..b7c0e66 100644
--- a/drivers/media/i2c/smiapp-pll.h
+++ b/drivers/media/i2c/smiapp-pll.h
@@ -35,6 +35,13 @@
 #define SMIAPP_PLL_FLAG_OP_PIX_CLOCK_PER_LANE			(1 << 0)
 #define SMIAPP_PLL_FLAG_NO_OP_CLOCKS				(1 << 1)
 
+struct smiapp_pll_branch {
+	uint16_t sys_clk_div;
+	uint16_t pix_clk_div;
+	uint32_t sys_clk_freq_hz;
+	uint32_t pix_clk_freq_hz;
+};
+
 struct smiapp_pll {
 	/* input values */
 	uint8_t bus_type;
@@ -58,17 +65,10 @@ struct smiapp_pll {
 	/* output values */
 	uint16_t pre_pll_clk_div;
 	uint16_t pll_multiplier;
-	uint16_t op_sys_clk_div;
-	uint16_t op_pix_clk_div;
-	uint16_t vt_sys_clk_div;
-	uint16_t vt_pix_clk_div;
-
 	uint32_t pll_ip_clk_freq_hz;
 	uint32_t pll_op_clk_freq_hz;
-	uint32_t op_sys_clk_freq_hz;
-	uint32_t op_pix_clk_freq_hz;
-	uint32_t vt_sys_clk_freq_hz;
-	uint32_t vt_pix_clk_freq_hz;
+	struct smiapp_pll_branch vt;
+	struct smiapp_pll_branch op;
 
 	uint32_t pixel_rate_csi;
 };
diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index daa179d..2f81c9c 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -205,12 +205,12 @@ static int smiapp_pll_configure(struct smiapp_sensor *sensor)
 	int rval;
 
 	rval = smiapp_write(
-		sensor, SMIAPP_REG_U16_VT_PIX_CLK_DIV, pll->vt_pix_clk_div);
+		sensor, SMIAPP_REG_U16_VT_PIX_CLK_DIV, pll->vt.pix_clk_div);
 	if (rval < 0)
 		return rval;
 
 	rval = smiapp_write(
-		sensor, SMIAPP_REG_U16_VT_SYS_CLK_DIV, pll->vt_sys_clk_div);
+		sensor, SMIAPP_REG_U16_VT_SYS_CLK_DIV, pll->vt.sys_clk_div);
 	if (rval < 0)
 		return rval;
 
@@ -227,17 +227,17 @@ static int smiapp_pll_configure(struct smiapp_sensor *sensor)
 	/* Lane op clock ratio does not apply here. */
 	rval = smiapp_write(
 		sensor, SMIAPP_REG_U32_REQUESTED_LINK_BIT_RATE_MBPS,
-		DIV_ROUND_UP(pll->op_sys_clk_freq_hz, 1000000 / 256 / 256));
+		DIV_ROUND_UP(pll->op.sys_clk_freq_hz, 1000000 / 256 / 256));
 	if (rval < 0 || sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
 		return rval;
 
 	rval = smiapp_write(
-		sensor, SMIAPP_REG_U16_OP_PIX_CLK_DIV, pll->op_pix_clk_div);
+		sensor, SMIAPP_REG_U16_OP_PIX_CLK_DIV, pll->op.pix_clk_div);
 	if (rval < 0)
 		return rval;
 
 	return smiapp_write(
-		sensor, SMIAPP_REG_U16_OP_SYS_CLK_DIV, pll->op_sys_clk_div);
+		sensor, SMIAPP_REG_U16_OP_SYS_CLK_DIV, pll->op.sys_clk_div);
 }
 
 static int smiapp_pll_update(struct smiapp_sensor *sensor)
@@ -299,7 +299,7 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 		return rval;
 
 	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_parray,
-				 pll->vt_pix_clk_freq_hz);
+				 pll->vt.pix_clk_freq_hz);
 	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_csi, pll->pixel_rate_csi);
 
 	return 0;
@@ -904,7 +904,7 @@ static int smiapp_update_mode(struct smiapp_sensor *sensor)
 	dev_dbg(&client->dev, "hblank\t\t%d\n", sensor->hblank->val);
 
 	dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
-		sensor->pll.vt_pix_clk_freq_hz /
+		sensor->pll.vt.pix_clk_freq_hz /
 		((sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width
 		  + sensor->hblank->val) *
 		 (sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height
-- 
1.7.10.4

[PATCH 06/17] smiapp-pll: Don't validate OP clocks if there are none

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Profile 0 sensors have no OP clocks, don't validate them.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 40a18ba..f83f25f 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -129,6 +129,14 @@ static int check_all_bounds(struct device *dev,
 			limits->op.min_pix_clk_freq_hz,
 			limits->op.max_pix_clk_freq_hz,
 			"op_pix_clk_freq_hz");
+
+	/*
+	 * If there are no OP clocks, the VT clocks are contained in
+	 * the OP clock struct.
+	 */
+	if (pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS)
+		return rval;
+
 	if (!rval)
 		rval = bounds_check(
 			dev, pll->vt.sys_clk_freq_hz,
-- 
1.7.10.4

[PATCH 07/17] smiapp-pll: Calculate OP clocks only for sensors that have them

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Profile 0 sensors have no OP clock branck in the clock tree. The PLL
calculator still calculated them, they just weren't used for anything.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |   82 +++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index f83f25f..862ca0c 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -89,7 +89,9 @@ static void print_pll(struct device *dev, struct smiapp_pll *pll)
 
 static int check_all_bounds(struct device *dev,
 			    const struct smiapp_pll_limits *limits,
-			    struct smiapp_pll *pll)
+			    const struct smiapp_pll_branch_limits *op_limits,
+			    struct smiapp_pll *pll,
+			    struct smiapp_pll_branch *op_pll)
 {
 	int rval;
 
@@ -109,25 +111,25 @@ static int check_all_bounds(struct device *dev,
 			"pll_op_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op.sys_clk_div,
-			limits->op.min_sys_clk_div, limits->op.max_sys_clk_div,
+			dev, op_pll->sys_clk_div,
+			op_limits->min_sys_clk_div, op_limits->max_sys_clk_div,
 			"op_sys_clk_div");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op.pix_clk_div,
-			limits->op.min_pix_clk_div, limits->op.max_pix_clk_div,
+			dev, op_pll->pix_clk_div,
+			op_limits->min_pix_clk_div, op_limits->max_pix_clk_div,
 			"op_pix_clk_div");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op.sys_clk_freq_hz,
-			limits->op.min_sys_clk_freq_hz,
-			limits->op.max_sys_clk_freq_hz,
+			dev, op_pll->sys_clk_freq_hz,
+			op_limits->min_sys_clk_freq_hz,
+			op_limits->max_sys_clk_freq_hz,
 			"op_sys_clk_freq_hz");
 	if (!rval)
 		rval = bounds_check(
-			dev, pll->op.pix_clk_freq_hz,
-			limits->op.min_pix_clk_freq_hz,
-			limits->op.max_pix_clk_freq_hz,
+			dev, op_pll->pix_clk_freq_hz,
+			op_limits->min_pix_clk_freq_hz,
+			op_limits->max_pix_clk_freq_hz,
 			"op_pix_clk_freq_hz");
 
 	/*
@@ -164,10 +166,11 @@ static int check_all_bounds(struct device *dev,
  *
  * @return Zero on success, error code on error.
  */
-static int __smiapp_pll_calculate(struct device *dev,
-				  const struct smiapp_pll_limits *limits,
-				  struct smiapp_pll *pll, uint32_t mul,
-				  uint32_t div, uint32_t lane_op_clock_ratio)
+static int __smiapp_pll_calculate(
+	struct device *dev, const struct smiapp_pll_limits *limits,
+	const struct smiapp_pll_branch_limits *op_limits,
+	struct smiapp_pll *pll, struct smiapp_pll_branch *op_pll, uint32_t mul,
+	uint32_t div, uint32_t lane_op_clock_ratio)
 {
 	uint32_t sys_div;
 	uint32_t best_pix_div = INT_MAX >> 1;
@@ -204,7 +207,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 		more_mul_max);
 	/* Don't go above the division capability of op sys clock divider. */
 	more_mul_max = min(more_mul_max,
-			   limits->op.max_sys_clk_div * pll->pre_pll_clk_div
+			   op_limits->max_sys_clk_div * pll->pre_pll_clk_div
 			   / div);
 	dev_dbg(dev, "more_mul_max: max_op_sys_clk_div check: %u\n",
 		more_mul_max);
@@ -234,8 +237,8 @@ static int __smiapp_pll_calculate(struct device *dev,
 
 	more_mul_factor = lcm(div, pll->pre_pll_clk_div) / div;
 	dev_dbg(dev, "more_mul_factor: %u\n", more_mul_factor);
-	more_mul_factor = lcm(more_mul_factor, limits->op.min_sys_clk_div);
-	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %u\n",
+	more_mul_factor = lcm(more_mul_factor, op_limits->min_sys_clk_div);
+	dev_dbg(dev, "more_mul_factor: min_op_sys_clk_div: %d\n",
 		more_mul_factor);
 	i = roundup(more_mul_min, more_mul_factor);
 	if (!is_one_or_even(i))
@@ -248,8 +251,8 @@ static int __smiapp_pll_calculate(struct device *dev,
 	}
 
 	pll->pll_multiplier = mul * i;
-	pll->op.sys_clk_div = div * i / pll->pre_pll_clk_div;
-	dev_dbg(dev, "op_sys_clk_div: %u\n", pll->op.sys_clk_div);
+	op_pll->sys_clk_div = div * i / pll->pre_pll_clk_div;
+	dev_dbg(dev, "op_sys_clk_div: %u\n", op_pll->sys_clk_div);
 
 	pll->pll_ip_clk_freq_hz = pll->ext_clk_freq_hz
 		/ pll->pre_pll_clk_div;
@@ -258,14 +261,19 @@ static int __smiapp_pll_calculate(struct device *dev,
 		* pll->pll_multiplier;
 
 	/* Derive pll_op_clk_freq_hz. */
-	pll->op.sys_clk_freq_hz =
-		pll->pll_op_clk_freq_hz / pll->op.sys_clk_div;
+	op_pll->sys_clk_freq_hz =
+		pll->pll_op_clk_freq_hz / op_pll->sys_clk_div;
 
-	pll->op.pix_clk_div = pll->bits_per_pixel;
-	dev_dbg(dev, "op_pix_clk_div: %u\n", pll->op.pix_clk_div);
+	op_pll->pix_clk_div = pll->bits_per_pixel;
+	dev_dbg(dev, "op_pix_clk_div: %u\n", op_pll->pix_clk_div);
 
-	pll->op.pix_clk_freq_hz =
-		pll->op.sys_clk_freq_hz / pll->op.pix_clk_div;
+	op_pll->pix_clk_freq_hz =
+		op_pll->sys_clk_freq_hz / op_pll->pix_clk_div;
+
+	if (pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS) {
+		/* No OP clocks --- VT clocks are used instead. */
+		goto out_skip_vt_calc;
+	}
 
 	/*
 	 * Some sensors perform analogue binning and some do this
@@ -293,7 +301,7 @@ static int __smiapp_pll_calculate(struct device *dev,
 	 * Find absolute limits for the factor of vt divider.
 	 */
 	dev_dbg(dev, "scale_m: %u\n", pll->scale_m);
-	min_vt_div = DIV_ROUND_UP(pll->op.pix_clk_div * pll->op.sys_clk_div
+	min_vt_div = DIV_ROUND_UP(op_pll->pix_clk_div * op_pll->sys_clk_div
 				  * pll->scale_n,
 				  lane_op_clock_ratio * vt_op_binning_div
 				  * pll->scale_m);
@@ -385,16 +393,19 @@ static int __smiapp_pll_calculate(struct device *dev,
 	pll->vt.pix_clk_freq_hz =
 		pll->vt.sys_clk_freq_hz / pll->vt.pix_clk_div;
 
+out_skip_vt_calc:
 	pll->pixel_rate_csi =
-		pll->op.pix_clk_freq_hz * lane_op_clock_ratio;
+		op_pll->pix_clk_freq_hz * lane_op_clock_ratio;
 
-	return check_all_bounds(dev, limits, pll);
+	return check_all_bounds(dev, limits, op_limits, pll, op_pll);
 }
 
 int smiapp_pll_calculate(struct device *dev,
 			 const struct smiapp_pll_limits *limits,
 			 struct smiapp_pll *pll)
 {
+	const struct smiapp_pll_branch_limits *op_limits = &limits->op;
+	struct smiapp_pll_branch *op_pll = &pll->op;
 	uint16_t min_pre_pll_clk_div;
 	uint16_t max_pre_pll_clk_div;
 	uint32_t lane_op_clock_ratio;
@@ -402,6 +413,16 @@ int smiapp_pll_calculate(struct device *dev,
 	unsigned int i;
 	int rval = -EINVAL;
 
+	if (pll->flags & SMIAPP_PLL_FLAG_NO_OP_CLOCKS) {
+		/*
+		 * If there's no OP PLL at all, use the VT values
+		 * instead. The OP values are ignored for the rest of
+		 * the PLL calculation.
+		 */
+		op_limits = &limits->vt;
+		op_pll = &pll->vt;
+	}
+
 	if (pll->flags & SMIAPP_PLL_FLAG_OP_PIX_CLOCK_PER_LANE)
 		lane_op_clock_ratio = pll->csi2.lanes;
 	else
@@ -457,7 +478,8 @@ int smiapp_pll_calculate(struct device *dev,
 	for (pll->pre_pll_clk_div = min_pre_pll_clk_div;
 	     pll->pre_pll_clk_div <= max_pre_pll_clk_div;
 	     pll->pre_pll_clk_div += 2 - (pll->pre_pll_clk_div & 1)) {
-		rval = __smiapp_pll_calculate(dev, limits, pll, mul, div,
+		rval = __smiapp_pll_calculate(dev, limits, op_limits, pll,
+					      op_pll, mul, div,
 					      lane_op_clock_ratio);
 		if (rval)
 			continue;
-- 
1.7.10.4

[PATCH 08/17] smiapp: The PLL calculator handles sensors with VT clocks only

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

No need to pretend the OP limits are there anymore.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 2f81c9c..54cb136 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -277,16 +277,6 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 	struct smiapp_pll *pll = &sensor->pll;
 	int rval;
 
-	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0) {
-		/*
-		 * Fill in operational clock divisors limits from the
-		 * video timing ones. On profile 0 sensors the
-		 * requirements regarding them are essentially the
-		 * same as on VT ones.
-		 */
-		lim.op = lim.vt;
-	}
-
 	pll->binning_horizontal = sensor->binning_horizontal;
 	pll->binning_vertical = sensor->binning_vertical;
 	pll->link_freq =
-- 
1.7.10.4

[PATCH 09/17] smiapp: Remove validation of op_pix_clk_div

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

op_pix_clk_div is directly assigned and not calculated. There's no need to
verify it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |    5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 862ca0c..0d5c503 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -116,11 +116,6 @@ static int check_all_bounds(struct device *dev,
 			"op_sys_clk_div");
 	if (!rval)
 		rval = bounds_check(
-			dev, op_pll->pix_clk_div,
-			op_limits->min_pix_clk_div, op_limits->max_pix_clk_div,
-			"op_pix_clk_div");
-	if (!rval)
-		rval = bounds_check(
 			dev, op_pll->sys_clk_freq_hz,
 			op_limits->min_sys_clk_freq_hz,
 			op_limits->max_sys_clk_freq_hz,
-- 
1.7.10.4

[PATCH 10/17] smiapp-pll: Add pixel rate in pixel array as output parameters

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The actual pixel array pixel rate may be something else than vt_pix_clk_freq
on some implementations. Add a new field which contains the corrected value.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp-pll.c |    1 +
 drivers/media/i2c/smiapp-pll.h |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/media/i2c/smiapp-pll.c b/drivers/media/i2c/smiapp-pll.c
index 0d5c503..e40d902 100644
--- a/drivers/media/i2c/smiapp-pll.c
+++ b/drivers/media/i2c/smiapp-pll.c
@@ -391,6 +391,7 @@ static int __smiapp_pll_calculate(
 out_skip_vt_calc:
 	pll->pixel_rate_csi =
 		op_pll->pix_clk_freq_hz * lane_op_clock_ratio;
+	pll->pixel_rate_pixel_array = pll->vt.pix_clk_freq_hz;
 
 	return check_all_bounds(dev, limits, op_limits, pll, op_pll);
 }
diff --git a/drivers/media/i2c/smiapp-pll.h b/drivers/media/i2c/smiapp-pll.h
index b7c0e66..e8f035a 100644
--- a/drivers/media/i2c/smiapp-pll.h
+++ b/drivers/media/i2c/smiapp-pll.h
@@ -71,6 +71,7 @@ struct smiapp_pll {
 	struct smiapp_pll_branch op;
 
 	uint32_t pixel_rate_csi;
+	uint32_t pixel_rate_pixel_array;
 };
 
 struct smiapp_pll_branch_limits {
-- 
1.7.10.4

[PATCH 11/17] smiapp: Use actual pixel rate calculated by the PLL calculator

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 54cb136..6db0e8d 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -289,7 +289,7 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 		return rval;
 
 	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_parray,
-				 pll->vt.pix_clk_freq_hz);
+				 pll->pixel_rate_pixel_array);
 	__v4l2_ctrl_s_ctrl_int64(sensor->pixel_rate_csi, pll->pixel_rate_csi);
 
 	return 0;
@@ -894,7 +894,7 @@ static int smiapp_update_mode(struct smiapp_sensor *sensor)
 	dev_dbg(&client->dev, "hblank\t\t%d\n", sensor->hblank->val);
 
 	dev_dbg(&client->dev, "real timeperframe\t100/%d\n",
-		sensor->pll.vt.pix_clk_freq_hz /
+		sensor->pll.pixel_rate_pixel_array /
 		((sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].width
 		  + sensor->hblank->val) *
 		 (sensor->pixel_array->crop[SMIAPP_PA_PAD_SRC].height
-- 
1.7.10.4

[PATCH 12/17] smiapp: Take mutex during PLL update in sensor initialisation

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The mutex does not serialise anything in this case but avoids a lockdep
warning from the control framework.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 6db0e8d..346ff5b 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2667,7 +2667,9 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
 	pll->scale_n = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
 
+	mutex_lock(&sensor->mutex);
 	rval = smiapp_update_mode(sensor);
+	mutex_unlock(&sensor->mutex);
 	if (rval) {
 		dev_err(&client->dev, "update mode failed\n");
 		goto out_nvm_release;
-- 
1.7.10.4

[PATCH 13/17] smiapp: Split calculating PLL with sensor's limits from updating it

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The first one is handy for just trying out a PLL configuration without a
need to apply it.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 346ff5b..a1244e6 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -240,7 +240,8 @@ static int smiapp_pll_configure(struct smiapp_sensor *sensor)
 		sensor, SMIAPP_REG_U16_OP_SYS_CLK_DIV, pll->op.sys_clk_div);
 }
 
-static int smiapp_pll_update(struct smiapp_sensor *sensor)
+static int smiapp_pll_try(struct smiapp_sensor *sensor,
+			  struct smiapp_pll *pll)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
 	struct smiapp_pll_limits lim = {
@@ -274,6 +275,12 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 		.min_line_length_pck_bin = sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK_BIN],
 		.min_line_length_pck = sensor->limits[SMIAPP_LIMIT_MIN_LINE_LENGTH_PCK],
 	};
+
+	return smiapp_pll_calculate(&client->dev, &lim, pll);
+}
+
+static int smiapp_pll_update(struct smiapp_sensor *sensor)
+{
 	struct smiapp_pll *pll = &sensor->pll;
 	int rval;
 
@@ -284,7 +291,7 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 	pll->scale_m = sensor->scale_m;
 	pll->bits_per_pixel = sensor->csi_format->compressed;
 
-	rval = smiapp_pll_calculate(&client->dev, &lim, pll);
+	rval = smiapp_pll_try(sensor, pll);
 	if (rval < 0)
 		return rval;
 
-- 
1.7.10.4

[PATCH 14/17] smiapp: Gather information on valid link rate and BPP combinations

[Sakari Ailus]

Not all link rates are possible with all BPP values.

Also rearrange other initialisation a little. Obtaining possible PLL
configurations earlier requires that.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   73 ++++++++++++++++++++++++--------
 drivers/media/i2c/smiapp/smiapp.h      |    8 ++++
 2 files changed, 64 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index a1244e6..b108b15 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -742,6 +742,7 @@ static int smiapp_get_limits_binning(struct smiapp_sensor *sensor)
 static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->src->sd);
+	struct smiapp_pll *pll = &sensor->pll;
 	unsigned int type, n;
 	unsigned int i, pixel_order;
 	int rval;
@@ -816,6 +817,45 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 		}
 	}
 
+	/* Figure out which BPP values can be used with which formats. */
+	pll->binning_horizontal = 1;
+	pll->binning_vertical = 1;
+	pll->scale_m = sensor->scale_m;
+
+	for (i = 0; i < ARRAY_SIZE(smiapp_csi_data_formats); i++) {
+		const struct smiapp_csi_data_format *f =
+			&smiapp_csi_data_formats[i];
+		unsigned int j;
+
+		BUG_ON(f->compressed < SMIAPP_COMPRESSED_BASE);
+		BUG_ON(f->compressed > SMIAPP_COMPRESSED_MAX);
+
+		if (!(sensor->default_mbus_frame_fmts & (1 << i)))
+			continue;
+
+		/* Did we already gather information for this BPP? */
+		if (sensor->valid_link_freqs[f->compressed
+					     - SMIAPP_COMPRESSED_BASE])
+			continue;
+
+		pll->bits_per_pixel = f->compressed;
+
+		for (j = 0; j < sensor->platform_data->op_sys_clock[j]; j++) {
+			pll->link_freq = sensor->platform_data->op_sys_clock[j];
+
+			rval = smiapp_pll_try(sensor, pll);
+			dev_info(&client->dev, "link freq %u Hz, bpp %u %s\n",
+				 pll->link_freq, pll->bits_per_pixel,
+				 rval ? "not ok" : "ok");
+			if (rval)
+				continue;
+
+			set_bit(j, &sensor->valid_link_freqs[
+					f->compressed
+					- SMIAPP_COMPRESSED_BASE]);
+		}
+	}
+
 	if (!sensor->csi_format) {
 		dev_err(&client->dev, "no supported mbus code found\n");
 		return -EINVAL;
@@ -2479,12 +2519,6 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 		goto out_power_off;
 	}
 
-	rval = smiapp_get_mbus_formats(sensor);
-	if (rval) {
-		rval = -ENODEV;
-		goto out_power_off;
-	}
-
 	if (sensor->limits[SMIAPP_LIMIT_BINNING_CAPABILITY]) {
 		u32 val;
 
@@ -2566,6 +2600,22 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 
 	sensor->scale_m = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
 
+	/* prepare PLL configuration input values */
+	pll->bus_type = SMIAPP_PLL_BUS_TYPE_CSI2;
+	pll->csi2.lanes = sensor->platform_data->lanes;
+	pll->ext_clk_freq_hz = sensor->platform_data->ext_clk;
+	pll->flags = smiapp_call_quirk(sensor, pll_flags);
+	pll->scale_n = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
+	/* Profile 0 sensors have no separate OP clock branch. */
+	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
+		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
+
+	rval = smiapp_get_mbus_formats(sensor);
+	if (rval) {
+		rval = -ENODEV;
+		goto out_cleanup;
+	}
+
 	for (i = 0; i < SMIAPP_SUBDEVS; i++) {
 		struct {
 			struct smiapp_subdev *ssd;
@@ -2663,17 +2713,6 @@ static int smiapp_registered(struct v4l2_subdev *subdev)
 	if (rval < 0)
 		goto out_nvm_release;
 
-	/* prepare PLL configuration input values */
-	pll->bus_type = SMIAPP_PLL_BUS_TYPE_CSI2;
-	pll->csi2.lanes = sensor->platform_data->lanes;
-	pll->ext_clk_freq_hz = sensor->platform_data->ext_clk;
-	pll->flags = smiapp_call_quirk(sensor, pll_flags);
-
-	/* Profile 0 sensors have no separate OP clock branch. */
-	if (sensor->minfo.smiapp_profile == SMIAPP_PROFILE_0)
-		pll->flags |= SMIAPP_PLL_FLAG_NO_OP_CLOCKS;
-	pll->scale_n = sensor->limits[SMIAPP_LIMIT_SCALER_N_MIN];
-
 	mutex_lock(&sensor->mutex);
 	rval = smiapp_update_mode(sensor);
 	mutex_unlock(&sensor->mutex);
diff --git a/drivers/media/i2c/smiapp/smiapp.h b/drivers/media/i2c/smiapp/smiapp.h
index 874b49f..f88f8ec 100644
--- a/drivers/media/i2c/smiapp/smiapp.h
+++ b/drivers/media/i2c/smiapp/smiapp.h
@@ -156,6 +156,11 @@ struct smiapp_csi_data_format {
 #define SMIAPP_PAD_SRC			1
 #define SMIAPP_PADS			2
 
+#define SMIAPP_COMPRESSED_BASE		8
+#define SMIAPP_COMPRESSED_MAX		12
+#define SMIAPP_NR_OF_COMPRESSED		(SMIAPP_COMPRESSED_MAX - \
+					 SMIAPP_COMPRESSED_BASE + 1)
+
 struct smiapp_binning_subtype {
 	u8 horizontal:4;
 	u8 vertical:4;
@@ -232,6 +237,9 @@ struct smiapp_sensor {
 
 	struct smiapp_pll pll;
 
+	/* Is a default format supported for a given BPP? */
+	unsigned long valid_link_freqs[SMIAPP_NR_OF_COMPRESSED];
+
 	/* Pixel array controls */
 	struct v4l2_ctrl *analog_gain;
 	struct v4l2_ctrl *exposure;
-- 
1.7.10.4

[PATCH 15/17] smiapp: Take valid link frequencies into account in supported mbus codes

[Sakari Ailus]

Some media bus codes may be unavailable depending on the available media bus
codes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index b108b15..798c129 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -806,14 +806,6 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 			dev_dbg(&client->dev, "jolly good! %d\n", j);
 
 			sensor->default_mbus_frame_fmts |= 1 << j;
-			if (!sensor->csi_format
-			    || f->width > sensor->csi_format->width
-			    || (f->width == sensor->csi_format->width
-				&& f->compressed
-				> sensor->csi_format->compressed)) {
-				sensor->csi_format = f;
-				sensor->internal_csi_format = f;
-			}
 		}
 	}
 
@@ -854,6 +846,22 @@ static int smiapp_get_mbus_formats(struct smiapp_sensor *sensor)
 					f->compressed
 					- SMIAPP_COMPRESSED_BASE]);
 		}
+		if (!sensor->valid_link_freqs[f->compressed
+					      - SMIAPP_COMPRESSED_BASE]) {
+			dev_info(&client->dev,
+				 "no valid link frequencies for %u bpp\n",
+				 f->compressed);
+			sensor->default_mbus_frame_fmts &= ~BIT(i);
+			continue;
+		}
+
+		if (!sensor->csi_format
+		    || f->width > sensor->csi_format->width
+		    || (f->width == sensor->csi_format->width
+			&& f->compressed > sensor->csi_format->compressed)) {
+			sensor->csi_format = f;
+			sensor->internal_csi_format = f;
+		}
 	}
 
 	if (!sensor->csi_format) {
-- 
1.7.10.4

[PATCH 16/17] smiapp: Update PLL when setting format

[Sakari Ailus]

The media bus format BPP does affect PLL. Recalculate PLL if the format
changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 798c129..537ca92 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -1758,8 +1758,18 @@ static int smiapp_set_format(struct v4l2_subdev *subdev,
 
 			if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 				if (csi_format->width !=
-				    sensor->csi_format->width)
+				    sensor->csi_format->width) {
+					int rval;
+
+					rval = smiapp_pll_update(sensor);
+					if (rval) {
+						mutex_unlock(&sensor->mutex);
+						return rval;
+					}
+
 					range_changed = true;
+				}
+
 
 				sensor->csi_format = csi_format;
 			}
-- 
1.7.10.4

[PATCH 17/17] smiapp: Decrease link frequency if media bus pixel format BPP requires

[Sakari Ailus]

Decrease the link frequency to the next lower if the user chooses a media
bus code (BPP) cannot be achieved using the selected link frequency.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c |   20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 537ca92..ce2c34d 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor *sensor)
 
 	pll->binning_horizontal = sensor->binning_horizontal;
 	pll->binning_vertical = sensor->binning_vertical;
-	pll->link_freq =
-		sensor->link_freq->qmenu_int[sensor->link_freq->val];
 	pll->scale_m = sensor->scale_m;
 	pll->bits_per_pixel = sensor->csi_format->compressed;
 
+	if (!test_bit(sensor->link_freq->val,
+		      &sensor->valid_link_freqs[
+			      sensor->csi_format->compressed
+			      - SMIAPP_COMPRESSED_BASE])) {
+		/*
+		 * Setting the link frequency will perform PLL
+		 * re-calculation already, so skip that.
+		 */
+		return __v4l2_ctrl_s_ctrl(
+			sensor->link_freq,
+			__ffs(sensor->valid_link_freqs[
+				      sensor->csi_format->compressed
+				      - SMIAPP_COMPRESSED_BASE]));
+	}
+
+	pll->link_freq =
+		sensor->link_freq->qmenu_int[sensor->link_freq->val];
+
 	rval = smiapp_pll_try(sensor, pll);
 	if (rval < 0)
 		return rval;
-- 
1.7.10.4

Re: [PATCH 17/17] smiapp: Decrease link frequency if media bus pixel format BPP requires

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Wednesday 17 September 2014 23:45:41 Sakari Ailus wrote:
> Decrease the link frequency to the next lower if the user chooses a media
> bus code (BPP) cannot be achieved using the selected link frequency.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c |   20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor
> *sensor)
> 
>  	pll->binning_horizontal = sensor->binning_horizontal;
>  	pll->binning_vertical = sensor->binning_vertical;
> -	pll->link_freq =
> -		sensor->link_freq->qmenu_int[sensor->link_freq->val];
>  	pll->scale_m = sensor->scale_m;
>  	pll->bits_per_pixel = sensor->csi_format->compressed;
> 
> +	if (!test_bit(sensor->link_freq->val,
> +		      &sensor->valid_link_freqs[
> +			      sensor->csi_format->compressed
> +			      - SMIAPP_COMPRESSED_BASE])) {
> +		/*
> +		 * Setting the link frequency will perform PLL
> +		 * re-calculation already, so skip that.
> +		 */
> +		return __v4l2_ctrl_s_ctrl(
> +			sensor->link_freq,
> +			__ffs(sensor->valid_link_freqs[
> +				      sensor->csi_format->compressed
> +				      - SMIAPP_COMPRESSED_BASE]));

I have an uneasy feeling about this, as smiapp_pll_update is called from the 
link freq s_ctrl handler. Have you double-checked the recursion bounds ?

> +	}
> +
> +	pll->link_freq =
> +		sensor->link_freq->qmenu_int[sensor->link_freq->val];
> +
>  	rval = smiapp_pll_try(sensor, pll);
>  	if (rval < 0)
>  		return rval;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 17/17] smiapp: Decrease link frequency if media bus pixel format BPP requires

[Sakari Ailus]

Hi Laurent,

Thank you for your comments.

On Fri, Sep 26, 2014 at 01:44:03PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 17 September 2014 23:45:41 Sakari Ailus wrote:
> > Decrease the link frequency to the next lower if the user chooses a media
> > bus code (BPP) cannot be achieved using the selected link frequency.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/smiapp/smiapp-core.c |   20 ++++++++++++++++++--
> >  1 file changed, 18 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/smiapp/smiapp-core.c
> > b/drivers/media/i2c/smiapp/smiapp-core.c index 537ca92..ce2c34d 100644
> > --- a/drivers/media/i2c/smiapp/smiapp-core.c
> > +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> > @@ -286,11 +286,27 @@ static int smiapp_pll_update(struct smiapp_sensor
> > *sensor)
> > 
> >  	pll->binning_horizontal = sensor->binning_horizontal;
> >  	pll->binning_vertical = sensor->binning_vertical;
> > -	pll->link_freq =
> > -		sensor->link_freq->qmenu_int[sensor->link_freq->val];
> >  	pll->scale_m = sensor->scale_m;
> >  	pll->bits_per_pixel = sensor->csi_format->compressed;
> > 
> > +	if (!test_bit(sensor->link_freq->val,
> > +		      &sensor->valid_link_freqs[
> > +			      sensor->csi_format->compressed
> > +			      - SMIAPP_COMPRESSED_BASE])) {
> > +		/*
> > +		 * Setting the link frequency will perform PLL
> > +		 * re-calculation already, so skip that.
> > +		 */
> > +		return __v4l2_ctrl_s_ctrl(
> > +			sensor->link_freq,
> > +			__ffs(sensor->valid_link_freqs[
> > +				      sensor->csi_format->compressed
> > +				      - SMIAPP_COMPRESSED_BASE]));
> 
> I have an uneasy feeling about this, as smiapp_pll_update is called from the 
> link freq s_ctrl handler. Have you double-checked the recursion bounds ?

We haven't actually done any PLL tree calculation yet here. The condition
will evaluate true in a case when the user chooses a format which isn't
available on a given link frequency, or chooses a link frequency which isn't
available for a given format.

The condition will be false the next time the function is called since we've
just chosen a valid combination of the two.

But now that you brought the topic up, I think the link frequency selection
should just probably return -EBUSY if the selected link frquency cannot be
used. Also __ffs() should be __fls() instead in order to still come up with
the highest link freqency.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk