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 ?
...
> +#define NEXT_PROPERTY(index, max) \
> + (WARN_ON(++(index) > ACPI_DEVICE_SWNODE_##max + 1) ? \
'>' -- > '>=' and drop ' + 1'?
> + 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.
...
> + 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? :-)
> + 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).
> + 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?
Also seems like {} are missing since the body is multi-line.
> + port->ep_props[NEXT_PROPERTY(ep_prop_index,
> + EP_LINK_FREQUENCIES)] =
Index on one line?
> + 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()
> + 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?
--
With Best Regards,
Andy Shevchenko
