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

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

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.

-- 
With Best Regards,
Andy Shevchenko