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;
> >
> > Note that his code lacks of endianess issues.
> >
> > > > }
--
With Best Regards,
Andy Shevchenko
