[PATCH 0/7] Omap3isp CCP2 support

[PATCH 0/7] Omap3isp CCP2 support

[Sakari Ailus]

Hi Pavel,

I rebased the ccp2 branch and went through the patches. I didn't find
anything really alarming there; I changed one commit description of
"omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode" that had
some junk in it as well as in the last patch changed the condition in
omap3isp_csiphy_release() that was obviously wrong.

Let me know what you think.

If we merge these, is there anything still missing from plain ccp2 support?

I'd like to get Laurent's comment on these, too, plus a confirmation
nothing is broken by these on the OMAP 3 boards he uses.


Pavel Machek (3):
  omap3isp: Parse CSI1 configuration from the device tree
  omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode
  omap3isp: Return -EPROBE_DEFER if the required regulators can't be
    obtained

Sakari Ailus (4):
  omap3isp: Ignore endpoints with invalid configuration
  omap3isp: Always initialise isp and mutex for csiphy1
  omap3isp: Correctly put the last iterated endpoint fwnode always
  omap3isp: Skip CSI-2 receiver initialisation in CCP2 configuration

 drivers/media/platform/omap3isp/isp.c       | 128 ++++++++++++++++++++--------
 drivers/media/platform/omap3isp/ispccp2.c   |  12 ++-
 drivers/media/platform/omap3isp/ispcsiphy.c |  46 ++++++----
 drivers/media/platform/omap3isp/ispreg.h    |   4 +
 drivers/media/platform/omap3isp/omap3isp.h  |   1 +
 5 files changed, 140 insertions(+), 51 deletions(-)

-- 
Kind regards,
Sakari

[PATCH 1/7] omap3isp: Ignore endpoints with invalid configuration

[Sakari Ailus]

If endpoint has an invalid configuration, ignore it instead of happily
proceeding to use it nonetheless. Ignoring such an endpoint is better than
failing since there could be multiple endpoints, only some of which are
bad.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/platform/omap3isp/isp.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index db2cccb57ceb..441eba1e02eb 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2110,10 +2110,12 @@ static int isp_fwnodes_parse(struct device *dev,
 		if (!isd)
 			goto error;
 
-		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
+		if (isp_fwnode_parse(dev, fwnode, isd)) {
+			devm_kfree(dev, isd);
+			continue;
+		}
 
-		if (isp_fwnode_parse(dev, fwnode, isd))
-			goto error;
+		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
 
 		isd->asd.match.fwnode.fwnode =
 			fwnode_graph_get_remote_port_parent(fwnode);
-- 
2.11.0

[PATCH 2/7] omap3isp: Parse CSI1 configuration from the device tree

[Sakari Ailus]

From: Pavel Machek <pavel@ucw.cz>

Add support for parsing CSI1 configuration.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c      | 106 +++++++++++++++++++++--------
 drivers/media/platform/omap3isp/omap3isp.h |   1 +
 2 files changed, 80 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 441eba1e02eb..80ed5a5f862a 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2017,6 +2017,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 	struct v4l2_fwnode_endpoint vep;
 	unsigned int i;
 	int ret;
+	bool csi1 = false;
 
 	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
 	if (ret)
@@ -2045,41 +2046,92 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:
-		/* FIXME: always assume CSI-2 for now. */
+		switch (vep.bus_type) {
+		case V4L2_MBUS_CCP2:
+		case V4L2_MBUS_CSI1:
+			dev_dbg(dev, "csi1/ccp2 configuration\n");
+			csi1 = true;
+			break;
+		case V4L2_MBUS_CSI2:
+			dev_dbg(dev, "csi2 configuration\n");
+			csi1 = false;
+			break;
+		default:
+			dev_err(dev, "unsupported bus type %u\n",
+				vep.bus_type);
+			return -EINVAL;
+		}
+
 		switch (vep.base.port) {
 		case ISP_OF_PHY_CSIPHY1:
-			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
 			break;
 		case ISP_OF_PHY_CSIPHY2:
-			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
 			break;
 		}
-		buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
-		buscfg->bus.csi2.lanecfg.clk.pol =
-			vep.bus.mipi_csi2.lane_polarities[0];
-		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
-			buscfg->bus.csi2.lanecfg.clk.pol,
-			buscfg->bus.csi2.lanecfg.clk.pos);
-
-		buscfg->bus.csi2.num_data_lanes =
-			vep.bus.mipi_csi2.num_data_lanes;
-
-		for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
-			buscfg->bus.csi2.lanecfg.data[i].pos =
-				vep.bus.mipi_csi2.data_lanes[i];
-			buscfg->bus.csi2.lanecfg.data[i].pol =
-				vep.bus.mipi_csi2.lane_polarities[i + 1];
+		if (csi1) {
+			buscfg->bus.ccp2.lanecfg.clk.pos =
+				vep.bus.mipi_csi1.clock_lane;
+			buscfg->bus.ccp2.lanecfg.clk.pol =
+				vep.bus.mipi_csi1.lane_polarity[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.ccp2.lanecfg.clk.pol,
+				buscfg->bus.ccp2.lanecfg.clk.pos);
+
+			buscfg->bus.ccp2.lanecfg.data[0].pos =
+				vep.bus.mipi_csi1.data_lane;
+			buscfg->bus.ccp2.lanecfg.data[0].pol =
+				vep.bus.mipi_csi1.lane_polarity[1];
+
 			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
-				buscfg->bus.csi2.lanecfg.data[i].pol,
-				buscfg->bus.csi2.lanecfg.data[i].pos);
+				buscfg->bus.ccp2.lanecfg.data[0].pol,
+				buscfg->bus.ccp2.lanecfg.data[0].pos);
+
+			buscfg->bus.ccp2.strobe_clk_pol =
+				vep.bus.mipi_csi1.clock_inv;
+			buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;
+			buscfg->bus.ccp2.ccp2_mode =
+				vep.bus_type == V4L2_MBUS_CCP2;
+			buscfg->bus.ccp2.vp_clk_pol = 1;
+
+			buscfg->bus.ccp2.crc = 1;
+		} else {
+			buscfg->bus.csi2.lanecfg.clk.pos =
+				vep.bus.mipi_csi2.clock_lane;
+			buscfg->bus.csi2.lanecfg.clk.pol =
+				vep.bus.mipi_csi2.lane_polarities[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.csi2.lanecfg.clk.pol,
+				buscfg->bus.csi2.lanecfg.clk.pos);
+
+			buscfg->bus.csi2.num_data_lanes =
+				vep.bus.mipi_csi2.num_data_lanes;
+
+			for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
+				buscfg->bus.csi2.lanecfg.data[i].pos =
+					vep.bus.mipi_csi2.data_lanes[i];
+				buscfg->bus.csi2.lanecfg.data[i].pol =
+					vep.bus.mipi_csi2.lane_polarities[i + 1];
+				dev_dbg(dev,
+					"data lane %u polarity %u, pos %u\n", i,
+					buscfg->bus.csi2.lanecfg.data[i].pol,
+					buscfg->bus.csi2.lanecfg.data[i].pos);
+			}
+			/*
+			 * FIXME: now we assume the CRC is always there.
+			 * Implement a way to obtain this information from the
+			 * sensor. Frame descriptors, perhaps?
+			 */
+
+			buscfg->bus.csi2.crc = 1;
 		}
-
-		/*
-		 * FIXME: now we assume the CRC is always there.
-		 * Implement a way to obtain this information from the
-		 * sensor. Frame descriptors, perhaps?
-		 */
-		buscfg->bus.csi2.crc = 1;
 		break;
 
 	default:
diff --git a/drivers/media/platform/omap3isp/omap3isp.h b/drivers/media/platform/omap3isp/omap3isp.h
index 3c26f9a3f508..672a9cf5aa4d 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	unsigned int vp_clk_pol:1;
 	struct isp_csiphy_lanes_cfg lanecfg;
 };
 
-- 
2.11.0

[PATCH 3/7] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

[Sakari Ailus]

From: Pavel Machek <pavel@ucw.cz>

ISP CSI1 module needs all the bits correctly set to work.

Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispccp2.c | 7 +++++--
 drivers/media/platform/omap3isp/ispreg.h  | 4 ++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 588f67a89f79..4f8fd0c00748 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -213,14 +213,17 @@ static int ccp2_phyif_config(struct isp_ccp2_device *ccp2,
 	struct isp_device *isp = to_isp_device(ccp2);
 	u32 val;
 
-	/* CCP2B mode */
 	val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL) |
-			    ISPCCP2_CTRL_IO_OUT_SEL | ISPCCP2_CTRL_MODE;
+			    ISPCCP2_CTRL_MODE;
 	/* Data/strobe physical layer */
 	BIT_SET(val, ISPCCP2_CTRL_PHY_SEL_SHIFT, ISPCCP2_CTRL_PHY_SEL_MASK,
 		buscfg->phy_layer);
+	BIT_SET(val, ISPCCP2_CTRL_IO_OUT_SEL_SHIFT,
+		ISPCCP2_CTRL_IO_OUT_SEL_MASK, buscfg->ccp2_mode);
 	BIT_SET(val, ISPCCP2_CTRL_INV_SHIFT, ISPCCP2_CTRL_INV_MASK,
 		buscfg->strobe_clk_pol);
+	BIT_SET(val, ISPCCP2_CTRL_VP_CLK_POL_SHIFT,
+		ISPCCP2_CTRL_VP_CLK_POL_MASK, buscfg->vp_clk_pol);
 	isp_reg_writel(isp, val, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
 
 	val = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCP2, ISPCCP2_CTRL);
diff --git a/drivers/media/platform/omap3isp/ispreg.h b/drivers/media/platform/omap3isp/ispreg.h
index b5ea8da0b904..d08483919a77 100644
--- a/drivers/media/platform/omap3isp/ispreg.h
+++ b/drivers/media/platform/omap3isp/ispreg.h
@@ -87,6 +87,8 @@
 #define ISPCCP2_CTRL_PHY_SEL_MASK	0x1
 #define ISPCCP2_CTRL_PHY_SEL_SHIFT	1
 #define ISPCCP2_CTRL_IO_OUT_SEL		(1 << 2)
+#define ISPCCP2_CTRL_IO_OUT_SEL_MASK	0x1
+#define ISPCCP2_CTRL_IO_OUT_SEL_SHIFT	2
 #define ISPCCP2_CTRL_MODE		(1 << 4)
 #define ISPCCP2_CTRL_VP_CLK_FORCE_ON	(1 << 9)
 #define ISPCCP2_CTRL_INV		(1 << 10)
@@ -94,6 +96,8 @@
 #define ISPCCP2_CTRL_INV_SHIFT		10
 #define ISPCCP2_CTRL_VP_ONLY_EN		(1 << 11)
 #define ISPCCP2_CTRL_VP_CLK_POL		(1 << 12)
+#define ISPCCP2_CTRL_VP_CLK_POL_MASK	0x1
+#define ISPCCP2_CTRL_VP_CLK_POL_SHIFT	12
 #define ISPCCP2_CTRL_VPCLK_DIV_SHIFT	15
 #define ISPCCP2_CTRL_VPCLK_DIV_MASK	0x1ffff /* [31:15] */
 #define ISPCCP2_CTRL_VP_OUT_CTRL_SHIFT	8 /* 3430 bits */
-- 
2.11.0

[PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

From: Pavel Machek <pavel@ucw.cz>

If regulator returns -EPROBE_DEFER, we need to return it too, so that
omap3isp will be re-probed when regulator is ready.

Signed-off-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c     | 3 ++-
 drivers/media/platform/omap3isp/ispccp2.c | 5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 80ed5a5f862a..4e6ba7f90e35 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device *isp)
 
 	ret = omap3isp_ccp2_init(isp);
 	if (ret < 0) {
-		dev_err(isp->dev, "CCP2 initialization failed\n");
+		if (ret != -EPROBE_DEFER)
+			dev_err(isp->dev, "CCP2 initialization failed\n");
 		goto error_ccp2;
 	}
 
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 4f8fd0c00748..47210b102bcb 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
 	if (isp->revision == ISP_REVISION_2_0) {
 		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
 		if (IS_ERR(ccp2->vdds_csib)) {
+			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
+				dev_dbg(isp->dev,
+					"Can't get regulator vdds_csib, deferring probing\n");
+				return -EPROBE_DEFER;
+			}
 			dev_dbg(isp->dev,
 				"Could not get regulator vdds_csib\n");
 			ccp2->vdds_csib = NULL;
-- 
2.11.0

[PATCH 5/7] omap3isp: Always initialise isp and mutex for csiphy1

[Sakari Ailus]

The PHY is still relevant for CCP2.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispcsiphy.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index addc6efbb033..2028bb519108 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -345,13 +345,14 @@ int omap3isp_csiphy_init(struct isp_device *isp)
 	phy2->phy_regs = OMAP3_ISP_IOMEM_CSIPHY2;
 	mutex_init(&phy2->mutex);
 
+	phy1->isp = isp;
+	mutex_init(&phy1->mutex);
+
 	if (isp->revision == ISP_REVISION_15_0) {
-		phy1->isp = isp;
 		phy1->csi2 = &isp->isp_csi2c;
 		phy1->num_data_lanes = ISP_CSIPHY1_NUM_DATA_LANES;
 		phy1->cfg_regs = OMAP3_ISP_IOMEM_CSI2C_REGS1;
 		phy1->phy_regs = OMAP3_ISP_IOMEM_CSIPHY1;
-		mutex_init(&phy1->mutex);
 	}
 
 	return 0;
-- 
2.11.0

[PATCH 6/7] omap3isp: Correctly put the last iterated endpoint fwnode always

[Sakari Ailus]

Put the last endpoint fwnode if there are too many endpoints to handle.
Also tell the user about about the condition.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4e6ba7f90e35..13a8ce4de18b 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2154,11 +2154,16 @@ static int isp_fwnodes_parse(struct device *dev,
 	if (!notifier->subdevs)
 		return -ENOMEM;
 
-	while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
-	       (fwnode = fwnode_graph_get_next_endpoint(
-			of_fwnode_handle(dev->of_node), fwnode))) {
+	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
+							fwnode))) {
 		struct isp_async_subdev *isd;
 
+		if (notifier->num_subdevs >= ISP_MAX_SUBDEVS) {
+			dev_warn(dev, "too many endpoints, ignoring\n");
+			fwnode_handle_put(fwnode);
+			break;
+		}
+
 		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
 		if (!isd)
 			goto error;
-- 
2.11.0

[PATCH 7/7] omap3isp: Skip CSI-2 receiver initialisation in CCP2 configuration

[Sakari Ailus]

If the CSI-2 receiver isn't part of the pipeline (or isn't there to begin
with), skip its initialisation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/ispcsiphy.c | 41 ++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 2028bb519108..bb2906061884 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -155,6 +155,19 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 	return 0;
 }
 
+static struct isp_pipeline *phy_to_isp_pipeline(struct isp_csiphy *phy)
+{
+	if (phy->csi2 && phy->csi2->subdev.entity.pipe)
+		return to_isp_pipeline(&phy->csi2->subdev.entity);
+
+	if (phy->isp->isp_ccp2.subdev.entity.pipe)
+		return to_isp_pipeline(&phy->isp->isp_ccp2.subdev.entity);
+
+	__WARN();
+
+	return NULL;
+}
+
 /*
  * TCLK values are OK at their reset values
  */
@@ -164,15 +177,18 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
-	struct isp_csi2_device *csi2 = phy->csi2;
-	struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+	struct isp_pipeline *pipe = phy_to_isp_pipeline(phy);
+	struct isp_bus_cfg *buscfg;
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
 	unsigned int i;
 	u32 reg;
 
+	if (!pipe)
+		return -EBUSY;
+
+	buscfg = pipe->external->host_priv;
 	if (!buscfg) {
 		struct isp_async_subdev *isd =
 			container_of(pipe->external->asd,
@@ -222,7 +238,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	csi2_ddrclk_khz = pipe->external_rate / 1000
 		/ (2 * hweight32(used_lanes)) * pipe->external_width;
 
-	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG0);
+	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
 
 	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
 		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
@@ -233,9 +249,9 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	reg |= (DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3)
 		<< ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
 
-	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
+	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
 
-	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG1);
+	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG1);
 
 	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
 		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
@@ -244,10 +260,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	reg |= TCLK_MISS << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
 	reg |= TCLK_SETTLE << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
 
-	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
+	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
 
 	/* DPHY lane configuration */
-	reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
+	reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
 
 	for (i = 0; i < num_data_lanes; i++) {
 		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
@@ -263,7 +279,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 	reg |= lanes->clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
 	reg |= lanes->clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
 
-	isp_reg_writel(csi2->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
+	isp_reg_writel(phy->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
 
 	return 0;
 }
@@ -311,11 +327,10 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 
 void omap3isp_csiphy_release(struct isp_csiphy *phy)
 {
+	struct isp_pipeline *pipe = phy_to_isp_pipeline(phy);
+
 	mutex_lock(&phy->mutex);
-	if (phy->phy_in_use) {
-		struct isp_csi2_device *csi2 = phy->csi2;
-		struct isp_pipeline *pipe =
-			to_isp_pipeline(&csi2->subdev.entity);
+	if (phy->phy_in_use && pipe) {
 		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
-- 
2.11.0

Re: [PATCH 1/7] omap3isp: Ignore endpoints with invalid configuration

[Sebastian Reichel]

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

Hi,

On Tue, Jul 18, 2017 at 01:01:10AM +0300, Sakari Ailus wrote:
> If endpoint has an invalid configuration, ignore it instead of happily
> proceeding to use it nonetheless. Ignoring such an endpoint is better than
> failing since there could be multiple endpoints, only some of which are
> bad.

I would expect a dev_warn(dev, "Ignore endpoint (broken configuration)!");

-- Sebastian

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/media/platform/omap3isp/isp.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index db2cccb57ceb..441eba1e02eb 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2110,10 +2110,12 @@ static int isp_fwnodes_parse(struct device *dev,
>  		if (!isd)
>  			goto error;
>  
> -		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> +		if (isp_fwnode_parse(dev, fwnode, isd)) {
> +			devm_kfree(dev, isd);
> +			continue;
> +		}
>  
> -		if (isp_fwnode_parse(dev, fwnode, isd))
> -			goto error;
> +		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
>  
>  		isd->asd.match.fwnode.fwnode =
>  			fwnode_graph_get_remote_port_parent(fwnode);
> -- 
> 2.11.0
> 

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

Re: [PATCH 6/7] omap3isp: Correctly put the last iterated endpoint fwnode always

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:15 Sakari Ailus wrote:
> Put the last endpoint fwnode if there are too many endpoints to handle.
> Also tell the user about about the condition.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

There are so many refcount-related issues with fwnodes, I wonder whether we 
could/should teach a static analyzer about that.

> ---
>  drivers/media/platform/omap3isp/isp.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 4e6ba7f90e35..13a8ce4de18b
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2154,11 +2154,16 @@ static int isp_fwnodes_parse(struct device *dev,
>  	if (!notifier->subdevs)
>  		return -ENOMEM;
> 
> -	while (notifier->num_subdevs < ISP_MAX_SUBDEVS &&
> -	       (fwnode = fwnode_graph_get_next_endpoint(
> -			of_fwnode_handle(dev->of_node), fwnode))) {
> +	while ((fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(dev),
> +							fwnode))) {
>  		struct isp_async_subdev *isd;
> 
> +		if (notifier->num_subdevs >= ISP_MAX_SUBDEVS) {
> +			dev_warn(dev, "too many endpoints, ignoring\n");
> +			fwnode_handle_put(fwnode);
> +			break;
> +		}
> +
>  		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
>  		if (!isd)
>  			goto error;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sebastian Reichel]

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

Hi,

On Tue, Jul 18, 2017 at 01:01:13AM +0300, Sakari Ailus wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c     | 3 ++-
>  drivers/media/platform/omap3isp/ispccp2.c | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 80ed5a5f862a..4e6ba7f90e35 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device *isp)
>  
>  	ret = omap3isp_ccp2_init(isp);
>  	if (ret < 0) {
> -		dev_err(isp->dev, "CCP2 initialization failed\n");
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(isp->dev, "CCP2 initialization failed\n");
>  		goto error_ccp2;
>  	}
>  
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
> index 4f8fd0c00748..47210b102bcb 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	if (isp->revision == ISP_REVISION_2_0) {
>  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>  		if (IS_ERR(ccp2->vdds_csib)) {
> +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> +				dev_dbg(isp->dev,
> +					"Can't get regulator vdds_csib, deferring probing\n");
> +				return -EPROBE_DEFER;
> +			}

I wonder if the right approach wouldn't be to always bail out for
errors. devm_regulator_get should provide a dummy regulator if
none is specified. If we get an error here it means something is
configured incorrectly or we have serious problems.

>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");
>  			ccp2->vdds_csib = NULL;

-- Sebastian

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

Re: [PATCH 7/7] omap3isp: Skip CSI-2 receiver initialisation in CCP2 configuration

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:16 Sakari Ailus wrote:
> If the CSI-2 receiver isn't part of the pipeline (or isn't there to begin
> with), skip its initialisation.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/ispcsiphy.c | 41 +++++++++++++++++---------
>  1 file changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> 2028bb519108..bb2906061884 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -155,6 +155,19 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> power) return 0;
>  }
> 
> +static struct isp_pipeline *phy_to_isp_pipeline(struct isp_csiphy *phy)
> +{
> +	if (phy->csi2 && phy->csi2->subdev.entity.pipe)
> +		return to_isp_pipeline(&phy->csi2->subdev.entity);
> +
> +	if (phy->isp->isp_ccp2.subdev.entity.pipe)
> +		return to_isp_pipeline(&phy->isp->isp_ccp2.subdev.entity);
> +
> +	__WARN();
> +
> +	return NULL;

If you changed the phy->csi2 pointer from a isp_csi2_device to a v4l2_subdev 
or media_entity (and renamed it accordingly, to something like "receiver" for 
instance, ideas for a better name are welcome) you could store a pointer to 
the appropriate CSI2 or CCP2 receiver and simplify this function, or even 
inline it as it is currently, calling to_isp_pipeline(phy->receiver). I think 
that would simplify the driver.

> +}
> +
>  /*
>   * TCLK values are OK at their reset values
>   */
> @@ -164,15 +177,18 @@ static int csiphy_set_power(struct isp_csiphy *phy,
> u32 power)
> 
>  static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  {
> -	struct isp_csi2_device *csi2 = phy->csi2;
> -	struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
> -	struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> +	struct isp_pipeline *pipe = phy_to_isp_pipeline(phy);
> +	struct isp_bus_cfg *buscfg;
>  	struct isp_csiphy_lanes_cfg *lanes;
>  	int csi2_ddrclk_khz;
>  	unsigned int num_data_lanes, used_lanes = 0;
>  	unsigned int i;
>  	u32 reg;
> 
> +	if (!pipe)
> +		return -EBUSY;
> +
> +	buscfg = pipe->external->host_priv;
>  	if (!buscfg) {
>  		struct isp_async_subdev *isd =
>  			container_of(pipe->external->asd,
> @@ -222,7 +238,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> *phy) csi2_ddrclk_khz = pipe->external_rate / 1000
>  		/ (2 * hweight32(used_lanes)) * pipe->external_width;
> 
> -	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG0);
> +	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG0);
> 
>  	reg &= ~(ISPCSIPHY_REG0_THS_TERM_MASK |
>  		 ISPCSIPHY_REG0_THS_SETTLE_MASK);
> @@ -233,9 +249,9 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> *phy) reg |= (DIV_ROUND_UP(90 * csi2_ddrclk_khz, 1000000) + 3)
>  		<< ISPCSIPHY_REG0_THS_SETTLE_SHIFT;
> 
> -	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> +	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG0);
> 
> -	reg = isp_reg_readl(csi2->isp, phy->phy_regs, ISPCSIPHY_REG1);
> +	reg = isp_reg_readl(phy->isp, phy->phy_regs, ISPCSIPHY_REG1);
> 
>  	reg &= ~(ISPCSIPHY_REG1_TCLK_TERM_MASK |
>  		 ISPCSIPHY_REG1_TCLK_MISS_MASK |
> @@ -244,10 +260,10 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> *phy) reg |= TCLK_MISS << ISPCSIPHY_REG1_TCLK_MISS_SHIFT;
>  	reg |= TCLK_SETTLE << ISPCSIPHY_REG1_TCLK_SETTLE_SHIFT;
> 
> -	isp_reg_writel(csi2->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> +	isp_reg_writel(phy->isp, reg, phy->phy_regs, ISPCSIPHY_REG1);
> 
>  	/* DPHY lane configuration */
> -	reg = isp_reg_readl(csi2->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> +	reg = isp_reg_readl(phy->isp, phy->cfg_regs, ISPCSI2_PHY_CFG);
> 
>  	for (i = 0; i < num_data_lanes; i++) {
>  		reg &= ~(ISPCSI2_PHY_CFG_DATA_POL_MASK(i + 1) |
> @@ -263,7 +279,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> *phy) reg |= lanes->clk.pol << ISPCSI2_PHY_CFG_CLOCK_POL_SHIFT;
>  	reg |= lanes->clk.pos << ISPCSI2_PHY_CFG_CLOCK_POSITION_SHIFT;
> 
> -	isp_reg_writel(csi2->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> +	isp_reg_writel(phy->isp, reg, phy->cfg_regs, ISPCSI2_PHY_CFG);
> 
>  	return 0;
>  }
> @@ -311,11 +327,10 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> 
>  void omap3isp_csiphy_release(struct isp_csiphy *phy)
>  {
> +	struct isp_pipeline *pipe = phy_to_isp_pipeline(phy);
> +
>  	mutex_lock(&phy->mutex);
> -	if (phy->phy_in_use) {
> -		struct isp_csi2_device *csi2 = phy->csi2;
> -		struct isp_pipeline *pipe =
> -			to_isp_pipeline(&csi2->subdev.entity);
> +	if (phy->phy_in_use && pipe) {
>  		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> 
>  		csiphy_routing_cfg(phy, buscfg->interface, false,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] omap3isp: Parse CSI1 configuration from the device tree

[Sebastian Reichel]

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

Hi,

On Tue, Jul 18, 2017 at 01:01:11AM +0300, Sakari Ailus wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> Add support for parsing CSI1 configuration.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  drivers/media/platform/omap3isp/isp.c      | 106 +++++++++++++++++++++--------
>  drivers/media/platform/omap3isp/omap3isp.h |   1 +
>  2 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 441eba1e02eb..80ed5a5f862a 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2017,6 +2017,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>  	struct v4l2_fwnode_endpoint vep;
>  	unsigned int i;
>  	int ret;
> +	bool csi1 = false;
>  
>  	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
>  	if (ret)
> @@ -2045,41 +2046,92 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwnode,
>  
>  	case ISP_OF_PHY_CSIPHY1:
>  	case ISP_OF_PHY_CSIPHY2:
> -		/* FIXME: always assume CSI-2 for now. */
> +		switch (vep.bus_type) {
> +		case V4L2_MBUS_CCP2:
> +		case V4L2_MBUS_CSI1:
> +			dev_dbg(dev, "csi1/ccp2 configuration\n");
> +			csi1 = true;
> +			break;
> +		case V4L2_MBUS_CSI2:
> +			dev_dbg(dev, "csi2 configuration\n");
> +			csi1 = false;
> +			break;
> +		default:
> +			dev_err(dev, "unsupported bus type %u\n",
> +				vep.bus_type);
> +			return -EINVAL;
> +		}
> +
>  		switch (vep.base.port) {
>  		case ISP_OF_PHY_CSIPHY1:
> -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>  			break;
>  		case ISP_OF_PHY_CSIPHY2:
> -			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>  			break;
>  		}
> -		buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
> -		buscfg->bus.csi2.lanecfg.clk.pol =
> -			vep.bus.mipi_csi2.lane_polarities[0];
> -		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> -			buscfg->bus.csi2.lanecfg.clk.pol,
> -			buscfg->bus.csi2.lanecfg.clk.pos);
> -
> -		buscfg->bus.csi2.num_data_lanes =
> -			vep.bus.mipi_csi2.num_data_lanes;
> -
> -		for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> -			buscfg->bus.csi2.lanecfg.data[i].pos =
> -				vep.bus.mipi_csi2.data_lanes[i];
> -			buscfg->bus.csi2.lanecfg.data[i].pol =
> -				vep.bus.mipi_csi2.lane_polarities[i + 1];
> +		if (csi1) {
> +			buscfg->bus.ccp2.lanecfg.clk.pos =
> +				vep.bus.mipi_csi1.clock_lane;
> +			buscfg->bus.ccp2.lanecfg.clk.pol =
> +				vep.bus.mipi_csi1.lane_polarity[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.ccp2.lanecfg.clk.pol,
> +				buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> +			buscfg->bus.ccp2.lanecfg.data[0].pos =
> +				vep.bus.mipi_csi1.data_lane;
> +			buscfg->bus.ccp2.lanecfg.data[0].pol =
> +				vep.bus.mipi_csi1.lane_polarity[1];
> +
>  			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> -				buscfg->bus.csi2.lanecfg.data[i].pol,
> -				buscfg->bus.csi2.lanecfg.data[i].pos);
> +				buscfg->bus.ccp2.lanecfg.data[0].pol,
> +				buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> +			buscfg->bus.ccp2.strobe_clk_pol =
> +				vep.bus.mipi_csi1.clock_inv;
> +			buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;
> +			buscfg->bus.ccp2.ccp2_mode =
> +				vep.bus_type == V4L2_MBUS_CCP2;
> +			buscfg->bus.ccp2.vp_clk_pol = 1;
> +
> +			buscfg->bus.ccp2.crc = 1;
> +		} else {
> +			buscfg->bus.csi2.lanecfg.clk.pos =
> +				vep.bus.mipi_csi2.clock_lane;
> +			buscfg->bus.csi2.lanecfg.clk.pol =
> +				vep.bus.mipi_csi2.lane_polarities[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.csi2.lanecfg.clk.pol,
> +				buscfg->bus.csi2.lanecfg.clk.pos);
> +
> +			buscfg->bus.csi2.num_data_lanes =
> +				vep.bus.mipi_csi2.num_data_lanes;
> +
> +			for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> +				buscfg->bus.csi2.lanecfg.data[i].pos =
> +					vep.bus.mipi_csi2.data_lanes[i];
> +				buscfg->bus.csi2.lanecfg.data[i].pol =
> +					vep.bus.mipi_csi2.lane_polarities[i + 1];
> +				dev_dbg(dev,
> +					"data lane %u polarity %u, pos %u\n", i,
> +					buscfg->bus.csi2.lanecfg.data[i].pol,
> +					buscfg->bus.csi2.lanecfg.data[i].pos);
> +			}
> +			/*
> +			 * FIXME: now we assume the CRC is always there.
> +			 * Implement a way to obtain this information from the
> +			 * sensor. Frame descriptors, perhaps?
> +			 */
> +
> +			buscfg->bus.csi2.crc = 1;
>  		}
> -
> -		/*
> -		 * FIXME: now we assume the CRC is always there.
> -		 * Implement a way to obtain this information from the
> -		 * sensor. Frame descriptors, perhaps?
> -		 */
> -		buscfg->bus.csi2.crc = 1;
>  		break;
>  
>  	default:
> diff --git a/drivers/media/platform/omap3isp/omap3isp.h b/drivers/media/platform/omap3isp/omap3isp.h
> index 3c26f9a3f508..672a9cf5aa4d 100644
> --- a/drivers/media/platform/omap3isp/omap3isp.h
> +++ b/drivers/media/platform/omap3isp/omap3isp.h
> @@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
>  	unsigned int ccp2_mode:1;
>  	unsigned int phy_layer:1;
>  	unsigned int vpclk_div:2;
> +	unsigned int vp_clk_pol:1;
>  	struct isp_csiphy_lanes_cfg lanecfg;
>  };
>  
> -- 
> 2.11.0
> 

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

Re: [PATCH 1/7] omap3isp: Ignore endpoints with invalid configuration

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:10 Sakari Ailus wrote:
> If endpoint has an invalid configuration, ignore it instead of happily
> proceeding to use it nonetheless. Ignoring such an endpoint is better than
> failing since there could be multiple endpoints, only some of which are
> bad.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Pavel Machek <pavel@ucw.cz>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/omap3isp/isp.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index db2cccb57ceb..441eba1e02eb
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2110,10 +2110,12 @@ static int isp_fwnodes_parse(struct device *dev,
>  		if (!isd)
>  			goto error;
> 
> -		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> +		if (isp_fwnode_parse(dev, fwnode, isd)) {
> +			devm_kfree(dev, isd);
> +			continue;
> +		}
> 
> -		if (isp_fwnode_parse(dev, fwnode, isd))
> -			goto error;
> +		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> 
>  		isd->asd.match.fwnode.fwnode =
>  			fwnode_graph_get_remote_port_parent(fwnode);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] omap3isp: Parse CSI1 configuration from the device tree

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:11 Sakari Ailus wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> Add support for parsing CSI1 configuration.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c      | 106 ++++++++++++++++++--------
>  drivers/media/platform/omap3isp/omap3isp.h |   1 +
>  2 files changed, 80 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 441eba1e02eb..80ed5a5f862a
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2017,6 +2017,7 @@ static int isp_fwnode_parse(struct device *dev, struct
> fwnode_handle *fwnode, struct v4l2_fwnode_endpoint vep;
>  	unsigned int i;
>  	int ret;
> +	bool csi1 = false;
> 
>  	ret = v4l2_fwnode_endpoint_parse(fwnode, &vep);
>  	if (ret)
> @@ -2045,41 +2046,92 @@ static int isp_fwnode_parse(struct device *dev,
> struct fwnode_handle *fwnode,
> 
>  	case ISP_OF_PHY_CSIPHY1:
>  	case ISP_OF_PHY_CSIPHY2:
> -		/* FIXME: always assume CSI-2 for now. */
> +		switch (vep.bus_type) {
> +		case V4L2_MBUS_CCP2:
> +		case V4L2_MBUS_CSI1:
> +			dev_dbg(dev, "csi1/ccp2 configuration\n");

Nitpicking, I would write it "CSI1/CCP2".

> +			csi1 = true;
> +			break;
> +		case V4L2_MBUS_CSI2:
> +			dev_dbg(dev, "csi2 configuration\n");

And "CSI2" here.

> +			csi1 = false;
> +			break;
> +		default:
> +			dev_err(dev, "unsupported bus type %u\n",
> +				vep.bus_type);
> +			return -EINVAL;
> +		}
> +
>  		switch (vep.base.port) {
>  		case ISP_OF_PHY_CSIPHY1:
> -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>  			break;
>  		case ISP_OF_PHY_CSIPHY2:
> -			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>  			break;
>  		}
> -		buscfg->bus.csi2.lanecfg.clk.pos = 
vep.bus.mipi_csi2.clock_lane;
> -		buscfg->bus.csi2.lanecfg.clk.pol =
> -			vep.bus.mipi_csi2.lane_polarities[0];
> -		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> -			buscfg->bus.csi2.lanecfg.clk.pol,
> -			buscfg->bus.csi2.lanecfg.clk.pos);
> -
> -		buscfg->bus.csi2.num_data_lanes =
> -			vep.bus.mipi_csi2.num_data_lanes;
> -
> -		for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) {
> -			buscfg->bus.csi2.lanecfg.data[i].pos =
> -				vep.bus.mipi_csi2.data_lanes[i];
> -			buscfg->bus.csi2.lanecfg.data[i].pol =
> -				vep.bus.mipi_csi2.lane_polarities[i + 1];
> +		if (csi1) {
> +			buscfg->bus.ccp2.lanecfg.clk.pos =
> +				vep.bus.mipi_csi1.clock_lane;
> +			buscfg->bus.ccp2.lanecfg.clk.pol =
> +				vep.bus.mipi_csi1.lane_polarity[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.ccp2.lanecfg.clk.pol,
> +				buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> +			buscfg->bus.ccp2.lanecfg.data[0].pos =
> +				vep.bus.mipi_csi1.data_lane;
> +			buscfg->bus.ccp2.lanecfg.data[0].pol =
> +				vep.bus.mipi_csi1.lane_polarity[1];
> +
>  			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> -				buscfg->bus.csi2.lanecfg.data[i].pol,
> -				buscfg->bus.csi2.lanecfg.data[i].pos);
> +				buscfg->bus.ccp2.lanecfg.data[0].pol,
> +				buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> +			buscfg->bus.ccp2.strobe_clk_pol =
> +				vep.bus.mipi_csi1.clock_inv;
> +			buscfg->bus.ccp2.phy_layer = vep.bus.mipi_csi1.strobe;
> +			buscfg->bus.ccp2.ccp2_mode =
> +				vep.bus_type == V4L2_MBUS_CCP2;
> +			buscfg->bus.ccp2.vp_clk_pol = 1;
> +
> +			buscfg->bus.ccp2.crc = 1;
> +		} else {
> +			buscfg->bus.csi2.lanecfg.clk.pos =
> +				vep.bus.mipi_csi2.clock_lane;
> +			buscfg->bus.csi2.lanecfg.clk.pol =
> +				vep.bus.mipi_csi2.lane_polarities[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.csi2.lanecfg.clk.pol,
> +				buscfg->bus.csi2.lanecfg.clk.pos);
> +
> +			buscfg->bus.csi2.num_data_lanes =
> +				vep.bus.mipi_csi2.num_data_lanes;
> +
> +			for (i = 0; i < buscfg->bus.csi2.num_data_lanes; i++) 
{
> +				buscfg->bus.csi2.lanecfg.data[i].pos =
> +					vep.bus.mipi_csi2.data_lanes[i];
> +				buscfg->bus.csi2.lanecfg.data[i].pol =
> +					vep.bus.mipi_csi2.lane_polarities[i + 
1];
> +				dev_dbg(dev,
> +					"data lane %u polarity %u, pos %u\n", 
i,
> +					buscfg->bus.csi2.lanecfg.data[i].pol,
> +					buscfg->bus.csi2.lanecfg.data[i].pos);
> +			}
> +			/*
> +			 * FIXME: now we assume the CRC is always there.
> +			 * Implement a way to obtain this information from the
> +			 * sensor. Frame descriptors, perhaps?
> +			 */
> +

Extra blank line. I would move it right before the comment.

With these small issues fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			buscfg->bus.csi2.crc = 1;
>  		}
> -
> -		/*
> -		 * FIXME: now we assume the CRC is always there.
> -		 * Implement a way to obtain this information from the
> -		 * sensor. Frame descriptors, perhaps?
> -		 */
> -		buscfg->bus.csi2.crc = 1;
>  		break;
> 
>  	default:
> diff --git a/drivers/media/platform/omap3isp/omap3isp.h
> b/drivers/media/platform/omap3isp/omap3isp.h index
> 3c26f9a3f508..672a9cf5aa4d 100644
> --- a/drivers/media/platform/omap3isp/omap3isp.h
> +++ b/drivers/media/platform/omap3isp/omap3isp.h
> @@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
>  	unsigned int ccp2_mode:1;
>  	unsigned int phy_layer:1;
>  	unsigned int vpclk_div:2;
> +	unsigned int vp_clk_pol:1;
>  	struct isp_csiphy_lanes_cfg lanecfg;
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday 18 Jul 2017 01:01:13 Sakari Ailus wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> If regulator returns -EPROBE_DEFER, we need to return it too, so that
> omap3isp will be re-probed when regulator is ready.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c     | 3 ++-
>  drivers/media/platform/omap3isp/ispccp2.c | 5 +++++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 80ed5a5f862a..4e6ba7f90e35
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1880,7 +1880,8 @@ static int isp_initialize_modules(struct isp_device
> *isp)
> 
>  	ret = omap3isp_ccp2_init(isp);
>  	if (ret < 0) {
> -		dev_err(isp->dev, "CCP2 initialization failed\n");
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(isp->dev, "CCP2 initialization failed\n");
>  		goto error_ccp2;
>  	}
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 4f8fd0c00748..47210b102bcb 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
>  	if (isp->revision == ISP_REVISION_2_0) {
>  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
>  		if (IS_ERR(ccp2->vdds_csib)) {
> +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> +				dev_dbg(isp->dev,
> +					"Can't get regulator vdds_csib, 
deferring probing\n");
> +				return -EPROBE_DEFER;
> +			}
>  			dev_dbg(isp->dev,
>  				"Could not get regulator vdds_csib\n");

I would just move this message above the -EPROBE_DEFER check and remove the 
one inside the check. Probe deferral debug information can be obtained by 
enabling the debug messages in the driver core.

>  			ccp2->vdds_csib = NULL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

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

Hi!

> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index
> > 4f8fd0c00748..47210b102bcb 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >  	if (isp->revision == ISP_REVISION_2_0) {
> >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> >  		if (IS_ERR(ccp2->vdds_csib)) {
> > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > +				dev_dbg(isp->dev,
> > +					"Can't get regulator vdds_csib, 
> deferring probing\n");
> > +				return -EPROBE_DEFER;
> > +			}
> >  			dev_dbg(isp->dev,
> >  				"Could not get regulator vdds_csib\n");
> 
> I would just move this message above the -EPROBE_DEFER check and remove the 
> one inside the check. Probe deferral debug information can be obtained by 
> enabling the debug messages in the driver core.

Actually, in such case perhaps the message in -EPROBE_DEFER could be
removed. Deferred probing happens all the time. OTOH "Could not get
regulator" probably should be dev_err(), as it will make device
unusable?

Thanks,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Laurent Pinchart]

Hi Pavel,

On Tuesday 18 Jul 2017 12:03:52 Pavel Machek wrote:
> Hi!
> 
> >> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> >> b/drivers/media/platform/omap3isp/ispccp2.c index
> >> 4f8fd0c00748..47210b102bcb 100644
> >> --- a/drivers/media/platform/omap3isp/ispccp2.c
> >> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> >> @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> >>  	if (isp->revision == ISP_REVISION_2_0) {
> >>  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> >>  		if (IS_ERR(ccp2->vdds_csib)) {
> >> +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> >> +				dev_dbg(isp->dev,
> >> +					"Can't get regulator vdds_csib,
> >> deferring probing\n");
> >> +				return -EPROBE_DEFER;
> >> +			}
> >> 
> >>  			dev_dbg(isp->dev,
> >>  				"Could not get regulator vdds_csib\n");
> > 
> > I would just move this message above the -EPROBE_DEFER check and remove
> > the one inside the check. Probe deferral debug information can be obtained
> > by enabling the debug messages in the driver core.
> 
> Actually, in such case perhaps the message in -EPROBE_DEFER could be
> removed. Deferred probing happens all the time. OTOH "Could not get
> regulator" probably should be dev_err(), as it will make device
> unusable?

Messages along the lines of "I'm deferring probe" are in my opinion not 
valuable, as we can get that from the driver core. Debug messages that tell 
why probe is being deferred can however be useful for debugging. That's why I 
proposed to move the regulator get error debug message above the probe 
deferral check, as it would then always be printed. Turning it into an error 
makes sense, but only when not deferring probe then.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

Hi Pavel,

On Tue, Jul 18, 2017 at 12:03:52PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > 4f8fd0c00748..47210b102bcb 100644
> > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > >  	if (isp->revision == ISP_REVISION_2_0) {
> > >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > >  		if (IS_ERR(ccp2->vdds_csib)) {
> > > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > +				dev_dbg(isp->dev,
> > > +					"Can't get regulator vdds_csib, 
> > deferring probing\n");
> > > +				return -EPROBE_DEFER;
> > > +			}
> > >  			dev_dbg(isp->dev,
> > >  				"Could not get regulator vdds_csib\n");
> > 
> > I would just move this message above the -EPROBE_DEFER check and remove the 
> > one inside the check. Probe deferral debug information can be obtained by 
> > enabling the debug messages in the driver core.
> 
> Actually, in such case perhaps the message in -EPROBE_DEFER could be
> removed. Deferred probing happens all the time. OTOH "Could not get
> regulator" probably should be dev_err(), as it will make device
> unusable?

Isn't this only if you need ccp2?

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

Re: [PATCH 0/7] Omap3isp CCP2 support

[Pavel Machek]

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

Hi!

> I rebased the ccp2 branch and went through the patches. I didn't find
> anything really alarming there; I changed one commit description of
> "omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode" that had
> some junk in it as well as in the last patch changed the condition in
> omap3isp_csiphy_release() that was obviously wrong.
> 
> Let me know what you think.
> 
> If we merge these, is there anything still missing from plain ccp2
> support?

I believe we are fine.

Tested-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Pavel Machek <pavel@ucw.cz>

There is still

commit 629fcfe04ef5f1aee5280b2e0208cc891503824a
Author: Pavel <pavel@ucw.cz>
Date:   Mon Feb 13 21:26:51 2017 +0100

    omap3isp: fix VP2SDR bit so capture (not preview) works

issue, but that's independend of ccp2 support, and driver is useful
without that fix. (Preview works ok, capture results in distorted
picture but...)

Plus I'll need to submit dts changes for N900, and subdev support for
camera flash/focus would be useful.

But with this series we have basic support in.

Best regards,
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/7] omap3isp: Ignore endpoints with invalid configuration

[Sakari Ailus]

On Tue, Jul 18, 2017 at 01:03:33AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Tue, Jul 18, 2017 at 01:01:10AM +0300, Sakari Ailus wrote:
> > If endpoint has an invalid configuration, ignore it instead of happily
> > proceeding to use it nonetheless. Ignoring such an endpoint is better than
> > failing since there could be multiple endpoints, only some of which are
> > bad.
> 
> I would expect a dev_warn(dev, "Ignore endpoint (broken configuration)!");

Hmm. Perhaps I'll just drop this patch.

This will be (hopefully) soon replaced by a framework function.

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

Re: [PATCH 6/7] omap3isp: Correctly put the last iterated endpoint fwnode always

[Sakari Ailus]

On Tue, Jul 18, 2017 at 11:40:22AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tuesday 18 Jul 2017 01:01:15 Sakari Ailus wrote:
> > Put the last endpoint fwnode if there are too many endpoints to handle.
> > Also tell the user about about the condition.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> There are so many refcount-related issues with fwnodes, I wonder whether we 
> could/should teach a static analyzer about that.

This will be actually soon replaced by a convenience function in the
frameworks in my recent RFC patchset.

I'll drop this patch (as I'll do the first one).

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

Re: [PATCH 7/7] omap3isp: Skip CSI-2 receiver initialisation in CCP2 configuration

[Sakari Ailus]

Hi Laurent,

On Tue, Jul 18, 2017 at 11:54:21AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Tuesday 18 Jul 2017 01:01:16 Sakari Ailus wrote:
> > If the CSI-2 receiver isn't part of the pipeline (or isn't there to begin
> > with), skip its initialisation.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/platform/omap3isp/ispcsiphy.c | 41 +++++++++++++++++---------
> >  1 file changed, 28 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> > b/drivers/media/platform/omap3isp/ispcsiphy.c index
> > 2028bb519108..bb2906061884 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -155,6 +155,19 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> > power) return 0;
> >  }
> > 
> > +static struct isp_pipeline *phy_to_isp_pipeline(struct isp_csiphy *phy)
> > +{
> > +	if (phy->csi2 && phy->csi2->subdev.entity.pipe)
> > +		return to_isp_pipeline(&phy->csi2->subdev.entity);
> > +
> > +	if (phy->isp->isp_ccp2.subdev.entity.pipe)
> > +		return to_isp_pipeline(&phy->isp->isp_ccp2.subdev.entity);
> > +
> > +	__WARN();
> > +
> > +	return NULL;
> 
> If you changed the phy->csi2 pointer from a isp_csi2_device to a v4l2_subdev 
> or media_entity (and renamed it accordingly, to something like "receiver" for 
> instance, ideas for a better name are welcome) you could store a pointer to 
> the appropriate CSI2 or CCP2 receiver and simplify this function, or even 
> inline it as it is currently, calling to_isp_pipeline(phy->receiver). I think 
> that would simplify the driver.

Yes; I'll store the entity to the phy struct. That way the function can be
indeed removed.

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

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

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

Hi!

> > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > > 4f8fd0c00748..47210b102bcb 100644
> > > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > > >  	if (isp->revision == ISP_REVISION_2_0) {
> > > >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > > >  		if (IS_ERR(ccp2->vdds_csib)) {
> > > > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > > +				dev_dbg(isp->dev,
> > > > +					"Can't get regulator vdds_csib, 
> > > deferring probing\n");
> > > > +				return -EPROBE_DEFER;
> > > > +			}
> > > >  			dev_dbg(isp->dev,
> > > >  				"Could not get regulator vdds_csib\n");
> > > 
> > > I would just move this message above the -EPROBE_DEFER check and remove the 
> > > one inside the check. Probe deferral debug information can be obtained by 
> > > enabling the debug messages in the driver core.
> > 
> > Actually, in such case perhaps the message in -EPROBE_DEFER could be
> > removed. Deferred probing happens all the time. OTOH "Could not get
> > regulator" probably should be dev_err(), as it will make device
> > unusable?
> 
> Isn't this only if you need ccp2?

No idea really. I only have N900 working with linux at the moment. I'm
trying to get N9 and N950 working, but no luck so far.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

On Tue, Jul 18, 2017 at 11:02:28PM +0200, Pavel Machek wrote:
> Hi!
> 
> > > > > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > > > > b/drivers/media/platform/omap3isp/ispccp2.c index
> > > > > 4f8fd0c00748..47210b102bcb 100644
> > > > > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > > > > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > > > > @@ -1140,6 +1140,11 @@ int omap3isp_ccp2_init(struct isp_device *isp)
> > > > >  	if (isp->revision == ISP_REVISION_2_0) {
> > > > >  		ccp2->vdds_csib = devm_regulator_get(isp->dev, "vdds_csib");
> > > > >  		if (IS_ERR(ccp2->vdds_csib)) {
> > > > > +			if (PTR_ERR(ccp2->vdds_csib) == -EPROBE_DEFER) {
> > > > > +				dev_dbg(isp->dev,
> > > > > +					"Can't get regulator vdds_csib, 
> > > > deferring probing\n");
> > > > > +				return -EPROBE_DEFER;
> > > > > +			}
> > > > >  			dev_dbg(isp->dev,
> > > > >  				"Could not get regulator vdds_csib\n");
> > > > 
> > > > I would just move this message above the -EPROBE_DEFER check and remove the 
> > > > one inside the check. Probe deferral debug information can be obtained by 
> > > > enabling the debug messages in the driver core.
> > > 
> > > Actually, in such case perhaps the message in -EPROBE_DEFER could be
> > > removed. Deferred probing happens all the time. OTOH "Could not get
> > > regulator" probably should be dev_err(), as it will make device
> > > unusable?
> > 
> > Isn't this only if you need ccp2?
> 
> No idea really. I only have N900 working with linux at the moment. I'm
> trying to get N9 and N950 working, but no luck so far.

Still no? :-(

Do you know if you get the kernel booting? Do you have access to the serial
console? I might have seen the e-mail chain but I lost the track. What
happens after the flasher has pushed the kernel to RAM and the boot starts?
It's wonderful for debugging if something's wrong...

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

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

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

Hi!

> > No idea really. I only have N900 working with linux at the moment. I'm
> > trying to get N9 and N950 working, but no luck so far.
> 
> Still no? :-(
> 
> Do you know if you get the kernel booting? Do you have access to the serial
> console? I might have seen the e-mail chain but I lost the track. What
> happens after the flasher has pushed the kernel to RAM and the boot starts?
> It's wonderful for debugging if something's wrong...

Still no. No serial cable, unfortunately. Flasher seems to run the
kernel, but I see no evidence new kernel started successfully. I was
told display is not expected to work, and on USB I see bootloader
disconnecting and that's it.

If you had a kernel binary that works for you, and does something I
can observe, that would be welcome :-).

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Sakari Ailus]

On Tue, Jul 18, 2017 at 11:27:12PM +0200, Pavel Machek wrote:
> Hi!

EHLO

> 
> > > No idea really. I only have N900 working with linux at the moment. I'm
> > > trying to get N9 and N950 working, but no luck so far.
> > 
> > Still no? :-(
> > 
> > Do you know if you get the kernel booting? Do you have access to the serial
> > console? I might have seen the e-mail chain but I lost the track. What
> > happens after the flasher has pushed the kernel to RAM and the boot starts?
> > It's wonderful for debugging if something's wrong...
> 
> Still no. No serial cable, unfortunately. Flasher seems to run the
> kernel, but I see no evidence new kernel started successfully. I was
> told display is not expected to work, and on USB I see bootloader
> disconnecting and that's it.
> 
> If you had a kernel binary that works for you, and does something I
> can observe, that would be welcome :-).

I put my .config I use for N9 here:

<URL:http://www.retiisi.org.uk/v4l2/tmp/config.n9>

The root filesystem is over NFS root with usbnet. You should see something
like this in dmesg:

[35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci
[35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1
[35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[35792.206252] usb 2-2: Product: Ethernet Gadget
[35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 with musb-hdrc

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

Re: [PATCH 4/7] omap3isp: Return -EPROBE_DEFER if the required regulators can't be obtained

[Pavel Machek]

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

Hi!

> > > > No idea really. I only have N900 working with linux at the moment. I'm
> > > > trying to get N9 and N950 working, but no luck so far.
> > > 
> > > Still no? :-(
> > > 
> > > Do you know if you get the kernel booting? Do you have access to the serial
> > > console? I might have seen the e-mail chain but I lost the track. What
> > > happens after the flasher has pushed the kernel to RAM and the boot starts?
> > > It's wonderful for debugging if something's wrong...
> > 
> > Still no. No serial cable, unfortunately. Flasher seems to run the
> > kernel, but I see no evidence new kernel started successfully. I was
> > told display is not expected to work, and on USB I see bootloader
> > disconnecting and that's it.
> > 
> > If you had a kernel binary that works for you, and does something I
> > can observe, that would be welcome :-).
> 
> I put my .config I use for N9 here:
> 
> <URL:http://www.retiisi.org.uk/v4l2/tmp/config.n9>
> 
> The root filesystem is over NFS root with usbnet. You should see something
> like this in dmesg:
> 
> [35792.056138] usb 2-2: new high-speed USB device number 58 using ehci-pci
> [35792.206238] usb 2-2: New USB device found, idVendor=0525, idProduct=a4a1
> [35792.206247] usb 2-2: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [35792.206252] usb 2-2: Product: Ethernet Gadget
> [35792.206257] usb 2-2: Manufacturer: Linux 4.13.0-rc1-00089-g4c341695f3b6 with musb-hdrc

Could not get it to work, same result as usual: no response on the
device, disconnect on USB and then quiet.

Could I get actual zImage-dtb from you? What development options are
enabled in your case? I was mostly using none -- sudo
../maemo/0xffff/src/0xFFFF -F "" -R 0 .

Thanks,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]