Hi Andy,
On Tue, Jan 17, 2023 at 04:59:18PM +0200, Andy Shevchenko wrote:
> On Tue, Jan 17, 2023 at 02:22:40PM +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.
>
> ...
>
> > +#define GRAPH_PORT_NAME(var, num) \
> > + (snprintf((var), sizeof(var), "port@%u", (num)) > sizeof(var))
>
> SWNODE_GRAPH_PORT_NAME_FMT ?
The name is not used anywhere else. I would keep it as-is.
>
> ...
>
> > +#define NEXT_PROPERTY(index, max) \
> > + (WARN_ON(++(index) > ACPI_DEVICE_SWNODE_##max + 1) ? \
>
> '>' -- > '>=' and drop ' + 1'?
Yeah.
>
> > + ACPI_DEVICE_SWNODE_##max : (index) - 1)
>
> ...
>
> > + static const char mipi_port_prefix[] = "mipi-img-port-";
>
> It's harder to read in the code, please put it in place.
There are multiple uses of it. It's better there's a single definition.
>
> ...
>
> > + port->ep_props[NEXT_PROPERTY(*ep_prop_index,
> > + EP_DATA_LANES)] =
>
> It's hard to read, taking into account that you split on index of the array.
>
> How much a new monitor costs for you? Maybe I can donate to make you use more
> than 80 from time to time? :-)
You know newspaper pages are split into multiple columns for a reason,
similarly web pages with text columns very seldom span the entire page
width. The number of characters per line tends to be less than --- you
might be surprised --- 80. The reason is readability.
>
> > + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes",
> > + port->data_lanes,
> > + num_lanes);
>
> ...
>
>
> > + ret = fwnode_property_read_u8_array(mipi_port_fwnode,
> > + "mipi-img-lane-polarities",
> > + u.val8, sizeof(u.val8));
> > + if (ret > 0) {
> > + unsigned int bytes = ret;
> > +
> > + /* Total number of lanes here is clock lane + data lanes */
> > + if (bytes << 3 >= 1 + num_lanes) {
>
> bytes * BITS_PER_BYTE? Or if you want to be super precise BITS_PER_TYPE(u8).
I think of these two, BITS_PER_TYPE(u8) looks better.
>
> > + unsigned int i;
> > +
> > + /* 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;
>
> > + port->ep_props[NEXT_PROPERTY(*ep_prop_index,
> > + EP_LANE_POLARITIES)] =
>
> Index on one line?
>
> > + PROPERTY_ENTRY_U32_ARRAY_LEN("lane-polarities",
> > + port->lane_polarities,
> > + 1 + num_lanes);
> > + } else {
> > + acpi_handle_warn(acpi_device_handle(device),
> > + "too few lane polarity bytes (%u)\n",
> > + bytes);
> > + }
> > + }
>
> ...
>
> > + unsigned int port_index = next_csi2_port_index(device->swnodes,
> > + port_nr);
>
> One line easier to read.
>
> ...
>
> > + if (!ret)
>
> Why not positive conditional?
The success case is handled first.
> Also seems like {} are missing since the body is multi-line.
Multiple lines as such isn't a reason to add braces (per coding style).
>
> > + port->ep_props[NEXT_PROPERTY(ep_prop_index,
> > + EP_LINK_FREQUENCIES)] =
>
> Index on one line?
This is more or less random.
>
> > + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies",
> > + port->link_frequencies,
> > + num_link_freqs);
> > + else
> > + acpi_handle_info(acpi_device_handle(device),
> > + "can't get link frequencies (%d)\n",
> > + ret);
>
> ...
>
> > + if (acpi_get_name(device->handle, ACPI_FULL_PATHNAME, &buffer) != AE_OK) {
>
> We have ACPI_SUCCESS() / ACPI_FAILURE()
Yes.
>
> > + acpi_handle_warn(acpi_device_handle(device), "cannot get path name\n");
> > + return;
> > + }
>
> ...
>
> > + ads->nodes[ACPI_DEVICE_SWNODE_ROOT] = (struct software_node) {
> > + .name = buffer.pointer,
> > + .properties = ads->dev_props,
> > + };
>
> Aren't you provided a macro for this?
I think I added a macro for this but forgot to use it. I'll address this
(and other issues) in v2.
--
Regards,
Sakari Ailus
