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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Andy,

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:
> 
> ...
> 
> > > > +		if (num_lanes > ARRAY_SIZE(port->data_lanes)) {
> > > 
> > > 		>= MAX_LANES(port)
> > 
> > I find the original better: it does this by referring to the array itself.
> 
> Whatever, it's just an example to show where it's being used.
> 
> > > > +			acpi_handle_warn(acpi_device_handle(device),
> > > > +					 "too many data lanes (%u)\n",
> > > > +					 num_lanes);
> > > > +			num_lanes = ARRAY_SIZE(port->data_lanes);
> > > 
> > > 			= MAX_LANES(port) - 1;
> > > 
> > > > +		}
> 
> ...
> 
> > > 	ret = fwnode_property_count_u8(mipi_port_fwnode, "mipi-img-dlane-polarities");
> > > 	if (ret < 0) {
> > > 		acpi_handle_debug(acpi_device_handle(device),
> > > 				  "no lane polarity provided\n");
> > > 	} else if (ret < 1 + num_lanes) {
> > > 		acpi_handle_warn(acpi_device_handle(device),
> > > 				 "too few lane polarity bytes (%u)\n", bytes);
> > > 	} else {
> > > 		// 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.

I still find this more complicated than the original code that also does
not need a temporary buffer.

> 
> > > 			// assuming that lane_polarities is zeroed by default...
> > > 			for_each_set_bit(i, polarity, 1 + num_lanes)
> > > 				port->lane_polarities[i] = 1;
> 
> Note that his code lacks of endianess issues.
> 
> > > 		}
> > > 	}
> 

-- 
Kind regards,

Sakari Ailus



[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]
  Powered by Linux