| Message ID | 20230123134617.265382-5-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 |
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; > +}
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; > > +}
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.
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)]); > +}
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; > } > }
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.
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)]); > > +}
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. > > } > > }
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. > > > > } > > > } >
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. > > > > > > }
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. > > > > > > > > }
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;
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(+)