[PATCH v3 0/14] media: make Marvell camera work on DT-based OLPC XO-1.75

[PATCH v3 0/14] media: make Marvell camera work on DT-based OLPC XO-1.75

[Lubomir Rintel]

Hi,

this is the third spin of a patchset that modernizes the Marvel MMP2 CCIC
driver. Notably, it ports it from the platform data (which seems unused as
the board support code never made it) to devicetree.

At the core of the rework is the move to asynchronous sensor discovery
and clock management with the standard clock framework. There are also
some straightforward fixes for the bitrotten parts.

There's probably still room for improvement, but as it is, it seems to
work well on OLPC XO-1.75 and doesn't break OLPC XO-1 (I've tested on
both platforms).

Changes since v2:
- All documented in individual patches.

Changes since v1:
- "marvell-ccic: drop ctlr_reset()" patch was replaced with a
  straightforward revert of the commit that added ctlr_reset() along with an
  explanation in commit message.
- Added collected Acks
- Other changes are noted in individial patches

Thanks,
Lubo

[PATCH v3 01/14] media: ov7670: split register setting from set_fmt() logic

[Lubomir Rintel]

This will allow us to restore the last set format after the device returns
from a power off.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- This patch was added to the series

 drivers/media/i2c/ov7670.c | 80 ++++++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 34 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index bc68a3a5b4ec..ee2302fbdeee 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -240,6 +240,7 @@ struct ov7670_info {
 	};
 	struct v4l2_mbus_framefmt format;
 	struct ov7670_format_struct *fmt;  /* Current format */
+	struct ov7670_win_size *wsize;
 	struct clk *clk;
 	struct gpio_desc *resetb_gpio;
 	struct gpio_desc *pwdn_gpio;
@@ -1003,48 +1004,20 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
 	return 0;
 }
 
-/*
- * Set a format.
- */
-static int ov7670_set_fmt(struct v4l2_subdev *sd,
-		struct v4l2_subdev_pad_config *cfg,
-		struct v4l2_subdev_format *format)
+static int ov7670_apply_fmt(struct v4l2_subdev *sd)
 {
-	struct ov7670_format_struct *ovfmt;
-	struct ov7670_win_size *wsize;
 	struct ov7670_info *info = to_state(sd);
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
-	struct v4l2_mbus_framefmt *mbus_fmt;
-#endif
+	struct ov7670_win_size *wsize = info->wsize;
 	unsigned char com7, com10 = 0;
 	int ret;
 
-	if (format->pad)
-		return -EINVAL;
-
-	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
-		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
-		if (ret)
-			return ret;
-#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
-		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
-		*mbus_fmt = format->format;
-		return 0;
-#else
-		return -ENOTTY;
-#endif
-	}
-
-	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);
-	if (ret)
-		return ret;
 	/*
 	 * COM7 is a pain in the ass, it doesn't like to be read then
 	 * quickly written afterward.  But we have everything we need
 	 * to set it absolutely here, as long as the format-specific
 	 * register sets list it first.
 	 */
-	com7 = ovfmt->regs[0].value;
+	com7 = info->fmt->regs[0].value;
 	com7 |= wsize->com7_bit;
 	ret = ov7670_write(sd, REG_COM7, com7);
 	if (ret)
@@ -1066,7 +1039,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 	/*
 	 * Now write the rest of the array.  Also store start/stops
 	 */
-	ret = ov7670_write_array(sd, ovfmt->regs + 1);
+	ret = ov7670_write_array(sd, info->fmt->regs + 1);
 	if (ret)
 		return ret;
 
@@ -1081,8 +1054,6 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 			return ret;
 	}
 
-	info->fmt = ovfmt;
-
 	/*
 	 * If we're running RGB565, we must rewrite clkrc after setting
 	 * the other parameters or the image looks poor.  If we're *not*
@@ -1100,6 +1071,46 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/*
+ * Set a format.
+ */
+static int ov7670_set_fmt(struct v4l2_subdev *sd,
+		struct v4l2_subdev_pad_config *cfg,
+		struct v4l2_subdev_format *format)
+{
+	struct ov7670_info *info = to_state(sd);
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+	struct v4l2_mbus_framefmt *mbus_fmt;
+#endif
+	int ret;
+
+	if (format->pad)
+		return -EINVAL;
+
+	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
+		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
+		if (ret)
+			return ret;
+#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
+		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
+		*mbus_fmt = format->format;
+		return 0;
+#else
+		return -ENOTTY;
+#endif
+	}
+
+	ret = ov7670_try_fmt_internal(sd, &format->format, &info->fmt, &info->wsize);
+	if (ret)
+		return ret;
+
+	ret = ov7670_apply_fmt(sd);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int ov7670_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_pad_config *cfg,
 			  struct v4l2_subdev_format *format)
@@ -1847,6 +1858,7 @@ static int ov7670_probe(struct i2c_client *client,
 
 	info->devtype = &ov7670_devdata[id->driver_data];
 	info->fmt = &ov7670_formats[0];
+	info->wsize = &info->devtype->win_sizes[0];
 
 	ov7670_get_default_format(sd, &info->format);
 
-- 
2.19.1

[PATCH v3 02/14] media: ov7670: split register setting from set_framerate() logic

[Lubomir Rintel]

This will allow us to restore the last set frame rate after the device
returns from a power off.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- This patch was added to the series

 drivers/media/i2c/ov7670.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index ee2302fbdeee..ead0c360df33 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -810,13 +810,24 @@ static void ov7675_get_framerate(struct v4l2_subdev *sd,
 			(4 * clkrc);
 }
 
+static int ov7675_apply_framerate(struct v4l2_subdev *sd)
+{
+	struct ov7670_info *info = to_state(sd);
+	int ret;
+
+	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
+	if (ret < 0)
+		return ret;
+
+	return ov7670_write(sd, REG_DBLV, info->pll_bypass ? DBLV_BYPASS : DBLV_X4);
+}
+
 static int ov7675_set_framerate(struct v4l2_subdev *sd,
 				 struct v4l2_fract *tpf)
 {
 	struct ov7670_info *info = to_state(sd);
 	u32 clkrc;
 	int pll_factor;
-	int ret;
 
 	/*
 	 * The formula is fps = 5/4*pixclk for YUV/RGB and
@@ -825,19 +836,10 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
 	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
 	 *
 	 */
-	if (info->pll_bypass) {
-		pll_factor = 1;
-		ret = ov7670_write(sd, REG_DBLV, DBLV_BYPASS);
-	} else {
-		pll_factor = PLL_FACTOR;
-		ret = ov7670_write(sd, REG_DBLV, DBLV_X4);
-	}
-	if (ret < 0)
-		return ret;
-
 	if (tpf->numerator == 0 || tpf->denominator == 0) {
 		clkrc = 0;
 	} else {
+		pll_factor = info->pll_bypass ? 1 : PLL_FACTOR;
 		clkrc = (5 * pll_factor * info->clock_speed * tpf->numerator) /
 			(4 * tpf->denominator);
 		if (info->fmt->mbus_code == MEDIA_BUS_FMT_SBGGR8_1X8)
@@ -859,11 +861,7 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
 	/* Recalculate frame rate */
 	ov7675_get_framerate(sd, tpf);
 
-	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
-	if (ret < 0)
-		return ret;
-
-	return ov7670_write(sd, REG_DBLV, DBLV_X4);
+	return ov7675_apply_framerate(sd);
 }
 
 static void ov7670_get_framerate_legacy(struct v4l2_subdev *sd,
-- 
2.19.1

[PATCH v3 03/14] media: ov7670: hook s_power onto v4l2 core

[Lubomir Rintel]

The commit 71862f63f351 ("media: ov7670: Add the ov7670_s_power function")
added a power control routing. However, it was not good enough to use as
a s_power() callback: it merely flipped on the power GPIOs without
restoring the register settings.

Fix this now and register an actual power callback.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- Restore the controls, format and frame rate on power on

 drivers/media/i2c/ov7670.c | 50 +++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index ead0c360df33..cbaab60aaaac 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -242,6 +242,7 @@ struct ov7670_info {
 	struct ov7670_format_struct *fmt;  /* Current format */
 	struct ov7670_win_size *wsize;
 	struct clk *clk;
+	int on;
 	struct gpio_desc *resetb_gpio;
 	struct gpio_desc *pwdn_gpio;
 	unsigned int mbus_config;	/* Media bus configuration flags */
@@ -1615,19 +1616,54 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
 }
 #endif
 
-static int ov7670_s_power(struct v4l2_subdev *sd, int on)
+static void ov7670_power_on(struct v4l2_subdev *sd)
 {
 	struct ov7670_info *info = to_state(sd);
 
+	if (info->on)
+		return;
+
 	if (info->pwdn_gpio)
-		gpiod_set_value(info->pwdn_gpio, !on);
-	if (on && info->resetb_gpio) {
+		gpiod_set_value(info->pwdn_gpio, 0);
+	if (info->resetb_gpio) {
 		gpiod_set_value(info->resetb_gpio, 1);
 		usleep_range(500, 1000);
 		gpiod_set_value(info->resetb_gpio, 0);
 		usleep_range(3000, 5000);
 	}
 
+	info->on = true;
+}
+
+static void ov7670_power_off(struct v4l2_subdev *sd)
+{
+	struct ov7670_info *info = to_state(sd);
+
+	if (!info->on)
+		return;
+
+	if (info->pwdn_gpio)
+		gpiod_set_value(info->pwdn_gpio, 1);
+
+	info->on = false;
+}
+
+static int ov7670_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct ov7670_info *info = to_state(sd);
+
+	if (info->on == on)
+		return 0;
+
+	if (on) {
+		ov7670_power_on (sd);
+		ov7670_apply_fmt(sd);
+		ov7675_apply_framerate(sd);
+		v4l2_ctrl_handler_setup(&info->hdl);
+	} else {
+		ov7670_power_off (sd);
+	}
+
 	return 0;
 }
 
@@ -1660,6 +1696,7 @@ static int ov7670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 static const struct v4l2_subdev_core_ops ov7670_core_ops = {
 	.reset = ov7670_reset,
 	.init = ov7670_init,
+	.s_power = ov7670_s_power,
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	.g_register = ov7670_g_register,
 	.s_register = ov7670_s_register,
@@ -1825,6 +1862,7 @@ static int ov7670_probe(struct i2c_client *client,
 		else
 			return ret;
 	}
+
 	if (info->clk) {
 		ret = clk_prepare_enable(info->clk);
 		if (ret)
@@ -1841,7 +1879,7 @@ static int ov7670_probe(struct i2c_client *client,
 	if (ret)
 		goto clk_disable;
 
-	ov7670_s_power(sd, 1);
+	ov7670_power_on(sd);
 
 	/* Make sure it's an ov7670 */
 	ret = ov7670_detect(sd);
@@ -1929,7 +1967,7 @@ static int ov7670_probe(struct i2c_client *client,
 hdl_free:
 	v4l2_ctrl_handler_free(&info->hdl);
 power_off:
-	ov7670_s_power(sd, 0);
+	ov7670_power_off(sd);
 clk_disable:
 	clk_disable_unprepare(info->clk);
 	return ret;
@@ -1945,7 +1983,7 @@ static int ov7670_remove(struct i2c_client *client)
 	v4l2_ctrl_handler_free(&info->hdl);
 	clk_disable_unprepare(info->clk);
 	media_entity_cleanup(&info->sd.entity);
-	ov7670_s_power(sd, 0);
+	ov7670_power_off(sd);
 	return 0;
 }
 
-- 
2.19.1

[PATCH v3 04/14] media: ov7670: control clock along with power

[Lubomir Rintel]

This provides more power saving when the sensor is off.

While at that, do the delay on power/clock enable even if the sensor driver
itself doesn't control the GPIOs. This is required for the OLPC XO-1
platform, that lacks the proper power/reset properties in its DT, but
needs the delay after the sensor is clocked up.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/media/i2c/ov7670.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
index cbaab60aaaac..d7635fb2d713 100644
--- a/drivers/media/i2c/ov7670.c
+++ b/drivers/media/i2c/ov7670.c
@@ -1623,14 +1623,17 @@ static void ov7670_power_on(struct v4l2_subdev *sd)
 	if (info->on)
 		return;
 
+	clk_prepare_enable(info->clk);
+
 	if (info->pwdn_gpio)
 		gpiod_set_value(info->pwdn_gpio, 0);
 	if (info->resetb_gpio) {
 		gpiod_set_value(info->resetb_gpio, 1);
 		usleep_range(500, 1000);
 		gpiod_set_value(info->resetb_gpio, 0);
-		usleep_range(3000, 5000);
 	}
+	if (info->pwdn_gpio || info->resetb_gpio || info->clk)
+		usleep_range(3000, 5000);
 
 	info->on = true;
 }
@@ -1642,6 +1645,8 @@ static void ov7670_power_off(struct v4l2_subdev *sd)
 	if (!info->on)
 		return;
 
+	clk_disable_unprepare(info->clk);
+
 	if (info->pwdn_gpio)
 		gpiod_set_value(info->pwdn_gpio, 1);
 
@@ -1863,24 +1868,20 @@ static int ov7670_probe(struct i2c_client *client,
 			return ret;
 	}
 
-	if (info->clk) {
-		ret = clk_prepare_enable(info->clk);
-		if (ret)
-			return ret;
+	ret = ov7670_init_gpio(client, info);
+	if (ret)
+		return ret;
 
+	ov7670_power_on(sd);
+
+	if (info->clk) {
 		info->clock_speed = clk_get_rate(info->clk) / 1000000;
 		if (info->clock_speed < 10 || info->clock_speed > 48) {
 			ret = -EINVAL;
-			goto clk_disable;
+			goto power_off;
 		}
 	}
 
-	ret = ov7670_init_gpio(client, info);
-	if (ret)
-		goto clk_disable;
-
-	ov7670_power_on(sd);
-
 	/* Make sure it's an ov7670 */
 	ret = ov7670_detect(sd);
 	if (ret) {
@@ -1960,6 +1961,7 @@ static int ov7670_probe(struct i2c_client *client,
 	if (ret < 0)
 		goto entity_cleanup;
 
+	ov7670_power_off(sd);
 	return 0;
 
 entity_cleanup:
@@ -1968,12 +1970,9 @@ static int ov7670_probe(struct i2c_client *client,
 	v4l2_ctrl_handler_free(&info->hdl);
 power_off:
 	ov7670_power_off(sd);
-clk_disable:
-	clk_disable_unprepare(info->clk);
 	return ret;
 }
 
-
 static int ov7670_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
@@ -1981,7 +1980,6 @@ static int ov7670_remove(struct i2c_client *client)
 
 	v4l2_async_unregister_subdev(sd);
 	v4l2_ctrl_handler_free(&info->hdl);
-	clk_disable_unprepare(info->clk);
 	media_entity_cleanup(&info->sd.entity);
 	ov7670_power_off(sd);
 	return 0;
-- 
2.19.1

[PATCH v3 05/14] media: dt-bindings: marvell,mmp2-ccic: Add Marvell MMP2 camera

[Lubomir Rintel]

Add Marvell MMP2 camera host interface.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- Added #clock-cells, clock-names, port

 .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt

diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
new file mode 100644
index 000000000000..e5e8ca90e7f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
@@ -0,0 +1,37 @@
+Marvell MMP2 camera host interface
+
+Required properties:
+ - compatible: Should be "marvell,mmp2-ccic"
+ - reg: register base and size
+ - interrupts: the interrupt number
+ - #clock-cells: must be 0
+ - any required generic properties defined in video-interfaces.txt
+
+Optional properties:
+ - clocks: input clock (see clock-bindings.txt)
+ - clock-names: names of the clocks used, may include "axi", "func" and
+                "phy"
+ - clock-output-names: should contain the name of the clock driving the
+                       sensor master clock MCLK
+
+Required subnodes:
+ - port: the parallel bus interface port with a single endpoint linked to
+         the sensor's endpoint as described in video-interfaces.txt
+
+Example:
+
+	camera0: camera@d420a000 {
+		compatible = "marvell,mmp2-ccic";
+		reg = <0xd420a000 0x800>;
+		interrupts = <42>;
+		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
+		clock-names = "axi";
+		#clock-cells = <0>;
+		clock-output-names = "mclk";
+
+		port {
+			camera0_0: endpoint {
+				remote-endpoint = <&ov7670_0>;
+			};
+		};
+	};
-- 
2.19.1

[PATCH v3 06/14] [media] marvell-ccic: fix DMA s/g desc number calculation

[Lubomir Rintel]

The commit d790b7eda953 ("[media] vb2-dma-sg: move dma_(un)map_sg here")
left dma_desc_nent unset. It previously contained the number of DMA
descriptors as returned from dma_map_sg().

We can now (since the commit referred to above) obtain the same value from
the sg_table and drop dma_desc_nent altogether.

Tested on OLPC XO-1.75 machine. Doesn't affect the OLPC XO-1's Cafe
driver, since that one doesn't do DMA.

Fixes: d790b7eda953df474f470169ebdf111c02fa7a2d
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index f1b301810260..d97f39bde9bd 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -200,7 +200,6 @@ struct mcam_vb_buffer {
 	struct list_head queue;
 	struct mcam_dma_desc *dma_desc;	/* Descriptor virtual address */
 	dma_addr_t dma_desc_pa;		/* Descriptor physical address */
-	int dma_desc_nent;		/* Number of mapped descriptors */
 };
 
 static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_v4l2_buffer *vb)
@@ -608,9 +607,11 @@ static void mcam_dma_contig_done(struct mcam_camera *cam, int frame)
 static void mcam_sg_next_buffer(struct mcam_camera *cam)
 {
 	struct mcam_vb_buffer *buf;
+	struct sg_table *sg_table;
 
 	buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer, queue);
 	list_del_init(&buf->queue);
+	sg_table = vb2_dma_sg_plane_desc(&buf->vb_buf.vb2_buf, 0);
 	/*
 	 * Very Bad Not Good Things happen if you don't clear
 	 * C1_DESC_ENA before making any descriptor changes.
@@ -618,7 +619,7 @@ static void mcam_sg_next_buffer(struct mcam_camera *cam)
 	mcam_reg_clear_bit(cam, REG_CTRL1, C1_DESC_ENA);
 	mcam_reg_write(cam, REG_DMA_DESC_Y, buf->dma_desc_pa);
 	mcam_reg_write(cam, REG_DESC_LEN_Y,
-			buf->dma_desc_nent*sizeof(struct mcam_dma_desc));
+			sg_table->nents*sizeof(struct mcam_dma_desc));
 	mcam_reg_write(cam, REG_DESC_LEN_U, 0);
 	mcam_reg_write(cam, REG_DESC_LEN_V, 0);
 	mcam_reg_set_bit(cam, REG_CTRL1, C1_DESC_ENA);
-- 
2.19.1

[PATCH v3 07/14] [media] marvell-ccic: don't generate EOF on parallel bus

[Lubomir Rintel]

The commit 05fed81625bf ("[media] marvell-ccic: add MIPI support for
marvell-ccic driver") that claimed to add CSI2 turned on C0_EOF_VSYNC for
parallel bus without a very good explanation.

That broke camera on OLPC XO-1.75 which precisely uses a sensor on a
parallel bus. Revert that chunk.

Tested on an OLPC XO-1.75.

Fixes: 05fed81625bf755cc67c5864cdfd18b69ea828d1
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index d97f39bde9bd..d24e5b7a3bc5 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -792,12 +792,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 	 * Make sure it knows we want to use hsync/vsync.
 	 */
 	mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC, C0_SIFM_MASK);
-	/*
-	 * This field controls the generation of EOF(DVP only)
-	 */
-	if (cam->bus_type != V4L2_MBUS_CSI2_DPHY)
-		mcam_reg_set_bit(cam, REG_CTRL0,
-				C0_EOF_VSYNC | C0_VEDGE_CTRL);
 }
 
 
-- 
2.19.1

[PATCH v3 08/14] Revert "[media] marvell-ccic: reset ccic phy when stop streaming for stability"

[Lubomir Rintel]

This accesses the clock registers directly and thus is going to stay in the
way of making the driver devicetree friendly.

No boards seems to actually use this. If it's somehow actually needed it
needs to be done differently.

This reverts commit 7c269f454e7a51b151d94f99344120efa1cd0acb.
---
 .../media/platform/marvell-ccic/mcam-core.c   |  6 -----
 .../media/platform/marvell-ccic/mcam-core.h   |  2 --
 .../media/platform/marvell-ccic/mmp-driver.c  | 25 -------------------
 3 files changed, 33 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index d24e5b7a3bc5..1b879035948c 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1154,12 +1154,6 @@ static void mcam_vb_stop_streaming(struct vb2_queue *vq)
 	if (cam->state != S_STREAMING)
 		return;
 	mcam_ctlr_stop_dma(cam);
-	/*
-	 * Reset the CCIC PHY after stopping streaming,
-	 * otherwise, the CCIC may be unstable.
-	 */
-	if (cam->ctlr_reset)
-		cam->ctlr_reset(cam);
 	/*
 	 * VB2 reclaims the buffers, so we need to forget
 	 * about them.
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index ad8955f9f0a1..a3a097a45e78 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -116,7 +116,6 @@ struct mcam_camera {
 	int mclk_src;	/* which clock source the mclk derives from */
 	int mclk_div;	/* Clock Divider Value for MCLK */
 
-	int ccic_id;
 	enum v4l2_mbus_type bus_type;
 	/* MIPI support */
 	/* The dphy config value, allocated in board file
@@ -137,7 +136,6 @@ struct mcam_camera {
 	int (*plat_power_up) (struct mcam_camera *cam);
 	void (*plat_power_down) (struct mcam_camera *cam);
 	void (*calc_dphy) (struct mcam_camera *cam);
-	void (*ctlr_reset) (struct mcam_camera *cam);
 
 	/*
 	 * Everything below here is private to the mcam core and
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 70a2833db0d1..dbfc597b955d 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -105,7 +105,6 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev)
 #define CPU_SUBSYS_PMU_BASE	0xd4282800
 #define REG_CCIC_DCGCR		0x28	/* CCIC dyn clock gate ctrl reg */
 #define REG_CCIC_CRCR		0x50	/* CCIC clk reset ctrl reg	*/
-#define REG_CCIC2_CRCR		0xf4	/* CCIC2 clk reset ctrl reg	*/
 
 static void mcam_clk_enable(struct mcam_camera *mcam)
 {
@@ -183,28 +182,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
 	mcam_clk_disable(mcam);
 }
 
-static void mcam_ctlr_reset(struct mcam_camera *mcam)
-{
-	unsigned long val;
-	struct mmp_camera *cam = mcam_to_cam(mcam);
-
-	if (mcam->ccic_id) {
-		/*
-		 * Using CCIC2
-		 */
-		val = ioread32(cam->power_regs + REG_CCIC2_CRCR);
-		iowrite32(val & ~0x2, cam->power_regs + REG_CCIC2_CRCR);
-		iowrite32(val | 0x2, cam->power_regs + REG_CCIC2_CRCR);
-	} else {
-		/*
-		 * Using CCIC1
-		 */
-		val = ioread32(cam->power_regs + REG_CCIC_CRCR);
-		iowrite32(val & ~0x2, cam->power_regs + REG_CCIC_CRCR);
-		iowrite32(val | 0x2, cam->power_regs + REG_CCIC_CRCR);
-	}
-}
-
 /*
  * calc the dphy register values
  * There are three dphy registers being used.
@@ -352,11 +329,9 @@ static int mmpcam_probe(struct platform_device *pdev)
 	mcam = &cam->mcam;
 	mcam->plat_power_up = mmpcam_power_up;
 	mcam->plat_power_down = mmpcam_power_down;
-	mcam->ctlr_reset = mcam_ctlr_reset;
 	mcam->calc_dphy = mmpcam_calc_dphy;
 	mcam->dev = &pdev->dev;
 	mcam->use_smbus = 0;
-	mcam->ccic_id = pdev->id;
 	mcam->mclk_min = pdata->mclk_min;
 	mcam->mclk_src = pdata->mclk_src;
 	mcam->mclk_div = pdata->mclk_div;
-- 
2.19.1

[PATCH v3 09/14] [media] marvell-ccic: drop unused stuff

[Lubomir Rintel]

Remove structure members and headers that are not actually used. Saves
us from some noise in subsequent cleanup commits.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/platform/marvell-ccic/mcam-core.c  | 1 -
 drivers/media/platform/marvell-ccic/mcam-core.h  | 2 --
 drivers/media/platform/marvell-ccic/mmp-driver.c | 2 --
 include/linux/platform_data/media/mmp-camera.h   | 1 -
 4 files changed, 6 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 1b879035948c..0113b8d37d03 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1776,7 +1776,6 @@ int mccic_register(struct mcam_camera *cam)
 	 */
 	sensor_cfg.clock_speed = cam->clock_speed;
 	sensor_cfg.use_smbus = cam->use_smbus;
-	cam->sensor_addr = ov7670_info.addr;
 	cam->sensor = v4l2_i2c_new_subdev_board(&cam->v4l2_dev,
 			cam->i2c_adapter, &ov7670_info, NULL);
 	if (cam->sensor == NULL) {
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index a3a097a45e78..b828b1bb59d3 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -112,7 +112,6 @@ struct mcam_camera {
 	short int use_smbus;	/* SMBUS or straight I2c? */
 	enum mcam_buffer_mode buffer_mode;
 
-	int mclk_min;	/* The minimal value of mclk */
 	int mclk_src;	/* which clock source the mclk derives from */
 	int mclk_div;	/* Clock Divider Value for MCLK */
 
@@ -152,7 +151,6 @@ struct mcam_camera {
 	 */
 	struct video_device vdev;
 	struct v4l2_subdev *sensor;
-	unsigned short sensor_addr;
 
 	/* Videobuf2 stuff */
 	struct vb2_queue vb_queue;
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index dbfc597b955d..f2e43b23af18 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -12,7 +12,6 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
-#include <linux/platform_data/i2c-gpio.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -332,7 +331,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	mcam->calc_dphy = mmpcam_calc_dphy;
 	mcam->dev = &pdev->dev;
 	mcam->use_smbus = 0;
-	mcam->mclk_min = pdata->mclk_min;
 	mcam->mclk_src = pdata->mclk_src;
 	mcam->mclk_div = pdata->mclk_div;
 	mcam->bus_type = pdata->bus_type;
diff --git a/include/linux/platform_data/media/mmp-camera.h b/include/linux/platform_data/media/mmp-camera.h
index d2d3a443eedf..4c3a80a45883 100644
--- a/include/linux/platform_data/media/mmp-camera.h
+++ b/include/linux/platform_data/media/mmp-camera.h
@@ -16,7 +16,6 @@ struct mmp_camera_platform_data {
 	int sensor_power_gpio;
 	int sensor_reset_gpio;
 	enum v4l2_mbus_type bus_type;
-	int mclk_min;	/* The minimal value of MCLK */
 	int mclk_src;	/* which clock source the MCLK derives from */
 	int mclk_div;	/* Clock Divider Value for MCLK */
 	/*
-- 
2.19.1

[PATCH v3 10/14] [media] marvell-ccic/mmp: enable clock before accessing registers

[Lubomir Rintel]

The access to REG_CLKCTRL or REG_CTRL1 without the clock enabled hangs
the machine. Enable the clock first.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/platform/marvell-ccic/mmp-driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index f2e43b23af18..05cba74c0d13 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -144,6 +144,7 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
  * Turn on power and clocks to the controller.
  */
 	mmpcam_power_up_ctlr(cam);
+	mcam_clk_enable(mcam);
 /*
  * Provide power to the sensor.
  */
@@ -157,8 +158,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
 	gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
 	mdelay(5);
 
-	mcam_clk_enable(mcam);
-
 	return 0;
 }
 
-- 
2.19.1

[PATCH v3 11/14] [media] marvell-ccic: rename the clocks

[Lubomir Rintel]

Use the names more suitable for devicetree bindings.

There are no board files utilizing this, thus we seem to be at liberty
at renaming this without consequences.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- This patch was added to the series

 drivers/media/platform/marvell-ccic/mmp-driver.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 05cba74c0d13..5c61317f56bc 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -33,7 +33,7 @@ MODULE_ALIAS("platform:mmp-camera");
 MODULE_AUTHOR("Jonathan Corbet <corbet@lwn.net>");
 MODULE_LICENSE("GPL");
 
-static char *mcam_clks[] = {"CCICAXICLK", "CCICFUNCLK", "CCICPHYCLK"};
+static char *mcam_clks[] = {"axi", "func", "phy"};
 
 struct mmp_camera {
 	void __iomem *power_regs;
-- 
2.19.1

[PATCH v3 12/14] [media] marvell-ccic/mmp: add devicetree support

[Lubomir Rintel]

The platform data is actually not used anywhere (along with the CSI
support) and should be safe to remove.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v1:
- s/This are/These are/ in a comment

 .../media/platform/marvell-ccic/mmp-driver.c  | 36 ++++++++++++++-----
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 5c61317f56bc..05b9fa8c6a6f 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -19,6 +19,8 @@
 #include <media/v4l2-device.h>
 #include <linux/platform_data/media/mmp-camera.h>
 #include <linux/device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
 #include <linux/io.h>
@@ -196,6 +198,9 @@ static void mmpcam_calc_dphy(struct mcam_camera *mcam)
 	struct device *dev = &cam->pdev->dev;
 	unsigned long tx_clk_esc;
 
+	if (!pdata)
+		return;
+
 	/*
 	 * If CSI2_DPHY3 is calculated dynamically,
 	 * pdata->lane_clk should be already set
@@ -314,10 +319,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	struct mmp_camera_platform_data *pdata;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata)
-		return -ENODEV;
-
 	cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
 	if (cam == NULL)
 		return -ENOMEM;
@@ -330,17 +331,29 @@ static int mmpcam_probe(struct platform_device *pdev)
 	mcam->calc_dphy = mmpcam_calc_dphy;
 	mcam->dev = &pdev->dev;
 	mcam->use_smbus = 0;
-	mcam->mclk_src = pdata->mclk_src;
-	mcam->mclk_div = pdata->mclk_div;
-	mcam->bus_type = pdata->bus_type;
-	mcam->dphy = pdata->dphy;
+	pdata = pdev->dev.platform_data;
+	if (pdata) {
+		mcam->mclk_src = pdata->mclk_src;
+		mcam->mclk_div = pdata->mclk_div;
+		mcam->bus_type = pdata->bus_type;
+		mcam->dphy = pdata->dphy;
+		mcam->lane = pdata->lane;
+	} else {
+		/*
+		 * These are values that used to be hardcoded in mcam-core and
+		 * work well on a OLPC XO 1.75 with a parallel bus sensor.
+		 * If it turns out other setups make sense, the values should
+		 * be obtained from the device tree.
+		 */
+		mcam->mclk_src = 3;
+		mcam->mclk_div = 2;
+	}
 	if (mcam->bus_type == V4L2_MBUS_CSI2_DPHY) {
 		cam->mipi_clk = devm_clk_get(mcam->dev, "mipi");
 		if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0))
 			return PTR_ERR(cam->mipi_clk);
 	}
 	mcam->mipi_enabled = false;
-	mcam->lane = pdata->lane;
 	mcam->chip_id = MCAM_ARMADA610;
 	mcam->buffer_mode = B_DMA_sg;
 	strscpy(mcam->bus_info, "platform:mmp-camera", sizeof(mcam->bus_info));
@@ -475,6 +488,10 @@ static int mmpcam_resume(struct platform_device *pdev)
 
 #endif
 
+static const struct of_device_id mmpcam_of_match[] = {
+	{ .compatible = "marvell,mmp2-ccic", },
+	{},
+};
 
 static struct platform_driver mmpcam_driver = {
 	.probe		= mmpcam_probe,
@@ -485,6 +502,7 @@ static struct platform_driver mmpcam_driver = {
 #endif
 	.driver = {
 		.name	= "mmp-camera",
+		.of_match_table = of_match_ptr(mmpcam_of_match),
 	}
 };
 
-- 
2.19.1

[PATCH v3 13/14] [media] marvell-ccic: use async notifier to get the sensor

[Lubomir Rintel]

An instance of a sensor on DT-based MMP2 platform is always going to be
created asynchronously.

Let's move the manual device creation away from the core to the Cafe
driver (used on OLPC XO-1, not present in DT) and set up appropriate
async matches: I2C on Cafe, FWNODE on MMP (OLPC XO-1.75).

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v2:
- Moved a typo fix hunk that was accidentally in the following patch
  here, to unbreak build.

 .../media/platform/marvell-ccic/cafe-driver.c |  49 ++++--
 .../media/platform/marvell-ccic/mcam-core.c   | 157 ++++++++++++------
 .../media/platform/marvell-ccic/mcam-core.h   |   5 +-
 .../media/platform/marvell-ccic/mmp-driver.c  |  27 +--
 .../linux/platform_data/media/mmp-camera.h    |   1 -
 5 files changed, 162 insertions(+), 77 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c
index 2986cb4b88d0..658294d319c0 100644
--- a/drivers/media/platform/marvell-ccic/cafe-driver.c
+++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
@@ -8,6 +8,7 @@
  *
  * Copyright 2006-11 One Laptop Per Child Association, Inc.
  * Copyright 2006-11 Jonathan Corbet <corbet@lwn.net>
+ * Copyright 2018 Lubomir Rintel <lkundrak@v3.sk>
  *
  * Written by Jonathan Corbet, corbet@lwn.net.
  *
@@ -27,6 +28,7 @@
 #include <linux/slab.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
+#include <media/i2c/ov7670.h>
 #include <linux/device.h>
 #include <linux/wait.h>
 #include <linux/delay.h>
@@ -52,6 +54,7 @@ struct cafe_camera {
 	int registered;			/* Fully initialized? */
 	struct mcam_camera mcam;
 	struct pci_dev *pdev;
+	struct i2c_adapter *i2c_adapter;
 	wait_queue_head_t smbus_wait;	/* Waiting on i2c events */
 };
 
@@ -351,15 +354,15 @@ static int cafe_smbus_setup(struct cafe_camera *cam)
 		return ret;
 	}
 
-	cam->mcam.i2c_adapter = adap;
+	cam->i2c_adapter = adap;
 	cafe_smbus_enable_irq(cam);
 	return 0;
 }
 
 static void cafe_smbus_shutdown(struct cafe_camera *cam)
 {
-	i2c_del_adapter(cam->mcam.i2c_adapter);
-	kfree(cam->mcam.i2c_adapter);
+	i2c_del_adapter(cam->i2c_adapter);
+	kfree(cam->i2c_adapter);
 }
 
 
@@ -452,6 +455,29 @@ static irqreturn_t cafe_irq(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
+/* -------------------------------------------------------------------------- */
+
+static struct ov7670_config sensor_cfg = {
+	/*
+	 * Exclude QCIF mode, because it only captures a tiny portion
+	 * of the sensor FOV
+	 */
+	.min_width = 320,
+	.min_height = 240,
+
+	/*
+	 * Set the clock speed for the XO 1; I don't believe this
+	 * driver has ever run anywhere else.
+	 */
+	.clock_speed = 45,
+	.use_smbus = 1,
+};
+
+struct i2c_board_info ov7670_info = {
+	.type = "ov7670",
+	.addr = 0x42 >> 1,
+	.platform_data = &sensor_cfg,
+};
 
 /* -------------------------------------------------------------------------- */
 /*
@@ -481,12 +507,6 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	mcam->plat_power_down = cafe_ctlr_power_down;
 	mcam->dev = &pdev->dev;
 	snprintf(mcam->bus_info, sizeof(mcam->bus_info), "PCI:%s", pci_name(pdev));
-	/*
-	 * Set the clock speed for the XO 1; I don't believe this
-	 * driver has ever run anywhere else.
-	 */
-	mcam->clock_speed = 45;
-	mcam->use_smbus = 1;
 	/*
 	 * Vmalloc mode for buffers is traditional with this driver.
 	 * We *might* be able to run DMA_contig, especially on a system
@@ -527,12 +547,21 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		goto out_pdown;
 
+	mcam->asd.match_type = V4L2_ASYNC_MATCH_I2C;
+	mcam->asd.match.i2c.adapter_id = i2c_adapter_id(cam->i2c_adapter);
+	mcam->asd.match.i2c.address = ov7670_info.addr;
+
 	ret = mccic_register(mcam);
-	if (ret == 0) {
+	if (ret)
+		goto out_smbus_shutdown;
+
+	if (i2c_new_device(cam->i2c_adapter, &ov7670_info)) {
 		cam->registered = 1;
 		return 0;
 	}
 
+	mccic_shutdown(mcam);
+out_smbus_shutdown:
 	cafe_smbus_shutdown(cam);
 out_pdown:
 	cafe_ctlr_power_down(mcam);
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 0113b8d37d03..87812b7287f0 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -4,6 +4,7 @@
  * so it needs platform-specific support outside of the core.
  *
  * Copyright 2011 Jonathan Corbet corbet@lwn.net
+ * Copyright 2018 Lubomir Rintel <lkundrak@v3.sk>
  */
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -26,7 +27,6 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
-#include <media/i2c/ov7670.h>
 #include <media/videobuf2-vmalloc.h>
 #include <media/videobuf2-dma-contig.h>
 #include <media/videobuf2-dma-sg.h>
@@ -93,6 +93,9 @@ MODULE_PARM_DESC(buffer_mode,
 #define sensor_call(cam, o, f, args...) \
 	v4l2_subdev_call(cam->sensor, o, f, ##args)
 
+#define notifier_to_mcam(notifier) \
+	container_of(notifier, struct mcam_camera, notifier)
+
 static struct mcam_format_struct {
 	__u8 *desc;
 	__u32 pixelformat;
@@ -1715,23 +1718,94 @@ EXPORT_SYMBOL_GPL(mccic_irq);
 /*
  * Registration and such.
  */
-static struct ov7670_config sensor_cfg = {
+
+static int mccic_notify_bound(struct v4l2_async_notifier *notifier,
+	struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd)
+{
+	struct mcam_camera *cam = notifier_to_mcam(notifier);
+	int ret;
+
+	mutex_lock(&cam->s_mutex);
+	if (cam->sensor) {
+		cam_err(cam, "sensor already bound\n");
+		ret = -EBUSY;
+		goto out;
+	}
+
+	v4l2_set_subdev_hostdata(subdev, cam);
+	cam->sensor = subdev;
+
+	ret = mcam_cam_init(cam);
+	if (ret) {
+		cam->sensor = NULL;
+		goto out;
+	}
+
+	ret = mcam_setup_vb2(cam);
+	if (ret) {
+		cam->sensor = NULL;
+		goto out;
+	}
+
+	cam->vdev = mcam_v4l_template;
+	cam->vdev.v4l2_dev = &cam->v4l2_dev;
+	cam->vdev.lock = &cam->s_mutex;
+	cam->vdev.queue = &cam->vb_queue;
+	video_set_drvdata(&cam->vdev, cam);
+	ret = video_register_device(&cam->vdev, VFL_TYPE_GRABBER, -1);
+	if (ret) {
+		cam->sensor = NULL;
+		goto out;
+	}
+
+	cam_dbg(cam, "sensor %s bound\n", subdev->name);
+out:
+	mutex_unlock(&cam->s_mutex);
+	return ret;
+}
+
+static void mccic_notify_unbind(struct v4l2_async_notifier *notifier,
+	struct v4l2_subdev *subdev, struct v4l2_async_subdev *asd)
+{
+	struct mcam_camera *cam = notifier_to_mcam(notifier);
+
+	mutex_lock(&cam->s_mutex);
+	if (cam->sensor != subdev) {
+		cam_err(cam, "sensor %s not bound\n", subdev->name);
+		goto out;
+	}
+
+	video_unregister_device(&cam->vdev);
+	cam->sensor = NULL;
+	cam_dbg(cam, "sensor %s unbound\n", subdev->name);
+
+out:
+	mutex_unlock(&cam->s_mutex);
+}
+
+static int mccic_notify_complete(struct v4l2_async_notifier *notifier)
+{
+	struct mcam_camera *cam = notifier_to_mcam(notifier);
+	int ret;
+
 	/*
-	 * Exclude QCIF mode, because it only captures a tiny portion
-	 * of the sensor FOV
+	 * Get the v4l2 setup done.
 	 */
-	.min_width = 320,
-	.min_height = 240,
-};
+	ret = v4l2_ctrl_handler_init(&cam->ctrl_handler, 10);
+	if (!ret)
+		cam->v4l2_dev.ctrl_handler = &cam->ctrl_handler;
+
+	return ret;
+}
 
+static const struct v4l2_async_notifier_operations mccic_notify_ops = {
+	.bound = mccic_notify_bound,
+	.unbind = mccic_notify_unbind,
+	.complete = mccic_notify_complete,
+};
 
 int mccic_register(struct mcam_camera *cam)
 {
-	struct i2c_board_info ov7670_info = {
-		.type = "ov7670",
-		.addr = 0x42 >> 1,
-		.platform_data = &sensor_cfg,
-	};
 	int ret;
 
 	/*
@@ -1744,17 +1818,20 @@ int mccic_register(struct mcam_camera *cam)
 		printk(KERN_ERR "marvell-cam: Cafe can't do S/G I/O, attempting vmalloc mode instead\n");
 		cam->buffer_mode = B_vmalloc;
 	}
+
 	if (!mcam_buffer_mode_supported(cam->buffer_mode)) {
 		printk(KERN_ERR "marvell-cam: buffer mode %d unsupported\n",
 				cam->buffer_mode);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
+
 	/*
 	 * Register with V4L
 	 */
 	ret = v4l2_device_register(cam->dev, &cam->v4l2_dev);
 	if (ret)
-		return ret;
+		goto out;
 
 	mutex_init(&cam->s_mutex);
 	cam->state = S_NOTREADY;
@@ -1764,43 +1841,20 @@ int mccic_register(struct mcam_camera *cam)
 	mcam_ctlr_init(cam);
 
 	/*
-	 * Get the v4l2 setup done.
+	 * Register sensor notifier.
 	 */
-	ret = v4l2_ctrl_handler_init(&cam->ctrl_handler, 10);
-	if (ret)
-		goto out_unregister;
-	cam->v4l2_dev.ctrl_handler = &cam->ctrl_handler;
-
-	/*
-	 * Try to find the sensor.
-	 */
-	sensor_cfg.clock_speed = cam->clock_speed;
-	sensor_cfg.use_smbus = cam->use_smbus;
-	cam->sensor = v4l2_i2c_new_subdev_board(&cam->v4l2_dev,
-			cam->i2c_adapter, &ov7670_info, NULL);
-	if (cam->sensor == NULL) {
-		ret = -ENODEV;
-		goto out_unregister;
+	v4l2_async_notifier_init(&cam->notifier);
+	ret = v4l2_async_notifier_add_subdev(&cam->notifier, &cam->asd);
+	if (ret) {
+		cam_warn(cam, "failed to add subdev to a notifier");
+		goto out;
 	}
 
-	ret = mcam_cam_init(cam);
-	if (ret)
-		goto out_unregister;
-
-	ret = mcam_setup_vb2(cam);
-	if (ret)
-		goto out_unregister;
-
-	mutex_lock(&cam->s_mutex);
-	cam->vdev = mcam_v4l_template;
-	cam->vdev.v4l2_dev = &cam->v4l2_dev;
-	cam->vdev.lock = &cam->s_mutex;
-	cam->vdev.queue = &cam->vb_queue;
-	video_set_drvdata(&cam->vdev, cam);
-	ret = video_register_device(&cam->vdev, VFL_TYPE_GRABBER, -1);
-	if (ret) {
-		mutex_unlock(&cam->s_mutex);
-		goto out_unregister;
+	cam->notifier.ops = &mccic_notify_ops;
+	ret = v4l2_async_notifier_register(&cam->v4l2_dev, &cam->notifier);
+	if (ret < 0) {
+		cam_warn(cam, "failed to register a sensor notifier");
+		goto out;
 	}
 
 	/*
@@ -1811,11 +1865,10 @@ int mccic_register(struct mcam_camera *cam)
 			cam_warn(cam, "Unable to alloc DMA buffers at load will try again later.");
 	}
 
-	mutex_unlock(&cam->s_mutex);
 	return 0;
 
-out_unregister:
-	v4l2_ctrl_handler_free(&cam->ctrl_handler);
+out:
+	v4l2_async_notifier_unregister(&cam->notifier);
 	v4l2_device_unregister(&cam->v4l2_dev);
 	return ret;
 }
@@ -1835,8 +1888,8 @@ void mccic_shutdown(struct mcam_camera *cam)
 	}
 	if (cam->buffer_mode == B_vmalloc)
 		mcam_free_dma_bufs(cam);
-	video_unregister_device(&cam->vdev);
 	v4l2_ctrl_handler_free(&cam->ctrl_handler);
+	v4l2_async_notifier_unregister(&cam->notifier);
 	v4l2_device_unregister(&cam->v4l2_dev);
 }
 EXPORT_SYMBOL_GPL(mccic_shutdown);
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index b828b1bb59d3..4a72213aca1a 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -102,14 +102,11 @@ struct mcam_camera {
 	 * These fields should be set by the platform code prior to
 	 * calling mcam_register().
 	 */
-	struct i2c_adapter *i2c_adapter;
 	unsigned char __iomem *regs;
 	unsigned regs_size; /* size in bytes of the register space */
 	spinlock_t dev_lock;
 	struct device *dev; /* For messages, dma alloc */
 	enum mcam_chip_id chip_id;
-	short int clock_speed;	/* Sensor clock speed, default 30 */
-	short int use_smbus;	/* SMBUS or straight I2c? */
 	enum mcam_buffer_mode buffer_mode;
 
 	int mclk_src;	/* which clock source the mclk derives from */
@@ -150,6 +147,8 @@ struct mcam_camera {
 	 * Subsystem structures.
 	 */
 	struct video_device vdev;
+	struct v4l2_async_notifier notifier;
+	struct v4l2_async_subdev asd;
 	struct v4l2_subdev *sensor;
 
 	/* Videobuf2 stuff */
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 05b9fa8c6a6f..efbffb06e25c 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -3,6 +3,7 @@
  * to work with the Armada 610 as used in the OLPC 1.75 system.
  *
  * Copyright 2011 Jonathan Corbet <corbet@lwn.net>
+ * Copyright 2018 Lubomir Rintel <lkundrak@v3.sk>
  *
  * This file may be distributed under the terms of the GNU General
  * Public License, version 2.
@@ -11,7 +12,6 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
@@ -316,6 +316,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	struct mmp_camera *cam;
 	struct mcam_camera *mcam;
 	struct resource *res;
+	struct fwnode_handle *ep;
 	struct mmp_camera_platform_data *pdata;
 	int ret;
 
@@ -330,7 +331,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	mcam->plat_power_down = mmpcam_power_down;
 	mcam->calc_dphy = mmpcam_calc_dphy;
 	mcam->dev = &pdev->dev;
-	mcam->use_smbus = 0;
 	pdata = pdev->dev.platform_data;
 	if (pdata) {
 		mcam->mclk_src = pdata->mclk_src;
@@ -374,15 +374,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(cam->power_regs))
 		return PTR_ERR(cam->power_regs);
-	/*
-	 * Find the i2c adapter.  This assumes, of course, that the
-	 * i2c bus is already up and functioning.
-	 */
-	mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device);
-	if (mcam->i2c_adapter == NULL) {
-		dev_err(&pdev->dev, "No i2c adapter\n");
-		return -ENODEV;
-	}
 	/*
 	 * Sensor GPIO pins.
 	 */
@@ -405,6 +396,19 @@ static int mmpcam_probe(struct platform_device *pdev)
 
 	mcam_init_clk(mcam);
 
+	/*
+	 * Create a match of the sensor against its OF node.
+	 */
+	ep = fwnode_graph_get_next_endpoint(of_fwnode_handle(pdev->dev.of_node),
+					    NULL);
+	if (!ep)
+		return -ENODEV;
+
+	mcam->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+	mcam->asd.match.fwnode = fwnode_graph_get_remote_port_parent(ep);
+
+	fwnode_handle_put(ep);
+
 	/*
 	 * Power the device up and hand it off to the core.
 	 */
@@ -414,6 +418,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	ret = mccic_register(mcam);
 	if (ret)
 		goto out_power_down;
+
 	/*
 	 * Finally, set up our IRQ now that the core is ready to
 	 * deal with it.
diff --git a/include/linux/platform_data/media/mmp-camera.h b/include/linux/platform_data/media/mmp-camera.h
index 4c3a80a45883..c573ebc40035 100644
--- a/include/linux/platform_data/media/mmp-camera.h
+++ b/include/linux/platform_data/media/mmp-camera.h
@@ -12,7 +12,6 @@ enum dphy3_algo {
 };
 
 struct mmp_camera_platform_data {
-	struct platform_device *i2c_device;
 	int sensor_power_gpio;
 	int sensor_reset_gpio;
 	enum v4l2_mbus_type bus_type;
-- 
2.19.1

[PATCH v3 14/14] [media] marvell-ccic: provide a clock for the sensor

[Lubomir Rintel]

The sensor needs the MCLK clock running when it's being probed. On
platforms where the sensor is instantiated from a DT (MMP2) it is going
to happen asynchronously.

Therefore, the current modus operandi, where the bridge driver fiddles
with the sensor power and clock itself is not going to fly. As the comments
wisely note, this doesn't even belong there.

Luckily, the ov7670 driver is already able to control its power and
reset lines, we can just drop the MMP platform glue altogether.

It also requests the clock via the standard clock subsystem. Good -- let's
set up a clock instance so that the sensor can ask us to enable the clock.
Note that this is pretty dumb at the moment: the clock is hardwired to a
particular frequency and parent. It was always the case.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>

---
Changes since v1:
- [kbuild/ia64] depend on COMMON_CLK.
- [smatch] fix bad return in mcam_v4l_open() leading to lock not getting
  released on error.

 drivers/media/platform/marvell-ccic/Kconfig   |   2 +
 .../media/platform/marvell-ccic/cafe-driver.c |   9 +-
 .../media/platform/marvell-ccic/mcam-core.c   | 156 +++++++++++++++---
 .../media/platform/marvell-ccic/mcam-core.h   |   3 +
 .../media/platform/marvell-ccic/mmp-driver.c  | 152 ++---------------
 .../linux/platform_data/media/mmp-camera.h    |   2 -
 6 files changed, 161 insertions(+), 163 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/Kconfig b/drivers/media/platform/marvell-ccic/Kconfig
index cf12e077203a..3e12eb25740a 100644
--- a/drivers/media/platform/marvell-ccic/Kconfig
+++ b/drivers/media/platform/marvell-ccic/Kconfig
@@ -1,6 +1,7 @@
 config VIDEO_CAFE_CCIC
 	tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
 	depends on PCI && I2C && VIDEO_V4L2
+	depends on COMMON_CLK
 	select VIDEO_OV7670
 	select VIDEOBUF2_VMALLOC
 	select VIDEOBUF2_DMA_CONTIG
@@ -14,6 +15,7 @@ config VIDEO_MMP_CAMERA
 	tristate "Marvell Armada 610 integrated camera controller support"
 	depends on I2C && VIDEO_V4L2
 	depends on ARCH_MMP || COMPILE_TEST
+	depends on COMMON_CLK
 	select VIDEO_OV7670
 	select I2C_GPIO
 	select VIDEOBUF2_VMALLOC
diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c
index 658294d319c0..0e712bb941ba 100644
--- a/drivers/media/platform/marvell-ccic/cafe-driver.c
+++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
@@ -33,6 +33,7 @@
 #include <linux/wait.h>
 #include <linux/delay.h>
 #include <linux/io.h>
+#include <linux/clkdev.h>
 
 #include "mcam-core.h"
 
@@ -533,11 +534,10 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 		goto out_iounmap;
 
 	/*
-	 * Initialize the controller and leave it powered up.  It will
-	 * stay that way until the sensor driver shows up.
+	 * Initialize the controller.
 	 */
 	cafe_ctlr_init(mcam);
-	cafe_ctlr_power_up(mcam);
+
 	/*
 	 * Set up I2C/SMBUS communications.  We have to drop the mutex here
 	 * because the sensor could attach in this call chain, leading to
@@ -555,6 +555,9 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		goto out_smbus_shutdown;
 
+	clkdev_create(mcam->mclk, "xclk", "%d-%04x",
+		i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr);
+
 	if (i2c_new_device(cam->i2c_adapter, &ov7670_info)) {
 		cam->registered = 1;
 		return 0;
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 87812b7287f0..982fbac6472d 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -22,6 +22,7 @@
 #include <linux/vmalloc.h>
 #include <linux/io.h>
 #include <linux/clk.h>
+#include <linux/clk-provider.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -303,9 +304,6 @@ static void mcam_enable_mipi(struct mcam_camera *mcam)
 		 */
 		mcam_reg_write(mcam, REG_CSI2_CTRL0,
 			CSI2_C0_MIPI_EN | CSI2_C0_ACT_LANE(mcam->lane));
-		mcam_reg_write(mcam, REG_CLKCTRL,
-			(mcam->mclk_src << 29) | mcam->mclk_div);
-
 		mcam->mipi_enabled = true;
 	}
 }
@@ -846,11 +844,6 @@ static void mcam_ctlr_init(struct mcam_camera *cam)
 	 * but it's good to be sure.
 	 */
 	mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE);
-	/*
-	 * Clock the sensor appropriately.  Controller clock should
-	 * be 48MHz, sensor "typical" value is half that.
-	 */
-	mcam_reg_write_mask(cam, REG_CLKCTRL, 2, CLK_DIV_MASK);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
 }
 
@@ -898,14 +891,15 @@ static int mcam_ctlr_power_up(struct mcam_camera *cam)
 	int ret;
 
 	spin_lock_irqsave(&cam->dev_lock, flags);
-	ret = cam->plat_power_up(cam);
-	if (ret) {
-		spin_unlock_irqrestore(&cam->dev_lock, flags);
-		return ret;
+	if (cam->plat_power_up) {
+		ret = cam->plat_power_up(cam);
+		if (ret) {
+			spin_unlock_irqrestore(&cam->dev_lock, flags);
+			return ret;
+		}
 	}
 	mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
-	msleep(5); /* Just to be sure */
 	return 0;
 }
 
@@ -920,10 +914,101 @@ static void mcam_ctlr_power_down(struct mcam_camera *cam)
 	 * power down routine.
 	 */
 	mcam_reg_set_bit(cam, REG_CTRL1, C1_PWRDWN);
-	cam->plat_power_down(cam);
+	if (cam->plat_power_down)
+		cam->plat_power_down(cam);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
 }
 
+/* ---------------------------------------------------------------------- */
+/*
+ * Controller clocks.
+ */
+static void mcam_clk_enable(struct mcam_camera *mcam)
+{
+	unsigned int i;
+
+	for (i = 0; i < NR_MCAM_CLK; i++) {
+		if (!IS_ERR(mcam->clk[i]))
+			clk_prepare_enable(mcam->clk[i]);
+	}
+}
+
+static void mcam_clk_disable(struct mcam_camera *mcam)
+{
+	int i;
+
+	for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
+		if (!IS_ERR(mcam->clk[i]))
+			clk_disable_unprepare(mcam->clk[i]);
+	}
+}
+
+/* ---------------------------------------------------------------------- */
+/*
+ * Master sensor clock.
+ */
+static int mclk_prepare(struct clk_hw *hw)
+{
+	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
+
+	clk_prepare(cam->clk[0]);
+	return 0;
+}
+
+static void mclk_unprepare(struct clk_hw *hw)
+{
+	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
+
+	clk_unprepare(cam->clk[0]);
+}
+
+static int mclk_enable(struct clk_hw *hw)
+{
+	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
+	int mclk_src;
+	int mclk_div;
+
+	/*
+	 * Clock the sensor appropriately.  Controller clock should
+	 * be 48MHz, sensor "typical" value is half that.
+	 */
+	if (cam->bus_type == V4L2_MBUS_CSI2_DPHY) {
+		mclk_src = cam->mclk_src;
+		mclk_div = cam->mclk_div;
+	} else {
+		mclk_src = 3;
+		mclk_div = 2;
+	}
+
+	clk_enable(cam->clk[0]);
+	mcam_reg_write(cam, REG_CLKCTRL, (mclk_src << 29) | mclk_div);
+	mcam_ctlr_power_up(cam);
+
+	return 0;
+}
+
+static void mclk_disable(struct clk_hw *hw)
+{
+	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
+
+	mcam_ctlr_power_down(cam);
+	clk_disable(cam->clk[0]);
+}
+
+static unsigned long mclk_recalc_rate(struct clk_hw *hw,
+				unsigned long parent_rate)
+{
+	return 48000000;
+}
+
+static const struct clk_ops mclk_ops = {
+	.prepare = mclk_prepare,
+	.unprepare = mclk_unprepare,
+	.enable = mclk_enable,
+	.disable = mclk_disable,
+	.recalc_rate = mclk_recalc_rate,
+};
+
 /* -------------------------------------------------------------------- */
 /*
  * Communications with the sensor.
@@ -948,7 +1033,6 @@ static int mcam_cam_init(struct mcam_camera *cam)
 	ret = __mcam_cam_reset(cam);
 	/* Get/set parameters? */
 	cam->state = S_IDLE;
-	mcam_ctlr_power_down(cam);
 	return ret;
 }
 
@@ -1584,9 +1668,10 @@ static int mcam_v4l_open(struct file *filp)
 	if (ret)
 		goto out;
 	if (v4l2_fh_is_singular_file(filp)) {
-		ret = mcam_ctlr_power_up(cam);
+		ret = sensor_call(cam, core, s_power, 1);
 		if (ret)
 			goto out;
+		mcam_clk_enable(cam);
 		__mcam_cam_reset(cam);
 		mcam_set_config_needed(cam, 1);
 	}
@@ -1608,7 +1693,8 @@ static int mcam_v4l_release(struct file *filp)
 	_vb2_fop_release(filp, NULL);
 	if (last_open) {
 		mcam_disable_mipi(cam);
-		mcam_ctlr_power_down(cam);
+		sensor_call(cam, core, s_power, 0);
+		mcam_clk_disable(cam);
 		if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read)
 			mcam_free_dma_bufs(cam);
 	}
@@ -1806,6 +1892,7 @@ static const struct v4l2_async_notifier_operations mccic_notify_ops = {
 
 int mccic_register(struct mcam_camera *cam)
 {
+	struct clk_init_data mclk_init = { };
 	int ret;
 
 	/*
@@ -1838,7 +1925,10 @@ int mccic_register(struct mcam_camera *cam)
 	mcam_set_config_needed(cam, 1);
 	cam->pix_format = mcam_def_pix_format;
 	cam->mbus_code = mcam_def_mbus_code;
-	mcam_ctlr_init(cam);
+
+	mcam_clk_enable(cam);
+	mcam_ctlr_init(cam); // XXX?
+	mcam_clk_disable(cam);
 
 	/*
 	 * Register sensor notifier.
@@ -1857,6 +1947,26 @@ int mccic_register(struct mcam_camera *cam)
 		goto out;
 	}
 
+	/*
+	 * Register sensor master clock.
+	 */
+	mclk_init.parent_names = NULL;
+	mclk_init.num_parents = 0;
+	mclk_init.ops = &mclk_ops;
+	mclk_init.name = "mclk";
+
+	of_property_read_string(cam->dev->of_node, "clock-output-names",
+							&mclk_init.name);
+
+	cam->mclk_hw.init = &mclk_init;
+
+	cam->mclk = devm_clk_register(cam->dev, &cam->mclk_hw);
+	if (IS_ERR(cam->mclk)) {
+		ret = PTR_ERR(cam->mclk);
+		dev_err(cam->dev, "can't register clock\n");
+		goto out;
+	}
+
 	/*
 	 * If so requested, try to get our DMA buffers now.
 	 */
@@ -1884,7 +1994,7 @@ void mccic_shutdown(struct mcam_camera *cam)
 	 */
 	if (!list_empty(&cam->vdev.fh_list)) {
 		cam_warn(cam, "Removing a device with users!\n");
-		mcam_ctlr_power_down(cam);
+		sensor_call(cam, core, s_power, 0);
 	}
 	if (cam->buffer_mode == B_vmalloc)
 		mcam_free_dma_bufs(cam);
@@ -1906,7 +2016,8 @@ void mccic_suspend(struct mcam_camera *cam)
 		enum mcam_state cstate = cam->state;
 
 		mcam_ctlr_stop_dma(cam);
-		mcam_ctlr_power_down(cam);
+		sensor_call(cam, core, s_power, 0);
+		mcam_clk_disable(cam);
 		cam->state = cstate;
 	}
 	mutex_unlock(&cam->s_mutex);
@@ -1919,14 +2030,15 @@ int mccic_resume(struct mcam_camera *cam)
 
 	mutex_lock(&cam->s_mutex);
 	if (!list_empty(&cam->vdev.fh_list)) {
-		ret = mcam_ctlr_power_up(cam);
+		mcam_clk_enable(cam);
+		ret = sensor_call(cam, core, s_power, 1);
 		if (ret) {
 			mutex_unlock(&cam->s_mutex);
 			return ret;
 		}
 		__mcam_cam_reset(cam);
 	} else {
-		mcam_ctlr_power_down(cam);
+		sensor_call(cam, core, s_power, 0);
 	}
 	mutex_unlock(&cam->s_mutex);
 
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index 4a72213aca1a..2e3a7567a76a 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -8,6 +8,7 @@
 #define _MCAM_CORE_H
 
 #include <linux/list.h>
+#include <linux/clk-provider.h>
 #include <media/v4l2-common.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dev.h>
@@ -125,6 +126,8 @@ struct mcam_camera {
 
 	/* clock tree support */
 	struct clk *clk[NR_MCAM_CLK];
+	struct clk_hw mclk_hw;
+	struct clk *mclk;
 
 	/*
 	 * Callbacks from the core to the platform code.
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index efbffb06e25c..7e0783dc9152 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -22,9 +22,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
-#include <linux/gpio.h>
 #include <linux/io.h>
-#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/pm.h>
 #include <linux/clk.h>
@@ -38,7 +36,6 @@ MODULE_LICENSE("GPL");
 static char *mcam_clks[] = {"axi", "func", "phy"};
 
 struct mmp_camera {
-	void __iomem *power_regs;
 	struct platform_device *pdev;
 	struct mcam_camera mcam;
 	struct list_head devlist;
@@ -94,94 +91,6 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev)
 	return NULL;
 }
 
-
-
-
-/*
- * Power-related registers; this almost certainly belongs
- * somewhere else.
- *
- * ARMADA 610 register manual, sec 7.2.1, p1842.
- */
-#define CPU_SUBSYS_PMU_BASE	0xd4282800
-#define REG_CCIC_DCGCR		0x28	/* CCIC dyn clock gate ctrl reg */
-#define REG_CCIC_CRCR		0x50	/* CCIC clk reset ctrl reg	*/
-
-static void mcam_clk_enable(struct mcam_camera *mcam)
-{
-	unsigned int i;
-
-	for (i = 0; i < NR_MCAM_CLK; i++) {
-		if (!IS_ERR(mcam->clk[i]))
-			clk_prepare_enable(mcam->clk[i]);
-	}
-}
-
-static void mcam_clk_disable(struct mcam_camera *mcam)
-{
-	int i;
-
-	for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
-		if (!IS_ERR(mcam->clk[i]))
-			clk_disable_unprepare(mcam->clk[i]);
-	}
-}
-
-/*
- * Power control.
- */
-static void mmpcam_power_up_ctlr(struct mmp_camera *cam)
-{
-	iowrite32(0x3f, cam->power_regs + REG_CCIC_DCGCR);
-	iowrite32(0x3805b, cam->power_regs + REG_CCIC_CRCR);
-	mdelay(1);
-}
-
-static int mmpcam_power_up(struct mcam_camera *mcam)
-{
-	struct mmp_camera *cam = mcam_to_cam(mcam);
-	struct mmp_camera_platform_data *pdata;
-
-/*
- * Turn on power and clocks to the controller.
- */
-	mmpcam_power_up_ctlr(cam);
-	mcam_clk_enable(mcam);
-/*
- * Provide power to the sensor.
- */
-	mcam_reg_write(mcam, REG_CLKCTRL, 0x60000002);
-	pdata = cam->pdev->dev.platform_data;
-	gpio_set_value(pdata->sensor_power_gpio, 1);
-	mdelay(5);
-	mcam_reg_clear_bit(mcam, REG_CTRL1, 0x10000000);
-	gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */
-	mdelay(5);
-	gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
-	mdelay(5);
-
-	return 0;
-}
-
-static void mmpcam_power_down(struct mcam_camera *mcam)
-{
-	struct mmp_camera *cam = mcam_to_cam(mcam);
-	struct mmp_camera_platform_data *pdata;
-/*
- * Turn off clocks and set reset lines
- */
-	iowrite32(0, cam->power_regs + REG_CCIC_DCGCR);
-	iowrite32(0, cam->power_regs + REG_CCIC_CRCR);
-/*
- * Shut down the sensor.
- */
-	pdata = cam->pdev->dev.platform_data;
-	gpio_set_value(pdata->sensor_power_gpio, 0);
-	gpio_set_value(pdata->sensor_reset_gpio, 0);
-
-	mcam_clk_disable(mcam);
-}
-
 /*
  * calc the dphy register values
  * There are three dphy registers being used.
@@ -327,8 +236,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&cam->devlist);
 
 	mcam = &cam->mcam;
-	mcam->plat_power_up = mmpcam_power_up;
-	mcam->plat_power_down = mmpcam_power_down;
 	mcam->calc_dphy = mmpcam_calc_dphy;
 	mcam->dev = &pdev->dev;
 	pdata = pdev->dev.platform_data;
@@ -366,33 +273,6 @@ static int mmpcam_probe(struct platform_device *pdev)
 	if (IS_ERR(mcam->regs))
 		return PTR_ERR(mcam->regs);
 	mcam->regs_size = resource_size(res);
-	/*
-	 * Power/clock memory is elsewhere; get it too.  Perhaps this
-	 * should really be managed outside of this driver?
-	 */
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(cam->power_regs))
-		return PTR_ERR(cam->power_regs);
-	/*
-	 * Sensor GPIO pins.
-	 */
-	ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio,
-							"cam-power");
-	if (ret) {
-		dev_err(&pdev->dev, "Can't get sensor power gpio %d",
-				pdata->sensor_power_gpio);
-		return ret;
-	}
-	gpio_direction_output(pdata->sensor_power_gpio, 0);
-	ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio,
-							"cam-reset");
-	if (ret) {
-		dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
-				pdata->sensor_reset_gpio);
-		return ret;
-	}
-	gpio_direction_output(pdata->sensor_reset_gpio, 0);
 
 	mcam_init_clk(mcam);
 
@@ -410,14 +290,21 @@ static int mmpcam_probe(struct platform_device *pdev)
 	fwnode_handle_put(ep);
 
 	/*
-	 * Power the device up and hand it off to the core.
+	 * Register the device with the core.
 	 */
-	ret = mmpcam_power_up(mcam);
-	if (ret)
-		return ret;
 	ret = mccic_register(mcam);
 	if (ret)
-		goto out_power_down;
+		return ret;
+
+	/*
+	 * Add OF clock provider.
+	 */
+	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get,
+								mcam->mclk);
+	if (ret) {
+		dev_err(&pdev->dev, "can't add DT clock provider\n");
+		goto out;
+	}
 
 	/*
 	 * Finally, set up our IRQ now that the core is ready to
@@ -426,7 +313,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (res == NULL) {
 		ret = -ENODEV;
-		goto out_unregister;
+		goto out;
 	}
 	cam->irq = res->start;
 	ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED,
@@ -436,10 +323,10 @@ static int mmpcam_probe(struct platform_device *pdev)
 		return 0;
 	}
 
-out_unregister:
+out:
+	fwnode_handle_put(mcam->asd.match.fwnode);
 	mccic_shutdown(mcam);
-out_power_down:
-	mmpcam_power_down(mcam);
+
 	return ret;
 }
 
@@ -450,7 +337,6 @@ static int mmpcam_remove(struct mmp_camera *cam)
 
 	mmpcam_remove_device(cam);
 	mccic_shutdown(mcam);
-	mmpcam_power_down(mcam);
 	return 0;
 }
 
@@ -482,12 +368,6 @@ static int mmpcam_resume(struct platform_device *pdev)
 {
 	struct mmp_camera *cam = mmpcam_find_device(pdev);
 
-	/*
-	 * Power up unconditionally just in case the core tries to
-	 * touch a register even if nothing was active before; trust
-	 * me, it's better this way.
-	 */
-	mmpcam_power_up_ctlr(cam);
 	return mccic_resume(&cam->mcam);
 }
 
diff --git a/include/linux/platform_data/media/mmp-camera.h b/include/linux/platform_data/media/mmp-camera.h
index c573ebc40035..53adaab64f28 100644
--- a/include/linux/platform_data/media/mmp-camera.h
+++ b/include/linux/platform_data/media/mmp-camera.h
@@ -12,8 +12,6 @@ enum dphy3_algo {
 };
 
 struct mmp_camera_platform_data {
-	int sensor_power_gpio;
-	int sensor_reset_gpio;
 	enum v4l2_mbus_type bus_type;
 	int mclk_src;	/* which clock source the MCLK derives from */
 	int mclk_div;	/* Clock Divider Value for MCLK */
-- 
2.19.1

Re: [PATCH v3 03/14] media: ov7670: hook s_power onto v4l2 core

[Sakari Ailus]

Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:08AM +0100, Lubomir Rintel wrote:
> The commit 71862f63f351 ("media: ov7670: Add the ov7670_s_power function")
> added a power control routing. However, it was not good enough to use as
> a s_power() callback: it merely flipped on the power GPIOs without
> restoring the register settings.
> 
> Fix this now and register an actual power callback.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v2:
> - Restore the controls, format and frame rate on power on
> 
>  drivers/media/i2c/ov7670.c | 50 +++++++++++++++++++++++++++++++++-----
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index ead0c360df33..cbaab60aaaac 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -242,6 +242,7 @@ struct ov7670_info {
>  	struct ov7670_format_struct *fmt;  /* Current format */
>  	struct ov7670_win_size *wsize;
>  	struct clk *clk;
> +	int on;
>  	struct gpio_desc *resetb_gpio;
>  	struct gpio_desc *pwdn_gpio;
>  	unsigned int mbus_config;	/* Media bus configuration flags */
> @@ -1615,19 +1616,54 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
>  }
>  #endif
>  
> -static int ov7670_s_power(struct v4l2_subdev *sd, int on)
> +static void ov7670_power_on(struct v4l2_subdev *sd)
>  {
>  	struct ov7670_info *info = to_state(sd);
>  
> +	if (info->on)
> +		return;
> +
>  	if (info->pwdn_gpio)
> -		gpiod_set_value(info->pwdn_gpio, !on);
> -	if (on && info->resetb_gpio) {
> +		gpiod_set_value(info->pwdn_gpio, 0);
> +	if (info->resetb_gpio) {
>  		gpiod_set_value(info->resetb_gpio, 1);
>  		usleep_range(500, 1000);
>  		gpiod_set_value(info->resetb_gpio, 0);
>  		usleep_range(3000, 5000);
>  	}
>  
> +	info->on = true;
> +}
> +
> +static void ov7670_power_off(struct v4l2_subdev *sd)
> +{
> +	struct ov7670_info *info = to_state(sd);
> +
> +	if (!info->on)
> +		return;
> +
> +	if (info->pwdn_gpio)
> +		gpiod_set_value(info->pwdn_gpio, 1);
> +
> +	info->on = false;
> +}
> +
> +static int ov7670_s_power(struct v4l2_subdev *sd, int on)
> +{
> +	struct ov7670_info *info = to_state(sd);
> +
> +	if (info->on == on)
> +		return 0;
> +
> +	if (on) {
> +		ov7670_power_on (sd);
> +		ov7670_apply_fmt(sd);
> +		ov7675_apply_framerate(sd);
> +		v4l2_ctrl_handler_setup(&info->hdl);
> +	} else {
> +		ov7670_power_off (sd);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -1660,6 +1696,7 @@ static int ov7670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
>  	.reset = ov7670_reset,
>  	.init = ov7670_init,
> +	.s_power = ov7670_s_power,
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	.g_register = ov7670_g_register,
>  	.s_register = ov7670_s_register,
> @@ -1825,6 +1862,7 @@ static int ov7670_probe(struct i2c_client *client,
>  		else
>  			return ret;
>  	}
> +
>  	if (info->clk) {
>  		ret = clk_prepare_enable(info->clk);
>  		if (ret)
> @@ -1841,7 +1879,7 @@ static int ov7670_probe(struct i2c_client *client,
>  	if (ret)
>  		goto clk_disable;
>  
> -	ov7670_s_power(sd, 1);
> +	ov7670_power_on(sd);
>  
>  	/* Make sure it's an ov7670 */
>  	ret = ov7670_detect(sd);
> @@ -1929,7 +1967,7 @@ static int ov7670_probe(struct i2c_client *client,
>  hdl_free:
>  	v4l2_ctrl_handler_free(&info->hdl);
>  power_off:
> -	ov7670_s_power(sd, 0);
> +	ov7670_power_off(sd);
>  clk_disable:
>  	clk_disable_unprepare(info->clk);
>  	return ret;
> @@ -1945,7 +1983,7 @@ static int ov7670_remove(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(&info->hdl);
>  	clk_disable_unprepare(info->clk);
>  	media_entity_cleanup(&info->sd.entity);
> -	ov7670_s_power(sd, 0);
> +	ov7670_power_off(sd);
>  	return 0;
>  }
>  

Could you consider instead switching to runtime PM? A few drivers such as
the ov2685 driver does that already.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 01/14] media: ov7670: split register setting from set_fmt() logic

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 4681 bytes --]

Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:06AM +0100, Lubomir Rintel wrote:
> This will allow us to restore the last set format after the device returns
> from a power off.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v2:
> - This patch was added to the series
>
>  drivers/media/i2c/ov7670.c | 80 ++++++++++++++++++++++----------------
>  1 file changed, 46 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index bc68a3a5b4ec..ee2302fbdeee 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -240,6 +240,7 @@ struct ov7670_info {
>  	};
>  	struct v4l2_mbus_framefmt format;
>  	struct ov7670_format_struct *fmt;  /* Current format */
> +	struct ov7670_win_size *wsize;
>  	struct clk *clk;
>  	struct gpio_desc *resetb_gpio;
>  	struct gpio_desc *pwdn_gpio;
> @@ -1003,48 +1004,20 @@ static int ov7670_try_fmt_internal(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> -/*
> - * Set a format.
> - */
> -static int ov7670_set_fmt(struct v4l2_subdev *sd,
> -		struct v4l2_subdev_pad_config *cfg,
> -		struct v4l2_subdev_format *format)
> +static int ov7670_apply_fmt(struct v4l2_subdev *sd)
>  {
> -	struct ov7670_format_struct *ovfmt;
> -	struct ov7670_win_size *wsize;
>  	struct ov7670_info *info = to_state(sd);
> -#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> -	struct v4l2_mbus_framefmt *mbus_fmt;
> -#endif
> +	struct ov7670_win_size *wsize = info->wsize;
>  	unsigned char com7, com10 = 0;
>  	int ret;
>
> -	if (format->pad)
> -		return -EINVAL;
> -
> -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
> -		if (ret)
> -			return ret;
> -#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> -		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);
> -		*mbus_fmt = format->format;
> -		return 0;
> -#else
> -		return -ENOTTY;
> -#endif
> -	}
> -
> -	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt, &wsize);
> -	if (ret)
> -		return ret;
>  	/*
>  	 * COM7 is a pain in the ass, it doesn't like to be read then
>  	 * quickly written afterward.  But we have everything we need
>  	 * to set it absolutely here, as long as the format-specific
>  	 * register sets list it first.
>  	 */
> -	com7 = ovfmt->regs[0].value;
> +	com7 = info->fmt->regs[0].value;
>  	com7 |= wsize->com7_bit;
>  	ret = ov7670_write(sd, REG_COM7, com7);
>  	if (ret)
> @@ -1066,7 +1039,7 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  	/*
>  	 * Now write the rest of the array.  Also store start/stops
>  	 */
> -	ret = ov7670_write_array(sd, ovfmt->regs + 1);
> +	ret = ov7670_write_array(sd, info->fmt->regs + 1);
>  	if (ret)
>  		return ret;
>
> @@ -1081,8 +1054,6 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  			return ret;
>  	}
>
> -	info->fmt = ovfmt;
> -
>  	/*
>  	 * If we're running RGB565, we must rewrite clkrc after setting
>  	 * the other parameters or the image looks poor.  If we're *not*
> @@ -1100,6 +1071,46 @@ static int ov7670_set_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>
> +/*
> + * Set a format.
> + */
> +static int ov7670_set_fmt(struct v4l2_subdev *sd,
> +		struct v4l2_subdev_pad_config *cfg,
> +		struct v4l2_subdev_format *format)
> +{
> +	struct ov7670_info *info = to_state(sd);
> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> +	struct v4l2_mbus_framefmt *mbus_fmt;
> +#endif
> +	int ret;
> +
> +	if (format->pad)
> +		return -EINVAL;
> +
> +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> +		ret = ov7670_try_fmt_internal(sd, &format->format, NULL, NULL);
> +		if (ret)
> +			return ret;
> +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> +		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format->pad);

This #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API seems to be quite in some
drivers... Maybe stubs should be defined in v4l2-subdev.h.

Anway, that's unrealted, the patch seems fine to me:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> +		*mbus_fmt = format->format;
> +		return 0;
> +#else
> +		return -ENOTTY;
> +#endif
> +	}
> +
> +	ret = ov7670_try_fmt_internal(sd, &format->format, &info->fmt, &info->wsize);
> +	if (ret)
> +		return ret;
> +
> +	ret = ov7670_apply_fmt(sd);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int ov7670_get_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_pad_config *cfg,
>  			  struct v4l2_subdev_format *format)
> @@ -1847,6 +1858,7 @@ static int ov7670_probe(struct i2c_client *client,
>
>  	info->devtype = &ov7670_devdata[id->driver_data];
>  	info->fmt = &ov7670_formats[0];
> +	info->wsize = &info->devtype->win_sizes[0];
>
>  	ov7670_get_default_format(sd, &info->format);
>
> --
> 2.19.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 05/14] media: dt-bindings: marvell,mmp2-ccic: Add Marvell MMP2 camera

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:10AM +0100, Lubomir Rintel wrote:
> Add Marvell MMP2 camera host interface.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v2:
> - Added #clock-cells, clock-names, port
>
>  .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
>
> diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> new file mode 100644
> index 000000000000..e5e8ca90e7f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> @@ -0,0 +1,37 @@
> +Marvell MMP2 camera host interface
> +
> +Required properties:
> + - compatible: Should be "marvell,mmp2-ccic"
> + - reg: register base and size
> + - interrupts: the interrupt number
> + - #clock-cells: must be 0
> + - any required generic properties defined in video-interfaces.txt

I don't think video-interfaces applies here. It described bindings to
be used for endpoint and port nodes.

> +
> +Optional properties:
> + - clocks: input clock (see clock-bindings.txt)

What do you think of:
"reference to the input clock as specified by clock-bindings.txt"

> + - clock-names: names of the clocks used, may include "axi", "func" and
> +                "phy"

"may include" is abit vague. Which clock should the interface be
powered from, and in which case?

> + - clock-output-names: should contain the name of the clock driving the
> +                       sensor master clock MCLK

This is a property for the clock provider part, and I will just list
the only clock this interfaces provides here:

    - clock-output-names: Optional clock source for sensors. Shall be "mclk".

See a comment on patch 14 on the use of the clock provider part.

> +
> +Required subnodes:
> + - port: the parallel bus interface port with a single endpoint linked to
> +         the sensor's endpoint as described in video-interfaces.txt
> +
> +Example:
> +
> +	camera0: camera@d420a000 {
> +		compatible = "marvell,mmp2-ccic";
> +		reg = <0xd420a000 0x800>;
> +		interrupts = <42>;
> +		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
> +		clock-names = "axi";
> +		#clock-cells = <0>;
> +		clock-output-names = "mclk";
> +
> +		port {
> +			camera0_0: endpoint {
> +				remote-endpoint = <&ov7670_0>;

I'm debated, your sensor does not support configuring the parallel bus,
that's fine, but as "bindings describe hardware" shouldn't you list
here the bus configurations the HW interface supports and list their default
values? Or there are none for real in this platform?

Thanks
  j


> +			};
> +		};
> +	};
> --
> 2.19.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 14/14] [media] marvell-ccic: provide a clock for the sensor

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 19555 bytes --]

HI Lubomir,

On Tue, Nov 20, 2018 at 11:03:19AM +0100, Lubomir Rintel wrote:
> The sensor needs the MCLK clock running when it's being probed. On
> platforms where the sensor is instantiated from a DT (MMP2) it is going
> to happen asynchronously.
>
> Therefore, the current modus operandi, where the bridge driver fiddles
> with the sensor power and clock itself is not going to fly. As the comments
> wisely note, this doesn't even belong there.
>
> Luckily, the ov7670 driver is already able to control its power and
> reset lines, we can just drop the MMP platform glue altogether.
>
> It also requests the clock via the standard clock subsystem. Good -- let's
> set up a clock instance so that the sensor can ask us to enable the clock.
> Note that this is pretty dumb at the moment: the clock is hardwired to a
> particular frequency and parent. It was always the case.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
>
> ---
> Changes since v1:
> - [kbuild/ia64] depend on COMMON_CLK.
> - [smatch] fix bad return in mcam_v4l_open() leading to lock not getting
>   released on error.
>
>  drivers/media/platform/marvell-ccic/Kconfig   |   2 +
>  .../media/platform/marvell-ccic/cafe-driver.c |   9 +-
>  .../media/platform/marvell-ccic/mcam-core.c   | 156 +++++++++++++++---
>  .../media/platform/marvell-ccic/mcam-core.h   |   3 +
>  .../media/platform/marvell-ccic/mmp-driver.c  | 152 ++---------------
>  .../linux/platform_data/media/mmp-camera.h    |   2 -
>  6 files changed, 161 insertions(+), 163 deletions(-)
>
> diff --git a/drivers/media/platform/marvell-ccic/Kconfig b/drivers/media/platform/marvell-ccic/Kconfig
> index cf12e077203a..3e12eb25740a 100644
> --- a/drivers/media/platform/marvell-ccic/Kconfig
> +++ b/drivers/media/platform/marvell-ccic/Kconfig
> @@ -1,6 +1,7 @@
>  config VIDEO_CAFE_CCIC
>  	tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
>  	depends on PCI && I2C && VIDEO_V4L2
> +	depends on COMMON_CLK
>  	select VIDEO_OV7670
>  	select VIDEOBUF2_VMALLOC
>  	select VIDEOBUF2_DMA_CONTIG
> @@ -14,6 +15,7 @@ config VIDEO_MMP_CAMERA
>  	tristate "Marvell Armada 610 integrated camera controller support"
>  	depends on I2C && VIDEO_V4L2
>  	depends on ARCH_MMP || COMPILE_TEST
> +	depends on COMMON_CLK
>  	select VIDEO_OV7670
>  	select I2C_GPIO
>  	select VIDEOBUF2_VMALLOC
> diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c b/drivers/media/platform/marvell-ccic/cafe-driver.c
> index 658294d319c0..0e712bb941ba 100644
> --- a/drivers/media/platform/marvell-ccic/cafe-driver.c
> +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
> @@ -33,6 +33,7 @@
>  #include <linux/wait.h>
>  #include <linux/delay.h>
>  #include <linux/io.h>
> +#include <linux/clkdev.h>
>
>  #include "mcam-core.h"
>
> @@ -533,11 +534,10 @@ static int cafe_pci_probe(struct pci_dev *pdev,
>  		goto out_iounmap;
>
>  	/*
> -	 * Initialize the controller and leave it powered up.  It will
> -	 * stay that way until the sensor driver shows up.
> +	 * Initialize the controller.
>  	 */
>  	cafe_ctlr_init(mcam);
> -	cafe_ctlr_power_up(mcam);
> +
>  	/*
>  	 * Set up I2C/SMBUS communications.  We have to drop the mutex here
>  	 * because the sensor could attach in this call chain, leading to
> @@ -555,6 +555,9 @@ static int cafe_pci_probe(struct pci_dev *pdev,
>  	if (ret)
>  		goto out_smbus_shutdown;
>
> +	clkdev_create(mcam->mclk, "xclk", "%d-%04x",
> +		i2c_adapter_id(cam->i2c_adapter), ov7670_info.addr);
> +
>  	if (i2c_new_device(cam->i2c_adapter, &ov7670_info)) {
>  		cam->registered = 1;
>  		return 0;
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
> index 87812b7287f0..982fbac6472d 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
> @@ -22,6 +22,7 @@
>  #include <linux/vmalloc.h>
>  #include <linux/io.h>
>  #include <linux/clk.h>
> +#include <linux/clk-provider.h>
>  #include <linux/videodev2.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ioctl.h>
> @@ -303,9 +304,6 @@ static void mcam_enable_mipi(struct mcam_camera *mcam)
>  		 */
>  		mcam_reg_write(mcam, REG_CSI2_CTRL0,
>  			CSI2_C0_MIPI_EN | CSI2_C0_ACT_LANE(mcam->lane));
> -		mcam_reg_write(mcam, REG_CLKCTRL,
> -			(mcam->mclk_src << 29) | mcam->mclk_div);
> -
>  		mcam->mipi_enabled = true;
>  	}
>  }
> @@ -846,11 +844,6 @@ static void mcam_ctlr_init(struct mcam_camera *cam)
>  	 * but it's good to be sure.
>  	 */
>  	mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE);
> -	/*
> -	 * Clock the sensor appropriately.  Controller clock should
> -	 * be 48MHz, sensor "typical" value is half that.
> -	 */
> -	mcam_reg_write_mask(cam, REG_CLKCTRL, 2, CLK_DIV_MASK);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
>  }
>
> @@ -898,14 +891,15 @@ static int mcam_ctlr_power_up(struct mcam_camera *cam)
>  	int ret;
>
>  	spin_lock_irqsave(&cam->dev_lock, flags);
> -	ret = cam->plat_power_up(cam);
> -	if (ret) {
> -		spin_unlock_irqrestore(&cam->dev_lock, flags);
> -		return ret;
> +	if (cam->plat_power_up) {
> +		ret = cam->plat_power_up(cam);
> +		if (ret) {
> +			spin_unlock_irqrestore(&cam->dev_lock, flags);
> +			return ret;
> +		}
>  	}
>  	mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
> -	msleep(5); /* Just to be sure */
>  	return 0;
>  }
>
> @@ -920,10 +914,101 @@ static void mcam_ctlr_power_down(struct mcam_camera *cam)
>  	 * power down routine.
>  	 */
>  	mcam_reg_set_bit(cam, REG_CTRL1, C1_PWRDWN);
> -	cam->plat_power_down(cam);
> +	if (cam->plat_power_down)
> +		cam->plat_power_down(cam);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
>  }
>
> +/* ---------------------------------------------------------------------- */
> +/*
> + * Controller clocks.
> + */
> +static void mcam_clk_enable(struct mcam_camera *mcam)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < NR_MCAM_CLK; i++) {
> +		if (!IS_ERR(mcam->clk[i]))
> +			clk_prepare_enable(mcam->clk[i]);
> +	}
> +}
> +
> +static void mcam_clk_disable(struct mcam_camera *mcam)
> +{
> +	int i;
> +
> +	for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
> +		if (!IS_ERR(mcam->clk[i]))
> +			clk_disable_unprepare(mcam->clk[i]);
> +	}
> +}
> +
> +/* ---------------------------------------------------------------------- */
> +/*
> + * Master sensor clock.
> + */
> +static int mclk_prepare(struct clk_hw *hw)
> +{
> +	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
> +
> +	clk_prepare(cam->clk[0]);
> +	return 0;
> +}
> +
> +static void mclk_unprepare(struct clk_hw *hw)
> +{
> +	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
> +
> +	clk_unprepare(cam->clk[0]);
> +}
> +
> +static int mclk_enable(struct clk_hw *hw)
> +{
> +	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
> +	int mclk_src;
> +	int mclk_div;
> +
> +	/*
> +	 * Clock the sensor appropriately.  Controller clock should
> +	 * be 48MHz, sensor "typical" value is half that.
> +	 */
> +	if (cam->bus_type == V4L2_MBUS_CSI2_DPHY) {
> +		mclk_src = cam->mclk_src;
> +		mclk_div = cam->mclk_div;
> +	} else {
> +		mclk_src = 3;
> +		mclk_div = 2;
> +	}
> +
> +	clk_enable(cam->clk[0]);
> +	mcam_reg_write(cam, REG_CLKCTRL, (mclk_src << 29) | mclk_div);
> +	mcam_ctlr_power_up(cam);
> +
> +	return 0;
> +}
> +
> +static void mclk_disable(struct clk_hw *hw)
> +{
> +	struct mcam_camera *cam = container_of(hw, struct mcam_camera, mclk_hw);
> +
> +	mcam_ctlr_power_down(cam);
> +	clk_disable(cam->clk[0]);
> +}
> +
> +static unsigned long mclk_recalc_rate(struct clk_hw *hw,
> +				unsigned long parent_rate)
> +{
> +	return 48000000;
> +}
> +
> +static const struct clk_ops mclk_ops = {
> +	.prepare = mclk_prepare,
> +	.unprepare = mclk_unprepare,
> +	.enable = mclk_enable,
> +	.disable = mclk_disable,
> +	.recalc_rate = mclk_recalc_rate,
> +};
> +
>  /* -------------------------------------------------------------------- */
>  /*
>   * Communications with the sensor.
> @@ -948,7 +1033,6 @@ static int mcam_cam_init(struct mcam_camera *cam)
>  	ret = __mcam_cam_reset(cam);
>  	/* Get/set parameters? */
>  	cam->state = S_IDLE;
> -	mcam_ctlr_power_down(cam);
>  	return ret;
>  }
>
> @@ -1584,9 +1668,10 @@ static int mcam_v4l_open(struct file *filp)
>  	if (ret)
>  		goto out;
>  	if (v4l2_fh_is_singular_file(filp)) {
> -		ret = mcam_ctlr_power_up(cam);
> +		ret = sensor_call(cam, core, s_power, 1);
>  		if (ret)
>  			goto out;
> +		mcam_clk_enable(cam);
>  		__mcam_cam_reset(cam);
>  		mcam_set_config_needed(cam, 1);
>  	}
> @@ -1608,7 +1693,8 @@ static int mcam_v4l_release(struct file *filp)
>  	_vb2_fop_release(filp, NULL);
>  	if (last_open) {
>  		mcam_disable_mipi(cam);
> -		mcam_ctlr_power_down(cam);
> +		sensor_call(cam, core, s_power, 0);
> +		mcam_clk_disable(cam);
>  		if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read)
>  			mcam_free_dma_bufs(cam);
>  	}
> @@ -1806,6 +1892,7 @@ static const struct v4l2_async_notifier_operations mccic_notify_ops = {
>
>  int mccic_register(struct mcam_camera *cam)
>  {
> +	struct clk_init_data mclk_init = { };
>  	int ret;
>
>  	/*
> @@ -1838,7 +1925,10 @@ int mccic_register(struct mcam_camera *cam)
>  	mcam_set_config_needed(cam, 1);
>  	cam->pix_format = mcam_def_pix_format;
>  	cam->mbus_code = mcam_def_mbus_code;
> -	mcam_ctlr_init(cam);
> +
> +	mcam_clk_enable(cam);

Am I mis-interpreting the clock bindings, or here you expose a clock
source, and sensors are supposed to reference it if they need to. It
is then the sensor driver that by calling clk_prepare_enable() on the
referenced clock triggers the call of the 'enable' function. It seems
to me that here you have exposed a clock provider, but the provider
itself enables its clocks... Am I confused?

Thanks
   j

> +	mcam_ctlr_init(cam); // XXX?
> +	mcam_clk_disable(cam);
>
>  	/*
>  	 * Register sensor notifier.
> @@ -1857,6 +1947,26 @@ int mccic_register(struct mcam_camera *cam)
>  		goto out;
>  	}
>
> +	/*
> +	 * Register sensor master clock.
> +	 */
> +	mclk_init.parent_names = NULL;
> +	mclk_init.num_parents = 0;
> +	mclk_init.ops = &mclk_ops;
> +	mclk_init.name = "mclk";
> +
> +	of_property_read_string(cam->dev->of_node, "clock-output-names",
> +							&mclk_init.name);
> +
> +	cam->mclk_hw.init = &mclk_init;
> +
> +	cam->mclk = devm_clk_register(cam->dev, &cam->mclk_hw);
> +	if (IS_ERR(cam->mclk)) {
> +		ret = PTR_ERR(cam->mclk);
> +		dev_err(cam->dev, "can't register clock\n");
> +		goto out;
> +	}
> +
>  	/*
>  	 * If so requested, try to get our DMA buffers now.
>  	 */
> @@ -1884,7 +1994,7 @@ void mccic_shutdown(struct mcam_camera *cam)
>  	 */
>  	if (!list_empty(&cam->vdev.fh_list)) {
>  		cam_warn(cam, "Removing a device with users!\n");
> -		mcam_ctlr_power_down(cam);
> +		sensor_call(cam, core, s_power, 0);
>  	}
>  	if (cam->buffer_mode == B_vmalloc)
>  		mcam_free_dma_bufs(cam);
> @@ -1906,7 +2016,8 @@ void mccic_suspend(struct mcam_camera *cam)
>  		enum mcam_state cstate = cam->state;
>
>  		mcam_ctlr_stop_dma(cam);
> -		mcam_ctlr_power_down(cam);
> +		sensor_call(cam, core, s_power, 0);
> +		mcam_clk_disable(cam);
>  		cam->state = cstate;
>  	}
>  	mutex_unlock(&cam->s_mutex);
> @@ -1919,14 +2030,15 @@ int mccic_resume(struct mcam_camera *cam)
>
>  	mutex_lock(&cam->s_mutex);
>  	if (!list_empty(&cam->vdev.fh_list)) {
> -		ret = mcam_ctlr_power_up(cam);
> +		mcam_clk_enable(cam);
> +		ret = sensor_call(cam, core, s_power, 1);
>  		if (ret) {
>  			mutex_unlock(&cam->s_mutex);
>  			return ret;
>  		}
>  		__mcam_cam_reset(cam);
>  	} else {
> -		mcam_ctlr_power_down(cam);
> +		sensor_call(cam, core, s_power, 0);
>  	}
>  	mutex_unlock(&cam->s_mutex);
>
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
> index 4a72213aca1a..2e3a7567a76a 100644
> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
> @@ -8,6 +8,7 @@
>  #define _MCAM_CORE_H
>
>  #include <linux/list.h>
> +#include <linux/clk-provider.h>
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-dev.h>
> @@ -125,6 +126,8 @@ struct mcam_camera {
>
>  	/* clock tree support */
>  	struct clk *clk[NR_MCAM_CLK];
> +	struct clk_hw mclk_hw;
> +	struct clk *mclk;
>
>  	/*
>  	 * Callbacks from the core to the platform code.
> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
> index efbffb06e25c..7e0783dc9152 100644
> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
> @@ -22,9 +22,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> -#include <linux/gpio.h>
>  #include <linux/io.h>
> -#include <linux/delay.h>
>  #include <linux/list.h>
>  #include <linux/pm.h>
>  #include <linux/clk.h>
> @@ -38,7 +36,6 @@ MODULE_LICENSE("GPL");
>  static char *mcam_clks[] = {"axi", "func", "phy"};
>
>  struct mmp_camera {
> -	void __iomem *power_regs;
>  	struct platform_device *pdev;
>  	struct mcam_camera mcam;
>  	struct list_head devlist;
> @@ -94,94 +91,6 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev)
>  	return NULL;
>  }
>
> -
> -
> -
> -/*
> - * Power-related registers; this almost certainly belongs
> - * somewhere else.
> - *
> - * ARMADA 610 register manual, sec 7.2.1, p1842.
> - */
> -#define CPU_SUBSYS_PMU_BASE	0xd4282800
> -#define REG_CCIC_DCGCR		0x28	/* CCIC dyn clock gate ctrl reg */
> -#define REG_CCIC_CRCR		0x50	/* CCIC clk reset ctrl reg	*/
> -
> -static void mcam_clk_enable(struct mcam_camera *mcam)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < NR_MCAM_CLK; i++) {
> -		if (!IS_ERR(mcam->clk[i]))
> -			clk_prepare_enable(mcam->clk[i]);
> -	}
> -}
> -
> -static void mcam_clk_disable(struct mcam_camera *mcam)
> -{
> -	int i;
> -
> -	for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
> -		if (!IS_ERR(mcam->clk[i]))
> -			clk_disable_unprepare(mcam->clk[i]);
> -	}
> -}
> -
> -/*
> - * Power control.
> - */
> -static void mmpcam_power_up_ctlr(struct mmp_camera *cam)
> -{
> -	iowrite32(0x3f, cam->power_regs + REG_CCIC_DCGCR);
> -	iowrite32(0x3805b, cam->power_regs + REG_CCIC_CRCR);
> -	mdelay(1);
> -}
> -
> -static int mmpcam_power_up(struct mcam_camera *mcam)
> -{
> -	struct mmp_camera *cam = mcam_to_cam(mcam);
> -	struct mmp_camera_platform_data *pdata;
> -
> -/*
> - * Turn on power and clocks to the controller.
> - */
> -	mmpcam_power_up_ctlr(cam);
> -	mcam_clk_enable(mcam);
> -/*
> - * Provide power to the sensor.
> - */
> -	mcam_reg_write(mcam, REG_CLKCTRL, 0x60000002);
> -	pdata = cam->pdev->dev.platform_data;
> -	gpio_set_value(pdata->sensor_power_gpio, 1);
> -	mdelay(5);
> -	mcam_reg_clear_bit(mcam, REG_CTRL1, 0x10000000);
> -	gpio_set_value(pdata->sensor_reset_gpio, 0); /* reset is active low */
> -	mdelay(5);
> -	gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
> -	mdelay(5);
> -
> -	return 0;
> -}
> -
> -static void mmpcam_power_down(struct mcam_camera *mcam)
> -{
> -	struct mmp_camera *cam = mcam_to_cam(mcam);
> -	struct mmp_camera_platform_data *pdata;
> -/*
> - * Turn off clocks and set reset lines
> - */
> -	iowrite32(0, cam->power_regs + REG_CCIC_DCGCR);
> -	iowrite32(0, cam->power_regs + REG_CCIC_CRCR);
> -/*
> - * Shut down the sensor.
> - */
> -	pdata = cam->pdev->dev.platform_data;
> -	gpio_set_value(pdata->sensor_power_gpio, 0);
> -	gpio_set_value(pdata->sensor_reset_gpio, 0);
> -
> -	mcam_clk_disable(mcam);
> -}
> -
>  /*
>   * calc the dphy register values
>   * There are three dphy registers being used.
> @@ -327,8 +236,6 @@ static int mmpcam_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&cam->devlist);
>
>  	mcam = &cam->mcam;
> -	mcam->plat_power_up = mmpcam_power_up;
> -	mcam->plat_power_down = mmpcam_power_down;
>  	mcam->calc_dphy = mmpcam_calc_dphy;
>  	mcam->dev = &pdev->dev;
>  	pdata = pdev->dev.platform_data;
> @@ -366,33 +273,6 @@ static int mmpcam_probe(struct platform_device *pdev)
>  	if (IS_ERR(mcam->regs))
>  		return PTR_ERR(mcam->regs);
>  	mcam->regs_size = resource_size(res);
> -	/*
> -	 * Power/clock memory is elsewhere; get it too.  Perhaps this
> -	 * should really be managed outside of this driver?
> -	 */
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
> -	if (IS_ERR(cam->power_regs))
> -		return PTR_ERR(cam->power_regs);
> -	/*
> -	 * Sensor GPIO pins.
> -	 */
> -	ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio,
> -							"cam-power");
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't get sensor power gpio %d",
> -				pdata->sensor_power_gpio);
> -		return ret;
> -	}
> -	gpio_direction_output(pdata->sensor_power_gpio, 0);
> -	ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio,
> -							"cam-reset");
> -	if (ret) {
> -		dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
> -				pdata->sensor_reset_gpio);
> -		return ret;
> -	}
> -	gpio_direction_output(pdata->sensor_reset_gpio, 0);
>
>  	mcam_init_clk(mcam);
>
> @@ -410,14 +290,21 @@ static int mmpcam_probe(struct platform_device *pdev)
>  	fwnode_handle_put(ep);
>
>  	/*
> -	 * Power the device up and hand it off to the core.
> +	 * Register the device with the core.
>  	 */
> -	ret = mmpcam_power_up(mcam);
> -	if (ret)
> -		return ret;
>  	ret = mccic_register(mcam);
>  	if (ret)
> -		goto out_power_down;
> +		return ret;
> +
> +	/*
> +	 * Add OF clock provider.
> +	 */
> +	ret = of_clk_add_provider(pdev->dev.of_node, of_clk_src_simple_get,
> +								mcam->mclk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "can't add DT clock provider\n");
> +		goto out;
> +	}
>
>  	/*
>  	 * Finally, set up our IRQ now that the core is ready to
> @@ -426,7 +313,7 @@ static int mmpcam_probe(struct platform_device *pdev)
>  	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>  	if (res == NULL) {
>  		ret = -ENODEV;
> -		goto out_unregister;
> +		goto out;
>  	}
>  	cam->irq = res->start;
>  	ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED,
> @@ -436,10 +323,10 @@ static int mmpcam_probe(struct platform_device *pdev)
>  		return 0;
>  	}
>
> -out_unregister:
> +out:
> +	fwnode_handle_put(mcam->asd.match.fwnode);
>  	mccic_shutdown(mcam);
> -out_power_down:
> -	mmpcam_power_down(mcam);
> +
>  	return ret;
>  }
>
> @@ -450,7 +337,6 @@ static int mmpcam_remove(struct mmp_camera *cam)
>
>  	mmpcam_remove_device(cam);
>  	mccic_shutdown(mcam);
> -	mmpcam_power_down(mcam);
>  	return 0;
>  }
>
> @@ -482,12 +368,6 @@ static int mmpcam_resume(struct platform_device *pdev)
>  {
>  	struct mmp_camera *cam = mmpcam_find_device(pdev);
>
> -	/*
> -	 * Power up unconditionally just in case the core tries to
> -	 * touch a register even if nothing was active before; trust
> -	 * me, it's better this way.
> -	 */
> -	mmpcam_power_up_ctlr(cam);
>  	return mccic_resume(&cam->mcam);
>  }
>
> diff --git a/include/linux/platform_data/media/mmp-camera.h b/include/linux/platform_data/media/mmp-camera.h
> index c573ebc40035..53adaab64f28 100644
> --- a/include/linux/platform_data/media/mmp-camera.h
> +++ b/include/linux/platform_data/media/mmp-camera.h
> @@ -12,8 +12,6 @@ enum dphy3_algo {
>  };
>
>  struct mmp_camera_platform_data {
> -	int sensor_power_gpio;
> -	int sensor_reset_gpio;
>  	enum v4l2_mbus_type bus_type;
>  	int mclk_src;	/* which clock source the MCLK derives from */
>  	int mclk_div;	/* Clock Divider Value for MCLK */
> --
> 2.19.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 05/14] media: dt-bindings: marvell,mmp2-ccic: Add Marvell MMP2 camera

[Sakari Ailus]

Hi Lubomir,

On Tue, Nov 20, 2018 at 11:03:10AM +0100, Lubomir Rintel wrote:
> Add Marvell MMP2 camera host interface.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v2:
> - Added #clock-cells, clock-names, port
> 
>  .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> new file mode 100644
> index 000000000000..e5e8ca90e7f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> @@ -0,0 +1,37 @@
> +Marvell MMP2 camera host interface
> +
> +Required properties:
> + - compatible: Should be "marvell,mmp2-ccic"
> + - reg: register base and size
> + - interrupts: the interrupt number
> + - #clock-cells: must be 0
> + - any required generic properties defined in video-interfaces.txt

Could you document what is relevant for the hardware? There are quite a few
properties documented there...

> +
> +Optional properties:
> + - clocks: input clock (see clock-bindings.txt)
> + - clock-names: names of the clocks used, may include "axi", "func" and
> +                "phy"
> + - clock-output-names: should contain the name of the clock driving the
> +                       sensor master clock MCLK
> +
> +Required subnodes:
> + - port: the parallel bus interface port with a single endpoint linked to
> +         the sensor's endpoint as described in video-interfaces.txt

Please use the full path to video-interfaces.txt. Same above, as well as
for clock-bindings.txt.

Are there endpoint properties that are applicable for the hardware?

> +
> +Example:
> +
> +	camera0: camera@d420a000 {
> +		compatible = "marvell,mmp2-ccic";
> +		reg = <0xd420a000 0x800>;
> +		interrupts = <42>;
> +		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
> +		clock-names = "axi";
> +		#clock-cells = <0>;
> +		clock-output-names = "mclk";
> +
> +		port {
> +			camera0_0: endpoint {
> +				remote-endpoint = <&ov7670_0>;
> +			};
> +		};
> +	};

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v3 03/14] media: ov7670: hook s_power onto v4l2 core

[Lubomir Rintel]

On Thu, 2018-11-22 at 14:21 +0200, Sakari Ailus wrote:
> Hi Lubomir,
> 
> On Tue, Nov 20, 2018 at 11:03:08AM +0100, Lubomir Rintel wrote:
> > The commit 71862f63f351 ("media: ov7670: Add the ov7670_s_power function")
> > added a power control routing. However, it was not good enough to use as
> > a s_power() callback: it merely flipped on the power GPIOs without
> > restoring the register settings.
> > 
> > Fix this now and register an actual power callback.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v2:
> > - Restore the controls, format and frame rate on power on
> > 
> >  drivers/media/i2c/ov7670.c | 50 +++++++++++++++++++++++++++++++++-----
> >  1 file changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index ead0c360df33..cbaab60aaaac 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -242,6 +242,7 @@ struct ov7670_info {
> >  	struct ov7670_format_struct *fmt;  /* Current format */
> >  	struct ov7670_win_size *wsize;
> >  	struct clk *clk;
> > +	int on;
> >  	struct gpio_desc *resetb_gpio;
> >  	struct gpio_desc *pwdn_gpio;
> >  	unsigned int mbus_config;	/* Media bus configuration flags */
> > @@ -1615,19 +1616,54 @@ static int ov7670_s_register(struct v4l2_subdev *sd, const struct v4l2_dbg_regis
> >  }
> >  #endif
> >  
> > -static int ov7670_s_power(struct v4l2_subdev *sd, int on)
> > +static void ov7670_power_on(struct v4l2_subdev *sd)
> >  {
> >  	struct ov7670_info *info = to_state(sd);
> >  
> > +	if (info->on)
> > +		return;
> > +
> >  	if (info->pwdn_gpio)
> > -		gpiod_set_value(info->pwdn_gpio, !on);
> > -	if (on && info->resetb_gpio) {
> > +		gpiod_set_value(info->pwdn_gpio, 0);
> > +	if (info->resetb_gpio) {
> >  		gpiod_set_value(info->resetb_gpio, 1);
> >  		usleep_range(500, 1000);
> >  		gpiod_set_value(info->resetb_gpio, 0);
> >  		usleep_range(3000, 5000);
> >  	}
> >  
> > +	info->on = true;
> > +}
> > +
> > +static void ov7670_power_off(struct v4l2_subdev *sd)
> > +{
> > +	struct ov7670_info *info = to_state(sd);
> > +
> > +	if (!info->on)
> > +		return;
> > +
> > +	if (info->pwdn_gpio)
> > +		gpiod_set_value(info->pwdn_gpio, 1);
> > +
> > +	info->on = false;
> > +}
> > +
> > +static int ov7670_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > +	struct ov7670_info *info = to_state(sd);
> > +
> > +	if (info->on == on)
> > +		return 0;
> > +
> > +	if (on) {
> > +		ov7670_power_on (sd);
> > +		ov7670_apply_fmt(sd);
> > +		ov7675_apply_framerate(sd);
> > +		v4l2_ctrl_handler_setup(&info->hdl);
> > +	} else {
> > +		ov7670_power_off (sd);
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1660,6 +1696,7 @@ static int ov7670_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> >  static const struct v4l2_subdev_core_ops ov7670_core_ops = {
> >  	.reset = ov7670_reset,
> >  	.init = ov7670_init,
> > +	.s_power = ov7670_s_power,
> >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> >  	.g_register = ov7670_g_register,
> >  	.s_register = ov7670_s_register,
> > @@ -1825,6 +1862,7 @@ static int ov7670_probe(struct i2c_client *client,
> >  		else
> >  			return ret;
> >  	}
> > +
> >  	if (info->clk) {
> >  		ret = clk_prepare_enable(info->clk);
> >  		if (ret)
> > @@ -1841,7 +1879,7 @@ static int ov7670_probe(struct i2c_client *client,
> >  	if (ret)
> >  		goto clk_disable;
> >  
> > -	ov7670_s_power(sd, 1);
> > +	ov7670_power_on(sd);
> >  
> >  	/* Make sure it's an ov7670 */
> >  	ret = ov7670_detect(sd);
> > @@ -1929,7 +1967,7 @@ static int ov7670_probe(struct i2c_client *client,
> >  hdl_free:
> >  	v4l2_ctrl_handler_free(&info->hdl);
> >  power_off:
> > -	ov7670_s_power(sd, 0);
> > +	ov7670_power_off(sd);
> >  clk_disable:
> >  	clk_disable_unprepare(info->clk);
> >  	return ret;
> > @@ -1945,7 +1983,7 @@ static int ov7670_remove(struct i2c_client *client)
> >  	v4l2_ctrl_handler_free(&info->hdl);
> >  	clk_disable_unprepare(info->clk);
> >  	media_entity_cleanup(&info->sd.entity);
> > -	ov7670_s_power(sd, 0);
> > +	ov7670_power_off(sd);
> >  	return 0;
> >  }
> >  
> 
> Could you consider instead switching to runtime PM? A few drivers such as
> the ov2685 driver does that already.

Yes, I'll take a look. Thanks for the suggestion. I didn't know such
thing exists, so it may take some time for me to grasp it though.

Take care,
Lubo

> 

Re: [PATCH v3 01/14] media: ov7670: split register setting from set_fmt() logic

[Lubomir Rintel]

On Thu, 2018-11-22 at 19:37 +0100, jacopo mondi wrote:
> Hi Lubomir,
> 
> On Tue, Nov 20, 2018 at 11:03:06AM +0100, Lubomir Rintel wrote:
> > This will allow us to restore the last set format after the device
> > returns
> > from a power off.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v2:
> > - This patch was added to the series
> > 
> >  drivers/media/i2c/ov7670.c | 80 ++++++++++++++++++++++----------
> > ------
> >  1 file changed, 46 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov7670.c
> > b/drivers/media/i2c/ov7670.c
> > index bc68a3a5b4ec..ee2302fbdeee 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -240,6 +240,7 @@ struct ov7670_info {
> >  	};
> >  	struct v4l2_mbus_framefmt format;
> >  	struct ov7670_format_struct *fmt;  /* Current format */
> > +	struct ov7670_win_size *wsize;
> >  	struct clk *clk;
> >  	struct gpio_desc *resetb_gpio;
> >  	struct gpio_desc *pwdn_gpio;
> > @@ -1003,48 +1004,20 @@ static int ov7670_try_fmt_internal(struct
> > v4l2_subdev *sd,
> >  	return 0;
> >  }
> > 
> > -/*
> > - * Set a format.
> > - */
> > -static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > -		struct v4l2_subdev_pad_config *cfg,
> > -		struct v4l2_subdev_format *format)
> > +static int ov7670_apply_fmt(struct v4l2_subdev *sd)
> >  {
> > -	struct ov7670_format_struct *ovfmt;
> > -	struct ov7670_win_size *wsize;
> >  	struct ov7670_info *info = to_state(sd);
> > -#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > -	struct v4l2_mbus_framefmt *mbus_fmt;
> > -#endif
> > +	struct ov7670_win_size *wsize = info->wsize;
> >  	unsigned char com7, com10 = 0;
> >  	int ret;
> > 
> > -	if (format->pad)
> > -		return -EINVAL;
> > -
> > -	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		ret = ov7670_try_fmt_internal(sd, &format->format,
> > NULL, NULL);
> > -		if (ret)
> > -			return ret;
> > -#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > -		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format-
> > >pad);
> > -		*mbus_fmt = format->format;
> > -		return 0;
> > -#else
> > -		return -ENOTTY;
> > -#endif
> > -	}
> > -
> > -	ret = ov7670_try_fmt_internal(sd, &format->format, &ovfmt,
> > &wsize);
> > -	if (ret)
> > -		return ret;
> >  	/*
> >  	 * COM7 is a pain in the ass, it doesn't like to be read then
> >  	 * quickly written afterward.  But we have everything we need
> >  	 * to set it absolutely here, as long as the format-specific
> >  	 * register sets list it first.
> >  	 */
> > -	com7 = ovfmt->regs[0].value;
> > +	com7 = info->fmt->regs[0].value;
> >  	com7 |= wsize->com7_bit;
> >  	ret = ov7670_write(sd, REG_COM7, com7);
> >  	if (ret)
> > @@ -1066,7 +1039,7 @@ static int ov7670_set_fmt(struct v4l2_subdev
> > *sd,
> >  	/*
> >  	 * Now write the rest of the array.  Also store start/stops
> >  	 */
> > -	ret = ov7670_write_array(sd, ovfmt->regs + 1);
> > +	ret = ov7670_write_array(sd, info->fmt->regs + 1);
> >  	if (ret)
> >  		return ret;
> > 
> > @@ -1081,8 +1054,6 @@ static int ov7670_set_fmt(struct v4l2_subdev
> > *sd,
> >  			return ret;
> >  	}
> > 
> > -	info->fmt = ovfmt;
> > -
> >  	/*
> >  	 * If we're running RGB565, we must rewrite clkrc after setting
> >  	 * the other parameters or the image looks poor.  If we're
> > *not*
> > @@ -1100,6 +1071,46 @@ static int ov7670_set_fmt(struct v4l2_subdev
> > *sd,
> >  	return 0;
> >  }
> > 
> > +/*
> > + * Set a format.
> > + */
> > +static int ov7670_set_fmt(struct v4l2_subdev *sd,
> > +		struct v4l2_subdev_pad_config *cfg,
> > +		struct v4l2_subdev_format *format)
> > +{
> > +	struct ov7670_info *info = to_state(sd);
> > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > +	struct v4l2_mbus_framefmt *mbus_fmt;
> > +#endif
> > +	int ret;
> > +
> > +	if (format->pad)
> > +		return -EINVAL;
> > +
> > +	if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > +		ret = ov7670_try_fmt_internal(sd, &format->format,
> > NULL, NULL);
> > +		if (ret)
> > +			return ret;
> > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > +		mbus_fmt = v4l2_subdev_get_try_format(sd, cfg, format-
> > >pad);
> 
> This #ifdef CONFIG_VIDEO_V4L2_SUBDEV_API seems to be quite in some
> drivers... Maybe stubs should be defined in v4l2-subdev.h.

Actually, I've also thought it's sorta ugly. I'll send out a patch that
does what you suggest.

> Anway, that's unrealted, the patch seems fine to me:
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
Lubo

> 
> Thanks
>   j
> 
> 
> > +		*mbus_fmt = format->format;
> > +		return 0;
> > +#else
> > +		return -ENOTTY;
> > +#endif
> > +	}
> > +
> > +	ret = ov7670_try_fmt_internal(sd, &format->format, &info->fmt,
> > &info->wsize);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ov7670_apply_fmt(sd);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static int ov7670_get_fmt(struct v4l2_subdev *sd,
> >  			  struct v4l2_subdev_pad_config *cfg,
> >  			  struct v4l2_subdev_format *format)
> > @@ -1847,6 +1858,7 @@ static int ov7670_probe(struct i2c_client
> > *client,
> > 
> >  	info->devtype = &ov7670_devdata[id->driver_data];
> >  	info->fmt = &ov7670_formats[0];
> > +	info->wsize = &info->devtype->win_sizes[0];
> > 
> >  	ov7670_get_default_format(sd, &info->format);
> > 
> > --
> > 2.19.1
> > 

Re: [PATCH v3 03/14] media: ov7670: hook s_power onto v4l2 core

[Sakari Ailus]

Hi Lubomir,

On Wed, Nov 28, 2018 at 12:29:33PM +0100, Lubomir Rintel wrote:
> On Thu, 2018-11-22 at 14:21 +0200, Sakari Ailus wrote:
> > Hi Lubomir,
> > 
> > On Tue, Nov 20, 2018 at 11:03:08AM +0100, Lubomir Rintel wrote:
> > > The commit 71862f63f351 ("media: ov7670: Add the ov7670_s_power function")
> > > added a power control routing. However, it was not good enough to use as
> > > a s_power() callback: it merely flipped on the power GPIOs without
> > > restoring the register settings.
> > > 
> > > Fix this now and register an actual power callback.
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > 
> > > ---
> > > Changes since v2:
> > > - Restore the controls, format and frame rate on power on
> > > 
> > >  drivers/media/i2c/ov7670.c | 50 +++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 44 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > > index ead0c360df33..cbaab60aaaac 100644
> > > --- a/drivers/media/i2c/ov7670.c
> > > +++ b/drivers/media/i2c/ov7670.c
...
> > > @@ -1945,7 +1983,7 @@ static int ov7670_remove(struct i2c_client *client)
> > >  	v4l2_ctrl_handler_free(&info->hdl);
> > >  	clk_disable_unprepare(info->clk);
> > >  	media_entity_cleanup(&info->sd.entity);
> > > -	ov7670_s_power(sd, 0);
> > > +	ov7670_power_off(sd);
> > >  	return 0;
> > >  }
> > >  
> > 
> > Could you consider instead switching to runtime PM? A few drivers such as
> > the ov2685 driver does that already.
> 
> Yes, I'll take a look. Thanks for the suggestion. I didn't know such
> thing exists, so it may take some time for me to grasp it though.

I'm be tempted to merge the ov7670 patches as they significantly improve
the driver, even if we're at the moment missing the runtime PM conversion.
It could be done later on as well.

Would that be ok for you?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 03/14] media: ov7670: hook s_power onto v4l2 core

[Lubomir Rintel]

On Thu, 2019-01-10 at 18:59 +0200, Sakari Ailus wrote:
> Hi Lubomir,
> 
> On Wed, Nov 28, 2018 at 12:29:33PM +0100, Lubomir Rintel wrote:
> > On Thu, 2018-11-22 at 14:21 +0200, Sakari Ailus wrote:
> > > Hi Lubomir,
> > > 
> > > On Tue, Nov 20, 2018 at 11:03:08AM +0100, Lubomir Rintel wrote:
> > > > The commit 71862f63f351 ("media: ov7670: Add the ov7670_s_power function")
> > > > added a power control routing. However, it was not good enough to use as
> > > > a s_power() callback: it merely flipped on the power GPIOs without
> > > > restoring the register settings.
> > > > 
> > > > Fix this now and register an actual power callback.
> > > > 
> > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > > 
> > > > ---
> > > > Changes since v2:
> > > > - Restore the controls, format and frame rate on power on
> > > > 
> > > >  drivers/media/i2c/ov7670.c | 50 +++++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 44 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > > > index ead0c360df33..cbaab60aaaac 100644
> > > > --- a/drivers/media/i2c/ov7670.c
> > > > +++ b/drivers/media/i2c/ov7670.c
> ...
> > > > @@ -1945,7 +1983,7 @@ static int ov7670_remove(struct i2c_client *client)
> > > >  	v4l2_ctrl_handler_free(&info->hdl);
> > > >  	clk_disable_unprepare(info->clk);
> > > >  	media_entity_cleanup(&info->sd.entity);
> > > > -	ov7670_s_power(sd, 0);
> > > > +	ov7670_power_off(sd);
> > > >  	return 0;
> > > >  }
> > > >  
> > > 
> > > Could you consider instead switching to runtime PM? A few drivers such as
> > > the ov2685 driver does that already.
> > 
> > Yes, I'll take a look. Thanks for the suggestion. I didn't know such
> > thing exists, so it may take some time for me to grasp it though.
> 
> I'm be tempted to merge the ov7670 patches as they significantly improve
> the driver, even if we're at the moment missing the runtime PM conversion.
> It could be done later on as well.
> 
> Would that be ok for you?

Yes, I'm fine with it. In fact, I'm happy about it.

I'd eventually like to implement the runtime PM, but it's not going to
happen before 5.1 merge window.

Cheers
Lubo

Re: [PATCH v3 02/14] media: ov7670: split register setting from set_framerate() logic

[Sakari Ailus]

On Tue, Nov 20, 2018 at 11:03:07AM +0100, Lubomir Rintel wrote:
> This will allow us to restore the last set frame rate after the device
> returns from a power off.
> 
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> 
> ---
> Changes since v2:
> - This patch was added to the series
> 
>  drivers/media/i2c/ov7670.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> index ee2302fbdeee..ead0c360df33 100644
> --- a/drivers/media/i2c/ov7670.c
> +++ b/drivers/media/i2c/ov7670.c
> @@ -810,13 +810,24 @@ static void ov7675_get_framerate(struct v4l2_subdev *sd,
>  			(4 * clkrc);
>  }
>  
> +static int ov7675_apply_framerate(struct v4l2_subdev *sd)
> +{
> +	struct ov7670_info *info = to_state(sd);
> +	int ret;
> +
> +	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ov7670_write(sd, REG_DBLV, info->pll_bypass ? DBLV_BYPASS : DBLV_X4);
> +}
> +
>  static int ov7675_set_framerate(struct v4l2_subdev *sd,
>  				 struct v4l2_fract *tpf)
>  {
>  	struct ov7670_info *info = to_state(sd);
>  	u32 clkrc;
>  	int pll_factor;
> -	int ret;
>  
>  	/*
>  	 * The formula is fps = 5/4*pixclk for YUV/RGB and
> @@ -825,19 +836,10 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
>  	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
>  	 *
>  	 */
> -	if (info->pll_bypass) {
> -		pll_factor = 1;
> -		ret = ov7670_write(sd, REG_DBLV, DBLV_BYPASS);
> -	} else {
> -		pll_factor = PLL_FACTOR;
> -		ret = ov7670_write(sd, REG_DBLV, DBLV_X4);
> -	}
> -	if (ret < 0)
> -		return ret;
> -
>  	if (tpf->numerator == 0 || tpf->denominator == 0) {
>  		clkrc = 0;
>  	} else {
> +		pll_factor = info->pll_bypass ? 1 : PLL_FACTOR;
>  		clkrc = (5 * pll_factor * info->clock_speed * tpf->numerator) /
>  			(4 * tpf->denominator);
>  		if (info->fmt->mbus_code == MEDIA_BUS_FMT_SBGGR8_1X8)
> @@ -859,11 +861,7 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
>  	/* Recalculate frame rate */
>  	ov7675_get_framerate(sd, tpf);
>  
> -	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> -	if (ret < 0)
> -		return ret;
> -
> -	return ov7670_write(sd, REG_DBLV, DBLV_X4);
> +	return ov7675_apply_framerate(sd);
>  }
>  
>  static void ov7670_get_framerate_legacy(struct v4l2_subdev *sd,

Unfortunately this one no longer applies due to Jacopo's patch "v4l2: i2c:
ov7670: Fix PLL bypass register values" in my tree. Could you resend it if
you still think it's useful, please?

My tree is here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/>

I've applied the rest there, with a trivial patch I'll send briefly.

Thanks.

-- 
Sakari Ailus

Re: [PATCH v3 02/14] media: ov7670: split register setting from set_framerate() logic

[Lubomir Rintel]

On Tue, 2019-01-15 at 01:03 +0200, Sakari Ailus wrote:
> On Tue, Nov 20, 2018 at 11:03:07AM +0100, Lubomir Rintel wrote:
> > This will allow us to restore the last set frame rate after the device
> > returns from a power off.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v2:
> > - This patch was added to the series
> > 
> >  drivers/media/i2c/ov7670.c | 30 ++++++++++++++----------------
> >  1 file changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > index ee2302fbdeee..ead0c360df33 100644
> > --- a/drivers/media/i2c/ov7670.c
> > +++ b/drivers/media/i2c/ov7670.c
> > @@ -810,13 +810,24 @@ static void ov7675_get_framerate(struct v4l2_subdev *sd,
> >  			(4 * clkrc);
> >  }
> >  
> > +static int ov7675_apply_framerate(struct v4l2_subdev *sd)
> > +{
> > +	struct ov7670_info *info = to_state(sd);
> > +	int ret;
> > +
> > +	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return ov7670_write(sd, REG_DBLV, info->pll_bypass ? DBLV_BYPASS : DBLV_X4);
> > +}
> > +
> >  static int ov7675_set_framerate(struct v4l2_subdev *sd,
> >  				 struct v4l2_fract *tpf)
> >  {
> >  	struct ov7670_info *info = to_state(sd);
> >  	u32 clkrc;
> >  	int pll_factor;
> > -	int ret;
> >  
> >  	/*
> >  	 * The formula is fps = 5/4*pixclk for YUV/RGB and
> > @@ -825,19 +836,10 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
> >  	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
> >  	 *
> >  	 */
> > -	if (info->pll_bypass) {
> > -		pll_factor = 1;
> > -		ret = ov7670_write(sd, REG_DBLV, DBLV_BYPASS);
> > -	} else {
> > -		pll_factor = PLL_FACTOR;
> > -		ret = ov7670_write(sd, REG_DBLV, DBLV_X4);
> > -	}
> > -	if (ret < 0)
> > -		return ret;
> > -
> >  	if (tpf->numerator == 0 || tpf->denominator == 0) {
> >  		clkrc = 0;
> >  	} else {
> > +		pll_factor = info->pll_bypass ? 1 : PLL_FACTOR;
> >  		clkrc = (5 * pll_factor * info->clock_speed * tpf->numerator) /
> >  			(4 * tpf->denominator);
> >  		if (info->fmt->mbus_code == MEDIA_BUS_FMT_SBGGR8_1X8)
> > @@ -859,11 +861,7 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
> >  	/* Recalculate frame rate */
> >  	ov7675_get_framerate(sd, tpf);
> >  
> > -	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	return ov7670_write(sd, REG_DBLV, DBLV_X4);
> > +	return ov7675_apply_framerate(sd);
> >  }
> >  
> >  static void ov7670_get_framerate_legacy(struct v4l2_subdev *sd,
> 
> Unfortunately this one no longer applies due to Jacopo's patch "v4l2: i2c:
> ov7670: Fix PLL bypass register values" in my tree. Could you resend it if
> you still think it's useful, please?
> 
> My tree is here:
> 
> <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
> 
> I've applied the rest there, with a trivial patch I'll send briefly.

Thank you.

It seems that you've accidentally attached the hunks of that patch to a
previous one though ("media: ov7670: split register setting from
set_fmt() logic"). There's just one conflicted line to resolve left.

I have a branch based on your tree that reverts the patch first and
then applies the two cleanly:

 git pull https://github.com/hackerspace/olpc-xo175-linux.git lr/ov7670

Feel free to pull, or, if you're fine with rebasing your tree, remove
the patch from your tree and then just pick the two.

Alternatively, if you prefer a patch by e-mail, please just let me know
when you've fixed things up in your tree.

Cheers
Lubo

> 
> Thanks.
> 

Re: [PATCH v3 02/14] media: ov7670: split register setting from set_framerate() logic

[Sakari Ailus]

On Tue, Jan 15, 2019 at 09:30:31AM +0100, Lubomir Rintel wrote:
> On Tue, 2019-01-15 at 01:03 +0200, Sakari Ailus wrote:
> > On Tue, Nov 20, 2018 at 11:03:07AM +0100, Lubomir Rintel wrote:
> > > This will allow us to restore the last set frame rate after the device
> > > returns from a power off.
> > > 
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > 
> > > ---
> > > Changes since v2:
> > > - This patch was added to the series
> > > 
> > >  drivers/media/i2c/ov7670.c | 30 ++++++++++++++----------------
> > >  1 file changed, 14 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/ov7670.c b/drivers/media/i2c/ov7670.c
> > > index ee2302fbdeee..ead0c360df33 100644
> > > --- a/drivers/media/i2c/ov7670.c
> > > +++ b/drivers/media/i2c/ov7670.c
> > > @@ -810,13 +810,24 @@ static void ov7675_get_framerate(struct v4l2_subdev *sd,
> > >  			(4 * clkrc);
> > >  }
> > >  
> > > +static int ov7675_apply_framerate(struct v4l2_subdev *sd)
> > > +{
> > > +	struct ov7670_info *info = to_state(sd);
> > > +	int ret;
> > > +
> > > +	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return ov7670_write(sd, REG_DBLV, info->pll_bypass ? DBLV_BYPASS : DBLV_X4);
> > > +}
> > > +
> > >  static int ov7675_set_framerate(struct v4l2_subdev *sd,
> > >  				 struct v4l2_fract *tpf)
> > >  {
> > >  	struct ov7670_info *info = to_state(sd);
> > >  	u32 clkrc;
> > >  	int pll_factor;
> > > -	int ret;
> > >  
> > >  	/*
> > >  	 * The formula is fps = 5/4*pixclk for YUV/RGB and
> > > @@ -825,19 +836,10 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
> > >  	 * pixclk = clock_speed / (clkrc + 1) * PLLfactor
> > >  	 *
> > >  	 */
> > > -	if (info->pll_bypass) {
> > > -		pll_factor = 1;
> > > -		ret = ov7670_write(sd, REG_DBLV, DBLV_BYPASS);
> > > -	} else {
> > > -		pll_factor = PLL_FACTOR;
> > > -		ret = ov7670_write(sd, REG_DBLV, DBLV_X4);
> > > -	}
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > >  	if (tpf->numerator == 0 || tpf->denominator == 0) {
> > >  		clkrc = 0;
> > >  	} else {
> > > +		pll_factor = info->pll_bypass ? 1 : PLL_FACTOR;
> > >  		clkrc = (5 * pll_factor * info->clock_speed * tpf->numerator) /
> > >  			(4 * tpf->denominator);
> > >  		if (info->fmt->mbus_code == MEDIA_BUS_FMT_SBGGR8_1X8)
> > > @@ -859,11 +861,7 @@ static int ov7675_set_framerate(struct v4l2_subdev *sd,
> > >  	/* Recalculate frame rate */
> > >  	ov7675_get_framerate(sd, tpf);
> > >  
> > > -	ret = ov7670_write(sd, REG_CLKRC, info->clkrc);
> > > -	if (ret < 0)
> > > -		return ret;
> > > -
> > > -	return ov7670_write(sd, REG_DBLV, DBLV_X4);
> > > +	return ov7675_apply_framerate(sd);
> > >  }
> > >  
> > >  static void ov7670_get_framerate_legacy(struct v4l2_subdev *sd,
> > 
> > Unfortunately this one no longer applies due to Jacopo's patch "v4l2: i2c:
> > ov7670: Fix PLL bypass register values" in my tree. Could you resend it if
> > you still think it's useful, please?
> > 
> > My tree is here:
> > 
> > <URL:https://git.linuxtv.org/sailus/media_tree.git/log/>
> > 
> > I've applied the rest there, with a trivial patch I'll send briefly.
> 
> Thank you.
> 
> It seems that you've accidentally attached the hunks of that patch to a
> previous one though ("media: ov7670: split register setting from
> set_fmt() logic"). There's just one conflicted line to resolve left.

Ouch.

> 
> I have a branch based on your tree that reverts the patch first and
> then applies the two cleanly:
> 
>  git pull https://github.com/hackerspace/olpc-xo175-linux.git lr/ov7670

I'll be rebasing my tree anyway; Mauro picks the patches from my pull
request, not merging it as such.

> 
> Feel free to pull, or, if you're fine with rebasing your tree, remove
> the patch from your tree and then just pick the two.
> 
> Alternatively, if you prefer a patch by e-mail, please just let me know
> when you've fixed things up in your tree.

I think it'd be easiest that I take these out of the tree and let you
resend just the ov7670 patches. Could you do that? I've updated the master
branch...

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 14/14] [media] marvell-ccic: provide a clock for the sensor

[Lubomir Rintel]

On Fri, 2018-11-23 at 08:44 +0100, jacopo mondi wrote:
> HI Lubomir,
> 
> On Tue, Nov 20, 2018 at 11:03:19AM +0100, Lubomir Rintel wrote:
> > The sensor needs the MCLK clock running when it's being probed. On
> > platforms where the sensor is instantiated from a DT (MMP2) it is going
> > to happen asynchronously.
> > 
> > Therefore, the current modus operandi, where the bridge driver fiddles
> > with the sensor power and clock itself is not going to fly. As the comments
> > wisely note, this doesn't even belong there.
> > 
> > Luckily, the ov7670 driver is already able to control its power and
> > reset lines, we can just drop the MMP platform glue altogether.
> > 
> > It also requests the clock via the standard clock subsystem. Good -- let's
> > set up a clock instance so that the sensor can ask us to enable the clock.
> > Note that this is pretty dumb at the moment: the clock is hardwired to a
> > particular frequency and parent. It was always the case.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v1:
> > - [kbuild/ia64] depend on COMMON_CLK.
> > - [smatch] fix bad return in mcam_v4l_open() leading to lock not getting
> >   released on error.
> > 
> >  drivers/media/platform/marvell-ccic/Kconfig   |   2 +
> >  .../media/platform/marvell-ccic/cafe-driver.c |   9 +-
> >  .../media/platform/marvell-ccic/mcam-core.c   | 156 +++++++++++++++---
> >  .../media/platform/marvell-ccic/mcam-core.h   |   3 +
> >  .../media/platform/marvell-ccic/mmp-driver.c  | 152 ++---------------
> >  .../linux/platform_data/media/mmp-camera.h    |   2 -
> >  6 files changed, 161 insertions(+), 163 deletions(-)
...
> >  int mccic_register(struct mcam_camera *cam)
> >  {
> > +	struct clk_init_data mclk_init = { };
> >  	int ret;
> > 
> >  	/*
> > @@ -1838,7 +1925,10 @@ int mccic_register(struct mcam_camera *cam)
> >  	mcam_set_config_needed(cam, 1);
> >  	cam->pix_format = mcam_def_pix_format;
> >  	cam->mbus_code = mcam_def_mbus_code;
> > -	mcam_ctlr_init(cam);
> > +
> > +	mcam_clk_enable(cam);
> 
> Am I mis-interpreting the clock bindings, or here you expose a clock
> source, and sensors are supposed to reference it if they need to. It
> is then the sensor driver that by calling clk_prepare_enable() on the
> referenced clock triggers the call of the 'enable' function. It seems
> to me that here you have exposed a clock provider, but the provider
> itself enables its clocks...

What mcam_clk_enable() enables is the clocks for the CCIC IP core; so
that we're able to access the registers.

(Note to self: we're enabling all the clocks here, we just need the
"axi" clock, clk_enable(cam->clk[0]), as done elsewhere, should be
sufficient.)

On the other hand, the "mclk" clock provider that is only registered
below provides the clock for the sensor itself that's provided by the
CCIC block.

>  Am I confused?

Chances are that I am. But maybe you're confusing mcam_clk_enable(cam)
(consumer, enables the CCIC's input clocks) with mclk_enable()
(provider, makes CCIC generate the clock for the sensor).

> 
> Thanks
>    j
> 
> > +	mcam_ctlr_init(cam); // XXX?

This looks like something I shouldn't have left in place though...

> > +	mcam_clk_disable(cam);
> > 
> >  	/*
> >  	 * Register sensor notifier.
> > @@ -1857,6 +1947,26 @@ int mccic_register(struct mcam_camera *cam)
> >  		goto out;
> >  	}
> > 
> > +	/*
> > +	 * Register sensor master clock.
> > +	 */
> > +	mclk_init.parent_names = NULL;
> > +	mclk_init.num_parents = 0;
> > +	mclk_init.ops = &mclk_ops;
> > +	mclk_init.name = "mclk";
> > +
> > +	of_property_read_string(cam->dev->of_node, "clock-output-names",
> > +							&mclk_init.name);
> > +
> > +	cam->mclk_hw.init = &mclk_init;
> > +
> > +	cam->mclk = devm_clk_register(cam->dev, &cam->mclk_hw);
> > +	if (IS_ERR(cam->mclk)) {
> > +		ret = PTR_ERR(cam->mclk);
> > +		dev_err(cam->dev, "can't register clock\n");
> > +		goto out;
> > +	}
> > +
> >  	/*
> >  	 * If so requested, try to get our DMA buffers now.
> >  	 */
> > @@ -1884,7 +1994,7 @@ void mccic_shutdown(struct mcam_camera *cam)
> >  	 */
> >  	if (!list_empty(&cam->vdev.fh_list)) {
> >  		cam_warn(cam, "Removing a device with users!\n");
> > -		mcam_ctlr_power_down(cam);
> > +		sensor_call(cam, core, s_power, 0);
> >  	}
> >  	if (cam->buffer_mode == B_vmalloc)
> >  		mcam_free_dma_bufs(cam);
> > @@ -1906,7 +2016,8 @@ void mccic_suspend(struct mcam_camera *cam)
> >  		enum mcam_state cstate = cam->state;
> > 
> >  		mcam_ctlr_stop_dma(cam);
> > -		mcam_ctlr_power_down(cam);
> > +		sensor_call(cam, core, s_power, 0);
> > +		mcam_clk_disable(cam);
> >  		cam->state = cstate;
> >  	}
> >  	mutex_unlock(&cam->s_mutex);
> > 
...

Cheers,
Lubo

Re: [PATCH v3 05/14] media: dt-bindings: marvell,mmp2-ccic: Add Marvell MMP2 camera

[Lubomir Rintel]

On Thu, 2018-11-22 at 21:08 +0100, jacopo mondi wrote:
> Hi Lubomir,
> 
> On Tue, Nov 20, 2018 at 11:03:10AM +0100, Lubomir Rintel wrote:
> > Add Marvell MMP2 camera host interface.
> > 
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > 
> > ---
> > Changes since v2:
> > - Added #clock-cells, clock-names, port
> > 
> >  .../bindings/media/marvell,mmp2-ccic.txt      | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> > new file mode 100644
> > index 000000000000..e5e8ca90e7f7
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/marvell,mmp2-ccic.txt
> > @@ -0,0 +1,37 @@
> > +Marvell MMP2 camera host interface
> > +
> > +Required properties:
> > + - compatible: Should be "marvell,mmp2-ccic"
> > + - reg: register base and size
> > + - interrupts: the interrupt number
> > + - #clock-cells: must be 0
> > + - any required generic properties defined in video-interfaces.txt
> 
> I don't think video-interfaces applies here. It described bindings to
> be used for endpoint and port nodes.

Yes. Will fix.

> > +
> > +Optional properties:
> > + - clocks: input clock (see clock-bindings.txt)
> 
> What do you think of:
> "reference to the input clock as specified by clock-bindings.txt"

Sounds good; I'll use that.

> > + - clock-names: names of the clocks used, may include "axi", "func" and
> > +                "phy"
> 
> "may include" is abit vague. Which clock should the interface be
> powered from, and in which case?

I did this somewhat intentionally, because the hardware documentation
is secret and the only clue about the clocks comes from what is
actually implemented in the driver.

I think I can still improve it somehow though, without too much risk of
stating things incorrectly.

> > + - clock-output-names: should contain the name of the clock driving the
> > +                       sensor master clock MCLK
> 
> This is a property for the clock provider part, and I will just list
> the only clock this interfaces provides here:
> 
>     - clock-output-names: Optional clock source for sensors. Shall be "mclk".

Thanks, will fix in next patch version.

> See a comment on patch 14 on the use of the clock provider part.
> 
> > +
> > +Required subnodes:
> > + - port: the parallel bus interface port with a single endpoint linked to
> > +         the sensor's endpoint as described in video-interfaces.txt
> > +
> > +Example:
> > +
> > +	camera0: camera@d420a000 {
> > +		compatible = "marvell,mmp2-ccic";
> > +		reg = <0xd420a000 0x800>;
> > +		interrupts = <42>;
> > +		clocks = <&soc_clocks MMP2_CLK_CCIC0>;
> > +		clock-names = "axi";
> > +		#clock-cells = <0>;
> > +		clock-output-names = "mclk";
> > +
> > +		port {
> > +			camera0_0: endpoint {
> > +				remote-endpoint = <&ov7670_0>;
> 
> I'm debated, your sensor does not support configuring the parallel bus,
> that's fine, but as "bindings describe hardware" shouldn't you list
> here the bus configurations the HW interface supports and list their default
> values? Or there are none for real in this platform?

Hard to tell, given Marvell won't publish the documentation.

The driver seems to have some rudimentary support for bus-type of
<V4L2_MBUS_CSI2_DPHY>, but I have no idea whether it's on MMP2
hardware. The 16x documentation, that may or may not be relevant, only
lists parallel formats.

I'd prefer not to take risks of making wrong guesses and don't list I
can not verify to be correct.

> Thanks
>   j

There are points above I have not responded to. I'll address them in
the next version of the patch.

Thank you
Lubo