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
