Hi Andy,
On Fri, Jan 27, 2023 at 12:35:12PM +0200, Andy Shevchenko wrote:
> On Thu, Jan 26, 2023 at 12:40:57AM +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 unsigned int next_csi2_port_index(struct acpi_device_software_nodes *ads,
> > + unsigned int port_nr)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ads->num_ports; i++) {
> > + struct acpi_device_software_node_port *port = &ads->ports[i];
> > +
> > + if (port->port_nr == port_nr)
> > + return i;
>
> > + if (port->port_nr != NO_CSI2_PORT)
> > + continue;
> > +
> > + port->port_nr = port_nr;
> > +
> > + return i;
>
> Maybe it would be better to use the same pattern as above?
>
> if (port->port_nr == NO_CSI2_PORT) {
> port->port_nr = port_nr;
> return i;
> }
Works for me...
>
> > + }
> > +
> > + return NO_CSI2_PORT;
> > +}
>
> ...
>
> > +static struct fwnode_handle *get_mipi_port_handle(struct acpi_device *device,
> > + unsigned int port)
> > +{
> > + static const char mipi_port_prefix[] = "mipi-img-port-";
> > + char mipi_port_name[sizeof(mipi_port_prefix) + 2];
>
> I think the following will be better:
>
> char mipi_port_name[16];
If the array is too short, this will generate a warning but... this is
already handled better than that. Why not to keep it?
>
>
> > + if (snprintf(mipi_port_name, sizeof(mipi_port_name), "%s%u",
> > + mipi_port_prefix, port) >= sizeof(mipi_port_name)) {
>
> if (snprintf(mipi_port_name, sizeof(mipi_port_name), "mipi-img-port-%u",
> 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);
> > +}
>
> ...
>
> > + union {
> > + u32 val;
>
> OK. I stopped here, because I'm not sure you have really sent an updated
> version. For sure the val and union is not used.
Indeed. My apologies --- I missed making a few changes I intended to do.
I'll go through those once more and then send v4.
>
> > + /* Data lanes + the clock lane */
> > + u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
> > + } u;
--
Kind regards,
Sakari Ailus
