[PATCH v2 0/5] Omap3isp CCP2 support

linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH v2 0/5] Omap3isp CCP2 support
@ 2017-08-16 12:51 Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 1/5] omap3isp: Parse CSI1 configuration from the device tree Sakari Ailus
                   ` (4 more replies)
  0 siblings, 5 replies; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

Hi folks,

Here's a respin of the omap3isp ccp2 support patches.

since v1:

- Root out patches already merged or not needed (omapisp: Ignore endpoints
  with invalid configuration).

- Rework the patch skipping CSI-2 receiver configuration for CCP2
  ("omap3isp: Skip CSI-2 receiver initialisation in CCP2 configuration").
  Instead of trying to figure out which receiver (CSI-2 or CCP2) is used,
  store the information to the isp_csiphy struct instead.

- Added a patch to correctly release a CSI-2 phy --- the PHY configuration
  coming from DT was previously ignored.

Pavel Machek (2):
  omap3isp: Parse CSI1 configuration from the device tree
  omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode

Sakari Ailus (3):
  omap3isp: Always initialise isp and mutex for csiphy1
  omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module
  omap3isp: Correctly configure routing in PHY release

 drivers/media/platform/omap3isp/isp.c       | 105 +++++++++++++++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c   |   9 ++-
 drivers/media/platform/omap3isp/ispcsi2.c   |   4 +-
 drivers/media/platform/omap3isp/ispcsiphy.c |  59 ++++++++--------
 drivers/media/platform/omap3isp/ispcsiphy.h |   6 +-
 drivers/media/platform/omap3isp/ispreg.h    |   4 ++
 drivers/media/platform/omap3isp/omap3isp.h  |   1 +
 7 files changed, 126 insertions(+), 62 deletions(-)

-- 
2.11.0

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 1/5] omap3isp: Parse CSI1 configuration from the device tree
  2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
@ 2017-08-16 12:51 ` Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 2/5] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode Sakari Ailus
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

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: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # on Beagleboard-xM + MPT9P031
---
 drivers/media/platform/omap3isp/isp.c      | 105 +++++++++++++++++++++--------
 drivers/media/platform/omap3isp/omap3isp.h |   1 +
 2 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 79aff6b989a1..6cb1f0495804 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2018,6 +2018,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)
@@ -2047,41 +2048,91 @@ 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, "CSI-1/CCP-2 configuration\n");
+			csi1 = true;
+			break;
+		case V4L2_MBUS_CSI2:
+			dev_dbg(dev, "CSI-2 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 dfd3cbe26ccd..9fb4d5bce004 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -110,6 +110,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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 2/5] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode
  2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 1/5] omap3isp: Parse CSI1 configuration from the device tree Sakari Ailus
@ 2017-08-16 12:51 ` Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 3/5] omap3isp: Always initialise isp and mutex for csiphy1 Sakari Ailus
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

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>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # on Beagleboard-xM + MPT9P031
---
 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 8b6f7d2e79a0..47210b102bcb 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 3/5] omap3isp: Always initialise isp and mutex for csiphy1
  2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 1/5] omap3isp: Parse CSI1 configuration from the device tree Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 2/5] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode Sakari Ailus
@ 2017-08-16 12:51 ` Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module Sakari Ailus
  2017-08-16 12:51 ` [PATCH v2 5/5] omap3isp: Correctly configure routing in PHY release Sakari Ailus
  4 siblings, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

The PHY is still relevant for CCP2.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # on Beagleboard-xM + MPT9P031
---
 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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module
  2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
                   ` (2 preceding siblings ...)
  2017-08-16 12:51 ` [PATCH v2 3/5] omap3isp: Always initialise isp and mutex for csiphy1 Sakari Ailus
@ 2017-08-16 12:51 ` Sakari Ailus
  2017-08-16 12:54   ` Laurent Pinchart
  2017-08-16 12:51 ` [PATCH v2 5/5] omap3isp: Correctly configure routing in PHY release Sakari Ailus
  4 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

The CSI PHY is associated with a CSI receiver. The code assumes this
receiver is a CSI2 module and relies on the CSI2 module object heavily to
access the ISP or pipeline objects. However, the receiver could also be a
CSI1/CCP2 module.

Pass a new CSI receiver entity pointer to the CSI PHY acquire function, and
replace all hardcoded usage of the CSI2 module with that CSI receiver
entity.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # on Beagleboard-xM + MPT9P031
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/platform/omap3isp/ispccp2.c   |  2 +-
 drivers/media/platform/omap3isp/ispcsi2.c   |  4 +--
 drivers/media/platform/omap3isp/ispcsiphy.c | 45 +++++++++++++----------------
 drivers/media/platform/omap3isp/ispcsiphy.h |  6 ++--
 4 files changed, 27 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 47210b102bcb..3db8df09cd9a 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -841,7 +841,7 @@ static int ccp2_s_stream(struct v4l2_subdev *sd, int enable)
 	switch (enable) {
 	case ISP_PIPELINE_STREAM_CONTINUOUS:
 		if (ccp2->phy) {
-			ret = omap3isp_csiphy_acquire(ccp2->phy);
+			ret = omap3isp_csiphy_acquire(ccp2->phy, &sd->entity);
 			if (ret < 0)
 				return ret;
 		}
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 7dae2fe0d42d..3ec37fed710b 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -490,7 +490,7 @@ int omap3isp_csi2_reset(struct isp_csi2_device *csi2)
 	if (!csi2->available)
 		return -ENODEV;
 
-	if (csi2->phy->phy_in_use)
+	if (csi2->phy->entity)
 		return -EBUSY;
 
 	isp_reg_set(isp, csi2->regs1, ISPCSI2_SYSCONFIG,
@@ -1053,7 +1053,7 @@ static int csi2_set_stream(struct v4l2_subdev *sd, int enable)
 
 	switch (enable) {
 	case ISP_PIPELINE_STREAM_CONTINUOUS:
-		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
+		if (omap3isp_csiphy_acquire(csi2->phy, &sd->entity) < 0)
 			return -ENODEV;
 		if (csi2->output & CSI2_OUTPUT_MEMORY)
 			omap3isp_sbl_enable(isp, OMAP3_ISP_SBL_CSI2A_WRITE);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index 2028bb519108..ed1eb9907ae0 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -164,22 +164,17 @@ 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 = to_isp_pipeline(phy->entity);
+	struct isp_async_subdev *isd =
+		container_of(pipe->external->asd, struct isp_async_subdev, asd);
+	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
+		pipe->external->host_priv : &isd->bus;
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
 	unsigned int i;
 	u32 reg;
 
-	if (!buscfg) {
-		struct isp_async_subdev *isd =
-			container_of(pipe->external->asd,
-				     struct isp_async_subdev, asd);
-		buscfg = &isd->bus;
-	}
-
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
 	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
 		lanes = &buscfg->bus.ccp2.lanecfg;
@@ -222,7 +217,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 +228,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 +239,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,12 +258,12 @@ 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;
 }
 
-int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
+int omap3isp_csiphy_acquire(struct isp_csiphy *phy, struct media_entity *entity)
 {
 	int rval;
 
@@ -288,6 +283,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 	if (rval < 0)
 		goto done;
 
+	phy->entity = entity;
+
 	rval = omap3isp_csiphy_config(phy);
 	if (rval < 0)
 		goto done;
@@ -301,10 +298,10 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 
 		csiphy_power_autoswitch_enable(phy, true);
 	}
-
-	phy->phy_in_use = 1;
-
 done:
+	if (rval < 0)
+		phy->entity = NULL;
+
 	mutex_unlock(&phy->mutex);
 	return rval;
 }
@@ -312,10 +309,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
 void omap3isp_csiphy_release(struct isp_csiphy *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->entity) {
+		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
 		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
@@ -325,7 +320,7 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 			csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
 		}
 		regulator_disable(phy->vdd);
-		phy->phy_in_use = 0;
+		phy->entity = NULL;
 	}
 	mutex_unlock(&phy->mutex);
 }
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h b/drivers/media/platform/omap3isp/ispcsiphy.h
index 978ca5c80a6c..91543a09b28a 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.h
+++ b/drivers/media/platform/omap3isp/ispcsiphy.h
@@ -25,9 +25,10 @@ struct regulator;
 struct isp_csiphy {
 	struct isp_device *isp;
 	struct mutex mutex;	/* serialize csiphy configuration */
-	u8 phy_in_use;
 	struct isp_csi2_device *csi2;
 	struct regulator *vdd;
+	/* the entity that acquired the phy */
+	struct media_entity *entity;
 
 	/* mem resources - enums as defined in enum isp_mem_resources */
 	unsigned int cfg_regs;
@@ -36,7 +37,8 @@ struct isp_csiphy {
 	u8 num_data_lanes;	/* number of CSI2 Data Lanes supported */
 };
 
-int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
+int omap3isp_csiphy_acquire(struct isp_csiphy *phy,
+			    struct media_entity *entity);
 void omap3isp_csiphy_release(struct isp_csiphy *phy);
 int omap3isp_csiphy_init(struct isp_device *isp);
 void omap3isp_csiphy_cleanup(struct isp_device *isp);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module
  2017-08-16 12:51 ` [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module Sakari Ailus
@ 2017-08-16 12:54   ` Laurent Pinchart
  0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2017-08-16 12:54 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, pavel

Hi Sakari,

Thank you for the patch.

On Wednesday 16 Aug 2017 15:51:49 Sakari Ailus wrote:
> The CSI PHY is associated with a CSI receiver. The code assumes this
> receiver is a CSI2 module and relies on the CSI2 module object heavily to
> access the ISP or pipeline objects. However, the receiver could also be a
> CSI1/CCP2 module.
> 
> Pass a new CSI receiver entity pointer to the CSI PHY acquire function, and
> replace all hardcoded usage of the CSI2 module with that CSI receiver
> entity.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> # on
> Beagleboard-xM + MPT9P031
> Acked-by: Pavel Machek <pavel@ucw.cz>

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

> ---
>  drivers/media/platform/omap3isp/ispccp2.c   |  2 +-
>  drivers/media/platform/omap3isp/ispcsi2.c   |  4 +--
>  drivers/media/platform/omap3isp/ispcsiphy.c | 45 +++++++++++--------------
>  drivers/media/platform/omap3isp/ispcsiphy.h |  6 ++--
>  4 files changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 47210b102bcb..3db8df09cd9a 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -841,7 +841,7 @@ static int ccp2_s_stream(struct v4l2_subdev *sd, int
> enable) switch (enable) {
>  	case ISP_PIPELINE_STREAM_CONTINUOUS:
>  		if (ccp2->phy) {
> -			ret = omap3isp_csiphy_acquire(ccp2->phy);
> +			ret = omap3isp_csiphy_acquire(ccp2->phy, &sd->entity);
>  			if (ret < 0)
>  				return ret;
>  		}
> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> b/drivers/media/platform/omap3isp/ispcsi2.c index
> 7dae2fe0d42d..3ec37fed710b 100644
> --- a/drivers/media/platform/omap3isp/ispcsi2.c
> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> @@ -490,7 +490,7 @@ int omap3isp_csi2_reset(struct isp_csi2_device *csi2)
>  	if (!csi2->available)
>  		return -ENODEV;
> 
> -	if (csi2->phy->phy_in_use)
> +	if (csi2->phy->entity)
>  		return -EBUSY;
> 
>  	isp_reg_set(isp, csi2->regs1, ISPCSI2_SYSCONFIG,
> @@ -1053,7 +1053,7 @@ static int csi2_set_stream(struct v4l2_subdev *sd, int
> enable)
> 
>  	switch (enable) {
>  	case ISP_PIPELINE_STREAM_CONTINUOUS:
> -		if (omap3isp_csiphy_acquire(csi2->phy) < 0)
> +		if (omap3isp_csiphy_acquire(csi2->phy, &sd->entity) < 0)
>  			return -ENODEV;
>  		if (csi2->output & CSI2_OUTPUT_MEMORY)
>  			omap3isp_sbl_enable(isp, OMAP3_ISP_SBL_CSI2A_WRITE);
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> 2028bb519108..ed1eb9907ae0 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -164,22 +164,17 @@ 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 = to_isp_pipeline(phy->entity);
> +	struct isp_async_subdev *isd =
> +		container_of(pipe->external->asd, struct isp_async_subdev, 
asd);
> +	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
> +		pipe->external->host_priv : &isd->bus;
>  	struct isp_csiphy_lanes_cfg *lanes;
>  	int csi2_ddrclk_khz;
>  	unsigned int num_data_lanes, used_lanes = 0;
>  	unsigned int i;
>  	u32 reg;
> 
> -	if (!buscfg) {
> -		struct isp_async_subdev *isd =
> -			container_of(pipe->external->asd,
> -				     struct isp_async_subdev, asd);
> -		buscfg = &isd->bus;
> -	}
> -
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> 
>  	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2) {
> 
>  		lanes = &buscfg->bus.ccp2.lanecfg;
> @@ -222,7 +217,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 +228,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 +239,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,12 +258,12 @@ 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;
>  }
> 
> -int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> +int omap3isp_csiphy_acquire(struct isp_csiphy *phy, struct media_entity
> *entity) {
>  	int rval;
> 
> @@ -288,6 +283,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  	if (rval < 0)
>  		goto done;
> 
> +	phy->entity = entity;
> +
>  	rval = omap3isp_csiphy_config(phy);
>  	if (rval < 0)
>  		goto done;
> @@ -301,10 +298,10 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
> 
>  		csiphy_power_autoswitch_enable(phy, true);
>  	}
> -
> -	phy->phy_in_use = 1;
> -
>  done:
> +	if (rval < 0)
> +		phy->entity = NULL;
> +
>  	mutex_unlock(&phy->mutex);
>  	return rval;
>  }
> @@ -312,10 +309,8 @@ int omap3isp_csiphy_acquire(struct isp_csiphy *phy)
>  void omap3isp_csiphy_release(struct isp_csiphy *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->entity) {
> +		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
>  		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> 
>  		csiphy_routing_cfg(phy, buscfg->interface, false,
> @@ -325,7 +320,7 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
>  			csiphy_set_power(phy, ISPCSI2_PHY_CFG_PWR_CMD_OFF);
>  		}
>  		regulator_disable(phy->vdd);
> -		phy->phy_in_use = 0;
> +		phy->entity = NULL;
>  	}
>  	mutex_unlock(&phy->mutex);
>  }
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.h
> b/drivers/media/platform/omap3isp/ispcsiphy.h index
> 978ca5c80a6c..91543a09b28a 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.h
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.h
> @@ -25,9 +25,10 @@ struct regulator;
>  struct isp_csiphy {
>  	struct isp_device *isp;
>  	struct mutex mutex;	/* serialize csiphy configuration */
> -	u8 phy_in_use;
>  	struct isp_csi2_device *csi2;
>  	struct regulator *vdd;
> +	/* the entity that acquired the phy */
> +	struct media_entity *entity;
> 
>  	/* mem resources - enums as defined in enum isp_mem_resources */
>  	unsigned int cfg_regs;
> @@ -36,7 +37,8 @@ struct isp_csiphy {
>  	u8 num_data_lanes;	/* number of CSI2 Data Lanes supported */
>  };
> 
> -int omap3isp_csiphy_acquire(struct isp_csiphy *phy);
> +int omap3isp_csiphy_acquire(struct isp_csiphy *phy,
> +			    struct media_entity *entity);
>  void omap3isp_csiphy_release(struct isp_csiphy *phy);
>  int omap3isp_csiphy_init(struct isp_device *isp);
>  void omap3isp_csiphy_cleanup(struct isp_device *isp);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2 5/5] omap3isp: Correctly configure routing in PHY release
  2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
                   ` (3 preceding siblings ...)
  2017-08-16 12:51 ` [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module Sakari Ailus
@ 2017-08-16 12:51 ` Sakari Ailus
  2017-08-16 16:52   ` [PATCH v2.1 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field Sakari Ailus
  4 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 12:51 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

The PHY configuration was obtained from DT when the PHY was acquired but
the same was not done when it was released. Fix this.

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

diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..45ed1adbd9ae 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -155,6 +155,17 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 	return 0;
 }
 
+static struct isp_bus_cfg *omap3isp_csiphy_get_phy_cfg(
+	struct isp_csiphy *phy)
+{
+	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
+	struct isp_async_subdev *isd =
+		container_of(pipe->external->asd, struct isp_async_subdev, asd);
+
+	return pipe->external->host_priv ?
+		pipe->external->host_priv : &isd->bus;
+}
+
 /*
  * TCLK values are OK at their reset values
  */
@@ -165,10 +176,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg = omap3isp_csiphy_get_phy_cfg(phy);
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -310,8 +318,7 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 {
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
-		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg = omap3isp_csiphy_get_phy_cfg(phy);
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2.1 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 12:51 ` [PATCH v2 5/5] omap3isp: Correctly configure routing in PHY release Sakari Ailus
@ 2017-08-16 16:52   ` Sakari Ailus
  2017-08-16 17:05     ` [PATCH v2.2 " Sakari Ailus
  0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 16:52 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

struct v4l2_subdev.host_priv is intended to be used by another driver. This
is hardly good design but back in the days of platform data was a quick
hack to get things done.

As the sub-device specific bus information can be stored to the ISP driver
specific struct allocated along with v4l2_async_subdev, keep the
information there and only there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
This patch replaces patch "omap3isp: Correctly configure routing in PHY
release".

 drivers/media/platform/omap3isp/isp.c       | 31 +++++++++--------------------
 drivers/media/platform/omap3isp/ispccdc.c   | 20 ++++++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c   |  4 +++-
 drivers/media/platform/omap3isp/ispcsi2.c   |  3 ++-
 drivers/media/platform/omap3isp/ispcsiphy.c | 11 +++++-----
 5 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6cb1f0495804..27c577fac8e9 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
 	return -EINVAL;
 }
 
-static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct isp_async_subdev *isd =
-		container_of(asd, struct isp_async_subdev, asd);
-
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
 	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
 	int ret;
 
 	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
@@ -2215,13 +2201,15 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		return ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
+		struct isp_async_subdev *isd;
+
+		if (!sd->asd)
+			continue;
+
+		isd = container_of(sd->asd, struct isp_async_subdev, asd);
+		ret = isp_link_entity(isp, &sd->entity, isd->bus.interface);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
@@ -2399,7 +2387,6 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_register_entities;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
 	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 4947876cfadf..0145b3dcd7a7 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1139,8 +1139,12 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
-			->bus.parallel;
+		struct isp_pipeline *pipe =
+			to_isp_pipeline(&ccdc->subdev.entity);
+
+		parcfg = &container_of(pipe->external->asd,
+				       struct isp_async_subdev,
+				       asd)->bus.bus.parallel;
 		ccdc->bt656 = parcfg->bt656;
 	}
 
@@ -2412,11 +2416,13 @@ static int ccdc_link_validate(struct v4l2_subdev *sd,
 
 	/* We've got a parallel sensor here. */
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		struct isp_parallel_cfg *parcfg =
-			&((struct isp_bus_cfg *)
-			  media_entity_to_v4l2_subdev(link->source->entity)
-			  ->host_priv)->bus.parallel;
-		parallel_shift = parcfg->data_lane_shift;
+		struct isp_pipeline *pipe =
+			to_isp_pipeline(&ccdc->subdev.entity);
+
+		parallel_shift =
+			container_of(pipe->external->asd,
+				     struct isp_async_subdev,
+				     asd)->bus.bus.parallel.data_lane_shift;
 	} else {
 		parallel_shift = 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 3db8df09cd9a..8561c4e4c5ac 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
  */
 static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 {
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	const struct isp_bus_cfg *buscfg;
 	struct v4l2_mbus_framefmt *format;
 	struct media_pad *pad;
@@ -361,7 +362,8 @@ static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 
 	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = &container_of(pipe->external->asd,
+			       struct isp_async_subdev, asd)->bus;
 
 	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
 	if (ret < 0)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 3ec37fed710b..2802f70fc8b3 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -566,7 +566,8 @@ static int csi2_configure(struct isp_csi2_device *csi2)
 
 	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = &container_of(pipe->external->asd,
+			       struct isp_async_subdev, asd)->bus;
 
 	csi2->frame_skip = 0;
 	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..ef79bf37c2dd 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -165,10 +165,9 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg =
+		&container_of(pipe->external->asd,
+			      struct isp_async_subdev, asd)->bus;
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -311,7 +310,9 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
 		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg =
+			&container_of(pipe->external->asd,
+				      struct isp_async_subdev, asd)->bus;
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2.2 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 16:52   ` [PATCH v2.1 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field Sakari Ailus
@ 2017-08-16 17:05     ` Sakari Ailus
  2017-08-16 17:24       ` Laurent Pinchart
  0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 17:05 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

struct v4l2_subdev.host_priv is intended to be used by another driver. This
is hardly good design but back in the days of platform data was a quick
hack to get things done.

As the sub-device specific bus information can be stored to the ISP driver
specific struct allocated along with v4l2_async_subdev, keep the
information there and only there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v2.1:

- Remove struct isp_async_subdev.sd field as it is now redundant.

 drivers/media/platform/omap3isp/isp.c       | 31 +++++++++--------------------
 drivers/media/platform/omap3isp/isp.h       |  1 -
 drivers/media/platform/omap3isp/ispccdc.c   | 20 ++++++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c   |  4 +++-
 drivers/media/platform/omap3isp/ispcsi2.c   |  3 ++-
 drivers/media/platform/omap3isp/ispcsiphy.c | 11 +++++-----
 6 files changed, 33 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6cb1f0495804..27c577fac8e9 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
 	return -EINVAL;
 }
 
-static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct isp_async_subdev *isd =
-		container_of(asd, struct isp_async_subdev, asd);
-
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
 	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
 	int ret;
 
 	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
@@ -2215,13 +2201,15 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		return ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
+		struct isp_async_subdev *isd;
+
+		if (!sd->asd)
+			continue;
+
+		isd = container_of(sd->asd, struct isp_async_subdev, asd);
+		ret = isp_link_entity(isp, &sd->entity, isd->bus.interface);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
@@ -2399,7 +2387,6 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_register_entities;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
 	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 2f2ae609c548..25472c81dcdd 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -226,7 +226,6 @@ struct isp_device {
 };
 
 struct isp_async_subdev {
-	struct v4l2_subdev *sd;
 	struct isp_bus_cfg bus;
 	struct v4l2_async_subdev asd;
 };
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 4947876cfadf..0145b3dcd7a7 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1139,8 +1139,12 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
-			->bus.parallel;
+		struct isp_pipeline *pipe =
+			to_isp_pipeline(&ccdc->subdev.entity);
+
+		parcfg = &container_of(pipe->external->asd,
+				       struct isp_async_subdev,
+				       asd)->bus.bus.parallel;
 		ccdc->bt656 = parcfg->bt656;
 	}
 
@@ -2412,11 +2416,13 @@ static int ccdc_link_validate(struct v4l2_subdev *sd,
 
 	/* We've got a parallel sensor here. */
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		struct isp_parallel_cfg *parcfg =
-			&((struct isp_bus_cfg *)
-			  media_entity_to_v4l2_subdev(link->source->entity)
-			  ->host_priv)->bus.parallel;
-		parallel_shift = parcfg->data_lane_shift;
+		struct isp_pipeline *pipe =
+			to_isp_pipeline(&ccdc->subdev.entity);
+
+		parallel_shift =
+			container_of(pipe->external->asd,
+				     struct isp_async_subdev,
+				     asd)->bus.bus.parallel.data_lane_shift;
 	} else {
 		parallel_shift = 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 3db8df09cd9a..8561c4e4c5ac 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
  */
 static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 {
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	const struct isp_bus_cfg *buscfg;
 	struct v4l2_mbus_framefmt *format;
 	struct media_pad *pad;
@@ -361,7 +362,8 @@ static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 
 	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = &container_of(pipe->external->asd,
+			       struct isp_async_subdev, asd)->bus;
 
 	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
 	if (ret < 0)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 3ec37fed710b..2802f70fc8b3 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -566,7 +566,8 @@ static int csi2_configure(struct isp_csi2_device *csi2)
 
 	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = &container_of(pipe->external->asd,
+			       struct isp_async_subdev, asd)->bus;
 
 	csi2->frame_skip = 0;
 	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..ef79bf37c2dd 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -165,10 +165,9 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg =
+		&container_of(pipe->external->asd,
+			      struct isp_async_subdev, asd)->bus;
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -311,7 +310,9 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
 		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg =
+			&container_of(pipe->external->asd,
+				      struct isp_async_subdev, asd)->bus;
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2.2 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 17:05     ` [PATCH v2.2 " Sakari Ailus
@ 2017-08-16 17:24       ` Laurent Pinchart
  2017-08-16 17:26         ` Sakari Ailus
  2017-08-16 17:39         ` [PATCH v2.3 " Sakari Ailus
  0 siblings, 2 replies; 16+ messages in thread
From: Laurent Pinchart @ 2017-08-16 17:24 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, pavel

Hi Sakari,

Thank you for the patch.

On Wednesday 16 Aug 2017 20:05:52 Sakari Ailus wrote:
> struct v4l2_subdev.host_priv is intended to be used by another driver. This
> is hardly good design but back in the days of platform data was a quick
> hack to get things done.
> 
> As the sub-device specific bus information can be stored to the ISP driver
> specific struct allocated along with v4l2_async_subdev, keep the
> information there and only there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This looks better to me than the previous approach. Please see below for one 
last comment.

> ---
> since v2.1:
> 
> - Remove struct isp_async_subdev.sd field as it is now redundant.
> 
>  drivers/media/platform/omap3isp/isp.c       | 31 ++++++++------------------
>  drivers/media/platform/omap3isp/isp.h       |  1 -
>  drivers/media/platform/omap3isp/ispccdc.c   | 20 ++++++++++++-------
>  drivers/media/platform/omap3isp/ispccp2.c   |  4 +++-
>  drivers/media/platform/omap3isp/ispcsi2.c   |  3 ++-
>  drivers/media/platform/omap3isp/ispcsiphy.c | 11 +++++-----
>  6 files changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 6cb1f0495804..27c577fac8e9
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
>  	return -EINVAL;
>  }
> 
> -static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> -{
> -	struct isp_async_subdev *isd =
> -		container_of(asd, struct isp_async_subdev, asd);
> -
> -	isd->sd = subdev;
> -	isd->sd->host_priv = &isd->bus;
> -
> -	return 0;
> -}
> -
>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  {
>  	struct isp_device *isp = container_of(async, struct isp_device,
>  					      notifier);
>  	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
>  	struct v4l2_subdev *sd;
> -	struct isp_bus_cfg *bus;
>  	int ret;
> 
>  	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> @@ -2215,13 +2201,15 @@ static int isp_subdev_notifier_complete(struct
> v4l2_async_notifier *async) return ret;
> 
>  	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> -		/* Only try to link entities whose interface was set on bound 
*/
> -		if (sd->host_priv) {
> -			bus = (struct isp_bus_cfg *)sd->host_priv;
> -			ret = isp_link_entity(isp, &sd->entity, bus-
>interface);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		struct isp_async_subdev *isd;
> +
> +		if (!sd->asd)
> +			continue;
> +
> +		isd = container_of(sd->asd, struct isp_async_subdev, asd);
> +		ret = isp_link_entity(isp, &sd->entity, isd->bus.interface);
> +		if (ret < 0)
> +			return ret;
>  	}
> 
>  	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> @@ -2399,7 +2387,6 @@ static int isp_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_register_entities;
> 
> -	isp->notifier.bound = isp_subdev_notifier_bound;
>  	isp->notifier.complete = isp_subdev_notifier_complete;
> 
>  	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 2f2ae609c548..25472c81dcdd
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -226,7 +226,6 @@ struct isp_device {
>  };
> 
>  struct isp_async_subdev {
> -	struct v4l2_subdev *sd;
>  	struct isp_bus_cfg bus;
>  	struct v4l2_async_subdev asd;
>  };
> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> b/drivers/media/platform/omap3isp/ispccdc.c index
> 4947876cfadf..0145b3dcd7a7 100644
> --- a/drivers/media/platform/omap3isp/ispccdc.c
> +++ b/drivers/media/platform/omap3isp/ispccdc.c
> @@ -1139,8 +1139,12 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
> -			->bus.parallel;
> +		struct isp_pipeline *pipe =
> +			to_isp_pipeline(&ccdc->subdev.entity);
> +
> +		parcfg = &container_of(pipe->external->asd,
> +				       struct isp_async_subdev,
> +				       asd)->bus.bus.parallel;

You use this construct in many places, how about adding

struct isp_bus_cfg *to_bus_cfg(struct v4l2_subdev *sd)
{
	return &container_of(sd->asd, struct isp_async_subdev, asd)->bus;
}

or possibly a macro to handle the const/non-const cases.

>  		ccdc->bt656 = parcfg->bt656;
>  	}
> 
> @@ -2412,11 +2416,13 @@ static int ccdc_link_validate(struct v4l2_subdev
> *sd,
> 
>  	/* We've got a parallel sensor here. */
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		struct isp_parallel_cfg *parcfg =
> -			&((struct isp_bus_cfg *)
> -			  media_entity_to_v4l2_subdev(link->source->entity)
> -			  ->host_priv)->bus.parallel;
> -		parallel_shift = parcfg->data_lane_shift;
> +		struct isp_pipeline *pipe =
> +			to_isp_pipeline(&ccdc->subdev.entity);
> +
> +		parallel_shift =
> +			container_of(pipe->external->asd,
> +				     struct isp_async_subdev,
> +				     asd)->bus.bus.parallel.data_lane_shift;
>  	} else {
>  		parallel_shift = 0;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 3db8df09cd9a..8561c4e4c5ac 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> *ccp2, */
>  static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
>  {
> +	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
>  	const struct isp_bus_cfg *buscfg;
>  	struct v4l2_mbus_framefmt *format;
>  	struct media_pad *pad;
> @@ -361,7 +362,8 @@ static int ccp2_if_configure(struct isp_ccp2_device
> *ccp2)
> 
>  	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = &container_of(pipe->external->asd,
> +			       struct isp_async_subdev, asd)->bus;
> 
>  	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
>  	if (ret < 0)
> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> b/drivers/media/platform/omap3isp/ispcsi2.c index
> 3ec37fed710b..2802f70fc8b3 100644
> --- a/drivers/media/platform/omap3isp/ispcsi2.c
> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> @@ -566,7 +566,8 @@ static int csi2_configure(struct isp_csi2_device *csi2)
> 
>  	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = &container_of(pipe->external->asd,
> +			       struct isp_async_subdev, asd)->bus;
> 
>  	csi2->frame_skip = 0;
>  	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> ed1eb9907ae0..ef79bf37c2dd 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -165,10 +165,9 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> power) static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  {
>  	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -	struct isp_async_subdev *isd =
> -		container_of(pipe->external->asd, struct isp_async_subdev, 
asd);
> -	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
> -		pipe->external->host_priv : &isd->bus;
> +	struct isp_bus_cfg *buscfg =
> +		&container_of(pipe->external->asd,
> +			      struct isp_async_subdev, asd)->bus;
>  	struct isp_csiphy_lanes_cfg *lanes;
>  	int csi2_ddrclk_khz;
>  	unsigned int num_data_lanes, used_lanes = 0;
> @@ -311,7 +310,9 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
>  	mutex_lock(&phy->mutex);
>  	if (phy->entity) {
>  		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> +		struct isp_bus_cfg *buscfg =
> +			&container_of(pipe->external->asd,
> +				      struct isp_async_subdev, asd)->bus;
> 
>  		csiphy_routing_cfg(phy, buscfg->interface, false,
>  				   buscfg->bus.ccp2.phy_layer);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2.2 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 17:24       ` Laurent Pinchart
@ 2017-08-16 17:26         ` Sakari Ailus
  2017-08-16 17:39         ` [PATCH v2.3 " Sakari Ailus
  1 sibling, 0 replies; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 17:26 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, pavel

On Wed, Aug 16, 2017 at 08:24:19PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Wednesday 16 Aug 2017 20:05:52 Sakari Ailus wrote:
> > struct v4l2_subdev.host_priv is intended to be used by another driver. This
> > is hardly good design but back in the days of platform data was a quick
> > hack to get things done.
> > 
> > As the sub-device specific bus information can be stored to the ISP driver
> > specific struct allocated along with v4l2_async_subdev, keep the
> > information there and only there.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> This looks better to me than the previous approach. Please see below for one 
> last comment.
> 
> > ---
> > since v2.1:
> > 
> > - Remove struct isp_async_subdev.sd field as it is now redundant.
> > 
> >  drivers/media/platform/omap3isp/isp.c       | 31 ++++++++------------------
> >  drivers/media/platform/omap3isp/isp.h       |  1 -
> >  drivers/media/platform/omap3isp/ispccdc.c   | 20 ++++++++++++-------
> >  drivers/media/platform/omap3isp/ispccp2.c   |  4 +++-
> >  drivers/media/platform/omap3isp/ispcsi2.c   |  3 ++-
> >  drivers/media/platform/omap3isp/ispcsiphy.c | 11 +++++-----
> >  6 files changed, 33 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 6cb1f0495804..27c577fac8e9
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
> >  	return -EINVAL;
> >  }
> > 
> > -static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> > -				     struct v4l2_subdev *subdev,
> > -				     struct v4l2_async_subdev *asd)
> > -{
> > -	struct isp_async_subdev *isd =
> > -		container_of(asd, struct isp_async_subdev, asd);
> > -
> > -	isd->sd = subdev;
> > -	isd->sd->host_priv = &isd->bus;
> > -
> > -	return 0;
> > -}
> > -
> >  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> >  {
> >  	struct isp_device *isp = container_of(async, struct isp_device,
> >  					      notifier);
> >  	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
> >  	struct v4l2_subdev *sd;
> > -	struct isp_bus_cfg *bus;
> >  	int ret;
> > 
> >  	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> > @@ -2215,13 +2201,15 @@ static int isp_subdev_notifier_complete(struct
> > v4l2_async_notifier *async) return ret;
> > 
> >  	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> > -		/* Only try to link entities whose interface was set on bound 
> */
> > -		if (sd->host_priv) {
> > -			bus = (struct isp_bus_cfg *)sd->host_priv;
> > -			ret = isp_link_entity(isp, &sd->entity, bus-
> >interface);
> > -			if (ret < 0)
> > -				return ret;
> > -		}
> > +		struct isp_async_subdev *isd;
> > +
> > +		if (!sd->asd)
> > +			continue;
> > +
> > +		isd = container_of(sd->asd, struct isp_async_subdev, asd);
> > +		ret = isp_link_entity(isp, &sd->entity, isd->bus.interface);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> > 
> >  	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> > @@ -2399,7 +2387,6 @@ static int isp_probe(struct platform_device *pdev)
> >  	if (ret < 0)
> >  		goto error_register_entities;
> > 
> > -	isp->notifier.bound = isp_subdev_notifier_bound;
> >  	isp->notifier.complete = isp_subdev_notifier_complete;
> > 
> >  	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
> > diff --git a/drivers/media/platform/omap3isp/isp.h
> > b/drivers/media/platform/omap3isp/isp.h index 2f2ae609c548..25472c81dcdd
> > 100644
> > --- a/drivers/media/platform/omap3isp/isp.h
> > +++ b/drivers/media/platform/omap3isp/isp.h
> > @@ -226,7 +226,6 @@ struct isp_device {
> >  };
> > 
> >  struct isp_async_subdev {
> > -	struct v4l2_subdev *sd;
> >  	struct isp_bus_cfg bus;
> >  	struct v4l2_async_subdev asd;
> >  };
> > diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> > b/drivers/media/platform/omap3isp/ispccdc.c index
> > 4947876cfadf..0145b3dcd7a7 100644
> > --- a/drivers/media/platform/omap3isp/ispccdc.c
> > +++ b/drivers/media/platform/omap3isp/ispccdc.c
> > @@ -1139,8 +1139,12 @@ static void ccdc_configure(struct isp_ccdc_device
> > *ccdc) pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
> >  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> >  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> > -		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
> > -			->bus.parallel;
> > +		struct isp_pipeline *pipe =
> > +			to_isp_pipeline(&ccdc->subdev.entity);
> > +
> > +		parcfg = &container_of(pipe->external->asd,
> > +				       struct isp_async_subdev,
> > +				       asd)->bus.bus.parallel;
> 
> You use this construct in many places, how about adding
> 
> struct isp_bus_cfg *to_bus_cfg(struct v4l2_subdev *sd)
> {
> 	return &container_of(sd->asd, struct isp_async_subdev, asd)->bus;
> }
> 
> or possibly a macro to handle the const/non-const cases.

I'll add that and send v2.3.

> 
> >  		ccdc->bt656 = parcfg->bt656;
> >  	}
> > 
> > @@ -2412,11 +2416,13 @@ static int ccdc_link_validate(struct v4l2_subdev
> > *sd,
> > 
> >  	/* We've got a parallel sensor here. */
> >  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> > -		struct isp_parallel_cfg *parcfg =
> > -			&((struct isp_bus_cfg *)
> > -			  media_entity_to_v4l2_subdev(link->source->entity)
> > -			  ->host_priv)->bus.parallel;
> > -		parallel_shift = parcfg->data_lane_shift;
> > +		struct isp_pipeline *pipe =
> > +			to_isp_pipeline(&ccdc->subdev.entity);
> > +
> > +		parallel_shift =
> > +			container_of(pipe->external->asd,
> > +				     struct isp_async_subdev,
> > +				     asd)->bus.bus.parallel.data_lane_shift;
> >  	} else {
> >  		parallel_shift = 0;
> >  	}
> > diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> > b/drivers/media/platform/omap3isp/ispccp2.c index
> > 3db8df09cd9a..8561c4e4c5ac 100644
> > --- a/drivers/media/platform/omap3isp/ispccp2.c
> > +++ b/drivers/media/platform/omap3isp/ispccp2.c
> > @@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> > *ccp2, */
> >  static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
> >  {
> > +	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
> >  	const struct isp_bus_cfg *buscfg;
> >  	struct v4l2_mbus_framefmt *format;
> >  	struct media_pad *pad;
> > @@ -361,7 +362,8 @@ static int ccp2_if_configure(struct isp_ccp2_device
> > *ccp2)
> > 
> >  	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
> >  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> > -	buscfg = sensor->host_priv;
> > +	buscfg = &container_of(pipe->external->asd,
> > +			       struct isp_async_subdev, asd)->bus;
> > 
> >  	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
> >  	if (ret < 0)
> > diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> > b/drivers/media/platform/omap3isp/ispcsi2.c index
> > 3ec37fed710b..2802f70fc8b3 100644
> > --- a/drivers/media/platform/omap3isp/ispcsi2.c
> > +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> > @@ -566,7 +566,8 @@ static int csi2_configure(struct isp_csi2_device *csi2)
> > 
> >  	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
> >  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> > -	buscfg = sensor->host_priv;
> > +	buscfg = &container_of(pipe->external->asd,
> > +			       struct isp_async_subdev, asd)->bus;
> > 
> >  	csi2->frame_skip = 0;
> >  	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
> > diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> > b/drivers/media/platform/omap3isp/ispcsiphy.c index
> > ed1eb9907ae0..ef79bf37c2dd 100644
> > --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> > +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> > @@ -165,10 +165,9 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> > power) static int omap3isp_csiphy_config(struct isp_csiphy *phy)
> >  {
> >  	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> > -	struct isp_async_subdev *isd =
> > -		container_of(pipe->external->asd, struct isp_async_subdev, 
> asd);
> > -	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
> > -		pipe->external->host_priv : &isd->bus;
> > +	struct isp_bus_cfg *buscfg =
> > +		&container_of(pipe->external->asd,
> > +			      struct isp_async_subdev, asd)->bus;
> >  	struct isp_csiphy_lanes_cfg *lanes;
> >  	int csi2_ddrclk_khz;
> >  	unsigned int num_data_lanes, used_lanes = 0;
> > @@ -311,7 +310,9 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
> >  	mutex_lock(&phy->mutex);
> >  	if (phy->entity) {
> >  		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> > -		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> > +		struct isp_bus_cfg *buscfg =
> > +			&container_of(pipe->external->asd,
> > +				      struct isp_async_subdev, asd)->bus;
> > 
> >  		csiphy_routing_cfg(phy, buscfg->interface, false,
> >  				   buscfg->bus.ccp2.phy_layer);
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2.3 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 17:24       ` Laurent Pinchart
  2017-08-16 17:26         ` Sakari Ailus
@ 2017-08-16 17:39         ` Sakari Ailus
  2017-08-16 19:32           ` Laurent Pinchart
  1 sibling, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 17:39 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

struct v4l2_subdev.host_priv is intended to be used by another driver. This
is hardly good design but back in the days of platform data was a quick
hack to get things done.

As the sub-device specific bus information can be stored to the ISP driver
specific struct allocated along with v4l2_async_subdev, keep the
information there and only there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v2.2:

- Introduce a macro v4l2_subdev_to_bus_cfg() in order to remove
  non-trivial usage in places where conversion from sub-device to ISP bus
  configuration is needed.

 drivers/media/platform/omap3isp/isp.c       | 29 +++++++----------------------
 drivers/media/platform/omap3isp/isp.h       |  4 +++-
 drivers/media/platform/omap3isp/ispccdc.c   | 12 ++++++------
 drivers/media/platform/omap3isp/ispccp2.c   |  3 ++-
 drivers/media/platform/omap3isp/ispcsi2.c   |  2 +-
 drivers/media/platform/omap3isp/ispcsiphy.c |  8 +++-----
 6 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6cb1f0495804..c3014c82d64d 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
 	return -EINVAL;
 }
 
-static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct isp_async_subdev *isd =
-		container_of(asd, struct isp_async_subdev, asd);
-
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
 	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
 	int ret;
 
 	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
@@ -2215,13 +2201,13 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		return ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
+		if (!sd->asd)
+			continue;
+
+		ret = isp_link_entity(isp, &sd->entity,
+				      v4l2_subdev_to_bus_cfg(sd)->interface);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
@@ -2399,7 +2385,6 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_register_entities;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
 	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 2f2ae609c548..e528df6efc09 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -226,11 +226,13 @@ struct isp_device {
 };
 
 struct isp_async_subdev {
-	struct v4l2_subdev *sd;
 	struct isp_bus_cfg bus;
 	struct v4l2_async_subdev asd;
 };
 
+#define v4l2_subdev_to_bus_cfg(sd) \
+	(&container_of((sd)->asd, struct isp_async_subdev, asd)->bus)
+
 #define v4l2_dev_to_isp_device(dev) \
 	container_of(dev, struct isp_device, v4l2_dev)
 
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 4947876cfadf..80fed9526945 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1139,7 +1139,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
+		parcfg = &v4l2_subdev_to_bus_cfg(
+			to_isp_pipeline(&ccdc->subdev.entity)->external)
 			->bus.parallel;
 		ccdc->bt656 = parcfg->bt656;
 	}
@@ -2412,11 +2413,10 @@ static int ccdc_link_validate(struct v4l2_subdev *sd,
 
 	/* We've got a parallel sensor here. */
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		struct isp_parallel_cfg *parcfg =
-			&((struct isp_bus_cfg *)
-			  media_entity_to_v4l2_subdev(link->source->entity)
-			  ->host_priv)->bus.parallel;
-		parallel_shift = parcfg->data_lane_shift;
+		parallel_shift =
+			v4l2_subdev_to_bus_cfg(
+				to_isp_pipeline(&ccdc->subdev.entity)->external)
+			->bus.parallel.data_lane_shift;
 	} else {
 		parallel_shift = 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 3db8df09cd9a..e062939d0d05 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
  */
 static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 {
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	const struct isp_bus_cfg *buscfg;
 	struct v4l2_mbus_framefmt *format;
 	struct media_pad *pad;
@@ -361,7 +362,7 @@ static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 
 	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
 	if (ret < 0)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 3ec37fed710b..a4d3d030e81e 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -566,7 +566,7 @@ static int csi2_configure(struct isp_csi2_device *csi2)
 
 	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	csi2->frame_skip = 0;
 	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..a28fb79abaac 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -165,10 +165,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -311,7 +308,8 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
 		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg =
+			v4l2_subdev_to_bus_cfg(pipe->external);
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2.3 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 17:39         ` [PATCH v2.3 " Sakari Ailus
@ 2017-08-16 19:32           ` Laurent Pinchart
  2017-08-16 20:39             ` [PATCH v2.4 " Sakari Ailus
  0 siblings, 1 reply; 16+ messages in thread
From: Laurent Pinchart @ 2017-08-16 19:32 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, pavel

Hi Sakari,

Thank you for the patch.

On Wednesday 16 Aug 2017 20:39:43 Sakari Ailus wrote:
> struct v4l2_subdev.host_priv is intended to be used by another driver. This
> is hardly good design but back in the days of platform data was a quick
> hack to get things done.
> 
> As the sub-device specific bus information can be stored to the ISP driver
> specific struct allocated along with v4l2_async_subdev, keep the
> information there and only there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v2.2:
> 
> - Introduce a macro v4l2_subdev_to_bus_cfg() in order to remove
>   non-trivial usage in places where conversion from sub-device to ISP bus
>   configuration is needed.
> 
>  drivers/media/platform/omap3isp/isp.c       | 29 ++++++--------------------
>  drivers/media/platform/omap3isp/isp.h       |  4 +++-
>  drivers/media/platform/omap3isp/ispccdc.c   | 12 ++++++------
>  drivers/media/platform/omap3isp/ispccp2.c   |  3 ++-
>  drivers/media/platform/omap3isp/ispcsi2.c   |  2 +-
>  drivers/media/platform/omap3isp/ispcsiphy.c |  8 +++-----
>  6 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 6cb1f0495804..c3014c82d64d
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
>  	return -EINVAL;
>  }
> 
> -static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> -{
> -	struct isp_async_subdev *isd =
> -		container_of(asd, struct isp_async_subdev, asd);
> -
> -	isd->sd = subdev;
> -	isd->sd->host_priv = &isd->bus;
> -
> -	return 0;
> -}
> -
>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  {
>  	struct isp_device *isp = container_of(async, struct isp_device,
>  					      notifier);
>  	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
>  	struct v4l2_subdev *sd;
> -	struct isp_bus_cfg *bus;
>  	int ret;
> 
>  	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> @@ -2215,13 +2201,13 @@ static int isp_subdev_notifier_complete(struct
> v4l2_async_notifier *async) return ret;
> 
>  	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> -		/* Only try to link entities whose interface was set on bound 
*/
> -		if (sd->host_priv) {
> -			bus = (struct isp_bus_cfg *)sd->host_priv;
> -			ret = isp_link_entity(isp, &sd->entity, bus-
>interface);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		if (!sd->asd)
> +			continue;
> +
> +		ret = isp_link_entity(isp, &sd->entity,
> +				      v4l2_subdev_to_bus_cfg(sd)->interface);
> +		if (ret < 0)
> +			return ret;
>  	}
> 
>  	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> @@ -2399,7 +2385,6 @@ static int isp_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_register_entities;
> 
> -	isp->notifier.bound = isp_subdev_notifier_bound;
>  	isp->notifier.complete = isp_subdev_notifier_complete;
> 
>  	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 2f2ae609c548..e528df6efc09
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -226,11 +226,13 @@ struct isp_device {
>  };
> 
>  struct isp_async_subdev {
> -	struct v4l2_subdev *sd;
>  	struct isp_bus_cfg bus;
>  	struct v4l2_async_subdev asd;
>  };
> 
> +#define v4l2_subdev_to_bus_cfg(sd) \
> +	(&container_of((sd)->asd, struct isp_async_subdev, asd)->bus)
> +
>  #define v4l2_dev_to_isp_device(dev) \
>  	container_of(dev, struct isp_device, v4l2_dev)
> 
> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> b/drivers/media/platform/omap3isp/ispccdc.c index
> 4947876cfadf..80fed9526945 100644
> --- a/drivers/media/platform/omap3isp/ispccdc.c
> +++ b/drivers/media/platform/omap3isp/ispccdc.c
> @@ -1139,7 +1139,8 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
> +		parcfg = &v4l2_subdev_to_bus_cfg(
> +			to_isp_pipeline(&ccdc->subdev.entity)->external)
>  			->bus.parallel;
>  		ccdc->bt656 = parcfg->bt656;
>  	}
> @@ -2412,11 +2413,10 @@ static int ccdc_link_validate(struct v4l2_subdev
> *sd,
> 
>  	/* We've got a parallel sensor here. */
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		struct isp_parallel_cfg *parcfg =
> -			&((struct isp_bus_cfg *)
> -			  media_entity_to_v4l2_subdev(link->source->entity)
> -			  ->host_priv)->bus.parallel;
> -		parallel_shift = parcfg->data_lane_shift;
> +		parallel_shift =
> +			v4l2_subdev_to_bus_cfg(
> +				to_isp_pipeline(&ccdc->subdev.entity)-
>external)
> +			->bus.parallel.data_lane_shift;

This oopses because pipe->external is NULL :-( pipe->external is set in 
isp_video_check_external_subdevs(), which is called after 
media_pipeline_start().

>  	} else {
>  		parallel_shift = 0;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 3db8df09cd9a..e062939d0d05 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> *ccp2, */
>  static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
>  {
> +	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
>  	const struct isp_bus_cfg *buscfg;
>  	struct v4l2_mbus_framefmt *format;
>  	struct media_pad *pad;
> @@ -361,7 +362,7 @@ static int ccp2_if_configure(struct isp_ccp2_device
> *ccp2)
> 
>  	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
>  	if (ret < 0)
> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> b/drivers/media/platform/omap3isp/ispcsi2.c index
> 3ec37fed710b..a4d3d030e81e 100644
> --- a/drivers/media/platform/omap3isp/ispcsi2.c
> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> @@ -566,7 +566,7 @@ static int csi2_configure(struct isp_csi2_device *csi2)
> 
>  	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  	csi2->frame_skip = 0;
>  	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> ed1eb9907ae0..a28fb79abaac 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -165,10 +165,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> power) static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  {
>  	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -	struct isp_async_subdev *isd =
> -		container_of(pipe->external->asd, struct isp_async_subdev, 
asd);
> -	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
> -		pipe->external->host_priv : &isd->bus;
> +	struct isp_bus_cfg *buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
>  	struct isp_csiphy_lanes_cfg *lanes;
>  	int csi2_ddrclk_khz;
>  	unsigned int num_data_lanes, used_lanes = 0;
> @@ -311,7 +308,8 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
>  	mutex_lock(&phy->mutex);
>  	if (phy->entity) {
>  		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> +		struct isp_bus_cfg *buscfg =
> +			v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  		csiphy_routing_cfg(phy, buscfg->interface, false,
>  				   buscfg->bus.ccp2.phy_layer);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH v2.4 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 19:32           ` Laurent Pinchart
@ 2017-08-16 20:39             ` Sakari Ailus
  2017-08-17 13:27               ` [PATCH v2.5 " Sakari Ailus
  0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-16 20:39 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

struct v4l2_subdev.host_priv is intended to be used by another driver. This
is hardly good design but back in the days of platform data was a quick
hack to get things done.

As the sub-device specific bus information can be stored to the ISP driver
specific struct allocated along with v4l2_async_subdev, keep the
information there and only there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v2.3:

- Fix obtaining the external entity in ispccdc.c.

 drivers/media/platform/omap3isp/isp.c       | 29 +++++++----------------------
 drivers/media/platform/omap3isp/isp.h       |  4 +++-
 drivers/media/platform/omap3isp/ispccdc.c   | 12 ++++++------
 drivers/media/platform/omap3isp/ispccp2.c   |  3 ++-
 drivers/media/platform/omap3isp/ispcsi2.c   |  2 +-
 drivers/media/platform/omap3isp/ispcsiphy.c |  8 +++-----
 6 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6cb1f0495804..c3014c82d64d 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
 	return -EINVAL;
 }
 
-static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct isp_async_subdev *isd =
-		container_of(asd, struct isp_async_subdev, asd);
-
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
 	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
 	int ret;
 
 	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
@@ -2215,13 +2201,13 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		return ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
+		if (!sd->asd)
+			continue;
+
+		ret = isp_link_entity(isp, &sd->entity,
+				      v4l2_subdev_to_bus_cfg(sd)->interface);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
@@ -2399,7 +2385,6 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_register_entities;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
 	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 2f2ae609c548..e528df6efc09 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -226,11 +226,13 @@ struct isp_device {
 };
 
 struct isp_async_subdev {
-	struct v4l2_subdev *sd;
 	struct isp_bus_cfg bus;
 	struct v4l2_async_subdev asd;
 };
 
+#define v4l2_subdev_to_bus_cfg(sd) \
+	(&container_of((sd)->asd, struct isp_async_subdev, asd)->bus)
+
 #define v4l2_dev_to_isp_device(dev) \
 	container_of(dev, struct isp_device, v4l2_dev)
 
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 4947876cfadf..1c12a8420801 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1139,7 +1139,8 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
+		parcfg = &v4l2_subdev_to_bus_cfg(
+			to_isp_pipeline(&ccdc->subdev.entity)->external)
 			->bus.parallel;
 		ccdc->bt656 = parcfg->bt656;
 	}
@@ -2412,11 +2413,10 @@ static int ccdc_link_validate(struct v4l2_subdev *sd,
 
 	/* We've got a parallel sensor here. */
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		struct isp_parallel_cfg *parcfg =
-			&((struct isp_bus_cfg *)
-			  media_entity_to_v4l2_subdev(link->source->entity)
-			  ->host_priv)->bus.parallel;
-		parallel_shift = parcfg->data_lane_shift;
+		parallel_shift =
+			v4l2_subdev_to_bus_cfg(media_entity_to_v4l2_subdev(
+						       link->source->entity))
+			->bus.parallel.data_lane_shift;
 	} else {
 		parallel_shift = 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 3db8df09cd9a..e062939d0d05 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
  */
 static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 {
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	const struct isp_bus_cfg *buscfg;
 	struct v4l2_mbus_framefmt *format;
 	struct media_pad *pad;
@@ -361,7 +362,7 @@ static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 
 	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
 	if (ret < 0)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 3ec37fed710b..a4d3d030e81e 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -566,7 +566,7 @@ static int csi2_configure(struct isp_csi2_device *csi2)
 
 	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	csi2->frame_skip = 0;
 	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..a28fb79abaac 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -165,10 +165,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -311,7 +308,8 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
 		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg =
+			v4l2_subdev_to_bus_cfg(pipe->external);
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v2.5 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-16 20:39             ` [PATCH v2.4 " Sakari Ailus
@ 2017-08-17 13:27               ` Sakari Ailus
  2017-08-17 18:29                 ` Laurent Pinchart
  0 siblings, 1 reply; 16+ messages in thread
From: Sakari Ailus @ 2017-08-17 13:27 UTC (permalink / raw)
  To: linux-media; +Cc: pavel, laurent.pinchart

struct v4l2_subdev.host_priv is intended to be used by another driver. This
is hardly good design but back in the days of platform data was a quick
hack to get things done.

As the sub-device specific bus information can be stored to the ISP driver
specific struct allocated along with v4l2_async_subdev, keep the
information there and only there.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v2.4:

- Use temporary variables to avoid somewhat lengthy statements.

 drivers/media/platform/omap3isp/isp.c       | 29 +++++++----------------------
 drivers/media/platform/omap3isp/isp.h       |  4 +++-
 drivers/media/platform/omap3isp/ispccdc.c   | 16 +++++++++-------
 drivers/media/platform/omap3isp/ispccp2.c   |  3 ++-
 drivers/media/platform/omap3isp/ispcsi2.c   |  2 +-
 drivers/media/platform/omap3isp/ispcsiphy.c |  8 +++-----
 6 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 6cb1f0495804..c3014c82d64d 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
 	return -EINVAL;
 }
 
-static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
-				     struct v4l2_subdev *subdev,
-				     struct v4l2_async_subdev *asd)
-{
-	struct isp_async_subdev *isd =
-		container_of(asd, struct isp_async_subdev, asd);
-
-	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
-
-	return 0;
-}
-
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
 	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg *bus;
 	int ret;
 
 	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
@@ -2215,13 +2201,13 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 		return ret;
 
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
-		/* Only try to link entities whose interface was set on bound */
-		if (sd->host_priv) {
-			bus = (struct isp_bus_cfg *)sd->host_priv;
-			ret = isp_link_entity(isp, &sd->entity, bus->interface);
-			if (ret < 0)
-				return ret;
-		}
+		if (!sd->asd)
+			continue;
+
+		ret = isp_link_entity(isp, &sd->entity,
+				      v4l2_subdev_to_bus_cfg(sd)->interface);
+		if (ret < 0)
+			return ret;
 	}
 
 	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
@@ -2399,7 +2385,6 @@ static int isp_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_register_entities;
 
-	isp->notifier.bound = isp_subdev_notifier_bound;
 	isp->notifier.complete = isp_subdev_notifier_complete;
 
 	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 2f2ae609c548..e528df6efc09 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -226,11 +226,13 @@ struct isp_device {
 };
 
 struct isp_async_subdev {
-	struct v4l2_subdev *sd;
 	struct isp_bus_cfg bus;
 	struct v4l2_async_subdev asd;
 };
 
+#define v4l2_subdev_to_bus_cfg(sd) \
+	(&container_of((sd)->asd, struct isp_async_subdev, asd)->bus)
+
 #define v4l2_dev_to_isp_device(dev) \
 	container_of(dev, struct isp_device, v4l2_dev)
 
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index 4947876cfadf..b66276ab5765 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1139,8 +1139,10 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
 	pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
-			->bus.parallel;
+		struct v4l2_subdev *sd =
+			to_isp_pipeline(&ccdc->subdev.entity)->external;
+
+		parcfg = &v4l2_subdev_to_bus_cfg(sd)->bus.parallel;
 		ccdc->bt656 = parcfg->bt656;
 	}
 
@@ -2412,11 +2414,11 @@ static int ccdc_link_validate(struct v4l2_subdev *sd,
 
 	/* We've got a parallel sensor here. */
 	if (ccdc->input == CCDC_INPUT_PARALLEL) {
-		struct isp_parallel_cfg *parcfg =
-			&((struct isp_bus_cfg *)
-			  media_entity_to_v4l2_subdev(link->source->entity)
-			  ->host_priv)->bus.parallel;
-		parallel_shift = parcfg->data_lane_shift;
+		struct v4l2_subdev *sd =
+			media_entity_to_v4l2_subdev(link->source->entity);
+		struct isp_bus_cfg *bus_cfg = v4l2_subdev_to_bus_cfg(sd);
+
+		parallel_shift = bus_cfg->bus.parallel.data_lane_shift;
 	} else {
 		parallel_shift = 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispccp2.c b/drivers/media/platform/omap3isp/ispccp2.c
index 3db8df09cd9a..e062939d0d05 100644
--- a/drivers/media/platform/omap3isp/ispccp2.c
+++ b/drivers/media/platform/omap3isp/ispccp2.c
@@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device *ccp2,
  */
 static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 {
+	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
 	const struct isp_bus_cfg *buscfg;
 	struct v4l2_mbus_framefmt *format;
 	struct media_pad *pad;
@@ -361,7 +362,7 @@ static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
 
 	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
 	if (ret < 0)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c b/drivers/media/platform/omap3isp/ispcsi2.c
index 3ec37fed710b..a4d3d030e81e 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -566,7 +566,7 @@ static int csi2_configure(struct isp_csi2_device *csi2)
 
 	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
 	sensor = media_entity_to_v4l2_subdev(pad->entity);
-	buscfg = sensor->host_priv;
+	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 
 	csi2->frame_skip = 0;
 	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index ed1eb9907ae0..a28fb79abaac 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -165,10 +165,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32 power)
 static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 {
 	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-	struct isp_async_subdev *isd =
-		container_of(pipe->external->asd, struct isp_async_subdev, asd);
-	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
-		pipe->external->host_priv : &isd->bus;
+	struct isp_bus_cfg *buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
 	struct isp_csiphy_lanes_cfg *lanes;
 	int csi2_ddrclk_khz;
 	unsigned int num_data_lanes, used_lanes = 0;
@@ -311,7 +308,8 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
 	mutex_lock(&phy->mutex);
 	if (phy->entity) {
 		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
-		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
+		struct isp_bus_cfg *buscfg =
+			v4l2_subdev_to_bus_cfg(pipe->external);
 
 		csiphy_routing_cfg(phy, buscfg->interface, false,
 				   buscfg->bus.ccp2.phy_layer);
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v2.5 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field
  2017-08-17 13:27               ` [PATCH v2.5 " Sakari Ailus
@ 2017-08-17 18:29                 ` Laurent Pinchart
  0 siblings, 0 replies; 16+ messages in thread
From: Laurent Pinchart @ 2017-08-17 18:29 UTC (permalink / raw)
  To: Sakari Ailus; +Cc: linux-media, pavel

Hi Sakari,

Thank you for the patch.

On Thursday 17 Aug 2017 16:27:38 Sakari Ailus wrote:
> struct v4l2_subdev.host_priv is intended to be used by another driver. This
> is hardly good design but back in the days of platform data was a quick
> hack to get things done.
> 
> As the sub-device specific bus information can be stored to the ISP driver
> specific struct allocated along with v4l2_async_subdev, keep the
> information there and only there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
> since v2.4:
> 
> - Use temporary variables to avoid somewhat lengthy statements.
> 
>  drivers/media/platform/omap3isp/isp.c       | 29 ++++++--------------------
>  drivers/media/platform/omap3isp/isp.h       |  4 +++-
>  drivers/media/platform/omap3isp/ispccdc.c   | 16 +++++++++-------
>  drivers/media/platform/omap3isp/ispccp2.c   |  3 ++-
>  drivers/media/platform/omap3isp/ispcsi2.c   |  2 +-
>  drivers/media/platform/omap3isp/ispcsiphy.c |  8 +++-----
>  6 files changed, 25 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 6cb1f0495804..c3014c82d64d
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2188,26 +2188,12 @@ static int isp_fwnodes_parse(struct device *dev,
>  	return -EINVAL;
>  }
> 
> -static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> -				     struct v4l2_subdev *subdev,
> -				     struct v4l2_async_subdev *asd)
> -{
> -	struct isp_async_subdev *isd =
> -		container_of(asd, struct isp_async_subdev, asd);
> -
> -	isd->sd = subdev;
> -	isd->sd->host_priv = &isd->bus;
> -
> -	return 0;
> -}
> -
>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  {
>  	struct isp_device *isp = container_of(async, struct isp_device,
>  					      notifier);
>  	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
>  	struct v4l2_subdev *sd;
> -	struct isp_bus_cfg *bus;
>  	int ret;
> 
>  	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> @@ -2215,13 +2201,13 @@ static int isp_subdev_notifier_complete(struct
> v4l2_async_notifier *async) return ret;
> 
>  	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> -		/* Only try to link entities whose interface was set on bound 
*/
> -		if (sd->host_priv) {
> -			bus = (struct isp_bus_cfg *)sd->host_priv;
> -			ret = isp_link_entity(isp, &sd->entity, bus-
>interface);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		if (!sd->asd)
> +			continue;
> +
> +		ret = isp_link_entity(isp, &sd->entity,
> +				      v4l2_subdev_to_bus_cfg(sd)->interface);
> +		if (ret < 0)
> +			return ret;
>  	}
> 
>  	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> @@ -2399,7 +2385,6 @@ static int isp_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_register_entities;
> 
> -	isp->notifier.bound = isp_subdev_notifier_bound;
>  	isp->notifier.complete = isp_subdev_notifier_complete;
> 
>  	ret = v4l2_async_notifier_register(&isp->v4l2_dev, &isp->notifier);
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index 2f2ae609c548..e528df6efc09
> 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -226,11 +226,13 @@ struct isp_device {
>  };
> 
>  struct isp_async_subdev {
> -	struct v4l2_subdev *sd;
>  	struct isp_bus_cfg bus;
>  	struct v4l2_async_subdev asd;
>  };
> 
> +#define v4l2_subdev_to_bus_cfg(sd) \
> +	(&container_of((sd)->asd, struct isp_async_subdev, asd)->bus)
> +
>  #define v4l2_dev_to_isp_device(dev) \
>  	container_of(dev, struct isp_device, v4l2_dev)
> 
> diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> b/drivers/media/platform/omap3isp/ispccdc.c index
> 4947876cfadf..b66276ab5765 100644
> --- a/drivers/media/platform/omap3isp/ispccdc.c
> +++ b/drivers/media/platform/omap3isp/ispccdc.c
> @@ -1139,8 +1139,10 @@ static void ccdc_configure(struct isp_ccdc_device
> *ccdc) pad = media_entity_remote_pad(&ccdc->pads[CCDC_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		parcfg = &((struct isp_bus_cfg *)sensor->host_priv)
> -			->bus.parallel;
> +		struct v4l2_subdev *sd =
> +			to_isp_pipeline(&ccdc->subdev.entity)->external;
> +
> +		parcfg = &v4l2_subdev_to_bus_cfg(sd)->bus.parallel;
>  		ccdc->bt656 = parcfg->bt656;
>  	}
> 
> @@ -2412,11 +2414,11 @@ static int ccdc_link_validate(struct v4l2_subdev
> *sd,
> 
>  	/* We've got a parallel sensor here. */
>  	if (ccdc->input == CCDC_INPUT_PARALLEL) {
> -		struct isp_parallel_cfg *parcfg =
> -			&((struct isp_bus_cfg *)
> -			  media_entity_to_v4l2_subdev(link->source->entity)
> -			  ->host_priv)->bus.parallel;
> -		parallel_shift = parcfg->data_lane_shift;
> +		struct v4l2_subdev *sd =
> +			media_entity_to_v4l2_subdev(link->source->entity);
> +		struct isp_bus_cfg *bus_cfg = v4l2_subdev_to_bus_cfg(sd);
> +
> +		parallel_shift = bus_cfg->bus.parallel.data_lane_shift;
>  	} else {
>  		parallel_shift = 0;
>  	}
> diff --git a/drivers/media/platform/omap3isp/ispccp2.c
> b/drivers/media/platform/omap3isp/ispccp2.c index
> 3db8df09cd9a..e062939d0d05 100644
> --- a/drivers/media/platform/omap3isp/ispccp2.c
> +++ b/drivers/media/platform/omap3isp/ispccp2.c
> @@ -350,6 +350,7 @@ static void ccp2_lcx_config(struct isp_ccp2_device
> *ccp2, */
>  static int ccp2_if_configure(struct isp_ccp2_device *ccp2)
>  {
> +	struct isp_pipeline *pipe = to_isp_pipeline(&ccp2->subdev.entity);
>  	const struct isp_bus_cfg *buscfg;
>  	struct v4l2_mbus_framefmt *format;
>  	struct media_pad *pad;
> @@ -361,7 +362,7 @@ static int ccp2_if_configure(struct isp_ccp2_device
> *ccp2)
> 
>  	pad = media_entity_remote_pad(&ccp2->pads[CCP2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  	ret = ccp2_phyif_config(ccp2, &buscfg->bus.ccp2);
>  	if (ret < 0)
> diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
> b/drivers/media/platform/omap3isp/ispcsi2.c index
> 3ec37fed710b..a4d3d030e81e 100644
> --- a/drivers/media/platform/omap3isp/ispcsi2.c
> +++ b/drivers/media/platform/omap3isp/ispcsi2.c
> @@ -566,7 +566,7 @@ static int csi2_configure(struct isp_csi2_device *csi2)
> 
>  	pad = media_entity_remote_pad(&csi2->pads[CSI2_PAD_SINK]);
>  	sensor = media_entity_to_v4l2_subdev(pad->entity);
> -	buscfg = sensor->host_priv;
> +	buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  	csi2->frame_skip = 0;
>  	v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index
> ed1eb9907ae0..a28fb79abaac 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -165,10 +165,7 @@ static int csiphy_set_power(struct isp_csiphy *phy, u32
> power) static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  {
>  	struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -	struct isp_async_subdev *isd =
> -		container_of(pipe->external->asd, struct isp_async_subdev, 
asd);
> -	struct isp_bus_cfg *buscfg = pipe->external->host_priv ?
> -		pipe->external->host_priv : &isd->bus;
> +	struct isp_bus_cfg *buscfg = v4l2_subdev_to_bus_cfg(pipe->external);
>  	struct isp_csiphy_lanes_cfg *lanes;
>  	int csi2_ddrclk_khz;
>  	unsigned int num_data_lanes, used_lanes = 0;
> @@ -311,7 +308,8 @@ void omap3isp_csiphy_release(struct isp_csiphy *phy)
>  	mutex_lock(&phy->mutex);
>  	if (phy->entity) {
>  		struct isp_pipeline *pipe = to_isp_pipeline(phy->entity);
> -		struct isp_bus_cfg *buscfg = pipe->external->host_priv;
> +		struct isp_bus_cfg *buscfg =
> +			v4l2_subdev_to_bus_cfg(pipe->external);
> 
>  		csiphy_routing_cfg(phy, buscfg->interface, false,
>  				   buscfg->bus.ccp2.phy_layer);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2017-08-17 18:28 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-16 12:51 [PATCH v2 0/5] Omap3isp CCP2 support Sakari Ailus
2017-08-16 12:51 ` [PATCH v2 1/5] omap3isp: Parse CSI1 configuration from the device tree Sakari Ailus
2017-08-16 12:51 ` [PATCH v2 2/5] omap3isp: Correctly set IO_OUT_SEL and VP_CLK_POL for CCP2 mode Sakari Ailus
2017-08-16 12:51 ` [PATCH v2 3/5] omap3isp: Always initialise isp and mutex for csiphy1 Sakari Ailus
2017-08-16 12:51 ` [PATCH v2 4/5] omap3isp: csiphy: Don't assume the CSI receiver is a CSI2 module Sakari Ailus
2017-08-16 12:54   ` Laurent Pinchart
2017-08-16 12:51 ` [PATCH v2 5/5] omap3isp: Correctly configure routing in PHY release Sakari Ailus
2017-08-16 16:52   ` [PATCH v2.1 5/5] omap3isp: Quit using struct v4l2_subdev.host_priv field Sakari Ailus
2017-08-16 17:05     ` [PATCH v2.2 " Sakari Ailus
2017-08-16 17:24       ` Laurent Pinchart
2017-08-16 17:26         ` Sakari Ailus
2017-08-16 17:39         ` [PATCH v2.3 " Sakari Ailus
2017-08-16 19:32           ` Laurent Pinchart
2017-08-16 20:39             ` [PATCH v2.4 " Sakari Ailus
2017-08-17 13:27               ` [PATCH v2.5 " Sakari Ailus
2017-08-17 18:29                 ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).