diff mbox series

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

Message ID 20230208152807.3064242-6-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 Feb. 8, 2023, 3:28 p.m. UTC
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(+)

Comments

Andy Shevchenko Feb. 8, 2023, 4:51 p.m. UTC | #1
On Wed, Feb 08, 2023 at 05:28:04PM +0200, Sakari Ailus wrote:
> 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.

...

> +	/*
> +	 * 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))) {

> +	if (fwnode_property_read_u32(fwnode, "rotation", &val) &&
> +	    ACPI_SUCCESS(acpi_get_physical_device_location(acpi_device_handle(device),
> +							   &pld))) {

Wouldn't be a bit better to use temporary variables for this?

	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
	if (ret) {
		acpi_handle handle = acpi_device_handle(device);
		acpi_status status;

		status = acpi_get_physical_device_location(handle, &pld);
		if (ACPI_SUCCESS(status)) {
			...
		}
	}

?

> +		ads->dev_props[NEXT_PROPERTY(prop_index, DEV_ROTATION)] =
> +			PROPERTY_ENTRY_U32("rotation", pld->rotation * 45U);
> +		kfree(pld);
> +	}
Sakari Ailus Feb. 8, 2023, 8:56 p.m. UTC | #2
Hi Andy,

On Wed, Feb 08, 2023 at 06:51:17PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 08, 2023 at 05:28:04PM +0200, Sakari Ailus wrote:
> > 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.
> 
> ...
> 
> > +	/*
> > +	 * 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))) {
> 
> > +	if (fwnode_property_read_u32(fwnode, "rotation", &val) &&
> > +	    ACPI_SUCCESS(acpi_get_physical_device_location(acpi_device_handle(device),
> > +							   &pld))) {
> 
> Wouldn't be a bit better to use temporary variables for this?
> 
> 	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> 	if (ret) {
> 		acpi_handle handle = acpi_device_handle(device);
> 		acpi_status status;
> 
> 		status = acpi_get_physical_device_location(handle, &pld);
> 		if (ACPI_SUCCESS(status)) {
> 			...
> 		}
> 	}
> 
> ?

It seems that val isn't used. So I'll use fwnode_property_present() which
I think is fine to test in if () directly.
diff mbox series

Patch

diff --git a/drivers/acpi/mipi.c b/drivers/acpi/mipi.c
index c3ceee1d119e7..584c08c2b8989 100644
--- a/drivers/acpi/mipi.c
+++ b/drivers/acpi/mipi.c
@@ -667,16 +667,32 @@  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 fwnode_handle *primary;
+	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 e05d1c1f6ac23..f73e65a21b894 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
 };