Hi Andy,
On Tue, Jan 24, 2023 at 09:26:31PM +0200, Andy Shevchenko wrote:
> 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?
We could remove the union, yes, with one more u32 of stack used.
>
> > + /* 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)
I find the original better: it does this by referring to the array itself.
>
> > + 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?!
Hmm. I think in the past, some of these functions have returned the number
of the entries even when the buffer is provided
(acpi_copy_property_array_string() still does!). Good catch, I'll fix this
for v3.
>
> > + 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);
You'll still needed to access invididual bits in val.
>
> // 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)]);
> > +}
--
Kind regards,
Sakari Ailus
