Hi Andy,
On Wed, Mar 29, 2023 at 05:38:04PM +0300, Andy Shevchenko wrote:
> On Tue, Mar 28, 2023 at 10:21:14PM +0300, Sakari Ailus wrote:
> > 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:
>
> ...
>
> > > > #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. :-)
>
>
> My point is that it needs to be introduced where the first user appears.
Yes, the first use of this macro is in this patch.
>
> > > #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.
>
> In this form it even takes line less
>
> #define NEXT_PROPERTY(i, m) \
> (WARN_ON((i) > ACPI_DEVICE_SWNODE_##m) ? ACPI_DEVICE_SWNODE_##m : (i)++)
I prefer the previous form as it is easier to grasp what it does --- the
names of the variables suggest what they are for.
>
> ...
>
> > > > + 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.
>
> No issue if you convert it to unsigned long:s first.
>
> > > (assuming initially they are 0:s)?
>
> I think open coding the bitmap operations here is not a win.
Sure, this could be converted to a different form and then the bits could
be accessed using existing API functions. But I don't think it'd be better
that way.
--
Kind regards,
Sakari Ailus
