Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor

[Jonathan Cameron]

On Mon, 10 Jun 2024 08:58:44 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:

> la 8. kesäk. 2024 klo 19.22 Jonathan Cameron (jic23@xxxxxxxxxx) kirjoitti:
> >
> > On Thu,  6 Jun 2024 17:29:42 +0100
> > Mudit Sharma <muditsharma.info@xxxxxxxxx> wrote:
> >  
> > > Add support for BH1745, which is an I2C colour sensor with red, green,
> > > blue and clear channels. It has a programmable active low interrupt
> > > pin. Interrupt occurs when the signal from the selected interrupt
> > > source channel crosses set interrupt threshold high or low level.
> > >
> > > This driver includes device attributes to configure the following:
> > > - Interrupt pin latch: The interrupt pin can be configured to
> > >   be latched (until interrupt register (0x60) is read or initialized)
> > >   or update after each measurement.
> > > - Interrupt source: The colour channel that will cause the interrupt
> > >   when channel will cross the set threshold high or low level.
> > >
> > > This driver also includes device attributes to present valid
> > > configuration options/values for:
> > > - Integration time
> > > - Interrupt colour source
> > > - Hardware gain
> > >  
> 
> > > +
> > > +#define BH1745_CHANNEL(_colour, _si, _addr)                                   \
> > > +     {                                                                     \
> > > +             .type = IIO_INTENSITY, .modified = 1,                         \
> > > +             .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),                 \
> > > +             .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \  
> >
> > Provide _SCALE instead of HARDWAREGAIN
> > As it's an intensity channel (and units are tricky for color sensors given
> > frequency dependence etc) all you need to do is ensure that if you halve
> > the _scale and measure the same light source, the computed
> > _RAW * _SCALE value remains constant.  
> 
> ...Which is likely to cause also the integration time setting to
> impact the SCALE.
> 
> You may or may not want to see the GTS-helpers
> (drivers/iio/industrialio-gts-helper.c) - which have their own tricky
> corners. I think Jonathan once suggested to me to keep the
> HARDWAREGAIN as a read-only attribute to ease seeing what is going on.
> For the last couple of days I've been reworking the BU27034 driver to
> work with the new sensor variant - and I can definitely see the value
> of the read-only HARDWAREGAIN when we have per channel gain settings +
> integration time setting which all contribute to the scale...

I'm wondering if that was good advice, but it's definitely better
than letting userspace control the gain and integration time separately
as there is no sensible way to know how to control that beyond -
it's a bit dark and I forgot I can change the integration time,
crank up the gain!

> 
> 
> Matti Vaittinen
> Linux kernel developer at ROHM Semiconductors
> Oulu Finland
> 
> ~~ When things go utterly wrong vim users can always type :help! ~~
> 
> Discuss - Estimate - Plan - Report and finally accomplish this:
> void do_work(int time) __attribute__ ((const));
>