Hi Andy,
On Tue, Mar 28, 2023 at 06:44:40PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 01:12:58PM +0300, Sakari Ailus wrote:
> > Prepare generating 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 NO_CSI2_PORT (UINT_MAX - 1)
>
> Has it been used before this patch?
I don't think so.
It has its current form as you preferred it earlier. :-)
>
> ...
>
> > +/* Print graph port name into a buffer, return non-zero if failed. */
> > +#define GRAPH_PORT_NAME(var, num) \
> > + (snprintf((var), sizeof(var), SWNODE_GRAPH_PORT_NAME_FMT, (num)) >= \
> > + sizeof(var))
>
> This macro evaluates sizeof(var) twice. Is it a problem?
I don't think it is.
>
> ...
>
> > +#define NEXT_PROPERTY(index, max) \
> > + (WARN_ON(++(index) > ACPI_DEVICE_SWNODE_##max + 1) ? \
> > + ACPI_DEVICE_SWNODE_##max : (index) - 1)
>
> This macro full of interesting effects. On top of that it's written in
> hard-to-read form.
>
> Why not at least
>
> #define NEXT_PROPERTY(index, max) \
> (WARN_ON((index) > ACPI_DEVICE_SWNODE_##max) ? \
> ACPI_DEVICE_SWNODE_##max : (index)++)
>
> ?
This appears functionally (almost) equivalent --- it wouldn't overflow.
I'll use this in v8.
>
> ...
>
> > + ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-lane-polarities");
> > + if (ret > 0) {
> > + unsigned int bytes = min_t(unsigned int, ret, sizeof(val));
> > +
> > + fwnode_property_read_u8_array(mipi_port_fwnode,
> > + "mipi-img-lane-polarities",
> > + val, bytes);
> > +
> > + /* 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] =
> > + (bool)(val[i >> 3] & (1 << (i & 7)));
>
> Casting to bool?!
This was the result of our earlier discussion on an earlier version of this
set.
>
> Can we read the array and convert it to bitmap, then this voodoo-ish code can
> be simplified to
>
> for_each_set_bit(i, ...)
> ..._polarities[i] = 1;
for_each_set_bit() operates on unsigned longs (usually u32 or u64) but we
have u8s here. There's an endianness issue there.
>
> (assuming initially they are 0:s)?
--
Kind regards,
Sakari Ailus
