Re: [PATCH v2 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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