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
