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

Andy Shevchenko
2023-01-25T12:03:44Z

On Wed, Jan 25, 2023 at 02:00:25PM +0200, Andy Shevchenko wrote:
> On Wed, Jan 25, 2023 at 01:53:32PM +0200, Sakari Ailus wrote:
> > On Wed, Jan 25, 2023 at 01:46:20PM +0200, Andy Shevchenko wrote:
> > > On Wed, Jan 25, 2023 at 10:56:41AM +0200, Sakari Ailus wrote:
> > > > 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:

...

> > > > > // 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.
> > >
> > > I didn't get this. The below is what it does in most efficient way.
> >
> > Ah. You're assining eight bits at a time.
>
> > Then the loop ends too late as i refers to a byte, not bit. This can be
> > addressed though. And a BUILD_BUG_ON() check for polarity being large
> > enough will be needed.
>
> You probably meant static_assert(), but see my reply to my reply where
> I caught up this. Yes, the loop conditional should rely on byte count.
>
> > I still find this more complicated than the original code that also does
> > not need a temporary buffer.
>
> Your magic formula with bit shifts and conjunctions is so hard to read
> and error prone, that makes me think of the proper APIs in the first place.
> That's why I'm tending to use this code, because it's much easier to get
> and maintain.
>
> > > > > // assuming that lane_polarities is zeroed by default...
> > > > > for_each_set_bit(i, polarity, 1 + num_lanes)
> > > > > port->lane_polarities[i] = 1;

This even can be optimized much more if we put a constant bit numbers and if
it's less than or equal to BITS_PER_LONG.

for_each_set_bit(i, polarity, MAX_LANES(port))

> > > Note that his code lacks of endianess issues.
> > >
> > > > > }

--
With Best Regards,
Andy Shevchenko