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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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;
> > +}

-- 
Kind regards,

Sakari Ailus