[PATCH v2 0/8] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support

[PATCH v2 0/8] ACPI _CRS CSI-2 and MIPI DisCo for Imaging support

[Sakari Ailus]

Hello all,

Here's an implementation of ACPI 6.4 _CRS CSI-2 resource descriptor and
MIPI DisCo for Imaging 1.0 [1]. What the two basically provide is an
officially sanctioned way to describe CSI-2 connected cameras to operating
system software, something DT based systems have enjoyed for quite some
time already.

The implementation digs the information from ACPI tables (_CRS descriptors
and data + property extensions) and constructs software nodes that are
compatible with Documentation/firmware-guide/acpi/dsd/graph.rst and
Documentation/devicetree/bindings/media/video-interface-devices.yaml . No
specific driver changes are needed.

These patches are on the testing branch of the linux-acpi tree where they
depend on the patch constifying the ACPI pathname argument for
acpi_get_handle() (commit 91fdb91ccca2b48572a1ccf1d382fd599e3e1237).

[1] https://www.mipi.org/specifications/mipi-disco-imaging

since v1:

- Update copyright notices.

- Include linux/types.h instead of linux/kernel.h in drivers/acpi/mipi.c.

- Use SWNODE_GRAPH_PORT_NAME_FMT instead of plain "port@%u" in
  GRAPH_PORT_NAME macro.

- Make the condition in NEXT_PROPERTY() macro easier to read.

- Unwrap lines to make them moderately longer than 80 characters.

- Use * BITS_PER_TYPE(u8) instead of << 3 to convert bytes to bits in
  init_port_csi2_common().

- Test ACPI framework call success using ACPI_SUCCESS() instead of
  comparing with AE_OK. Likewise for ACPI_FAILURE and != AE_OK.

- Use newly added SOFTWARE_NODE() macro to construct the root software
  node.

- Use str_has_prefix() to test for a string prefix instead of memcmp().

- Add pr_fmt() macro to drivers/acpi/property.c.

- Move logical or operators to the end of the line in
  acpi_properties_prepare().

- Improve bad node type error in acpi_parse_string_ref().

Sakari Ailus (8):
  ACPI: property: Parse data node string references in properties
  ACPI: property: Parse _CRS CSI-2 descriptor
  device property: Add SOFTWARE_NODE() macro for defining software nodes
  ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging
  ACPI: property: Dig "rotation" property for devices with CSI2 _CRS
  ACPI: property: Rename parsed MIPI DisCo for Imaging properties
  ACPI: property: Skip MIPI property table without "mipi-img" prefix
  ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support

 drivers/acpi/Makefile    |   2 +-
 drivers/acpi/internal.h  |   9 +
 drivers/acpi/mipi.c      | 761 +++++++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c  | 128 +++++--
 drivers/acpi/scan.c      |  33 +-
 include/acpi/acpi_bus.h  |  61 ++++
 include/linux/property.h |   7 +
 7 files changed, 972 insertions(+), 29 deletions(-)
 create mode 100644 drivers/acpi/mipi.c

-- 
2.30.2

[PATCH v2 1/8] ACPI: property: Parse data node string references in properties

[Sakari Ailus]

Add support for parsing property references using strings, besides
reference objects that were previously supported. This allows also
referencing data nodes which was not possible with reference objects.

Also add pr_fmt() macro to prefix printouts.

While at it, update copyright.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/property.c | 106 ++++++++++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index b8d9eb9a433ed..ae1f8259e76a5 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -2,14 +2,17 @@
 /*
  * ACPI device specific properties support.
  *
- * Copyright (C) 2014, Intel Corporation
+ * Copyright (C) 2014--2023, Intel Corporation
  * All rights reserved.
  *
  * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
  *          Darren Hart <dvhart@linux.intel.com>
  *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *	    Sakari Ailus <sakari.ailus@linux.intel.com>
  */
 
+#define pr_fmt(fmt) "ACPI: " fmt
+
 #include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/export.h>
@@ -795,7 +798,8 @@ acpi_fwnode_get_named_child_node(const struct fwnode_handle *fwnode,
 static int acpi_get_ref_args(struct fwnode_reference_args *args,
 			     struct fwnode_handle *ref_fwnode,
 			     const union acpi_object **element,
-			     const union acpi_object *end, size_t num_args)
+			     const union acpi_object *end, size_t num_args,
+			     bool subnode_string)
 {
 	u32 nargs = 0, i;
 
@@ -803,13 +807,16 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	 * Find the referred data extension node under the
 	 * referred device node.
 	 */
-	for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
-	     (*element)++) {
-		const char *child_name = (*element)->string.pointer;
-
-		ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode, child_name);
-		if (!ref_fwnode)
-			return -EINVAL;
+	if (subnode_string) {
+		for (; *element < end && (*element)->type == ACPI_TYPE_STRING;
+		     (*element)++) {
+			const char *child_name = (*element)->string.pointer;
+
+			ref_fwnode = acpi_fwnode_get_named_child_node(ref_fwnode,
+								      child_name);
+			if (!ref_fwnode)
+				return -EINVAL;
+		}
 	}
 
 	/*
@@ -820,7 +827,8 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	for (i = 0; (*element) + i < end && i < num_args; i++) {
 		acpi_object_type type = (*element)[i].type;
 
-		if (type == ACPI_TYPE_LOCAL_REFERENCE)
+		if (type == ACPI_TYPE_LOCAL_REFERENCE ||
+		    (!subnode_string && type == ACPI_TYPE_STRING))
 			break;
 
 		if (type == ACPI_TYPE_INTEGER)
@@ -844,6 +852,43 @@ static int acpi_get_ref_args(struct fwnode_reference_args *args,
 	return 0;
 }
 
+static struct fwnode_handle *
+acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
+{
+	acpi_handle scope, handle;
+	struct acpi_data_node *dn;
+	struct acpi_device *device;
+	acpi_status status;
+
+	if (is_acpi_device_node(fwnode)) {
+		scope = to_acpi_device_node(fwnode)->handle;
+	} else if (is_acpi_data_node(fwnode)) {
+		scope = to_acpi_data_node(fwnode)->handle;
+	} else {
+		pr_err("bad node type for node %pfw\n", fwnode);
+		return ERR_PTR(-EINVAL);
+	}
+
+	status = acpi_get_handle(scope, refstring, &handle);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_debug(scope, "can't get handle for %s", refstring);
+		return ERR_PTR(-EINVAL);
+	}
+
+	device = acpi_fetch_acpi_dev(handle);
+	if (device)
+		return acpi_fwnode_handle(device);
+
+	status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
+				    (void **)&dn, NULL);
+	if (ACPI_FAILURE(status) || !dn) {
+		acpi_handle_debug(handle, "can't find subnode");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &dn->fwnode;
+}
+
 /**
  * __acpi_node_get_property_reference - returns handle to the referenced object
  * @fwnode: Firmware node to get the property from
@@ -886,6 +931,7 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 	const union acpi_object *element, *end;
 	const union acpi_object *obj;
 	const struct acpi_device_data *data;
+	struct fwnode_handle *ref_fwnode;
 	struct acpi_device *device;
 	int ret, idx = 0;
 
@@ -909,16 +955,29 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 
 		args->fwnode = acpi_fwnode_handle(device);
 		args->nargs = 0;
+		return 0;
+	case ACPI_TYPE_STRING:
+		if (index)
+			return -ENOENT;
+
+		ref_fwnode = acpi_parse_string_ref(fwnode, obj->string.pointer);
+		if (IS_ERR(ref_fwnode))
+			return PTR_ERR(ref_fwnode);
+
+		args->fwnode = ref_fwnode;
+		args->nargs = 0;
+
 		return 0;
 	case ACPI_TYPE_PACKAGE:
 		/*
 		 * If it is not a single reference, then it is a package of
-		 * references followed by number of ints as follows:
+		 * references, followed by number of ints as follows:
 		 *
 		 *  Package () { REF, INT, REF, INT, INT }
 		 *
-		 * The index argument is then used to determine which reference
-		 * the caller wants (along with the arguments).
+		 * Here, REF may be either a local reference or a string. The
+		 * index argument is then used to determine which reference the
+		 * caller wants (along with the arguments).
 		 */
 		break;
 	default:
@@ -942,7 +1001,26 @@ int __acpi_node_get_property_reference(const struct fwnode_handle *fwnode,
 
 			ret = acpi_get_ref_args(idx == index ? args : NULL,
 						acpi_fwnode_handle(device),
-						&element, end, num_args);
+						&element, end, num_args, true);
+			if (ret < 0)
+				return ret;
+
+			if (idx == index)
+				return 0;
+
+			break;
+		case ACPI_TYPE_STRING:
+			ref_fwnode =
+				acpi_parse_string_ref(fwnode,
+						      element->string.pointer);
+			if (IS_ERR(ref_fwnode))
+				return PTR_ERR(ref_fwnode);
+
+			element++;
+
+			ret = acpi_get_ref_args(idx == index ? args : NULL,
+						ref_fwnode, &element, end,
+						num_args, false);
 			if (ret < 0)
 				return ret;
 
-- 
2.30.2

[PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Sakari Ailus]

Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
configuration. For now, only figure out where the descriptor is present in
order to allow adding information from it to related devices.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/Makefile   |   2 +-
 drivers/acpi/internal.h |   7 +
 drivers/acpi/mipi.c     | 320 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c     |  16 +-
 include/acpi/acpi_bus.h |  11 ++
 5 files changed, 351 insertions(+), 5 deletions(-)
 create mode 100644 drivers/acpi/mipi.c

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index a5b649e71ab1b..a98fa1bc15548 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -37,7 +37,7 @@ acpi-$(CONFIG_ACPI_SLEEP)	+= proc.o
 # ACPI Bus and Device Drivers
 #
 acpi-y				+= bus.o glue.o
-acpi-y				+= scan.o
+acpi-y				+= scan.o mipi.o
 acpi-y				+= resource.o
 acpi-y				+= acpi_processor.o
 acpi-y				+= processor_core.o
diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index ec584442fb298..1ec4aa92bf17a 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -282,4 +282,11 @@ void acpi_init_lpit(void);
 static inline void acpi_init_lpit(void) { }
 #endif
 
+/*--------------------------------------------------------------------------
+				ACPI and MIPI DisCo for Imaging conversion
+  -------------------------------------------------------------------------- */
+
+void acpi_crs_csi2_swnodes_del_free(void);
+void acpi_bus_scan_crs_csi2(acpi_handle handle);
+
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
new file mode 100644
index 0000000000000..2a8cd8ee074e3
--- /dev/null
+++ b/drivers/acpi/mipi.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MIPI DisCo for Imaging support.
+ *
+ * Copyright (C) 2023 Intel Corporation
+ */
+
+#include <linux/acpi.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/overflow.h>
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/sort.h>
+#include <linux/string.h>
+
+#include "internal.h"
+
+struct crs_csi2_swnodes {
+	struct list_head list;
+	acpi_handle handle;
+	struct acpi_device_software_nodes *ads;
+};
+
+static LIST_HEAD(crs_csi2_swnodes);
+
+static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes)
+{
+	list_del(&swnodes->list);
+	kfree(swnodes);
+}
+
+void acpi_crs_csi2_swnodes_del_free(void)
+{
+	struct crs_csi2_swnodes *swnodes, *swnodes_tmp;
+
+	list_for_each_entry_safe(swnodes, swnodes_tmp, &crs_csi2_swnodes, list)
+		crs_csi2_swnode_del_free(swnodes);
+}
+
+struct crs_csi2_instance {
+	struct list_head list;
+	struct acpi_resource_csi2_serialbus csi2;
+	acpi_handle remote_handle;
+	char remote_name[];
+};
+
+struct crs_csi2 {
+	struct list_head list;
+	acpi_handle handle;
+	struct list_head buses;
+};
+
+struct scan_check_crs_csi2_context {
+	struct list_head res_list;
+	acpi_handle handle;
+};
+
+static acpi_status scan_check_crs_csi2_instance(struct acpi_resource *res,
+						void *context)
+{
+	struct scan_check_crs_csi2_context *inst_context = context;
+	struct acpi_resource_csi2_serialbus *csi2;
+	struct crs_csi2_instance *inst;
+	acpi_handle remote_handle;
+	acpi_status status;
+
+	if (res->type != ACPI_RESOURCE_TYPE_SERIAL_BUS)
+		return AE_OK;
+
+	csi2 = &res->data.csi2_serial_bus;
+
+	if (csi2->type != ACPI_RESOURCE_SERIAL_TYPE_CSI2)
+		return AE_OK;
+
+	if (!csi2->resource_source.string_length) {
+		acpi_handle_debug(inst_context->handle,
+				  "invalid resource source string length\n");
+		return AE_OK;
+	}
+
+	status = acpi_get_handle(NULL, csi2->resource_source.string_ptr,
+				 &remote_handle);
+	if (status != AE_OK) {
+		acpi_handle_warn(inst_context->handle,
+				 "cannot get handle for %s\n",
+				 csi2->resource_source.string_ptr);
+		return AE_OK;
+	}
+
+	inst = kmalloc(struct_size(inst, remote_name,
+				   csi2->resource_source.string_length),
+		       GFP_KERNEL);
+	if (!inst)
+		return AE_OK;
+
+	inst->csi2 = *csi2;
+	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
+	       csi2->resource_source.string_length);
+	inst->csi2.resource_source.string_ptr = inst->remote_name;
+	inst->remote_handle = remote_handle;
+
+	list_add(&inst->list, &inst_context->res_list);
+
+	return AE_OK;
+}
+
+static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level,
+				       void *context, void **ret)
+{
+	struct scan_check_crs_csi2_context inst_context = {
+		.handle = handle,
+		.res_list = LIST_HEAD_INIT(inst_context.res_list),
+	};
+	struct list_head *list = context;
+	struct crs_csi2 *csi2;
+
+	INIT_LIST_HEAD(&inst_context.res_list);
+
+	acpi_walk_resources(handle, METHOD_NAME__CRS,
+			    scan_check_crs_csi2_instance, &inst_context);
+
+	if (list_empty(&inst_context.res_list))
+		return AE_OK;
+
+	csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL);
+	if (!csi2)
+		return AE_OK;
+
+	csi2->handle = handle;
+	list_replace(&inst_context.res_list, &csi2->buses);
+	list_add(&csi2->list, list);
+
+	return AE_OK;
+}
+
+struct acpi_handle_ref {
+	acpi_handle handle;
+	unsigned int count;
+};
+
+#define NO_CSI2_PORT (~1U)
+
+static int crs_handle_cmp(const void *__a, const void *__b)
+{
+	const struct acpi_handle_ref *a = __a, *b = __b;
+
+	return a->handle < b->handle ? -1 : a->handle > b->handle;
+}
+
+static void crs_csi2_release(struct list_head *crs_csi2_handles)
+{
+	struct crs_csi2 *csi2, *csi2_tmp;
+
+	list_for_each_entry_safe(csi2, csi2_tmp, crs_csi2_handles, list) {
+		struct crs_csi2_instance *inst, *inst_tmp;
+
+		list_for_each_entry_safe(inst, inst_tmp, &csi2->buses, list) {
+			list_del(&inst->list);
+			kfree(inst);
+		}
+
+		list_del(&csi2->list);
+		kfree(csi2);
+	}
+}
+
+/**
+ * acpi_bus_scan_crs_csi2 - Scan a device and its child devices for _CRS CSI-2
+ *
+ * @handle: ACPI handle to scan
+ *
+ * This function does a number of things:
+ *
+ * 1. Scan an ACPI device and its children for _CRS CSI-2 instances. The
+ *    instances are stored for later use.
+ *
+ * 2. Count how many references to other devices _CRS CSI-2 instances have in
+ *    total.
+ *
+ * 3. Count the number of references to other devices for each _CRS CSI-2
+ *    instance.
+ *
+ * 4. Allocate memory for swnodes each ACPI device requires later on, and
+ *    generate a list of such allocations.
+ *
+ * Note that struct acpi_device isn't available yet at this time.
+ *
+ * acpi_scan_lock in scan.c must be held when calling this function.
+ */
+void acpi_bus_scan_crs_csi2(acpi_handle handle)
+{
+	struct acpi_handle_ref *handle_refs;
+	struct acpi_handle_ref *this = NULL;
+	LIST_HEAD(crs_csi2_handles);
+	unsigned int handle_count = 0, this_count;
+	unsigned int curr = 0;
+	struct crs_csi2 *csi2;
+
+	/* Collect the devices that have a _CRS CSI-2 resource */
+	acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, ACPI_UINT32_MAX,
+			    scan_check_crs_csi2, NULL, &crs_csi2_handles, NULL);
+
+	/*
+	 * Figure out how much temporary storage we need for counting
+	 * connections in each device.
+	 */
+	list_for_each_entry(csi2, &crs_csi2_handles, list) {
+		struct crs_csi2_instance *inst;
+
+		handle_count++;
+
+		list_for_each_entry(inst, &csi2->buses, list)
+			handle_count++;
+	}
+
+	/* No handles? Bail out here. */
+	if (!handle_count)
+		return;
+
+	handle_refs = kcalloc(handle_count + 1, sizeof(*handle_refs),
+			      GFP_KERNEL);
+	if (!handle_refs) {
+		acpi_handle_debug(handle, "no memory for %u handle refs\n",
+				  handle_count + 1);
+		return;
+	}
+
+	/* Associate handles to the number of references. */
+	list_for_each_entry(csi2, &crs_csi2_handles, list) {
+		struct crs_csi2_instance *inst;
+		struct acpi_handle_ref *handle_ref;
+
+		handle_ref = &handle_refs[curr++];
+		handle_ref->handle = csi2->handle;
+
+		list_for_each_entry(inst, &csi2->buses, list) {
+			handle_refs[curr].handle = inst->remote_handle;
+			handle_refs[curr].count = 1;
+			handle_ref->count++;
+			curr++;
+		}
+	}
+
+	handle_refs[curr].handle = NULL;
+
+	sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp,
+	     NULL);
+
+	/*
+	 * Finally count references in each handle, allocate space for device
+	 * specific ports, properties and fill the _CRS CSI2 descriptor
+	 * originated data.
+	 */
+	this = handle_refs;
+	this_count = this->count;
+	for (curr = 1; curr < handle_count + 1; curr++) {
+		struct acpi_device_software_nodes *ads;
+		struct crs_csi2_swnodes *swnodes;
+		size_t alloc_size;
+		void *end;
+
+		if (this->handle == handle_refs[curr].handle) {
+			this_count += handle_refs[curr].count;
+			continue;
+		}
+
+		/*
+		 * Allocate memory for ports, node pointers (number of nodes +
+		 * 1 (guardian), nodes (root + number of ports * 2 (for for
+		 * every port there is an endpoint)).
+		 */
+		if (check_mul_overflow(sizeof(*ads->ports) +
+				       sizeof(*ads->nodes) * 2 +
+				       sizeof(*ads->nodeptrs) * 2,
+				       (size_t)this_count, &alloc_size) ||
+		    check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
+				       sizeof(*ads->nodeptrs) * 2,
+				       alloc_size, &alloc_size)) {
+			acpi_handle_warn(handle, "too many handles (%u)",
+					 this_count);
+			continue;
+		}
+
+		swnodes = kzalloc(sizeof(*swnodes), GFP_KERNEL);
+		ads = kzalloc(alloc_size, GFP_KERNEL);
+		ads->ports = (void *)(ads + 1);
+		ads->nodes = (void *)(ads->ports + this_count);
+		ads->nodeptrs = (void *)(ads->nodes +
+					 this_count * 2 + 1);
+		end = ads->nodeptrs + this_count * 2 + 2;
+		if (!swnodes || !ads ||
+		    WARN_ON((void *)ads + alloc_size != end)) {
+			kfree(swnodes);
+			kfree(ads);
+			acpi_handle_debug(handle,
+					  "cannot allocate for %u swnodes\n",
+					  this_count);
+		} else {
+			unsigned int i;
+
+			ads->num_ports = this_count;
+			for (i = 0; i < this_count * 2 + 1; i++)
+				ads->nodeptrs[i] = &ads->nodes[i];
+			ads->nodeptrs[i] = NULL;
+			for (i = 0; i < this_count; i++)
+				ads->ports[i].port_nr = NO_CSI2_PORT;
+			swnodes->handle = this->handle;
+			swnodes->ads = ads;
+			list_add(&swnodes->list, &crs_csi2_swnodes);
+		}
+
+		this = &handle_refs[curr];
+		this_count = this->count;
+	}
+
+	kfree(handle_refs);
+
+	crs_csi2_release(&crs_csi2_handles);
+}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 0c6f06abe3f47..50de874b8f208 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -2423,9 +2423,12 @@ EXPORT_SYMBOL_GPL(acpi_dev_get_next_consumer_dev);
 int acpi_bus_scan(acpi_handle handle)
 {
 	struct acpi_device *device = NULL;
+	int ret = 0;
 
 	acpi_bus_scan_second_pass = false;
 
+	acpi_bus_scan_crs_csi2(handle);
+
 	/* Pass 1: Avoid enumerating devices with missing dependencies. */
 
 	if (ACPI_SUCCESS(acpi_bus_check_add(handle, true, &device)))
@@ -2433,13 +2436,15 @@ int acpi_bus_scan(acpi_handle handle)
 				    acpi_bus_check_add_1, NULL, NULL,
 				    (void **)&device);
 
-	if (!device)
-		return -ENODEV;
+	if (!device) {
+		ret = -ENODEV;
+		goto out_release;
+	}
 
 	acpi_bus_attach(device, (void *)true);
 
 	if (!acpi_bus_scan_second_pass)
-		return 0;
+		goto out_release;
 
 	/* Pass 2: Enumerate all of the remaining devices. */
 
@@ -2452,7 +2457,10 @@ int acpi_bus_scan(acpi_handle handle)
 
 	acpi_bus_attach(device, NULL);
 
-	return 0;
+out_release:
+	acpi_crs_csi2_swnodes_del_free();
+
+	return ret;
 }
 EXPORT_SYMBOL(acpi_bus_scan);
 
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index e44be31115a67..a05fe22c1175c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -360,6 +360,17 @@ struct acpi_device_data {
 
 struct acpi_gpio_mapping;
 
+struct acpi_device_software_node_port {
+	unsigned int port_nr;
+};
+
+struct acpi_device_software_nodes {
+	struct acpi_device_software_node_port *ports;
+	struct software_node *nodes;
+	const struct software_node **nodeptrs;
+	unsigned int num_ports;
+};
+
 /* Device */
 struct acpi_device {
 	u32 pld_crc;
-- 
2.30.2

[PATCH v2 3/8] device property: Add SOFTWARE_NODE() macro for defining software nodes

[Sakari Ailus]

Add SOFTWARE_NODE() macro in order to make defining software nodes look
nicer. This is analogous to different PROPERTY_ENTRY_*() macros for
defining properties.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/property.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/property.h b/include/linux/property.h
index 37179e3abad5c..6745a86bc9b97 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -477,6 +477,13 @@ struct software_node {
 	const struct property_entry *properties;
 };
 
+#define SOFTWARE_NODE(_name_, _properties_, _parent_)	\
+	(struct software_node) {			\
+		.name = _name_,				\
+		.properties = _properties_,		\
+		.parent = _parent_,			\
+	}
+
 bool is_software_node(const struct fwnode_handle *fwnode);
 const struct software_node *
 to_software_node(const struct fwnode_handle *fwnode);
-- 
2.30.2

[PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Sakari Ailus]

Generate software nodes for information parsed from ACPI _CRS for CSI-2 as
well as MIPI DisCo for Imaging spec. The software nodes are compliant with
existing ACPI or DT definitions and are parsed by relevant drivers without
changes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/internal.h |   1 +
 drivers/acpi/mipi.c     | 346 ++++++++++++++++++++++++++++++++++++++++
 drivers/acpi/scan.c     |  17 ++
 include/acpi/acpi_bus.h |  49 ++++++
 4 files changed, 413 insertions(+)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 1ec4aa92bf17a..fac87404e294c 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -288,5 +288,6 @@ static inline void acpi_init_lpit(void) { }
 
 void acpi_crs_csi2_swnodes_del_free(void);
 void acpi_bus_scan_crs_csi2(acpi_handle handle);
+void acpi_init_swnodes(struct acpi_device *device);
 
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 2a8cd8ee074e3..3b8bb72ae4027 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -14,6 +14,8 @@
 #include <linux/sort.h>
 #include <linux/string.h>
 
+#include <media/v4l2-fwnode.h>
+
 #include "internal.h"
 
 struct crs_csi2_swnodes {
@@ -24,6 +26,18 @@ struct crs_csi2_swnodes {
 
 static LIST_HEAD(crs_csi2_swnodes);
 
+static struct acpi_device_software_nodes *
+crs_csi2_swnode_get(acpi_handle handle)
+{
+	struct crs_csi2_swnodes *swnodes;
+
+	list_for_each_entry(swnodes, &crs_csi2_swnodes, list)
+		if (swnodes->handle == handle)
+			return swnodes->ads;
+
+	return NULL;
+}
+
 static void crs_csi2_swnode_del_free(struct crs_csi2_swnodes *swnodes)
 {
 	list_del(&swnodes->list);
@@ -141,6 +155,32 @@ struct acpi_handle_ref {
 
 #define NO_CSI2_PORT (~1U)
 
+static unsigned int next_csi2_port_index(struct acpi_device_software_nodes *ads,
+					 unsigned int port_nr)
+{
+	unsigned int i;
+
+	for (i = 0; i < ads->num_ports; i++) {
+		struct acpi_device_software_node_port *port = &ads->ports[i];
+
+		if (port->port_nr == port_nr)
+			return i;
+
+		if (port->port_nr != NO_CSI2_PORT)
+			continue;
+
+		port->port_nr = port_nr;
+
+		return i;
+	}
+
+	return NO_CSI2_PORT;
+}
+
+#define GRAPH_PORT_NAME(var, num)					   \
+	(snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) > \
+	 sizeof(var))
+
 static int crs_handle_cmp(const void *__a, const void *__b)
 {
 	const struct acpi_handle_ref *a = __a, *b = __b;
@@ -165,6 +205,9 @@ static void crs_csi2_release(struct list_head *crs_csi2_handles)
 	}
 }
 
+#define ACPI_CRS_CSI2_PHY_TYPE_C	0
+#define ACPI_CRS_CSI2_PHY_TYPE_D	1
+
 /**
  * acpi_bus_scan_crs_csi2 - Scan a device and its child devices for _CRS CSI-2
  *
@@ -184,6 +227,8 @@ static void crs_csi2_release(struct list_head *crs_csi2_handles)
  * 4. Allocate memory for swnodes each ACPI device requires later on, and
  *    generate a list of such allocations.
  *
+ * 5. Set up properties for software nodes.
+ *
  * Note that struct acpi_device isn't available yet at this time.
  *
  * acpi_scan_lock in scan.c must be held when calling this function.
@@ -314,7 +359,308 @@ void acpi_bus_scan_crs_csi2(acpi_handle handle)
 		this_count = this->count;
 	}
 
+	/*
+	 * Allocate and set up necessary software nodes for each device and set
+	 * up properties from _CRS CSI2 descriptor.
+	 */
+	list_for_each_entry(csi2, &crs_csi2_handles, list) {
+		struct acpi_device_software_nodes *local_swnodes;
+		struct crs_csi2_instance *inst;
+
+		local_swnodes = crs_csi2_swnode_get(csi2->handle);
+		if (WARN_ON_ONCE(!local_swnodes))
+			continue;
+
+		list_for_each_entry(inst, &csi2->buses, list) {
+			struct acpi_device_software_nodes *remote_swnodes;
+			struct acpi_device_software_node_port *local_port;
+			struct acpi_device_software_node_port *remote_port;
+			struct software_node *local_node, *remote_node;
+			unsigned int local_index, remote_index;
+			unsigned int bus_type;
+
+			remote_swnodes = crs_csi2_swnode_get(inst->remote_handle);
+			if (WARN_ON_ONCE(!remote_swnodes))
+				continue;
+
+			local_index = next_csi2_port_index(local_swnodes, inst->csi2.local_port_instance);
+			remote_index = next_csi2_port_index(remote_swnodes, inst->csi2.resource_source.index);
+
+			if (WARN_ON_ONCE(local_index >= local_swnodes->num_ports) ||
+			    WARN_ON_ONCE(remote_index >= remote_swnodes->num_ports))
+				goto out_free;
+
+			switch (inst->csi2.phy_type) {
+			case ACPI_CRS_CSI2_PHY_TYPE_C:
+				bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_CPHY;
+				break;
+			case ACPI_CRS_CSI2_PHY_TYPE_D:
+				bus_type = V4L2_FWNODE_BUS_TYPE_CSI2_DPHY;
+				break;
+			default:
+				acpi_handle_info(handle,
+						 "ignoring CSI-2 PHY type %u\n",
+						 inst->csi2.phy_type);
+				continue;
+			}
+
+			local_port = &local_swnodes->ports[local_index];
+			local_node = &local_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(local_index)];
+			local_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(local_node);
+			local_port->crs_csi2_local = true;
+
+			remote_port = &remote_swnodes->ports[remote_index];
+			remote_node = &remote_swnodes->nodes[ACPI_DEVICE_SWNODE_EP(remote_index)];
+			remote_port->remote_ep_ref[0] = SOFTWARE_NODE_REFERENCE(remote_node);
+
+			local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+				PROPERTY_ENTRY_REF_ARRAY("remote-endpoint",
+							 remote_port->remote_ep_ref);
+			local_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+				PROPERTY_ENTRY_U32("bus-type", bus_type);
+			local_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+				PROPERTY_ENTRY_U32("reg", 0);
+			local_port->port_props[ACPI_DEVICE_SWNODE_PRT_REG] =
+				PROPERTY_ENTRY_U32("reg", inst->csi2.local_port_instance);
+			if (GRAPH_PORT_NAME(local_port->port_name,
+					    inst->csi2.local_port_instance))
+				acpi_handle_warn(handle,
+						 "name for local port %u too long",
+						 inst->csi2.local_port_instance);
+
+			remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REMOTE_EP] =
+				PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", local_port->remote_ep_ref);
+			remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_BUS_TYPE] =
+				PROPERTY_ENTRY_U32("bus-type", bus_type);
+			remote_port->ep_props[ACPI_DEVICE_SWNODE_EP_REG] =
+				PROPERTY_ENTRY_U32("reg", 0);
+			remote_port->port_props[ACPI_DEVICE_SWNODE_PRT_REG] =
+				PROPERTY_ENTRY_U32("reg",
+						   inst->csi2.resource_source.index);
+			if (GRAPH_PORT_NAME(remote_port->port_name,
+					    inst->csi2.resource_source.index))
+				acpi_handle_warn(handle,
+						 "name for remote port %u too long",
+						 inst->csi2.resource_source.index);
+		}
+	}
+
+out_free:
 	kfree(handle_refs);
 
 	crs_csi2_release(&crs_csi2_handles);
 }
+
+/*
+ * Get the index of the next property in the property array, with a given
+ * maximum values.
+ */
+#define NEXT_PROPERTY(index, max)				\
+	(WARN_ON(++(index) >= ACPI_DEVICE_SWNODE_##max) ?	\
+	 ACPI_DEVICE_SWNODE_##max : (index) - 1)
+
+static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
+						  unsigned int port)
+{
+	static const char mipi_port_prefix[] = "mipi-img-port-";
+	char mipi_port_name[sizeof(mipi_port_prefix) + 2];
+
+	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
+		     mipi_port_prefix, port) > sizeof(mipi_port_name)) {
+		acpi_handle_info(acpi_device_handle(device),
+				 "mipi port name too long for port %u\n", port);
+		return NULL;
+	}
+
+	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
+					   mipi_port_name);
+}
+
+static void init_port_csi2_common(struct acpi_device *device,
+				  struct fwnode_handle *mipi_port_fwnode,
+				  unsigned int *ep_prop_index,
+				  unsigned int port_nr)
+{
+	unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
+	struct acpi_device_software_nodes *ads = device->swnodes;
+	struct acpi_device_software_node_port *port = &ads->ports[port_index];
+	unsigned int num_lanes = 0;
+	union {
+		u32 val;
+		/* Data lanes + the clock lane */
+		u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
+	} u;
+	int ret;
+
+	*ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
+
+	if (GRAPH_PORT_NAME(port->port_name, port_nr))
+		return;
+
+	ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
+		SOFTWARE_NODE(port->port_name, port->port_props,
+			      &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
+
+	ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", u.val8);
+	if (!ret) {
+		port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
+			PROPERTY_ENTRY_U32("clock-lanes", *u.val8);
+	}
+	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
+	if (ret > 0) {
+		num_lanes = ret;
+
+		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
+			acpi_handle_warn(acpi_device_handle(device),
+					 "too many data lanes (%u)\n",
+					 num_lanes);
+			num_lanes = ARRAY_SIZE(port->data_lanes);
+		}
+
+		ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
+						    u.val8, num_lanes);
+		if (!ret) {
+			unsigned int i;
+
+			for (i = 0; i < num_lanes; i++)
+				port->data_lanes[i] = u.val8[i];
+
+			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
+				PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", port->data_lanes,
+							     num_lanes);
+		}
+	}
+	ret = fwnode_property_read_u8_array(mipi_port_fwnode,
+					    "mipi-img-lane-polarities",
+					    u.val8, sizeof(u.val8));
+	if (ret > 0) {
+		unsigned int bytes = ret;
+
+		/* Total number of lanes here is clock lane + data lanes */
+		if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
+			unsigned int i;
+
+			/* Move polarity bits to the lane polarity u32 array */
+			for (i = 0; i < 1 + num_lanes; i++)
+				port->lane_polarities[i] =
+					(u.val8[i >> 3] & (1 << (i & 7))) ?
+					1U : 0U;
+
+			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_LANE_POLARITIES)] =
+				PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
+							     port->lane_polarities,
+							     1 + num_lanes);
+		} else {
+			acpi_handle_warn(acpi_device_handle(device),
+					 "too few lane polarity bytes (%u)\n",
+					 bytes);
+		}
+	}
+
+	ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
+		SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
+			      &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
+}
+
+static void init_port_csi2_local(struct acpi_device *device,
+				 unsigned int port_nr)
+{
+	unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
+	struct fwnode_handle *mipi_port_fwnode =
+		get_mipi_port_handle(device, port_nr);
+	struct acpi_device_software_node_port *port =
+		&device->swnodes->ports[port_index];
+	unsigned int ep_prop_index;
+	int ret;
+
+	init_port_csi2_common(device, mipi_port_fwnode, &ep_prop_index, port_nr);
+
+	ret = fwnode_property_count_u64(mipi_port_fwnode, "mipi-img-link-frequencies");
+	if (ret > 0) {
+		unsigned int num_link_freqs = ret;
+
+		if (num_link_freqs > ARRAY_SIZE(port->link_frequencies)) {
+			acpi_handle_info(acpi_device_handle(device),
+					 "too many link frequencies %u\n",
+					 num_link_freqs);
+			num_link_freqs = ARRAY_SIZE(port->link_frequencies);
+		}
+
+		ret = fwnode_property_read_u64_array(mipi_port_fwnode,
+						     "mipi-img-link-frequencies",
+						     port->link_frequencies,
+						     num_link_freqs);
+		if (!ret)
+			port->ep_props[NEXT_PROPERTY(ep_prop_index, EP_LINK_FREQUENCIES)] =
+				PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
+							     port->link_frequencies,
+							     num_link_freqs);
+		else
+			acpi_handle_info(acpi_device_handle(device),
+					 "can't get link frequencies (%d)\n",
+					 ret);
+	}
+
+	fwnode_handle_put(mipi_port_fwnode);
+}
+
+static void init_port_csi2_remote(struct acpi_device *device,
+				  unsigned int port_nr)
+{
+	struct fwnode_handle *mipi_port_fwnode = get_mipi_port_handle(device, port_nr);
+	unsigned int ep_prop_index;
+
+	init_port_csi2_common(device, mipi_port_fwnode, &ep_prop_index, port_nr);
+
+	fwnode_handle_put(mipi_port_fwnode);
+}
+
+/**
+ * acpi_init_swnodes - Set up software nodes for properties gathered elsewhere
+ *
+ * @device: ACPI device for which the software nodes are initialised
+ *
+ * Initialise and register software nodes for properties for which the data is
+ * gathered elsewhere, e.g. _CRS CSI-2 descriptors. The process itself takes
+ * place before this function is called.
+ *
+ * acpi_scan_lock in scan.c must be held when calling this function.
+ */
+void acpi_init_swnodes(struct acpi_device *device)
+{
+	struct acpi_device_software_nodes *ads;
+	struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER };
+	unsigned int i;
+	int ret;
+
+	device->swnodes = ads = crs_csi2_swnode_get(device->handle);
+	if (!ads)
+		return;
+
+	if (ACPI_FAILURE(acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer))) {
+		acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
+		return;
+	}
+
+	ads->nodes[ACPI_DEVICE_SWNODE_ROOT] =
+		SOFTWARE_NODE(buffer.pointer, ads->dev_props, NULL);
+
+	for (i = 0; i < ads->num_ports; i++) {
+		struct acpi_device_software_node_port *port = &ads->ports[i];
+
+		if (port->crs_csi2_local)
+			init_port_csi2_local(device, port->port_nr);
+		else
+			init_port_csi2_remote(device, port->port_nr);
+	}
+
+	ret = software_node_register_node_group(ads->nodeptrs);
+	if (ret < 0) {
+		acpi_handle_warn(acpi_device_handle(device),
+				 "cannot register software nodes (%d)!\n", ret);
+		device->swnodes = NULL;
+		return;
+	}
+
+	device->fwnode.secondary = software_node_fwnode(ads->nodes);
+}
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 50de874b8f208..29ef8200b50bb 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -449,10 +449,26 @@ static void acpi_free_power_resources_lists(struct acpi_device *device)
 	}
 }
 
+static void acpi_free_swnodes(struct acpi_device *device)
+{
+	struct acpi_device_software_nodes *ads = device->swnodes;
+
+	if (!ads)
+		return;
+
+	software_node_unregister_node_group(ads->nodeptrs);
+	set_secondary_fwnode(&device->dev, NULL);
+	kfree(ads->nodes[ACPI_DEVICE_SWNODE_ROOT].name);
+	kfree(ads);
+
+	device->swnodes = NULL;
+}
+
 static void acpi_device_release(struct device *dev)
 {
 	struct acpi_device *acpi_dev = to_acpi_device(dev);
 
+	acpi_free_swnodes(acpi_dev);
 	acpi_free_properties(acpi_dev);
 	acpi_free_pnp_ids(&acpi_dev->pnp);
 	acpi_free_power_resources_lists(acpi_dev);
@@ -1771,6 +1787,7 @@ void acpi_init_device_object(struct acpi_device *device, acpi_handle handle,
 	acpi_device_get_busid(device);
 	acpi_set_pnp_ids(handle, &device->pnp, type);
 	acpi_init_properties(device);
+	acpi_init_swnodes(device);
 	acpi_bus_get_flags(device);
 	device->flags.match_driver = false;
 	device->flags.initialized = true;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index a05fe22c1175c..9a7729e96d14c 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -360,8 +360,54 @@ struct acpi_device_data {
 
 struct acpi_gpio_mapping;
 
+enum acpi_device_swnode_dev_props {
+	ACPI_DEVICE_SWNODE_DEV_NUM_OF,
+	ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES
+};
+
+enum acpi_device_swnode_port_props {
+	ACPI_DEVICE_SWNODE_PRT_REG,
+	ACPI_DEVICE_SWNODE_PRT_NUM_OF,
+	ACPI_DEVICE_SWNODE_PRT_NUM_ENTRIES
+};
+
+enum acpi_device_swnode_ep_props {
+	ACPI_DEVICE_SWNODE_EP_REMOTE_EP,
+	ACPI_DEVICE_SWNODE_EP_BUS_TYPE,
+	ACPI_DEVICE_SWNODE_EP_REG,
+	ACPI_DEVICE_SWNODE_EP_CLOCK_LANES,
+	ACPI_DEVICE_SWNODE_EP_DATA_LANES,
+	ACPI_DEVICE_SWNODE_EP_LANE_POLARITIES,
+	/* TX only */
+	ACPI_DEVICE_SWNODE_EP_LINK_FREQUENCIES,
+	ACPI_DEVICE_SWNODE_EP_NUM_OF,
+	ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES
+};
+
+#define ACPI_DEVICE_SWNODE_ROOT			0
+/*
+ * Each device has a root swnode plus two times as many nodes as the
+ * number of CSI-2 ports.
+ */
+#define ACPI_DEVICE_SWNODE_PRT(port)		(1 + 2 * (port))
+#define ACPI_DEVICE_SWNODE_EP(endpoint)	\
+	(ACPI_DEVICE_SWNODE_PRT(endpoint) + 1)
+
+#define ACPI_DEVICE_SWNODE_CSI2_DATA_LANES		4
+
 struct acpi_device_software_node_port {
+	char port_name[8];
+	u32 data_lanes[ACPI_DEVICE_SWNODE_CSI2_DATA_LANES];
+	u32 lane_polarities[1 /* clock lane */ +
+			    ACPI_DEVICE_SWNODE_CSI2_DATA_LANES];
+	u64 link_frequencies[4];
 	unsigned int port_nr;
+	bool crs_csi2_local;
+
+	struct property_entry port_props[ACPI_DEVICE_SWNODE_PRT_NUM_ENTRIES];
+	struct property_entry ep_props[ACPI_DEVICE_SWNODE_EP_NUM_ENTRIES];
+
+	struct software_node_ref_args remote_ep_ref[1];
 };
 
 struct acpi_device_software_nodes {
@@ -369,6 +415,8 @@ struct acpi_device_software_nodes {
 	struct software_node *nodes;
 	const struct software_node **nodeptrs;
 	unsigned int num_ports;
+
+	struct property_entry dev_props[ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES];
 };
 
 /* Device */
@@ -377,6 +425,7 @@ struct acpi_device {
 	int device_type;
 	acpi_handle handle;		/* no handle for fixed hardware */
 	struct fwnode_handle fwnode;
+	struct acpi_device_software_nodes *swnodes;
 	struct list_head wakeup_list;
 	struct list_head del_list;
 	struct acpi_device_status status;
-- 
2.30.2

[PATCH v2 5/8] ACPI: property: Dig "rotation" property for devices with CSI2 _CRS

[Sakari Ailus]

Dig "rotation" property value for devices with _CRS CSI2 resource
descriptor. The value comes from _PLD (physical location of device)
object, if it exists for the device.

This way camera sensor drivers that know the "rotation" property do not
need to care about _PLD on ACPI.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/mipi.c     | 16 ++++++++++++++++
 include/acpi/acpi_bus.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 3b8bb72ae4027..4addcb4cb9ce1 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -628,15 +628,31 @@ static void init_port_csi2_remote(struct acpi_device *device,
  */
 void acpi_init_swnodes(struct acpi_device *device)
 {
+	struct fwnode_handle *fwnode = acpi_fwnode_handle(device);
 	struct acpi_device_software_nodes *ads;
 	struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER };
+	struct acpi_pld_info *pld;
+	unsigned int prop_index = 0;
 	unsigned int i;
+	u32 val;
 	int ret;
 
 	device->swnodes = ads = crs_csi2_swnode_get(device->handle);
 	if (!ads)
 		return;
 
+	/*
+	 * Check if "rotation" property exists and if it doesn't but there's a
+	 * _PLD object, then get the rotation value from there.
+	 */
+	if (fwnode_property_read_u32(fwnode, "rotation", &val) &&
+	    ACPI_SUCCESS(acpi_get_physical_device_location(acpi_device_handle(device),
+							   &pld))) {
+		ads->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] =
+			PROPERTY_ENTRY_U32("rotation", pld->rotation * 45U);
+		kfree(pld);
+	}
+
 	if (ACPI_FAILURE(acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer))) {
 		acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
 		return;
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 9a7729e96d14c..3c4a1daed33f1 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -361,6 +361,7 @@ struct acpi_device_data {
 struct acpi_gpio_mapping;
 
 enum acpi_device_swnode_dev_props {
+	ACPI_DEVICE_SWNODE_DEV_ROTATION,
 	ACPI_DEVICE_SWNODE_DEV_NUM_OF,
 	ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES
 };
-- 
2.30.2

[PATCH v2 6/8] ACPI: property: Rename parsed MIPI DisCo for Imaging properties

[Sakari Ailus]

MIPI DisCo for Imaging defines properties for sensor-adjacent devices such
as EEPROM, LED flash or lens VCM as either device or sub-node references.
This is compliant with existing DT definitions apart from property names.

Rename parsed MIPI-defined properties so drivers will have a unified view
of them as defined in DT and already parsed by drivers. This can be done
in-place as the MIPI-defined property strings are always longer than the
DT one. This also results in loss of constness in parser function
arguments.

Individual bindings to devices could define the references differently
between MIPI DisCo for Imaging and DT, in terms of device or sub-node
references. This will still need to be handled in the drivers themselves.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/internal.h |  1 +
 drivers/acpi/mipi.c     | 37 +++++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c | 22 ++++++++++++----------
 3 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index fac87404e294c..f107094bfe16f 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -289,5 +289,6 @@ static inline void acpi_init_lpit(void) { }
 void acpi_crs_csi2_swnodes_del_free(void);
 void acpi_bus_scan_crs_csi2(acpi_handle handle);
 void acpi_init_swnodes(struct acpi_device *device);
+void acpi_properties_prepare_mipi(union acpi_object *elements);
 
 #endif /* _ACPI_INTERNAL_H_ */
diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 4addcb4cb9ce1..9177170952104 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -680,3 +680,40 @@ void acpi_init_swnodes(struct acpi_device *device)
 
 	device->fwnode.secondary = software_node_fwnode(ads->nodes);
 }
+
+static const struct mipi_disco_prop {
+	const char *mipi_prop;
+	const char *dt_prop;
+} mipi_disco_props[] = {
+	{ "mipi-img-lens-focus", "lens-focus" },
+	{ "mipi-img-flash-leds", "flash-leds" },
+	{ "mipi-img-clock-frequency", "clock-frequency" },
+	{ "mipi-img-led-max-current", "led-max-microamp" },
+	{ "mipi-img-flash-max-current", "flash-max-microamp" },
+	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
+};
+
+/**
+ * acpi_properties_prepare_mipi - Rename MIPI properties as commin DT ones
+ *
+ * @elements: ACPI object containing _DSD properties for a device node
+ *
+ * Renames MIPI-defined properties as common DT ones. The pre-requisite is that
+ * the names of all such MIPI properties are no longer than the corresponding DT
+ * ones.
+ */
+void acpi_properties_prepare_mipi(union acpi_object *elements)
+{
+	unsigned int i;
+
+	/* Replace MIPI DisCo for Imaging property names with DT equivalents. */
+	for (i = 0; i < ARRAY_SIZE(mipi_disco_props); i++) {
+		if (!strcmp(mipi_disco_props[i].mipi_prop,
+			    elements[0].string.pointer)) {
+			WARN_ON(strscpy(elements[0].string.pointer,
+					mipi_disco_props[i].dt_prop,
+					elements[0].string.length) < 0);
+			break;
+		}
+	}
+}
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index ae1f8259e76a5..6ab4e422d157c 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -124,7 +124,7 @@ static bool acpi_nondev_subnode_extract(union acpi_object *desc,
 }
 
 static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
-					const union acpi_object *link,
+					union acpi_object *link,
 					struct list_head *list,
 					struct fwnode_handle *parent)
 {
@@ -145,7 +145,7 @@ static bool acpi_nondev_subnode_data_ok(acpi_handle handle,
 }
 
 static bool acpi_nondev_subnode_ok(acpi_handle scope,
-				   const union acpi_object *link,
+				   union acpi_object *link,
 				   struct list_head *list,
 				   struct fwnode_handle *parent)
 {
@@ -276,22 +276,24 @@ static bool acpi_property_value_ok(const union acpi_object *value)
 	return false;
 }
 
-static bool acpi_properties_format_valid(const union acpi_object *properties)
+static bool acpi_properties_prepare(union acpi_object *properties)
 {
-	int i;
+	unsigned int i;
 
 	for (i = 0; i < properties->package.count; i++) {
-		const union acpi_object *property;
+		union acpi_object *property = &properties->package.elements[i];
+		union acpi_object *elements = property->package.elements;
 
-		property = &properties->package.elements[i];
 		/*
 		 * Only two elements allowed, the first one must be a string and
 		 * the second one has to satisfy certain conditions.
 		 */
-		if (property->package.count != 2
-		    || property->package.elements[0].type != ACPI_TYPE_STRING
-		    || !acpi_property_value_ok(&property->package.elements[1]))
+		if (property->package.count != 2 ||
+		    elements[0].type != ACPI_TYPE_STRING ||
+		    !acpi_property_value_ok(&elements[1]))
 			return false;
+
+		acpi_properties_prepare_mipi(elements);
 	}
 	return true;
 }
@@ -523,7 +525,7 @@ static bool acpi_extract_properties(acpi_handle scope, union acpi_object *desc,
 		 * We found the matching GUID. Now validate the format of the
 		 * package immediately following it.
 		 */
-		if (!acpi_properties_format_valid(properties))
+		if (!acpi_properties_prepare(properties))
 			continue;
 
 		acpi_data_add_props(data, (const guid_t *)guid->buffer.pointer,
-- 
2.30.2

[PATCH v2 7/8] ACPI: property: Skip MIPI property table without "mipi-img" prefix

[Sakari Ailus]

For all _DSD properties, skip going through the MIPI DisCo for Imaging
property name substitution table if the property doesn't have "mipi-img-"
prefix.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/mipi.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 9177170952104..3cb698b094ac1 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -681,16 +681,18 @@ void acpi_init_swnodes(struct acpi_device *device)
 	device->fwnode.secondary = software_node_fwnode(ads->nodes);
 }
 
+#define MIPI_IMG_PREFIX "mipi-img-"
+
 static const struct mipi_disco_prop {
 	const char *mipi_prop;
 	const char *dt_prop;
 } mipi_disco_props[] = {
-	{ "mipi-img-lens-focus", "lens-focus" },
-	{ "mipi-img-flash-leds", "flash-leds" },
-	{ "mipi-img-clock-frequency", "clock-frequency" },
-	{ "mipi-img-led-max-current", "led-max-microamp" },
-	{ "mipi-img-flash-max-current", "flash-max-microamp" },
-	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
+	{ MIPI_IMG_PREFIX "lens-focus", "lens-focus" },
+	{ MIPI_IMG_PREFIX "flash-leds", "flash-leds" },
+	{ MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" },
+	{ MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" },
+	{ MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" },
+	{ MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" },
 };
 
 /**
@@ -706,6 +708,9 @@ void acpi_properties_prepare_mipi(union acpi_object *elements)
 {
 	unsigned int i;
 
+	if (!str_has_prefix(elements[0].string.pointer, MIPI_IMG_PREFIX))
+		return;
+
 	/* Replace MIPI DisCo for Imaging property names with DT equivalents. */
 	for (i = 0; i < ARRAY_SIZE(mipi_disco_props); i++) {
 		if (!strcmp(mipi_disco_props[i].mipi_prop,
-- 
2.30.2

[PATCH v2 8/8] ACPI: property: Document _CRS CSI-2 and DisCo for Imaging support

[Sakari Ailus]

Document how ACPI _CRS CSI-2 and DisCo for Imaging works. It's non-trivial
so such documentation can be useful.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/mipi.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index 3cb698b094ac1..24cef9d501ebc 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -3,6 +3,43 @@
  * MIPI DisCo for Imaging support.
  *
  * Copyright (C) 2023 Intel Corporation
+ *
+ * _CRS CSI-2 descriptors, as defined starting from ACPI 6.4 [1], contain
+ * information on cross-device CSI-2 bus configuration. The descriptors are
+ * located under transmitter devices, and the receiver devices have no direct
+ * way to access them even if the information in these descriptors is equally
+ * important for receivers. This information is amended with MIPI DisCo for
+ * Imaging [2] specification that defines _DSD data nodes and properties.
+ *
+ * The support for these is based on two-fold approach, firstly renaming
+ * properties where semantics matches and secondly gathering information to
+ * generate properties using information gathered from various sources. The
+ * former is trivial (see acpi_properties_prepare_mipi() at the end of the
+ * file) whereas the latter requires a more elaborate explanation.
+ *
+ * acpi_bus_scan_crs_csi2() scans an ACPI bus for devices with _CRS CSI-2
+ * descriptors and stores them to a linked list. This is done as traversing just
+ * this list is much smaller task than the entire DSDT. This list is then used
+ * to figure out how much memory is needed for swnodes related to a given ACPI
+ * device (handle). Further on, the same function sets the property values for
+ * the properties the values of which are obtained from the _CRS CSI-2
+ * descriptor. The information is stored into another list where the information
+ * can be looked up based on device's acpi_handle as the struct acpi_device
+ * isn't available yet at this point (and could not, as cross-device references
+ * need to be set up before the devices are available for drivers to probe).
+ *
+ * For each struct acpi_device, acpi_init_swnodes() further obtains information
+ * required to find out the values for the rest of the properties needed by
+ * drivers. This includes all port and endpoint properties as the node
+ * structures used by DT graphs and DisCo for Imaging are different. Finally the
+ * function registers software nodes for the device and sets the secondary
+ * pointer for the ACPI device's fwnode.
+ *
+ * Access to data the structures is serialised using acpi_scan_lock in scan.c.
+ *
+ * [1] https://uefi.org/sites/default/files/resources/ACPI_Spec_6_4_Jan22.pdf
+ *
+ * [2] https://www.mipi.org/specifications/mipi-disco-imaging
  */
 
 #include <linux/acpi.h>
-- 
2.30.2

Re: [PATCH v2 1/8] ACPI: property: Parse data node string references in properties

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus wrote:
> Add support for parsing property references using strings, besides
> reference objects that were previously supported. This allows also
> referencing data nodes which was not possible with reference objects.
> 
> Also add pr_fmt() macro to prefix printouts.
> 
> While at it, update copyright.

...

> - * Copyright (C) 2014, Intel Corporation
> + * Copyright (C) 2014--2023, Intel Corporation

Isn't one dash enough? 

$ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l
37

$ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l
15064


>   * All rights reserved.
>   *
>   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
>   *          Darren Hart <dvhart@linux.intel.com>
>   *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> + *	    Sakari Ailus <sakari.ailus@linux.intel.com>

Seems wrong indentation in comparison to the others.

>   */

...

> +static struct fwnode_handle *
> +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> +{
> +	acpi_handle scope, handle;
> +	struct acpi_data_node *dn;
> +	struct acpi_device *device;
> +	acpi_status status;
> +
> +	if (is_acpi_device_node(fwnode)) {

> +		scope = to_acpi_device_node(fwnode)->handle;

Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()...

> +	} else if (is_acpi_data_node(fwnode)) {

> +		scope = to_acpi_data_node(fwnode)->handle;

...but not for this.

> +	} else {
> +		pr_err("bad node type for node %pfw\n", fwnode);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	status = acpi_get_handle(scope, refstring, &handle);
> +	if (ACPI_FAILURE(status)) {
> +		acpi_handle_debug(scope, "can't get handle for %s", refstring);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	device = acpi_fetch_acpi_dev(handle);
> +	if (device)
> +		return acpi_fwnode_handle(device);
> +
> +	status = acpi_get_data_full(handle, acpi_nondev_subnode_tag,
> +				    (void **)&dn, NULL);
> +	if (ACPI_FAILURE(status) || !dn) {
> +		acpi_handle_debug(handle, "can't find subnode");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return &dn->fwnode;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:
> Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> configuration. For now, only figure out where the descriptor is present in
> order to allow adding information from it to related devices.

...

> +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> +	       csi2->resource_source.string_length);

Why don't we use strscpy()? Is it really strings? Or is it some abuse of
the ACPI object type?

...

> +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level,
> +				       void *context, void **ret)
> +{
> +	struct scan_check_crs_csi2_context inst_context = {
> +		.handle = handle,
> +		.res_list = LIST_HEAD_INIT(inst_context.res_list),
> +	};
> +	struct list_head *list = context;
> +	struct crs_csi2 *csi2;

> +	INIT_LIST_HEAD(&inst_context.res_list);

Why do you need this? I don't see that variable is static...

> +	acpi_walk_resources(handle, METHOD_NAME__CRS,
> +			    scan_check_crs_csi2_instance, &inst_context);
> +
> +	if (list_empty(&inst_context.res_list))
> +		return AE_OK;
> +
> +	csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL);
> +	if (!csi2)
> +		return AE_OK;
> +
> +	csi2->handle = handle;
> +	list_replace(&inst_context.res_list, &csi2->buses);
> +	list_add(&csi2->list, list);

Hmm... Can list_swap() be used here?

> +	return AE_OK;
> +}

...

> +	/*
> +	 * Figure out how much temporary storage we need for counting
> +	 * connections in each device.
> +	 */
> +	list_for_each_entry(csi2, &crs_csi2_handles, list) {
> +		struct crs_csi2_instance *inst;
> +
> +		handle_count++;

> +		list_for_each_entry(inst, &csi2->buses, list)
> +			handle_count++;

list_count_nodes()?

> +	}

...

> +	sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp,
> +	     NULL);

Yes, I would leave it on one line.

...

> +		if (check_mul_overflow(sizeof(*ads->ports) +
> +				       sizeof(*ads->nodes) * 2 +
> +				       sizeof(*ads->nodeptrs) * 2,
> +				       (size_t)this_count, &alloc_size) ||

Can this_count be of size_t type from the beginning?

> +		    check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
> +				       sizeof(*ads->nodeptrs) * 2,
> +				       alloc_size, &alloc_size)) {
> +			acpi_handle_warn(handle, "too many handles (%u)",
> +					 this_count);
> +			continue;
> +		}

...

> +		ads->nodeptrs = (void *)(ads->nodes +
> +					 this_count * 2 + 1);

Why this is not on one line? (I have got less than 80).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
> Generate software nodes for information parsed from ACPI _CRS for CSI-2 as
> well as MIPI DisCo for Imaging spec. The software nodes are compliant with
> existing ACPI or DT definitions and are parsed by relevant drivers without
> changes.

...

> +static struct acpi_device_software_nodes *
> +crs_csi2_swnode_get(acpi_handle handle)

It's 81 on one line. Why not to join?

> +{
> +	struct crs_csi2_swnodes *swnodes;
> +
> +	list_for_each_entry(swnodes, &crs_csi2_swnodes, list)
> +		if (swnodes->handle == handle)
> +			return swnodes->ads;
> +
> +	return NULL;
> +}

...

> +#define GRAPH_PORT_NAME(var, num)					   \
> +	(snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) > \
> +	 sizeof(var))

>= ?

("excluding the trailing '\0'")

...

> +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> +						  unsigned int port)
> +{

> +	static const char mipi_port_prefix[] = "mipi-img-port-";

It's used only once in this function, why not keeping it in the format string?

> +	char mipi_port_name[sizeof(mipi_port_prefix) + 2];
> +
> +	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> +		     mipi_port_prefix, port) > sizeof(mipi_port_name)) {
> +		acpi_handle_info(acpi_device_handle(device),
> +				 "mipi port name too long for port %u\n", port);
> +		return NULL;
> +	}
> +
> +	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
> +					   mipi_port_name);
> +}

...

> +			/* Move polarity bits to the lane polarity u32 array */
> +			for (i = 0; i < 1 + num_lanes; i++)
> +				port->lane_polarities[i] =
> +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> +					1U : 0U;

Wouldn't

				port->lane_polarities[i] =
					!!(u.val8[i >> 3] & (1 << (i & 7)));

be better?

...

> +	ret = software_node_register_node_group(ads->nodeptrs);
> +	if (ret < 0) {
> +		acpi_handle_warn(acpi_device_handle(device),
> +				 "cannot register software nodes (%d)!\n", ret);
> +		device->swnodes = NULL;
> +		return;
> +	}

> +	device->fwnode.secondary = software_node_fwnode(ads->nodes);

	struct fwnode_handle *primary;
	...
	primary = acpi_fwnode_handle(device);
	primary->secondary = ...

?

The point is to avoid direct dereferences of fwnode in struct acpi_device.


...

> +static void acpi_free_swnodes(struct acpi_device *device)
> +{
> +	struct acpi_device_software_nodes *ads = device->swnodes;
> +
> +	if (!ads)
> +		return;
> +
> +	software_node_unregister_node_group(ads->nodeptrs);

> +	set_secondary_fwnode(&device->dev, NULL);

Interestingly you are not use same API above. Why?

> +	kfree(ads->nodes[ACPI_DEVICE_SWNODE_ROOT].name);
> +	kfree(ads);
> +
> +	device->swnodes = NULL;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/8] ACPI: property: Skip MIPI property table without "mipi-img" prefix

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 03:46:16PM +0200, Sakari Ailus wrote:
> For all _DSD properties, skip going through the MIPI DisCo for Imaging
> property name substitution table if the property doesn't have "mipi-img-"
> prefix.

...

> -	{ "mipi-img-lens-focus", "lens-focus" },
> -	{ "mipi-img-flash-leds", "flash-leds" },
> -	{ "mipi-img-clock-frequency", "clock-frequency" },
> -	{ "mipi-img-led-max-current", "led-max-microamp" },
> -	{ "mipi-img-flash-max-current", "flash-max-microamp" },
> -	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
> +	{ MIPI_IMG_PREFIX "lens-focus", "lens-focus" },
> +	{ MIPI_IMG_PREFIX "flash-leds", "flash-leds" },
> +	{ MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" },
> +	{ MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" },
> +	{ MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" },
> +	{ MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" },

I don't thing it ads to the readability, so I don't know why this (part of the)
change is needed.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/8] ACPI: property: Parse data node string references in properties

[Sakari Ailus]

Hi Andy,

On Mon, Jan 23, 2023 at 04:51:33PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus wrote:
> > Add support for parsing property references using strings, besides
> > reference objects that were previously supported. This allows also
> > referencing data nodes which was not possible with reference objects.
> > 
> > Also add pr_fmt() macro to prefix printouts.
> > 
> > While at it, update copyright.
> 
> ...
> 
> > - * Copyright (C) 2014, Intel Corporation
> > + * Copyright (C) 2014--2023, Intel Corporation
> 
> Isn't one dash enough? 
> 
> $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l
> 37
> 
> $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l
> 15064

This is a range, not hyphenation. There's no different character in the
ASCII character set for the former, commonly two regular dashes are used.
There probably would be a correct Unicode character though.

> 
> 
> >   * All rights reserved.
> >   *
> >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> >   *          Darren Hart <dvhart@linux.intel.com>
> >   *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > + *	    Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Seems wrong indentation in comparison to the others.

Tabs are preferred for intendation. I can change all the lines to use tab.
How about that?

> 
> >   */
> 
> ...
> 
> > +static struct fwnode_handle *
> > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> > +{
> > +	acpi_handle scope, handle;
> > +	struct acpi_data_node *dn;
> > +	struct acpi_device *device;
> > +	acpi_status status;
> > +
> > +	if (is_acpi_device_node(fwnode)) {
> 
> > +		scope = to_acpi_device_node(fwnode)->handle;
> 
> Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()...
> 
> > +	} else if (is_acpi_data_node(fwnode)) {
> 
> > +		scope = to_acpi_data_node(fwnode)->handle;
> 
> ...but not for this.

I'd either prefer to keep them as-is, as it's easy to see what's being done
there, or add a new macro --- or a function to do this.  Say,
acpi_fwnode_acpi_handle(), as this is clearly ACPI specific and to
differentiate between ACPI handles and fwnode handles.

ACPI_HANDLE_FWNODE()'s name suggests it would do something else than it
does, if you consider the current fwnode API.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Sakari Ailus]

Hi Andy,

On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:
> > Parse newly added ACPI _CRS CSI-2 descriptor for CSI-2 and camera
> > configuration. For now, only figure out where the descriptor is present in
> > order to allow adding information from it to related devices.
> 
> ...
> 
> > +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > +	       csi2->resource_source.string_length);
> 
> Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> the ACPI object type?

I didn't find a guarantee it would be nil terminated. Albeit I'm fine
switching to strscpy() if there's such a guarantee.

> 
> ...
> 
> > +static acpi_status scan_check_crs_csi2(acpi_handle handle, u32 nesting_level,
> > +				       void *context, void **ret)
> > +{
> > +	struct scan_check_crs_csi2_context inst_context = {
> > +		.handle = handle,
> > +		.res_list = LIST_HEAD_INIT(inst_context.res_list),
> > +	};
> > +	struct list_head *list = context;
> > +	struct crs_csi2 *csi2;
> 
> > +	INIT_LIST_HEAD(&inst_context.res_list);
> 
> Why do you need this? I don't see that variable is static...

Ah. It's not static. But this is a leftover from development time and can
be removed, it's initialised in variable declaration.

> 
> > +	acpi_walk_resources(handle, METHOD_NAME__CRS,
> > +			    scan_check_crs_csi2_instance, &inst_context);
> > +
> > +	if (list_empty(&inst_context.res_list))
> > +		return AE_OK;
> > +
> > +	csi2 = kmalloc(sizeof(*csi2), GFP_KERNEL);
> > +	if (!csi2)
> > +		return AE_OK;
> > +
> > +	csi2->handle = handle;
> > +	list_replace(&inst_context.res_list, &csi2->buses);
> > +	list_add(&csi2->list, list);
> 
> Hmm... Can list_swap() be used here?

We're replacing an entry in a list and then adding an entry to another. How
would you use list_swap() here?

> 
> > +	return AE_OK;
> > +}
> 
> ...
> 
> > +	/*
> > +	 * Figure out how much temporary storage we need for counting
> > +	 * connections in each device.
> > +	 */
> > +	list_for_each_entry(csi2, &crs_csi2_handles, list) {
> > +		struct crs_csi2_instance *inst;
> > +
> > +		handle_count++;
> 
> > +		list_for_each_entry(inst, &csi2->buses, list)
> > +			handle_count++;
> 
> list_count_nodes()?

Are you suggesting adding a new list API function or using one that's not
in the linux-acpi/testing branch yet?

> 
> > +	}
> 
> ...
> 
> > +	sort(handle_refs, handle_count, sizeof(*handle_refs), crs_handle_cmp,
> > +	     NULL);
> 
> Yes, I would leave it on one line.

Works for me.

> 
> ...
> 
> > +		if (check_mul_overflow(sizeof(*ads->ports) +
> > +				       sizeof(*ads->nodes) * 2 +
> > +				       sizeof(*ads->nodeptrs) * 2,
> > +				       (size_t)this_count, &alloc_size) ||
> 
> Can this_count be of size_t type from the beginning?

I think so.

> 
> > +		    check_add_overflow(sizeof(*ads) + sizeof(*ads->nodes) +
> > +				       sizeof(*ads->nodeptrs) * 2,
> > +				       alloc_size, &alloc_size)) {
> > +			acpi_handle_warn(handle, "too many handles (%u)",
> > +					 this_count);
> > +			continue;
> > +		}
> 
> ...
> 
> > +		ads->nodeptrs = (void *)(ads->nodes +
> > +					 this_count * 2 + 1);
> 
> Why this is not on one line? (I have got less than 80).

Probably there was more on that line but I forgot to unwrap when removing
whatever was there. I'll address this for v3.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 1/8] ACPI: property: Parse data node string references in properties

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 05:53:59PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 04:51:33PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:10PM +0200, Sakari Ailus wrote:

...

> > > - * Copyright (C) 2014, Intel Corporation
> > > + * Copyright (C) 2014--2023, Intel Corporation
> > 
> > Isn't one dash enough? 
> > 
> > $ git grep -n 'opyright.*[0-9]--[0-9]' | wc -l
> > 37
> > 
> > $ git grep -n 'opyright.*[0-9]-[0-9]' | wc -l
> > 15064
> 
> This is a range, not hyphenation. There's no different character in the
> ASCII character set for the former, commonly two regular dashes are used.
> There probably would be a correct Unicode character though.

Fine, but it's not even close to be called "a common use" as I showed by
running `git grep`.

> > >   * All rights reserved.
> > >   *
> > >   * Authors: Mika Westerberg <mika.westerberg@linux.intel.com>
> > >   *          Darren Hart <dvhart@linux.intel.com>
> > >   *          Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > + *	    Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > Seems wrong indentation in comparison to the others.
> 
> Tabs are preferred for intendation. I can change all the lines to use tab.

Dunno, not a maintainer. I just pointed to inconsistency in the comment lines.

> > >   */

...

> > > +static struct fwnode_handle *
> > > +acpi_parse_string_ref(const struct fwnode_handle *fwnode, const char *refstring)
> > > +{
> > > +	acpi_handle scope, handle;
> > > +	struct acpi_data_node *dn;
> > > +	struct acpi_device *device;
> > > +	acpi_status status;
> > > +
> > > +	if (is_acpi_device_node(fwnode)) {
> > 
> > > +		scope = to_acpi_device_node(fwnode)->handle;
> > 
> > Interestingly that we have a helper for this -- ACPI_HANDLE_FWNODE()...
> > 
> > > +	} else if (is_acpi_data_node(fwnode)) {
> > 
> > > +		scope = to_acpi_data_node(fwnode)->handle;
> > 
> > ...but not for this.
> 
> I'd either prefer to keep them as-is, as it's easy to see what's being done
> there, or add a new macro --- or a function to do this.  Say,
> acpi_fwnode_acpi_handle(), as this is clearly ACPI specific and to
> differentiate between ACPI handles and fwnode handles.

Since it's an ACPI glue layer code, I'm not insisting on changes. Just pointed
out that we have a helper function for one of the cases.

> ACPI_HANDLE_FWNODE()'s name suggests it would do something else than it
> does, if you consider the current fwnode API.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:

...

> > > +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > > +	       csi2->resource_source.string_length);
> > 
> > Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> > the ACPI object type?
> 
> I didn't find a guarantee it would be nil terminated. Albeit I'm fine
> switching to strscpy() if there's such a guarantee.

Following this logic neither memcpy() (and especially memcpy()!) call
guarantees that. But hold on, have you actually read strscpy() documentation?

...

> > > +	list_replace(&inst_context.res_list, &csi2->buses);
> > > +	list_add(&csi2->list, list);
> > 
> > Hmm... Can list_swap() be used here?
> 
> We're replacing an entry in a list and then adding an entry to another. How
> would you use list_swap() here?

I see, so it is about two different lists.

...

> > > +	/*
> > > +	 * Figure out how much temporary storage we need for counting
> > > +	 * connections in each device.
> > > +	 */
> > > +	list_for_each_entry(csi2, &crs_csi2_handles, list) {
> > > +		struct crs_csi2_instance *inst;
> > > +
> > > +		handle_count++;
> > 
> > > +		list_for_each_entry(inst, &csi2->buses, list)
> > > +			handle_count++;
> > 
> > list_count_nodes()?
> 
> Are you suggesting adding a new list API function or using one that's not
> in the linux-acpi/testing branch yet?

It's in USB tree.
I'm fine if you switch your code later on.

> > > +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/8] device property: Add SOFTWARE_NODE() macro for defining software nodes

[Heikki Krogerus]

On Mon, Jan 23, 2023 at 03:46:12PM +0200, Sakari Ailus wrote:
> Add SOFTWARE_NODE() macro in order to make defining software nodes look
> nicer. This is analogous to different PROPERTY_ENTRY_*() macros for
> defining properties.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  include/linux/property.h | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 37179e3abad5c..6745a86bc9b97 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -477,6 +477,13 @@ struct software_node {
>  	const struct property_entry *properties;
>  };
>  
> +#define SOFTWARE_NODE(_name_, _properties_, _parent_)	\
> +	(struct software_node) {			\
> +		.name = _name_,				\
> +		.properties = _properties_,		\
> +		.parent = _parent_,			\
> +	}
> +
>  bool is_software_node(const struct fwnode_handle *fwnode);
>  const struct software_node *
>  to_software_node(const struct fwnode_handle *fwnode);
> -- 
> 2.30.2

-- 
heikki

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Sakari Ailus]

Hi Andy,

On Mon, Jan 23, 2023 at 05:23:49PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
> > Generate software nodes for information parsed from ACPI _CRS for CSI-2 as
> > well as MIPI DisCo for Imaging spec. The software nodes are compliant with
> > existing ACPI or DT definitions and are parsed by relevant drivers without
> > changes.
> 
> ...
> 
> > +static struct acpi_device_software_nodes *
> > +crs_csi2_swnode_get(acpi_handle handle)
> 
> It's 81 on one line. Why not to join?

Works for me.

> 
> > +{
> > +	struct crs_csi2_swnodes *swnodes;
> > +
> > +	list_for_each_entry(swnodes, &crs_csi2_swnodes, list)
> > +		if (swnodes->handle == handle)
> > +			return swnodes->ads;
> > +
> > +	return NULL;
> > +}
> 
> ...
> 
> > +#define GRAPH_PORT_NAME(var, num)					   \
> > +	(snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) > \
> > +	 sizeof(var))
> 
> >= ?
> 
> ("excluding the trailing '\0'")

Thanks, indeed. A bug introduced in v2.

> 
> ...
> 
> > +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> > +						  unsigned int port)
> > +{
> 
> > +	static const char mipi_port_prefix[] = "mipi-img-port-";
> 
> It's used only once in this function, why not keeping it in the format string?

Twice, not once. My point was that it's critical the strings remain the
same length, and certainly what that string actually is, is less important.

> 
> > +	char mipi_port_name[sizeof(mipi_port_prefix) + 2];
> > +
> > +	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> > +		     mipi_port_prefix, port) > sizeof(mipi_port_name)) {
> > +		acpi_handle_info(acpi_device_handle(device),
> > +				 "mipi port name too long for port %u\n", port);
> > +		return NULL;
> > +	}
> > +
> > +	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
> > +					   mipi_port_name);
> > +}
> 
> ...
> 
> > +			/* Move polarity bits to the lane polarity u32 array */
> > +			for (i = 0; i < 1 + num_lanes; i++)
> > +				port->lane_polarities[i] =
> > +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> > +					1U : 0U;
> 
> Wouldn't
> 
> 				port->lane_polarities[i] =
> 					!!(u.val8[i >> 3] & (1 << (i & 7)));
> 
> be better?

It would work, yes, although the target is a u32. Casting to bool would
look nicer to me. I lean towards what it is at the moment but bool seems
fairly reasonable, too.

> 
> ...
> 
> > +	ret = software_node_register_node_group(ads->nodeptrs);
> > +	if (ret < 0) {
> > +		acpi_handle_warn(acpi_device_handle(device),
> > +				 "cannot register software nodes (%d)!\n", ret);
> > +		device->swnodes = NULL;
> > +		return;
> > +	}
> 
> > +	device->fwnode.secondary = software_node_fwnode(ads->nodes);
> 
> 	struct fwnode_handle *primary;
> 	...
> 	primary = acpi_fwnode_handle(device);
> 	primary->secondary = ...
> 
> ?
> 
> The point is to avoid direct dereferences of fwnode in struct acpi_device.

Yes.

> 
> 
> ...
> 
> > +static void acpi_free_swnodes(struct acpi_device *device)
> > +{
> > +	struct acpi_device_software_nodes *ads = device->swnodes;
> > +
> > +	if (!ads)
> > +		return;
> > +
> > +	software_node_unregister_node_group(ads->nodeptrs);
> 
> > +	set_secondary_fwnode(&device->dev, NULL);
> 
> Interestingly you are not use same API above. Why?

Good question.

dev->fwnode hasn't been set when assigning the secondary fwnode in
acpi_init_swnodes(), therefore set_secondary_fwnode() won't do what it
should.

It can be still called here as it just sets dev->fwnode->secondary NULL.

I can add a comment mentioning this.

I think it'd be better to have a set of fwnodes attached to a device rather
than one primary and another secondary, with various levels of success
depending on the order of assigning them. But I think it's out of scope of
this set.

> 
> > +	kfree(ads->nodes[ACPI_DEVICE_SWNODE_ROOT].name);
> > +	kfree(ads);
> > +
> > +	device->swnodes = NULL;
> > +}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Sakari Ailus]

Hi Andy,

On Mon, Jan 23, 2023 at 07:25:25PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote:
> > On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > > > +	       csi2->resource_source.string_length);
> > > 
> > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> > > the ACPI object type?
> > 
> > I didn't find a guarantee it would be nil terminated. Albeit I'm fine
> > switching to strscpy() if there's such a guarantee.
> 
> Following this logic neither memcpy() (and especially memcpy()!) call
> guarantees that. But hold on, have you actually read strscpy() documentation?

Yes. And there is such a guarantee, too. The string_length contains the
length of the string, including the terminating nil character. I have no
problem with strscpy() but it won't affect the end result in any way. :-)

> 
> ...
> 
> > > > +	list_replace(&inst_context.res_list, &csi2->buses);
> > > > +	list_add(&csi2->list, list);
> > > 
> > > Hmm... Can list_swap() be used here?
> > 
> > We're replacing an entry in a list and then adding an entry to another. How
> > would you use list_swap() here?
> 
> I see, so it is about two different lists.
> 
> ...
> 
> > > > +	/*
> > > > +	 * Figure out how much temporary storage we need for counting
> > > > +	 * connections in each device.
> > > > +	 */
> > > > +	list_for_each_entry(csi2, &crs_csi2_handles, list) {
> > > > +		struct crs_csi2_instance *inst;
> > > > +
> > > > +		handle_count++;
> > > 
> > > > +		list_for_each_entry(inst, &csi2->buses, list)
> > > > +			handle_count++;
> > > 
> > > list_count_nodes()?
> > 
> > Are you suggesting adding a new list API function or using one that's not
> > in the linux-acpi/testing branch yet?
> 
> It's in USB tree.
> I'm fine if you switch your code later on.

Ack, I'll post a patch once this hits the linux-acpi tree.

> 
> > > > +	}
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 7/8] ACPI: property: Skip MIPI property table without "mipi-img" prefix

[Sakari Ailus]

On Mon, Jan 23, 2023 at 05:27:46PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:16PM +0200, Sakari Ailus wrote:
> > For all _DSD properties, skip going through the MIPI DisCo for Imaging
> > property name substitution table if the property doesn't have "mipi-img-"
> > prefix.
> 
> ...
> 
> > -	{ "mipi-img-lens-focus", "lens-focus" },
> > -	{ "mipi-img-flash-leds", "flash-leds" },
> > -	{ "mipi-img-clock-frequency", "clock-frequency" },
> > -	{ "mipi-img-led-max-current", "led-max-microamp" },
> > -	{ "mipi-img-flash-max-current", "flash-max-microamp" },
> > -	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
> > +	{ MIPI_IMG_PREFIX "lens-focus", "lens-focus" },
> > +	{ MIPI_IMG_PREFIX "flash-leds", "flash-leds" },
> > +	{ MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" },
> > +	{ MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" },
> > +	{ MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" },
> > +	{ MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" },
> 
> I don't thing it ads to the readability, so I don't know why this (part of the)
> change is needed.

Ok, I'll drop this chunk.

-- 
Sakari Ailus

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 05:43:22PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 05:23:49PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

...

> > > +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> > > +						  unsigned int port)
> > > +{
> > 
> > > +	static const char mipi_port_prefix[] = "mipi-img-port-";
> > 
> > It's used only once in this function, why not keeping it in the format string?
> 
> Twice, not once. My point was that it's critical the strings remain the
> same length, and certainly what that string actually is, is less important.

Still can be placed twice as is. But fine, I leave it to maintainers.

> > > +	char mipi_port_name[sizeof(mipi_port_prefix) + 2];
> > > +
> > > +	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> > > +		     mipi_port_prefix, port) > sizeof(mipi_port_name)) {

Btw, seems also a candidate for >= ?

> > > +		acpi_handle_info(acpi_device_handle(device),
> > > +				 "mipi port name too long for port %u\n", port);
> > > +		return NULL;
> > > +	}
> > > +
> > > +	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
> > > +					   mipi_port_name);
> > > +}

...

> > > +			/* Move polarity bits to the lane polarity u32 array */
> > > +			for (i = 0; i < 1 + num_lanes; i++)
> > > +				port->lane_polarities[i] =
> > > +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> > > +					1U : 0U;
> > 
> > Wouldn't
> > 
> > 				port->lane_polarities[i] =
> > 					!!(u.val8[i >> 3] & (1 << (i & 7)));
> > 
> > be better?
> 
> It would work, yes, although the target is a u32. Casting to bool would
> look nicer to me. I lean towards what it is at the moment but bool seems
> fairly reasonable, too.

I think we can do even better and switch this to bitmap APIs.

I'll comment separately with the better context given.

...

> dev->fwnode hasn't been set when assigning the secondary fwnode in
> acpi_init_swnodes(), therefore set_secondary_fwnode() won't do what it
> should.
> 
> It can be still called here as it just sets dev->fwnode->secondary NULL.
> 
> I can add a comment mentioning this.

Or maybe drop the use of the specific API and rather do something similar
to the above?

> I think it'd be better to have a set of fwnodes attached to a device rather
> than one primary and another secondary, with various levels of success
> depending on the order of assigning them. But I think it's out of scope of
> this set.

Yeah, but it's quite a big topic out of the scope of this series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/8] ACPI: property: Parse _CRS CSI-2 descriptor

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 05:52:08PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 07:25:25PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 06:07:43PM +0200, Sakari Ailus wrote:
> > > On Mon, Jan 23, 2023 at 05:07:09PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 23, 2023 at 03:46:11PM +0200, Sakari Ailus wrote:

...

> > > > > +	memcpy(inst->remote_name, csi2->resource_source.string_ptr,
> > > > > +	       csi2->resource_source.string_length);
> > > > 
> > > > Why don't we use strscpy()? Is it really strings? Or is it some abuse of
> > > > the ACPI object type?
> > > 
> > > I didn't find a guarantee it would be nil terminated. Albeit I'm fine
> > > switching to strscpy() if there's such a guarantee.
> > 
> > Following this logic neither memcpy() (and especially memcpy()!) call
> > guarantees that. But hold on, have you actually read strscpy() documentation?
> 
> Yes. And there is such a guarantee, too. The string_length contains the
> length of the string, including the terminating nil character. I have no
> problem with strscpy() but it won't affect the end result in any way. :-)

At run time won't be any differences, at reading and maintaining it has a lot.
It gets rid of the confusion: "is this actually string or not?"

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/8] ACPI: property: Skip MIPI property table without "mipi-img" prefix

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 05:54:00PM +0200, Sakari Ailus wrote:
> On Mon, Jan 23, 2023 at 05:27:46PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:16PM +0200, Sakari Ailus wrote:

...

> > > -	{ "mipi-img-lens-focus", "lens-focus" },
> > > -	{ "mipi-img-flash-leds", "flash-leds" },
> > > -	{ "mipi-img-clock-frequency", "clock-frequency" },
> > > -	{ "mipi-img-led-max-current", "led-max-microamp" },
> > > -	{ "mipi-img-flash-max-current", "flash-max-microamp" },
> > > -	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
> > > +	{ MIPI_IMG_PREFIX "lens-focus", "lens-focus" },
> > > +	{ MIPI_IMG_PREFIX "flash-leds", "flash-leds" },
> > > +	{ MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" },
> > > +	{ MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" },
> > > +	{ MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" },
> > > +	{ MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" },
> > 
> > I don't thing it ads to the readability, so I don't know why this (part of the)
> > change is needed.
> 
> Ok, I'll drop this chunk.

Thank you. What you can do, though, is to make the second list on the same
column (in the previous patch) if you consider it would be better.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

As promised the idea of bitmap APIs.

Also I have stumbled over couple of suspicious places. See below.

...

> +static void init_port_csi2_common(struct acpi_device *device,
> +				  struct fwnode_handle *mipi_port_fwnode,
> +				  unsigned int *ep_prop_index,
> +				  unsigned int port_nr)
> +{
> +	unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
> +	struct acpi_device_software_nodes *ads = device->swnodes;
> +	struct acpi_device_software_node_port *port = &ads->ports[port_index];
> +	unsigned int num_lanes = 0;
> +	union {
> +		u32 val;

// Not sure why this even exists.
// And hence why do we need union?

> +		/* Data lanes + the clock lane */
> +		u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
> +	} u;

Somewhere

#define MAX_LANES(port)		(ARRAY_SIZE((port)->data_lanes) + 1)

	u8 val8[BITS_TO_BYTES(MAX_LANES(port))];

...

	/* Data lanes + the clock lane */
	DECLARE_BITMAP(polarity, MAX_LANES(port)));

> +	int ret;
> +
> +	*ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
> +
> +	if (GRAPH_PORT_NAME(port->port_name, port_nr))
> +		return;
> +
> +	ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
> +		SOFTWARE_NODE(port->port_name, port->port_props,
> +			      &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
> +
> +	ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", u.val8);
> +	if (!ret) {
> +		port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
> +			PROPERTY_ENTRY_U32("clock-lanes", *u.val8);
> +	}

> +	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
> +	if (ret > 0) {
> +		num_lanes = ret;
> +
> +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {

		>= MAX_LANES(port)

> +			acpi_handle_warn(acpi_device_handle(device),
> +					 "too many data lanes (%u)\n",
> +					 num_lanes);
> +			num_lanes = ARRAY_SIZE(port->data_lanes);

			= MAX_LANES(port) - 1;

> +		}
> +
> +		ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
> +						    u.val8, num_lanes);

> +		if (!ret) {
> +			unsigned int i;
> +
> +			for (i = 0; i < num_lanes; i++)
> +				port->data_lanes[i] = u.val8[i];
> +
> +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
> +				PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", port->data_lanes,
> +							     num_lanes);
> +		}
> +	}

> +	ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> +					    "mipi-img-lane-polarities",
> +					    u.val8, sizeof(u.val8));
> +	if (ret > 0) {

How is it supposed to work?!

> +		unsigned int bytes = ret;
> +
> +		/* Total number of lanes here is clock lane + data lanes */
> +		if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
> +			unsigned int i;
> +
> +			/* Move polarity bits to the lane polarity u32 array */
> +			for (i = 0; i < 1 + num_lanes; i++)
> +				port->lane_polarities[i] =
> +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> +					1U : 0U;
> +
> +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_LANE_POLARITIES)] =
> +				PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
> +							     port->lane_polarities,
> +							     1 + num_lanes);
> +		} else {
> +			acpi_handle_warn(acpi_device_handle(device),
> +					 "too few lane polarity bytes (%u)\n",
> +					 bytes);
> +		}
> +	}

	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
	if (ret < 0) {
		acpi_handle_debug(acpi_device_handle(device),
				  "no lane polarity provided\n");
	} else if (ret < 1 + num_lanes) {
		acpi_handle_warn(acpi_device_handle(device),
				 "too few lane polarity bytes (%u)\n", bytes);
	} else {
		// assuming we dropped the union and renamed to val...
		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
						    "mipi-img-lane-polarities",
						    val, sizeof(val));
		if (ret) {
			...can't read... (debug message?)
		} else {
			unsigned int i;

			for (i = 0; i < 1 + num_lanes; i++)
				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);

			// assuming that lane_polarities is zeroed by default...
			for_each_set_bit(i, polarity, 1 + num_lanes)
				port->lane_polarities[i] = 1;
		}
	}

> +	ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
> +		SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
> +			      &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

...

> 	u8 val8[BITS_TO_BYTES(MAX_LANES(port))];

Here forgot to rename

	u8 val[BITS_TO_BYTES(MAX_LANES(port))];

And it seems also good to have

#define MAX_LANES_BYTES(port)	BITS_TO_BYTES(MAX_LANES(port))

	u8 val[MAX_LANES_BYTES(port)];

...

> 	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
> 	if (ret < 0) {
> 		acpi_handle_debug(acpi_device_handle(device),
> 				  "no lane polarity provided\n");
> 	} else if (ret < 1 + num_lanes) {
> 		acpi_handle_warn(acpi_device_handle(device),
> 				 "too few lane polarity bytes (%u)\n", bytes);
> 	} else {
> 		// assuming we dropped the union and renamed to val...
> 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> 						    "mipi-img-lane-polarities",
> 						    val, sizeof(val));
> 		if (ret) {
> 			...can't read... (debug message?)
> 		} else {
> 			unsigned int i;
> 
> 			for (i = 0; i < 1 + num_lanes; i++)

Here something like

			for (i = 0; i < MAX_LANES_BYTES(port); i++)

But I'm tired for today, please double check. I hope you got the idea.

> 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
> 
> 			// assuming that lane_polarities is zeroed by default...
> 			for_each_set_bit(i, polarity, 1 + num_lanes)
> 				port->lane_polarities[i] = 1;
> 		}
> 	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/8] ACPI: property: Skip MIPI property table without "mipi-img" prefix

[Sakari Ailus]

On Tue, Jan 24, 2023 at 06:41:14PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 24, 2023 at 05:54:00PM +0200, Sakari Ailus wrote:
> > On Mon, Jan 23, 2023 at 05:27:46PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 23, 2023 at 03:46:16PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > -	{ "mipi-img-lens-focus", "lens-focus" },
> > > > -	{ "mipi-img-flash-leds", "flash-leds" },
> > > > -	{ "mipi-img-clock-frequency", "clock-frequency" },
> > > > -	{ "mipi-img-led-max-current", "led-max-microamp" },
> > > > -	{ "mipi-img-flash-max-current", "flash-max-microamp" },
> > > > -	{ "mipi-img-flash-max-timeout", "flash-max-timeout-us" },
> > > > +	{ MIPI_IMG_PREFIX "lens-focus", "lens-focus" },
> > > > +	{ MIPI_IMG_PREFIX "flash-leds", "flash-leds" },
> > > > +	{ MIPI_IMG_PREFIX "clock-frequency", "clock-frequency" },
> > > > +	{ MIPI_IMG_PREFIX "led-max-current", "led-max-microamp" },
> > > > +	{ MIPI_IMG_PREFIX "flash-max-current", "flash-max-microamp" },
> > > > +	{ MIPI_IMG_PREFIX "flash-max-timeout", "flash-max-timeout-us" },
> > > 
> > > I don't thing it ads to the readability, so I don't know why this (part of the)
> > > change is needed.
> > 
> > Ok, I'll drop this chunk.
> 
> Thank you. What you can do, though, is to make the second list on the same
> column (in the previous patch) if you consider it would be better.

Yes, makes sense.

-- 
Sakari Ailus

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Sakari Ailus]

Hi Andy,

On Tue, Jan 24, 2023 at 06:38:26PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 24, 2023 at 05:43:22PM +0200, Sakari Ailus wrote:
> > On Mon, Jan 23, 2023 at 05:23:49PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> > > > +						  unsigned int port)
> > > > +{
> > > 
> > > > +	static const char mipi_port_prefix[] = "mipi-img-port-";
> > > 
> > > It's used only once in this function, why not keeping it in the format string?
> > 
> > Twice, not once. My point was that it's critical the strings remain the
> > same length, and certainly what that string actually is, is less important.
> 
> Still can be placed twice as is. But fine, I leave it to maintainers.
> 
> > > > +	char mipi_port_name[sizeof(mipi_port_prefix) + 2];
> > > > +
> > > > +	if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> > > > +		     mipi_port_prefix, port) > sizeof(mipi_port_name)) {
> 
> Btw, seems also a candidate for >= ?

Good catch. snprintf() excludes '\0' from the return value.

> 
> > > > +		acpi_handle_info(acpi_device_handle(device),
> > > > +				 "mipi port name too long for port %u\n", port);
> > > > +		return NULL;
> > > > +	}
> > > > +
> > > > +	return fwnode_get_named_child_node(acpi_fwnode_handle(device),
> > > > +					   mipi_port_name);
> > > > +}
> 
> ...
> 
> > > > +			/* Move polarity bits to the lane polarity u32 array */
> > > > +			for (i = 0; i < 1 + num_lanes; i++)
> > > > +				port->lane_polarities[i] =
> > > > +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> > > > +					1U : 0U;
> > > 
> > > Wouldn't
> > > 
> > > 				port->lane_polarities[i] =
> > > 					!!(u.val8[i >> 3] & (1 << (i & 7)));
> > > 
> > > be better?
> > 
> > It would work, yes, although the target is a u32. Casting to bool would
> > look nicer to me. I lean towards what it is at the moment but bool seems
> > fairly reasonable, too.
> 
> I think we can do even better and switch this to bitmap APIs.
> 
> I'll comment separately with the better context given.

The problem with the bitmap APIs is that they work on unsigned long, so
there are endian issues that will need to be handled. I thing it's simply
not worth it: either you have temporary space for this or a new API is
needed. What the line above is, after all, fairly trivial.

> 
> ...
> 
> > dev->fwnode hasn't been set when assigning the secondary fwnode in
> > acpi_init_swnodes(), therefore set_secondary_fwnode() won't do what it
> > should.
> > 
> > It can be still called here as it just sets dev->fwnode->secondary NULL.
> > 
> > I can add a comment mentioning this.
> 
> Or maybe drop the use of the specific API and rather do something similar
> to the above?

Yes, that's what I actually thought. But still a comment is good to have
here, so someone doesn't try to convert this to use the functions intended
for this (!).

> 
> > I think it'd be better to have a set of fwnodes attached to a device rather
> > than one primary and another secondary, with various levels of success
> > depending on the order of assigning them. But I think it's out of scope of
> > this set.
> 
> Yeah, but it's quite a big topic out of the scope of this series.

Yes.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Sakari Ailus]

Hi Andy,

On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
> 
> As promised the idea of bitmap APIs.
> 
> Also I have stumbled over couple of suspicious places. See below.
> 
> ...
> 
> > +static void init_port_csi2_common(struct acpi_device *device,
> > +				  struct fwnode_handle *mipi_port_fwnode,
> > +				  unsigned int *ep_prop_index,
> > +				  unsigned int port_nr)
> > +{
> > +	unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
> > +	struct acpi_device_software_nodes *ads = device->swnodes;
> > +	struct acpi_device_software_node_port *port = &ads->ports[port_index];
> > +	unsigned int num_lanes = 0;
> > +	union {
> > +		u32 val;
> 
> // Not sure why this even exists.
> // And hence why do we need union?

We could remove the union, yes, with one more u32 of stack used.

> 
> > +		/* Data lanes + the clock lane */
> > +		u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
> > +	} u;
> 
> Somewhere
> 
> #define MAX_LANES(port)		(ARRAY_SIZE((port)->data_lanes) + 1)
> 
> 	u8 val8[BITS_TO_BYTES(MAX_LANES(port))];
> 
> ...
> 
> 	/* Data lanes + the clock lane */
> 	DECLARE_BITMAP(polarity, MAX_LANES(port)));
> 
> > +	int ret;
> > +
> > +	*ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
> > +
> > +	if (GRAPH_PORT_NAME(port->port_name, port_nr))
> > +		return;
> > +
> > +	ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
> > +		SOFTWARE_NODE(port->port_name, port->port_props,
> > +			      &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
> > +
> > +	ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", u.val8);
> > +	if (!ret) {
> > +		port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
> > +			PROPERTY_ENTRY_U32("clock-lanes", *u.val8);
> > +	}
> 
> > +	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
> > +	if (ret > 0) {
> > +		num_lanes = ret;
> > +
> > +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
> 
> 		>= MAX_LANES(port)

I find the original better: it does this by referring to the array itself.

> 
> > +			acpi_handle_warn(acpi_device_handle(device),
> > +					 "too many data lanes (%u)\n",
> > +					 num_lanes);
> > +			num_lanes = ARRAY_SIZE(port->data_lanes);
> 
> 			= MAX_LANES(port) - 1;
> 
> > +		}
> > +
> > +		ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
> > +						    u.val8, num_lanes);
> 
> > +		if (!ret) {
> > +			unsigned int i;
> > +
> > +			for (i = 0; i < num_lanes; i++)
> > +				port->data_lanes[i] = u.val8[i];
> > +
> > +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
> > +				PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", port->data_lanes,
> > +							     num_lanes);
> > +		}
> > +	}
> 
> > +	ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > +					    "mipi-img-lane-polarities",
> > +					    u.val8, sizeof(u.val8));
> > +	if (ret > 0) {
> 
> How is it supposed to work?!

Hmm. I think in the past, some of these functions have returned the number
of the entries even when the buffer is provided
(acpi_copy_property_array_string() still does!). Good catch, I'll fix this
for v3.

> 
> > +		unsigned int bytes = ret;
> > +
> > +		/* Total number of lanes here is clock lane + data lanes */
> > +		if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
> > +			unsigned int i;
> > +
> > +			/* Move polarity bits to the lane polarity u32 array */
> > +			for (i = 0; i < 1 + num_lanes; i++)
> > +				port->lane_polarities[i] =
> > +					(u.val8[i >> 3] & (1 << (i & 7))) ?
> > +					1U : 0U;
> > +
> > +			port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_LANE_POLARITIES)] =
> > +				PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
> > +							     port->lane_polarities,
> > +							     1 + num_lanes);
> > +		} else {
> > +			acpi_handle_warn(acpi_device_handle(device),
> > +					 "too few lane polarity bytes (%u)\n",
> > +					 bytes);
> > +		}
> > +	}
> 
> 	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
> 	if (ret < 0) {
> 		acpi_handle_debug(acpi_device_handle(device),
> 				  "no lane polarity provided\n");
> 	} else if (ret < 1 + num_lanes) {
> 		acpi_handle_warn(acpi_device_handle(device),
> 				 "too few lane polarity bytes (%u)\n", bytes);
> 	} else {
> 		// assuming we dropped the union and renamed to val...
> 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> 						    "mipi-img-lane-polarities",
> 						    val, sizeof(val));
> 		if (ret) {
> 			...can't read... (debug message?)
> 		} else {
> 			unsigned int i;
> 
> 			for (i = 0; i < 1 + num_lanes; i++)
> 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);

You'll still needed to access invididual bits in val.

> 
> 			// assuming that lane_polarities is zeroed by default...
> 			for_each_set_bit(i, polarity, 1 + num_lanes)
> 				port->lane_polarities[i] = 1;
> 		}
> 	}
> 
> > +	ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
> > +		SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
> > +			      &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
> > +}

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 10:56:41AM +0200, Sakari Ailus wrote:
> On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

...

> > > +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
> > 
> > 		>= MAX_LANES(port)
> 
> I find the original better: it does this by referring to the array itself.

Whatever, it's just an example to show where it's being used.

> > > +			acpi_handle_warn(acpi_device_handle(device),
> > > +					 "too many data lanes (%u)\n",
> > > +					 num_lanes);
> > > +			num_lanes = ARRAY_SIZE(port->data_lanes);
> > 
> > 			= MAX_LANES(port) - 1;
> > 
> > > +		}

...

> > 	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
> > 	if (ret < 0) {
> > 		acpi_handle_debug(acpi_device_handle(device),
> > 				  "no lane polarity provided\n");
> > 	} else if (ret < 1 + num_lanes) {
> > 		acpi_handle_warn(acpi_device_handle(device),
> > 				 "too few lane polarity bytes (%u)\n", bytes);
> > 	} else {
> > 		// assuming we dropped the union and renamed to val...
> > 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > 						    "mipi-img-lane-polarities",
> > 						    val, sizeof(val));
> > 		if (ret) {
> > 			...can't read... (debug message?)
> > 		} else {
> > 			unsigned int i;
> > 
> > 			for (i = 0; i < 1 + num_lanes; i++)
> > 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
> 
> You'll still needed to access invididual bits in val.

I didn't get this. The below is what it does in most efficient way.

> > 			// assuming that lane_polarities is zeroed by default...
> > 			for_each_set_bit(i, polarity, 1 + num_lanes)
> > 				port->lane_polarities[i] = 1;

Note that his code lacks of endianess issues.

> > 		}
> > 	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Sakari Ailus]

Hi Andy,

On Wed, Jan 25, 2023 at 01:46:20PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 10:56:41AM +0200, Sakari Ailus wrote:
> > On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
> 
> ...
> 
> > > > +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
> > > 
> > > 		>= MAX_LANES(port)
> > 
> > I find the original better: it does this by referring to the array itself.
> 
> Whatever, it's just an example to show where it's being used.
> 
> > > > +			acpi_handle_warn(acpi_device_handle(device),
> > > > +					 "too many data lanes (%u)\n",
> > > > +					 num_lanes);
> > > > +			num_lanes = ARRAY_SIZE(port->data_lanes);
> > > 
> > > 			= MAX_LANES(port) - 1;
> > > 
> > > > +		}
> 
> ...
> 
> > > 	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
> > > 	if (ret < 0) {
> > > 		acpi_handle_debug(acpi_device_handle(device),
> > > 				  "no lane polarity provided\n");
> > > 	} else if (ret < 1 + num_lanes) {
> > > 		acpi_handle_warn(acpi_device_handle(device),
> > > 				 "too few lane polarity bytes (%u)\n", bytes);
> > > 	} else {
> > > 		// assuming we dropped the union and renamed to val...
> > > 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > > 						    "mipi-img-lane-polarities",
> > > 						    val, sizeof(val));
> > > 		if (ret) {
> > > 			...can't read... (debug message?)
> > > 		} else {
> > > 			unsigned int i;
> > > 
> > > 			for (i = 0; i < 1 + num_lanes; i++)
> > > 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
> > 
> > You'll still needed to access invididual bits in val.
> 
> I didn't get this. The below is what it does in most efficient way.

Ah. You're assining eight bits at a time.

Then the loop ends too late as i refers to a byte, not bit. This can be
addressed though. And a BUILD_BUG_ON() check for polarity being large
enough will be needed.

I still find this more complicated than the original code that also does
not need a temporary buffer.

> 
> > > 			// assuming that lane_polarities is zeroed by default...
> > > 			for_each_set_bit(i, polarity, 1 + num_lanes)
> > > 				port->lane_polarities[i] = 1;
> 
> Note that his code lacks of endianess issues.
> 
> > > 		}
> > > 	}
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 01:53:32PM +0200, Sakari Ailus wrote:
> On Wed, Jan 25, 2023 at 01:46:20PM +0200, Andy Shevchenko wrote:
> > On Wed, Jan 25, 2023 at 10:56:41AM +0200, Sakari Ailus wrote:
> > > On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

...

> > > > 		// assuming we dropped the union and renamed to val...
> > > > 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > > > 						    "mipi-img-lane-polarities",
> > > > 						    val, sizeof(val));
> > > > 		if (ret) {
> > > > 			...can't read... (debug message?)
> > > > 		} else {
> > > > 			unsigned int i;
> > > > 
> > > > 			for (i = 0; i < 1 + num_lanes; i++)
> > > > 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
> > > 
> > > You'll still needed to access invididual bits in val.
> > 
> > I didn't get this. The below is what it does in most efficient way.
> 
> Ah. You're assining eight bits at a time.

> Then the loop ends too late as i refers to a byte, not bit. This can be
> addressed though. And a BUILD_BUG_ON() check for polarity being large
> enough will be needed.

You probably meant static_assert(), but see my reply to my reply where
I caught up this. Yes, the loop conditional should rely on byte count.

> I still find this more complicated than the original code that also does
> not need a temporary buffer.

Your magic formula with bit shifts and conjunctions is so hard to read
and error prone, that makes me think of the proper APIs in the first place.
That's why I'm tending to use this code, because it's much easier to get
and maintain.

> > > > 			// assuming that lane_polarities is zeroed by default...
> > > > 			for_each_set_bit(i, polarity, 1 + num_lanes)
> > > > 				port->lane_polarities[i] = 1;
> > 
> > Note that his code lacks of endianess issues.
> > 
> > > > 		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko]

On Wed, Jan 25, 2023 at 02:00:25PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 01:53:32PM +0200, Sakari Ailus wrote:
> > On Wed, Jan 25, 2023 at 01:46:20PM +0200, Andy Shevchenko wrote:
> > > On Wed, Jan 25, 2023 at 10:56:41AM +0200, Sakari Ailus wrote:
> > > > On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> > > > > On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:

...

> > > > > 		// assuming we dropped the union and renamed to val...
> > > > > 		ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > > > > 						    "mipi-img-lane-polarities",
> > > > > 						    val, sizeof(val));
> > > > > 		if (ret) {
> > > > > 			...can't read... (debug message?)
> > > > > 		} else {
> > > > > 			unsigned int i;
> > > > > 
> > > > > 			for (i = 0; i < 1 + num_lanes; i++)
> > > > > 				bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
> > > > 
> > > > You'll still needed to access invididual bits in val.
> > > 
> > > I didn't get this. The below is what it does in most efficient way.
> > 
> > Ah. You're assining eight bits at a time.
> 
> > Then the loop ends too late as i refers to a byte, not bit. This can be
> > addressed though. And a BUILD_BUG_ON() check for polarity being large
> > enough will be needed.
> 
> You probably meant static_assert(), but see my reply to my reply where
> I caught up this. Yes, the loop conditional should rely on byte count.
> 
> > I still find this more complicated than the original code that also does
> > not need a temporary buffer.
> 
> Your magic formula with bit shifts and conjunctions is so hard to read
> and error prone, that makes me think of the proper APIs in the first place.
> That's why I'm tending to use this code, because it's much easier to get
> and maintain.
> 
> > > > > 			// assuming that lane_polarities is zeroed by default...
> > > > > 			for_each_set_bit(i, polarity, 1 + num_lanes)
> > > > > 				port->lane_polarities[i] = 1;

This even can be optimized much more if we put a constant bit numbers and if
it's less than or equal to BITS_PER_LONG.

			for_each_set_bit(i, polarity, MAX_LANES(port))

> > > Note that his code lacks of endianess issues.
> > > 
> > > > > 		}

-- 
With Best Regards,
Andy Shevchenko