Hi,
On 11/2/21 00:43, Daniel Scally wrote:
> Hi Hans
<snip>
>> 2. I need some help with all the fwnode link stuff (I'm not very familiar
>> with this). There seems to be a chicken and egg problem here though,
>> because the v4l2subdev for the VCM does not register because of async stuff
>> and if we add it to the "graph" then my idea to enumerate the VCMs
>> from the SSDB on the complete() callback won't work. But we can do this
>> on a per sensor basis instead from the cio2_notifier_bound() callback
>> instead I guess ?
>
>
> I think on top of your work in the cio2-bridge for patch 3 you can do this:
>
>
> 1. Create another software node against the cio2_sensor struct, with the
> name coming from the vcm_types array
>
> 2. Assign that software node to board_info.swnode in
> cio2_bridge_instantiate_vcm_i2c_client()
>
> 3. Add another entry to dev_properties for the sensor, that is named
> "lens-focus" and contains a reference to the software_node created in #2
> just like the references to the sensor/cio2 nodes.
>
>
> This way when the sensor driver calls
> v4l2_async_register_subdev_sensor() it should create a notifier that
> looks for that VCM client to bind. I think then rather than putting
> anything in the .bound() / .complete() callbacks, we should modify core
> to do _something_ when async matching some subdevs. The something would
> depend on the kind of devices that match, for example with the sensor
> driver and the ipu3-cio2 driver, there's an entity whos function is
> MEDIA_ENT_F_VID_IF_BRIDGE matching to an entity whos function is
> MEDIA_ENT_F_CAM_SENSOR, and it seems to me that every scenario like that
> is going to result in media pad links being created. Similarly for our
> sensor that's a device with entity function MEDIA_ENT_F_LENS matching to
> MEDIA_ENT_F_CAM_SENSOR, and I think that in those cases we can create
> either an interface link or a new kind of link (maybe
> "MEDIA_LNK_FL_ANCILLARY_LINK" or something...) between the two to show
> that they form a single logical unit, which we can then report to libcamera.
>
>
> Hope that makes sense...
Ok, so I gave this a try, see the attached patches, but the v4l2-subdev for
the VCM still does not show up.
I think that instead I need to build a full link between the sensor
and the VCM similar to the cio2 <-> sensor link. Both ends of that link
have:
<base-swnode attached to the device>
|
--<port-swnode named (SWNODE_GRAPH_PORT_NAME_FMT, X), where X is 0 on the
| sensor side and the link nr on the cio2 side
|
--<end-point-swnode named (SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0)
And then the 2 endpoints contain a swref property pointing to the
other endpoint swnode.
I think we need a similar setup adding a swnode child named
(SWNODE_GRAPH_PORT_NAME_FMT, 1), to the nodes[SWNODE_SENSOR_HID] node.
Note 1, since 0 is the "port" to the cio2, this new port child then
gets an endpoint "0" child itself, likewise we add a "port 0" child
to the vcm swnode, with a "endpoint 0" child below that and then have
the 2 endpoints contain swref properties pointing to each other.
I think that this will properly make the VCm part of the graph and
will make its v4l2-subdev get instantiated when the graph is
complete. Before I spend a bunch of time on implementing this,
let me ask this:
Does this sound reasonable / like I'm heading in the right direction?
Regards,
Hans
p.s.
I have found a new solution for the probe-ordering problem which
is patch 2 of the attached patches, I personally I'm happy with
this solution. I hope you like it too.
From 5443552f9eb179e8b1286da6ce5a67579af39bf1 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Mon, 1 Nov 2021 13:59:23 +0100
Subject: [PATCH] i2c: acpi: Add i2c_acpi_new_device_by_fwnode() function
Change i2c_acpi_new_device() into i2c_acpi_new_device_by_fwnode() and
add a static inline wrapper providing the old i2c_acpi_new_device()
behavior.
This is necessary because in some cases we may only have access
to the fwnode / acpi_device and not to the matching physical-node
struct device *.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/i2c/i2c-core-acpi.c | 17 +++++++++++------
include/linux/i2c.h | 17 +++++++++++++----
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 71eee5bc17ab..6331ca037b80 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -480,8 +480,9 @@ struct notifier_block i2c_acpi_notifier = {
};
/**
- * i2c_acpi_new_device - Create i2c-client for the Nth I2cSerialBus resource
- * @dev: Device owning the ACPI resources to get the client from
+ * i2c_acpi_new_device_by_fwnode - Create i2c-client for the Nth I2cSerialBus
+ * resource
+ * @fwnode: fwnode with the ACPI resources to get the client from
* @index: Index of ACPI resource to get
* @info: describes the I2C device; note this is modified (addr gets set)
* Context: can sleep
@@ -497,15 +498,19 @@ struct notifier_block i2c_acpi_notifier = {
* Returns a pointer to the new i2c-client, or error pointer in case of failure.
* Specifically, -EPROBE_DEFER is returned if the adapter is not found.
*/
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
- struct i2c_board_info *info)
+struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
+ int index,
+ struct i2c_board_info *info)
{
- struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct acpi_device *adev = to_acpi_device_node(fwnode);
struct i2c_acpi_lookup lookup;
struct i2c_adapter *adapter;
LIST_HEAD(resource_list);
int ret;
+ if (!adev)
+ return ERR_PTR(-ENODEV);
+
memset(&lookup, 0, sizeof(lookup));
lookup.info = info;
lookup.device_handle = acpi_device_handle(adev);
@@ -527,7 +532,7 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
return i2c_new_client_device(adapter, info);
}
-EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
+EXPORT_SYMBOL_GPL(i2c_acpi_new_device_by_fwnode);
#ifdef CONFIG_ACPI_I2C_OPREGION
static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 2ce3efbe9198..3967523fff36 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -1012,8 +1012,9 @@ bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
struct acpi_resource_i2c_serialbus **i2c);
int i2c_acpi_client_count(struct acpi_device *adev);
u32 i2c_acpi_find_bus_speed(struct device *dev);
-struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
- struct i2c_board_info *info);
+struct i2c_client *i2c_acpi_new_device_by_fwnode(struct fwnode_handle *fwnode,
+ int index,
+ struct i2c_board_info *info);
struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
#else
static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
@@ -1029,8 +1030,9 @@ static inline u32 i2c_acpi_find_bus_speed(struct device *dev)
{
return 0;
}
-static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
- int index, struct i2c_board_info *info)
+static inline struct i2c_client *i2c_acpi_new_device_by_fwnode(
+ struct fwnode_handle *fwnode, int index,
+ struct i2c_board_info *info)
{
return ERR_PTR(-ENODEV);
}
@@ -1040,4 +1042,11 @@ static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle ha
}
#endif /* CONFIG_ACPI */
+static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
+ int index,
+ struct i2c_board_info *info)
+{
+ return i2c_acpi_new_device_by_fwnode(dev->fwnode, index, info);
+}
+
#endif /* _LINUX_I2C_H */
--
2.31.1
From 6558941359385481828c641c259adbe2b529670a Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Mon, 8 Nov 2021 10:41:26 +0100
Subject: [PATCH 2/4] media: ipu3-cio2: Defer probing until the PMIC is fully
setup
On devices where things are not fully describe in devicetree (1)
and where the code thus falls back to calling cio2_bridge_init(),
the i2c-clients for any VCMs also need to be instantiated manually.
The VCM can be probed by its driver as soon as the code instantiates
the i2c-client and this probing must not happen before the PMIC is
fully setup.
Make cio2_bridge_init() return -EPROBE_DEFER when the PMIC is not
fully-setup, deferring the probe of the ipu3-cio2 driver.
This is a preparation patch for adding VCM enumeration support to
the ipu3-cio2-bridge code.
1) Through embedding of devicetree info in the ACPI tables
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/media/pci/intel/ipu3/cio2-bridge.c | 37 ++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index 4550be801311..e337780e9328 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -308,6 +308,40 @@ static int cio2_bridge_connect_sensors(struct cio2_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.
+ */
+int cio2_bridge_sensors_are_ready(void)
+{
+ struct acpi_device *adev;
+ bool ready = true;
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
+ const struct cio2_sensor_config *cfg =
+ &cio2_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 cio2_bridge_init(struct pci_dev *cio2)
{
struct device *dev = &cio2->dev;
@@ -316,6 +350,9 @@ int cio2_bridge_init(struct pci_dev *cio2)
unsigned int i;
int ret;
+ if (!cio2_bridge_sensors_are_ready())
+ return -EPROBE_DEFER;
+
bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
if (!bridge)
return -ENOMEM;
--
2.31.1
From 8e482241dd70ef853668640a6df6ec64fad37a2f Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Mon, 8 Nov 2021 10:47:56 +0100
Subject: [PATCH 3/4] media: ipu3-cio2: Call cio2_bridge_init() before anything
else
Since cio2_bridge_init() may now return -EPROBE_DEFER it is best to
call it before anything else.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
index 76fd4e6e8e46..097f2fe6e605 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c
@@ -1713,11 +1713,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
struct cio2_device *cio2;
int r;
- cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
- if (!cio2)
- return -ENOMEM;
- cio2->pci_dev = pci_dev;
-
/*
* On some platforms no connections to sensors are defined in firmware,
* if the device has no endpoints then we can try to build those as
@@ -1735,6 +1730,11 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
return r;
}
+ cio2 = devm_kzalloc(dev, sizeof(*cio2), GFP_KERNEL);
+ if (!cio2)
+ return -ENOMEM;
+ cio2->pci_dev = pci_dev;
+
r = pcim_enable_device(pci_dev);
if (r) {
dev_err(dev, "failed to enable device (%d)\n", r);
--
2.31.1
From 0cc0dad1d61b5fd14bc533183ecb543578a8f44b Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Mon, 8 Nov 2021 10:57:50 +0100
Subject: [PATCH 4/4] media: ipu3-cio2: Add support for instantiating
i2c-clients for VCMs
Some sensors come with a variable-focus lens where the lens focus is
controller by a VCM (Voice Coil Motor). If there is a VCM for the
lens-focus, and if so which one, is described on the vcm_type field
of the ACPI SSDB table.
These VCMs are a second I2C device listed as an extra I2cSerialBusV2
resource in the same ACPI device as the sensor. The i2c-core-acpi.c
code only instantiates an i2c-client for the first I2cSerialBusV2
resource.
Add support for instantiating an i2c-client for the VCM with
the type of the i2c-client set based on the SSDB vcm_type field.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/media/pci/intel/ipu3/cio2-bridge.c | 56 ++++++++++++++++++++++
drivers/media/pci/intel/ipu3/cio2-bridge.h | 16 ++++++-
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.c b/drivers/media/pci/intel/ipu3/cio2-bridge.c
index e337780e9328..ff8196b960e7 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.c
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.c
@@ -3,6 +3,7 @@
#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>
@@ -38,6 +39,18 @@ static const struct cio2_property_names prop_names = {
.link_frequencies = "link-frequencies",
};
+static const char * const cio2_vcm_types[] = {
+ "ad5823",
+ "dw9714",
+ "ad5816",
+ "dw9719",
+ "dw9718",
+ "dw9806b",
+ "wv517s",
+ "lc898122xa",
+ "lc898212axb",
+};
+
static int cio2_bridge_read_acpi_buffer(struct acpi_device *adev, char *id,
void *data, u32 size)
{
@@ -134,6 +147,13 @@ static void cio2_bridge_create_fwnode_properties(
sensor->dev_properties[2] = PROPERTY_ENTRY_U32(
sensor->prop_names.orientation,
orientation);
+ if (sensor->ssdb.vcmtype) {
+ sensor->vcm_ref[0] =
+ SOFTWARE_NODE_REFERENCE(&sensor->swnodes[SWNODE_VCM]);
+ sensor->dev_properties[3] =
+ PROPERTY_ENTRY_REF_ARRAY("lens-focus",
+ sensor->vcm_ref);
+ }
sensor->ep_properties[0] = PROPERTY_ENTRY_U32(
sensor->prop_names.bus_type,
@@ -195,6 +215,33 @@ static void cio2_bridge_create_connection_swnodes(struct cio2_bridge *bridge,
sensor->node_names.endpoint,
&nodes[SWNODE_CIO2_PORT],
sensor->cio2_properties);
+ if (sensor->ssdb.vcmtype)
+ nodes[SWNODE_VCM] = NODE_VCM(
+ cio2_vcm_types[sensor->ssdb.vcmtype - 1]);
+}
+
+static void cio2_bridge_instantiate_vcm_i2c_client(struct cio2_sensor *sensor)
+{
+ struct i2c_board_info board_info = { };
+ char name[16];
+
+ if (!sensor->ssdb.vcmtype)
+ return;
+
+ snprintf(name, sizeof(name), "%s-VCM", acpi_dev_name(sensor->adev));
+ board_info.dev_name = name;
+ strscpy(board_info.type, cio2_vcm_types[sensor->ssdb.vcmtype - 1],
+ 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;
+ }
}
static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
@@ -207,6 +254,7 @@ static void cio2_bridge_unregister_sensors(struct cio2_bridge *bridge)
software_node_unregister_nodes(sensor->swnodes);
ACPI_FREE(sensor->pld);
acpi_dev_put(sensor->adev);
+ i2c_unregister_device(sensor->vcm_i2c_client);
}
}
@@ -239,6 +287,12 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
if (ret)
goto err_put_adev;
+ if (sensor->ssdb.vcmtype > ARRAY_SIZE(cio2_vcm_types)) {
+ dev_warn(&adev->dev, "Unknown VCM type %d\n",
+ sensor->ssdb.vcmtype);
+ sensor->ssdb.vcmtype = 0;
+ }
+
status = acpi_get_physical_device_location(adev->handle, &sensor->pld);
if (ACPI_FAILURE(status)) {
ret = -ENODEV;
@@ -269,6 +323,8 @@ static int cio2_bridge_connect_sensor(const struct cio2_sensor_config *cfg,
sensor->adev = acpi_dev_get(adev);
adev->fwnode.secondary = fwnode;
+ cio2_bridge_instantiate_vcm_i2c_client(sensor);
+
dev_info(&cio2->dev, "Found supported sensor %s\n",
acpi_dev_name(adev));
diff --git a/drivers/media/pci/intel/ipu3/cio2-bridge.h b/drivers/media/pci/intel/ipu3/cio2-bridge.h
index 202c7d494f7a..4418cbd08208 100644
--- a/drivers/media/pci/intel/ipu3/cio2-bridge.h
+++ b/drivers/media/pci/intel/ipu3/cio2-bridge.h
@@ -8,6 +8,8 @@
#include "ipu3-cio2.h"
+struct i2c_client;
+
#define CIO2_HID "INT343E"
#define CIO2_MAX_LANES 4
#define MAX_NUM_LINK_FREQS 3
@@ -42,12 +44,19 @@
.properties = _PROPS, \
}
+#define NODE_VCM(_TYPE) \
+ (const struct software_node) { \
+ .name = _TYPE, \
+ }
+
enum cio2_sensor_swnodes {
SWNODE_SENSOR_HID,
SWNODE_SENSOR_PORT,
SWNODE_SENSOR_ENDPOINT,
SWNODE_CIO2_PORT,
SWNODE_CIO2_ENDPOINT,
+ /* Must be last because it is optional / maybe empty */
+ SWNODE_VCM,
SWNODE_COUNT
};
@@ -106,8 +115,10 @@ struct cio2_sensor_config {
struct cio2_sensor {
char name[ACPI_ID_LEN];
struct acpi_device *adev;
+ struct i2c_client *vcm_i2c_client;
- struct software_node swnodes[6];
+ /* SWNODE_COUNT + 1 for terminating empty node */
+ struct software_node swnodes[SWNODE_COUNT + 1];
struct cio2_node_names node_names;
struct cio2_sensor_ssdb ssdb;
@@ -115,10 +126,11 @@ struct cio2_sensor {
struct cio2_property_names prop_names;
struct property_entry ep_properties[5];
- struct property_entry dev_properties[4];
+ struct property_entry dev_properties[5];
struct property_entry cio2_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];
};
struct cio2_bridge {
--
2.31.1
