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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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