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
