Received-SPF: pass (google.com: domain of linux-kernel+bounces-207719-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240606162948.83903-1-muditsharma.info@gmail.com>
 <20240606162948.83903-2-muditsharma.info@gmail.com> <20240608172227.17996c75@jic23-huawei>
In-Reply-To: <20240608172227.17996c75@jic23-huawei>
From: Matti Vaittinen <mazziesaccount@gmail.com>
Date: Mon, 10 Jun 2024 08:58:44 +0300
Message-ID: <CANhJrGM9czj0RL3OLCgRHEKc2QOjG9P0AZTrZxvYUk65TCpHRg@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] iio: light: ROHM BH1745 colour sensor
To: Jonathan Cameron <jic23@kernel.org>
Cc: Mudit Sharma <muditsharma.info@gmail.com>, lars@metafoo.de, krzk+dt@kernel.org, 
	conor+dt@kernel.org, robh@kernel.org, ivan.orlov0322@gmail.com, 
	javier.carrasco.cruz@gmail.com, linux-kernel@vger.kernel.org, 
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

la 8. kes=C3=A4k. 2024 klo 19.22 Jonathan Cameron (jic23@kernel.org) kirjoi=
tti:
>
> On Thu,  6 Jun 2024 17:29:42 +0100
> Mudit Sharma <muditsharma.info@gmail.com> 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 =3D IIO_INTENSITY, .modified =3D 1,                =
         \
> > +             .info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW),          =
       \
> > +             .info_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_HARDWAREG=
AIN) | \
>
> Provide _SCALE instead of HARDWAREGAIN
> As it's an intensity channel (and units are tricky for color sensors give=
n
> 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...


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));