Re: [PATCH v1 2/3] iio: light: ROHM BU27008 color sensor

[Jonathan Cameron]

On Mon, 24 Apr 2023 06:21:23 +0000
"Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx> wrote:

> On 4/23/23 15:57, Jonathan Cameron wrote:
> > On Fri, 21 Apr 2023 12:39:36 +0300
> > Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
> >   
> >> The ROHM BU27008 is a sensor with 5 photodiodes (red, green, blue, clear
> >> and IR) with four configurable channels. Red and green being always
> >> available and two out of the rest three (blue, clear, IR) can be
> >> selected to be simultaneously measured. Typical application is adjusting
> >> LCD backlight of TVs, mobile phones and tablet PCs.
> >>
> >> Add initial support for the ROHM BU27008 color sensor.
> >>   - raw_read() of RGB and clear channels
> >>   - triggered buffer w/ DRDY interrtupt
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>  
> > 
> > Hi Matti,
> > 
> > Biggest issue in here is some confusion over data packing when some channels
> > are enabled.  The driver must pack the channels that are enabled (seen
> > from active_scan_mask) according to general IIO rules.  So naturally aligned
> > buffers.  Thus for this device it should always be packed into
> > 
> > struct scan {
> > 	__le16 chans[4];
> > 	s64 ts __aligned(8); /*it's aligned anyway but better not to make reviewers think ;) */
> > };
> > 
> > Even though there are 5 possible channels.  If one in the middle isnt' enabled (e.g. blue)
> > then clear and IR shift to lower words. of the buffer.  
> 
> Ah, right. So I had misunderstood how the buffer works. I thought the 
> scan_mask was only used to disallow unsupported channel-enabling 
> configurations. If I understand your statement correctly, the scan_mask 
> is used to determine the 'place of data' in the buffer when certain 
> configuration is used. (I'll check this from the code but if the IIO 
> handles data as I now think - that's cool! It should indeed simplify the 
> buffer in driver side!).

'Place' is a little confusing (English is imprecise sometimes).  Order
is perhaps more precise.

Place can mean same as order - as in 1st place in a race, but it can
also mean a specific location such as a place at a table where only
some seats are full.

The handling for this came about as part of the multiple consumer
support for SoC ADCs but it was also useful for ripping out a whole
load of driver specific handling that did similar repacking and letting
the generic code handle repacking data.  It is fairly optimal and
does things like larger memcpys if there are a set of channels
that need moving, and cleanly makes it a noop if there is nothing
to do at all.  All those tricks came IIRC from individual drivers
that had previously been doing this magic.

Jonathan