Hi Andy,
On Tue, Jan 24, 2023 at 06:38:26PM +0200, Andy Shevchenko wrote:
> 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 >= ?
Good catch. snprintf() excludes '\0' from the return value.
>
> > > > + 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.
The problem with the bitmap APIs is that they work on unsigned long, so
there are endian issues that will need to be handled. I thing it's simply
not worth it: either you have temporary space for this or a new API is
needed. What the line above is, after all, fairly trivial.
>
> ...
>
> > 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?
Yes, that's what I actually thought. But still a comment is good to have
here, so someone doesn't try to convert this to use the functions intended
for 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.
>
> Yeah, but it's quite a big topic out of the scope of this series.
Yes.
--
Regards,
Sakari Ailus
