Re: [PATCH 1/2] dt-bindings: iio: light: Add ROHM BH1730FVC binding

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 12 May 2026 07:42:46 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 11/05/2026 18:14, Jonathan Cameron wrote:
> > On Mon, 11 May 2026 13:43:56 +0300
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 11/05/2026 11:22, Matti Vaittinen wrote:  
> >>> Thanks for patches Alexandre!
> >>>
> >>> It's nice to see these upstreamed :)
> >>>
> >>> On 10/05/2026 21:09, Alexandre Hamamdjian wrote:  
> >>>> From: CTCaer <ctcaer@gmail.com>
> >>>>
> >>>> Add a YAML binding for the ROHM BH1730FVC ambient light sensor.
> >>>> Documents the required compatible string, the als-vdd/als-vid
> >>>> regulators, and the rohm,integration-cycle, rohm,lux-multiplier,
> >>>> rohm,opt-win-coeff and rohm,gain-coeff calibration properties
> >>>> consumed by the driver.  
> >>
> >> // snip
> >>  
> >>>> +  rohm,opt-win-coeff:
> >>>> +    description:
> >>>> +      Optical-window calibration coefficients. Specified as a flat
> >>>> list of
> >>>> +      triplets <rc cv ci>, one triplet per window region, where rc is
> >>>> the
> >>>> +      visible/IR ratio cutoff and cv/ci are the visible and IR weighting
> >>>> +      factors used in that region.
> >>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> >>>> +    items:
> >>>> +      minItems: 3
> >>>> +      maxItems: 3  
> >>>
> >>> I am not sure if I read the driver patch (2/2) correctly, but if I did,
> >>> then these coefficients are used to compute Luxes out of the raw sensor
> >>> data. I believe it would help anyone integrating (or investigating) this
> >>> sensor, if you added the actual formula here as a comment. If I read
> >>> this right, the formula is _somehting_ like:
> >>>
> >>>
> >>> Lx = (cv[win] * ch0_data - ci[win] * ch1_data) / gain / int_time
> >>>
> >>> Here the cv[win] and ci[win] are selected from the opt-win-coeff -table,
> >>> depending on the measured ch1_data/ch0_data ratio, right?  
> >>
> >> One thing came to my mind. This 'window' -approach for lux calculation
> >> is not too unique. For example the rohm-bu27034.c uses similar approach.
> >>
> >> The thing is that some of the sensors have more than 2 channels. (For
> >> example, the first version of BU27034 did. [That was BU27034NUC, which
> >> got cancelled when BU27034_A_NUC emerged]). These ICs may still may use
> >> similar approach of having light regions, determined by ratio of (2)
> >> channels. BUT, they may then have more than 2 coefficients / window.
> >>
> >> So, maybe this could be made generic enough so it could be re-used for
> >> such devices if needed? I am not sure if other manufacturers but ROHM
> >> does this in Lux computations - if yes, then it might be worth making
> >> this more generic and not just a ROHM property? Maybe Jonathan has some
> >> insight on other Lux computations.  
> > 
> > It used to be very common to have multiple sensor / window setups for
> > ambient light sensors - though perhaps less so on more modern devices
> > (we have one on list today where they just say use the green channel
> >   of an RGB sensor - so there are more windows but not relevant to
> >   illuminance measurement).
> > 
> > Sometimes the window bit isn't well enough described in the datasheet
> > so we only dealt with the parts on the actual sensor package and those
> > were handled in driver rather than being in dt.
> > 
> > What I'm not sure on here is how much of what is being described
> > is part of the 'chip' packaging - i.e. the bit that is constant for
> > all instances of this device and how much is part of the wider
> > device - i.e. the laptop / phone etc window infront of the sensor.
> > 
> > The chip bit we shouldn't have dt, the other part we should and it
> > would indeed be interesting to work on a generalizing that description.  
> 
> Hm. I kind of agree. The constant, 'common for all sensors' 
> -coefficients should probably be hard-coded as sensor defaults. The 
> BH1730 data-sheet seems to be describing a set of coefficients. I 
> suppose that's what comes from the packaging(?)
> 
> And just to contradict myself...
> 
> I can imagine two reasons to alter these coefficients:
> 
> 1.(st) being what Jonathan described. Eg. having something like a lens 
> or a glass or whatever, on top of the sensor. That can alter the light 
> entering the sensor, and require change to the coefficients. And indeed, 
> it would make sense to only describe this in the DT, because the sensor 
> is already described. That'd mean the DT should only contain the 
> 'coefficient delta' caused by <add the cause here>.
> 
> 2. In theory, there could also be another reason. I bet the sensors are 
> all 'individuals' to some extent. So, for something requiring very high 
> accuracy, there could be some kind of calibration process at device 
> manufacturing. This might produce more accurate, device specific 
> coefficients. Considering this use-case, I am not 100% convinced it's 
> "wrong" to be able to give the device-specific coefficients from the DT. 
> (Because, here the coefficients really are a property of the device 
> itself and aren't added by some external part).

Agreed this happens - but you can't really put it in DT as every device
then has it's own DT - which probably means an early boot firmware patching
the DT based on something in ROM or similar.  Given that is usually impractical
we have handled this via firmware files - if no file, use defaults.

> 
> And, if we consider allowing describing (the more accurate) device 
> coefficients from the DT (for case 2), then it would just be simpler to 
> always provide the "full coefficients" from the DT, no matter if they 
> are caused by the device packaging or added lens/glass/XXX. And, as the 
> infamous, and not even existing rohm,dh2228fv device shows, people do 
> use the simplest solutions even when it isn't really right ;)
> 
> So, I am tempted to suggest we just go with the flow, and allow 
> describing the coefficients from the DT, without separating the source, 
> (lens/glass/device-packaging) which is what this patch does. (If I read 
> it right.) However, I would like to see a property which can be re-used 
> by other devices as this seems to be pretty common. Yeah, it's probably 
> not _right_, but it feels practical.

I'm fine with doing that if we are sure the coefficients are incorporating
windows in front of the sensor.  I know that wasn't the case for some
of the previous devices we've covered - typically because they either
had pretty optically clear windows - or didn't have one at all.

> 
> Well, this is just my 0.5 cents, and even I may change my opinion on 
> this though :)

I thought that was just me ;)
> 
> Yours,
> 	-- Matti
>