Re: [PATCH 4/8] ACPI: property: Generate camera swnodes for ACPI and DisCo for Imaging

Andy Shevchenko
2023-01-17T15:01:50Z

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