Re: [PATCH v7 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging

From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: linux-acpi@vger.kernel.org, linux-media@vger.kernel.org,
	rafael@kernel.org, heikki.krogerus@linux.intel.com
Subject: Re: [PATCH v7 05/10] ACPI: property: Prepare generating swnodes for ACPI and DisCo for Imaging
Date: Wed, 29 Mar 2023 17:38:04 +0300	[thread overview]
Message-ID: <ZCRNTFzTS/FhqKhr@smile.fi.intel.com> (raw)
In-Reply-To: <ZCM+KiZpLt/qf59F@kekkonen.localdomain>

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.

> > #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)++)

...

> > > +	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.

-- 
With Best Regards,
Andy Shevchenko