On Mon, Jan 23, 2023 at 03:46:13PM +0200, Sakari Ailus wrote:
As promised the idea of bitmap APIs.
Also I have stumbled over couple of suspicious places. See below.
...
> +static void init_port_csi2_common(struct acpi_device *device,
> + struct fwnode_handle *mipi_port_fwnode,
> + unsigned int *ep_prop_index,
> + unsigned int port_nr)
> +{
> + unsigned int port_index = next_csi2_port_index(device->swnodes, port_nr);
> + struct acpi_device_software_nodes *ads = device->swnodes;
> + struct acpi_device_software_node_port *port = &ads->ports[port_index];
> + unsigned int num_lanes = 0;
> + union {
> + u32 val;
// Not sure why this even exists.
// And hence why do we need union?
> + /* Data lanes + the clock lane */
> + u8 val8[BITS_TO_BYTES(ARRAY_SIZE(port->data_lanes) + 1)];
> + } u;
Somewhere
#define MAX_LANES(port) (ARRAY_SIZE((port)->data_lanes) + 1)
u8 val8[BITS_TO_BYTES(MAX_LANES(port))];
...
/* Data lanes + the clock lane */
DECLARE_BITMAP(polarity, MAX_LANES(port)));
> + int ret;
> +
> + *ep_prop_index = ACPI_DEVICE_SWNODE_EP_CLOCK_LANES;
> +
> + if (GRAPH_PORT_NAME(port->port_name, port_nr))
> + return;
> +
> + ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)] =
> + SOFTWARE_NODE(port->port_name, port->port_props,
> + &ads->nodes[ACPI_DEVICE_SWNODE_ROOT]);
> +
> + ret = fwnode_property_read_u8(mipi_port_fwnode, "mipi-img-clock-lane", u.val8);
> + if (!ret) {
> + port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_CLOCK_LANES)] =
> + PROPERTY_ENTRY_U32("clock-lanes", *u.val8);
> + }
> + ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-data-lanes");
> + if (ret > 0) {
> + num_lanes = ret;
> +
> + if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
>= MAX_LANES(port)
> + acpi_handle_warn(acpi_device_handle(device),
> + "too many data lanes (%u)\n",
> + num_lanes);
> + num_lanes = ARRAY_SIZE(port->data_lanes);
= MAX_LANES(port) - 1;
> + }
> +
> + ret = fwnode_property_read_u8_array(mipi_port_fwnode, "mipi-img-data-lanes",
> + u.val8, num_lanes);
> + if (!ret) {
> + unsigned int i;
> +
> + for (i = 0; i < num_lanes; i++)
> + port->data_lanes[i] = u.val8[i];
> +
> + port->ep_props[NEXT_PROPERTY(*ep_prop_index, EP_DATA_LANES)] =
> + 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) {
How is it supposed to work?!
> + unsigned int bytes = ret;
> +
> + /* Total number of lanes here is clock lane + data lanes */
> + if (bytes * BITS_PER_TYPE(u8) >= 1 + num_lanes) {
> + 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)] =
> + 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);
> + }
> + }
ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
if (ret < 0) {
acpi_handle_debug(acpi_device_handle(device),
"no lane polarity provided\n");
} else if (ret < 1 + num_lanes) {
acpi_handle_warn(acpi_device_handle(device),
"too few lane polarity bytes (%u)\n", bytes);
} else {
// assuming we dropped the union and renamed to val...
ret = fwnode_property_read_u8_array(mipi_port_fwnode,
"mipi-img-lane-polarities",
val, sizeof(val));
if (ret) {
...can't read... (debug message?)
} else {
unsigned int i;
for (i = 0; i < 1 + num_lanes; i++)
bitmap_set_value8(polarity, val[i], i * BITS_PER_BYTE);
// assuming that lane_polarities is zeroed by default...
for_each_set_bit(i, polarity, 1 + num_lanes)
port->lane_polarities[i] = 1;
}
}
> + ads->nodes[ACPI_DEVICE_SWNODE_EP(port_index)] =
> + SOFTWARE_NODE("endpoint@0", ads->ports[port_index].ep_props,
> + &ads->nodes[ACPI_DEVICE_SWNODE_PRT(port_index)]);
> +}
--
With Best Regards,
Andy Shevchenko
