[PATCH v2 00/15] media: ipu-bridge: Shared with atomisp, rework VCM instantiation

[PATCH v2 00/15] media: ipu-bridge: Shared with atomisp, rework VCM instantiation

[Hans de Goede]

Hi All,

Here is v2 of my patch-series to make the atomisp code share the
ACPI -> sensor fwnode bridge code with the IPU3 (and IPU6 code).

The last 2 patches also rework VCM instantiation, which was my
initial reason for unifying / sharing the code.

Changes in v2:
- Rebase on top of f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge
  to ipu bridge and move out of ipu3")
  (rebase on top of sailus/media_tree.git/for-6.6-1.4-signed)
- Share the ipu_supported_sensors[] array between atomisp and IPU3/IPU6
  (leave it in ipu-bridge.c instead of giving each consumer its own copy)
- 2 new bugfixes:
  media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings  
  media: ipu-bridge: Allow building as module

Original cover-letter:

While working on adding (proper) VCM support to the atomisp code
I found myself copying yet more code from
drivers/media/pci/intel/ipu3/cio2-bridge.c into the atomisp code.

So I decided that it really was time to factor out the common code
(most of the code) from intel/ipu3/cio2-bridge.c into its own
helper library and then share it between the atomisp and IPU3 code.

This will hopefully also be useful for the ongoing work to upstream
IPU6 input system support which also needs this functionality and
currently contains a 3th copy of this code in the out of tree driver.

This set consists of the following parts:

Patches 1-4  Bugfixes for recent changes
Patches 5-12 Cleanup / preparation patches
Patch 13     Drop ipu-bridge code copy from atomisp, switching to
             the shared ipu-bridge module
Patch 14     Rework how VCM client instantiation is done so that
             a device-link can be added from VCM to sensor to
             fix issues with the VCM power-state being tied to
             the sensor power state
Patch 15     Example patch to show how patch 11 avoids the need
             for hacks in VCM drivers caused by the shared power state
             (not intended for merging)

Regards,

Hans


Hans de Goede (15):
  media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings
  media: ipu-bridge: Do not use on stack memory for software_node.name
    field
  media: ipu-bridge: Move initialization of node_names.vcm to
    ipu_bridge_init_swnode_names()
  media: ipu-bridge: Allow building as module
  media: ipu-bridge: Make ipu_bridge_init() take a regular struct device
    as argument
  media: ipu-bridge: Store dev pointer in struct ipu_bridge
  media: ipu-bridge: Only keep PLD around while parsing
  media: ipu-bridge: Add a ipu_bridge_parse_ssdb() helper function
  media: ipu-bridge: Drop early setting of sensor->adev
  media: ipu-bridge: Add a parse_sensor_fwnode callback to
    ipu_bridge_init()
  media: ipu-bridge: Move ipu-bridge.h to include/media/
  media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]
  media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()
  media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor
  [RFC] media: dw9719: Drop hack to enable "vsio" regulator

 drivers/media/i2c/dw9719.c                    |  27 +-
 drivers/media/pci/intel/Kconfig               |  19 +-
 drivers/media/pci/intel/ipu-bridge.c          | 334 +++++++++++-------
 drivers/media/pci/intel/ipu3/Kconfig          |  20 ++
 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  10 +-
 drivers/staging/media/atomisp/Kconfig         |   2 +
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ----
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 328 +++--------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 .../pci/intel => include/media}/ipu-bridge.h  |  26 +-
 10 files changed, 303 insertions(+), 531 deletions(-)
 rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (80%)

-- 
2.41.0

[PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Hans de Goede]

When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
sensor->adev is not set yet.

So if either of the dev_warn() calls about unknown values are hit this
will lead to a NULL pointer deref.

Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
on errors harder, to fix this.

Fixes: 485aa3df0dff ("media: ipu3-cio2: Parse sensor orientation and rotation")
Cc: Fabian Wüthrich <me@fabwu.ch>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 62daa8c1f6b1..f0927f80184d 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -308,6 +308,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 		}
 
 		sensor = &bridge->sensors[bridge->n_sensors];
+		/*
+		 * Borrow our adev ref to the sensor for now, on success
+		 * acpi_dev_get(adev) is done further below.
+		 */
+		sensor->adev = adev;
 
 		ret = ipu_bridge_read_acpi_buffer(adev, "SSDB",
 						  &sensor->ssdb,
-- 
2.41.0

[PATCH v2 02/15] media: ipu-bridge: Do not use on stack memory for software_node.name field

[Hans de Goede]

Commit 567f97bd381f ("media: ipu3-cio2: support multiple sensors and VCMs
with same HID") introduced an on stack vcm_name and then uses this for
the name field of the software_node struct used for the vcm.

But the software_node struct is much longer lived then the current
stack-frame, so this is no good.

Instead extend the ipu_node_names struct with an extra field to store
the vcm software_node name and use that.

Note this also changes the length of the allocated buffer from
ACPI_ID_LEN + 4 to 16. the name is filled with "<ipu_vcm_types[x]>-%u"
where ipu_vcm_types[x] is not an ACPI_ID. The maximum length of
the strings in the ipu_vcm_types[] array is 11 + 5 bytes for "-255\0"
means 16 bytes are needed in the worst case scenario.

Fixes: 567f97bd381f ("media: ipu3-cio2: support multiple sensors and VCMs with same HID")
Cc: Bingbu Cao <bingbu.cao@intel.com>
Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 7 +++----
 drivers/media/pci/intel/ipu-bridge.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index f0927f80184d..ef6c6cb7b51b 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -220,7 +220,6 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 						 struct ipu_sensor *sensor)
 {
 	struct software_node *nodes = sensor->swnodes;
-	char vcm_name[ACPI_ID_LEN + 4];
 
 	ipu_bridge_init_swnode_names(sensor);
 
@@ -240,10 +239,10 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 						sensor->ipu_properties);
 	if (sensor->ssdb.vcmtype) {
 		/* append ssdb.link to distinguish VCM nodes with same HID */
-		snprintf(vcm_name, sizeof(vcm_name), "%s-%u",
-			 ipu_vcm_types[sensor->ssdb.vcmtype - 1],
+		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
+			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
 			 sensor->ssdb.link);
-		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
+		nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
 	}
 
 	ipu_bridge_init_swnode_group(sensor);
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 8cb733c03e2f..6cce712a0f34 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -103,6 +103,7 @@ struct ipu_node_names {
 	char port[7];
 	char endpoint[11];
 	char remote_port[7];
+	char vcm[16];
 };
 
 struct ipu_sensor_config {
-- 
2.41.0

[PATCH v2 03/15] media: ipu-bridge: Move initialization of node_names.vcm to ipu_bridge_init_swnode_names()

[Hans de Goede]

Move initialization of node_names.vcm to ipu_bridge_init_swnode_names()
where it belongs.

And make the initialization of nodes[SWNODE_VCM] unconditional,
ipu_bridge_init_swnode_group() takes care of not registering it
when there is no VCM.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index ef6c6cb7b51b..1c88fd925a8b 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -201,6 +201,12 @@ static void ipu_bridge_init_swnode_names(struct ipu_sensor *sensor)
 	snprintf(sensor->node_names.endpoint,
 		 sizeof(sensor->node_names.endpoint),
 		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
+	if (sensor->ssdb.vcmtype) {
+		/* append ssdb.link to distinguish nodes with same model VCM */
+		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
+			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
+			 sensor->ssdb.link);
+	}
 }
 
 static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
@@ -237,13 +243,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 						sensor->node_names.endpoint,
 						&nodes[SWNODE_IPU_PORT],
 						sensor->ipu_properties);
-	if (sensor->ssdb.vcmtype) {
-		/* append ssdb.link to distinguish VCM nodes with same HID */
-		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
-			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
-			 sensor->ssdb.link);
-		nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
-	}
+	nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
 
 	ipu_bridge_init_swnode_group(sensor);
 }
-- 
2.41.0

[PATCH v2 04/15] media: ipu-bridge: Allow building as module

[Hans de Goede]

After commit f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu
bridge and move out of ipu3") the ipu-bridge code is always builtin
even if all (its only) consumers are build as module.

Fix this by turning "config IPU_BRIDGE" into a pure library Kconfig
option (not user selectable, must be selected by consumers) and
re-introducing the CIO2_BRIDGE Kconfig bits in .../pci/intel/ipu3/Kconfig
which were dropped to still allow building ipu3-cio2 without ipu-bridge
support.

Fixes: f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu bridge and move out of ipu3")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/Kconfig      | 19 ++-----------------
 drivers/media/pci/intel/ipu-bridge.c |  4 ++++
 drivers/media/pci/intel/ipu3/Kconfig | 20 ++++++++++++++++++++
 3 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/media/pci/intel/Kconfig b/drivers/media/pci/intel/Kconfig
index 64a29b0b7033..d4d723591475 100644
--- a/drivers/media/pci/intel/Kconfig
+++ b/drivers/media/pci/intel/Kconfig
@@ -1,21 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config IPU_BRIDGE
-	bool "Intel IPU Sensors Bridge"
-	depends on VIDEO_IPU3_CIO2 && ACPI
-	depends on I2C
-	help
-	  This extension provides an API for the Intel IPU driver to create
-	  connections to cameras that are hidden in the SSDB buffer in ACPI.
-	  It can be used to enable support for cameras in detachable / hybrid
-	  devices that ship with Windows.
-
-	  Say Y here if your device is a detachable / hybrid laptop that comes
-	  with Windows installed by the OEM, for example:
-
-		- Microsoft Surface models (except Surface Pro 3)
-		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
-		- Dell 7285
-
-	  If in doubt, say N here.
+	tristate
+	depends on ACPI && I2C
 
 source "drivers/media/pci/intel/ipu3/Kconfig"
diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 1c88fd925a8b..97b544736af2 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -497,3 +497,7 @@ int ipu_bridge_init(struct pci_dev *ipu)
 	return ret;
 }
 EXPORT_SYMBOL_NS_GPL(ipu_bridge_init, INTEL_IPU_BRIDGE);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Dan Scally <djrscally@gmail.com>");
+MODULE_DESCRIPTION("Intel IPU ACPI Sensors Bridge");
diff --git a/drivers/media/pci/intel/ipu3/Kconfig b/drivers/media/pci/intel/ipu3/Kconfig
index 9be06ee81ff0..0951545eab21 100644
--- a/drivers/media/pci/intel/ipu3/Kconfig
+++ b/drivers/media/pci/intel/ipu3/Kconfig
@@ -8,6 +8,7 @@ config VIDEO_IPU3_CIO2
 	select VIDEO_V4L2_SUBDEV_API
 	select V4L2_FWNODE
 	select VIDEOBUF2_DMA_SG
+	select IPU_BRIDGE if CIO2_BRIDGE
 
 	help
 	  This is the Intel IPU3 CIO2 CSI-2 receiver unit, found in Intel
@@ -17,3 +18,22 @@ config VIDEO_IPU3_CIO2
 	  Say Y or M here if you have a Skylake/Kaby Lake SoC with MIPI CSI-2
 	  connected camera.
 	  The module will be called ipu3-cio2.
+
+config CIO2_BRIDGE
+	bool "IPU3 CIO2 Sensors Bridge"
+	depends on VIDEO_IPU3_CIO2 && ACPI
+	depends on I2C
+	help
+	  This extension provides an API for the ipu3-cio2 driver to create
+	  connections to cameras that are hidden in the SSDB buffer in ACPI.
+	  It can be used to enable support for cameras in detachable / hybrid
+	  devices that ship with Windows.
+
+	  Say Y here if your device is a detachable / hybrid laptop that comes
+	  with Windows installed by the OEM, for example:
+
+		- Microsoft Surface models (except Surface Pro 3)
+		- The Lenovo Miix line (for example the 510, 520, 710 and 720)
+		- Dell 7285
+
+	  If in doubt, say N here.
-- 
2.41.0

[PATCH v2 05/15] media: ipu-bridge: Make ipu_bridge_init() take a regular struct device as argument

[Hans de Goede]

Make ipu_bridge_init() take a regular struct device, rather then
a pci_dev as argument.

This is a preparation patch for making the ipu-bridge code more generic
so that it can be shared with the atomisp driver.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c     | 16 +++++++---------
 drivers/media/pci/intel/ipu-bridge.h     |  4 ++--
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  2 +-
 3 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 97b544736af2..9027a8d2d176 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,7 +4,6 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
-#include <linux/pci.h>
 #include <linux/property.h>
 #include <media/v4l2-fwnode.h>
 
@@ -288,7 +287,7 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 
 static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 				     struct ipu_bridge *bridge,
-				     struct pci_dev *ipu)
+				     struct device *dev)
 {
 	struct fwnode_handle *fwnode, *primary;
 	struct ipu_sensor *sensor;
@@ -302,7 +301,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		if (bridge->n_sensors >= IPU_MAX_PORTS) {
 			acpi_dev_put(adev);
-			dev_err(&ipu->dev, "Exceeded available IPU ports\n");
+			dev_err(dev, "Exceeded available IPU ports\n");
 			return -EINVAL;
 		}
 
@@ -362,7 +361,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		ipu_bridge_instantiate_vcm_i2c_client(sensor);
 
-		dev_info(&ipu->dev, "Found supported sensor %s\n",
+		dev_info(dev, "Found supported sensor %s\n",
 			 acpi_dev_name(adev));
 
 		bridge->n_sensors++;
@@ -380,7 +379,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 }
 
 static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
-				      struct pci_dev *ipu)
+				      struct device *dev)
 {
 	unsigned int i;
 	int ret;
@@ -389,7 +388,7 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
 		const struct ipu_sensor_config *cfg =
 			&ipu_supported_sensors[i];
 
-		ret = ipu_bridge_connect_sensor(cfg, bridge, ipu);
+		ret = ipu_bridge_connect_sensor(cfg, bridge, dev);
 		if (ret)
 			goto err_unregister_sensors;
 	}
@@ -435,9 +434,8 @@ static int ipu_bridge_sensors_are_ready(void)
 	return ready;
 }
 
-int ipu_bridge_init(struct pci_dev *ipu)
+int ipu_bridge_init(struct device *dev)
 {
-	struct device *dev = &ipu->dev;
 	struct fwnode_handle *fwnode;
 	struct ipu_bridge *bridge;
 	unsigned int i;
@@ -470,7 +468,7 @@ int ipu_bridge_init(struct pci_dev *ipu)
 	for (i = 0; i < IPU_MAX_LANES; i++)
 		bridge->data_lanes[i] = i + 1;
 
-	ret = ipu_bridge_connect_sensors(bridge, ipu);
+	ret = ipu_bridge_connect_sensors(bridge, dev);
 	if (ret || bridge->n_sensors == 0)
 		goto err_unregister_ipu;
 
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 6cce712a0f34..8c1437f252d2 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -144,9 +144,9 @@ struct ipu_bridge {
 };
 
 #if IS_ENABLED(CONFIG_IPU_BRIDGE)
-int ipu_bridge_init(struct pci_dev *ipu);
+int ipu_bridge_init(struct device *dev);
 #else
-static inline int ipu_bridge_init(struct pci_dev *ipu) { return 0; }
+static inline int ipu_bridge_init(struct device *dev) { return 0; }
 #endif
 
 #endif
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index dc09fbdb062b..4068fa0a5ecf 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1725,7 +1725,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			return -EINVAL;
 		}
 
-		r = ipu_bridge_init(pci_dev);
+		r = ipu_bridge_init(dev);
 		if (r)
 			return r;
 	}
-- 
2.41.0

[PATCH v2 06/15] media: ipu-bridge: Store dev pointer in struct ipu_bridge

[Hans de Goede]

Store the dev pointer in struct ipu_bridge instead of passing it
around 3 levels deep.

This takes up slightly more memory but further patches in this series
add more data which needs to be passed around making passing
everything as arguments cumbersome and those further patches also
add data to struct ipu_bridge.

To be consistent with these upcoming patches also add
the dev pointer to struct ipu_bridge.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 15 +++++++--------
 drivers/media/pci/intel/ipu-bridge.h |  1 +
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 9027a8d2d176..8e91d9b3e0fe 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -286,8 +286,7 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 }
 
 static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
-				     struct ipu_bridge *bridge,
-				     struct device *dev)
+				     struct ipu_bridge *bridge)
 {
 	struct fwnode_handle *fwnode, *primary;
 	struct ipu_sensor *sensor;
@@ -301,7 +300,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		if (bridge->n_sensors >= IPU_MAX_PORTS) {
 			acpi_dev_put(adev);
-			dev_err(dev, "Exceeded available IPU ports\n");
+			dev_err(bridge->dev, "Exceeded available IPU ports\n");
 			return -EINVAL;
 		}
 
@@ -361,7 +360,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		ipu_bridge_instantiate_vcm_i2c_client(sensor);
 
-		dev_info(dev, "Found supported sensor %s\n",
+		dev_info(bridge->dev, "Found supported sensor %s\n",
 			 acpi_dev_name(adev));
 
 		bridge->n_sensors++;
@@ -378,8 +377,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 	return ret;
 }
 
-static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
-				      struct device *dev)
+static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 {
 	unsigned int i;
 	int ret;
@@ -388,7 +386,7 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
 		const struct ipu_sensor_config *cfg =
 			&ipu_supported_sensors[i];
 
-		ret = ipu_bridge_connect_sensor(cfg, bridge, dev);
+		ret = ipu_bridge_connect_sensor(cfg, bridge);
 		if (ret)
 			goto err_unregister_sensors;
 	}
@@ -451,6 +449,7 @@ int ipu_bridge_init(struct device *dev)
 	strscpy(bridge->ipu_node_name, IPU_HID,
 		sizeof(bridge->ipu_node_name));
 	bridge->ipu_hid_node.name = bridge->ipu_node_name;
+	bridge->dev = dev;
 
 	ret = software_node_register(&bridge->ipu_hid_node);
 	if (ret < 0) {
@@ -468,7 +467,7 @@ int ipu_bridge_init(struct device *dev)
 	for (i = 0; i < IPU_MAX_LANES; i++)
 		bridge->data_lanes[i] = i + 1;
 
-	ret = ipu_bridge_connect_sensors(bridge, dev);
+	ret = ipu_bridge_connect_sensors(bridge);
 	if (ret || bridge->n_sensors == 0)
 		goto err_unregister_ipu;
 
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 8c1437f252d2..6cb68e3344dc 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -136,6 +136,7 @@ struct ipu_sensor {
 };
 
 struct ipu_bridge {
+	struct device *dev;
 	char ipu_node_name[ACPI_ID_LEN];
 	struct software_node ipu_hid_node;
 	u32 data_lanes[4];
-- 
2.41.0

[PATCH v2 07/15] media: ipu-bridge: Only keep PLD around while parsing

[Hans de Goede]

There is no need to keep a reference to the PLD struct around,
it is only used once the get the sensor orientation.

Make ipu_bridge_parse_orientation() also get + put the PLD.

This is a preparation patch for making the ipu-bridge code more generic
so that it can be shared with the atomisp driver.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 48 ++++++++++++++++------------
 drivers/media/pci/intel/ipu-bridge.h |  1 -
 2 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 8e91d9b3e0fe..3a984d688b42 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -112,23 +112,39 @@ static u32 ipu_bridge_parse_rotation(struct ipu_sensor *sensor)
 	}
 }
 
-static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct ipu_sensor *sensor)
+static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
 {
-	switch (sensor->pld->panel) {
+	enum v4l2_fwnode_orientation orientation;
+	struct acpi_pld_info *pld;
+	acpi_status status;
+
+	status = acpi_get_physical_device_location(adev->handle, &pld);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(&adev->dev, "_PLD call failed using unknown orientation\n");
+		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
+	}
+
+	switch (pld->panel) {
 	case ACPI_PLD_PANEL_FRONT:
-		return V4L2_FWNODE_ORIENTATION_FRONT;
+		orientation = V4L2_FWNODE_ORIENTATION_FRONT;
+		break;
 	case ACPI_PLD_PANEL_BACK:
-		return V4L2_FWNODE_ORIENTATION_BACK;
+		orientation = V4L2_FWNODE_ORIENTATION_BACK;
+		break;
 	case ACPI_PLD_PANEL_TOP:
 	case ACPI_PLD_PANEL_LEFT:
 	case ACPI_PLD_PANEL_RIGHT:
 	case ACPI_PLD_PANEL_UNKNOWN:
-		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
+		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
+		break;
 	default:
-		dev_warn(&sensor->adev->dev, "Unknown _PLD panel value %d\n",
-			 sensor->pld->panel);
-		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
+		dev_warn(&adev->dev, "Unknown _PLD panel val %d\n", pld->panel);
+		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
+		break;
 	}
+
+	ACPI_FREE(pld);
+	return orientation;
 }
 
 static void ipu_bridge_create_fwnode_properties(
@@ -140,7 +156,7 @@ static void ipu_bridge_create_fwnode_properties(
 	enum v4l2_fwnode_orientation orientation;
 
 	rotation = ipu_bridge_parse_rotation(sensor);
-	orientation = ipu_bridge_parse_orientation(sensor);
+	orientation = ipu_bridge_parse_orientation(sensor->adev);
 
 	sensor->prop_names = prop_names;
 
@@ -279,7 +295,6 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 	for (i = 0; i < bridge->n_sensors; i++) {
 		sensor = &bridge->sensors[i];
 		software_node_unregister_node_group(sensor->group);
-		ACPI_FREE(sensor->pld);
 		acpi_dev_put(sensor->adev);
 		i2c_unregister_device(sensor->vcm_i2c_client);
 	}
@@ -291,7 +306,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 	struct fwnode_handle *fwnode, *primary;
 	struct ipu_sensor *sensor;
 	struct acpi_device *adev;
-	acpi_status status;
 	int ret;
 
 	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
@@ -326,17 +340,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 			sensor->ssdb.vcmtype = 0;
 		}
 
-		status = acpi_get_physical_device_location(adev->handle, &sensor->pld);
-		if (ACPI_FAILURE(status)) {
-			ret = -ENODEV;
-			goto err_put_adev;
-		}
-
 		if (sensor->ssdb.lanes > IPU_MAX_LANES) {
 			dev_err(&adev->dev,
 				"Number of lanes in SSDB is invalid\n");
 			ret = -EINVAL;
-			goto err_free_pld;
+			goto err_put_adev;
 		}
 
 		ipu_bridge_create_fwnode_properties(sensor, bridge, cfg);
@@ -344,7 +352,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		ret = software_node_register_node_group(sensor->group);
 		if (ret)
-			goto err_free_pld;
+			goto err_put_adev;
 
 		fwnode = software_node_fwnode(&sensor->swnodes[
 						      SWNODE_SENSOR_HID]);
@@ -370,8 +378,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 err_free_swnodes:
 	software_node_unregister_node_group(sensor->group);
-err_free_pld:
-	ACPI_FREE(sensor->pld);
 err_put_adev:
 	acpi_dev_put(adev);
 	return ret;
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 6cb68e3344dc..907ca833a7c1 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -124,7 +124,6 @@ struct ipu_sensor {
 	struct ipu_node_names node_names;
 
 	struct ipu_sensor_ssdb ssdb;
-	struct acpi_pld_info *pld;
 
 	struct ipu_property_names prop_names;
 	struct property_entry ep_properties[5];
-- 
2.41.0

[PATCH v2 08/15] media: ipu-bridge: Add a ipu_bridge_parse_ssdb() helper function

[Hans de Goede]

The code to go from ACPI sensor info to a fwnode-tree with connector
nodes and endpoint properties is 99% the same for the atomisp2 and
the IPU3.

The main difference is that atomisp2 devices do not have a SSDB table
with various info.

Abstract out the parsing of the sensor's ACPI fwnode into a helper
function and store the parsed results, rather then the raw SSDB
in struct ipu_sensor.

This is a preparation patch for making the ipu-bridge code more generic
so that it can be shared with the atomisp driver.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 96 +++++++++++++++-------------
 drivers/media/pci/intel/ipu-bridge.h |  8 ++-
 2 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 3a984d688b42..daa9dde78c64 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -97,17 +97,18 @@ static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
 	return ret;
 }
 
-static u32 ipu_bridge_parse_rotation(struct ipu_sensor *sensor)
+static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
+				     struct ipu_sensor_ssdb *ssdb)
 {
-	switch (sensor->ssdb.degree) {
+	switch (ssdb->degree) {
 	case IPU_SENSOR_ROTATION_NORMAL:
 		return 0;
 	case IPU_SENSOR_ROTATION_INVERTED:
 		return 180;
 	default:
-		dev_warn(&sensor->adev->dev,
+		dev_warn(&adev->dev,
 			 "Unknown rotation %d. Assume 0 degree rotation\n",
-			 sensor->ssdb.degree);
+			 ssdb->degree);
 		return 0;
 	}
 }
@@ -147,17 +148,43 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
 	return orientation;
 }
 
+static int ipu_bridge_parse_ssdb(struct acpi_device *adev,
+				 struct ipu_sensor *sensor)
+{
+	struct ipu_sensor_ssdb ssdb = {};
+	int ret;
+
+	ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", &ssdb, sizeof(ssdb));
+	if (ret)
+		return ret;
+
+	if (ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
+		dev_warn(&adev->dev, "Unknown VCM type %d\n", ssdb.vcmtype);
+		ssdb.vcmtype = 0;
+	}
+
+	if (ssdb.lanes > IPU_MAX_LANES) {
+		dev_err(&adev->dev, "Number of lanes in SSDB is invalid\n");
+		return -EINVAL;
+	}
+
+	sensor->link = ssdb.link;
+	sensor->lanes = ssdb.lanes;
+	sensor->mclkspeed = ssdb.mclkspeed;
+	sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb);
+	sensor->orientation = ipu_bridge_parse_orientation(adev);
+
+	if (ssdb.vcmtype)
+		sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];
+
+	return 0;
+}
+
 static void ipu_bridge_create_fwnode_properties(
 	struct ipu_sensor *sensor,
 	struct ipu_bridge *bridge,
 	const struct ipu_sensor_config *cfg)
 {
-	u32 rotation;
-	enum v4l2_fwnode_orientation orientation;
-
-	rotation = ipu_bridge_parse_rotation(sensor);
-	orientation = ipu_bridge_parse_orientation(sensor->adev);
-
 	sensor->prop_names = prop_names;
 
 	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_IPU_ENDPOINT]);
@@ -165,14 +192,14 @@ static void ipu_bridge_create_fwnode_properties(
 
 	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.clock_frequency,
-					sensor->ssdb.mclkspeed);
+					sensor->mclkspeed);
 	sensor->dev_properties[1] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.rotation,
-					rotation);
+					sensor->rotation);
 	sensor->dev_properties[2] = PROPERTY_ENTRY_U32(
 					sensor->prop_names.orientation,
-					orientation);
-	if (sensor->ssdb.vcmtype) {
+					sensor->orientation);
+	if (sensor->vcm_type) {
 		sensor->vcm_ref[0] =
 			SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_VCM]);
 		sensor->dev_properties[3] =
@@ -184,8 +211,7 @@ static void ipu_bridge_create_fwnode_properties(
 					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
 	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
-					bridge->data_lanes,
-					sensor->ssdb.lanes);
+					bridge->data_lanes, sensor->lanes);
 	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
 					sensor->prop_names.remote_endpoint,
 					sensor->local_ref);
@@ -198,8 +224,7 @@ static void ipu_bridge_create_fwnode_properties(
 
 	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
 					sensor->prop_names.data_lanes,
-					bridge->data_lanes,
-					sensor->ssdb.lanes);
+					bridge->data_lanes, sensor->lanes);
 	sensor->ipu_properties[1] = PROPERTY_ENTRY_REF_ARRAY(
 					sensor->prop_names.remote_endpoint,
 					sensor->remote_ref);
@@ -209,18 +234,17 @@ static void ipu_bridge_init_swnode_names(struct ipu_sensor *sensor)
 {
 	snprintf(sensor->node_names.remote_port,
 		 sizeof(sensor->node_names.remote_port),
-		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->ssdb.link);
+		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->link);
 	snprintf(sensor->node_names.port,
 		 sizeof(sensor->node_names.port),
 		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
 	snprintf(sensor->node_names.endpoint,
 		 sizeof(sensor->node_names.endpoint),
 		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
-	if (sensor->ssdb.vcmtype) {
-		/* append ssdb.link to distinguish nodes with same model VCM */
+	if (sensor->vcm_type) {
+		/* append link to distinguish nodes with same model VCM */
 		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
-			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
-			 sensor->ssdb.link);
+			 "%s-%u", sensor->vcm_type, sensor->link);
 	}
 }
 
@@ -233,7 +257,7 @@ static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
 	sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
 	sensor->group[SWNODE_IPU_PORT] = &nodes[SWNODE_IPU_PORT];
 	sensor->group[SWNODE_IPU_ENDPOINT] = &nodes[SWNODE_IPU_ENDPOINT];
-	if (sensor->ssdb.vcmtype)
+	if (sensor->vcm_type)
 		sensor->group[SWNODE_VCM] =  &nodes[SWNODE_VCM];
 }
 
@@ -268,13 +292,12 @@ static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
 	struct i2c_board_info board_info = { };
 	char name[16];
 
-	if (!sensor->ssdb.vcmtype)
+	if (!sensor->vcm_type)
 		return;
 
 	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
 	board_info.dev_name = name;
-	strscpy(board_info.type, ipu_vcm_types[sensor->ssdb.vcmtype - 1],
-		ARRAY_SIZE(board_info.type));
+	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
 	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
 
 	sensor->vcm_i2c_client =
@@ -325,27 +348,12 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 		 */
 		sensor->adev = adev;
 
-		ret = ipu_bridge_read_acpi_buffer(adev, "SSDB",
-						  &sensor->ssdb,
-						  sizeof(sensor->ssdb));
+		ret = ipu_bridge_parse_ssdb(adev, sensor);
 		if (ret)
 			goto err_put_adev;
 
 		snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
-			 cfg->hid, sensor->ssdb.link);
-
-		if (sensor->ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
-			dev_warn(&adev->dev, "Unknown VCM type %d\n",
-				 sensor->ssdb.vcmtype);
-			sensor->ssdb.vcmtype = 0;
-		}
-
-		if (sensor->ssdb.lanes > IPU_MAX_LANES) {
-			dev_err(&adev->dev,
-				"Number of lanes in SSDB is invalid\n");
-			ret = -EINVAL;
-			goto err_put_adev;
-		}
+			 cfg->hid, sensor->link);
 
 		ipu_bridge_create_fwnode_properties(sensor, bridge, cfg);
 		ipu_bridge_create_connection_swnodes(bridge, sensor);
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index 907ca833a7c1..a8b89c4b95bc 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -5,6 +5,7 @@
 
 #include <linux/property.h>
 #include <linux/types.h>
+#include <media/v4l2-fwnode.h>
 
 struct i2c_client;
 
@@ -123,7 +124,12 @@ struct ipu_sensor {
 	struct software_node swnodes[SWNODE_COUNT];
 	struct ipu_node_names node_names;
 
-	struct ipu_sensor_ssdb ssdb;
+	u8 link;
+	u8 lanes;
+	u32 mclkspeed;
+	u32 rotation;
+	enum v4l2_fwnode_orientation orientation;
+	const char *vcm_type;
 
 	struct ipu_property_names prop_names;
 	struct property_entry ep_properties[5];
-- 
2.41.0

[PATCH v2 09/15] media: ipu-bridge: Drop early setting of sensor->adev

[Hans de Goede]

sensor->adev is no longer dereferenced before it is permanently set by:

	sensor->adev = acpi_dev_get(adev);

So the early assignment with a borrowed reference can be dropped.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index daa9dde78c64..891dc0b7c960 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -342,11 +342,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 		}
 
 		sensor = &bridge->sensors[bridge->n_sensors];
-		/*
-		 * Borrow our adev ref to the sensor for now, on success
-		 * acpi_dev_get(adev) is done further below.
-		 */
-		sensor->adev = adev;
 
 		ret = ipu_bridge_parse_ssdb(adev, sensor);
 		if (ret)
-- 
2.41.0

[PATCH v2 10/15] media: ipu-bridge: Add a parse_sensor_fwnode callback to ipu_bridge_init()

[Hans de Goede]

Add a parse_sensor_fwnode callback to ipu_bridge_init(), so that
ipu_bridge_init() can be used with other sensor fwnode parse functions
then just ipu_bridge_parse_ssdb().

This will allow the ipu3-bridge code to also be used by the atomisp
driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c     | 11 +++++++----
 drivers/media/pci/intel/ipu-bridge.h     | 10 ++++++++--
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 891dc0b7c960..d60992fbdbeb 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -148,8 +148,7 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
 	return orientation;
 }
 
-static int ipu_bridge_parse_ssdb(struct acpi_device *adev,
-				 struct ipu_sensor *sensor)
+int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor)
 {
 	struct ipu_sensor_ssdb ssdb = {};
 	int ret;
@@ -179,6 +178,7 @@ static int ipu_bridge_parse_ssdb(struct acpi_device *adev,
 
 	return 0;
 }
+EXPORT_SYMBOL_NS_GPL(ipu_bridge_parse_ssdb, INTEL_IPU_BRIDGE);
 
 static void ipu_bridge_create_fwnode_properties(
 	struct ipu_sensor *sensor,
@@ -343,7 +343,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 
 		sensor = &bridge->sensors[bridge->n_sensors];
 
-		ret = ipu_bridge_parse_ssdb(adev, sensor);
+		ret = bridge->parse_sensor_fwnode(adev, sensor);
 		if (ret)
 			goto err_put_adev;
 
@@ -441,7 +441,9 @@ static int ipu_bridge_sensors_are_ready(void)
 	return ready;
 }
 
-int ipu_bridge_init(struct device *dev)
+int ipu_bridge_init(struct device *dev,
+		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
+					       struct ipu_sensor *sensor))
 {
 	struct fwnode_handle *fwnode;
 	struct ipu_bridge *bridge;
@@ -459,6 +461,7 @@ int ipu_bridge_init(struct device *dev)
 		sizeof(bridge->ipu_node_name));
 	bridge->ipu_hid_node.name = bridge->ipu_node_name;
 	bridge->dev = dev;
+	bridge->parse_sensor_fwnode = parse_sensor_fwnode;
 
 	ret = software_node_register(&bridge->ipu_hid_node);
 	if (ret < 0) {
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
index a8b89c4b95bc..969d8d7d6b93 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -142,6 +142,8 @@ struct ipu_sensor {
 
 struct ipu_bridge {
 	struct device *dev;
+	int (*parse_sensor_fwnode)(struct acpi_device *adev,
+				   struct ipu_sensor *sensor);
 	char ipu_node_name[ACPI_ID_LEN];
 	struct software_node ipu_hid_node;
 	u32 data_lanes[4];
@@ -150,9 +152,13 @@ struct ipu_bridge {
 };
 
 #if IS_ENABLED(CONFIG_IPU_BRIDGE)
-int ipu_bridge_init(struct device *dev);
+int ipu_bridge_init(struct device *dev,
+		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
+					       struct ipu_sensor *sensor));
+int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor);
 #else
-static inline int ipu_bridge_init(struct device *dev) { return 0; }
+/* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */
+#define ipu_bridge_init(dev, parse_sensor_fwnode)	(0)
 #endif
 
 #endif
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 4068fa0a5ecf..26c4c1375990 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1725,7 +1725,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 			return -EINVAL;
 		}
 
-		r = ipu_bridge_init(dev);
+		r = ipu_bridge_init(dev, ipu_bridge_parse_ssdb);
 		if (r)
 			return r;
 	}
-- 
2.41.0

[PATCH v2 11/15] media: ipu-bridge: Move ipu-bridge.h to include/media/

[Hans de Goede]

Move ipu-bridge.h to include/media/, so that it can also be used by
the atomisp code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c                    | 4 ++--
 drivers/media/pci/intel/ipu3/ipu3-cio2.c                | 3 ++-
 {drivers/media/pci/intel => include/media}/ipu-bridge.h | 0
 3 files changed, 4 insertions(+), 3 deletions(-)
 rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (100%)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index d60992fbdbeb..eb7c56e8ef9f 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -5,9 +5,9 @@
 #include <linux/device.h>
 #include <linux/i2c.h>
 #include <linux/property.h>
-#include <media/v4l2-fwnode.h>
 
-#include "ipu-bridge.h"
+#include <media/ipu-bridge.h>
+#include <media/v4l2-fwnode.h>
 
 /*
  * Extend this array with ACPI Hardware IDs of devices known to be working
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 26c4c1375990..51a6d7cc44d2 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -22,6 +22,8 @@
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
 #include <linux/vmalloc.h>
+
+#include <media/ipu-bridge.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -29,7 +31,6 @@
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-dma-sg.h>
 
-#include "../ipu-bridge.h"
 #include "ipu3-cio2.h"
 
 struct ipu3_cio2_fmt {
diff --git a/drivers/media/pci/intel/ipu-bridge.h b/include/media/ipu-bridge.h
similarity index 100%
rename from drivers/media/pci/intel/ipu-bridge.h
rename to include/media/ipu-bridge.h
-- 
2.41.0

[PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

The GalaxyCore GC0310 is used together with the atomisp no various
devices, add it to ipu_supported_sensors[].

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index eb7c56e8ef9f..07a34f20af8e 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
 	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
 	/* Omnivision ov13b10 */
 	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
+	/* GalaxyCore GC0310 */
+	IPU_SENSOR_CONFIG("INT0310", 0),
 };
 
 static const struct ipu_property_names prop_names = {
-- 
2.41.0

[PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Hans de Goede]

Remove the duplicate IPU ACPI bridge code and use the new
shared ipu_bridge_init() functionality.

Note this will also use / assume v4l2-async device instantation for
ov5693 sensors on atomisp devices since ipu_supported_sensors[]
already contains a match for this.

This is fine since recent atomisp improvements allow the atomisp code
to work with generic v4l2 sensor drivers and using an unmodified
drivers/media/i2c/ov5693.c has been successfully tested on
an Acer Iconia W4 820 tablet with Bay an ISP2400 + OV5693 sensor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/Kconfig         |   2 +
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ----
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 326 +++---------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 4 files changed, 48 insertions(+), 348 deletions(-)

diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
index e9b168ba97bf..1c1007362e21 100644
--- a/drivers/staging/media/atomisp/Kconfig
+++ b/drivers/staging/media/atomisp/Kconfig
@@ -12,9 +12,11 @@ menuconfig INTEL_ATOMISP
 config VIDEO_ATOMISP
 	tristate "Intel Atom Image Signal Processor Driver"
 	depends on VIDEO_DEV && INTEL_ATOMISP
+	depends on MEDIA_PCI_SUPPORT && I2C
 	depends on PMIC_OPREGION
 	select V4L2_FWNODE
 	select IOSF_MBI
+	select IPU_BRIDGE
 	select VIDEOBUF2_VMALLOC
 	select VIDEO_V4L2_SUBDEV_API
 	help
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
index 16ddb3ab2963..8a112acba1e0 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
@@ -30,9 +30,6 @@
 #define CSI2_PAD_SOURCE		1
 #define CSI2_PADS_NUM		2
 
-#define CSI2_MAX_LANES		4
-#define CSI2_MAX_LINK_FREQS	3
-
 #define CSI2_MAX_ACPI_GPIOS	2u
 
 struct acpi_device;
@@ -55,70 +52,6 @@ struct atomisp_csi2_acpi_gpio_parsing_data {
 	unsigned int map_count;
 };
 
-enum atomisp_csi2_sensor_swnodes {
-	SWNODE_SENSOR,
-	SWNODE_SENSOR_PORT,
-	SWNODE_SENSOR_ENDPOINT,
-	SWNODE_CSI2_PORT,
-	SWNODE_CSI2_ENDPOINT,
-	SWNODE_COUNT
-};
-
-struct atomisp_csi2_property_names {
-	char clock_frequency[16];
-	char rotation[9];
-	char bus_type[9];
-	char data_lanes[11];
-	char remote_endpoint[16];
-	char link_frequencies[17];
-};
-
-struct atomisp_csi2_node_names {
-	char port[7];
-	char endpoint[11];
-	char remote_port[7];
-};
-
-struct atomisp_csi2_sensor_config {
-	const char *hid;
-	int lanes;
-	int nr_link_freqs;
-	u64 link_freqs[CSI2_MAX_LINK_FREQS];
-};
-
-struct atomisp_csi2_sensor {
-	/* Append port in "-%u" format as suffix of HID */
-	char name[ACPI_ID_LEN + 4];
-	struct acpi_device *adev;
-	int port;
-	int lanes;
-
-	/* SWNODE_COUNT + 1 for terminating NULL */
-	const struct software_node *group[SWNODE_COUNT + 1];
-	struct software_node swnodes[SWNODE_COUNT];
-	struct atomisp_csi2_node_names node_names;
-	struct atomisp_csi2_property_names prop_names;
-	/* "clock-frequency", "rotation" + terminating entry */
-	struct property_entry dev_properties[3];
-	/* "bus-type", "data-lanes", "remote-endpoint" + "link-freq" + terminating entry */
-	struct property_entry ep_properties[5];
-	/* "data-lanes", "remote-endpoint" + terminating entry */
-	struct property_entry csi2_properties[3];
-	struct software_node_ref_args local_ref[1];
-	struct software_node_ref_args remote_ref[1];
-	struct software_node_ref_args vcm_ref[1];
-	/* GPIO mappings storage */
-	struct atomisp_csi2_acpi_gpio_map gpio_map;
-};
-
-struct atomisp_csi2_bridge {
-	struct software_node csi2_node;
-	char csi2_node_name[14];
-	u32 data_lanes[CSI2_MAX_LANES];
-	unsigned int n_sensors;
-	struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
-};
-
 struct atomisp_mipi_csi2_device {
 	struct v4l2_subdev subdev;
 	struct media_pad pads[CSI2_PADS_NUM];
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index 0d12ba78d9c1..d7b8c3de0e17 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -14,31 +14,14 @@
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/property.h>
+
+#include <media/ipu-bridge.h>
 #include <media/v4l2-fwnode.h>
 
 #include "atomisp_cmd.h"
 #include "atomisp_csi2.h"
 #include "atomisp_internal.h"
 
-#define NODE_SENSOR(_HID, _PROPS)		\
-	((const struct software_node) {		\
-		.name = _HID,			\
-		.properties = _PROPS,		\
-	})
-
-#define NODE_PORT(_PORT, _SENSOR_NODE)		\
-	((const struct software_node) {		\
-		.name = _PORT,			\
-		.parent = _SENSOR_NODE,		\
-	})
-
-#define NODE_ENDPOINT(_EP, _PORT, _PROPS)	\
-	((const struct software_node) {		\
-		.name = _EP,			\
-		.parent = _PORT,		\
-		.properties = _PROPS,		\
-	})
-
 #define PMC_CLK_RATE_19_2MHZ			19200000
 
 /*
@@ -83,21 +66,6 @@ static const guid_t atomisp_dsm_guid =
 	GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
 		  0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe);
 
-/*
- * Extend this array with ACPI Hardware IDs of sensors known to be working
- * plus the default number of links + link-frequencies.
- *
- * Do not add an entry for a sensor that is not actually supported,
- * or which have not yet been converted to work without atomisp_gmin
- * power-management and with v4l2-async probing.
- */
-static const struct atomisp_csi2_sensor_config supported_sensors[] = {
-	/* GalaxyCore GC0310 */
-	{ "INT0310", 1 },
-	/* Omnivision OV2680 */
-	{ "OVTI2680", 1 },
-};
-
 /*
  * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
  * code from atomisp_gmin_platform.c.
@@ -400,8 +368,7 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
  * the INT3472 discrete.c code and there is some overlap, but there are
  * enough differences that it is difficult to share the code.
  */
-static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
-					  struct acpi_device *adev)
+static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 {
 	struct atomisp_csi2_acpi_gpio_parsing_data data = { };
 	LIST_HEAD(resource_list);
@@ -469,9 +436,12 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
 		}
 	}
 
+	data.map = kzalloc(sizeof(*data.map), GFP_KERNEL);
+	if (!data.map)
+		return -ENOMEM;
+
 	/* Now parse the ACPI resources and build the lookup table */
 	data.adev = adev;
-	data.map = &sensor->gpio_map;
 	ret = acpi_dev_get_resources(adev, &resource_list,
 				     atomisp_csi2_handle_acpi_gpio_res, &data);
 	if (ret < 0)
@@ -491,220 +461,59 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
 	return ret;
 }
 
-static const struct atomisp_csi2_property_names prop_names = {
-	.clock_frequency = "clock-frequency",
-	.rotation = "rotation",
-	.bus_type = "bus-type",
-	.data_lanes = "data-lanes",
-	.remote_endpoint = "remote-endpoint",
-	.link_frequencies = "link-frequencies",
-};
-
-static void atomisp_csi2_create_fwnode_properties(struct atomisp_csi2_sensor *sensor,
-						  struct atomisp_csi2_bridge *bridge,
-						  const struct atomisp_csi2_sensor_config *cfg)
+static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
+					    struct ipu_sensor *sensor)
 {
-	sensor->prop_names = prop_names;
+	static const struct acpi_device_id dual_lane_sensors[] = {
+		{ "INT33BE" },	/* OV5693 */
+		{}
+	};
+	int ret, clock_num, lanes;
 
-	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
-	sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
+	/*
+	 * ACPI takes care of turning the PMC clock on and off, but on BYT
+	 * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
+	 * Get the PMC-clock number from ACPI PR0 method and set it to 19.2 MHz.
+	 * The PMC-clock number is also used to determine the default CSI port.
+	 */
+	clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
 
-	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
-						       PMC_CLK_RATE_19_2MHZ);
-	sensor->dev_properties[1] = PROPERTY_ENTRY_U32(sensor->prop_names.rotation, 0);
+	ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
+	if (ret)
+		return ret;
 
-	sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
-						      V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
-	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
-								bridge->data_lanes,
-								sensor->lanes);
-	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
-							    sensor->local_ref);
-	if (cfg->nr_link_freqs > 0)
-		sensor->ep_properties[3] =
-			PROPERTY_ENTRY_U64_ARRAY_LEN(sensor->prop_names.link_frequencies,
-						     cfg->link_freqs, cfg->nr_link_freqs);
-
-	sensor->csi2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
-								  bridge->data_lanes,
-								  sensor->lanes);
-	sensor->csi2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
-							      sensor->remote_ref);
-}
-
-static void atomisp_csi2_init_swnode_names(struct atomisp_csi2_sensor *sensor)
-{
-	snprintf(sensor->node_names.remote_port,
-		 sizeof(sensor->node_names.remote_port),
-		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->port);
-	snprintf(sensor->node_names.port,
-		 sizeof(sensor->node_names.port),
-		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
-	snprintf(sensor->node_names.endpoint,
-		 sizeof(sensor->node_names.endpoint),
-		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
-}
-
-static void atomisp_csi2_init_swnode_group(struct atomisp_csi2_sensor *sensor)
-{
-	struct software_node *nodes = sensor->swnodes;
-
-	sensor->group[SWNODE_SENSOR] = &nodes[SWNODE_SENSOR];
-	sensor->group[SWNODE_SENSOR_PORT] = &nodes[SWNODE_SENSOR_PORT];
-	sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
-	sensor->group[SWNODE_CSI2_PORT] = &nodes[SWNODE_CSI2_PORT];
-	sensor->group[SWNODE_CSI2_ENDPOINT] = &nodes[SWNODE_CSI2_ENDPOINT];
-}
-
-static void atomisp_csi2_create_connection_swnodes(struct atomisp_csi2_bridge *bridge,
-						   struct atomisp_csi2_sensor *sensor)
-{
-	struct software_node *nodes = sensor->swnodes;
-
-	atomisp_csi2_init_swnode_names(sensor);
-
-	nodes[SWNODE_SENSOR] = NODE_SENSOR(sensor->name,
-					   sensor->dev_properties);
-	nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
-					      &nodes[SWNODE_SENSOR]);
-	nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
-						      &nodes[SWNODE_SENSOR_PORT],
-						      sensor->ep_properties);
-	nodes[SWNODE_CSI2_PORT] = NODE_PORT(sensor->node_names.remote_port,
-					    &bridge->csi2_node);
-	nodes[SWNODE_CSI2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
-						    &nodes[SWNODE_CSI2_PORT],
-						    sensor->csi2_properties);
-
-	atomisp_csi2_init_swnode_group(sensor);
-}
-
-static void atomisp_csi2_unregister_sensors(struct atomisp_csi2_bridge *bridge)
-{
-	struct atomisp_csi2_sensor *sensor;
-	unsigned int i;
-
-	for (i = 0; i < bridge->n_sensors; i++) {
-		sensor = &bridge->sensors[i];
-		software_node_unregister_node_group(sensor->group);
-		acpi_dev_remove_driver_gpios(sensor->adev);
-		acpi_dev_put(sensor->adev);
+	sensor->link = atomisp_csi2_get_port(adev, clock_num);
+	if (sensor->link >= ATOMISP_CAMERA_NR_PORTS) {
+		dev_err(&adev->dev, "Invalid port: %u\n", sensor->link);
+		return -EINVAL;
 	}
-}
 
-static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *cfg,
-				       struct atomisp_csi2_bridge *bridge,
-				       struct atomisp_device *isp)
-{
-	struct fwnode_handle *fwnode, *primary;
-	struct atomisp_csi2_sensor *sensor;
-	struct acpi_device *adev;
-	int ret, clock_num;
+	if (!acpi_match_device_ids(adev, dual_lane_sensors))
+		lanes = 2;
+	else
+		lanes = 1;
 
-	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
-		if (!adev->status.enabled)
-			continue;
-
-		if (bridge->n_sensors >= ATOMISP_CAMERA_NR_PORTS) {
-			dev_err(isp->dev, "Exceeded available CSI2 ports\n");
-			ret = -EOVERFLOW;
-			goto err_put_adev;
-		}
-
-		sensor = &bridge->sensors[bridge->n_sensors];
-
-		/*
-		 * ACPI takes care of turning the PMC clock on and off, but on BYT
-		 * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
-		 * Get the PMC-clock number from ACPI _PR0 method and set it to 19.2 MHz.
-		 * The PMC-clock number is also used to determine the default CSI port.
-		 */
-		clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
-
-		ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
-		if (ret)
-			goto err_put_adev;
-
-		sensor->port = atomisp_csi2_get_port(adev, clock_num);
-		if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
-			acpi_handle_err(adev->handle, "Invalid port: %d\n", sensor->port);
-			ret = -EINVAL;
-			goto err_put_adev;
-		}
-
-		sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", cfg->lanes);
-		if (sensor->lanes > CSI2_MAX_LANES) {
-			acpi_handle_err(adev->handle, "Invalid number of lanes: %d\n", sensor->lanes);
-			ret = -EINVAL;
-			goto err_put_adev;
-		}
-
-		ret = atomisp_csi2_add_gpio_mappings(sensor, adev);
-		if (ret)
-			goto err_put_adev;
-
-		snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
-			 cfg->hid, sensor->port);
-
-		atomisp_csi2_create_fwnode_properties(sensor, bridge, cfg);
-		atomisp_csi2_create_connection_swnodes(bridge, sensor);
-
-		ret = software_node_register_node_group(sensor->group);
-		if (ret)
-			goto err_remove_mappings;
-
-		fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR]);
-		if (!fwnode) {
-			ret = -ENODEV;
-			goto err_free_swnodes;
-		}
-
-		sensor->adev = acpi_dev_get(adev);
-
-		primary = acpi_fwnode_handle(adev);
-		primary->secondary = fwnode;
-
-		bridge->n_sensors++;
+	sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
+	if (sensor->lanes > IPU_MAX_LANES) {
+		dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
+		return -EINVAL;
 	}
 
+	ret = atomisp_csi2_add_gpio_mappings(adev);
+	if (ret)
+		return ret;
+
+	sensor->mclkspeed = PMC_CLK_RATE_19_2MHZ;
+	sensor->rotation = 0;
+	sensor->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
+
 	return 0;
-
-err_free_swnodes:
-	software_node_unregister_node_group(sensor->group);
-err_remove_mappings:
-	acpi_dev_remove_driver_gpios(adev);
-err_put_adev:
-	acpi_dev_put(adev);
-	return ret;
-}
-
-static int atomisp_csi2_connect_sensors(struct atomisp_csi2_bridge *bridge,
-					struct atomisp_device *isp)
-{
-	unsigned int i;
-	int ret;
-
-	for (i = 0; i < ARRAY_SIZE(supported_sensors); i++) {
-		const struct atomisp_csi2_sensor_config *cfg = &supported_sensors[i];
-
-		ret = atomisp_csi2_connect_sensor(cfg, bridge, isp);
-		if (ret)
-			goto err_unregister_sensors;
-	}
-
-	return 0;
-
-err_unregister_sensors:
-	atomisp_csi2_unregister_sensors(bridge);
-	return ret;
 }
 
 int atomisp_csi2_bridge_init(struct atomisp_device *isp)
 {
-	struct atomisp_csi2_bridge *bridge;
 	struct device *dev = isp->dev;
 	struct fwnode_handle *fwnode;
-	int i, ret;
 
 	/*
 	 * This function is intended to run only once and then leave
@@ -716,52 +525,7 @@ int atomisp_csi2_bridge_init(struct atomisp_device *isp)
 	if (fwnode && fwnode->secondary)
 		return 0;
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
-	if (!bridge)
-		return -ENOMEM;
-
-	strscpy(bridge->csi2_node_name, "atomisp-csi2", sizeof(bridge->csi2_node_name));
-	bridge->csi2_node.name = bridge->csi2_node_name;
-
-	ret = software_node_register(&bridge->csi2_node);
-	if (ret < 0) {
-		dev_err(dev, "Failed to register the CSI2 HID node\n");
-		goto err_free_bridge;
-	}
-
-	/*
-	 * Map the lane arrangement, which is fixed for the ISP2 (meaning we
-	 * only need one, rather than one per sensor). We include it as a
-	 * member of the bridge struct rather than a global variable so
-	 * that it survives if the module is unloaded along with the rest of
-	 * the struct.
-	 */
-	for (i = 0; i < CSI2_MAX_LANES; i++)
-		bridge->data_lanes[i] = i + 1;
-
-	ret = atomisp_csi2_connect_sensors(bridge, isp);
-	if (ret || bridge->n_sensors == 0)
-		goto err_unregister_csi2;
-
-	fwnode = software_node_fwnode(&bridge->csi2_node);
-	if (!fwnode) {
-		dev_err(dev, "Error getting fwnode from csi2 software_node\n");
-		ret = -ENODEV;
-		goto err_unregister_sensors;
-	}
-
-	set_secondary_fwnode(dev, fwnode);
-
-	return 0;
-
-err_unregister_sensors:
-	atomisp_csi2_unregister_sensors(bridge);
-err_unregister_csi2:
-	software_node_unregister(&bridge->csi2_node);
-err_free_bridge:
-	kfree(bridge);
-
-	return ret;
+	return ipu_bridge_init(dev, atomisp_csi2_parse_sensor_fwnode);
 }
 
 /******* V4L2 sub-device asynchronous registration callbacks***********/
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index c43b916a006e..0d80f0893a2e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1615,3 +1615,4 @@ MODULE_AUTHOR("Wen Wang <wen.w.wang@intel.com>");
 MODULE_AUTHOR("Xiaolin Zhang <xiaolin.zhang@intel.com>");
 MODULE_LICENSE("GPL");
 MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver");
+MODULE_IMPORT_NS(INTEL_IPU_BRIDGE);
-- 
2.41.0

[PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Hans de Goede]

In most cases when a VCM is used there is a single integrated module
with the sensor + VCM + lens. This means that the sensor and VCM often
share regulators and possibly also something like a powerdown pin.

In the ACPI tables this is modelled as a single ACPI device with
multiple I2cSerialBus resources.

On atomisp devices the regulators and clks are modelled as ACPI
power-resources, which are controlled by the (ACPI) power state
of the sensor. So the sensor must be in D0 power state for the VCM
to work.

To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
so that the sensor will automatically be runtime-resumed whenever
the VCM is runtime-resumed.

This requires the probing of the VCM and thus the creation of the VCM
I2C-client to be delayed till after the sensor driver has bound.

Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
bound op, so that it is done after the sensor driver has bound; and
add code to add the device-link.

This fixes the problem with the shared ACPI power-resources on atomisp2
and this avoids the need for VCM related workarounds on IPU3 / IPU6.

E.g. until now the dw9719 driver needed to get and control a Vsio
(V sensor IO) regulator since that needs to be enabled to enable I2C
pass-through on the PMIC on the sensor module. So the driver was
controlling this regulator even though the actual dw9719 chip has no
Vsio pin / power-plane.

This also removes the need for ipu_bridge_init() to return
-EPROBE_DEFER since the VCM is now instantiated later.

Reviewed-by: Andy Shevchenko <andy@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/pci/intel/ipu-bridge.c     | 159 +++++++++++++++--------
 drivers/media/pci/intel/ipu3/ipu3-cio2.c |   5 +
 include/media/ipu-bridge.h               |   5 +-
 3 files changed, 109 insertions(+), 60 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index 07a34f20af8e..32dabc16a7b4 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,7 +4,9 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
 #include <media/v4l2-fwnode.h>
@@ -289,29 +291,112 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 	ipu_bridge_init_swnode_group(sensor);
 }
 
-static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
-{
-	struct i2c_board_info board_info = { };
+/*
+ * The actual instantiation must be done from a workqueue to avoid
+ * a deadlock on taking list_lock from v4l2-async twice.
+ */
+struct ipu_bridge_instantiate_vcm_work_data {
+	struct work_struct work;
+	struct device *sensor;
 	char name[16];
+	struct i2c_board_info board_info;
+};
 
-	if (!sensor->vcm_type)
-		return;
+static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
+{
+	struct ipu_bridge_instantiate_vcm_work_data *work =
+		container_of(_work,
+			     struct ipu_bridge_instantiate_vcm_work_data,
+			     work);
+	struct acpi_device *adev = ACPI_COMPANION(work->sensor);
+	struct i2c_client *vcm_client;
+	bool put_fwnode = true;
+	int ret;
 
-	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
-	board_info.dev_name = name;
-	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
-	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
-
-	sensor->vcm_i2c_client =
-		i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(sensor->adev),
-					      1, &board_info);
-	if (IS_ERR(sensor->vcm_i2c_client)) {
-		dev_warn(&sensor->adev->dev, "Error instantiation VCM i2c-client: %ld\n",
-			 PTR_ERR(sensor->vcm_i2c_client));
-		sensor->vcm_i2c_client = NULL;
+	/*
+	 * The client may get probed before the device_link gets added below
+	 * make sure the sensor is powered-up during probe.
+	 */
+	ret = pm_runtime_get_sync(work->sensor);
+	if (ret < 0) {
+		dev_err(work->sensor, "Error %d runtime-resuming sensor, cannot instantiate VCM\n",
+			ret);
+		goto out;
 	}
+
+	/*
+	 * Note the client is created only once and then kept around
+	 * even after a rmmod, just like the software-nodes.
+	 */
+	vcm_client = i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(adev),
+						   1, &work->board_info);
+	if (IS_ERR(vcm_client)) {
+		dev_err(work->sensor, "Error instantiating VCM client: %ld\n",
+			PTR_ERR(vcm_client));
+		goto out;
+	}
+
+	device_link_add(&vcm_client->dev, work->sensor, DL_FLAG_PM_RUNTIME);
+
+	dev_info(work->sensor, "Instantiated %s VCM\n", work->board_info.type);
+	put_fwnode = false; /* Ownership has passed to the i2c-client */
+
+out:
+	pm_runtime_put(work->sensor);
+	put_device(work->sensor);
+	if (put_fwnode)
+		fwnode_handle_put(work->board_info.fwnode);
+	kfree(work);
 }
 
+int ipu_bridge_instantiate_vcm(struct device *sensor)
+{
+	struct ipu_bridge_instantiate_vcm_work_data *work;
+	struct fwnode_handle *vcm_fwnode;
+	struct i2c_client *vcm_client;
+	struct acpi_device *adev;
+	char *sep;
+
+	adev = ACPI_COMPANION(sensor);
+	if (!adev)
+		return 0;
+
+	vcm_fwnode = fwnode_find_reference(dev_fwnode(sensor), "lens-focus", 0);
+	if (IS_ERR(vcm_fwnode))
+		return 0;
+
+	/* When reloading modules the client will already exist */
+	vcm_client = i2c_find_device_by_fwnode(vcm_fwnode);
+	if (vcm_client) {
+		fwnode_handle_put(vcm_fwnode);
+		put_device(&vcm_client->dev);
+		return 0;
+	}
+
+	work = kzalloc(sizeof(*work), GFP_KERNEL);
+	if (!work) {
+		fwnode_handle_put(vcm_fwnode);
+		return -ENOMEM;
+	}
+
+	INIT_WORK(&work->work, ipu_bridge_instantiate_vcm_work);
+	work->sensor = get_device(sensor);
+	snprintf(work->name, sizeof(work->name), "%s-VCM",
+		 acpi_dev_name(adev));
+	work->board_info.dev_name = work->name;
+	work->board_info.fwnode = vcm_fwnode;
+	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
+		I2C_NAME_SIZE);
+	/* Strip "-<link>" postfix */
+	sep = strchrnul(work->board_info.type, '-');
+	*sep = 0;
+
+	queue_work(system_long_wq, &work->work);
+
+	return 0;
+}
+EXPORT_SYMBOL(ipu_bridge_instantiate_vcm);
+
 static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 {
 	struct ipu_sensor *sensor;
@@ -321,7 +406,6 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
 		sensor = &bridge->sensors[i];
 		software_node_unregister_node_group(sensor->group);
 		acpi_dev_put(sensor->adev);
-		i2c_unregister_device(sensor->vcm_i2c_client);
 	}
 }
 
@@ -371,8 +455,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 		primary = acpi_fwnode_handle(adev);
 		primary->secondary = fwnode;
 
-		ipu_bridge_instantiate_vcm_i2c_client(sensor);
-
 		dev_info(bridge->dev, "Found supported sensor %s\n",
 			 acpi_dev_name(adev));
 
@@ -409,40 +491,6 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
 	return ret;
 }
 
-/*
- * The VCM cannot be probed until the PMIC is completely setup. We cannot rely
- * on -EPROBE_DEFER for this, since the consumer<->supplier relations between
- * the VCM and regulators/clks are not described in ACPI, instead they are
- * passed as board-data to the PMIC drivers. Since -PROBE_DEFER does not work
- * for the clks/regulators the VCM i2c-clients must not be instantiated until
- * the PMIC is fully setup.
- *
- * The sensor/VCM ACPI device has an ACPI _DEP on the PMIC, check this using the
- * acpi_dev_ready_for_enumeration() helper, like the i2c-core-acpi code does
- * for the sensors.
- */
-static int ipu_bridge_sensors_are_ready(void)
-{
-	struct acpi_device *adev;
-	bool ready = true;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
-		const struct ipu_sensor_config *cfg =
-			&ipu_supported_sensors[i];
-
-		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
-			if (!adev->status.enabled)
-				continue;
-
-			if (!acpi_dev_ready_for_enumeration(adev))
-				ready = false;
-		}
-	}
-
-	return ready;
-}
-
 int ipu_bridge_init(struct device *dev,
 		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
 					       struct ipu_sensor *sensor))
@@ -452,9 +500,6 @@ int ipu_bridge_init(struct device *dev,
 	unsigned int i;
 	int ret;
 
-	if (!ipu_bridge_sensors_are_ready())
-		return -EPROBE_DEFER;
-
 	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
 	if (!bridge)
 		return -ENOMEM;
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 51a6d7cc44d2..690fc1c919af 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -1388,10 +1388,15 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
 	struct cio2_device *cio2 = to_cio2_device(notifier);
 	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
 	struct cio2_queue *q;
+	int ret;
 
 	if (cio2->queue[s_asd->csi2.port].sensor)
 		return -EBUSY;
 
+	ret = ipu_bridge_instantiate_vcm(sd->dev);
+	if (ret)
+		return ret;
+
 	q = &cio2->queue[s_asd->csi2.port];
 
 	q->csi2 = s_asd->csi2;
diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
index 969d8d7d6b93..31b138170c73 100644
--- a/include/media/ipu-bridge.h
+++ b/include/media/ipu-bridge.h
@@ -7,8 +7,6 @@
 #include <linux/types.h>
 #include <media/v4l2-fwnode.h>
 
-struct i2c_client;
-
 #define IPU_HID				"INT343E"
 #define IPU_MAX_LANES				4
 #define IPU_MAX_PORTS				4
@@ -117,7 +115,6 @@ struct ipu_sensor {
 	/* append ssdb.link(u8) in "-%u" format as suffix of HID */
 	char name[ACPI_ID_LEN + 4];
 	struct acpi_device *adev;
-	struct i2c_client *vcm_i2c_client;
 
 	/* SWNODE_COUNT + 1 for terminating NULL */
 	const struct software_node *group[SWNODE_COUNT + 1];
@@ -156,9 +153,11 @@ int ipu_bridge_init(struct device *dev,
 		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
 					       struct ipu_sensor *sensor));
 int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor);
+int ipu_bridge_instantiate_vcm(struct device *sensor);
 #else
 /* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */
 #define ipu_bridge_init(dev, parse_sensor_fwnode)	(0)
+static inline int ipu_bridge_instantiate_vcm(struct device *s) { return 0; }
 #endif
 
 #endif
-- 
2.41.0

[PATCH v2 15/15] [RFC] media: dw9719: Drop hack to enable "vsio" regulator

[Hans de Goede]

Drop the hack where the driver is getting + enabling a "vsio" regulator
even though the dw9719 does not have a vsio pin / power-plane at all.

Now that drivers/media/pci/intel/ipu-bridge.c adds a device-link
making the VCM a consumer of the sensor this hack is no longer necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note to reviewers, the dw9719 driver is not upstream yet (I plan to
resubmit it soon with this squashed in). This patch is only included
in this patch-set to illustrate how the VCM -> sensor device-link
avoids the need for hacks like this.
---
 drivers/media/i2c/dw9719.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
index 94b76b4b2463..5fe01a125c1a 100644
--- a/drivers/media/i2c/dw9719.c
+++ b/drivers/media/i2c/dw9719.c
@@ -35,14 +35,12 @@
 #define DW9719_DEFAULT_VCM_FREQ		0x60
 #define DW9719_ENABLE_RINGING		0x02
 
-#define NUM_REGULATORS			2
-
 #define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
 
 struct dw9719_device {
 	struct device *dev;
 	struct i2c_client *client;
-	struct regulator_bulk_data regulators[NUM_REGULATORS];
+	struct regulator *regulator;
 	struct v4l2_subdev sd;
 
 	struct dw9719_v4l2_ctrls {
@@ -131,14 +129,14 @@ static int dw9719_detect(struct dw9719_device *dw9719)
 
 static int dw9719_power_down(struct dw9719_device *dw9719)
 {
-	return regulator_bulk_disable(NUM_REGULATORS, dw9719->regulators);
+	return regulator_disable(dw9719->regulator);
 }
 
 static int dw9719_power_up(struct dw9719_device *dw9719)
 {
 	int ret;
 
-	ret = regulator_bulk_enable(NUM_REGULATORS, dw9719->regulators);
+	ret = regulator_enable(dw9719->regulator);
 	if (ret)
 		return ret;
 
@@ -315,21 +313,10 @@ static int dw9719_probe(struct i2c_client *client)
 	dw9719->client = client;
 	dw9719->dev = &client->dev;
 
-	dw9719->regulators[0].supply = "vdd";
-	/*
-	 * The DW9719 has only the 1 VDD voltage input, but some PMICs such as
-	 * the TPS68470 PMIC have I2C passthrough capability, to disconnect the
-	 * sensor's I2C pins from the I2C bus when the sensors VSIO (Sensor-IO)
-	 * is off, because some sensors then short these pins to ground;
-	 * and the DW9719 might sit behind this passthrough, this it needs to
-	 * enable VSIO as that will also enable the I2C passthrough.
-	 */
-	dw9719->regulators[1].supply = "vsio";
-
-	ret = devm_regulator_bulk_get(&client->dev, NUM_REGULATORS,
-				      dw9719->regulators);
-	if (ret)
-		return dev_err_probe(&client->dev, ret, "getting regulators\n");
+	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(dw9719->regulator))
+		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),
+				     "getting regulator\n");
 
 	v4l2_i2c_subdev_init(&dw9719->sd, client, &dw9719_ops);
 	dw9719->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-- 
2.41.0

Re: [PATCH v2 04/15] media: ipu-bridge: Allow building as module

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> After commit f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu
> bridge and move out of ipu3") the ipu-bridge code is always builtin

built in

> even if all (its only) consumers are build as modules.

I'm not sure I understood the remark in parentheses correctly. All of
its consumers are the only consumers of this module?

> Fix this by turning "config IPU_BRIDGE" into a pure library Kconfig
> option (not user selectable, must be selected by consumers) and
> re-introducing the CIO2_BRIDGE Kconfig bits in .../pci/intel/ipu3/Kconfig
> which were dropped to still allow building ipu3-cio2 without ipu-bridge
> support.

...

>  config IPU_BRIDGE
> -       bool "Intel IPU Sensors Bridge"
> -       depends on VIDEO_IPU3_CIO2 && ACPI
> -       depends on I2C
> -       help
> -         This extension provides an API for the Intel IPU driver to create
> -         connections to cameras that are hidden in the SSDB buffer in ACPI.
> -         It can be used to enable support for cameras in detachable / hybrid
> -         devices that ship with Windows.
> -
> -         Say Y here if your device is a detachable / hybrid laptop that comes
> -         with Windows installed by the OEM, for example:
> -
> -               - Microsoft Surface models (except Surface Pro 3)
> -               - The Lenovo Miix line (for example the 510, 520, 710 and 720)
> -               - Dell 7285
> -
> -         If in doubt, say N here.
> +       tristate
> +       depends on ACPI && I2C

Can we leave

  depends on I2C

line untouched?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
> sensor->adev is not set yet.
>
> So if either of the dev_warn() calls about unknown values are hit this
> will lead to a NULL pointer deref.
>
> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
> on errors harder, to fix this.

TBH, I don't like this approach, it seems a bit dirty to me.

First of all, why do we need pci_dev to be a parameter in this function?
Second, why don't we consistently use the ACPI handle (with respective
acpi_handle_*() macros to print messages)?

So, my proposal here is to actually save the ACPI device handle in the
sensor object and use it for the messaging. It makes it consistent and
doesn't require to rewrite adev field which seems the dirty part to
me.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 09/15] media: ipu-bridge: Drop early setting of sensor->adev

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> sensor->adev is no longer dereferenced before it is permanently set by:
>
>         sensor->adev = acpi_dev_get(adev);
>
> So the early assignment with a borrowed reference can be dropped.

Ah, now I see that that change was temporary. Can we avoid backporting
it please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 10/15] media: ipu-bridge: Add a parse_sensor_fwnode callback to ipu_bridge_init()

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:21 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add a parse_sensor_fwnode callback to ipu_bridge_init(), so that

->parse_sensor_fwnode()

> ipu_bridge_init() can be used with other sensor fwnode parse functions
> then just ipu_bridge_parse_ssdb().
>
> This will allow the ipu3-bridge code to also be used by the atomisp
> driver.

...

> +int ipu_bridge_init(struct device *dev,
> +                   int (*parse_sensor_fwnode)(struct acpi_device *adev,
> +                                              struct ipu_sensor *sensor))

Wondering if

  typedef ipu_parse_sensor_fwnode_t ...

will make all the code better looking.

...

> +       int (*parse_sensor_fwnode)(struct acpi_device *adev,
> +                                  struct ipu_sensor *sensor);

Ditto.

...

> +int ipu_bridge_init(struct device *dev,
> +                   int (*parse_sensor_fwnode)(struct acpi_device *adev,
> +                                              struct ipu_sensor *sensor));

Ditto.

...

> +/* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */

@parse_sensor_fwnode
argument

> +#define ipu_bridge_init(dev, parse_sensor_fwnode)      (0)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 11/15] media: ipu-bridge: Move ipu-bridge.h to include/media/

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Move ipu-bridge.h to include/media/, so that it can also be used by
> the atomisp code.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c                    | 4 ++--
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c                | 3 ++-
>  {drivers/media/pci/intel => include/media}/ipu-bridge.h | 0
>  3 files changed, 4 insertions(+), 3 deletions(-)
>  rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (100%)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index d60992fbdbeb..eb7c56e8ef9f 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -5,9 +5,9 @@
>  #include <linux/device.h>
>  #include <linux/i2c.h>
>  #include <linux/property.h>
> -#include <media/v4l2-fwnode.h>
>
> -#include "ipu-bridge.h"
> +#include <media/ipu-bridge.h>
> +#include <media/v4l2-fwnode.h>
>
>  /*
>   * Extend this array with ACPI Hardware IDs of devices known to be working
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 26c4c1375990..51a6d7cc44d2 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -22,6 +22,8 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
>  #include <linux/vmalloc.h>
> +
> +#include <media/ipu-bridge.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -29,7 +31,6 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-dma-sg.h>
>
> -#include "../ipu-bridge.h"
>  #include "ipu3-cio2.h"
>
>  struct ipu3_cio2_fmt {
> diff --git a/drivers/media/pci/intel/ipu-bridge.h b/include/media/ipu-bridge.h
> similarity index 100%
> rename from drivers/media/pci/intel/ipu-bridge.h
> rename to include/media/ipu-bridge.h
> --
> 2.41.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The GalaxyCore GC0310 is used together with the atomisp no various

on ?

> devices, add it to ipu_supported_sensors[].

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index eb7c56e8ef9f..07a34f20af8e 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>         IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>         /* Omnivision ov13b10 */
>         IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> +       /* GalaxyCore GC0310 */
> +       IPU_SENSOR_CONFIG("INT0310", 0),
>  };
>
>  static const struct ipu_property_names prop_names = {
> --
> 2.41.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Remove the duplicate IPU ACPI bridge code and use the new
> shared ipu_bridge_init() functionality.
>
> Note this will also use / assume v4l2-async device instantation for

instantiation

> ov5693 sensors on atomisp devices since ipu_supported_sensors[]
> already contains a match for this.
>
> This is fine since recent atomisp improvements allow the atomisp code
> to work with generic v4l2 sensor drivers and using an unmodified
> drivers/media/i2c/ov5693.c has been successfully tested on
> an Acer Iconia W4 820 tablet with Bay an ISP2400 + OV5693 sensor.

with an?
with Bay Trail and ?

This is a cool patch!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

A few remarks below.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/Kconfig         |   2 +
>  .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ----
>  .../media/atomisp/pci/atomisp_csi2_bridge.c   | 326 +++---------------
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
>  4 files changed, 48 insertions(+), 348 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
> index e9b168ba97bf..1c1007362e21 100644
> --- a/drivers/staging/media/atomisp/Kconfig
> +++ b/drivers/staging/media/atomisp/Kconfig
> @@ -12,9 +12,11 @@ menuconfig INTEL_ATOMISP
>  config VIDEO_ATOMISP
>         tristate "Intel Atom Image Signal Processor Driver"
>         depends on VIDEO_DEV && INTEL_ATOMISP

> +       depends on MEDIA_PCI_SUPPORT && I2C

These two on a single "depends on" line makes me confused.
I would split the PCI part from the I2C one. However, I haven't
checked the context of other media drivers, maybe this is an existing
idiom, in that case let it be.

>         depends on PMIC_OPREGION
>         select V4L2_FWNODE
>         select IOSF_MBI
> +       select IPU_BRIDGE
>         select VIDEOBUF2_VMALLOC
>         select VIDEO_V4L2_SUBDEV_API
>         help
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> index 16ddb3ab2963..8a112acba1e0 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
> @@ -30,9 +30,6 @@
>  #define CSI2_PAD_SOURCE                1
>  #define CSI2_PADS_NUM          2
>
> -#define CSI2_MAX_LANES         4
> -#define CSI2_MAX_LINK_FREQS    3
> -
>  #define CSI2_MAX_ACPI_GPIOS    2u
>
>  struct acpi_device;
> @@ -55,70 +52,6 @@ struct atomisp_csi2_acpi_gpio_parsing_data {
>         unsigned int map_count;
>  };
>
> -enum atomisp_csi2_sensor_swnodes {
> -       SWNODE_SENSOR,
> -       SWNODE_SENSOR_PORT,
> -       SWNODE_SENSOR_ENDPOINT,
> -       SWNODE_CSI2_PORT,
> -       SWNODE_CSI2_ENDPOINT,
> -       SWNODE_COUNT
> -};
> -
> -struct atomisp_csi2_property_names {
> -       char clock_frequency[16];
> -       char rotation[9];
> -       char bus_type[9];
> -       char data_lanes[11];
> -       char remote_endpoint[16];
> -       char link_frequencies[17];
> -};
> -
> -struct atomisp_csi2_node_names {
> -       char port[7];
> -       char endpoint[11];
> -       char remote_port[7];
> -};
> -
> -struct atomisp_csi2_sensor_config {
> -       const char *hid;
> -       int lanes;
> -       int nr_link_freqs;
> -       u64 link_freqs[CSI2_MAX_LINK_FREQS];
> -};
> -
> -struct atomisp_csi2_sensor {
> -       /* Append port in "-%u" format as suffix of HID */
> -       char name[ACPI_ID_LEN + 4];
> -       struct acpi_device *adev;
> -       int port;
> -       int lanes;
> -
> -       /* SWNODE_COUNT + 1 for terminating NULL */
> -       const struct software_node *group[SWNODE_COUNT + 1];
> -       struct software_node swnodes[SWNODE_COUNT];
> -       struct atomisp_csi2_node_names node_names;
> -       struct atomisp_csi2_property_names prop_names;
> -       /* "clock-frequency", "rotation" + terminating entry */
> -       struct property_entry dev_properties[3];
> -       /* "bus-type", "data-lanes", "remote-endpoint" + "link-freq" + terminating entry */
> -       struct property_entry ep_properties[5];
> -       /* "data-lanes", "remote-endpoint" + terminating entry */
> -       struct property_entry csi2_properties[3];
> -       struct software_node_ref_args local_ref[1];
> -       struct software_node_ref_args remote_ref[1];
> -       struct software_node_ref_args vcm_ref[1];
> -       /* GPIO mappings storage */
> -       struct atomisp_csi2_acpi_gpio_map gpio_map;
> -};
> -
> -struct atomisp_csi2_bridge {
> -       struct software_node csi2_node;
> -       char csi2_node_name[14];
> -       u32 data_lanes[CSI2_MAX_LANES];
> -       unsigned int n_sensors;
> -       struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
> -};
> -
>  struct atomisp_mipi_csi2_device {
>         struct v4l2_subdev subdev;
>         struct media_pad pads[CSI2_PADS_NUM];
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> index 0d12ba78d9c1..d7b8c3de0e17 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> @@ -14,31 +14,14 @@
>  #include <linux/device.h>
>  #include <linux/dmi.h>
>  #include <linux/property.h>
> +
> +#include <media/ipu-bridge.h>
>  #include <media/v4l2-fwnode.h>
>
>  #include "atomisp_cmd.h"
>  #include "atomisp_csi2.h"
>  #include "atomisp_internal.h"
>
> -#define NODE_SENSOR(_HID, _PROPS)              \
> -       ((const struct software_node) {         \
> -               .name = _HID,                   \
> -               .properties = _PROPS,           \
> -       })
> -
> -#define NODE_PORT(_PORT, _SENSOR_NODE)         \
> -       ((const struct software_node) {         \
> -               .name = _PORT,                  \
> -               .parent = _SENSOR_NODE,         \
> -       })
> -
> -#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
> -       ((const struct software_node) {         \
> -               .name = _EP,                    \
> -               .parent = _PORT,                \
> -               .properties = _PROPS,           \
> -       })
> -
>  #define PMC_CLK_RATE_19_2MHZ                   19200000
>
>  /*
> @@ -83,21 +66,6 @@ static const guid_t atomisp_dsm_guid =
>         GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
>                   0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe);
>
> -/*
> - * Extend this array with ACPI Hardware IDs of sensors known to be working
> - * plus the default number of links + link-frequencies.
> - *
> - * Do not add an entry for a sensor that is not actually supported,
> - * or which have not yet been converted to work without atomisp_gmin
> - * power-management and with v4l2-async probing.
> - */
> -static const struct atomisp_csi2_sensor_config supported_sensors[] = {
> -       /* GalaxyCore GC0310 */
> -       { "INT0310", 1 },
> -       /* Omnivision OV2680 */
> -       { "OVTI2680", 1 },
> -};
> -
>  /*
>   * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
>   * code from atomisp_gmin_platform.c.
> @@ -400,8 +368,7 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
>   * the INT3472 discrete.c code and there is some overlap, but there are
>   * enough differences that it is difficult to share the code.
>   */
> -static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
> -                                         struct acpi_device *adev)
> +static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  {
>         struct atomisp_csi2_acpi_gpio_parsing_data data = { };
>         LIST_HEAD(resource_list);
> @@ -469,9 +436,12 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
>                 }
>         }
>
> +       data.map = kzalloc(sizeof(*data.map), GFP_KERNEL);
> +       if (!data.map)
> +               return -ENOMEM;
> +
>         /* Now parse the ACPI resources and build the lookup table */
>         data.adev = adev;
> -       data.map = &sensor->gpio_map;
>         ret = acpi_dev_get_resources(adev, &resource_list,
>                                      atomisp_csi2_handle_acpi_gpio_res, &data);
>         if (ret < 0)
> @@ -491,220 +461,59 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
>         return ret;
>  }
>
> -static const struct atomisp_csi2_property_names prop_names = {
> -       .clock_frequency = "clock-frequency",
> -       .rotation = "rotation",
> -       .bus_type = "bus-type",
> -       .data_lanes = "data-lanes",
> -       .remote_endpoint = "remote-endpoint",
> -       .link_frequencies = "link-frequencies",
> -};
> -
> -static void atomisp_csi2_create_fwnode_properties(struct atomisp_csi2_sensor *sensor,
> -                                                 struct atomisp_csi2_bridge *bridge,
> -                                                 const struct atomisp_csi2_sensor_config *cfg)
> +static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
> +                                           struct ipu_sensor *sensor)
>  {
> -       sensor->prop_names = prop_names;
> +       static const struct acpi_device_id dual_lane_sensors[] = {
> +               { "INT33BE" },  /* OV5693 */
> +               {}
> +       };
> +       int ret, clock_num, lanes;
>
> -       sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
> -       sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
> +       /*
> +        * ACPI takes care of turning the PMC clock on and off, but on BYT
> +        * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
> +        * Get the PMC-clock number from ACPI PR0 method and set it to 19.2 MHz.
> +        * The PMC-clock number is also used to determine the default CSI port.
> +        */
> +       clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
>
> -       sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
> -                                                      PMC_CLK_RATE_19_2MHZ);
> -       sensor->dev_properties[1] = PROPERTY_ENTRY_U32(sensor->prop_names.rotation, 0);
> +       ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
> +       if (ret)
> +               return ret;
>
> -       sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
> -                                                     V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
> -       sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> -                                                               bridge->data_lanes,
> -                                                               sensor->lanes);
> -       sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> -                                                           sensor->local_ref);
> -       if (cfg->nr_link_freqs > 0)
> -               sensor->ep_properties[3] =
> -                       PROPERTY_ENTRY_U64_ARRAY_LEN(sensor->prop_names.link_frequencies,
> -                                                    cfg->link_freqs, cfg->nr_link_freqs);
> -
> -       sensor->csi2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
> -                                                                 bridge->data_lanes,
> -                                                                 sensor->lanes);
> -       sensor->csi2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
> -                                                             sensor->remote_ref);
> -}
> -
> -static void atomisp_csi2_init_swnode_names(struct atomisp_csi2_sensor *sensor)
> -{
> -       snprintf(sensor->node_names.remote_port,
> -                sizeof(sensor->node_names.remote_port),
> -                SWNODE_GRAPH_PORT_NAME_FMT, sensor->port);
> -       snprintf(sensor->node_names.port,
> -                sizeof(sensor->node_names.port),
> -                SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
> -       snprintf(sensor->node_names.endpoint,
> -                sizeof(sensor->node_names.endpoint),
> -                SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
> -}
> -
> -static void atomisp_csi2_init_swnode_group(struct atomisp_csi2_sensor *sensor)
> -{
> -       struct software_node *nodes = sensor->swnodes;
> -
> -       sensor->group[SWNODE_SENSOR] = &nodes[SWNODE_SENSOR];
> -       sensor->group[SWNODE_SENSOR_PORT] = &nodes[SWNODE_SENSOR_PORT];
> -       sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
> -       sensor->group[SWNODE_CSI2_PORT] = &nodes[SWNODE_CSI2_PORT];
> -       sensor->group[SWNODE_CSI2_ENDPOINT] = &nodes[SWNODE_CSI2_ENDPOINT];
> -}
> -
> -static void atomisp_csi2_create_connection_swnodes(struct atomisp_csi2_bridge *bridge,
> -                                                  struct atomisp_csi2_sensor *sensor)
> -{
> -       struct software_node *nodes = sensor->swnodes;
> -
> -       atomisp_csi2_init_swnode_names(sensor);
> -
> -       nodes[SWNODE_SENSOR] = NODE_SENSOR(sensor->name,
> -                                          sensor->dev_properties);
> -       nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
> -                                             &nodes[SWNODE_SENSOR]);
> -       nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> -                                                     &nodes[SWNODE_SENSOR_PORT],
> -                                                     sensor->ep_properties);
> -       nodes[SWNODE_CSI2_PORT] = NODE_PORT(sensor->node_names.remote_port,
> -                                           &bridge->csi2_node);
> -       nodes[SWNODE_CSI2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
> -                                                   &nodes[SWNODE_CSI2_PORT],
> -                                                   sensor->csi2_properties);
> -
> -       atomisp_csi2_init_swnode_group(sensor);
> -}
> -
> -static void atomisp_csi2_unregister_sensors(struct atomisp_csi2_bridge *bridge)
> -{
> -       struct atomisp_csi2_sensor *sensor;
> -       unsigned int i;
> -
> -       for (i = 0; i < bridge->n_sensors; i++) {
> -               sensor = &bridge->sensors[i];
> -               software_node_unregister_node_group(sensor->group);
> -               acpi_dev_remove_driver_gpios(sensor->adev);
> -               acpi_dev_put(sensor->adev);
> +       sensor->link = atomisp_csi2_get_port(adev, clock_num);
> +       if (sensor->link >= ATOMISP_CAMERA_NR_PORTS) {
> +               dev_err(&adev->dev, "Invalid port: %u\n", sensor->link);
> +               return -EINVAL;
>         }
> -}
>
> -static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *cfg,
> -                                      struct atomisp_csi2_bridge *bridge,
> -                                      struct atomisp_device *isp)
> -{
> -       struct fwnode_handle *fwnode, *primary;
> -       struct atomisp_csi2_sensor *sensor;
> -       struct acpi_device *adev;
> -       int ret, clock_num;
> +       if (!acpi_match_device_ids(adev, dual_lane_sensors))
> +               lanes = 2;
> +       else
> +               lanes = 1;

Can we use positive conditional?

       if (acpi_match_device_ids(adev, dual_lane_sensors))
               lanes = 1;
       else
               lanes = 2;

> -       for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> -               if (!adev->status.enabled)
> -                       continue;
> -
> -               if (bridge->n_sensors >= ATOMISP_CAMERA_NR_PORTS) {
> -                       dev_err(isp->dev, "Exceeded available CSI2 ports\n");
> -                       ret = -EOVERFLOW;
> -                       goto err_put_adev;
> -               }
> -
> -               sensor = &bridge->sensors[bridge->n_sensors];
> -
> -               /*
> -                * ACPI takes care of turning the PMC clock on and off, but on BYT
> -                * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
> -                * Get the PMC-clock number from ACPI _PR0 method and set it to 19.2 MHz.
> -                * The PMC-clock number is also used to determine the default CSI port.
> -                */
> -               clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
> -
> -               ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
> -               if (ret)
> -                       goto err_put_adev;
> -
> -               sensor->port = atomisp_csi2_get_port(adev, clock_num);
> -               if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
> -                       acpi_handle_err(adev->handle, "Invalid port: %d\n", sensor->port);
> -                       ret = -EINVAL;
> -                       goto err_put_adev;
> -               }
> -
> -               sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", cfg->lanes);
> -               if (sensor->lanes > CSI2_MAX_LANES) {
> -                       acpi_handle_err(adev->handle, "Invalid number of lanes: %d\n", sensor->lanes);
> -                       ret = -EINVAL;
> -                       goto err_put_adev;
> -               }
> -
> -               ret = atomisp_csi2_add_gpio_mappings(sensor, adev);
> -               if (ret)
> -                       goto err_put_adev;
> -
> -               snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> -                        cfg->hid, sensor->port);
> -
> -               atomisp_csi2_create_fwnode_properties(sensor, bridge, cfg);
> -               atomisp_csi2_create_connection_swnodes(bridge, sensor);
> -
> -               ret = software_node_register_node_group(sensor->group);
> -               if (ret)
> -                       goto err_remove_mappings;
> -
> -               fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR]);
> -               if (!fwnode) {
> -                       ret = -ENODEV;
> -                       goto err_free_swnodes;
> -               }
> -
> -               sensor->adev = acpi_dev_get(adev);
> -
> -               primary = acpi_fwnode_handle(adev);
> -               primary->secondary = fwnode;
> -
> -               bridge->n_sensors++;
> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
> +       if (sensor->lanes > IPU_MAX_LANES) {
> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);

Yeah, I think we would be consistent in using the ACPI handle to print
the messages from ACPI sensor devices.

> +               return -EINVAL;
>         }
>
> +       ret = atomisp_csi2_add_gpio_mappings(adev);
> +       if (ret)
> +               return ret;
> +
> +       sensor->mclkspeed = PMC_CLK_RATE_19_2MHZ;
> +       sensor->rotation = 0;
> +       sensor->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +
>         return 0;
> -
> -err_free_swnodes:
> -       software_node_unregister_node_group(sensor->group);
> -err_remove_mappings:
> -       acpi_dev_remove_driver_gpios(adev);
> -err_put_adev:
> -       acpi_dev_put(adev);
> -       return ret;
> -}
> -
> -static int atomisp_csi2_connect_sensors(struct atomisp_csi2_bridge *bridge,
> -                                       struct atomisp_device *isp)
> -{
> -       unsigned int i;
> -       int ret;
> -
> -       for (i = 0; i < ARRAY_SIZE(supported_sensors); i++) {
> -               const struct atomisp_csi2_sensor_config *cfg = &supported_sensors[i];
> -
> -               ret = atomisp_csi2_connect_sensor(cfg, bridge, isp);
> -               if (ret)
> -                       goto err_unregister_sensors;
> -       }
> -
> -       return 0;
> -
> -err_unregister_sensors:
> -       atomisp_csi2_unregister_sensors(bridge);
> -       return ret;
>  }
>
>  int atomisp_csi2_bridge_init(struct atomisp_device *isp)
>  {
> -       struct atomisp_csi2_bridge *bridge;
>         struct device *dev = isp->dev;
>         struct fwnode_handle *fwnode;
> -       int i, ret;
>
>         /*
>          * This function is intended to run only once and then leave
> @@ -716,52 +525,7 @@ int atomisp_csi2_bridge_init(struct atomisp_device *isp)
>         if (fwnode && fwnode->secondary)
>                 return 0;
>
> -       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> -       if (!bridge)
> -               return -ENOMEM;
> -
> -       strscpy(bridge->csi2_node_name, "atomisp-csi2", sizeof(bridge->csi2_node_name));
> -       bridge->csi2_node.name = bridge->csi2_node_name;
> -
> -       ret = software_node_register(&bridge->csi2_node);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to register the CSI2 HID node\n");
> -               goto err_free_bridge;
> -       }
> -
> -       /*
> -        * Map the lane arrangement, which is fixed for the ISP2 (meaning we
> -        * only need one, rather than one per sensor). We include it as a
> -        * member of the bridge struct rather than a global variable so
> -        * that it survives if the module is unloaded along with the rest of
> -        * the struct.
> -        */
> -       for (i = 0; i < CSI2_MAX_LANES; i++)
> -               bridge->data_lanes[i] = i + 1;
> -
> -       ret = atomisp_csi2_connect_sensors(bridge, isp);
> -       if (ret || bridge->n_sensors == 0)
> -               goto err_unregister_csi2;
> -
> -       fwnode = software_node_fwnode(&bridge->csi2_node);
> -       if (!fwnode) {
> -               dev_err(dev, "Error getting fwnode from csi2 software_node\n");
> -               ret = -ENODEV;
> -               goto err_unregister_sensors;
> -       }
> -
> -       set_secondary_fwnode(dev, fwnode);
> -
> -       return 0;
> -
> -err_unregister_sensors:
> -       atomisp_csi2_unregister_sensors(bridge);
> -err_unregister_csi2:
> -       software_node_unregister(&bridge->csi2_node);
> -err_free_bridge:
> -       kfree(bridge);
> -
> -       return ret;
> +       return ipu_bridge_init(dev, atomisp_csi2_parse_sensor_fwnode);
>  }
>
>  /******* V4L2 sub-device asynchronous registration callbacks***********/
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index c43b916a006e..0d80f0893a2e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -1615,3 +1615,4 @@ MODULE_AUTHOR("Wen Wang <wen.w.wang@intel.com>");
>  MODULE_AUTHOR("Xiaolin Zhang <xiaolin.zhang@intel.com>");
>  MODULE_LICENSE("GPL");
>  MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver");
> +MODULE_IMPORT_NS(INTEL_IPU_BRIDGE);
> --
> 2.41.0
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> In most cases when a VCM is used there is a single integrated module
> with the sensor + VCM + lens. This means that the sensor and VCM often
> share regulators and possibly also something like a powerdown pin.
>
> In the ACPI tables this is modelled as a single ACPI device with
> multiple I2cSerialBus resources.
>
> On atomisp devices the regulators and clks are modelled as ACPI
> power-resources, which are controlled by the (ACPI) power state
> of the sensor. So the sensor must be in D0 power state for the VCM
> to work.
>
> To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
> so that the sensor will automatically be runtime-resumed whenever
> the VCM is runtime-resumed.
>
> This requires the probing of the VCM and thus the creation of the VCM
> I2C-client to be delayed till after the sensor driver has bound.
>
> Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
> bound op, so that it is done after the sensor driver has bound; and
> add code to add the device-link.
>
> This fixes the problem with the shared ACPI power-resources on atomisp2
> and this avoids the need for VCM related workarounds on IPU3 / IPU6.
>
> E.g. until now the dw9719 driver needed to get and control a Vsio
> (V sensor IO) regulator since that needs to be enabled to enable I2C
> pass-through on the PMIC on the sensor module. So the driver was
> controlling this regulator even though the actual dw9719 chip has no
> Vsio pin / power-plane.
>
> This also removes the need for ipu_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.

...

> +static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
> +{
> +       struct ipu_bridge_instantiate_vcm_work_data *work =
> +               container_of(_work,
> +                            struct ipu_bridge_instantiate_vcm_work_data,
> +                            work);

Just noticed this plenty of *work.

Perhaps call the parameter work and the stack variable wdata or so?

>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Dan Scally]

Hi Hans

On 30/06/2023 13:06, Hans de Goede wrote:
> The GalaxyCore GC0310 is used together with the atomisp no various


s/no/on

> devices, add it to ipu_supported_sensors[].
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   drivers/media/pci/intel/ipu-bridge.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index eb7c56e8ef9f..07a34f20af8e 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>   	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>   	/* Omnivision ov13b10 */
>   	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> +	/* GalaxyCore GC0310 */
> +	IPU_SENSOR_CONFIG("INT0310", 0),
>   };
>   
>   static const struct ipu_property_names prop_names = {

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Sakari Ailus]

Hi Hans,

On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
> The GalaxyCore GC0310 is used together with the atomisp no various
> devices, add it to ipu_supported_sensors[].
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index eb7c56e8ef9f..07a34f20af8e 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>  	/* Omnivision ov13b10 */
>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> +	/* GalaxyCore GC0310 */
> +	IPU_SENSOR_CONFIG("INT0310", 0),


Where is this HID from? A DSDT somewhere??

>  };
>  
>  static const struct ipu_property_names prop_names = {

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

Hi Sakari,

On 7/4/23 12:49, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
>> The GalaxyCore GC0310 is used together with the atomisp no various
>> devices, add it to ipu_supported_sensors[].
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>> index eb7c56e8ef9f..07a34f20af8e 100644
>> --- a/drivers/media/pci/intel/ipu-bridge.c
>> +++ b/drivers/media/pci/intel/ipu-bridge.c
>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>>  	/* Omnivision ov13b10 */
>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
>> +	/* GalaxyCore GC0310 */
>> +	IPU_SENSOR_CONFIG("INT0310", 0),
> 
> 
> Where is this HID from? A DSDT somewhere??

This is used in many atomisp DSDTs on many atomisp devices,
see below for a quick grep to my no disk "database" of DSDTs.

This is also already used by the existing gc0310 driver for
atomisp:

https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c

Note with the latest media_stage.git branch, which has
my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:

https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/

The gc0310 driver is now a fully standard v4l2 driver no longer using any
atomisp-isms / custom API. As such I have a patch pending to move
it to drivers/media/i2c :

https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/

that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
yourself, this should be applied on top of the latest media_stage/master
not on top of 6.4-rc1 .

Regards,

Hans

p.s.

The promised list of matching DSDT-s. Note not all of these necessary actually
use the sensor (but some do) let me know if you want a DSDT of a device which
actually uses the GC0310 :

[hans@shalem ~]$ grep -l INT0310 hwinfo/*/dsdt.dsl
hwinfo/CAT-T20-tablet/dsdt.dsl
hwinfo/acer-One-S1003/dsdt.dsl
hwinfo/acer-s1003-2/dsdt.dsl
hwinfo/chuwi-hi8-Q32G22160505024/dsdt.dsl
hwinfo/chuwi-hi8/dsdt.dsl
hwinfo/chuwi-vi8-cwi501/dsdt.dsl
hwinfo/chuwi-vi8/dsdt.dsl
hwinfo/cube-iwork8-air/dsdt.dsl
hwinfo/cyberbook-t116/dsdt.dsl
hwinfo/endless-HKC/dsdt.dsl
hwinfo/estar-beauty-hd-mid7316r/dsdt.dsl
hwinfo/gp-electronics-7inch-win10/dsdt.dsl
hwinfo/hp-stream7/dsdt.dsl
hwinfo/insignia-flex8-NS-P08W7100/dsdt.dsl
hwinfo/irbis-nb41/dsdt.dsl
hwinfo/irbis-tw36/dsdt.dsl
hwinfo/irbis-tw90/dsdt.dsl
hwinfo/itworks-tw891/dsdt.dsl
hwinfo/jumper-ezbook2/dsdt.dsl
hwinfo/lamina-i8270/dsdt.dsl
hwinfo/lenovo-ideapad-miix-310/dsdt.dsl
hwinfo/lenovo-ideapad-miix-320/dsdt.dsl
hwinfo/medion-akoya-e2212t/dsdt.dsl
hwinfo/medion-e2215t/dsdt.dsl
hwinfo/medion-e2228t/dsdt.dsl
hwinfo/mele-pcg03/dsdt.dsl
hwinfo/mele-pcg09/dsdt.dsl
hwinfo/minix-neo-z83-4-xxx/dsdt.dsl
hwinfo/minix-neo-z83-4-hans-de-goede/dsdt.dsl
hwinfo/minix-neo-z83-4-unknown/dsdt.dsl
hwinfo/mpman-converter9/dsdt.dsl
hwinfo/mpman-mpwin895cl/dsdt.dsl
hwinfo/nextbook-ares-8a/dsdt.dsl
hwinfo/nuvision-TM800W560L/dsdt.dsl
hwinfo/odys-vario-pro12/dsdt.dsl
hwinfo/onda-v891w-ONDA.D890HBBNR0A/dsdt.dsl
hwinfo/onda-v891w-ONDA.W89EBBN08/dsdt.dsl
hwinfo/pipe-w2s/dsdt.dsl
hwinfo/pipe-w4/dsdt.dsl
hwinfo/ployer-momo7w/dsdt.dsl
hwinfo/pov-mobii-tab-p1005w-232/dsdt.dsl
hwinfo/pov-mobii-tab-p1006w-232-3g/dsdt.dsl
hwinfo/rca-cambio-w101-v2/dsdt.dsl
hwinfo/schneider-SCL141CTP/dsdt.dsl
hwinfo/trekstor-surftab-wintron-7.0/dsdt.dsl
hwinfo/trekstor-twin-10.1/dsdt.dsl
hwinfo/umax-visionbook-10wi/dsdt.dsl
hwinfo/yours-y8w81/dsdt.dsl
[hans@shalem ~]$ 

Re: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Dan Scally]

On 30/06/2023 16:23, Andy Shevchenko wrote:
> On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
>> sensor->adev is not set yet.
>>
>> So if either of the dev_warn() calls about unknown values are hit this
>> will lead to a NULL pointer deref.
>>
>> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
>> on errors harder, to fix this.
> TBH, I don't like this approach, it seems a bit dirty to me.
>
> First of all, why do we need pci_dev to be a parameter in this function?
> Second, why don't we consistently use the ACPI handle (with respective
> acpi_handle_*() macros to print messages)?
>
> So, my proposal here is to actually save the ACPI device handle in the
> sensor object and use it for the messaging. It makes it consistent and
> doesn't require to rewrite adev field which seems the dirty part to
> me.


It's a bit finicky but I don't think it's so bad; the refcounting is all fine, the later 
acpi_dev_get() is only to hold a reference once the next loop iteration frees the existing one and 
the rewrite should store the exact same pointer...we could just not store the result of the 
acpi_dev_get() call to avoid that weird rewrite perhaps?

>
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 02/15] media: ipu-bridge: Do not use on stack memory for software_node.name field

[Dan Scally]

Hi Hans

On 30/06/2023 13:06, Hans de Goede wrote:
> Commit 567f97bd381f ("media: ipu3-cio2: support multiple sensors and VCMs
> with same HID") introduced an on stack vcm_name and then uses this for
> the name field of the software_node struct used for the vcm.
>
> But the software_node struct is much longer lived then the current
> stack-frame, so this is no good.
>
> Instead extend the ipu_node_names struct with an extra field to store
> the vcm software_node name and use that.
>
> Note this also changes the length of the allocated buffer from
> ACPI_ID_LEN + 4 to 16. the name is filled with "<ipu_vcm_types[x]>-%u"
> where ipu_vcm_types[x] is not an ACPI_ID. The maximum length of
> the strings in the ipu_vcm_types[] array is 11 + 5 bytes for "-255\0"
> means 16 bytes are needed in the worst case scenario.
>
> Fixes: 567f97bd381f ("media: ipu3-cio2: support multiple sensors and VCMs with same HID")
> Cc: Bingbu Cao <bingbu.cao@intel.com>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/media/pci/intel/ipu-bridge.c | 7 +++----
>   drivers/media/pci/intel/ipu-bridge.h | 1 +
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index f0927f80184d..ef6c6cb7b51b 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -220,7 +220,6 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>   						 struct ipu_sensor *sensor)
>   {
>   	struct software_node *nodes = sensor->swnodes;
> -	char vcm_name[ACPI_ID_LEN + 4];
>   
>   	ipu_bridge_init_swnode_names(sensor);
>   
> @@ -240,10 +239,10 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>   						sensor->ipu_properties);
>   	if (sensor->ssdb.vcmtype) {
>   		/* append ssdb.link to distinguish VCM nodes with same HID */
> -		snprintf(vcm_name, sizeof(vcm_name), "%s-%u",
> -			 ipu_vcm_types[sensor->ssdb.vcmtype - 1],
> +		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
> +			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
>   			 sensor->ssdb.link);
> -		nodes[SWNODE_VCM] = NODE_VCM(vcm_name);
> +		nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
>   	}
>   
>   	ipu_bridge_init_swnode_group(sensor);
> diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
> index 8cb733c03e2f..6cce712a0f34 100644
> --- a/drivers/media/pci/intel/ipu-bridge.h
> +++ b/drivers/media/pci/intel/ipu-bridge.h
> @@ -103,6 +103,7 @@ struct ipu_node_names {
>   	char port[7];
>   	char endpoint[11];
>   	char remote_port[7];
> +	char vcm[16];
>   };
>   
>   struct ipu_sensor_config {

Re: [PATCH v2 03/15] media: ipu-bridge: Move initialization of node_names.vcm to ipu_bridge_init_swnode_names()

[Dan Scally]

On 30/06/2023 13:06, Hans de Goede wrote:
> Move initialization of node_names.vcm to ipu_bridge_init_swnode_names()
> where it belongs.
>
> And make the initialization of nodes[SWNODE_VCM] unconditional,
> ipu_bridge_init_swnode_group() takes care of not registering it
> when there is no VCM.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>   drivers/media/pci/intel/ipu-bridge.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index ef6c6cb7b51b..1c88fd925a8b 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -201,6 +201,12 @@ static void ipu_bridge_init_swnode_names(struct ipu_sensor *sensor)
>   	snprintf(sensor->node_names.endpoint,
>   		 sizeof(sensor->node_names.endpoint),
>   		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
> +	if (sensor->ssdb.vcmtype) {
> +		/* append ssdb.link to distinguish nodes with same model VCM */
> +		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
> +			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
> +			 sensor->ssdb.link);
> +	}
>   }
>   
>   static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
> @@ -237,13 +243,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>   						sensor->node_names.endpoint,
>   						&nodes[SWNODE_IPU_PORT],
>   						sensor->ipu_properties);
> -	if (sensor->ssdb.vcmtype) {
> -		/* append ssdb.link to distinguish VCM nodes with same HID */
> -		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
> -			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
> -			 sensor->ssdb.link);
> -		nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
> -	}
> +	nodes[SWNODE_VCM] = NODE_VCM(sensor->node_names.vcm);
>   
>   	ipu_bridge_init_swnode_group(sensor);
>   }

Re: [PATCH v2 04/15] media: ipu-bridge: Allow building as module

[Hans de Goede]

Hi,

On 6/30/23 16:08, Andy Shevchenko wrote:
> On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> After commit f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu
>> bridge and move out of ipu3") the ipu-bridge code is always builtin
> 
> built in
> 
>> even if all (its only) consumers are build as modules.
> 
> I'm not sure I understood the remark in parentheses correctly. All of
> its consumers are the only consumers of this module?

There is only 1 consumer so both all of its consumers and its only
consumer, I'll drop the confusing "(its only)" part.

> 
>> Fix this by turning "config IPU_BRIDGE" into a pure library Kconfig
>> option (not user selectable, must be selected by consumers) and
>> re-introducing the CIO2_BRIDGE Kconfig bits in .../pci/intel/ipu3/Kconfig
>> which were dropped to still allow building ipu3-cio2 without ipu-bridge
>> support.
> 
> ...
> 
>>  config IPU_BRIDGE
>> -       bool "Intel IPU Sensors Bridge"
>> -       depends on VIDEO_IPU3_CIO2 && ACPI
>> -       depends on I2C
>> -       help
>> -         This extension provides an API for the Intel IPU driver to create
>> -         connections to cameras that are hidden in the SSDB buffer in ACPI.
>> -         It can be used to enable support for cameras in detachable / hybrid
>> -         devices that ship with Windows.
>> -
>> -         Say Y here if your device is a detachable / hybrid laptop that comes
>> -         with Windows installed by the OEM, for example:
>> -
>> -               - Microsoft Surface models (except Surface Pro 3)
>> -               - The Lenovo Miix line (for example the 510, 520, 710 and 720)
>> -               - Dell 7285
>> -
>> -         If in doubt, say N here.
>> +       tristate
>> +       depends on ACPI && I2C
> 
> Can we leave
> 
>   depends on I2C
> 
> line untouched?

Sure, I'll change that for v3.

Regards,

Hans

Re: [PATCH v2 09/15] media: ipu-bridge: Drop early setting of sensor->adev

[Hans de Goede]

Hi,

On 6/30/23 16:30, Andy Shevchenko wrote:
> On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> sensor->adev is no longer dereferenced before it is permanently set by:
>>
>>         sensor->adev = acpi_dev_get(adev);
>>
>> So the early assignment with a borrowed reference can be dropped.
> 
> Ah, now I see that that change was temporary.

Ack

> Can we avoid backporting
> it please?

Patch 1/15 does fix a NULL pointer deref oops, so backporting it would
be a good thing to do IMHO.

Regards,

Hans

Re: [PATCH v2 07/15] media: ipu-bridge: Only keep PLD around while parsing

[Dan Scally]

Hi Hans

On 30/06/2023 13:06, Hans de Goede wrote:
> There is no need to keep a reference to the PLD struct around,
> it is only used once the get the sensor orientation.
>
> Make ipu_bridge_parse_orientation() also get + put the PLD.
>
> This is a preparation patch for making the ipu-bridge code more generic
> so that it can be shared with the atomisp driver.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/media/pci/intel/ipu-bridge.c | 48 ++++++++++++++++------------
>   drivers/media/pci/intel/ipu-bridge.h |  1 -
>   2 files changed, 27 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 8e91d9b3e0fe..3a984d688b42 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -112,23 +112,39 @@ static u32 ipu_bridge_parse_rotation(struct ipu_sensor *sensor)
>   	}
>   }
>   
> -static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct ipu_sensor *sensor)
> +static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_device *adev)
>   {
> -	switch (sensor->pld->panel) {
> +	enum v4l2_fwnode_orientation orientation;
> +	struct acpi_pld_info *pld;
> +	acpi_status status;
> +
> +	status = acpi_get_physical_device_location(adev->handle, &pld);
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(&adev->dev, "_PLD call failed using unknown orientation\n");


I think some punctuation after "_PLD call failed", and I'd probably say "default" rather than "unknown".


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

> +		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +	}
> +
> +	switch (pld->panel) {
>   	case ACPI_PLD_PANEL_FRONT:
> -		return V4L2_FWNODE_ORIENTATION_FRONT;
> +		orientation = V4L2_FWNODE_ORIENTATION_FRONT;
> +		break;
>   	case ACPI_PLD_PANEL_BACK:
> -		return V4L2_FWNODE_ORIENTATION_BACK;
> +		orientation = V4L2_FWNODE_ORIENTATION_BACK;
> +		break;
>   	case ACPI_PLD_PANEL_TOP:
>   	case ACPI_PLD_PANEL_LEFT:
>   	case ACPI_PLD_PANEL_RIGHT:
>   	case ACPI_PLD_PANEL_UNKNOWN:
> -		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +		break;
>   	default:
> -		dev_warn(&sensor->adev->dev, "Unknown _PLD panel value %d\n",
> -			 sensor->pld->panel);
> -		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +		dev_warn(&adev->dev, "Unknown _PLD panel val %d\n", pld->panel);
> +		orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
> +		break;
>   	}
> +
> +	ACPI_FREE(pld);
> +	return orientation;
>   }
>   
>   static void ipu_bridge_create_fwnode_properties(
> @@ -140,7 +156,7 @@ static void ipu_bridge_create_fwnode_properties(
>   	enum v4l2_fwnode_orientation orientation;
>   
>   	rotation = ipu_bridge_parse_rotation(sensor);
> -	orientation = ipu_bridge_parse_orientation(sensor);
> +	orientation = ipu_bridge_parse_orientation(sensor->adev);
>   
>   	sensor->prop_names = prop_names;
>   
> @@ -279,7 +295,6 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   	for (i = 0; i < bridge->n_sensors; i++) {
>   		sensor = &bridge->sensors[i];
>   		software_node_unregister_node_group(sensor->group);
> -		ACPI_FREE(sensor->pld);
>   		acpi_dev_put(sensor->adev);
>   		i2c_unregister_device(sensor->vcm_i2c_client);
>   	}
> @@ -291,7 +306,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   	struct fwnode_handle *fwnode, *primary;
>   	struct ipu_sensor *sensor;
>   	struct acpi_device *adev;
> -	acpi_status status;
>   	int ret;
>   
>   	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> @@ -326,17 +340,11 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   			sensor->ssdb.vcmtype = 0;
>   		}
>   
> -		status = acpi_get_physical_device_location(adev->handle, &sensor->pld);
> -		if (ACPI_FAILURE(status)) {
> -			ret = -ENODEV;
> -			goto err_put_adev;
> -		}
> -
>   		if (sensor->ssdb.lanes > IPU_MAX_LANES) {
>   			dev_err(&adev->dev,
>   				"Number of lanes in SSDB is invalid\n");
>   			ret = -EINVAL;
> -			goto err_free_pld;
> +			goto err_put_adev;
>   		}
>   
>   		ipu_bridge_create_fwnode_properties(sensor, bridge, cfg);
> @@ -344,7 +352,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   
>   		ret = software_node_register_node_group(sensor->group);
>   		if (ret)
> -			goto err_free_pld;
> +			goto err_put_adev;
>   
>   		fwnode = software_node_fwnode(&sensor->swnodes[
>   						      SWNODE_SENSOR_HID]);
> @@ -370,8 +378,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   
>   err_free_swnodes:
>   	software_node_unregister_node_group(sensor->group);
> -err_free_pld:
> -	ACPI_FREE(sensor->pld);
>   err_put_adev:
>   	acpi_dev_put(adev);
>   	return ret;
> diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
> index 6cb68e3344dc..907ca833a7c1 100644
> --- a/drivers/media/pci/intel/ipu-bridge.h
> +++ b/drivers/media/pci/intel/ipu-bridge.h
> @@ -124,7 +124,6 @@ struct ipu_sensor {
>   	struct ipu_node_names node_names;
>   
>   	struct ipu_sensor_ssdb ssdb;
> -	struct acpi_pld_info *pld;
>   
>   	struct ipu_property_names prop_names;
>   	struct property_entry ep_properties[5];

Re: [PATCH v2 08/15] media: ipu-bridge: Add a ipu_bridge_parse_ssdb() helper function

[Dan Scally]

On 30/06/2023 13:06, Hans de Goede wrote:
> The code to go from ACPI sensor info to a fwnode-tree with connector
> nodes and endpoint properties is 99% the same for the atomisp2 and
> the IPU3.
>
> The main difference is that atomisp2 devices do not have a SSDB table
> with various info.
>
> Abstract out the parsing of the sensor's ACPI fwnode into a helper
> function and store the parsed results, rather then the raw SSDB
> in struct ipu_sensor.
>
> This is a preparation patch for making the ipu-bridge code more generic
> so that it can be shared with the atomisp driver.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/media/pci/intel/ipu-bridge.c | 96 +++++++++++++++-------------
>   drivers/media/pci/intel/ipu-bridge.h |  8 ++-
>   2 files changed, 59 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 3a984d688b42..daa9dde78c64 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -97,17 +97,18 @@ static int ipu_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
>   	return ret;
>   }
>   
> -static u32 ipu_bridge_parse_rotation(struct ipu_sensor *sensor)
> +static u32 ipu_bridge_parse_rotation(struct acpi_device *adev,
> +				     struct ipu_sensor_ssdb *ssdb)
>   {
> -	switch (sensor->ssdb.degree) {
> +	switch (ssdb->degree) {
>   	case IPU_SENSOR_ROTATION_NORMAL:
>   		return 0;
>   	case IPU_SENSOR_ROTATION_INVERTED:
>   		return 180;
>   	default:
> -		dev_warn(&sensor->adev->dev,
> +		dev_warn(&adev->dev,
>   			 "Unknown rotation %d. Assume 0 degree rotation\n",
> -			 sensor->ssdb.degree);
> +			 ssdb->degree);
>   		return 0;
>   	}
>   }
> @@ -147,17 +148,43 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
>   	return orientation;
>   }
>   
> +static int ipu_bridge_parse_ssdb(struct acpi_device *adev,
> +				 struct ipu_sensor *sensor)
> +{
> +	struct ipu_sensor_ssdb ssdb = {};
> +	int ret;
> +
> +	ret = ipu_bridge_read_acpi_buffer(adev, "SSDB", &ssdb, sizeof(ssdb));
> +	if (ret)
> +		return ret;
> +
> +	if (ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
> +		dev_warn(&adev->dev, "Unknown VCM type %d\n", ssdb.vcmtype);
> +		ssdb.vcmtype = 0;
> +	}
> +
> +	if (ssdb.lanes > IPU_MAX_LANES) {
> +		dev_err(&adev->dev, "Number of lanes in SSDB is invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	sensor->link = ssdb.link;
> +	sensor->lanes = ssdb.lanes;
> +	sensor->mclkspeed = ssdb.mclkspeed;
> +	sensor->rotation = ipu_bridge_parse_rotation(adev, &ssdb);
> +	sensor->orientation = ipu_bridge_parse_orientation(adev);
> +
> +	if (ssdb.vcmtype)
> +		sensor->vcm_type = ipu_vcm_types[ssdb.vcmtype - 1];
> +
> +	return 0;
> +}
> +
>   static void ipu_bridge_create_fwnode_properties(
>   	struct ipu_sensor *sensor,
>   	struct ipu_bridge *bridge,
>   	const struct ipu_sensor_config *cfg)
>   {
> -	u32 rotation;
> -	enum v4l2_fwnode_orientation orientation;
> -
> -	rotation = ipu_bridge_parse_rotation(sensor);
> -	orientation = ipu_bridge_parse_orientation(sensor->adev);
> -
>   	sensor->prop_names = prop_names;
>   
>   	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_IPU_ENDPOINT]);
> @@ -165,14 +192,14 @@ static void ipu_bridge_create_fwnode_properties(
>   
>   	sensor->dev_properties[0] = PROPERTY_ENTRY_U32(
>   					sensor->prop_names.clock_frequency,
> -					sensor->ssdb.mclkspeed);
> +					sensor->mclkspeed);
>   	sensor->dev_properties[1] = PROPERTY_ENTRY_U32(
>   					sensor->prop_names.rotation,
> -					rotation);
> +					sensor->rotation);
>   	sensor->dev_properties[2] = PROPERTY_ENTRY_U32(
>   					sensor->prop_names.orientation,
> -					orientation);
> -	if (sensor->ssdb.vcmtype) {
> +					sensor->orientation);
> +	if (sensor->vcm_type) {
>   		sensor->vcm_ref[0] =
>   			SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_VCM]);
>   		sensor->dev_properties[3] =
> @@ -184,8 +211,7 @@ static void ipu_bridge_create_fwnode_properties(
>   					V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
>   	sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>   					sensor->prop_names.data_lanes,
> -					bridge->data_lanes,
> -					sensor->ssdb.lanes);
> +					bridge->data_lanes, sensor->lanes);
>   	sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(
>   					sensor->prop_names.remote_endpoint,
>   					sensor->local_ref);
> @@ -198,8 +224,7 @@ static void ipu_bridge_create_fwnode_properties(
>   
>   	sensor->ipu_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(
>   					sensor->prop_names.data_lanes,
> -					bridge->data_lanes,
> -					sensor->ssdb.lanes);
> +					bridge->data_lanes, sensor->lanes);
>   	sensor->ipu_properties[1] = PROPERTY_ENTRY_REF_ARRAY(
>   					sensor->prop_names.remote_endpoint,
>   					sensor->remote_ref);
> @@ -209,18 +234,17 @@ static void ipu_bridge_init_swnode_names(struct ipu_sensor *sensor)
>   {
>   	snprintf(sensor->node_names.remote_port,
>   		 sizeof(sensor->node_names.remote_port),
> -		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->ssdb.link);
> +		 SWNODE_GRAPH_PORT_NAME_FMT, sensor->link);
>   	snprintf(sensor->node_names.port,
>   		 sizeof(sensor->node_names.port),
>   		 SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
>   	snprintf(sensor->node_names.endpoint,
>   		 sizeof(sensor->node_names.endpoint),
>   		 SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
> -	if (sensor->ssdb.vcmtype) {
> -		/* append ssdb.link to distinguish nodes with same model VCM */
> +	if (sensor->vcm_type) {
> +		/* append link to distinguish nodes with same model VCM */
>   		snprintf(sensor->node_names.vcm, sizeof(sensor->node_names.vcm),
> -			 "%s-%u", ipu_vcm_types[sensor->ssdb.vcmtype - 1],
> -			 sensor->ssdb.link);
> +			 "%s-%u", sensor->vcm_type, sensor->link);
>   	}
>   }
>   
> @@ -233,7 +257,7 @@ static void ipu_bridge_init_swnode_group(struct ipu_sensor *sensor)
>   	sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
>   	sensor->group[SWNODE_IPU_PORT] = &nodes[SWNODE_IPU_PORT];
>   	sensor->group[SWNODE_IPU_ENDPOINT] = &nodes[SWNODE_IPU_ENDPOINT];
> -	if (sensor->ssdb.vcmtype)
> +	if (sensor->vcm_type)
>   		sensor->group[SWNODE_VCM] =  &nodes[SWNODE_VCM];
>   }
>   
> @@ -268,13 +292,12 @@ static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
>   	struct i2c_board_info board_info = { };
>   	char name[16];
>   
> -	if (!sensor->ssdb.vcmtype)
> +	if (!sensor->vcm_type)
>   		return;
>   
>   	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
>   	board_info.dev_name = name;
> -	strscpy(board_info.type, ipu_vcm_types[sensor->ssdb.vcmtype - 1],
> -		ARRAY_SIZE(board_info.type));
> +	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
>   	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
>   
>   	sensor->vcm_i2c_client =
> @@ -325,27 +348,12 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   		 */
>   		sensor->adev = adev;
>   
> -		ret = ipu_bridge_read_acpi_buffer(adev, "SSDB",
> -						  &sensor->ssdb,
> -						  sizeof(sensor->ssdb));
> +		ret = ipu_bridge_parse_ssdb(adev, sensor);
>   		if (ret)
>   			goto err_put_adev;
>   
>   		snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
> -			 cfg->hid, sensor->ssdb.link);
> -
> -		if (sensor->ssdb.vcmtype > ARRAY_SIZE(ipu_vcm_types)) {
> -			dev_warn(&adev->dev, "Unknown VCM type %d\n",
> -				 sensor->ssdb.vcmtype);
> -			sensor->ssdb.vcmtype = 0;
> -		}
> -
> -		if (sensor->ssdb.lanes > IPU_MAX_LANES) {
> -			dev_err(&adev->dev,
> -				"Number of lanes in SSDB is invalid\n");
> -			ret = -EINVAL;
> -			goto err_put_adev;
> -		}
> +			 cfg->hid, sensor->link);
>   
>   		ipu_bridge_create_fwnode_properties(sensor, bridge, cfg);
>   		ipu_bridge_create_connection_swnodes(bridge, sensor);
> diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
> index 907ca833a7c1..a8b89c4b95bc 100644
> --- a/drivers/media/pci/intel/ipu-bridge.h
> +++ b/drivers/media/pci/intel/ipu-bridge.h
> @@ -5,6 +5,7 @@
>   
>   #include <linux/property.h>
>   #include <linux/types.h>
> +#include <media/v4l2-fwnode.h>
>   
>   struct i2c_client;
>   
> @@ -123,7 +124,12 @@ struct ipu_sensor {
>   	struct software_node swnodes[SWNODE_COUNT];
>   	struct ipu_node_names node_names;
>   
> -	struct ipu_sensor_ssdb ssdb;
> +	u8 link;
> +	u8 lanes;
> +	u32 mclkspeed;
> +	u32 rotation;
> +	enum v4l2_fwnode_orientation orientation;
> +	const char *vcm_type;
>   
>   	struct ipu_property_names prop_names;
>   	struct property_entry ep_properties[5];

Re: [PATCH v2 05/15] media: ipu-bridge: Make ipu_bridge_init() take a regular struct device as argument

[Dan Scally]

On 30/06/2023 13:06, Hans de Goede wrote:
> Make ipu_bridge_init() take a regular struct device, rather then
> a pci_dev as argument.
>
> This is a preparation patch for making the ipu-bridge code more generic
> so that it can be shared with the atomisp driver.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>


And same for 6/15.

>   drivers/media/pci/intel/ipu-bridge.c     | 16 +++++++---------
>   drivers/media/pci/intel/ipu-bridge.h     |  4 ++--
>   drivers/media/pci/intel/ipu3/ipu3-cio2.c |  2 +-
>   3 files changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 97b544736af2..9027a8d2d176 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -4,7 +4,6 @@
>   #include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/i2c.h>
> -#include <linux/pci.h>
>   #include <linux/property.h>
>   #include <media/v4l2-fwnode.h>
>   
> @@ -288,7 +287,7 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   
>   static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   				     struct ipu_bridge *bridge,
> -				     struct pci_dev *ipu)
> +				     struct device *dev)
>   {
>   	struct fwnode_handle *fwnode, *primary;
>   	struct ipu_sensor *sensor;
> @@ -302,7 +301,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   
>   		if (bridge->n_sensors >= IPU_MAX_PORTS) {
>   			acpi_dev_put(adev);
> -			dev_err(&ipu->dev, "Exceeded available IPU ports\n");
> +			dev_err(dev, "Exceeded available IPU ports\n");
>   			return -EINVAL;
>   		}
>   
> @@ -362,7 +361,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   
>   		ipu_bridge_instantiate_vcm_i2c_client(sensor);
>   
> -		dev_info(&ipu->dev, "Found supported sensor %s\n",
> +		dev_info(dev, "Found supported sensor %s\n",
>   			 acpi_dev_name(adev));
>   
>   		bridge->n_sensors++;
> @@ -380,7 +379,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   }
>   
>   static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
> -				      struct pci_dev *ipu)
> +				      struct device *dev)
>   {
>   	unsigned int i;
>   	int ret;
> @@ -389,7 +388,7 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge,
>   		const struct ipu_sensor_config *cfg =
>   			&ipu_supported_sensors[i];
>   
> -		ret = ipu_bridge_connect_sensor(cfg, bridge, ipu);
> +		ret = ipu_bridge_connect_sensor(cfg, bridge, dev);
>   		if (ret)
>   			goto err_unregister_sensors;
>   	}
> @@ -435,9 +434,8 @@ static int ipu_bridge_sensors_are_ready(void)
>   	return ready;
>   }
>   
> -int ipu_bridge_init(struct pci_dev *ipu)
> +int ipu_bridge_init(struct device *dev)
>   {
> -	struct device *dev = &ipu->dev;
>   	struct fwnode_handle *fwnode;
>   	struct ipu_bridge *bridge;
>   	unsigned int i;
> @@ -470,7 +468,7 @@ int ipu_bridge_init(struct pci_dev *ipu)
>   	for (i = 0; i < IPU_MAX_LANES; i++)
>   		bridge->data_lanes[i] = i + 1;
>   
> -	ret = ipu_bridge_connect_sensors(bridge, ipu);
> +	ret = ipu_bridge_connect_sensors(bridge, dev);
>   	if (ret || bridge->n_sensors == 0)
>   		goto err_unregister_ipu;
>   
> diff --git a/drivers/media/pci/intel/ipu-bridge.h b/drivers/media/pci/intel/ipu-bridge.h
> index 6cce712a0f34..8c1437f252d2 100644
> --- a/drivers/media/pci/intel/ipu-bridge.h
> +++ b/drivers/media/pci/intel/ipu-bridge.h
> @@ -144,9 +144,9 @@ struct ipu_bridge {
>   };
>   
>   #if IS_ENABLED(CONFIG_IPU_BRIDGE)
> -int ipu_bridge_init(struct pci_dev *ipu);
> +int ipu_bridge_init(struct device *dev);
>   #else
> -static inline int ipu_bridge_init(struct pci_dev *ipu) { return 0; }
> +static inline int ipu_bridge_init(struct device *dev) { return 0; }
>   #endif
>   
>   #endif
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index dc09fbdb062b..4068fa0a5ecf 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1725,7 +1725,7 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
>   			return -EINVAL;
>   		}
>   
> -		r = ipu_bridge_init(pci_dev);
> +		r = ipu_bridge_init(dev);
>   		if (r)
>   			return r;
>   	}

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Dan Scally]

Hi Both

On 30/06/2023 16:45, Andy Shevchenko wrote:
> On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> Remove the duplicate IPU ACPI bridge code and use the new
>> shared ipu_bridge_init() functionality.
>>
>> Note this will also use / assume v4l2-async device instantation for
> instantiation
>
>> ov5693 sensors on atomisp devices since ipu_supported_sensors[]
>> already contains a match for this.
>>
>> This is fine since recent atomisp improvements allow the atomisp code
>> to work with generic v4l2 sensor drivers and using an unmodified
>> drivers/media/i2c/ov5693.c has been successfully tested on
>> an Acer Iconia W4 820 tablet with Bay an ISP2400 + OV5693 sensor.
> with an?
> with Bay Trail and ?
>
> This is a cool patch!
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
>
> A few remarks below.
>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/staging/media/atomisp/Kconfig         |   2 +
>>   .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ----
>>   .../media/atomisp/pci/atomisp_csi2_bridge.c   | 326 +++---------------
>>   .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
>>   4 files changed, 48 insertions(+), 348 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
>> index e9b168ba97bf..1c1007362e21 100644
>> --- a/drivers/staging/media/atomisp/Kconfig
>> +++ b/drivers/staging/media/atomisp/Kconfig
>> @@ -12,9 +12,11 @@ menuconfig INTEL_ATOMISP
>>   config VIDEO_ATOMISP
>>          tristate "Intel Atom Image Signal Processor Driver"
>>          depends on VIDEO_DEV && INTEL_ATOMISP
>> +       depends on MEDIA_PCI_SUPPORT && I2C
> These two on a single "depends on" line makes me confused.
> I would split the PCI part from the I2C one. However, I haven't
> checked the context of other media drivers, maybe this is an existing
> idiom, in that case let it be.
>
>>          depends on PMIC_OPREGION
>>          select V4L2_FWNODE
>>          select IOSF_MBI
>> +       select IPU_BRIDGE
>>          select VIDEOBUF2_VMALLOC
>>          select VIDEO_V4L2_SUBDEV_API
>>          help
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2.h b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
>> index 16ddb3ab2963..8a112acba1e0 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2.h
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2.h
>> @@ -30,9 +30,6 @@
>>   #define CSI2_PAD_SOURCE                1
>>   #define CSI2_PADS_NUM          2
>>
>> -#define CSI2_MAX_LANES         4
>> -#define CSI2_MAX_LINK_FREQS    3
>> -
>>   #define CSI2_MAX_ACPI_GPIOS    2u
>>
>>   struct acpi_device;
>> @@ -55,70 +52,6 @@ struct atomisp_csi2_acpi_gpio_parsing_data {
>>          unsigned int map_count;
>>   };
>>
>> -enum atomisp_csi2_sensor_swnodes {
>> -       SWNODE_SENSOR,
>> -       SWNODE_SENSOR_PORT,
>> -       SWNODE_SENSOR_ENDPOINT,
>> -       SWNODE_CSI2_PORT,
>> -       SWNODE_CSI2_ENDPOINT,
>> -       SWNODE_COUNT
>> -};
>> -
>> -struct atomisp_csi2_property_names {
>> -       char clock_frequency[16];
>> -       char rotation[9];
>> -       char bus_type[9];
>> -       char data_lanes[11];
>> -       char remote_endpoint[16];
>> -       char link_frequencies[17];
>> -};
>> -
>> -struct atomisp_csi2_node_names {
>> -       char port[7];
>> -       char endpoint[11];
>> -       char remote_port[7];
>> -};
>> -
>> -struct atomisp_csi2_sensor_config {
>> -       const char *hid;
>> -       int lanes;
>> -       int nr_link_freqs;
>> -       u64 link_freqs[CSI2_MAX_LINK_FREQS];
>> -};
>> -
>> -struct atomisp_csi2_sensor {
>> -       /* Append port in "-%u" format as suffix of HID */
>> -       char name[ACPI_ID_LEN + 4];
>> -       struct acpi_device *adev;
>> -       int port;
>> -       int lanes;
>> -
>> -       /* SWNODE_COUNT + 1 for terminating NULL */
>> -       const struct software_node *group[SWNODE_COUNT + 1];
>> -       struct software_node swnodes[SWNODE_COUNT];
>> -       struct atomisp_csi2_node_names node_names;
>> -       struct atomisp_csi2_property_names prop_names;
>> -       /* "clock-frequency", "rotation" + terminating entry */
>> -       struct property_entry dev_properties[3];
>> -       /* "bus-type", "data-lanes", "remote-endpoint" + "link-freq" + terminating entry */
>> -       struct property_entry ep_properties[5];
>> -       /* "data-lanes", "remote-endpoint" + terminating entry */
>> -       struct property_entry csi2_properties[3];
>> -       struct software_node_ref_args local_ref[1];
>> -       struct software_node_ref_args remote_ref[1];
>> -       struct software_node_ref_args vcm_ref[1];
>> -       /* GPIO mappings storage */
>> -       struct atomisp_csi2_acpi_gpio_map gpio_map;
>> -};
>> -
>> -struct atomisp_csi2_bridge {
>> -       struct software_node csi2_node;
>> -       char csi2_node_name[14];
>> -       u32 data_lanes[CSI2_MAX_LANES];
>> -       unsigned int n_sensors;
>> -       struct atomisp_csi2_sensor sensors[ATOMISP_CAMERA_NR_PORTS];
>> -};
>> -
>>   struct atomisp_mipi_csi2_device {
>>          struct v4l2_subdev subdev;
>>          struct media_pad pads[CSI2_PADS_NUM];
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
>> index 0d12ba78d9c1..d7b8c3de0e17 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
>> @@ -14,31 +14,14 @@
>>   #include <linux/device.h>
>>   #include <linux/dmi.h>
>>   #include <linux/property.h>
>> +
>> +#include <media/ipu-bridge.h>
>>   #include <media/v4l2-fwnode.h>
>>
>>   #include "atomisp_cmd.h"
>>   #include "atomisp_csi2.h"
>>   #include "atomisp_internal.h"
>>
>> -#define NODE_SENSOR(_HID, _PROPS)              \
>> -       ((const struct software_node) {         \
>> -               .name = _HID,                   \
>> -               .properties = _PROPS,           \
>> -       })
>> -
>> -#define NODE_PORT(_PORT, _SENSOR_NODE)         \
>> -       ((const struct software_node) {         \
>> -               .name = _PORT,                  \
>> -               .parent = _SENSOR_NODE,         \
>> -       })
>> -
>> -#define NODE_ENDPOINT(_EP, _PORT, _PROPS)      \
>> -       ((const struct software_node) {         \
>> -               .name = _EP,                    \
>> -               .parent = _PORT,                \
>> -               .properties = _PROPS,           \
>> -       })
>> -
>>   #define PMC_CLK_RATE_19_2MHZ                   19200000
>>
>>   /*
>> @@ -83,21 +66,6 @@ static const guid_t atomisp_dsm_guid =
>>          GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
>>                    0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe);
>>
>> -/*
>> - * Extend this array with ACPI Hardware IDs of sensors known to be working
>> - * plus the default number of links + link-frequencies.
>> - *
>> - * Do not add an entry for a sensor that is not actually supported,
>> - * or which have not yet been converted to work without atomisp_gmin
>> - * power-management and with v4l2-async probing.
>> - */
>> -static const struct atomisp_csi2_sensor_config supported_sensors[] = {
>> -       /* GalaxyCore GC0310 */
>> -       { "INT0310", 1 },
>> -       /* Omnivision OV2680 */
>> -       { "OVTI2680", 1 },
>> -};
>> -
>>   /*
>>    * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
>>    * code from atomisp_gmin_platform.c.
>> @@ -400,8 +368,7 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
>>    * the INT3472 discrete.c code and there is some overlap, but there are
>>    * enough differences that it is difficult to share the code.
>>    */
>> -static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
>> -                                         struct acpi_device *adev)
>> +static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>>   {
>>          struct atomisp_csi2_acpi_gpio_parsing_data data = { };
>>          LIST_HEAD(resource_list);
>> @@ -469,9 +436,12 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
>>                  }
>>          }
>>
>> +       data.map = kzalloc(sizeof(*data.map), GFP_KERNEL);
>> +       if (!data.map)
>> +               return -ENOMEM;
>> +
>>          /* Now parse the ACPI resources and build the lookup table */
>>          data.adev = adev;
>> -       data.map = &sensor->gpio_map;
>>          ret = acpi_dev_get_resources(adev, &resource_list,
>>                                       atomisp_csi2_handle_acpi_gpio_res, &data);
>>          if (ret < 0)
>> @@ -491,220 +461,59 @@ static int atomisp_csi2_add_gpio_mappings(struct atomisp_csi2_sensor *sensor,
>>          return ret;
>>   }
>>
>> -static const struct atomisp_csi2_property_names prop_names = {
>> -       .clock_frequency = "clock-frequency",
>> -       .rotation = "rotation",
>> -       .bus_type = "bus-type",
>> -       .data_lanes = "data-lanes",
>> -       .remote_endpoint = "remote-endpoint",
>> -       .link_frequencies = "link-frequencies",
>> -};
>> -
>> -static void atomisp_csi2_create_fwnode_properties(struct atomisp_csi2_sensor *sensor,
>> -                                                 struct atomisp_csi2_bridge *bridge,
>> -                                                 const struct atomisp_csi2_sensor_config *cfg)
>> +static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
>> +                                           struct ipu_sensor *sensor)
>>   {
>> -       sensor->prop_names = prop_names;
>> +       static const struct acpi_device_id dual_lane_sensors[] = {
>> +               { "INT33BE" },  /* OV5693 */
>> +               {}
>> +       };
>> +       int ret, clock_num, lanes;
>>
>> -       sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
>> -       sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
>> +       /*
>> +        * ACPI takes care of turning the PMC clock on and off, but on BYT
>> +        * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
>> +        * Get the PMC-clock number from ACPI PR0 method and set it to 19.2 MHz.
>> +        * The PMC-clock number is also used to determine the default CSI port.
>> +        */
>> +       clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
>>
>> -       sensor->dev_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.clock_frequency,
>> -                                                      PMC_CLK_RATE_19_2MHZ);
>> -       sensor->dev_properties[1] = PROPERTY_ENTRY_U32(sensor->prop_names.rotation, 0);
>> +       ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
>> +       if (ret)
>> +               return ret;
>>
>> -       sensor->ep_properties[0] = PROPERTY_ENTRY_U32(sensor->prop_names.bus_type,
>> -                                                     V4L2_FWNODE_BUS_TYPE_CSI2_DPHY);
>> -       sensor->ep_properties[1] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
>> -                                                               bridge->data_lanes,
>> -                                                               sensor->lanes);
>> -       sensor->ep_properties[2] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
>> -                                                           sensor->local_ref);
>> -       if (cfg->nr_link_freqs > 0)
>> -               sensor->ep_properties[3] =
>> -                       PROPERTY_ENTRY_U64_ARRAY_LEN(sensor->prop_names.link_frequencies,
>> -                                                    cfg->link_freqs, cfg->nr_link_freqs);
>> -
>> -       sensor->csi2_properties[0] = PROPERTY_ENTRY_U32_ARRAY_LEN(sensor->prop_names.data_lanes,
>> -                                                                 bridge->data_lanes,
>> -                                                                 sensor->lanes);
>> -       sensor->csi2_properties[1] = PROPERTY_ENTRY_REF_ARRAY(sensor->prop_names.remote_endpoint,
>> -                                                             sensor->remote_ref);
>> -}
>> -
>> -static void atomisp_csi2_init_swnode_names(struct atomisp_csi2_sensor *sensor)
>> -{
>> -       snprintf(sensor->node_names.remote_port,
>> -                sizeof(sensor->node_names.remote_port),
>> -                SWNODE_GRAPH_PORT_NAME_FMT, sensor->port);
>> -       snprintf(sensor->node_names.port,
>> -                sizeof(sensor->node_names.port),
>> -                SWNODE_GRAPH_PORT_NAME_FMT, 0); /* Always port 0 */
>> -       snprintf(sensor->node_names.endpoint,
>> -                sizeof(sensor->node_names.endpoint),
>> -                SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0); /* And endpoint 0 */
>> -}
>> -
>> -static void atomisp_csi2_init_swnode_group(struct atomisp_csi2_sensor *sensor)
>> -{
>> -       struct software_node *nodes = sensor->swnodes;
>> -
>> -       sensor->group[SWNODE_SENSOR] = &nodes[SWNODE_SENSOR];
>> -       sensor->group[SWNODE_SENSOR_PORT] = &nodes[SWNODE_SENSOR_PORT];
>> -       sensor->group[SWNODE_SENSOR_ENDPOINT] = &nodes[SWNODE_SENSOR_ENDPOINT];
>> -       sensor->group[SWNODE_CSI2_PORT] = &nodes[SWNODE_CSI2_PORT];
>> -       sensor->group[SWNODE_CSI2_ENDPOINT] = &nodes[SWNODE_CSI2_ENDPOINT];
>> -}
>> -
>> -static void atomisp_csi2_create_connection_swnodes(struct atomisp_csi2_bridge *bridge,
>> -                                                  struct atomisp_csi2_sensor *sensor)
>> -{
>> -       struct software_node *nodes = sensor->swnodes;
>> -
>> -       atomisp_csi2_init_swnode_names(sensor);
>> -
>> -       nodes[SWNODE_SENSOR] = NODE_SENSOR(sensor->name,
>> -                                          sensor->dev_properties);
>> -       nodes[SWNODE_SENSOR_PORT] = NODE_PORT(sensor->node_names.port,
>> -                                             &nodes[SWNODE_SENSOR]);
>> -       nodes[SWNODE_SENSOR_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
>> -                                                     &nodes[SWNODE_SENSOR_PORT],
>> -                                                     sensor->ep_properties);
>> -       nodes[SWNODE_CSI2_PORT] = NODE_PORT(sensor->node_names.remote_port,
>> -                                           &bridge->csi2_node);
>> -       nodes[SWNODE_CSI2_ENDPOINT] = NODE_ENDPOINT(sensor->node_names.endpoint,
>> -                                                   &nodes[SWNODE_CSI2_PORT],
>> -                                                   sensor->csi2_properties);
>> -
>> -       atomisp_csi2_init_swnode_group(sensor);
>> -}
>> -
>> -static void atomisp_csi2_unregister_sensors(struct atomisp_csi2_bridge *bridge)
>> -{
>> -       struct atomisp_csi2_sensor *sensor;
>> -       unsigned int i;
>> -
>> -       for (i = 0; i < bridge->n_sensors; i++) {
>> -               sensor = &bridge->sensors[i];
>> -               software_node_unregister_node_group(sensor->group);
>> -               acpi_dev_remove_driver_gpios(sensor->adev);
>> -               acpi_dev_put(sensor->adev);
>> +       sensor->link = atomisp_csi2_get_port(adev, clock_num);
>> +       if (sensor->link >= ATOMISP_CAMERA_NR_PORTS) {
>> +               dev_err(&adev->dev, "Invalid port: %u\n", sensor->link);
>> +               return -EINVAL;
>>          }
>> -}
>>
>> -static int atomisp_csi2_connect_sensor(const struct atomisp_csi2_sensor_config *cfg,
>> -                                      struct atomisp_csi2_bridge *bridge,
>> -                                      struct atomisp_device *isp)
>> -{
>> -       struct fwnode_handle *fwnode, *primary;
>> -       struct atomisp_csi2_sensor *sensor;
>> -       struct acpi_device *adev;
>> -       int ret, clock_num;
>> +       if (!acpi_match_device_ids(adev, dual_lane_sensors))
>> +               lanes = 2;
>> +       else
>> +               lanes = 1;
> Can we use positive conditional?
>
>         if (acpi_match_device_ids(adev, dual_lane_sensors))
>                 lanes = 1;
>         else
>                 lanes = 2;


Or perhaps "if (acpi_match_device_ids(adev, dual_lane_sensors) == 0)"?


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>
>> -       for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
>> -               if (!adev->status.enabled)
>> -                       continue;
>> -
>> -               if (bridge->n_sensors >= ATOMISP_CAMERA_NR_PORTS) {
>> -                       dev_err(isp->dev, "Exceeded available CSI2 ports\n");
>> -                       ret = -EOVERFLOW;
>> -                       goto err_put_adev;
>> -               }
>> -
>> -               sensor = &bridge->sensors[bridge->n_sensors];
>> -
>> -               /*
>> -                * ACPI takes care of turning the PMC clock on and off, but on BYT
>> -                * the clock defaults to 25 MHz instead of the expected 19.2 MHz.
>> -                * Get the PMC-clock number from ACPI _PR0 method and set it to 19.2 MHz.
>> -                * The PMC-clock number is also used to determine the default CSI port.
>> -                */
>> -               clock_num = atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(adev);
>> -
>> -               ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
>> -               if (ret)
>> -                       goto err_put_adev;
>> -
>> -               sensor->port = atomisp_csi2_get_port(adev, clock_num);
>> -               if (sensor->port >= ATOMISP_CAMERA_NR_PORTS) {
>> -                       acpi_handle_err(adev->handle, "Invalid port: %d\n", sensor->port);
>> -                       ret = -EINVAL;
>> -                       goto err_put_adev;
>> -               }
>> -
>> -               sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", cfg->lanes);
>> -               if (sensor->lanes > CSI2_MAX_LANES) {
>> -                       acpi_handle_err(adev->handle, "Invalid number of lanes: %d\n", sensor->lanes);
>> -                       ret = -EINVAL;
>> -                       goto err_put_adev;
>> -               }
>> -
>> -               ret = atomisp_csi2_add_gpio_mappings(sensor, adev);
>> -               if (ret)
>> -                       goto err_put_adev;
>> -
>> -               snprintf(sensor->name, sizeof(sensor->name), "%s-%u",
>> -                        cfg->hid, sensor->port);
>> -
>> -               atomisp_csi2_create_fwnode_properties(sensor, bridge, cfg);
>> -               atomisp_csi2_create_connection_swnodes(bridge, sensor);
>> -
>> -               ret = software_node_register_node_group(sensor->group);
>> -               if (ret)
>> -                       goto err_remove_mappings;
>> -
>> -               fwnode = software_node_fwnode(&sensor->swnodes[SWNODE_SENSOR]);
>> -               if (!fwnode) {
>> -                       ret = -ENODEV;
>> -                       goto err_free_swnodes;
>> -               }
>> -
>> -               sensor->adev = acpi_dev_get(adev);
>> -
>> -               primary = acpi_fwnode_handle(adev);
>> -               primary->secondary = fwnode;
>> -
>> -               bridge->n_sensors++;
>> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
>> +       if (sensor->lanes > IPU_MAX_LANES) {
>> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
> Yeah, I think we would be consistent in using the ACPI handle to print
> the messages from ACPI sensor devices.
>
>> +               return -EINVAL;
>>          }
>>
>> +       ret = atomisp_csi2_add_gpio_mappings(adev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       sensor->mclkspeed = PMC_CLK_RATE_19_2MHZ;
>> +       sensor->rotation = 0;
>> +       sensor->orientation = V4L2_FWNODE_ORIENTATION_EXTERNAL;
>> +
>>          return 0;
>> -
>> -err_free_swnodes:
>> -       software_node_unregister_node_group(sensor->group);
>> -err_remove_mappings:
>> -       acpi_dev_remove_driver_gpios(adev);
>> -err_put_adev:
>> -       acpi_dev_put(adev);
>> -       return ret;
>> -}
>> -
>> -static int atomisp_csi2_connect_sensors(struct atomisp_csi2_bridge *bridge,
>> -                                       struct atomisp_device *isp)
>> -{
>> -       unsigned int i;
>> -       int ret;
>> -
>> -       for (i = 0; i < ARRAY_SIZE(supported_sensors); i++) {
>> -               const struct atomisp_csi2_sensor_config *cfg = &supported_sensors[i];
>> -
>> -               ret = atomisp_csi2_connect_sensor(cfg, bridge, isp);
>> -               if (ret)
>> -                       goto err_unregister_sensors;
>> -       }
>> -
>> -       return 0;
>> -
>> -err_unregister_sensors:
>> -       atomisp_csi2_unregister_sensors(bridge);
>> -       return ret;
>>   }
>>
>>   int atomisp_csi2_bridge_init(struct atomisp_device *isp)
>>   {
>> -       struct atomisp_csi2_bridge *bridge;
>>          struct device *dev = isp->dev;
>>          struct fwnode_handle *fwnode;
>> -       int i, ret;
>>
>>          /*
>>           * This function is intended to run only once and then leave
>> @@ -716,52 +525,7 @@ int atomisp_csi2_bridge_init(struct atomisp_device *isp)
>>          if (fwnode && fwnode->secondary)
>>                  return 0;
>>
>> -       bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
>> -       if (!bridge)
>> -               return -ENOMEM;
>> -
>> -       strscpy(bridge->csi2_node_name, "atomisp-csi2", sizeof(bridge->csi2_node_name));
>> -       bridge->csi2_node.name = bridge->csi2_node_name;
>> -
>> -       ret = software_node_register(&bridge->csi2_node);
>> -       if (ret < 0) {
>> -               dev_err(dev, "Failed to register the CSI2 HID node\n");
>> -               goto err_free_bridge;
>> -       }
>> -
>> -       /*
>> -        * Map the lane arrangement, which is fixed for the ISP2 (meaning we
>> -        * only need one, rather than one per sensor). We include it as a
>> -        * member of the bridge struct rather than a global variable so
>> -        * that it survives if the module is unloaded along with the rest of
>> -        * the struct.
>> -        */
>> -       for (i = 0; i < CSI2_MAX_LANES; i++)
>> -               bridge->data_lanes[i] = i + 1;
>> -
>> -       ret = atomisp_csi2_connect_sensors(bridge, isp);
>> -       if (ret || bridge->n_sensors == 0)
>> -               goto err_unregister_csi2;
>> -
>> -       fwnode = software_node_fwnode(&bridge->csi2_node);
>> -       if (!fwnode) {
>> -               dev_err(dev, "Error getting fwnode from csi2 software_node\n");
>> -               ret = -ENODEV;
>> -               goto err_unregister_sensors;
>> -       }
>> -
>> -       set_secondary_fwnode(dev, fwnode);
>> -
>> -       return 0;
>> -
>> -err_unregister_sensors:
>> -       atomisp_csi2_unregister_sensors(bridge);
>> -err_unregister_csi2:
>> -       software_node_unregister(&bridge->csi2_node);
>> -err_free_bridge:
>> -       kfree(bridge);
>> -
>> -       return ret;
>> +       return ipu_bridge_init(dev, atomisp_csi2_parse_sensor_fwnode);
>>   }
>>
>>   /******* V4L2 sub-device asynchronous registration callbacks***********/
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
>> index c43b916a006e..0d80f0893a2e 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
>> @@ -1615,3 +1615,4 @@ MODULE_AUTHOR("Wen Wang <wen.w.wang@intel.com>");
>>   MODULE_AUTHOR("Xiaolin Zhang <xiaolin.zhang@intel.com>");
>>   MODULE_LICENSE("GPL");
>>   MODULE_DESCRIPTION("Intel ATOM Platform ISP Driver");
>> +MODULE_IMPORT_NS(INTEL_IPU_BRIDGE);
>> --
>> 2.41.0
>>
>

Re: [PATCH v2 09/15] media: ipu-bridge: Drop early setting of sensor->adev

[Andy Shevchenko]

On Tue, Jul 04, 2023 at 01:20:33PM +0200, Hans de Goede wrote:
> On 6/30/23 16:30, Andy Shevchenko wrote:
> > On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> sensor->adev is no longer dereferenced before it is permanently set by:
> >>
> >>         sensor->adev = acpi_dev_get(adev);
> >>
> >> So the early assignment with a borrowed reference can be dropped.
> > 
> > Ah, now I see that that change was temporary.
> 
> Ack
> 
> > Can we avoid backporting
> > it please?
> 
> Patch 1/15 does fix a NULL pointer deref oops, so backporting it would
> be a good thing to do IMHO.

Then better to go with ACPI handle. I don't like the rewriting same field
(even if it's the same information). It's prone to errors which might be
hard to find.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Andy Shevchenko]

On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote:
> On 30/06/2023 16:23, Andy Shevchenko wrote:
> > On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > > When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
> > > sensor->adev is not set yet.
> > > 
> > > So if either of the dev_warn() calls about unknown values are hit this
> > > will lead to a NULL pointer deref.
> > > 
> > > Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
> > > on errors harder, to fix this.
> > TBH, I don't like this approach, it seems a bit dirty to me.
> > 
> > First of all, why do we need pci_dev to be a parameter in this function?
> > Second, why don't we consistently use the ACPI handle (with respective
> > acpi_handle_*() macros to print messages)?
> > 
> > So, my proposal here is to actually save the ACPI device handle in the
> > sensor object and use it for the messaging. It makes it consistent and
> > doesn't require to rewrite adev field which seems the dirty part to
> > me.
> 
> It's a bit finicky but I don't think it's so bad; the refcounting is all
> fine, the later acpi_dev_get() is only to hold a reference once the next
> loop iteration frees the existing one and the rewrite should store the exact
> same pointer...we could just not store the result of the acpi_dev_get() call
> to avoid that weird rewrite perhaps?

For short term solution in between the patches I might agree with you, but
backporting. Backporting a bad code doesn't make it better even if it fixes
nasty bug. And I proposed the solution. We may kill the handle same way as
we are killing the awkwardness of this assignment later in the series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Andy Shevchenko]

On Tue, Jul 04, 2023 at 02:55:02PM +0100, Dan Scally wrote:
> On 30/06/2023 16:45, Andy Shevchenko wrote:
> > On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:

(Can you remove unneeded context when replying to the messages, please?)

...

> > > +       if (!acpi_match_device_ids(adev, dual_lane_sensors))
> > > +               lanes = 2;
> > > +       else
> > > +               lanes = 1;
> > Can we use positive conditional?
> > 
> >         if (acpi_match_device_ids(adev, dual_lane_sensors))
> >                 lanes = 1;
> >         else
> >                 lanes = 2;
> 
> Or perhaps "if (acpi_match_device_ids(adev, dual_lane_sensors) == 0)"?

Unfortunately this will be more confusing. The above mentioned API returns
the error code or 0 on success.

	ret = acpi_match_device_ids(adev, dual_lane_sensors);
        if (ret)
                lanes = 1;
        else
                lanes = 2;

probably is the best to have semantics of returned code more or less
easy to get.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Hans de Goede]

Hi,

On 7/4/23 16:28, Andy Shevchenko wrote:
> On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote:
>> On 30/06/2023 16:23, Andy Shevchenko wrote:
>>> On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
>>>> sensor->adev is not set yet.
>>>>
>>>> So if either of the dev_warn() calls about unknown values are hit this
>>>> will lead to a NULL pointer deref.
>>>>
>>>> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
>>>> on errors harder, to fix this.
>>> TBH, I don't like this approach, it seems a bit dirty to me.
>>>
>>> First of all, why do we need pci_dev to be a parameter in this function?
>>> Second, why don't we consistently use the ACPI handle (with respective
>>> acpi_handle_*() macros to print messages)?
>>>
>>> So, my proposal here is to actually save the ACPI device handle in the
>>> sensor object and use it for the messaging. It makes it consistent and
>>> doesn't require to rewrite adev field which seems the dirty part to
>>> me.
>>
>> It's a bit finicky but I don't think it's so bad; the refcounting is all
>> fine, the later acpi_dev_get() is only to hold a reference once the next
>> loop iteration frees the existing one and the rewrite should store the exact
>> same pointer...we could just not store the result of the acpi_dev_get() call
>> to avoid that weird rewrite perhaps?
> 
> For short term solution in between the patches I might agree with you, but
> backporting. Backporting a bad code doesn't make it better even if it fixes
> nasty bug. And I proposed the solution. We may kill the handle same way as
> we are killing the awkwardness of this assignment later in the series.

Yeah, no sorry. As Dan pointed out this fix is fine and I don't feel
like re-writing it just because you don't like it.

I don't see any real technical arguments against this approach, just
you not liking it.

Regards,

Hans

Re: [PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Dan Scally]

Hi Hans

On 30/06/2023 13:06, Hans de Goede wrote:
> In most cases when a VCM is used there is a single integrated module
> with the sensor + VCM + lens. This means that the sensor and VCM often
> share regulators and possibly also something like a powerdown pin.
>
> In the ACPI tables this is modelled as a single ACPI device with
> multiple I2cSerialBus resources.
>
> On atomisp devices the regulators and clks are modelled as ACPI
> power-resources, which are controlled by the (ACPI) power state
> of the sensor. So the sensor must be in D0 power state for the VCM
> to work.
>
> To make this work add a device-link with DL_FLAG_PM_RUNTIME flag
> so that the sensor will automatically be runtime-resumed whenever
> the VCM is runtime-resumed.
>
> This requires the probing of the VCM and thus the creation of the VCM
> I2C-client to be delayed till after the sensor driver has bound.
>
> Move the instantiation of the VCM I2C-client to the v4l2_async_notifier
> bound op, so that it is done after the sensor driver has bound; and
> add code to add the device-link.
>
> This fixes the problem with the shared ACPI power-resources on atomisp2
> and this avoids the need for VCM related workarounds on IPU3 / IPU6.
>
> E.g. until now the dw9719 driver needed to get and control a Vsio
> (V sensor IO) regulator since that needs to be enabled to enable I2C
> pass-through on the PMIC on the sensor module. So the driver was
> controlling this regulator even though the actual dw9719 chip has no
> Vsio pin / power-plane.
>
> This also removes the need for ipu_bridge_init() to return
> -EPROBE_DEFER since the VCM is now instantiated later.
>
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---


This is a really cool way of solving the problem I think; Sakari mentioned it to me a little while 
ago, it's nice to see the implementation, thanks for doing it. Just one comment below for 
ipu_bridge_instantiate_vcm(), but:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>


I also tested the set on my Go and as far as I can tell everything works fine. Not really applicable 
to 13/15 since that's atomisp specific but otherwise:


Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

>   drivers/media/pci/intel/ipu-bridge.c     | 159 +++++++++++++++--------
>   drivers/media/pci/intel/ipu3/ipu3-cio2.c |   5 +
>   include/media/ipu-bridge.h               |   5 +-
>   3 files changed, 109 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 07a34f20af8e..32dabc16a7b4 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -4,7 +4,9 @@
>   #include <linux/acpi.h>
>   #include <linux/device.h>
>   #include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
>   #include <linux/property.h>
> +#include <linux/workqueue.h>
>   
>   #include <media/ipu-bridge.h>
>   #include <media/v4l2-fwnode.h>
> @@ -289,29 +291,112 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>   	ipu_bridge_init_swnode_group(sensor);
>   }
>   
> -static void ipu_bridge_instantiate_vcm_i2c_client(struct ipu_sensor *sensor)
> -{
> -	struct i2c_board_info board_info = { };
> +/*
> + * The actual instantiation must be done from a workqueue to avoid
> + * a deadlock on taking list_lock from v4l2-async twice.
> + */
> +struct ipu_bridge_instantiate_vcm_work_data {
> +	struct work_struct work;
> +	struct device *sensor;
>   	char name[16];
> +	struct i2c_board_info board_info;
> +};
>   
> -	if (!sensor->vcm_type)
> -		return;
> +static void ipu_bridge_instantiate_vcm_work(struct work_struct *_work)
> +{
> +	struct ipu_bridge_instantiate_vcm_work_data *work =
> +		container_of(_work,
> +			     struct ipu_bridge_instantiate_vcm_work_data,
> +			     work);
> +	struct acpi_device *adev = ACPI_COMPANION(work->sensor);
> +	struct i2c_client *vcm_client;
> +	bool put_fwnode = true;
> +	int ret;
>   
> -	snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
> -	board_info.dev_name = name;
> -	strscpy(board_info.type, sensor->vcm_type, ARRAY_SIZE(board_info.type));
> -	board_info.swnode = &sensor->swnodes[SWNODE_VCM];
> -
> -	sensor->vcm_i2c_client =
> -		i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(sensor->adev),
> -					      1, &board_info);
> -	if (IS_ERR(sensor->vcm_i2c_client)) {
> -		dev_warn(&sensor->adev->dev, "Error instantiation VCM i2c-client: %ld\n",
> -			 PTR_ERR(sensor->vcm_i2c_client));
> -		sensor->vcm_i2c_client = NULL;
> +	/*
> +	 * The client may get probed before the device_link gets added below
> +	 * make sure the sensor is powered-up during probe.
> +	 */
> +	ret = pm_runtime_get_sync(work->sensor);
> +	if (ret < 0) {
> +		dev_err(work->sensor, "Error %d runtime-resuming sensor, cannot instantiate VCM\n",
> +			ret);
> +		goto out;
>   	}
> +
> +	/*
> +	 * Note the client is created only once and then kept around
> +	 * even after a rmmod, just like the software-nodes.
> +	 */
> +	vcm_client = i2c_acpi_new_device_by_fwnode(acpi_fwnode_handle(adev),
> +						   1, &work->board_info);
> +	if (IS_ERR(vcm_client)) {
> +		dev_err(work->sensor, "Error instantiating VCM client: %ld\n",
> +			PTR_ERR(vcm_client));
> +		goto out;
> +	}
> +
> +	device_link_add(&vcm_client->dev, work->sensor, DL_FLAG_PM_RUNTIME);
> +
> +	dev_info(work->sensor, "Instantiated %s VCM\n", work->board_info.type);
> +	put_fwnode = false; /* Ownership has passed to the i2c-client */
> +
> +out:
> +	pm_runtime_put(work->sensor);
> +	put_device(work->sensor);
> +	if (put_fwnode)
> +		fwnode_handle_put(work->board_info.fwnode);
> +	kfree(work);
>   }
>   
> +int ipu_bridge_instantiate_vcm(struct device *sensor)
> +{
> +	struct ipu_bridge_instantiate_vcm_work_data *work;
> +	struct fwnode_handle *vcm_fwnode;
> +	struct i2c_client *vcm_client;
> +	struct acpi_device *adev;
> +	char *sep;
> +
> +	adev = ACPI_COMPANION(sensor);
> +	if (!adev)
> +		return 0;
> +
> +	vcm_fwnode = fwnode_find_reference(dev_fwnode(sensor), "lens-focus", 0);
> +	if (IS_ERR(vcm_fwnode))
> +		return 0;
> +
> +	/* When reloading modules the client will already exist */
> +	vcm_client = i2c_find_device_by_fwnode(vcm_fwnode);
> +	if (vcm_client) {
> +		fwnode_handle_put(vcm_fwnode);
> +		put_device(&vcm_client->dev);
> +		return 0;
> +	}
> +
> +	work = kzalloc(sizeof(*work), GFP_KERNEL);
> +	if (!work) {
> +		fwnode_handle_put(vcm_fwnode);
> +		return -ENOMEM;
> +	}
> +
> +	INIT_WORK(&work->work, ipu_bridge_instantiate_vcm_work);
> +	work->sensor = get_device(sensor);
> +	snprintf(work->name, sizeof(work->name), "%s-VCM",
> +		 acpi_dev_name(adev));
> +	work->board_info.dev_name = work->name;
> +	work->board_info.fwnode = vcm_fwnode;
> +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> +		I2C_NAME_SIZE);
> +	/* Strip "-<link>" postfix */
> +	sep = strchrnul(work->board_info.type, '-');
> +	*sep = 0;

I think strreplace(work->board_info.type, '-', '\0') here would be cleaner, and either way probably 
we need #include <linux/string.h> for the str* funcs here


> +
> +	queue_work(system_long_wq, &work->work);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(ipu_bridge_instantiate_vcm);
> +
>   static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   {
>   	struct ipu_sensor *sensor;
> @@ -321,7 +406,6 @@ static void ipu_bridge_unregister_sensors(struct ipu_bridge *bridge)
>   		sensor = &bridge->sensors[i];
>   		software_node_unregister_node_group(sensor->group);
>   		acpi_dev_put(sensor->adev);
> -		i2c_unregister_device(sensor->vcm_i2c_client);
>   	}
>   }
>   
> @@ -371,8 +455,6 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>   		primary = acpi_fwnode_handle(adev);
>   		primary->secondary = fwnode;
>   
> -		ipu_bridge_instantiate_vcm_i2c_client(sensor);
> -
>   		dev_info(bridge->dev, "Found supported sensor %s\n",
>   			 acpi_dev_name(adev));
>   
> @@ -409,40 +491,6 @@ static int ipu_bridge_connect_sensors(struct ipu_bridge *bridge)
>   	return ret;
>   }
>   
> -/*
> - * The VCM cannot be probed until the PMIC is completely setup. We cannot rely
> - * on -EPROBE_DEFER for this, since the consumer<->supplier relations between
> - * the VCM and regulators/clks are not described in ACPI, instead they are
> - * passed as board-data to the PMIC drivers. Since -PROBE_DEFER does not work
> - * for the clks/regulators the VCM i2c-clients must not be instantiated until
> - * the PMIC is fully setup.
> - *
> - * The sensor/VCM ACPI device has an ACPI _DEP on the PMIC, check this using the
> - * acpi_dev_ready_for_enumeration() helper, like the i2c-core-acpi code does
> - * for the sensors.
> - */
> -static int ipu_bridge_sensors_are_ready(void)
> -{
> -	struct acpi_device *adev;
> -	bool ready = true;
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> -		const struct ipu_sensor_config *cfg =
> -			&ipu_supported_sensors[i];
> -
> -		for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> -			if (!adev->status.enabled)
> -				continue;
> -
> -			if (!acpi_dev_ready_for_enumeration(adev))
> -				ready = false;
> -		}
> -	}
> -
> -	return ready;
> -}
> -
>   int ipu_bridge_init(struct device *dev,
>   		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
>   					       struct ipu_sensor *sensor))
> @@ -452,9 +500,6 @@ int ipu_bridge_init(struct device *dev,
>   	unsigned int i;
>   	int ret;
>   
> -	if (!ipu_bridge_sensors_are_ready())
> -		return -EPROBE_DEFER;
> -
>   	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
>   	if (!bridge)
>   		return -ENOMEM;
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 51a6d7cc44d2..690fc1c919af 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1388,10 +1388,15 @@ static int cio2_notifier_bound(struct v4l2_async_notifier *notifier,
>   	struct cio2_device *cio2 = to_cio2_device(notifier);
>   	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
>   	struct cio2_queue *q;
> +	int ret;
>   
>   	if (cio2->queue[s_asd->csi2.port].sensor)
>   		return -EBUSY;
>   
> +	ret = ipu_bridge_instantiate_vcm(sd->dev);
> +	if (ret)
> +		return ret;
> +
>   	q = &cio2->queue[s_asd->csi2.port];
>   
>   	q->csi2 = s_asd->csi2;
> diff --git a/include/media/ipu-bridge.h b/include/media/ipu-bridge.h
> index 969d8d7d6b93..31b138170c73 100644
> --- a/include/media/ipu-bridge.h
> +++ b/include/media/ipu-bridge.h
> @@ -7,8 +7,6 @@
>   #include <linux/types.h>
>   #include <media/v4l2-fwnode.h>
>   
> -struct i2c_client;
> -
>   #define IPU_HID				"INT343E"
>   #define IPU_MAX_LANES				4
>   #define IPU_MAX_PORTS				4
> @@ -117,7 +115,6 @@ struct ipu_sensor {
>   	/* append ssdb.link(u8) in "-%u" format as suffix of HID */
>   	char name[ACPI_ID_LEN + 4];
>   	struct acpi_device *adev;
> -	struct i2c_client *vcm_i2c_client;
>   
>   	/* SWNODE_COUNT + 1 for terminating NULL */
>   	const struct software_node *group[SWNODE_COUNT + 1];
> @@ -156,9 +153,11 @@ int ipu_bridge_init(struct device *dev,
>   		    int (*parse_sensor_fwnode)(struct acpi_device *adev,
>   					       struct ipu_sensor *sensor));
>   int ipu_bridge_parse_ssdb(struct acpi_device *adev, struct ipu_sensor *sensor);
> +int ipu_bridge_instantiate_vcm(struct device *sensor);
>   #else
>   /* Use a define to avoid the parse_sensor_fwnode arg getting evaluated */
>   #define ipu_bridge_init(dev, parse_sensor_fwnode)	(0)
> +static inline int ipu_bridge_instantiate_vcm(struct device *s) { return 0; }
>   #endif
>   
>   #endif

Re: [PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Andy Shevchenko]

On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
> On 30/06/2023 13:06, Hans de Goede wrote:

...

> > +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> > +		I2C_NAME_SIZE);
> > +	/* Strip "-<link>" postfix */
> > +	sep = strchrnul(work->board_info.type, '-');
> > +	*sep = 0;
> 
> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
> and either way probably we need #include <linux/string.h> for the str* funcs
> here

What we need is something like strcut(str, '<$CHAR>').

But related to the above code we can (besides using sizeof() instead
of I2C_NAME_SIZE):

	snprintf(work->board_info.type, sizeof(work->board_info.type),
		 "%pfwP", vcm_fwnode);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Hans de Goede]

Hi,

On 7/4/23 16:32, Andy Shevchenko wrote:
> On Tue, Jul 04, 2023 at 02:55:02PM +0100, Dan Scally wrote:
>> On 30/06/2023 16:45, Andy Shevchenko wrote:
>>> On Fri, Jun 30, 2023 at 2:07 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
> (Can you remove unneeded context when replying to the messages, please?)
> 
> ...
> 
>>>> +       if (!acpi_match_device_ids(adev, dual_lane_sensors))
>>>> +               lanes = 2;
>>>> +       else
>>>> +               lanes = 1;
>>> Can we use positive conditional?
>>>
>>>         if (acpi_match_device_ids(adev, dual_lane_sensors))
>>>                 lanes = 1;
>>>         else
>>>                 lanes = 2;
>>
>> Or perhaps "if (acpi_match_device_ids(adev, dual_lane_sensors) == 0)"?
> 
> Unfortunately this will be more confusing. The above mentioned API returns
> the error code or 0 on success.
> 
> 	ret = acpi_match_device_ids(adev, dual_lane_sensors);
>         if (ret)
>                 lanes = 1;
>         else
>                 lanes = 2;
> 
> probably is the best to have semantics of returned code more or less
> easy to get.

For adding vcm instantiation support I'm going to need more per-sensor
atomisp bridge specific data. So I'll switch this to using match-data
in the next version, using only match-data for sensors where lanes != 1
(or with a VCM, which usually are the same set of sensors).

Regards,

Hans

Re: [PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Hans de Goede]

Hi,

On 7/4/23 17:27, Andy Shevchenko wrote:
> On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
>> On 30/06/2023 13:06, Hans de Goede wrote:
> 
> ...
> 
>>> +	strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
>>> +		I2C_NAME_SIZE);
>>> +	/* Strip "-<link>" postfix */
>>> +	sep = strchrnul(work->board_info.type, '-');
>>> +	*sep = 0;
>>
>> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
>> and either way probably we need #include <linux/string.h> for the str* funcs
>> here
> 
> What we need is something like strcut(str, '<$CHAR>').
> 
> But related to the above code we can (besides using sizeof() instead
> of I2C_NAME_SIZE):
> 
> 	snprintf(work->board_info.type, sizeof(work->board_info.type),
> 		 "%pfwP", vcm_fwnode);

"%pfwP" ? what on earth does that do ?

Regards,

Hans

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Hans de Goede]

Hi,

On 6/30/23 16:45, Andy Shevchenko wrote:
>> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
>> +       if (sensor->lanes > IPU_MAX_LANES) {
>> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
> 
> Yeah, I think we would be consistent in using the ACPI handle to print
> the messages from ACPI sensor devices.

I do agree that we need to be consistent, but I regret having switched
to using the handle for this in the csi2-bridge code. The dmesg logs
this results in are much harder to read. Most devices typically have
2 different sensors and normally it is quite easy to see in the logs
which GPIOs, etc. are being used for the sensor.

But after the move to using the ACPI handle for logging the logs
show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
OVTI2680 and INT0310 making it much harder to figure on what
is going on (first need to do
"cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
to find out which path belongs to which sensor).

So I would rather get rid of the handle based logging, because it
is very cumbersome to use.

I'll add an extra patch to the next version of the set to switch all
the logging to using the acpi_device for logging.

Regards,

Hans

Re: [PATCH v2 01/15] media: ipu-bridge: Fix null pointer deref on SSDB/PLD parsing warnings

[Andy Shevchenko]

On Tue, Jul 4, 2023 at 5:50 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/4/23 16:28, Andy Shevchenko wrote:
> > On Tue, Jul 04, 2023 at 12:02:00PM +0100, Dan Scally wrote:
> >> On 30/06/2023 16:23, Andy Shevchenko wrote:
> >>> On Fri, Jun 30, 2023 at 2:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>> When ipu_bridge_parse_rotation() and ipu_bridge_parse_orientation() run
> >>>> sensor->adev is not set yet.
> >>>>
> >>>> So if either of the dev_warn() calls about unknown values are hit this
> >>>> will lead to a NULL pointer deref.
> >>>>
> >>>> Set sensor->adev earlier, with a borrowed ref to avoid making unrolling
> >>>> on errors harder, to fix this.
> >>> TBH, I don't like this approach, it seems a bit dirty to me.
> >>>
> >>> First of all, why do we need pci_dev to be a parameter in this function?
> >>> Second, why don't we consistently use the ACPI handle (with respective
> >>> acpi_handle_*() macros to print messages)?
> >>>
> >>> So, my proposal here is to actually save the ACPI device handle in the
> >>> sensor object and use it for the messaging. It makes it consistent and
> >>> doesn't require to rewrite adev field which seems the dirty part to
> >>> me.
> >>
> >> It's a bit finicky but I don't think it's so bad; the refcounting is all
> >> fine, the later acpi_dev_get() is only to hold a reference once the next
> >> loop iteration frees the existing one and the rewrite should store the exact
> >> same pointer...we could just not store the result of the acpi_dev_get() call
> >> to avoid that weird rewrite perhaps?
> >
> > For short term solution in between the patches I might agree with you, but
> > backporting. Backporting a bad code doesn't make it better even if it fixes
> > nasty bug. And I proposed the solution. We may kill the handle same way as
> > we are killing the awkwardness of this assignment later in the series.
>
> Yeah, no sorry. As Dan pointed out this fix is fine and I don't feel
> like re-writing it just because you don't like it.
>
> I don't see any real technical arguments against this approach, just
> you not liking it.

OK.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Andy Shevchenko]

On Tue, Jul 4, 2023 at 10:21 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 6/30/23 16:45, Andy Shevchenko wrote:

...

> >> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
> >
> > Yeah, I think we would be consistent in using the ACPI handle to print
> > the messages from ACPI sensor devices.
>
> I do agree that we need to be consistent, but I regret having switched
> to using the handle for this in the csi2-bridge code. The dmesg logs
> this results in are much harder to read. Most devices typically have
> 2 different sensors and normally it is quite easy to see in the logs
> which GPIOs, etc. are being used for the sensor.
>
> But after the move to using the ACPI handle for logging the logs
> show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
> OVTI2680 and INT0310 making it much harder to figure on what
> is going on (first need to do
> "cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
> to find out which path belongs to which sensor).
>
> So I would rather get rid of the handle based logging, because it
> is very cumbersome to use.

I see, you are right. The ACPI handle may not be the best for Linux
kernel drivers.

> I'll add an extra patch to the next version of the set to switch all
> the logging to using the acpi_device for logging.

Sounds good.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/15] media: ipu-bridge: Add a runtime-pm device-link between VCM and sensor

[Andy Shevchenko]

On Tue, Jul 4, 2023 at 6:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 7/4/23 17:27, Andy Shevchenko wrote:
> > On Tue, Jul 04, 2023 at 04:07:43PM +0100, Dan Scally wrote:
> >> On 30/06/2023 13:06, Hans de Goede wrote:

...

> >>> +   strscpy(work->board_info.type, fwnode_get_name(vcm_fwnode),
> >>> +           I2C_NAME_SIZE);
> >>> +   /* Strip "-<link>" postfix */
> >>> +   sep = strchrnul(work->board_info.type, '-');
> >>> +   *sep = 0;
> >>
> >> I think strreplace(work->board_info.type, '-', '\0') here would be cleaner,
> >> and either way probably we need #include <linux/string.h> for the str* funcs
> >> here
> >
> > What we need is something like strcut(str, '<$CHAR>').
> >
> > But related to the above code we can (besides using sizeof() instead
> > of I2C_NAME_SIZE):
> >
> >       snprintf(work->board_info.type, sizeof(work->board_info.type),
> >                "%pfwP", vcm_fwnode);
>
> "%pfwP" ? what on earth does that do ?

Prints fwnode name (as you opencoded with strscpy() + fwnode_get_name() above).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Sakari Ailus]

Hi Hans,

On Tue, Jul 04, 2023 at 09:21:47PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 6/30/23 16:45, Andy Shevchenko wrote:
> >> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
> >> +       if (sensor->lanes > IPU_MAX_LANES) {
> >> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
> > 
> > Yeah, I think we would be consistent in using the ACPI handle to print
> > the messages from ACPI sensor devices.
> 
> I do agree that we need to be consistent, but I regret having switched
> to using the handle for this in the csi2-bridge code. The dmesg logs
> this results in are much harder to read. Most devices typically have
> 2 different sensors and normally it is quite easy to see in the logs
> which GPIOs, etc. are being used for the sensor.
> 
> But after the move to using the ACPI handle for logging the logs
> show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
> OVTI2680 and INT0310 making it much harder to figure on what
> is going on (first need to do
> "cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
> to find out which path belongs to which sensor).

In cases such as the above, the developer probably needs to address issues
not in the sensor driver but in the ACPI tables (or in IPU bridge code). So
for this reason I'd prefer printing the device path instead of the HID
(which is also somewhat opaque).

> 
> So I would rather get rid of the handle based logging, because it
> is very cumbersome to use.

The V4L2 async and fwnode frameworks use handles, too, for the same reason.

That said, I don't mind printing device names either. AFAIR Laurent
actually proposed that recently for the V4L2 fwnode and even promised to
send a patch. :-)

> 
> I'll add an extra patch to the next version of the set to switch all
> the logging to using the acpi_device for logging.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Hans de Goede]

Hi,

On 7/5/23 12:38, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Jul 04, 2023 at 09:21:47PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 6/30/23 16:45, Andy Shevchenko wrote:
>>>> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
>>>> +       if (sensor->lanes > IPU_MAX_LANES) {
>>>> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
>>>
>>> Yeah, I think we would be consistent in using the ACPI handle to print
>>> the messages from ACPI sensor devices.
>>
>> I do agree that we need to be consistent, but I regret having switched
>> to using the handle for this in the csi2-bridge code. The dmesg logs
>> this results in are much harder to read. Most devices typically have
>> 2 different sensors and normally it is quite easy to see in the logs
>> which GPIOs, etc. are being used for the sensor.
>>
>> But after the move to using the ACPI handle for logging the logs
>> show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
>> OVTI2680 and INT0310 making it much harder to figure on what
>> is going on (first need to do
>> "cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
>> to find out which path belongs to which sensor).
> 
> In cases such as the above, the developer probably needs to address issues
> not in the sensor driver but in the ACPI tables (or in IPU bridge code). So
> for this reason I'd prefer printing the device path instead of the HID
> (which is also somewhat opaque).
> 
>>
>> So I would rather get rid of the handle based logging, because it
>> is very cumbersome to use.
> 
> The V4L2 async and fwnode frameworks use handles, too, for the same reason.
> 
> That said, I don't mind printing device names either. AFAIR Laurent
> actually proposed that recently for the V4L2 fwnode and even promised to
> send a patch. :-)

Hmm, ok. I'll keep the acpi_handle logging then and add a " %s:", dev_name() to
the logs so that we log both the ACPI handle path and the dev-name / HID.

>> I'll add an extra patch to the next version of the set to switch all
>> the logging to using the acpi_device for logging.

So this extra patch is going to add logging of the dev_name() instead then.

Regards,

Hans

Re: [PATCH v2 13/15] media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()

[Sakari Ailus]

Hi Hans,

On Wed, Jul 05, 2023 at 12:51:50PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/5/23 12:38, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Tue, Jul 04, 2023 at 09:21:47PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 6/30/23 16:45, Andy Shevchenko wrote:
> >>>> +       sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
> >>>> +       if (sensor->lanes > IPU_MAX_LANES) {
> >>>> +               dev_err(&adev->dev, "Invalid lane-count: %d\n", sensor->lanes);
> >>>
> >>> Yeah, I think we would be consistent in using the ACPI handle to print
> >>> the messages from ACPI sensor devices.
> >>
> >> I do agree that we need to be consistent, but I regret having switched
> >> to using the handle for this in the csi2-bridge code. The dmesg logs
> >> this results in are much harder to read. Most devices typically have
> >> 2 different sensors and normally it is quite easy to see in the logs
> >> which GPIOs, etc. are being used for the sensor.
> >>
> >> But after the move to using the ACPI handle for logging the logs
> >> show up prefixed with \_SB_.I2C2.CAM8 resp CAM2 rather then with
> >> OVTI2680 and INT0310 making it much harder to figure on what
> >> is going on (first need to do
> >> "cat /sys/bus/i2c/devices/i2c-OVTI2680:00/firmware_node/path"
> >> to find out which path belongs to which sensor).
> > 
> > In cases such as the above, the developer probably needs to address issues
> > not in the sensor driver but in the ACPI tables (or in IPU bridge code). So
> > for this reason I'd prefer printing the device path instead of the HID
> > (which is also somewhat opaque).
> > 
> >>
> >> So I would rather get rid of the handle based logging, because it
> >> is very cumbersome to use.
> > 
> > The V4L2 async and fwnode frameworks use handles, too, for the same reason.
> > 
> > That said, I don't mind printing device names either. AFAIR Laurent
> > actually proposed that recently for the V4L2 fwnode and even promised to
> > send a patch. :-)
> 
> Hmm, ok. I'll keep the acpi_handle logging then and add a " %s:", dev_name() to
> the logs so that we log both the ACPI handle path and the dev-name / HID.
> 
> >> I'll add an extra patch to the next version of the set to switch all
> >> the logging to using the acpi_device for logging.
> 
> So this extra patch is going to add logging of the dev_name() instead then.

Sounds good to me.

-- 
Sakari Ailus

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Sakari Ailus]

Hi Hans,

On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> On 7/4/23 12:49, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
> >> The GalaxyCore GC0310 is used together with the atomisp no various
> >> devices, add it to ipu_supported_sensors[].
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> >> index eb7c56e8ef9f..07a34f20af8e 100644
> >> --- a/drivers/media/pci/intel/ipu-bridge.c
> >> +++ b/drivers/media/pci/intel/ipu-bridge.c
> >> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
> >>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
> >>  	/* Omnivision ov13b10 */
> >>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> >> +	/* GalaxyCore GC0310 */
> >> +	IPU_SENSOR_CONFIG("INT0310", 0),
> > 
> > 
> > Where is this HID from? A DSDT somewhere??
> 
> This is used in many atomisp DSDTs on many atomisp devices,
> see below for a quick grep to my no disk "database" of DSDTs.
> 
> This is also already used by the existing gc0310 driver for
> atomisp:
> 
> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> 
> Note with the latest media_stage.git branch, which has
> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
> 
> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
> 
> The gc0310 driver is now a fully standard v4l2 driver no longer using any
> atomisp-isms / custom API. As such I have a patch pending to move
> it to drivers/media/i2c :
> 
> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
> 
> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
> yourself, this should be applied on top of the latest media_stage/master
> not on top of 6.4-rc1 .

Ack, thanks for the info. This seems to be from a different range than the
newer ones with INT34 prefix. Must be an older allocation I guess.

> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> The promised list of matching DSDT-s. Note not all of these necessary actually
> use the sensor (but some do) let me know if you want a DSDT of a device which
> actually uses the GC0310 :

With the above explanation, I have no concerns.

Thanks.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

Hi,

On 7/5/23 13:38, Sakari Ailus wrote:
> Hi Hans,
> 
> On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
>> Hi Sakari,
>>
>> On 7/4/23 12:49, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
>>>> The GalaxyCore GC0310 is used together with the atomisp no various
>>>> devices, add it to ipu_supported_sensors[].
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>>>> index eb7c56e8ef9f..07a34f20af8e 100644
>>>> --- a/drivers/media/pci/intel/ipu-bridge.c
>>>> +++ b/drivers/media/pci/intel/ipu-bridge.c
>>>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>>>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>>>>  	/* Omnivision ov13b10 */
>>>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
>>>> +	/* GalaxyCore GC0310 */
>>>> +	IPU_SENSOR_CONFIG("INT0310", 0),
>>>
>>>
>>> Where is this HID from? A DSDT somewhere??
>>
>> This is used in many atomisp DSDTs on many atomisp devices,
>> see below for a quick grep to my no disk "database" of DSDTs.
>>
>> This is also already used by the existing gc0310 driver for
>> atomisp:
>>
>> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>>
>> Note with the latest media_stage.git branch, which has
>> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
>>
>> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
>>
>> The gc0310 driver is now a fully standard v4l2 driver no longer using any
>> atomisp-isms / custom API. As such I have a patch pending to move
>> it to drivers/media/i2c :
>>
>> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
>>
>> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
>> yourself, this should be applied on top of the latest media_stage/master
>> not on top of 6.4-rc1 .
> 
> Ack, thanks for the info.

Does this count as your Acked-by for the patch to mv gc0310.c to
drivers/media/i2c in my next media-atomisp pull-req ?

or ... ?

> This seems to be from a different range than the
> newer ones with INT34 prefix. Must be an older allocation I guess.

Right.

Regards,

Hans

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Sakari Ailus]

Hi Hans,

On Wed, Jul 05, 2023 at 01:50:27PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/5/23 13:38, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
> >> Hi Sakari,
> >>
> >> On 7/4/23 12:49, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
> >>>> The GalaxyCore GC0310 is used together with the atomisp no various
> >>>> devices, add it to ipu_supported_sensors[].
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
> >>>>  1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> >>>> index eb7c56e8ef9f..07a34f20af8e 100644
> >>>> --- a/drivers/media/pci/intel/ipu-bridge.c
> >>>> +++ b/drivers/media/pci/intel/ipu-bridge.c
> >>>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
> >>>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
> >>>>  	/* Omnivision ov13b10 */
> >>>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> >>>> +	/* GalaxyCore GC0310 */
> >>>> +	IPU_SENSOR_CONFIG("INT0310", 0),
> >>>
> >>>
> >>> Where is this HID from? A DSDT somewhere??
> >>
> >> This is used in many atomisp DSDTs on many atomisp devices,
> >> see below for a quick grep to my no disk "database" of DSDTs.
> >>
> >> This is also already used by the existing gc0310 driver for
> >> atomisp:
> >>
> >> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> >>
> >> Note with the latest media_stage.git branch, which has
> >> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
> >>
> >> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
> >>
> >> The gc0310 driver is now a fully standard v4l2 driver no longer using any
> >> atomisp-isms / custom API. As such I have a patch pending to move
> >> it to drivers/media/i2c :
> >>
> >> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
> >>
> >> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
> >> yourself, this should be applied on top of the latest media_stage/master
> >> not on top of 6.4-rc1 .
> > 
> > Ack, thanks for the info.
> 
> Does this count as your Acked-by for the patch to mv gc0310.c to
> drivers/media/i2c in my next media-atomisp pull-req ?
> 
> or ... ?

Can I take the set once others think it's good? I have Wentong's patch that
goes on top.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

Hi,

On 7/5/23 14:10, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Jul 05, 2023 at 01:50:27PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/5/23 13:38, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
>>>> Hi Sakari,
>>>>
>>>> On 7/4/23 12:49, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
>>>>>> The GalaxyCore GC0310 is used together with the atomisp no various
>>>>>> devices, add it to ipu_supported_sensors[].
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>>>>>> index eb7c56e8ef9f..07a34f20af8e 100644
>>>>>> --- a/drivers/media/pci/intel/ipu-bridge.c
>>>>>> +++ b/drivers/media/pci/intel/ipu-bridge.c
>>>>>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>>>>>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>>>>>>  	/* Omnivision ov13b10 */
>>>>>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
>>>>>> +	/* GalaxyCore GC0310 */
>>>>>> +	IPU_SENSOR_CONFIG("INT0310", 0),
>>>>>
>>>>>
>>>>> Where is this HID from? A DSDT somewhere??
>>>>
>>>> This is used in many atomisp DSDTs on many atomisp devices,
>>>> see below for a quick grep to my no disk "database" of DSDTs.
>>>>
>>>> This is also already used by the existing gc0310 driver for
>>>> atomisp:
>>>>
>>>> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>>>>
>>>> Note with the latest media_stage.git branch, which has
>>>> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
>>>>
>>>> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
>>>>
>>>> The gc0310 driver is now a fully standard v4l2 driver no longer using any
>>>> atomisp-isms / custom API. As such I have a patch pending to move
>>>> it to drivers/media/i2c :
>>>>
>>>> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
>>>>
>>>> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
>>>> yourself, this should be applied on top of the latest media_stage/master
>>>> not on top of 6.4-rc1 .
>>>
>>> Ack, thanks for the info.
>>
>> Does this count as your Acked-by for the patch to mv gc0310.c to
>> drivers/media/i2c in my next media-atomisp pull-req ?
>>
>> or ... ?
> 
> Can I take the set once others think it's good? I have Wentong's patch that
> goes on top.

Yes you can and I'm expecting you to take this set, the question about if
your "Ack" as your Acked-by is about:

https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/

which is not part of the set.

I hope this helps clarify our little mis-communication which we seem to
be heaving here.

Regards,

Hans

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Sakari Ailus]

Hi Hans,

On Wed, Jul 05, 2023 at 02:33:30PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 7/5/23 14:10, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Jul 05, 2023 at 01:50:27PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 7/5/23 13:38, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
> >>>> Hi Sakari,
> >>>>
> >>>> On 7/4/23 12:49, Sakari Ailus wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
> >>>>>> The GalaxyCore GC0310 is used together with the atomisp no various
> >>>>>> devices, add it to ipu_supported_sensors[].
> >>>>>>
> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>> ---
> >>>>>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
> >>>>>>  1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> >>>>>> index eb7c56e8ef9f..07a34f20af8e 100644
> >>>>>> --- a/drivers/media/pci/intel/ipu-bridge.c
> >>>>>> +++ b/drivers/media/pci/intel/ipu-bridge.c
> >>>>>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
> >>>>>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
> >>>>>>  	/* Omnivision ov13b10 */
> >>>>>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
> >>>>>> +	/* GalaxyCore GC0310 */
> >>>>>> +	IPU_SENSOR_CONFIG("INT0310", 0),
> >>>>>
> >>>>>
> >>>>> Where is this HID from? A DSDT somewhere??
> >>>>
> >>>> This is used in many atomisp DSDTs on many atomisp devices,
> >>>> see below for a quick grep to my no disk "database" of DSDTs.
> >>>>
> >>>> This is also already used by the existing gc0310 driver for
> >>>> atomisp:
> >>>>
> >>>> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> >>>>
> >>>> Note with the latest media_stage.git branch, which has
> >>>> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
> >>>>
> >>>> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
> >>>>
> >>>> The gc0310 driver is now a fully standard v4l2 driver no longer using any
> >>>> atomisp-isms / custom API. As such I have a patch pending to move
> >>>> it to drivers/media/i2c :
> >>>>
> >>>> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
> >>>>
> >>>> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
> >>>> yourself, this should be applied on top of the latest media_stage/master
> >>>> not on top of 6.4-rc1 .
> >>>
> >>> Ack, thanks for the info.
> >>
> >> Does this count as your Acked-by for the patch to mv gc0310.c to
> >> drivers/media/i2c in my next media-atomisp pull-req ?
> >>
> >> or ... ?
> > 
> > Can I take the set once others think it's good? I have Wentong's patch that
> > goes on top.
> 
> Yes you can and I'm expecting you to take this set, the question about if
> your "Ack" as your Acked-by is about:
> 
> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
> 
> which is not part of the set.
> 
> I hope this helps clarify our little mis-communication which we seem to
> be heaving here.

Sorry, I somehow ignored this part of your original message.

I'll review the set soon.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

Hi,

On 7/5/23 15:28, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Jul 05, 2023 at 02:33:30PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 7/5/23 14:10, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Jul 05, 2023 at 01:50:27PM +0200, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 7/5/23 13:38, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> On 7/4/23 12:49, Sakari Ailus wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> On Fri, Jun 30, 2023 at 01:06:40PM +0200, Hans de Goede wrote:
>>>>>>>> The GalaxyCore GC0310 is used together with the atomisp no various
>>>>>>>> devices, add it to ipu_supported_sensors[].
>>>>>>>>
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> ---
>>>>>>>>  drivers/media/pci/intel/ipu-bridge.c | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
>>>>>>>> index eb7c56e8ef9f..07a34f20af8e 100644
>>>>>>>> --- a/drivers/media/pci/intel/ipu-bridge.c
>>>>>>>> +++ b/drivers/media/pci/intel/ipu-bridge.c
>>>>>>>> @@ -36,6 +36,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>>>>>>>>  	IPU_SENSOR_CONFIG("INT3537", 1, 437000000),
>>>>>>>>  	/* Omnivision ov13b10 */
>>>>>>>>  	IPU_SENSOR_CONFIG("OVTIDB10", 1, 560000000),
>>>>>>>> +	/* GalaxyCore GC0310 */
>>>>>>>> +	IPU_SENSOR_CONFIG("INT0310", 0),
>>>>>>>
>>>>>>>
>>>>>>> Where is this HID from? A DSDT somewhere??
>>>>>>
>>>>>> This is used in many atomisp DSDTs on many atomisp devices,
>>>>>> see below for a quick grep to my no disk "database" of DSDTs.
>>>>>>
>>>>>> This is also already used by the existing gc0310 driver for
>>>>>> atomisp:
>>>>>>
>>>>>> https://git.linuxtv.org/media_stage.git/tree/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>>>>>>
>>>>>> Note with the latest media_stage.git branch, which has
>>>>>> my "[GIT PULL] media: atomisp: Changes for 6.5-1" merged:
>>>>>>
>>>>>> https://lore.kernel.org/linux-media/4177be8a-7a77-c452-7b98-91d5e5af8e8b@redhat.com/
>>>>>>
>>>>>> The gc0310 driver is now a fully standard v4l2 driver no longer using any
>>>>>> atomisp-isms / custom API. As such I have a patch pending to move
>>>>>> it to drivers/media/i2c :
>>>>>>
>>>>>> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
>>>>>>
>>>>>> that patch is waiting for you to ack (or merge) it. Note if you chose to merge this
>>>>>> yourself, this should be applied on top of the latest media_stage/master
>>>>>> not on top of 6.4-rc1 .
>>>>>
>>>>> Ack, thanks for the info.
>>>>
>>>> Does this count as your Acked-by for the patch to mv gc0310.c to
>>>> drivers/media/i2c in my next media-atomisp pull-req ?
>>>>
>>>> or ... ?
>>>
>>> Can I take the set once others think it's good? I have Wentong's patch that
>>> goes on top.
>>
>> Yes you can and I'm expecting you to take this set, the question about if
>> your "Ack" as your Acked-by is about:
>>
>> https://lore.kernel.org/linux-media/20230525190100.130010-6-hdegoede@redhat.com/
>>
>> which is not part of the set.
>>
>> I hope this helps clarify our little mis-communication which we seem to
>> be heaving here.
> 
> Sorry, I somehow ignored this part of your original message.
> 
> I'll review the set soon.

Thanks.

Note patches 1-4 of that set are already in media-staging. Only 5/5 is still
pending, I did not send that directly to Mauro because it touches files
outside driver/staging/media/atomisp.

Regards,

Hans

Re: [PATCH v2 12/15] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:38:51AM +0000, Sakari Ailus wrote:
> On Tue, Jul 04, 2023 at 12:59:53PM +0200, Hans de Goede wrote:

...

> Ack, thanks for the info. This seems to be from a different range than
> the newer ones with INT34 prefix. Must be an older allocation I guess.

I believe it is from Intel Medfield times. Maybe there were even phones on
marked with that sensor.

-- 
With Best Regards,
Andy Shevchenko