Hi Sakari, On Tue, Oct 31, 2023 at 11:33 AM Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > Hi Rafael, > > On subject: > > s/DiSco/DisCo/ Huh Thanks for catching this! > On Fri, Oct 20, 2023 at 04:39:27PM +0200, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > > > Add information extracted from the MIPI DiSco for Imaging device > > Ditto. > > > properties to software nodes created during the CSI-2 connection graph > > discovery. > > > > Link: https://www.mipi.org/specifications/mipi-di > > This URL is broken. The correct URL is: > > https://www.mipi.org/specifications/mipi-disco-imaging OK > > Co-developed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx> > > --- > > drivers/acpi/internal.h | 1 > > drivers/acpi/mipi-di.c | 240 +++++++++++++++++++++++++++++++++++++++++++++++- > > How about mipi-disco.c? I wouldn't mind having mipi-disco-imaging.c either. Well, mipi-disco-imaging is a bit too long and the meaning of mipi-disco would not be clear IMO. What about mipi-disco-img? [cut] > > + ret = fwnode_property_count_u8(port_fwnode, "mipi-img-lane-polarities"); > > + if (ret > 0) { > > + unsigned long mask; > > + unsigned int i; > > + > > + /* > > + * Total number of lanes here is clock lane + data lanes. > > + * Require that number to be low enough so they all can be > > + * covered by the bits in one byte. > > + */ > > + BUILD_BUG_ON(BITS_PER_TYPE(u8) <= ARRAY_SIZE(port->data_lanes)); > > + > > + fwnode_property_read_u8_array(port_fwnode, > > + "mipi-img-lane-polarities", > > + val, 1); > > + > > + for (mask = val[0], i = 0; i < num_lanes + 1; i++) > > + port->lane_polarities[i] = test_bit(i, &mask); > > This works only up to seven lanes. True, but then ACPI_DEVICE_CSI2_DATA_LANES is defined as 4. > I guess this can be addressed later in > case we get devices with 8 lanes. But adding a comment here (or where the > maximum number of lanes is defined) that this needs to be fixed would be > prudent to avoid introducing a bug later on. The comment above talks about this, but I can add one more comment next to the ACPI_DEVICE_CSI2_DATA_LANES definition. > > + > > + 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_info(handle, "No lane polarity bytes\n"); > > + } > > + > > + swnodes->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] = > > + SOFTWARE_NODE("endpoint@0", swnodes->ports[port_index].ep_props, > > + &swnodes->nodes[ACPI_DEVICE_SWNODE_PORT(port_index)]); > > + > > + if (port->crs_csi2_local) > > + init_csi2_port_local(adev, port, port_fwnode, ep_prop_index); > > +} > > + > > +#define MIPI_IMG_PORT_PREFIX "mipi-img-port-" > > + > > +static struct fwnode_handle *get_mipi_port_handle(struct fwnode_handle *adev_fwnode, > > + unsigned int port_nr) > > +{ > > + char port_name[sizeof(MIPI_IMG_PORT_PREFIX) + 2]; > > + > > + if (snprintf(port_name, sizeof(port_name), "%s%u", > > + MIPI_IMG_PORT_PREFIX, port_nr) >= sizeof(port_name)) > > + return NULL; > > + > > + return fwnode_get_named_child_node(adev_fwnode, port_name); > > +} > > + > > +static void init_crs_csi2_swnodes(struct crs_csi2 *csi2) > > +{ > > + struct acpi_buffer buffer = { .length = ACPI_ALLOCATE_BUFFER }; > > + struct acpi_device_software_nodes *swnodes = csi2->swnodes; > > + acpi_handle handle = csi2->handle; > > + struct fwnode_handle *adev_fwnode; > > + struct acpi_device *adev; > > + acpi_status status; > > + unsigned int i; > > + int ret; > > + > > + /* > > + * Bail out if the swnodes are not available (either they have not been > > + * allocated or they have been assigned to the device already). > > + */ > > + if (!swnodes) > > + return; > > + > > + adev = acpi_fetch_acpi_dev(handle); > > + if (!adev) > > + return; > > + > > + adev_fwnode = acpi_fwnode_handle(adev); > > + > > + status = acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer); > > + if (ACPI_FAILURE(status)) { > > + acpi_handle_info(handle, "Unable to get the path name\n"); > > + return; > > + } > > + > > + swnodes->nodes[ACPI_DEVICE_SWNODE_ROOT] = > > + SOFTWARE_NODE(buffer.pointer, swnodes->dev_props, NULL); > > + > > + for (i = 0; i < swnodes->num_ports; i++) { > > + struct acpi_device_software_node_port *port = &swnodes->ports[i]; > > + struct fwnode_handle *port_fwnode; > > + > > + /* > > + * The MIPI DisCo for Imaging specification defines _DSD device > > + * properties for providing CSI-2 port parameters that can be > > + * accessed through the generic device properties framework. To > > + * access them, it is first necessary to find the data node > > + * representing the port under the given ACPI device object. > > + */ > > + port_fwnode = get_mipi_port_handle(adev_fwnode, port->port_nr); > > + if (!port_fwnode) { > > + acpi_handle_info(handle, > > + "MIPI port name too long for port %u\n", > > + port->port_nr); > > + continue; > > + } > > + > > + init_csi2_port(adev, swnodes, port, port_fwnode, i); > > + > > + fwnode_handle_put(port_fwnode); > > + } > > + > > + ret = software_node_register_node_group(swnodes->nodeptrs); > > + if (ret < 0) { > > + acpi_handle_info(handle, > > + "Unable to register software nodes (%d)\n", ret); > > + return; > > + } > > + > > + adev->swnodes = swnodes; > > + adev_fwnode->secondary = software_node_fwnode(swnodes->nodes); > > + > > + /* > > + * Prevents the swnodes from this csi2 entry from being assigned again > > + * or freed prematurely. > > + */ > > + csi2->swnodes = NULL; > > +} > > + > > +/** > > + * acpi_mipi_init_crs_csi2_swnodes - Initialize _CRS CSI-2 software nodes > > + * > > + * Use MIPI DiSco for Imaging device properties to finalize the initialization > > + * of CSI-2 software nodes for all ACPI device objects that have been already > > + * enumerated. > > + */ > > +void acpi_mipi_init_crs_csi2_swnodes(void) > > +{ > > + struct crs_csi2 *csi2, *csi2_tmp; > > + > > + list_for_each_entry_safe(csi2, csi2_tmp, &acpi_mipi_crs_csi2_list, entry) > > + init_crs_csi2_swnodes(csi2); > > +} > > + > > /** > > * acpi_mipi_crs_csi2_cleanup - Free _CRS CSI-2 temporary data > > */ > > Index: linux-pm/drivers/acpi/internal.h > > =================================================================== > > --- linux-pm.orig/drivers/acpi/internal.h > > +++ linux-pm/drivers/acpi/internal.h > > @@ -287,6 +287,7 @@ static inline void acpi_init_lpit(void) > > > > void acpi_mipi_check_crs_csi2(acpi_handle handle); > > void acpi_mipi_scan_crs_csi2(void); > > +void acpi_mipi_init_crs_csi2_swnodes(void); > > void acpi_mipi_crs_csi2_cleanup(void); > > > > #endif /* _ACPI_INTERNAL_H_ */ > > Index: linux-pm/drivers/acpi/scan.c > > =================================================================== > > --- linux-pm.orig/drivers/acpi/scan.c > > +++ linux-pm/drivers/acpi/scan.c > > @@ -2447,6 +2447,13 @@ static void acpi_scan_postponed_branch(a > > > > acpi_walk_namespace(ACPI_TYPE_ANY, handle, ACPI_UINT32_MAX, > > acpi_bus_check_add_2, NULL, NULL, (void **)&adev); > > + > > + /* > > + * Populate the ACPI _CRS CSI-2 software nodes for the ACPI devices that > > + * have been added above. > > + */ > > + acpi_mipi_init_crs_csi2_swnodes(); > > + > > acpi_bus_attach(adev, NULL); > > } > > > > @@ -2516,11 +2523,12 @@ int acpi_bus_scan(acpi_handle handle) > > return -ENODEV; > > > > /* > > - * Allocate ACPI _CRS CSI-2 software nodes using information extracted > > + * Set up ACPI _CRS CSI-2 software nodes using information extracted > > * from the _CRS CSI-2 resource descriptors during the ACPI namespace > > - * walk above. > > + * walk above and MIPI DiSco for Imaging device properties. > > */ > > acpi_mipi_scan_crs_csi2(); > > + acpi_mipi_init_crs_csi2_swnodes(); > > > > acpi_bus_attach(device, (void *)true); > > > > Index: linux-pm/include/acpi/acpi_bus.h > > =================================================================== > > --- linux-pm.orig/include/acpi/acpi_bus.h > > +++ linux-pm/include/acpi/acpi_bus.h > > @@ -366,10 +366,17 @@ struct acpi_device_data { > > > > struct acpi_gpio_mapping; > > > > +#define ACPI_DEVICE_SWNODE_ROOT 0 > > + > > #define ACPI_DEVICE_CSI2_DATA_LANES 4 > > > > #define ACPI_DEVICE_SWNODE_PORT_NAME_LENGTH 8 > > > > +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_PORT_REG, > > ACPI_DEVICE_SWNODE_PORT_NUM_OF, > > @@ -425,12 +432,14 @@ struct acpi_device_software_node_port { > > > > /** > > * struct acpi_device_software_nodes - Software nodes for an ACPI device > > + * @dev_props: Device properties. > > * @nodes: Software nodes for root as well as ports and endpoints. > > * @nodeprts: Array of software node pointers, for (un)registering them. > > * @ports: Information related to each port and endpoint within a port. > > * @num_ports: The number of ports. > > */ > > struct acpi_device_software_nodes { > > + struct property_entry dev_props[ACPI_DEVICE_SWNODE_DEV_NUM_ENTRIES]; > > struct software_node *nodes; > > const struct software_node **nodeptrs; > > struct acpi_device_software_node_port *ports; > > @@ -455,6 +464,7 @@ struct acpi_device { > > struct acpi_device_data data; > > struct acpi_scan_handler *handler; > > struct acpi_hotplug_context *hp; > > + struct acpi_device_software_nodes *swnodes; > > const struct acpi_gpio_mapping *driver_gpios; > > void *driver_data; > > struct device dev; > > > > > > > > -- > Regards, > > Sakari Ailus >
