diff mbox series

[v8,08/10] ACPI: property: Rename parsed MIPI DisCo for Imaging properties

Message ID 20230329100951.1522322-9-sakari.ailus@linux.intel.com (mailing list archive)
State Superseded, archived
Headers show
Series ACPI _CRS CSI-2 and MIPI DisCo for Imaging support | expand

Commit Message

Sakari Ailus March 29, 2023, 10:09 a.m. UTC
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     | 36 ++++++++++++++++++++++++++++++++++++
 drivers/acpi/property.c | 22 ++++++++++++----------
 3 files changed, 49 insertions(+), 10 deletions(-)

Comments

Rafael J. Wysocki May 19, 2023, 6:34 p.m. UTC | #1
On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> 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.

I don't particularly like this idea.

One of the drawbacks is that if somebody doesn't care about DT
bindings (for instance, because they will always run on platforms
without DT), they won't be able to use the MIPI-defined property names
in their code.

I would very much prefer to add a set of DT-defined properties with
the same values.  The, whoever wants to use the property names from
the DT bindings, they will be able to do that, but it will be also
possible to use the MIPI-defined ones.

The previous patch adds the "rotation" property to the swnodes set, so
I don't see any problems with doing that for the properties in
question.
Sakari Ailus May 19, 2023, 6:42 p.m. UTC | #2
Hi Rafael,

On Fri, May 19, 2023 at 08:34:34PM +0200, Rafael J. Wysocki wrote:
> On Wed, Mar 29, 2023 at 12:10 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > 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.
> 
> I don't particularly like this idea.
> 
> One of the drawbacks is that if somebody doesn't care about DT
> bindings (for instance, because they will always run on platforms
> without DT), they won't be able to use the MIPI-defined property names
> in their code.
> 
> I would very much prefer to add a set of DT-defined properties with
> the same values.  The, whoever wants to use the property names from
> the DT bindings, they will be able to do that, but it will be also
> possible to use the MIPI-defined ones.
> 
> The previous patch adds the "rotation" property to the swnodes set, so
> I don't see any problems with doing that for the properties in
> question.

I don't think this would be a problem really, no, but I question the need
to ever access the MIPI specification names in Linux outside this piece of
code. Drivers for cameras, lens controllers and LED flashes generally try
to avoid being specific to a given firmware interface and the established
de facto naming of these properties in the kernel is aligned with
Devicetree.

I'd like to see differences only when the functionality differs, otherwise
they should be the same. Creating a copy when you can modify it is waste of
a bit of memory. On the upside, the object memory could remain const that
way.
diff mbox series

Patch

diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
index 36799cde281c..ef5af9fd9ffa 100644
--- a/drivers/acpi/internal.h
+++ b/drivers/acpi/internal.h
@@ -318,5 +318,6 @@  void acpi_bus_scan_check_crs_csi2(acpi_handle handle, struct acpi_scan_context *
 void acpi_bus_scan_crs_csi2_release(struct list_head *crs_csi2_handles);
 void acpi_bus_scan_crs_csi2(struct acpi_scan_context_csi2 *ctx);
 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 0232cc123ead..576301b9bb89 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -714,3 +714,39 @@  void acpi_init_swnodes(struct acpi_device *device)
 	primary = acpi_fwnode_handle(device);
 	primary->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)
+{
+	char *e0 = elements[0].string.pointer;
+	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, e0)) {
+			WARN_ON(strscpy(e0, 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 1892787e73a6..541bda2c118f 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -127,7 +127,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)
 {
@@ -148,7 +148,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)
 {
@@ -292,22 +292,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;
 }
@@ -539,7 +541,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,