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

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

[Hans de Goede]

Hi All,

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

This series also rework VCM instantiation, which was my
initial reason for unifying / sharing the code.

Rafael, this v3 now includes a small ACPI patch:
  ACPI: bus: Introduce acpi_match_acpi_device() function

It is probably easiest if you can give an ack for merging this
through the media tree. May we have your ack for this?

Sakari, I know that you have other pending patches depending
on this, patches 1-14 can be merged independent of the new
ACPI patch. Only the atomisp changes require that and those
can also be merged later.

Changes in v3:
- New patches:
  media: i2c: Add driver for DW9719 VCM
  ACPI: bus: Introduce acpi_match_acpi_device() function
  media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging
  media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation
- media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init():
  - Add a table with per sensor-HID atomisp_sensor_config settings
    for sensors which have lanes != 1 or which may have a VCM
    (VCM support is added in a follow-up patch)
  - Switch to acpi_handle_err() for logging errors
  - Set orientation based on CSI link/port

This set now consists of the following parts:

Patches 1-4  Bugfixes for recent changes
Patches 5-12 Cleanup / preparation patches
Patch 13     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 14     Resubmit DW9719 VCM driver upstream now that Vsio hack is gone
Patch 15     New ACPI helper needed by atomisp bridge code
Patch 16     Drop ipu-bridge code copy from atomisp, switching to
             the shared ipu-bridge module
Patch 17-18  Further atomisp bridge code improvements

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.

Regards,

Hans


Daniel Scally (1):
  media: i2c: Add driver for DW9719 VCM

Hans de Goede (17):
  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: ipu-bridge: Add a runtime-pm device-link between VCM and sensor
  ACPI: bus: Introduce acpi_match_acpi_device() function
  media: atomisp: csi2-bridge: Switch to new common ipu_bridge_init()
  media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info()
    logging
  media: atomisp: csi2-bridge: Add support for VCM I2C-client
    instantiation

 MAINTAINERS                                   |   7 +
 drivers/acpi/bus.c                            |  22 +-
 drivers/media/i2c/Kconfig                     |  11 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/dw9719.c                    | 427 ++++++++++++++++++
 drivers/media/pci/intel/Kconfig               |  18 +-
 drivers/media/pci/intel/ipu-bridge.c          | 333 ++++++++------
 drivers/media/pci/intel/ipu3/Kconfig          |  20 +
 drivers/media/pci/intel/ipu3/ipu3-cio2.c      |  10 +-
 drivers/staging/media/atomisp/Kconfig         |   3 +
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ---
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 424 ++++++-----------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 include/acpi/acpi_bus.h                       |   2 +
 .../pci/intel => include/media}/ipu-bridge.h  |  27 +-
 15 files changed, 851 insertions(+), 522 deletions(-)
 create mode 100644 drivers/media/i2c/dw9719.c
 rename {drivers/media/pci/intel => include/media}/ipu-bridge.h (80%)

-- 
2.41.0

[PATCH v3 01/18] 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

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

[Dan Scally]

Hi Hans

On 05/07/2023 22:29, Hans de Goede 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.
>
> 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>
> ---

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


And same for the corresponding 09/18

>   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,

[PATCH v3 02/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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 v3 03/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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 v3 04/18] 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 built in
even if all 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      | 18 ++----------------
 drivers/media/pci/intel/ipu-bridge.c |  4 ++++
 drivers/media/pci/intel/ipu3/Kconfig | 20 ++++++++++++++++++++
 3 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/media/pci/intel/Kconfig b/drivers/media/pci/intel/Kconfig
index 64a29b0b7033..3179184d7616 100644
--- a/drivers/media/pci/intel/Kconfig
+++ b/drivers/media/pci/intel/Kconfig
@@ -1,21 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 config IPU_BRIDGE
-	bool "Intel IPU Sensors Bridge"
-	depends on VIDEO_IPU3_CIO2 && ACPI
+	tristate
+	depends on 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.
 
 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

Re: [PATCH v3 04/18] media: ipu-bridge: Allow building as module

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:29:56PM +0200, Hans de Goede wrote:
> After commit f54eb0ac7c1a ("media: ipu3-cio2: rename cio2 bridge to ipu
> bridge and move out of ipu3") the ipu-bridge code is always built in
> even if all 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.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

> 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      | 18 ++----------------
>  drivers/media/pci/intel/ipu-bridge.c |  4 ++++
>  drivers/media/pci/intel/ipu3/Kconfig | 20 ++++++++++++++++++++
>  3 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/Kconfig b/drivers/media/pci/intel/Kconfig
> index 64a29b0b7033..3179184d7616 100644
> --- a/drivers/media/pci/intel/Kconfig
> +++ b/drivers/media/pci/intel/Kconfig
> @@ -1,21 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config IPU_BRIDGE
> -	bool "Intel IPU Sensors Bridge"
> -	depends on VIDEO_IPU3_CIO2 && ACPI
> +	tristate
> +	depends on 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.
>  
>  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
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 05/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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 v3 06/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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 v3 07/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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..11cfab641513 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 default 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 v3 08/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
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 11cfab641513..ee0ca5e6e33b 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 v3 09/18] 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 ee0ca5e6e33b..1f01f2c8b8a6 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 v3 10/18] 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>
---
Changes in v3:
- Add ipu_parse_sensor_fwnode_t type for the callback function
---
 drivers/media/pci/intel/ipu-bridge.c     | 10 ++++++----
 drivers/media/pci/intel/ipu-bridge.h     | 11 +++++++++--
 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 1f01f2c8b8a6..db67a75ae1b7 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,8 @@ static int ipu_bridge_sensors_are_ready(void)
 	return ready;
 }
 
-int ipu_bridge_init(struct device *dev)
+int ipu_bridge_init(struct device *dev,
+		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode)
 {
 	struct fwnode_handle *fwnode;
 	struct ipu_bridge *bridge;
@@ -459,6 +460,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..7d84b22b2111 100644
--- a/drivers/media/pci/intel/ipu-bridge.h
+++ b/drivers/media/pci/intel/ipu-bridge.h
@@ -140,8 +140,12 @@ struct ipu_sensor {
 	struct software_node_ref_args vcm_ref[1];
 };
 
+typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
+					 struct ipu_sensor *sensor);
+
 struct ipu_bridge {
 	struct device *dev;
+	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
 	char ipu_node_name[ACPI_ID_LEN];
 	struct software_node ipu_hid_node;
 	u32 data_lanes[4];
@@ -150,9 +154,12 @@ struct ipu_bridge {
 };
 
 #if IS_ENABLED(CONFIG_IPU_BRIDGE)
-int ipu_bridge_init(struct device *dev);
+int ipu_bridge_init(struct device *dev,
+		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode);
+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 argument 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

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

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:30:02PM +0200, Hans de Goede wrote:
> 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.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3:
> - Add ipu_parse_sensor_fwnode_t type for the callback function
> ---
>  drivers/media/pci/intel/ipu-bridge.c     | 10 ++++++----
>  drivers/media/pci/intel/ipu-bridge.h     | 11 +++++++++--
>  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 1f01f2c8b8a6..db67a75ae1b7 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,8 @@ static int ipu_bridge_sensors_are_ready(void)
>  	return ready;
>  }
>  
> -int ipu_bridge_init(struct device *dev)
> +int ipu_bridge_init(struct device *dev,
> +		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode)
>  {
>  	struct fwnode_handle *fwnode;
>  	struct ipu_bridge *bridge;
> @@ -459,6 +460,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..7d84b22b2111 100644
> --- a/drivers/media/pci/intel/ipu-bridge.h
> +++ b/drivers/media/pci/intel/ipu-bridge.h
> @@ -140,8 +140,12 @@ struct ipu_sensor {
>  	struct software_node_ref_args vcm_ref[1];
>  };
>  
> +typedef int (*ipu_parse_sensor_fwnode_t)(struct acpi_device *adev,
> +					 struct ipu_sensor *sensor);
> +
>  struct ipu_bridge {
>  	struct device *dev;
> +	ipu_parse_sensor_fwnode_t parse_sensor_fwnode;
>  	char ipu_node_name[ACPI_ID_LEN];
>  	struct software_node ipu_hid_node;
>  	u32 data_lanes[4];
> @@ -150,9 +154,12 @@ struct ipu_bridge {
>  };
>  
>  #if IS_ENABLED(CONFIG_IPU_BRIDGE)
> -int ipu_bridge_init(struct device *dev);
> +int ipu_bridge_init(struct device *dev,
> +		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode);
> +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 argument 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
> 

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 11/18] 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.

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 db67a75ae1b7..277711db867a 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 v3 12/18] media: ipu-bridge: Add GalaxyCore GC0310 to ipu_supported_sensors[]

[Hans de Goede]

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

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.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 277711db867a..6f7650a3a199 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 v3 13/18] 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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Tested-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
-Rename local variables named work to data to avoid having
 work->work (now data->work)
-Use snprintf "%pfwP" to set type to fwnode name
---
 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 6f7650a3a199..0e506ab541e8 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -4,7 +4,10 @@
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
+#include <linux/pm_runtime.h>
 #include <linux/property.h>
+#include <linux/string.h>
+#include <linux/workqueue.h>
 
 #include <media/ipu-bridge.h>
 #include <media/v4l2-fwnode.h>
@@ -289,29 +292,111 @@ 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 *data =
+		container_of(work, struct ipu_bridge_instantiate_vcm_work_data,
+			     work);
+	struct acpi_device *adev = ACPI_COMPANION(data->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(data->sensor);
+	if (ret < 0) {
+		dev_err(data->sensor, "Error %d runtime-resuming sensor, cannot instantiate VCM\n",
+			ret);
+		goto out_pm_put;
 	}
+
+	/*
+	 * 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, &data->board_info);
+	if (IS_ERR(vcm_client)) {
+		dev_err(data->sensor, "Error instantiating VCM client: %ld\n",
+			PTR_ERR(vcm_client));
+		goto out_pm_put;
+	}
+
+	device_link_add(&vcm_client->dev, data->sensor, DL_FLAG_PM_RUNTIME);
+
+	dev_info(data->sensor, "Instantiated %s VCM\n", data->board_info.type);
+	put_fwnode = false; /* Ownership has passed to the i2c-client */
+
+out_pm_put:
+	pm_runtime_put(data->sensor);
+	put_device(data->sensor);
+	if (put_fwnode)
+		fwnode_handle_put(data->board_info.fwnode);
+	kfree(data);
 }
 
+int ipu_bridge_instantiate_vcm(struct device *sensor)
+{
+	struct ipu_bridge_instantiate_vcm_work_data *data;
+	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;
+	}
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		fwnode_handle_put(vcm_fwnode);
+		return -ENOMEM;
+	}
+
+	INIT_WORK(&data->work, ipu_bridge_instantiate_vcm_work);
+	data->sensor = get_device(sensor);
+	snprintf(data->name, sizeof(data->name), "%s-VCM",
+		 acpi_dev_name(adev));
+	data->board_info.dev_name = data->name;
+	data->board_info.fwnode = vcm_fwnode;
+	snprintf(data->board_info.type, sizeof(data->board_info.type),
+		 "%pfwP", vcm_fwnode);
+	/* Strip "-<link>" postfix */
+	sep = strchrnul(data->board_info.type, '-');
+	*sep = 0;
+
+	queue_work(system_long_wq, &data->work);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(ipu_bridge_instantiate_vcm, INTEL_IPU_BRIDGE);
+
 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,
 		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode)
 {
@@ -451,9 +499,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 7d84b22b2111..ceda2a801948 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];
@@ -157,9 +154,11 @@ struct ipu_bridge {
 int ipu_bridge_init(struct device *dev,
 		    ipu_parse_sensor_fwnode_t parse_sensor_fwnode);
 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 argument 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 v3 14/18] media: i2c: Add driver for DW9719 VCM

[Hans de Goede]

From: Daniel Scally <djrscally@gmail.com>

Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
and registers a control to set the desired focus.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3 (Hans de Goede)
- New patch in v3 of this series based on Dan Scally's initial
  DW9719 upstream submission:
  https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/
- Drop hack to enable "vsio" regulator, this is no longer necessary
  now that there is a device-link making the VCM a runtime-pm consumer
  of the sensor
- Add checking of device-properties for sac-mode and vcm-freq,
  as requested by Sakari, this is done similar to the dw9768:
  Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
  Note no devicetree binding doc is added since currently only
  i2c_device_id enumeration (instantiated by IPU bridge) is
  supported
---
 MAINTAINERS                |   7 +
 drivers/media/i2c/Kconfig  |  11 +
 drivers/media/i2c/Makefile |   1 +
 drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
 4 files changed, 446 insertions(+)
 create mode 100644 drivers/media/i2c/dw9719.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 494682dd437f..cf8e799f6ea2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6266,6 +6266,13 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml
 F:	drivers/media/i2c/dw9714.c
 
+DONGWOON DW9719 LENS VOICE COIL DRIVER
+M:	Daniel Scally <djrscally@gmail.com>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+T:	git git://linuxtv.org/media_tree.git
+F:	drivers/media/i2c/dw9719.c
+
 DONGWOON DW9768 LENS VOICE COIL DRIVER
 L:	linux-media@vger.kernel.org
 S:	Orphan
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 26dc365365d8..4864f1df3c7a 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -875,6 +875,17 @@ config VIDEO_DW9714
 	  capability. This is designed for linear control of
 	  voice coil motors, controlled via I2C serial interface.
 
+config VIDEO_DW9719
+	tristate "DW9719 lens voice coil support"
+	depends on I2C && VIDEO_DEV
+	select MEDIA_CONTROLLER
+	select VIDEO_V4L2_SUBDEV_API
+	select V4L2_ASYNC
+	help
+	  This is a driver for the DW9719 camera lens voice coil.
+	  This is designed for linear control of voice coil motors,
+	  controlled via I2C serial interface.
+
 config VIDEO_DW9768
 	tristate "DW9768 lens voice coil support"
 	depends on I2C && VIDEO_DEV
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index d175a2e2fb19..745f8d07e649 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o
 obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
 obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o
 obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
+obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
 obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
 obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
new file mode 100644
index 000000000000..7b83ae102131
--- /dev/null
+++ b/drivers/media/i2c/dw9719.c
@@ -0,0 +1,427 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2012 Intel Corporation
+
+/*
+ * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
+ * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
+ */
+
+#include <asm/unaligned.h>
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+#include <linux/types.h>
+
+#include <media/v4l2-common.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+#define DW9719_MAX_FOCUS_POS	1023
+#define DW9719_CTRL_STEPS	16
+#define DW9719_CTRL_DELAY_US	1000
+#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
+
+#define DW9719_INFO			0
+#define DW9719_ID			0xF1
+#define DW9719_CONTROL			2
+#define DW9719_VCM_CURRENT		3
+
+#define DW9719_MODE			6
+#define DW9719_VCM_FREQ			7
+
+#define DW9719_MODE_SAC_SHIFT		4
+#define DW9719_MODE_SAC3		4
+
+#define DW9719_DEFAULT_VCM_FREQ		0x60
+
+#define DW9719_ENABLE_RINGING		0x02
+
+#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
+
+struct dw9719_device {
+	struct device *dev;
+	struct i2c_client *client;
+	struct regulator *regulator;
+	struct v4l2_subdev sd;
+	u32 sac_mode;
+	u32 vcm_freq;
+
+	struct dw9719_v4l2_ctrls {
+		struct v4l2_ctrl_handler handler;
+		struct v4l2_ctrl *focus;
+	} ctrls;
+};
+
+static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
+{
+	struct i2c_msg msg[2];
+	u8 buf[2] = { reg };
+	int ret;
+
+	msg[0].addr = client->addr;
+	msg[0].flags = 0;
+	msg[0].len = 1;
+	msg[0].buf = buf;
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].len = 1;
+	msg[1].buf = &buf[1];
+	*val = 0;
+
+	ret = i2c_transfer(client->adapter, msg, 2);
+	if (ret < 0)
+		return ret;
+
+	*val = buf[1];
+
+	return 0;
+}
+
+static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
+{
+	struct i2c_msg msg;
+	int ret;
+
+	u8 buf[2] = { reg, val };
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = sizeof(buf);
+	msg.buf = buf;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
+{
+	struct i2c_msg msg;
+	u8 buf[3] = { reg };
+	int ret;
+
+	put_unaligned_be16(val, buf + 1);
+
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = sizeof(buf);
+	msg.buf = buf;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int dw9719_detect(struct dw9719_device *dw9719)
+{
+	int ret;
+	u8 val;
+
+	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
+	if (ret < 0)
+		return ret;
+
+	if (val != DW9719_ID) {
+		dev_err(dw9719->dev, "Failed to detect correct id\n");
+		ret = -ENXIO;
+	}
+
+	return 0;
+}
+
+static int dw9719_power_down(struct dw9719_device *dw9719)
+{
+	return regulator_disable(dw9719->regulator);
+}
+
+static int dw9719_power_up(struct dw9719_device *dw9719)
+{
+	int ret;
+
+	ret = regulator_enable(dw9719->regulator);
+	if (ret)
+		return ret;
+
+	/* Jiggle SCL pin to wake up device */
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
+
+	/* Need 100us to transit from SHUTDOWN to STANDBY*/
+	usleep_range(100, 1000);
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
+			     DW9719_ENABLE_RINGING);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE,
+			     dw9719->sac_mode << DW9719_MODE_SAC_SHIFT);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ, dw9719->vcm_freq);
+	if (ret < 0)
+		goto fail_powerdown;
+
+	return 0;
+
+fail_powerdown:
+	dw9719_power_down(dw9719);
+	return ret;
+}
+
+static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
+{
+	int ret;
+
+	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
+	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct dw9719_device *dw9719 = container_of(ctrl->handler,
+						    struct dw9719_device,
+						    ctrls.handler);
+	int ret;
+
+	/* Only apply changes to the controls if the device is powered up */
+	if (!pm_runtime_get_if_in_use(dw9719->dev))
+		return 0;
+
+	switch (ctrl->id) {
+	case V4L2_CID_FOCUS_ABSOLUTE:
+		ret = dw9719_t_focus_abs(dw9719, ctrl->val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	pm_runtime_put(dw9719->dev);
+
+	return ret;
+}
+
+static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
+	.s_ctrl = dw9719_set_ctrl,
+};
+
+static int __maybe_unused dw9719_suspend(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct dw9719_device *dw9719 = to_dw9719_device(sd);
+	int ret;
+	int val;
+
+	for (val = dw9719->ctrls.focus->val; val >= 0;
+	     val -= DW9719_CTRL_STEPS) {
+		ret = dw9719_t_focus_abs(dw9719, val);
+		if (ret)
+			return ret;
+
+		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
+	}
+
+	return dw9719_power_down(dw9719);
+}
+
+static int __maybe_unused dw9719_resume(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct dw9719_device *dw9719 = to_dw9719_device(sd);
+	int current_focus = dw9719->ctrls.focus->val;
+	int ret;
+	int val;
+
+	ret = dw9719_power_up(dw9719);
+	if (ret)
+		return ret;
+
+	for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
+	     val += DW9719_CTRL_STEPS) {
+		ret = dw9719_t_focus_abs(dw9719, val);
+		if (ret)
+			goto err_power_down;
+
+		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
+	}
+
+	return 0;
+
+err_power_down:
+	dw9719_power_down(dw9719);
+	return ret;
+}
+
+static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	return pm_runtime_resume_and_get(sd->dev);
+}
+
+static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+	pm_runtime_put(sd->dev);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
+	.open = dw9719_open,
+	.close = dw9719_close,
+};
+
+static int dw9719_init_controls(struct dw9719_device *dw9719)
+{
+	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
+	int ret;
+
+	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
+	if (ret)
+		return ret;
+
+	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
+						V4L2_CID_FOCUS_ABSOLUTE, 0,
+						DW9719_MAX_FOCUS_POS, 1, 0);
+
+	if (dw9719->ctrls.handler.error) {
+		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
+		ret = dw9719->ctrls.handler.error;
+		goto err_free_handler;
+	}
+
+	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
+
+	return ret;
+
+err_free_handler:
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+	return ret;
+}
+
+static const struct v4l2_subdev_ops dw9719_ops = { };
+
+static int dw9719_probe(struct i2c_client *client)
+{
+	struct dw9719_device *dw9719;
+	int ret;
+
+	dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
+	if (!dw9719)
+		return -ENOMEM;
+
+	dw9719->client = client;
+	dw9719->dev = &client->dev;
+
+	dw9719->sac_mode = DW9719_MODE_SAC3;
+	dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
+
+	/* Optional indication of SAC mode select */
+	device_property_read_u32(&client->dev, "dongwoon,sac-mode",
+				 &dw9719->sac_mode);
+
+	/* Optional indication of VCM frequency */
+	device_property_read_u32(&client->dev, "dongwoon,vcm-freq",
+				 &dw9719->vcm_freq);
+
+	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;
+	dw9719->sd.internal_ops = &dw9719_internal_ops;
+
+	ret = dw9719_init_controls(dw9719);
+	if (ret)
+		return ret;
+
+	ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
+	if (ret < 0)
+		goto err_free_ctrl_handler;
+
+	dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
+
+	/*
+	 * We need the driver to work in the event that pm runtime is disable in
+	 * the kernel, so power up and verify the chip now. In the event that
+	 * runtime pm is disabled this will leave the chip on, so that the lens
+	 * will work.
+	 */
+
+	ret = dw9719_power_up(dw9719);
+	if (ret)
+		goto err_cleanup_media;
+
+	ret = dw9719_detect(dw9719);
+	if (ret)
+		goto err_powerdown;
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_enable(&client->dev);
+
+	ret = v4l2_async_register_subdev(&dw9719->sd);
+	if (ret < 0)
+		goto err_pm_runtime;
+
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
+
+	return ret;
+
+err_pm_runtime:
+	pm_runtime_disable(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+err_powerdown:
+	dw9719_power_down(dw9719);
+err_cleanup_media:
+	media_entity_cleanup(&dw9719->sd.entity);
+err_free_ctrl_handler:
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+
+	return ret;
+}
+
+static void dw9719_remove(struct i2c_client *client)
+{
+	struct v4l2_subdev *sd = i2c_get_clientdata(client);
+	struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device, sd);
+
+	pm_runtime_disable(&client->dev);
+	v4l2_async_unregister_subdev(sd);
+	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
+	media_entity_cleanup(&dw9719->sd.entity);
+}
+
+static const struct i2c_device_id dw9719_id_table[] = {
+	{ "dw9719" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
+
+static const struct dev_pm_ops dw9719_pm_ops = {
+	SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
+};
+
+static struct i2c_driver dw9719_i2c_driver = {
+	.driver = {
+		.name = "dw9719",
+		.pm = &dw9719_pm_ops,
+	},
+	.probe_new = dw9719_probe,
+	.remove = dw9719_remove,
+	.id_table = dw9719_id_table,
+};
+module_i2c_driver(dw9719_i2c_driver);
+
+MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
+MODULE_DESCRIPTION("DW9719 VCM Driver");
+MODULE_LICENSE("GPL");
-- 
2.41.0

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Sakari Ailus]

Hi Hans,

Thanks for the update.

On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:
> From: Daniel Scally <djrscally@gmail.com>
> 
> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3 (Hans de Goede)
> - New patch in v3 of this series based on Dan Scally's initial
>   DW9719 upstream submission:
>   https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/
> - Drop hack to enable "vsio" regulator, this is no longer necessary
>   now that there is a device-link making the VCM a runtime-pm consumer
>   of the sensor
> - Add checking of device-properties for sac-mode and vcm-freq,
>   as requested by Sakari, this is done similar to the dw9768:
>   Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>   Note no devicetree binding doc is added since currently only
>   i2c_device_id enumeration (instantiated by IPU bridge) is
>   supported
> ---
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 446 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9719.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 494682dd437f..cf8e799f6ea2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6266,6 +6266,13 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml
>  F:	drivers/media/i2c/dw9714.c
>  
> +DONGWOON DW9719 LENS VOICE COIL DRIVER
> +M:	Daniel Scally <djrscally@gmail.com>
> +L:	linux-media@vger.kernel.org
> +S:	Maintained
> +T:	git git://linuxtv.org/media_tree.git
> +F:	drivers/media/i2c/dw9719.c
> +
>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>  L:	linux-media@vger.kernel.org
>  S:	Orphan
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 26dc365365d8..4864f1df3c7a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -875,6 +875,17 @@ config VIDEO_DW9714
>  	  capability. This is designed for linear control of
>  	  voice coil motors, controlled via I2C serial interface.
>  
> +config VIDEO_DW9719
> +	tristate "DW9719 lens voice coil support"
> +	depends on I2C && VIDEO_DEV
> +	select MEDIA_CONTROLLER
> +	select VIDEO_V4L2_SUBDEV_API
> +	select V4L2_ASYNC
> +	help
> +	  This is a driver for the DW9719 camera lens voice coil.
> +	  This is designed for linear control of voice coil motors,
> +	  controlled via I2C serial interface.
> +
>  config VIDEO_DW9768
>  	tristate "DW9768 lens voice coil support"
>  	depends on I2C && VIDEO_DEV
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index d175a2e2fb19..745f8d07e649 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o
>  obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
>  obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o
>  obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> new file mode 100644
> index 000000000000..7b83ae102131
> --- /dev/null
> +++ b/drivers/media/i2c/dw9719.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2012 Intel Corporation
> +
> +/*
> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
> + */
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define DW9719_MAX_FOCUS_POS	1023
> +#define DW9719_CTRL_STEPS	16
> +#define DW9719_CTRL_DELAY_US	1000
> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
> +
> +#define DW9719_INFO			0
> +#define DW9719_ID			0xF1
> +#define DW9719_CONTROL			2
> +#define DW9719_VCM_CURRENT		3
> +
> +#define DW9719_MODE			6
> +#define DW9719_VCM_FREQ			7
> +
> +#define DW9719_MODE_SAC_SHIFT		4
> +#define DW9719_MODE_SAC3		4
> +
> +#define DW9719_DEFAULT_VCM_FREQ		0x60
> +
> +#define DW9719_ENABLE_RINGING		0x02
> +
> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
> +
> +struct dw9719_device {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regulator *regulator;
> +	struct v4l2_subdev sd;
> +	u32 sac_mode;
> +	u32 vcm_freq;
> +
> +	struct dw9719_v4l2_ctrls {
> +		struct v4l2_ctrl_handler handler;
> +		struct v4l2_ctrl *focus;
> +	} ctrls;
> +};
> +
> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +	struct i2c_msg msg[2];
> +	u8 buf[2] = { reg };
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 1;
> +	msg[0].buf = buf;
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = 1;
> +	msg[1].buf = &buf[1];
> +	*val = 0;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = buf[1];
> +
> +	return 0;
> +}
> +
> +static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
> +{
> +	struct i2c_msg msg;
> +	int ret;
> +
> +	u8 buf[2] = { reg, val };
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = sizeof(buf);
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
> +{
> +	struct i2c_msg msg;
> +	u8 buf[3] = { reg };
> +	int ret;
> +
> +	put_unaligned_be16(val, buf + 1);
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = sizeof(buf);
> +	msg.buf = buf;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +	return ret < 0 ? ret : 0;
> +}

Seems like a use case for the new CCI register access framework. :-)

Doesn't need to be on this set, though: there are lot of other drivers that
could be converted as well.

> +
> +static int dw9719_detect(struct dw9719_device *dw9719)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != DW9719_ID) {
> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
> +		ret = -ENXIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dw9719_power_down(struct dw9719_device *dw9719)
> +{
> +	return regulator_disable(dw9719->regulator);
> +}
> +
> +static int dw9719_power_up(struct dw9719_device *dw9719)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(dw9719->regulator);
> +	if (ret)
> +		return ret;
> +
> +	/* Jiggle SCL pin to wake up device */
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
> +
> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/
> +	usleep_range(100, 1000);
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
> +			     DW9719_ENABLE_RINGING);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE,
> +			     dw9719->sac_mode << DW9719_MODE_SAC_SHIFT);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ, dw9719->vcm_freq);
> +	if (ret < 0)
> +		goto fail_powerdown;
> +
> +	return 0;
> +
> +fail_powerdown:
> +	dw9719_power_down(dw9719);
> +	return ret;
> +}
> +
> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> +{
> +	int ret;
> +
> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct dw9719_device *dw9719 = container_of(ctrl->handler,
> +						    struct dw9719_device,
> +						    ctrls.handler);
> +	int ret;
> +
> +	/* Only apply changes to the controls if the device is powered up */
> +	if (!pm_runtime_get_if_in_use(dw9719->dev))
> +		return 0;
> +
> +	switch (ctrl->id) {
> +	case V4L2_CID_FOCUS_ABSOLUTE:
> +		ret = dw9719_t_focus_abs(dw9719, ctrl->val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	pm_runtime_put(dw9719->dev);
> +
> +	return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
> +	.s_ctrl = dw9719_set_ctrl,
> +};
> +
> +static int __maybe_unused dw9719_suspend(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int ret;
> +	int val;
> +
> +	for (val = dw9719->ctrls.focus->val; val >= 0;
> +	     val -= DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			return ret;
> +
> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +	}
> +
> +	return dw9719_power_down(dw9719);
> +}
> +
> +static int __maybe_unused dw9719_resume(struct device *dev)
> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int current_focus = dw9719->ctrls.focus->val;
> +	int ret;
> +	int val;
> +
> +	ret = dw9719_power_up(dw9719);
> +	if (ret)
> +		return ret;
> +
> +	for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
> +	     val += DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			goto err_power_down;
> +
> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +	}
> +
> +	return 0;
> +
> +err_power_down:
> +	dw9719_power_down(dw9719);
> +	return ret;
> +}
> +
> +static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +	pm_runtime_put(sd->dev);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
> +	.open = dw9719_open,
> +	.close = dw9719_close,
> +};
> +
> +static int dw9719_init_controls(struct dw9719_device *dw9719)
> +{
> +	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
> +	int ret;
> +
> +	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
> +	if (ret)
> +		return ret;

This check can be dropped.

> +
> +	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
> +						V4L2_CID_FOCUS_ABSOLUTE, 0,
> +						DW9719_MAX_FOCUS_POS, 1, 0);
> +
> +	if (dw9719->ctrls.handler.error) {
> +		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
> +		ret = dw9719->ctrls.handler.error;
> +		goto err_free_handler;
> +	}
> +
> +	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
> +
> +	return ret;
> +
> +err_free_handler:
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +	return ret;
> +}
> +
> +static const struct v4l2_subdev_ops dw9719_ops = { };
> +
> +static int dw9719_probe(struct i2c_client *client)
> +{
> +	struct dw9719_device *dw9719;
> +	int ret;
> +
> +	dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
> +	if (!dw9719)
> +		return -ENOMEM;
> +
> +	dw9719->client = client;
> +	dw9719->dev = &client->dev;
> +
> +	dw9719->sac_mode = DW9719_MODE_SAC3;
> +	dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
> +
> +	/* Optional indication of SAC mode select */
> +	device_property_read_u32(&client->dev, "dongwoon,sac-mode",
> +				 &dw9719->sac_mode);
> +
> +	/* Optional indication of VCM frequency */
> +	device_property_read_u32(&client->dev, "dongwoon,vcm-freq",
> +				 &dw9719->vcm_freq);
> +
> +	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;
> +	dw9719->sd.internal_ops = &dw9719_internal_ops;
> +
> +	ret = dw9719_init_controls(dw9719);
> +	if (ret)
> +		return ret;
> +
> +	ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
> +	if (ret < 0)
> +		goto err_free_ctrl_handler;
> +
> +	dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +	/*
> +	 * We need the driver to work in the event that pm runtime is disable in
> +	 * the kernel, so power up and verify the chip now. In the event that
> +	 * runtime pm is disabled this will leave the chip on, so that the lens
> +	 * will work.
> +	 */
> +
> +	ret = dw9719_power_up(dw9719);
> +	if (ret)
> +		goto err_cleanup_media;
> +
> +	ret = dw9719_detect(dw9719);
> +	if (ret)
> +		goto err_powerdown;
> +
> +	pm_runtime_set_active(&client->dev);
> +	pm_runtime_get_noresume(&client->dev);
> +	pm_runtime_enable(&client->dev);
> +
> +	ret = v4l2_async_register_subdev(&dw9719->sd);
> +	if (ret < 0)
> +		goto err_pm_runtime;
> +
> +	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +	pm_runtime_put_autosuspend(&client->dev);
> +
> +	return ret;
> +
> +err_pm_runtime:
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +err_powerdown:
> +	dw9719_power_down(dw9719);
> +err_cleanup_media:
> +	media_entity_cleanup(&dw9719->sd.entity);
> +err_free_ctrl_handler:
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +
> +	return ret;
> +}
> +
> +static void dw9719_remove(struct i2c_client *client)
> +{
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device, sd);

No need for such a long line.

> +
> +	pm_runtime_disable(&client->dev);
> +	v4l2_async_unregister_subdev(sd);
> +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +	media_entity_cleanup(&dw9719->sd.entity);

Missing call to dw9719_power_down().

I can address these while applying (assuming that's all in terms of changes
needed).

> +}
> +
> +static const struct i2c_device_id dw9719_id_table[] = {
> +	{ "dw9719" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
> +
> +static const struct dev_pm_ops dw9719_pm_ops = {
> +	SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
> +};
> +
> +static struct i2c_driver dw9719_i2c_driver = {
> +	.driver = {
> +		.name = "dw9719",
> +		.pm = &dw9719_pm_ops,
> +	},
> +	.probe_new = dw9719_probe,
> +	.remove = dw9719_remove,
> +	.id_table = dw9719_id_table,
> +};
> +module_i2c_driver(dw9719_i2c_driver);
> +
> +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
> +MODULE_DESCRIPTION("DW9719 VCM Driver");
> +MODULE_LICENSE("GPL");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 07:47:38AM +0000, Sakari Ailus wrote:
> On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:

...

> > +static int dw9719_init_controls(struct dw9719_device *dw9719)
> > +{
> > +	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
> > +	int ret;
> > +
> > +	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
> > +	if (ret)
> > +		return ret;
> 
> This check can be dropped.

The obvious question why that API returns int instead of void?

> > +	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
> > +						V4L2_CID_FOCUS_ABSOLUTE, 0,
> > +						DW9719_MAX_FOCUS_POS, 1, 0);
> > +
> > +	if (dw9719->ctrls.handler.error) {
> > +		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
> > +		ret = dw9719->ctrls.handler.error;
> > +		goto err_free_handler;
> > +	}
> > +
> > +	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;

> > +	return ret;

	return 0;

?

> > +err_free_handler:
> > +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> > +	return ret;
> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Sakari Ailus]

Hi Andy,

On Thu, Jul 06, 2023 at 12:14:27PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 07:47:38AM +0000, Sakari Ailus wrote:
> > On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:
> 
> ...
> 
> > > +static int dw9719_init_controls(struct dw9719_device *dw9719)
> > > +{
> > > +	const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
> > > +	int ret;
> > > +
> > > +	ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
> > > +	if (ret)
> > > +		return ret;
> > 
> > This check can be dropped.
> 
> The obvious question why that API returns int instead of void?

I guess it's for historical reasons. The control handler initialisation
functions won't do anything if the handler is in error state. I guess this
could be changed.

Cc Hans.

> 
> > > +	dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
> > > +						V4L2_CID_FOCUS_ABSOLUTE, 0,
> > > +						DW9719_MAX_FOCUS_POS, 1, 0);
> > > +
> > > +	if (dw9719->ctrls.handler.error) {
> > > +		dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
> > > +		ret = dw9719->ctrls.handler.error;
> > > +		goto err_free_handler;
> > > +	}
> > > +
> > > +	dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
> 
> > > +	return ret;
> 
> 	return 0;
> 
> ?

Makes sense.

> 
> > > +err_free_handler:
> > > +	v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> > > +	return ret;
> > > +}
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:
> From: Daniel Scally <djrscally@gmail.com>
> 
> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.

...

> +/*
> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:

Sakari, also long line? :-)

> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
> + */

...

> +#include <asm/unaligned.h>

Usually we include headers from generic to particular / private,
hence asm/* usually goes after linux/*.

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>

...

> +#define DW9719_CTRL_DELAY_US	1000

USEC_PER_MSEC ?

...

> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)

NSEC_PER_MSEC ?

...

> +#define DW9719_DEFAULT_VCM_FREQ		0x60

Any comment what this value means in Hz?

...

> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)

You can make this no-op at compile time by...

...

> +struct dw9719_device {
> +	struct device *dev;
> +	struct i2c_client *client;
> +	struct regulator *regulator;

> +	struct v4l2_subdev sd;

...having this to be the first member in the structure.

However bloat-o-meter can show grow of the code in case the dev is used more
often. The rule of thumb is to combine two aspects:
- frequency of usage (hence pointer arithmetics);
- hot path vs. slow path (hence importance of the lesser code).

> +	u32 sac_mode;
> +	u32 vcm_freq;
> +
> +	struct dw9719_v4l2_ctrls {
> +		struct v4l2_ctrl_handler handler;
> +		struct v4l2_ctrl *focus;
> +	} ctrls;
> +};

...

> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +	struct i2c_msg msg[2];
> +	u8 buf[2] = { reg };
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;

> +	msg[0].len = 1;
> +	msg[0].buf = buf;

	sizeof(buf[0])
	&buf[0]

looks more explicit.

> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = 1;
> +	msg[1].buf = &buf[1];

Ditto.

> +	*val = 0;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);

ARRAY_SIZE()

> +	if (ret < 0)
> +		return ret;
> +
> +	*val = buf[1];
> +
> +	return 0;
> +}

But as Sakari said this perhaps could go into CCI library.

...

> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val != DW9719_ID) {
> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
> +		ret = -ENXIO;

		return -ENXIO;

> +	}
> +
> +	return 0;

...

> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/

Missing space.

> +	usleep_range(100, 1000);

Perhaps fsleep() would be better, but I'm fine with either here.

...

> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> +{
> +	int ret;

Redundant?

> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);

> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

	return _wr16(...);

or can it return positive values?

> +}

...

> +static int __maybe_unused dw9719_suspend(struct device *dev)

Can we use new PM macros instead of __maybe_unused?

> +{
> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +	int ret;
> +	int val;
> +
> +	for (val = dw9719->ctrls.focus->val; val >= 0;
> +	     val -= DW9719_CTRL_STEPS) {
> +		ret = dw9719_t_focus_abs(dw9719, val);
> +		if (ret)
> +			return ret;

> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);

fsleep() ?

> +	}
> +
> +	return dw9719_power_down(dw9719);
> +}

> +static int __maybe_unused dw9719_resume(struct device *dev)
> +{

As per above function.

...

> +err_power_down:

In one functions you use err_ in another fail_, be consistent.

> +	dw9719_power_down(dw9719);
> +	return ret;
> +}

...

> +	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(dw9719->regulator))
> +		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),

With

	struct device *dev = &client->dev;

code may look neater.

> +				     "getting regulator\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Sakari Ailus]

Hi Andy,

On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote:
...
> > +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> > +{
> > +	struct i2c_msg msg[2];
> > +	u8 buf[2] = { reg };
> > +	int ret;
> > +
> > +	msg[0].addr = client->addr;
> > +	msg[0].flags = 0;
> 
> > +	msg[0].len = 1;
> > +	msg[0].buf = buf;
> 
> 	sizeof(buf[0])
> 	&buf[0]
> 
> looks more explicit.

The original seems fine to me. Note that this code will disappear soon.

Same for the other comments regarding register access functions (apart from
the return one).

> 
> > +	msg[1].addr = client->addr;
> > +	msg[1].flags = I2C_M_RD;
> > +	msg[1].len = 1;
> > +	msg[1].buf = &buf[1];
> 
> Ditto.
> 
> > +	*val = 0;
> > +
> > +	ret = i2c_transfer(client->adapter, msg, 2);
> 
> ARRAY_SIZE()
> 
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = buf[1];
> > +
> > +	return 0;
> > +}
> 
> But as Sakari said this perhaps could go into CCI library.
> 
> ...
> 
> > +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (val != DW9719_ID) {
> > +		dev_err(dw9719->dev, "Failed to detect correct id\n");
> > +		ret = -ENXIO;
> 
> 		return -ENXIO;
> 
> > +	}
> > +
> > +	return 0;
> 
> ...
> 
> > +	/* Need 100us to transit from SHUTDOWN to STANDBY*/
> 
> Missing space.
> 
> > +	usleep_range(100, 1000);
> 
> Perhaps fsleep() would be better, but I'm fine with either here.
> 
> ...
> 
> > +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> > +{
> > +	int ret;
> 
> Redundant?
> 
> > +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);

This is redundant, too, btw., the control framework already handles this.

> 
> > +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> 
> 	return _wr16(...);
> 
> or can it return positive values?
> 
> > +}
> 
> ...
> 
> > +static int __maybe_unused dw9719_suspend(struct device *dev)
> 
> Can we use new PM macros instead of __maybe_unused?
> 
> > +{
> > +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
> > +	int ret;
> > +	int val;
> > +
> > +	for (val = dw9719->ctrls.focus->val; val >= 0;
> > +	     val -= DW9719_CTRL_STEPS) {
> > +		ret = dw9719_t_focus_abs(dw9719, val);
> > +		if (ret)
> > +			return ret;
> 
> > +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> 
> fsleep() ?
> 
> > +	}
> > +
> > +	return dw9719_power_down(dw9719);
> > +}
> 
> > +static int __maybe_unused dw9719_resume(struct device *dev)
> > +{
> 
> As per above function.
> 
> ...
> 
> > +err_power_down:
> 
> In one functions you use err_ in another fail_, be consistent.
> 
> > +	dw9719_power_down(dw9719);
> > +	return ret;
> > +}
> 
> ...
> 
> > +	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
> > +	if (IS_ERR(dw9719->regulator))
> > +		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),
> 
> With
> 
> 	struct device *dev = &client->dev;
> 
> code may look neater.

I prefer it as-is.

> 
> > +				     "getting regulator\n");
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 10:27:55AM +0000, Sakari Ailus wrote:
> On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote:

...

> > > +	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
> > > +	if (IS_ERR(dw9719->regulator))
> > > +		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),
> > 
> > With
> > 
> > 	struct device *dev = &client->dev;
> > 
> > code may look neater.
> 
> I prefer it as-is.

May I ask what the technical rationale behind your preferences? Esp. taking
into account how picky you are for 80 character limit.

> > > +				     "getting regulator\n");

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Sakari Ailus]

Hi Andy,

On Thu, Jul 06, 2023 at 01:48:25PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 10:27:55AM +0000, Sakari Ailus wrote:
> > On Thu, Jul 06, 2023 at 01:06:07PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
> > > > +	dw9719->regulator = devm_regulator_get(&client->dev, "vdd");
> > > > +	if (IS_ERR(dw9719->regulator))
> > > > +		return dev_err_probe(&client->dev, PTR_ERR(dw9719->regulator),
> > > 
> > > With
> > > 
> > > 	struct device *dev = &client->dev;
> > > 
> > > code may look neater.
> > 
> > I prefer it as-is.
> 
> May I ask what the technical rationale behind your preferences? Esp. taking
> into account how picky you are for 80 character limit.

The device is already available via &client->dev. Sure, you do remove a few
characters per line but also introduce a new variable to be aware of. In
this case there's no line break that would be affected.

That being said, I certainly don't have a strong opinion on this. If Hans
prefers to have a local variable for this I'm totally fine with that. I
just think there's no need to.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Hans de Goede]

Hi,

On 7/6/23 12:06, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:
>> From: Daniel Scally <djrscally@gmail.com>
>>
>> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
>> and registers a control to set the desired focus.
> 
> ...
> 
>> +/*
>> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
> 
> Sakari, also long line? :-)

Nope, this is 79 chars.

> 
>> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
>> + */
> 
> ...
> 
>> +#include <asm/unaligned.h>
> 
> Usually we include headers from generic to particular / private,
> hence asm/* usually goes after linux/*.

Ack (gone after switching to CCI helpers in next version).

>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
> 
> ...
> 
>> +#define DW9719_CTRL_DELAY_US	1000
> 
> USEC_PER_MSEC ?

I don't see how that helps readability.

> 
> ...
> 
>> +#define DELAY_MAX_PER_STEP_NS	(1000000 * 1023)
> 
> NSEC_PER_MSEC ?

This define is not used so I've dropped it for the next version.

> 
> ...
> 
>> +#define DW9719_DEFAULT_VCM_FREQ		0x60
> 
> Any comment what this value means in Hz?

This comes directly from the Android driver, no idea what this actually means (no datasheet).

> 
> ...
> 
>> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
> 
> You can make this no-op at compile time by...
> 
> ...
> 
>> +struct dw9719_device {
>> +	struct device *dev;
>> +	struct i2c_client *client;
>> +	struct regulator *regulator;
> 
>> +	struct v4l2_subdev sd;
> 
> ...having this to be the first member in the structure.

Ack.

> However bloat-o-meter can show grow of the code in case the dev is used more
> often. The rule of thumb is to combine two aspects:
> - frequency of usage (hence pointer arithmetics);
> - hot path vs. slow path (hence importance of the lesser code).
> 
>> +	u32 sac_mode;
>> +	u32 vcm_freq;
>> +
>> +	struct dw9719_v4l2_ctrls {
>> +		struct v4l2_ctrl_handler handler;
>> +		struct v4l2_ctrl *focus;
>> +	} ctrls;
>> +};
> 
> ...
> 
>> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
>> +{
>> +	struct i2c_msg msg[2];
>> +	u8 buf[2] = { reg };
>> +	int ret;
>> +
>> +	msg[0].addr = client->addr;
>> +	msg[0].flags = 0;
> 
>> +	msg[0].len = 1;
>> +	msg[0].buf = buf;
> 
> 	sizeof(buf[0])
> 	&buf[0]
> 
> looks more explicit.
> 
>> +	msg[1].addr = client->addr;
>> +	msg[1].flags = I2C_M_RD;
>> +	msg[1].len = 1;
>> +	msg[1].buf = &buf[1];
> 
> Ditto.
> 
>> +	*val = 0;
>> +
>> +	ret = i2c_transfer(client->adapter, msg, 2);
> 
> ARRAY_SIZE()
> 
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val = buf[1];
>> +
>> +	return 0;
>> +}
> 
> But as Sakari said this perhaps could go into CCI library.

Right, this is all gone after switching to the new CCI helpers.



> 
> ...
> 
>> +	ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (val != DW9719_ID) {
>> +		dev_err(dw9719->dev, "Failed to detect correct id\n");
>> +		ret = -ENXIO;
> 
> 		return -ENXIO;
> 
>> +	}
>> +
>> +	return 0;
> 
> ...
> 
>> +	/* Need 100us to transit from SHUTDOWN to STANDBY*/
> 
> Missing space.
> 
>> +	usleep_range(100, 1000);
> 
> Perhaps fsleep() would be better, but I'm fine with either here.

fsleep() indeed is better here.

> 
> ...
> 
>> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
>> +{
>> +	int ret;
> 
> Redundant?
> 
>> +	value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
> 
>> +	ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> 	return _wr16(...);
> 
> or can it return positive values?

Ack, fixed.

> 
>> +}
> 
> ...
> 
>> +static int __maybe_unused dw9719_suspend(struct device *dev)
> 
> Can we use new PM macros instead of __maybe_unused?

Ack, fixed.

>> +{
>> +	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>> +	struct dw9719_device *dw9719 = to_dw9719_device(sd);
>> +	int ret;
>> +	int val;
>> +
>> +	for (val = dw9719->ctrls.focus->val; val >= 0;
>> +	     val -= DW9719_CTRL_STEPS) {
>> +		ret = dw9719_t_focus_abs(dw9719, val);
>> +		if (ret)
>> +			return ret;
> 
>> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> 
> fsleep() ?

fsleep would expand to:

		usleep_range(DW9719_CTRL_DELAY_US,  2 * DW9719_CTRL_DELAY_US);

making the loop take up to twice as long.


> 
>> +	}
>> +
>> +	return dw9719_power_down(dw9719);
>> +}
> 
>> +static int __maybe_unused dw9719_resume(struct device *dev)
>> +{
> 
> As per above function.
> 
> ...
> 
>> +err_power_down:
> 
> In one functions you use err_ in another fail_, be consistent.

Ack, fixed.

Regards,

Hans

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 04:34:41PM +0200, Hans de Goede wrote:
> On 7/6/23 12:06, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:06PM +0200, Hans de Goede wrote:

...

> >> +		usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> > 
> > fsleep() ?
> 
> fsleep would expand to:
> 
> 		usleep_range(DW9719_CTRL_DELAY_US,  2 * DW9719_CTRL_DELAY_US);
> 
> making the loop take up to twice as long.

Is it a problem here? If so, perhaps one line to explain that?

> >> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Dave Stevenson]

Hi Hans

On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote:
>
> From: Daniel Scally <djrscally@gmail.com>
>
> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
> and registers a control to set the desired focus.
>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v3 (Hans de Goede)
> - New patch in v3 of this series based on Dan Scally's initial
>   DW9719 upstream submission:
>   https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/
> - Drop hack to enable "vsio" regulator, this is no longer necessary
>   now that there is a device-link making the VCM a runtime-pm consumer
>   of the sensor
> - Add checking of device-properties for sac-mode and vcm-freq,
>   as requested by Sakari, this is done similar to the dw9768:
>   Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>   Note no devicetree binding doc is added since currently only
>   i2c_device_id enumeration (instantiated by IPU bridge) is
>   supported
> ---
>  MAINTAINERS                |   7 +
>  drivers/media/i2c/Kconfig  |  11 +
>  drivers/media/i2c/Makefile |   1 +
>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 446 insertions(+)
>  create mode 100644 drivers/media/i2c/dw9719.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 494682dd437f..cf8e799f6ea2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6266,6 +6266,13 @@ T:       git git://linuxtv.org/media_tree.git
>  F:     Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml
>  F:     drivers/media/i2c/dw9714.c
>
> +DONGWOON DW9719 LENS VOICE COIL DRIVER
> +M:     Daniel Scally <djrscally@gmail.com>
> +L:     linux-media@vger.kernel.org
> +S:     Maintained
> +T:     git git://linuxtv.org/media_tree.git
> +F:     drivers/media/i2c/dw9719.c
> +
>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>  L:     linux-media@vger.kernel.org
>  S:     Orphan
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index 26dc365365d8..4864f1df3c7a 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -875,6 +875,17 @@ config VIDEO_DW9714
>           capability. This is designed for linear control of
>           voice coil motors, controlled via I2C serial interface.
>
> +config VIDEO_DW9719
> +       tristate "DW9719 lens voice coil support"
> +       depends on I2C && VIDEO_DEV
> +       select MEDIA_CONTROLLER
> +       select VIDEO_V4L2_SUBDEV_API
> +       select V4L2_ASYNC
> +       help
> +         This is a driver for the DW9719 camera lens voice coil.
> +         This is designed for linear control of voice coil motors,
> +         controlled via I2C serial interface.
> +
>  config VIDEO_DW9768
>         tristate "DW9768 lens voice coil support"
>         depends on I2C && VIDEO_DEV
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index d175a2e2fb19..745f8d07e649 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o
>  obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
>  obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o
>  obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
> new file mode 100644
> index 000000000000..7b83ae102131
> --- /dev/null
> +++ b/drivers/media/i2c/dw9719.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2012 Intel Corporation
> +
> +/*
> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
> + */
> +
> +#include <asm/unaligned.h>
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define DW9719_MAX_FOCUS_POS   1023
> +#define DW9719_CTRL_STEPS      16
> +#define DW9719_CTRL_DELAY_US   1000
> +#define DELAY_MAX_PER_STEP_NS  (1000000 * 1023)
> +
> +#define DW9719_INFO                    0
> +#define DW9719_ID                      0xF1
> +#define DW9719_CONTROL                 2
> +#define DW9719_VCM_CURRENT             3
> +
> +#define DW9719_MODE                    6
> +#define DW9719_VCM_FREQ                        7
> +
> +#define DW9719_MODE_SAC_SHIFT          4
> +#define DW9719_MODE_SAC3               4
> +
> +#define DW9719_DEFAULT_VCM_FREQ                0x60
> +
> +#define DW9719_ENABLE_RINGING          0x02

This register setup and the ramping up/down code is nearly identical
to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose
SAC (Smart Actuator Control) for damping the movement, but dw9807 does
support it.

The only really quirky bit here is the "Jiggle SCL pin to wake up
device", but I can't find a datasheet to know anything more about
that. The other apparent distinction would be whether DW9719 has the
VBUSY bit in the status register that dw9807 is abiding by, whilst
this driver doesn't.

Should this be a new driver, or a variant of dw9807-vcm?

Cheers
  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/media/i2c/dw9807-vcm.c

> +
> +#define to_dw9719_device(x) container_of(x, struct dw9719_device, sd)
> +
> +struct dw9719_device {
> +       struct device *dev;
> +       struct i2c_client *client;
> +       struct regulator *regulator;
> +       struct v4l2_subdev sd;
> +       u32 sac_mode;
> +       u32 vcm_freq;
> +
> +       struct dw9719_v4l2_ctrls {
> +               struct v4l2_ctrl_handler handler;
> +               struct v4l2_ctrl *focus;
> +       } ctrls;
> +};
> +
> +static int dw9719_i2c_rd8(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +       struct i2c_msg msg[2];
> +       u8 buf[2] = { reg };
> +       int ret;
> +
> +       msg[0].addr = client->addr;
> +       msg[0].flags = 0;
> +       msg[0].len = 1;
> +       msg[0].buf = buf;
> +
> +       msg[1].addr = client->addr;
> +       msg[1].flags = I2C_M_RD;
> +       msg[1].len = 1;
> +       msg[1].buf = &buf[1];
> +       *val = 0;
> +
> +       ret = i2c_transfer(client->adapter, msg, 2);
> +       if (ret < 0)
> +               return ret;
> +
> +       *val = buf[1];
> +
> +       return 0;
> +}
> +
> +static int dw9719_i2c_wr8(struct i2c_client *client, u8 reg, u8 val)
> +{
> +       struct i2c_msg msg;
> +       int ret;
> +
> +       u8 buf[2] = { reg, val };
> +
> +       msg.addr = client->addr;
> +       msg.flags = 0;
> +       msg.len = sizeof(buf);
> +       msg.buf = buf;
> +
> +       ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static int dw9719_i2c_wr16(struct i2c_client *client, u8 reg, u16 val)
> +{
> +       struct i2c_msg msg;
> +       u8 buf[3] = { reg };
> +       int ret;
> +
> +       put_unaligned_be16(val, buf + 1);
> +
> +       msg.addr = client->addr;
> +       msg.flags = 0;
> +       msg.len = sizeof(buf);
> +       msg.buf = buf;
> +
> +       ret = i2c_transfer(client->adapter, &msg, 1);
> +
> +       return ret < 0 ? ret : 0;
> +}
> +
> +static int dw9719_detect(struct dw9719_device *dw9719)
> +{
> +       int ret;
> +       u8 val;
> +
> +       ret = dw9719_i2c_rd8(dw9719->client, DW9719_INFO, &val);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (val != DW9719_ID) {
> +               dev_err(dw9719->dev, "Failed to detect correct id\n");
> +               ret = -ENXIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int dw9719_power_down(struct dw9719_device *dw9719)
> +{
> +       return regulator_disable(dw9719->regulator);
> +}
> +
> +static int dw9719_power_up(struct dw9719_device *dw9719)
> +{
> +       int ret;
> +
> +       ret = regulator_enable(dw9719->regulator);
> +       if (ret)
> +               return ret;
> +
> +       /* Jiggle SCL pin to wake up device */
> +       ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL, 1);
> +
> +       /* Need 100us to transit from SHUTDOWN to STANDBY*/
> +       usleep_range(100, 1000);
> +
> +       ret = dw9719_i2c_wr8(dw9719->client, DW9719_CONTROL,
> +                            DW9719_ENABLE_RINGING);
> +       if (ret < 0)
> +               goto fail_powerdown;
> +
> +       ret = dw9719_i2c_wr8(dw9719->client, DW9719_MODE,
> +                            dw9719->sac_mode << DW9719_MODE_SAC_SHIFT);
> +       if (ret < 0)
> +               goto fail_powerdown;
> +
> +       ret = dw9719_i2c_wr8(dw9719->client, DW9719_VCM_FREQ, dw9719->vcm_freq);
> +       if (ret < 0)
> +               goto fail_powerdown;
> +
> +       return 0;
> +
> +fail_powerdown:
> +       dw9719_power_down(dw9719);
> +       return ret;
> +}
> +
> +static int dw9719_t_focus_abs(struct dw9719_device *dw9719, s32 value)
> +{
> +       int ret;
> +
> +       value = clamp(value, 0, DW9719_MAX_FOCUS_POS);
> +       ret = dw9719_i2c_wr16(dw9719->client, DW9719_VCM_CURRENT, value);
> +       if (ret < 0)
> +               return ret;
> +
> +       return 0;
> +}
> +
> +static int dw9719_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +       struct dw9719_device *dw9719 = container_of(ctrl->handler,
> +                                                   struct dw9719_device,
> +                                                   ctrls.handler);
> +       int ret;
> +
> +       /* Only apply changes to the controls if the device is powered up */
> +       if (!pm_runtime_get_if_in_use(dw9719->dev))
> +               return 0;
> +
> +       switch (ctrl->id) {
> +       case V4L2_CID_FOCUS_ABSOLUTE:
> +               ret = dw9719_t_focus_abs(dw9719, ctrl->val);
> +               break;
> +       default:
> +               ret = -EINVAL;
> +       }
> +
> +       pm_runtime_put(dw9719->dev);
> +
> +       return ret;
> +}
> +
> +static const struct v4l2_ctrl_ops dw9719_ctrl_ops = {
> +       .s_ctrl = dw9719_set_ctrl,
> +};
> +
> +static int __maybe_unused dw9719_suspend(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +       int ret;
> +       int val;
> +
> +       for (val = dw9719->ctrls.focus->val; val >= 0;
> +            val -= DW9719_CTRL_STEPS) {
> +               ret = dw9719_t_focus_abs(dw9719, val);
> +               if (ret)
> +                       return ret;
> +
> +               usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +       }
> +
> +       return dw9719_power_down(dw9719);
> +}
> +
> +static int __maybe_unused dw9719_resume(struct device *dev)
> +{
> +       struct v4l2_subdev *sd = dev_get_drvdata(dev);
> +       struct dw9719_device *dw9719 = to_dw9719_device(sd);
> +       int current_focus = dw9719->ctrls.focus->val;
> +       int ret;
> +       int val;
> +
> +       ret = dw9719_power_up(dw9719);
> +       if (ret)
> +               return ret;
> +
> +       for (val = current_focus % DW9719_CTRL_STEPS; val < current_focus;
> +            val += DW9719_CTRL_STEPS) {
> +               ret = dw9719_t_focus_abs(dw9719, val);
> +               if (ret)
> +                       goto err_power_down;
> +
> +               usleep_range(DW9719_CTRL_DELAY_US, DW9719_CTRL_DELAY_US + 10);
> +       }
> +
> +       return 0;
> +
> +err_power_down:
> +       dw9719_power_down(dw9719);
> +       return ret;
> +}
> +
> +static int dw9719_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       return pm_runtime_resume_and_get(sd->dev);
> +}
> +
> +static int dw9719_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> +{
> +       pm_runtime_put(sd->dev);
> +
> +       return 0;
> +}
> +
> +static const struct v4l2_subdev_internal_ops dw9719_internal_ops = {
> +       .open = dw9719_open,
> +       .close = dw9719_close,
> +};
> +
> +static int dw9719_init_controls(struct dw9719_device *dw9719)
> +{
> +       const struct v4l2_ctrl_ops *ops = &dw9719_ctrl_ops;
> +       int ret;
> +
> +       ret = v4l2_ctrl_handler_init(&dw9719->ctrls.handler, 1);
> +       if (ret)
> +               return ret;
> +
> +       dw9719->ctrls.focus = v4l2_ctrl_new_std(&dw9719->ctrls.handler, ops,
> +                                               V4L2_CID_FOCUS_ABSOLUTE, 0,
> +                                               DW9719_MAX_FOCUS_POS, 1, 0);
> +
> +       if (dw9719->ctrls.handler.error) {
> +               dev_err(dw9719->dev, "Error initialising v4l2 ctrls\n");
> +               ret = dw9719->ctrls.handler.error;
> +               goto err_free_handler;
> +       }
> +
> +       dw9719->sd.ctrl_handler = &dw9719->ctrls.handler;
> +
> +       return ret;
> +
> +err_free_handler:
> +       v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +       return ret;
> +}
> +
> +static const struct v4l2_subdev_ops dw9719_ops = { };
> +
> +static int dw9719_probe(struct i2c_client *client)
> +{
> +       struct dw9719_device *dw9719;
> +       int ret;
> +
> +       dw9719 = devm_kzalloc(&client->dev, sizeof(*dw9719), GFP_KERNEL);
> +       if (!dw9719)
> +               return -ENOMEM;
> +
> +       dw9719->client = client;
> +       dw9719->dev = &client->dev;
> +
> +       dw9719->sac_mode = DW9719_MODE_SAC3;
> +       dw9719->vcm_freq = DW9719_DEFAULT_VCM_FREQ;
> +
> +       /* Optional indication of SAC mode select */
> +       device_property_read_u32(&client->dev, "dongwoon,sac-mode",
> +                                &dw9719->sac_mode);
> +
> +       /* Optional indication of VCM frequency */
> +       device_property_read_u32(&client->dev, "dongwoon,vcm-freq",
> +                                &dw9719->vcm_freq);
> +
> +       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;
> +       dw9719->sd.internal_ops = &dw9719_internal_ops;
> +
> +       ret = dw9719_init_controls(dw9719);
> +       if (ret)
> +               return ret;
> +
> +       ret = media_entity_pads_init(&dw9719->sd.entity, 0, NULL);
> +       if (ret < 0)
> +               goto err_free_ctrl_handler;
> +
> +       dw9719->sd.entity.function = MEDIA_ENT_F_LENS;
> +
> +       /*
> +        * We need the driver to work in the event that pm runtime is disable in
> +        * the kernel, so power up and verify the chip now. In the event that
> +        * runtime pm is disabled this will leave the chip on, so that the lens
> +        * will work.
> +        */
> +
> +       ret = dw9719_power_up(dw9719);
> +       if (ret)
> +               goto err_cleanup_media;
> +
> +       ret = dw9719_detect(dw9719);
> +       if (ret)
> +               goto err_powerdown;
> +
> +       pm_runtime_set_active(&client->dev);
> +       pm_runtime_get_noresume(&client->dev);
> +       pm_runtime_enable(&client->dev);
> +
> +       ret = v4l2_async_register_subdev(&dw9719->sd);
> +       if (ret < 0)
> +               goto err_pm_runtime;
> +
> +       pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> +       pm_runtime_use_autosuspend(&client->dev);
> +       pm_runtime_put_autosuspend(&client->dev);
> +
> +       return ret;
> +
> +err_pm_runtime:
> +       pm_runtime_disable(&client->dev);
> +       pm_runtime_put_noidle(&client->dev);
> +err_powerdown:
> +       dw9719_power_down(dw9719);
> +err_cleanup_media:
> +       media_entity_cleanup(&dw9719->sd.entity);
> +err_free_ctrl_handler:
> +       v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +
> +       return ret;
> +}
> +
> +static void dw9719_remove(struct i2c_client *client)
> +{
> +       struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +       struct dw9719_device *dw9719 = container_of(sd, struct dw9719_device, sd);
> +
> +       pm_runtime_disable(&client->dev);
> +       v4l2_async_unregister_subdev(sd);
> +       v4l2_ctrl_handler_free(&dw9719->ctrls.handler);
> +       media_entity_cleanup(&dw9719->sd.entity);
> +}
> +
> +static const struct i2c_device_id dw9719_id_table[] = {
> +       { "dw9719" },
> +       { }
> +};
> +MODULE_DEVICE_TABLE(i2c, dw9719_id_table);
> +
> +static const struct dev_pm_ops dw9719_pm_ops = {
> +       SET_RUNTIME_PM_OPS(dw9719_suspend, dw9719_resume, NULL)
> +};
> +
> +static struct i2c_driver dw9719_i2c_driver = {
> +       .driver = {
> +               .name = "dw9719",
> +               .pm = &dw9719_pm_ops,
> +       },
> +       .probe_new = dw9719_probe,
> +       .remove = dw9719_remove,
> +       .id_table = dw9719_id_table,
> +};
> +module_i2c_driver(dw9719_i2c_driver);
> +
> +MODULE_AUTHOR("Daniel Scally <djrscally@gmail.com>");
> +MODULE_DESCRIPTION("DW9719 VCM Driver");
> +MODULE_LICENSE("GPL");
> --
> 2.41.0
>

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Hans de Goede]

Hi Dave,

On 7/6/23 13:18, Dave Stevenson wrote:
> Hi Hans
> 
> On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Daniel Scally <djrscally@gmail.com>
>>
>> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
>> and registers a control to set the desired focus.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3 (Hans de Goede)
>> - New patch in v3 of this series based on Dan Scally's initial
>>   DW9719 upstream submission:
>>   https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/
>> - Drop hack to enable "vsio" regulator, this is no longer necessary
>>   now that there is a device-link making the VCM a runtime-pm consumer
>>   of the sensor
>> - Add checking of device-properties for sac-mode and vcm-freq,
>>   as requested by Sakari, this is done similar to the dw9768:
>>   Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>>   Note no devicetree binding doc is added since currently only
>>   i2c_device_id enumeration (instantiated by IPU bridge) is
>>   supported
>> ---
>>  MAINTAINERS                |   7 +
>>  drivers/media/i2c/Kconfig  |  11 +
>>  drivers/media/i2c/Makefile |   1 +
>>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 446 insertions(+)
>>  create mode 100644 drivers/media/i2c/dw9719.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 494682dd437f..cf8e799f6ea2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6266,6 +6266,13 @@ T:       git git://linuxtv.org/media_tree.git
>>  F:     Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml
>>  F:     drivers/media/i2c/dw9714.c
>>
>> +DONGWOON DW9719 LENS VOICE COIL DRIVER
>> +M:     Daniel Scally <djrscally@gmail.com>
>> +L:     linux-media@vger.kernel.org
>> +S:     Maintained
>> +T:     git git://linuxtv.org/media_tree.git
>> +F:     drivers/media/i2c/dw9719.c
>> +
>>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>>  L:     linux-media@vger.kernel.org
>>  S:     Orphan
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 26dc365365d8..4864f1df3c7a 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -875,6 +875,17 @@ config VIDEO_DW9714
>>           capability. This is designed for linear control of
>>           voice coil motors, controlled via I2C serial interface.
>>
>> +config VIDEO_DW9719
>> +       tristate "DW9719 lens voice coil support"
>> +       depends on I2C && VIDEO_DEV
>> +       select MEDIA_CONTROLLER
>> +       select VIDEO_V4L2_SUBDEV_API
>> +       select V4L2_ASYNC
>> +       help
>> +         This is a driver for the DW9719 camera lens voice coil.
>> +         This is designed for linear control of voice coil motors,
>> +         controlled via I2C serial interface.
>> +
>>  config VIDEO_DW9768
>>         tristate "DW9768 lens voice coil support"
>>         depends on I2C && VIDEO_DEV
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index d175a2e2fb19..745f8d07e649 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o
>>  obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
>>  obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o
>>  obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
>> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
>> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
>> new file mode 100644
>> index 000000000000..7b83ae102131
>> --- /dev/null
>> +++ b/drivers/media/i2c/dw9719.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2012 Intel Corporation
>> +
>> +/*
>> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
>> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#define DW9719_MAX_FOCUS_POS   1023
>> +#define DW9719_CTRL_STEPS      16
>> +#define DW9719_CTRL_DELAY_US   1000
>> +#define DELAY_MAX_PER_STEP_NS  (1000000 * 1023)
>> +
>> +#define DW9719_INFO                    0
>> +#define DW9719_ID                      0xF1
>> +#define DW9719_CONTROL                 2
>> +#define DW9719_VCM_CURRENT             3
>> +
>> +#define DW9719_MODE                    6
>> +#define DW9719_VCM_FREQ                        7
>> +
>> +#define DW9719_MODE_SAC_SHIFT          4
>> +#define DW9719_MODE_SAC3               4
>> +
>> +#define DW9719_DEFAULT_VCM_FREQ                0x60
>> +
>> +#define DW9719_ENABLE_RINGING          0x02
> 
> This register setup and the ramping up/down code is nearly identical
> to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose
> SAC (Smart Actuator Control) for damping the movement, but dw9807 does
> support it.
> 
> The only really quirky bit here is the "Jiggle SCL pin to wake up
> device", but I can't find a datasheet to know anything more about
> that. The other apparent distinction would be whether DW9719 has the
> VBUSY bit in the status register that dw9807 is abiding by, whilst
> this driver doesn't.
> 
> Should this be a new driver, or a variant of dw9807-vcm?

That is a good question and there also have been lots of other
remarks on this driver / patch.

So lets continue with this series without this patch (it is
not necessary for the rest of the series) and then I'll submit
a new version of the patch as a new standalone series/patch
when I have some time to work on this more.

I'll then also check to see if instead of this driver a patch
to modify dw9807-vcm makes more sense.

Regards,

Hans

Re: [PATCH v3 14/18] media: i2c: Add driver for DW9719 VCM

[Hans de Goede]

Hi Dave,

On 7/6/23 13:18, Dave Stevenson wrote:
> Hi Hans
> 
> On Wed, 5 Jul 2023 at 22:33, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> From: Daniel Scally <djrscally@gmail.com>
>>
>> Add a driver for the DW9719 VCM. The driver creates a v4l2 subdevice
>> and registers a control to set the desired focus.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v3 (Hans de Goede)
>> - New patch in v3 of this series based on Dan Scally's initial
>>   DW9719 upstream submission:
>>   https://lore.kernel.org/all/20211128232115.38833-1-djrscally@gmail.com/
>> - Drop hack to enable "vsio" regulator, this is no longer necessary
>>   now that there is a device-link making the VCM a runtime-pm consumer
>>   of the sensor
>> - Add checking of device-properties for sac-mode and vcm-freq,
>>   as requested by Sakari, this is done similar to the dw9768:
>>   Documentation/devicetree/bindings/media/i2c/dongwoon,dw9768.yaml
>>   Note no devicetree binding doc is added since currently only
>>   i2c_device_id enumeration (instantiated by IPU bridge) is
>>   supported
>> ---
>>  MAINTAINERS                |   7 +
>>  drivers/media/i2c/Kconfig  |  11 +
>>  drivers/media/i2c/Makefile |   1 +
>>  drivers/media/i2c/dw9719.c | 427 +++++++++++++++++++++++++++++++++++++
>>  4 files changed, 446 insertions(+)
>>  create mode 100644 drivers/media/i2c/dw9719.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 494682dd437f..cf8e799f6ea2 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6266,6 +6266,13 @@ T:       git git://linuxtv.org/media_tree.git
>>  F:     Documentation/devicetree/bindings/media/i2c/dongwoon,dw9714.yaml
>>  F:     drivers/media/i2c/dw9714.c
>>
>> +DONGWOON DW9719 LENS VOICE COIL DRIVER
>> +M:     Daniel Scally <djrscally@gmail.com>
>> +L:     linux-media@vger.kernel.org
>> +S:     Maintained
>> +T:     git git://linuxtv.org/media_tree.git
>> +F:     drivers/media/i2c/dw9719.c
>> +
>>  DONGWOON DW9768 LENS VOICE COIL DRIVER
>>  L:     linux-media@vger.kernel.org
>>  S:     Orphan
>> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
>> index 26dc365365d8..4864f1df3c7a 100644
>> --- a/drivers/media/i2c/Kconfig
>> +++ b/drivers/media/i2c/Kconfig
>> @@ -875,6 +875,17 @@ config VIDEO_DW9714
>>           capability. This is designed for linear control of
>>           voice coil motors, controlled via I2C serial interface.
>>
>> +config VIDEO_DW9719
>> +       tristate "DW9719 lens voice coil support"
>> +       depends on I2C && VIDEO_DEV
>> +       select MEDIA_CONTROLLER
>> +       select VIDEO_V4L2_SUBDEV_API
>> +       select V4L2_ASYNC
>> +       help
>> +         This is a driver for the DW9719 camera lens voice coil.
>> +         This is designed for linear control of voice coil motors,
>> +         controlled via I2C serial interface.
>> +
>>  config VIDEO_DW9768
>>         tristate "DW9768 lens voice coil support"
>>         depends on I2C && VIDEO_DEV
>> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
>> index d175a2e2fb19..745f8d07e649 100644
>> --- a/drivers/media/i2c/Makefile
>> +++ b/drivers/media/i2c/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_VIDEO_DS90UB913) += ds90ub913.o
>>  obj-$(CONFIG_VIDEO_DS90UB953) += ds90ub953.o
>>  obj-$(CONFIG_VIDEO_DS90UB960) += ds90ub960.o
>>  obj-$(CONFIG_VIDEO_DW9714) += dw9714.o
>> +obj-$(CONFIG_VIDEO_DW9719) += dw9719.o
>>  obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
>>  obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
>>  obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
>> diff --git a/drivers/media/i2c/dw9719.c b/drivers/media/i2c/dw9719.c
>> new file mode 100644
>> index 000000000000..7b83ae102131
>> --- /dev/null
>> +++ b/drivers/media/i2c/dw9719.c
>> @@ -0,0 +1,427 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2012 Intel Corporation
>> +
>> +/*
>> + * Based on linux/modules/camera/drivers/media/i2c/imx/dw9719.c in this repo:
>> + * https://github.com/ZenfoneArea/android_kernel_asus_zenfone5
>> + */
>> +
>> +#include <asm/unaligned.h>
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/types.h>
>> +
>> +#include <media/v4l2-common.h>
>> +#include <media/v4l2-ctrls.h>
>> +#include <media/v4l2-subdev.h>
>> +
>> +#define DW9719_MAX_FOCUS_POS   1023
>> +#define DW9719_CTRL_STEPS      16
>> +#define DW9719_CTRL_DELAY_US   1000
>> +#define DELAY_MAX_PER_STEP_NS  (1000000 * 1023)
>> +
>> +#define DW9719_INFO                    0
>> +#define DW9719_ID                      0xF1
>> +#define DW9719_CONTROL                 2
>> +#define DW9719_VCM_CURRENT             3
>> +
>> +#define DW9719_MODE                    6
>> +#define DW9719_VCM_FREQ                        7
>> +
>> +#define DW9719_MODE_SAC_SHIFT          4
>> +#define DW9719_MODE_SAC3               4
>> +
>> +#define DW9719_DEFAULT_VCM_FREQ                0x60
>> +
>> +#define DW9719_ENABLE_RINGING          0x02
> 
> This register setup and the ramping up/down code is nearly identical
> to the existing dw9807-vcm driver[1]. Admittedly that doesn't expose
> SAC (Smart Actuator Control) for damping the movement, but dw9807 does
> support it.
> 
> The only really quirky bit here is the "Jiggle SCL pin to wake up
> device", but I can't find a datasheet to know anything more about
> that. The other apparent distinction would be whether DW9719 has the
> VBUSY bit in the status register that dw9807 is abiding by, whilst
> this driver doesn't.
> 
> Should this be a new driver, or a variant of dw9807-vcm?

So I did a quick check and there are some interesting differences,
like the dw9719 code writing 1 to the CTRL register on resume /
powerup while as the dw9807 code writes 0 on resume / powerup.

And I have been unable to find a datasheet for either model.

This means that both drivers have some black-box aspects,
like the powerup differences, to them (both come from Android source
dumps I think).

This + the small size of these drivers, makes me think that it
would be best to just keep this as a separate driver.

So for the next standalone (not as part of this series)
submission I plan to stick with having a separate driver
and I'll try to address all the other review remarks.

Regards,

Hans

[PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Hans de Goede]

Some ACPI glue code (1) may want to do an acpi_device_id match while
it only has a struct acpi_device available because the first physical
node may not have been instantiated yet.

Add a new acpi_match_acpi_device() helper for this, which takes
a "struct acpi_device *" as argument rather then the "struct device *"
which acpi_match_device() takes.

1) E.g. code which parses ACPI tables to transforms them
into more standard kernel data structures like fwnodes

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/bus.c      | 22 ++++++++++++++++++----
 include/acpi/acpi_bus.h |  2 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index d161ff707de4..de75ad675f92 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -888,6 +888,23 @@ static bool __acpi_match_device(struct acpi_device *device,
 	return true;
 }
 
+/**
+ * acpi_match_acpi_device - Match a struct acpi_device against a given list of ACPI IDs
+ * @ids: Array of struct acpi_device_id object to match against.
+ * @adev: The acpi_device structure to match.
+ *
+ * Return a pointer to the first matching ID on success or %NULL on failure.
+ */
+const struct acpi_device_id *acpi_match_acpi_device(const struct acpi_device_id *ids,
+						    struct acpi_device *adev)
+{
+	const struct acpi_device_id *id = NULL;
+
+	__acpi_match_device(adev, ids, NULL, &id, NULL);
+	return id;
+}
+EXPORT_SYMBOL_GPL(acpi_match_acpi_device);
+
 /**
  * acpi_match_device - Match a struct device against a given list of ACPI IDs
  * @ids: Array of struct acpi_device_id object to match against.
@@ -902,10 +919,7 @@ static bool __acpi_match_device(struct acpi_device *device,
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev)
 {
-	const struct acpi_device_id *id = NULL;
-
-	__acpi_match_device(acpi_companion_match(dev), ids, NULL, &id, NULL);
-	return id;
+	return acpi_match_acpi_device(ids, acpi_companion_match(dev));
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index c941d99162c0..db854e78b2f8 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -561,6 +561,8 @@ void acpi_bus_trim(struct acpi_device *start);
 acpi_status acpi_bus_get_ejd(acpi_handle handle, acpi_handle * ejd);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids);
+const struct acpi_device_id *acpi_match_acpi_device(const struct acpi_device_id *ids,
+						    struct acpi_device *adev);
 void acpi_set_modalias(struct acpi_device *adev, const char *default_id,
 		       char *modalias, size_t len);
 int acpi_create_dir(struct acpi_device *);
-- 
2.41.0

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:
> Some ACPI glue code (1) may want to do an acpi_device_id match while
> it only has a struct acpi_device available because the first physical
> node may not have been instantiated yet.
> 
> Add a new acpi_match_acpi_device() helper for this, which takes
> a "struct acpi_device *" as argument rather then the "struct device *"
> which acpi_match_device() takes.
> 
> 1) E.g. code which parses ACPI tables to transforms them
> into more standard kernel data structures like fwnodes

Looks like it's v1 of my original patch, anyway this is now in Linux Next as
2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Hans de Goede]

Hi,

On 7/6/23 11:19, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:
>> Some ACPI glue code (1) may want to do an acpi_device_id match while
>> it only has a struct acpi_device available because the first physical
>> node may not have been instantiated yet.
>>
>> Add a new acpi_match_acpi_device() helper for this, which takes
>> a "struct acpi_device *" as argument rather then the "struct device *"
>> which acpi_match_device() takes.
>>
>> 1) E.g. code which parses ACPI tables to transforms them
>> into more standard kernel data structures like fwnodes
> 
> Looks like it's v1 of my original patch, anyway this is now in Linux Next as
> 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").

Ah interesting, it does indeed look a lot like your version.
but it was developed independently.

Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp
changes in this series which rely on this are intended for 6.6-rc1 too.

So we still need to figure out how to merge this.

Regards,

Hans

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 02:29:35PM +0200, Hans de Goede wrote:
> On 7/6/23 11:19, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:

...

> > Looks like it's v1 of my original patch, anyway this is now in Linux Next as
> > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").
> 
> Ah interesting, it does indeed look a lot like your version.
> but it was developed independently.

Very interesting! It's a second patch of me that collides with someone's else
work.

> Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp
> changes in this series which rely on this are intended for 6.6-rc1 too.
> 
> So we still need to figure out how to merge this.

Standard way? I.e. ask Rafael for immutable tag/branch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Rafael J. Wysocki]

On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 7/6/23 11:19, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:
> >> Some ACPI glue code (1) may want to do an acpi_device_id match while
> >> it only has a struct acpi_device available because the first physical
> >> node may not have been instantiated yet.
> >>
> >> Add a new acpi_match_acpi_device() helper for this, which takes
> >> a "struct acpi_device *" as argument rather then the "struct device *"
> >> which acpi_match_device() takes.
> >>
> >> 1) E.g. code which parses ACPI tables to transforms them
> >> into more standard kernel data structures like fwnodes
> >
> > Looks like it's v1 of my original patch, anyway this is now in Linux Next as
> > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").
>
> Ah interesting, it does indeed look a lot like your version.
> but it was developed independently.
>
> Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp
> changes in this series which rely on this are intended for 6.6-rc1 too.

No, the material Andy is talking about will be pushed for 6.5-rc1
(probably even today), because it is part of a fix for systems that
are broken in the field.

> So we still need to figure out how to merge this.

This shouldn't be a problem.

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Hans de Goede]

Hi,

On 7/6/23 15:26, Rafael J. Wysocki wrote:
> On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 7/6/23 11:19, Andy Shevchenko wrote:
>>> On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:
>>>> Some ACPI glue code (1) may want to do an acpi_device_id match while
>>>> it only has a struct acpi_device available because the first physical
>>>> node may not have been instantiated yet.
>>>>
>>>> Add a new acpi_match_acpi_device() helper for this, which takes
>>>> a "struct acpi_device *" as argument rather then the "struct device *"
>>>> which acpi_match_device() takes.
>>>>
>>>> 1) E.g. code which parses ACPI tables to transforms them
>>>> into more standard kernel data structures like fwnodes
>>>
>>> Looks like it's v1 of my original patch, anyway this is now in Linux Next as
>>> 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").
>>
>> Ah interesting, it does indeed look a lot like your version.
>> but it was developed independently.
>>
>> Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp
>> changes in this series which rely on this are intended for 6.6-rc1 too.
> 
> No, the material Andy is talking about will be pushed for 6.5-rc1
> (probably even today), because it is part of a fix for systems that
> are broken in the field.
> 
>> So we still need to figure out how to merge this.
> 
> This shouldn't be a problem.

Great, thank you.

Regards,

Hans

Re: [PATCH v3 15/18] ACPI: bus: Introduce acpi_match_acpi_device() function

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 03:26:20PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jul 6, 2023 at 2:29 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 7/6/23 11:19, Andy Shevchenko wrote:
> > > On Wed, Jul 05, 2023 at 11:30:07PM +0200, Hans de Goede wrote:

...

> > > Looks like it's v1 of my original patch, anyway this is now in Linux Next as
> > > 2b5ae9604949 ("ACPI: bus: Introduce acpi_match_acpi_device() helper").
> >
> > Ah interesting, it does indeed look a lot like your version.
> > but it was developed independently.
> >
> > Unfortunately it seems that this is headed for 6.6-rc1 and the atomisp
> > changes in this series which rely on this are intended for 6.6-rc1 too.
> 
> No, the material Andy is talking about will be pushed for 6.5-rc1
> (probably even today), because it is part of a fix for systems that
> are broken in the field.

Oh, totally forgot about that.

> > So we still need to figure out how to merge this.
> 
> This shouldn't be a problem.

True, and thank you!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 16/18] 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 instantiation 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 an ISP2400 + OV5693 sensor.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Add a table with per sensor-HID atomisp_sensor_config settings
  for sensors which have lanes != 1 or which may have a VCM
  (VCM support is added in a follow-up patch)
- Switch to acpi_handle_err() for logging errors
- Set orientation based on CSI link/port
---
 drivers/staging/media/atomisp/Kconfig         |   3 +
 .../staging/media/atomisp/pci/atomisp_csi2.h  |  67 ----
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 337 ++++--------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |   1 +
 4 files changed, 65 insertions(+), 343 deletions(-)

diff --git a/drivers/staging/media/atomisp/Kconfig b/drivers/staging/media/atomisp/Kconfig
index e9b168ba97bf..5d8917160d41 100644
--- a/drivers/staging/media/atomisp/Kconfig
+++ b/drivers/staging/media/atomisp/Kconfig
@@ -12,9 +12,12 @@ menuconfig INTEL_ATOMISP
 config VIDEO_ATOMISP
 	tristate "Intel Atom Image Signal Processor Driver"
 	depends on VIDEO_DEV && INTEL_ATOMISP
+	depends on MEDIA_PCI_SUPPORT
 	depends on PMIC_OPREGION
+	depends on I2C
 	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..551c6fd244fd 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,18 @@ 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 },
+struct atomisp_sensor_config {
+	int lanes;
 };
 
+#define ATOMISP_SENSOR_CONFIG(_HID, _LANES)				\
+{									\
+	.id = _HID,							\
+	.driver_data = (long)&((const struct atomisp_sensor_config) {	\
+		.lanes = _LANES,					\
+	})								\
+}
+
 /*
  * gmin_cfg parsing code. This is a cleaned up version of the gmin_cfg parsing
  * code from atomisp_gmin_platform.c.
@@ -400,8 +380,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 +448,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 +473,68 @@ 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 const struct acpi_device_id atomisp_sensor_configs[] = {
+	ATOMISP_SENSOR_CONFIG("INT33BE", 2),	/* OV5693 */
+	{}
 };
 
-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;
-
-	sensor->local_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_CSI2_ENDPOINT]);
-	sensor->remote_ref[0] = SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_SENSOR_ENDPOINT]);
-
-	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);
-
-	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);
-	}
-}
-
-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;
+	const struct acpi_device_id *id;
 	int ret, clock_num;
+	int lanes = 1;
 
-	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
-		if (!adev->status.enabled)
-			continue;
+	id = acpi_match_acpi_device(atomisp_sensor_configs, adev);
+	if (id) {
+		struct atomisp_sensor_config *cfg =
+			(struct atomisp_sensor_config *)id->driver_data;
 
-		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++;
+		lanes = cfg->lanes;
 	}
 
-	return 0;
+	/*
+	 * 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);
 
-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;
-}
+	ret = atomisp_csi2_set_pmc_clk_freq(adev, clock_num);
+	if (ret)
+		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;
+	sensor->link = atomisp_csi2_get_port(adev, clock_num);
+	if (sensor->link >= ATOMISP_CAMERA_NR_PORTS) {
+		acpi_handle_err(adev->handle, "%s: Invalid port: %u\n",
+				dev_name(&adev->dev), sensor->link);
+		return -EINVAL;
 	}
 
-	return 0;
+	sensor->lanes = gmin_cfg_get_int(adev, "CsiLanes", lanes);
+	if (sensor->lanes > IPU_MAX_LANES) {
+		acpi_handle_err(adev->handle, "%s: Invalid lane-count: %d\n",
+				dev_name(&adev->dev), sensor->lanes);
+		return -EINVAL;
+	}
 
-err_unregister_sensors:
-	atomisp_csi2_unregister_sensors(bridge);
-	return ret;
+	ret = atomisp_csi2_add_gpio_mappings(adev);
+	if (ret)
+		return ret;
+
+	sensor->mclkspeed = PMC_CLK_RATE_19_2MHZ;
+	sensor->rotation = 0;
+	sensor->orientation = (sensor->link == 1) ?
+		V4L2_FWNODE_ORIENTATION_BACK : V4L2_FWNODE_ORIENTATION_FRONT;
+
+	return 0;
 }
 
 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 +546,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 v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Hans de Goede]

acpi_handle_info() uses the ACPI path to the handle as prefix for messages
e.g. : "\_SB_.I2C2.CAM8".

This makes it hard for users to figure out which csi2-bridge messages
belong to which sensor since the actual sensor drivers uses the ACPI
device name (typically "HID:00") for logging.

Extend the acpi_handle_info() (and err and warn) logging to also log
the device name to make it easier to match csi2-bridge messages with
sensor driver log messages.

Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 51 ++++++++++++-------
 1 file changed, 34 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index 551c6fd244fd..8124be486e2e 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -131,7 +131,8 @@ static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
 			if (!val)
 				break;
 
-			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
+			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
+					 dev_name(&adev->dev), key, val);
 			break;
 		}
 	}
@@ -156,7 +157,8 @@ static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key
 		if (strcmp(key, gv->key))
 			continue;
 
-		acpi_handle_info(adev->handle, "Using DMI entry %s=%s\n", key, gv->val);
+		acpi_handle_info(adev->handle, "%s: Using DMI entry %s=%s\n",
+				 dev_name(&adev->dev), key, gv->val);
 		return kstrdup(gv->val, GFP_KERNEL);
 	}
 
@@ -192,7 +194,8 @@ static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int defau
 	return int_val;
 
 out_use_default:
-	acpi_handle_info(adev->handle, "Using default %s=%d\n", key, default_val);
+	acpi_handle_info(adev->handle, "%s: Using default %s=%d\n",
+			 dev_name(&adev->dev), key, default_val);
 	return default_val;
 }
 
@@ -235,7 +238,8 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
 	ACPI_FREE(buffer.pointer);
 
 	if (ret < 0)
-		acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n");
+		acpi_handle_warn(adev->handle, "%s: Could not find PMC clk in _PR0\n",
+				 dev_name(&adev->dev));
 
 	return ret;
 }
@@ -254,7 +258,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
 	clk = clk_get(NULL, name);
 	if (IS_ERR(clk)) {
 		ret = PTR_ERR(clk);
-		acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret);
+		acpi_handle_err(adev->handle, "%s: Error getting clk %s: %d\n",
+				dev_name(&adev->dev), name, ret);
 		return ret;
 	}
 
@@ -268,7 +273,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
 	if (!ret)
 		ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ);
 	if (ret)
-		acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret);
+		acpi_handle_err(adev->handle, "%s: Error setting clk-rate for %s: %d\n",
+				dev_name(&adev->dev), name, ret);
 
 	clk_put(clk);
 	return ret;
@@ -317,7 +323,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
 
 	if (i == data->settings_count) {
 		acpi_handle_warn(data->adev->handle,
-				 "Could not find DSM GPIO settings for pin %u\n", pin);
+				 "%s: Could not find DSM GPIO settings for pin %u\n",
+				 dev_name(&data->adev->dev), pin);
 		return 1;
 	}
 
@@ -329,7 +336,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
 		name = "powerdown-gpios";
 		break;
 	default:
-		acpi_handle_warn(data->adev->handle, "Unknown GPIO type 0x%02lx for pin %u\n",
+		acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n",
+				 dev_name(&data->adev->dev),
 				 INTEL_GPIO_DSM_TYPE(settings), pin);
 		return 1;
 	}
@@ -354,7 +362,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
 	data->map->mapping[i].size = 1;
 	data->map_count++;
 
-	acpi_handle_info(data->adev->handle, "%s crs %d %s pin %u active-%s\n", name,
+	acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n",
+			 dev_name(&data->adev->dev), name,
 			 data->res_count - 1, agpio->resource_source.string_ptr,
 			 pin, active_low ? "low" : "high");
 
@@ -391,7 +400,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 	obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
 				      0x00, 1, NULL, ACPI_TYPE_STRING);
 	if (obj) {
-		acpi_handle_info(adev->handle, "Sensor module id: '%s'\n", obj->string.pointer);
+		acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n",
+				 dev_name(&adev->dev), obj->string.pointer);
 		ACPI_FREE(obj);
 	}
 
@@ -405,7 +415,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 				      &intel_sensor_gpio_info_guid, 0x00, 1,
 				      NULL, ACPI_TYPE_INTEGER);
 	if (!obj) {
-		acpi_handle_err(adev->handle, "No _DSM entry for GPIO pin count\n");
+		acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n",
+				dev_name(&adev->dev));
 		return -EIO;
 	}
 
@@ -413,7 +424,9 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 	ACPI_FREE(obj);
 
 	if (data.settings_count > CSI2_MAX_ACPI_GPIOS) {
-		acpi_handle_err(adev->handle, "Too many GPIOs %u > %u\n", data.settings_count, CSI2_MAX_ACPI_GPIOS);
+		acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n",
+				dev_name(&adev->dev), data.settings_count,
+				CSI2_MAX_ACPI_GPIOS);
 		return -EOVERFLOW;
 	}
 
@@ -427,7 +440,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 					      0x00, i + 2,
 					      NULL, ACPI_TYPE_INTEGER);
 		if (!obj) {
-			acpi_handle_err(adev->handle, "No _DSM entry for pin %u\n", i);
+			acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n",
+					dev_name(&adev->dev), i);
 			return -EIO;
 		}
 
@@ -442,7 +456,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 			    INTEL_GPIO_DSM_PIN(data.settings[j]))
 				continue;
 
-			acpi_handle_err(adev->handle, "Duplicate pin number %lu\n",
+			acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n",
+					dev_name(&adev->dev),
 					INTEL_GPIO_DSM_PIN(data.settings[i]));
 			return -EIO;
 		}
@@ -463,12 +478,14 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 
 	if (data.map_count != data.settings_count ||
 	    data.res_count != data.settings_count)
-		acpi_handle_warn(adev->handle, "ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
-				 data.settings_count, data.res_count, data.map_count);
+		acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
+				 dev_name(&adev->dev), data.settings_count,
+				 data.res_count, data.map_count);
 
 	ret = acpi_dev_add_driver_gpios(adev, data.map->mapping);
 	if (ret)
-		acpi_handle_err(adev->handle, "Error adding driver GPIOs: %d\n", ret);
+		acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n",
+				dev_name(&adev->dev), ret);
 
 	return ret;
 }
-- 
2.41.0

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:
> acpi_handle_info() uses the ACPI path to the handle as prefix for messages
> e.g. : "\_SB_.I2C2.CAM8".
> 
> This makes it hard for users to figure out which csi2-bridge messages
> belong to which sensor since the actual sensor drivers uses the ACPI
> device name (typically "HID:00") for logging.
> 
> Extend the acpi_handle_info() (and err and warn) logging to also log
> the device name to make it easier to match csi2-bridge messages with
> sensor driver log messages.

Fine by me, one suggestion below (up to you, guys)
Reviewed-by: Andy Shevchenko <andy@kernel.org>

> Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/pci/atomisp_csi2_bridge.c   | 51 ++++++++++++-------
>  1 file changed, 34 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> index 551c6fd244fd..8124be486e2e 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> @@ -131,7 +131,8 @@ static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
>  			if (!val)
>  				break;
>  
> -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> +					 dev_name(&adev->dev), key, val);

Maybe (maybe!) it's a candidate to have something like

v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
thinking about it?

>  			break;
>  		}
>  	}
> @@ -156,7 +157,8 @@ static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key
>  		if (strcmp(key, gv->key))
>  			continue;
>  
> -		acpi_handle_info(adev->handle, "Using DMI entry %s=%s\n", key, gv->val);
> +		acpi_handle_info(adev->handle, "%s: Using DMI entry %s=%s\n",
> +				 dev_name(&adev->dev), key, gv->val);
>  		return kstrdup(gv->val, GFP_KERNEL);
>  	}
>  
> @@ -192,7 +194,8 @@ static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int defau
>  	return int_val;
>  
>  out_use_default:
> -	acpi_handle_info(adev->handle, "Using default %s=%d\n", key, default_val);
> +	acpi_handle_info(adev->handle, "%s: Using default %s=%d\n",
> +			 dev_name(&adev->dev), key, default_val);
>  	return default_val;
>  }
>  
> @@ -235,7 +238,8 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
>  	ACPI_FREE(buffer.pointer);
>  
>  	if (ret < 0)
> -		acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n");
> +		acpi_handle_warn(adev->handle, "%s: Could not find PMC clk in _PR0\n",
> +				 dev_name(&adev->dev));
>  
>  	return ret;
>  }
> @@ -254,7 +258,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
>  	clk = clk_get(NULL, name);
>  	if (IS_ERR(clk)) {
>  		ret = PTR_ERR(clk);
> -		acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret);
> +		acpi_handle_err(adev->handle, "%s: Error getting clk %s: %d\n",
> +				dev_name(&adev->dev), name, ret);
>  		return ret;
>  	}
>  
> @@ -268,7 +273,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
>  	if (!ret)
>  		ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ);
>  	if (ret)
> -		acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret);
> +		acpi_handle_err(adev->handle, "%s: Error setting clk-rate for %s: %d\n",
> +				dev_name(&adev->dev), name, ret);
>  
>  	clk_put(clk);
>  	return ret;
> @@ -317,7 +323,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
>  
>  	if (i == data->settings_count) {
>  		acpi_handle_warn(data->adev->handle,
> -				 "Could not find DSM GPIO settings for pin %u\n", pin);
> +				 "%s: Could not find DSM GPIO settings for pin %u\n",
> +				 dev_name(&data->adev->dev), pin);
>  		return 1;
>  	}
>  
> @@ -329,7 +336,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
>  		name = "powerdown-gpios";
>  		break;
>  	default:
> -		acpi_handle_warn(data->adev->handle, "Unknown GPIO type 0x%02lx for pin %u\n",
> +		acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n",
> +				 dev_name(&data->adev->dev),
>  				 INTEL_GPIO_DSM_TYPE(settings), pin);
>  		return 1;
>  	}
> @@ -354,7 +362,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
>  	data->map->mapping[i].size = 1;
>  	data->map_count++;
>  
> -	acpi_handle_info(data->adev->handle, "%s crs %d %s pin %u active-%s\n", name,
> +	acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n",
> +			 dev_name(&data->adev->dev), name,
>  			 data->res_count - 1, agpio->resource_source.string_ptr,
>  			 pin, active_low ? "low" : "high");
>  
> @@ -391,7 +400,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  	obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
>  				      0x00, 1, NULL, ACPI_TYPE_STRING);
>  	if (obj) {
> -		acpi_handle_info(adev->handle, "Sensor module id: '%s'\n", obj->string.pointer);
> +		acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n",
> +				 dev_name(&adev->dev), obj->string.pointer);
>  		ACPI_FREE(obj);
>  	}
>  
> @@ -405,7 +415,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  				      &intel_sensor_gpio_info_guid, 0x00, 1,
>  				      NULL, ACPI_TYPE_INTEGER);
>  	if (!obj) {
> -		acpi_handle_err(adev->handle, "No _DSM entry for GPIO pin count\n");
> +		acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n",
> +				dev_name(&adev->dev));
>  		return -EIO;
>  	}
>  
> @@ -413,7 +424,9 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  	ACPI_FREE(obj);
>  
>  	if (data.settings_count > CSI2_MAX_ACPI_GPIOS) {
> -		acpi_handle_err(adev->handle, "Too many GPIOs %u > %u\n", data.settings_count, CSI2_MAX_ACPI_GPIOS);
> +		acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n",
> +				dev_name(&adev->dev), data.settings_count,
> +				CSI2_MAX_ACPI_GPIOS);
>  		return -EOVERFLOW;
>  	}
>  
> @@ -427,7 +440,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  					      0x00, i + 2,
>  					      NULL, ACPI_TYPE_INTEGER);
>  		if (!obj) {
> -			acpi_handle_err(adev->handle, "No _DSM entry for pin %u\n", i);
> +			acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n",
> +					dev_name(&adev->dev), i);
>  			return -EIO;
>  		}
>  
> @@ -442,7 +456,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  			    INTEL_GPIO_DSM_PIN(data.settings[j]))
>  				continue;
>  
> -			acpi_handle_err(adev->handle, "Duplicate pin number %lu\n",
> +			acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n",
> +					dev_name(&adev->dev),
>  					INTEL_GPIO_DSM_PIN(data.settings[i]));
>  			return -EIO;
>  		}
> @@ -463,12 +478,14 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
>  
>  	if (data.map_count != data.settings_count ||
>  	    data.res_count != data.settings_count)
> -		acpi_handle_warn(adev->handle, "ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
> -				 data.settings_count, data.res_count, data.map_count);
> +		acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
> +				 dev_name(&adev->dev), data.settings_count,
> +				 data.res_count, data.map_count);
>  
>  	ret = acpi_dev_add_driver_gpios(adev, data.map->mapping);
>  	if (ret)
> -		acpi_handle_err(adev->handle, "Error adding driver GPIOs: %d\n", ret);
> +		acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n",
> +				dev_name(&adev->dev), ret);
>  
>  	return ret;
>  }
> -- 
> 2.41.0
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Laurent Pinchart]

On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:
> > acpi_handle_info() uses the ACPI path to the handle as prefix for messages
> > e.g. : "\_SB_.I2C2.CAM8".
> > 
> > This makes it hard for users to figure out which csi2-bridge messages
> > belong to which sensor since the actual sensor drivers uses the ACPI
> > device name (typically "HID:00") for logging.
> > 
> > Extend the acpi_handle_info() (and err and warn) logging to also log
> > the device name to make it easier to match csi2-bridge messages with
> > sensor driver log messages.
> 
> Fine by me, one suggestion below (up to you, guys)
> Reviewed-by: Andy Shevchenko <andy@kernel.org>
> 
> > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  .../media/atomisp/pci/atomisp_csi2_bridge.c   | 51 ++++++++++++-------
> >  1 file changed, 34 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> > index 551c6fd244fd..8124be486e2e 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
> > @@ -131,7 +131,8 @@ static char *gmin_cfg_get_dsm(struct acpi_device *adev, const char *key)
> >  			if (!val)
> >  				break;
> >  
> > -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> > +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> > +					 dev_name(&adev->dev), key, val);
> 
> Maybe (maybe!) it's a candidate to have something like
> 
> v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
> thinking about it?

Or acpi_dev_info() that would take an acpi_device pointer. Or just just
dev_info(&adev->dev) ?

> >  			break;
> >  		}
> >  	}
> > @@ -156,7 +157,8 @@ static char *gmin_cfg_get_dmi_override(struct acpi_device *adev, const char *key
> >  		if (strcmp(key, gv->key))
> >  			continue;
> >  
> > -		acpi_handle_info(adev->handle, "Using DMI entry %s=%s\n", key, gv->val);
> > +		acpi_handle_info(adev->handle, "%s: Using DMI entry %s=%s\n",
> > +				 dev_name(&adev->dev), key, gv->val);
> >  		return kstrdup(gv->val, GFP_KERNEL);
> >  	}
> >  
> > @@ -192,7 +194,8 @@ static int gmin_cfg_get_int(struct acpi_device *adev, const char *key, int defau
> >  	return int_val;
> >  
> >  out_use_default:
> > -	acpi_handle_info(adev->handle, "Using default %s=%d\n", key, default_val);
> > +	acpi_handle_info(adev->handle, "%s: Using default %s=%d\n",
> > +			 dev_name(&adev->dev), key, default_val);
> >  	return default_val;
> >  }
> >  
> > @@ -235,7 +238,8 @@ static int atomisp_csi2_get_pmc_clk_nr_from_acpi_pr0(struct acpi_device *adev)
> >  	ACPI_FREE(buffer.pointer);
> >  
> >  	if (ret < 0)
> > -		acpi_handle_warn(adev->handle, "Could not find PMC clk in _PR0\n");
> > +		acpi_handle_warn(adev->handle, "%s: Could not find PMC clk in _PR0\n",
> > +				 dev_name(&adev->dev));
> >  
> >  	return ret;
> >  }
> > @@ -254,7 +258,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
> >  	clk = clk_get(NULL, name);
> >  	if (IS_ERR(clk)) {
> >  		ret = PTR_ERR(clk);
> > -		acpi_handle_err(adev->handle, "Error getting clk %s:%d\n", name, ret);
> > +		acpi_handle_err(adev->handle, "%s: Error getting clk %s: %d\n",
> > +				dev_name(&adev->dev), name, ret);
> >  		return ret;
> >  	}
> >  
> > @@ -268,7 +273,8 @@ static int atomisp_csi2_set_pmc_clk_freq(struct acpi_device *adev, int clock_num
> >  	if (!ret)
> >  		ret = clk_set_rate(clk, PMC_CLK_RATE_19_2MHZ);
> >  	if (ret)
> > -		acpi_handle_err(adev->handle, "Error setting clk-rate for %s:%d\n", name, ret);
> > +		acpi_handle_err(adev->handle, "%s: Error setting clk-rate for %s: %d\n",
> > +				dev_name(&adev->dev), name, ret);
> >  
> >  	clk_put(clk);
> >  	return ret;
> > @@ -317,7 +323,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
> >  
> >  	if (i == data->settings_count) {
> >  		acpi_handle_warn(data->adev->handle,
> > -				 "Could not find DSM GPIO settings for pin %u\n", pin);
> > +				 "%s: Could not find DSM GPIO settings for pin %u\n",
> > +				 dev_name(&data->adev->dev), pin);
> >  		return 1;
> >  	}
> >  
> > @@ -329,7 +336,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
> >  		name = "powerdown-gpios";
> >  		break;
> >  	default:
> > -		acpi_handle_warn(data->adev->handle, "Unknown GPIO type 0x%02lx for pin %u\n",
> > +		acpi_handle_warn(data->adev->handle, "%s: Unknown GPIO type 0x%02lx for pin %u\n",
> > +				 dev_name(&data->adev->dev),
> >  				 INTEL_GPIO_DSM_TYPE(settings), pin);
> >  		return 1;
> >  	}
> > @@ -354,7 +362,8 @@ static int atomisp_csi2_handle_acpi_gpio_res(struct acpi_resource *ares, void *_
> >  	data->map->mapping[i].size = 1;
> >  	data->map_count++;
> >  
> > -	acpi_handle_info(data->adev->handle, "%s crs %d %s pin %u active-%s\n", name,
> > +	acpi_handle_info(data->adev->handle, "%s: %s crs %d %s pin %u active-%s\n",
> > +			 dev_name(&data->adev->dev), name,
> >  			 data->res_count - 1, agpio->resource_source.string_ptr,
> >  			 pin, active_low ? "low" : "high");
> >  
> > @@ -391,7 +400,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  	obj = acpi_evaluate_dsm_typed(adev->handle, &intel_sensor_module_guid,
> >  				      0x00, 1, NULL, ACPI_TYPE_STRING);
> >  	if (obj) {
> > -		acpi_handle_info(adev->handle, "Sensor module id: '%s'\n", obj->string.pointer);
> > +		acpi_handle_info(adev->handle, "%s: Sensor module id: '%s'\n",
> > +				 dev_name(&adev->dev), obj->string.pointer);
> >  		ACPI_FREE(obj);
> >  	}
> >  
> > @@ -405,7 +415,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  				      &intel_sensor_gpio_info_guid, 0x00, 1,
> >  				      NULL, ACPI_TYPE_INTEGER);
> >  	if (!obj) {
> > -		acpi_handle_err(adev->handle, "No _DSM entry for GPIO pin count\n");
> > +		acpi_handle_err(adev->handle, "%s: No _DSM entry for GPIO pin count\n",
> > +				dev_name(&adev->dev));
> >  		return -EIO;
> >  	}
> >  
> > @@ -413,7 +424,9 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  	ACPI_FREE(obj);
> >  
> >  	if (data.settings_count > CSI2_MAX_ACPI_GPIOS) {
> > -		acpi_handle_err(adev->handle, "Too many GPIOs %u > %u\n", data.settings_count, CSI2_MAX_ACPI_GPIOS);
> > +		acpi_handle_err(adev->handle, "%s: Too many GPIOs %u > %u\n",
> > +				dev_name(&adev->dev), data.settings_count,
> > +				CSI2_MAX_ACPI_GPIOS);
> >  		return -EOVERFLOW;
> >  	}
> >  
> > @@ -427,7 +440,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  					      0x00, i + 2,
> >  					      NULL, ACPI_TYPE_INTEGER);
> >  		if (!obj) {
> > -			acpi_handle_err(adev->handle, "No _DSM entry for pin %u\n", i);
> > +			acpi_handle_err(adev->handle, "%s: No _DSM entry for pin %u\n",
> > +					dev_name(&adev->dev), i);
> >  			return -EIO;
> >  		}
> >  
> > @@ -442,7 +456,8 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  			    INTEL_GPIO_DSM_PIN(data.settings[j]))
> >  				continue;
> >  
> > -			acpi_handle_err(adev->handle, "Duplicate pin number %lu\n",
> > +			acpi_handle_err(adev->handle, "%s: Duplicate pin number %lu\n",
> > +					dev_name(&adev->dev),
> >  					INTEL_GPIO_DSM_PIN(data.settings[i]));
> >  			return -EIO;
> >  		}
> > @@ -463,12 +478,14 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
> >  
> >  	if (data.map_count != data.settings_count ||
> >  	    data.res_count != data.settings_count)
> > -		acpi_handle_warn(adev->handle, "ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
> > -				 data.settings_count, data.res_count, data.map_count);
> > +		acpi_handle_warn(adev->handle, "%s: ACPI GPIO resources vs DSM GPIO-info count mismatch (dsm: %d res: %d map %d\n",
> > +				 dev_name(&adev->dev), data.settings_count,
> > +				 data.res_count, data.map_count);
> >  
> >  	ret = acpi_dev_add_driver_gpios(adev, data.map->mapping);
> >  	if (ret)
> > -		acpi_handle_err(adev->handle, "Error adding driver GPIOs: %d\n", ret);
> > +		acpi_handle_err(adev->handle, "%s: Error adding driver GPIOs: %d\n",
> > +				dev_name(&adev->dev), ret);
> >  
> >  	return ret;
> >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote:
> On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:

...

> > > -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> > > +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> > > +					 dev_name(&adev->dev), key, val);
> > 
> > Maybe (maybe!) it's a candidate to have something like
> > 
> > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
> > thinking about it?
> 
> Or acpi_dev_info() that would take an acpi_device pointer.

(which is an equivalent to the below)

> Or just just dev_info(&adev->dev) ?

The point is to print ACPI handle *and* device name. There are no existing
helpers for that.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Laurent Pinchart]

On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote:
> > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote:
> > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:
> 
> ...
> 
> > > > -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> > > > +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> > > > +					 dev_name(&adev->dev), key, val);
> > > 
> > > Maybe (maybe!) it's a candidate to have something like
> > > 
> > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
> > > thinking about it?
> > 
> > Or acpi_dev_info() that would take an acpi_device pointer.
> 
> (which is an equivalent to the below)
> 
> > Or just just dev_info(&adev->dev) ?
> 
> The point is to print ACPI handle *and* device name. There are no existing
> helpers for that.

Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that.
It shouldn't be V4L2-specific in my opinion.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 04:07:08PM +0300, Laurent Pinchart wrote:
> On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote:
> > > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:

...

> > > > > -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> > > > > +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> > > > > +					 dev_name(&adev->dev), key, val);
> > > > 
> > > > Maybe (maybe!) it's a candidate to have something like
> > > > 
> > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
> > > > thinking about it?
> > > 
> > > Or acpi_dev_info() that would take an acpi_device pointer.
> > 
> > (which is an equivalent to the below)
> > 
> > > Or just just dev_info(&adev->dev) ?
> > 
> > The point is to print ACPI handle *and* device name. There are no existing
> > helpers for that.
> 
> Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that.
> It shouldn't be V4L2-specific in my opinion.

But
1) so far seems only v4l2 is interested in this information (I haven't checked
   for existing users outside of v4l2);
2) the proposed naming doesn't suggest it's also about ACPI handle. :-)

Although ACPI seems a good common denominator for these, indeed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 17/18] media: atomisp: csi2-bridge: Add dev_name() to acpi_handle_info() logging

[Sakari Ailus]

On Thu, Jul 06, 2023 at 04:07:08PM +0300, Laurent Pinchart wrote:
> On Thu, Jul 06, 2023 at 03:23:33PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 06, 2023 at 02:12:24PM +0300, Laurent Pinchart wrote:
> > > On Thu, Jul 06, 2023 at 01:09:29PM +0300, Andy Shevchenko wrote:
> > > > On Wed, Jul 05, 2023 at 11:30:09PM +0200, Hans de Goede wrote:
> > 
> > ...
> > 
> > > > > -			acpi_handle_info(adev->handle, "Using DSM entry %s=%s\n", key, val);
> > > > > +			acpi_handle_info(adev->handle, "%s: Using DSM entry %s=%s\n",
> > > > > +					 dev_name(&adev->dev), key, val);
> > > > 
> > > > Maybe (maybe!) it's a candidate to have something like
> > > > 
> > > > v4l2_acpi_log_info(adev, ...) which combines both and unloads the code from
> > > > thinking about it?
> > > 
> > > Or acpi_dev_info() that would take an acpi_device pointer.
> > 
> > (which is an equivalent to the below)
> > 
> > > Or just just dev_info(&adev->dev) ?
> > 
> > The point is to print ACPI handle *and* device name. There are no existing
> > helpers for that.
> 
> Then a new acpi_dev_info(struct acpi_device *adev, ...) could do that.
> It shouldn't be V4L2-specific in my opinion.

I certainy have no objections for having a helper for this, but IMO the
current code is fine, too.

-- 
Sakari Ailus

[PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Hans de Goede]

Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
the v4l2-async bound op so that an I2C-client will be instatiated for
the VCM.

Note unfortunately on atomisp the _DSM to get the VCM type sometimes
returns a VCM even though there is none. Since VCMs are typically only
used together with certain sensors, work around this by adding a vcm
field to atomisp_sensor_config and only check for a VCM when that is set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_csi2_bridge.c   | 44 ++++++++++++++++++-
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index 8124be486e2e..92693206e4ca 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -66,15 +66,25 @@ static const guid_t atomisp_dsm_guid =
 	GUID_INIT(0xdc2f6c4f, 0x045b, 0x4f1d,
 		  0x97, 0xb9, 0x88, 0x2a, 0x68, 0x60, 0xa4, 0xbe);
 
+/*
+ * 75c9a639-5c8a-4a00-9f48-a9c3b5da789f
+ * This _DSM GUID returns a string giving the VCM type e.g. "AD5823".
+ */
+static const guid_t vcm_dsm_guid =
+	GUID_INIT(0x75c9a639, 0x5c8a, 0x4a00,
+		  0x9f, 0x48, 0xa9, 0xc3, 0xb5, 0xda, 0x78, 0x9f);
+
 struct atomisp_sensor_config {
 	int lanes;
+	bool vcm;
 };
 
-#define ATOMISP_SENSOR_CONFIG(_HID, _LANES)				\
+#define ATOMISP_SENSOR_CONFIG(_HID, _LANES, _VCM)			\
 {									\
 	.id = _HID,							\
 	.driver_data = (long)&((const struct atomisp_sensor_config) {	\
 		.lanes = _LANES,					\
+		.vcm = _VCM,						\
 	})								\
 }
 
@@ -490,8 +500,28 @@ static int atomisp_csi2_add_gpio_mappings(struct acpi_device *adev)
 	return ret;
 }
 
+static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
+{
+	union acpi_object *obj;
+	char *vcm_type;
+
+	obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
+				      NULL, ACPI_TYPE_STRING);
+	if (!obj)
+		return NULL;
+
+	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
+	ACPI_FREE(obj);
+
+	if (!vcm_type)
+		return NULL;
+
+	string_lower(vcm_type, vcm_type);
+	return vcm_type;
+}
+
 static const struct acpi_device_id atomisp_sensor_configs[] = {
-	ATOMISP_SENSOR_CONFIG("INT33BE", 2),	/* OV5693 */
+	ATOMISP_SENSOR_CONFIG("INT33BE", 2, true),	/* OV5693 */
 	{}
 };
 
@@ -500,6 +530,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
 {
 	const struct acpi_device_id *id;
 	int ret, clock_num;
+	bool vcm = false;
 	int lanes = 1;
 
 	id = acpi_match_acpi_device(atomisp_sensor_configs, adev);
@@ -508,6 +539,7 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
 			(struct atomisp_sensor_config *)id->driver_data;
 
 		lanes = cfg->lanes;
+		vcm = cfg->vcm;
 	}
 
 	/*
@@ -545,6 +577,9 @@ static int atomisp_csi2_parse_sensor_fwnode(struct acpi_device *adev,
 	sensor->orientation = (sensor->link == 1) ?
 		V4L2_FWNODE_ORIENTATION_BACK : V4L2_FWNODE_ORIENTATION_FRONT;
 
+	if (vcm)
+		sensor->vcm_type = atomisp_csi2_get_vcm_type(adev);
+
 	return 0;
 }
 
@@ -583,6 +618,7 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
 {
 	struct atomisp_device *isp = notifier_to_atomisp(notifier);
 	struct sensor_async_subdev *s_asd = to_sensor_asd(asd);
+	int ret;
 
 	if (s_asd->port >= ATOMISP_CAMERA_NR_PORTS) {
 		dev_err(isp->dev, "port %d not supported\n", s_asd->port);
@@ -594,6 +630,10 @@ static int atomisp_notifier_bound(struct v4l2_async_notifier *notifier,
 		return -EBUSY;
 	}
 
+	ret = ipu_bridge_instantiate_vcm(sd->dev);
+	if (ret)
+		return ret;
+
 	isp->sensor_subdevs[s_asd->port] = sd;
 	return 0;
 }
-- 
2.41.0

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Andy Shevchenko]

On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
> the v4l2-async bound op so that an I2C-client will be instatiated for
> the VCM.
> 
> Note unfortunately on atomisp the _DSM to get the VCM type sometimes
> returns a VCM even though there is none. Since VCMs are typically only
> used together with certain sensors, work around this by adding a vcm
> field to atomisp_sensor_config and only check for a VCM when that is set.

...

> +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
> +{
> +	union acpi_object *obj;
> +	char *vcm_type;
> +
> +	obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
> +				      NULL, ACPI_TYPE_STRING);
> +	if (!obj)
> +		return NULL;
> +
> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);

Where is the counterpart kfree()?

> +	ACPI_FREE(obj);
> +
> +	if (!vcm_type)
> +		return NULL;
> +
> +	string_lower(vcm_type, vcm_type);
> +	return vcm_type;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Hans de Goede]

Hi,

On 7/6/23 12:15, Andy Shevchenko wrote:
> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>> Fill sensor->vcm_type and call intel_cio2_bridge_instantiate_vcm() from
>> the v4l2-async bound op so that an I2C-client will be instatiated for
>> the VCM.
>>
>> Note unfortunately on atomisp the _DSM to get the VCM type sometimes
>> returns a VCM even though there is none. Since VCMs are typically only
>> used together with certain sensors, work around this by adding a vcm
>> field to atomisp_sensor_config and only check for a VCM when that is set.
> 
> ...
> 
>> +static char *atomisp_csi2_get_vcm_type(struct acpi_device *adev)
>> +{
>> +	union acpi_object *obj;
>> +	char *vcm_type;
>> +
>> +	obj = acpi_evaluate_dsm_typed(adev->handle, &vcm_dsm_guid, 0, 0,
>> +				      NULL, ACPI_TYPE_STRING);
>> +	if (!obj)
>> +		return NULL;
>> +
>> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
> 
> Where is the counterpart kfree()?

The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
code only creates those once and them leaves them in memory, even on
a rmmod. So this is deliberately leaked just like that the ipu_bridge
struct which contains all the swnode-s is deliberately leaked by
ipu-bridge.c

Regards,

Hans



> 
>> +	ACPI_FREE(obj);
>> +
>> +	if (!vcm_type)
>> +		return NULL;
>> +
>> +	string_lower(vcm_type, vcm_type);
>> +	return vcm_type;
>> +}
> 

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
> On 7/6/23 12:15, Andy Shevchenko wrote:
> > On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:

> >> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
> > 
> > Where is the counterpart kfree()?
> 
> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
> code only creates those once and them leaves them in memory, even on
> a rmmod. So this is deliberately leaked just like that the ipu_bridge
> struct which contains all the swnode-s is deliberately leaked by
> ipu-bridge.c

Should we worry about those leakages?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Hans de Goede]

Hi,

On 7/6/23 14:42, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
>> On 7/6/23 12:15, Andy Shevchenko wrote:
>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> 
>>>> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>
>>> Where is the counterpart kfree()?
>>
>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
>> code only creates those once and them leaves them in memory, even on
>> a rmmod. So this is deliberately leaked just like that the ipu_bridge
>> struct which contains all the swnode-s is deliberately leaked by
>> ipu-bridge.c
> 
> Should we worry about those leakages?

No this is by design because removing the swnodes while e.g. a sensor
driver might still be bound to the i2c-client is trouble-some and
the callers of ipu_bridge_init check if it has already run and then
skip calling it.

So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
ipu_bridge_init() will not get called a second time. Instead
the old swnodes (1) which are already set as secondary fwnodes for
the sensor and bridge devices are re-used.

Regards,

Hans


1) + the properties they contain

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Andy Shevchenko]

On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote:
> On 7/6/23 14:42, Andy Shevchenko wrote:
> > On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
> >> On 7/6/23 12:15, Andy Shevchenko wrote:
> >>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
> > 
> >>>> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
> >>>
> >>> Where is the counterpart kfree()?
> >>
> >> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
> >> code only creates those once and them leaves them in memory, even on
> >> a rmmod. So this is deliberately leaked just like that the ipu_bridge
> >> struct which contains all the swnode-s is deliberately leaked by
> >> ipu-bridge.c
> > 
> > Should we worry about those leakages?
> 
> No this is by design because removing the swnodes while e.g. a sensor
> driver might still be bound to the i2c-client is trouble-some and
> the callers of ipu_bridge_init check if it has already run and then
> skip calling it.
> 
> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
> ipu_bridge_init() will not get called a second time. Instead
> the old swnodes (1) which are already set as secondary fwnodes for
> the sensor and bridge devices are re-used.

But this will be actual leak if we hot unplug/plug back the device, right?
(I think we can do that in some [debug?] cases).

Whatever, it's out of scope of this series...

> 1) + the properties they contain

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 18/18] media: atomisp: csi2-bridge: Add support for VCM I2C-client instantiation

[Hans de Goede]

Hi,

On 7/6/23 14:56, Andy Shevchenko wrote:
> On Thu, Jul 06, 2023 at 02:47:36PM +0200, Hans de Goede wrote:
>> On 7/6/23 14:42, Andy Shevchenko wrote:
>>> On Thu, Jul 06, 2023 at 02:31:14PM +0200, Hans de Goede wrote:
>>>> On 7/6/23 12:15, Andy Shevchenko wrote:
>>>>> On Wed, Jul 05, 2023 at 11:30:10PM +0200, Hans de Goede wrote:
>>>
>>>>>> +	vcm_type = kstrdup(obj->string.pointer, GFP_KERNEL);
>>>>>
>>>>> Where is the counterpart kfree()?
>>>>
>>>> The vcm-type is stored in one of the generated sw-nodes and the ipu-bridge
>>>> code only creates those once and them leaves them in memory, even on
>>>> a rmmod. So this is deliberately leaked just like that the ipu_bridge
>>>> struct which contains all the swnode-s is deliberately leaked by
>>>> ipu-bridge.c
>>>
>>> Should we worry about those leakages?
>>
>> No this is by design because removing the swnodes while e.g. a sensor
>> driver might still be bound to the i2c-client is trouble-some and
>> the callers of ipu_bridge_init check if it has already run and then
>> skip calling it.
>>
>> So after a rmmod + modprobe of the atomisp / ipu3-cio2 driver
>> ipu_bridge_init() will not get called a second time. Instead
>> the old swnodes (1) which are already set as secondary fwnodes for
>> the sensor and bridge devices are re-used.
> 
> But this will be actual leak if we hot unplug/plug back the device, right?
> (I think we can do that in some [debug?] cases).

No, the kstrdup() is called from the parse_sensor_fwnode() callback passed
to ipu_bridge_init(), so it will only happen once even on unbind + re-bind
or rmmod + modprobe.

Regards,

Hans