Message-ID: <20230304191111.60791366@jic23-huawei>
Date:   Sat, 4 Mar 2023 19:11:11 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shreeya Patel <shreeya.patel@...labora.com>,
        Zhigang Shi <Zhigang.Shi@...eon.com>,
        Paul Gazzillo <paul@...zz.com>,
        Dmitry Osipenko <dmitry.osipenko@...labora.com>,
        Liam Beguin <liambeguin@...il.com>,
        Peter Rosin <peda@...ntia.se>,
        Randy Dunlap <rdunlap@...radead.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 5/6] iio: light: ROHM BU27034 Ambient Light Sensor



> >   
> >> + * need to modify the calculation but I hope this gives a starting point for
> >> + * those working with such devices.  
> > 
> > That will need some dt bindings - though for now I guess we have no idea
> > what they would be unless there are some hints on the datasheet?
> >   
> 
> Yes. That is the problem. And even though we would hope we get the 
> complete bindings from day 1 - I don't see it really a problem to add 
> things like the 'lens-whateveritwillbe' when needed. What should be 
> ensured then is the "property not found" case will default to the open-air.

We have had app notes in the past that suggest particular useful factors
for common filters or indeed the correction factor format that should be used
if the device has some non volatile memory to store it in.  If neither
is true here, it's going to be 'interesting' to support.

> 
> The one thing we could add is sysfs attribute stating the 'open-air' for 
> those who use the raw-values as they will be impacted by lens and won't 
> be compensated by the driver like preprocessed values should be. Not 
> sure if we want to do it yet though as I don't know if there will be any 
> use for it in upstream.

Leave it for now.

> 
> >> + *
> >> + * The first case (D1/D0 < 0.87) can be computed to a form:
> >> + * lx = 0.004521097 * D1 - 0.002663996 * D0 + 0.00012213 * D1 * D1 / D0
> >> + */
> >> +static int bu27034_get_lux(struct bu27034_data *data, int *val)
> >> +{
> >> +	unsigned int gain0, gain1, meastime;
> >> +	unsigned int d1_d0_ratio_scaled;
> >> +	u16 res[3], ch0, ch1;
> >> +	u64 helper64;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&data->mutex);
> >> +	ret = bu27034_get_result_unlocked(data, &res[0]);  
> > 
> > res
> > as it is expecting a point to an array so that is more natural than pointing
> > to the first element even if that's the same result.  
> 
> This is pretty much the only thing I disagree with you :) For me it has 
> always been much clearer to use pointer to first element - as the type 
> of first element is what we are using. Type of an array (in my head) is 
> something less well defined. I think this difference is best visible 
> with the sizeof(arr) Vs. sizeof(&arr[0]).
> 
> I think I didn't change this for v2. I in any case expect to see v3 and 
> probably a few others as well - so I will change this to some of the 
> later versions if I didn't get you convinced that the &res[0] is Ok.

I don't care that strongly. Though to me res is no less clear than
&res[0] when we are intending to access the whole array.

If we had a series of individual element accesses or other partial
writes of the array in the functions then I'd agree with you.

> 
> >   
> >> +	if (ret)
> >> +		goto unlock_out;
> >> +
> >> +	/* Avoid div by zero */
> >> +	if (!res[0])  
> > 
> > res[0] = max(1, res[0]); perhaps?  
> 
> This would have been better, yes. However, I did change the data 
> collection quite a bit for v2 - and there these values may not be in 
> native byte order - so check for !res[0] feels more correct for v2 than 
> comparing to value when the value format is not "correct".

Ok. Maybe... I'll take a look.

> 
> >> +	case IIO_CHAN_INFO_RAW:
> >> +	{
> >> +		u16 res[3];
> >> +
> >> +		if (chan->type != IIO_INTENSITY)
> >> +			return -EINVAL;
> >> +
> >> +		if (chan->channel < BU27034_CHAN_DATA0 ||
> >> +		    chan->channel > BU27034_CHAN_DATA2)
> >> +			return -EINVAL;
> >> +		/*
> >> +		 * Reading one channel at a time is inefficient.
> >> +		 *
> >> +		 * Hence we run the measurement on the background and always
> >> +		 * read all the channels. There are following caveats:
> >> +		 * 1) The VALID bit handling is racy. Valid bit clearing is not
> >> +		 * tied to reading the data in the hardware. We clear the
> >> +		 * valid-bit manually _after_ we have read the data - but this
> >> +		 * means there is a small time-window where new result may
> >> +		 * arrive between read and clear. This means we can miss a
> >> +		 * sample. For normal use this should not be fatal because
> >> +		 * usually the light is changing slowly. There might be
> >> +		 * use-cases for measuring more rapidly changing light but this
> >> +		 * driver is unsuitable for those cases anyways. (Smallest
> >> +		 * measurement time we support is 55 mS.)  
> > 
> > Given there is no general fix for that, not much you can do even if you don't want to
> > miss the data.
> >   
> >> +		 * 2) Data readings more frequent than the meas_time will return
> >> +		 * the same cached values. This should not be a problem for the
> >> +		 * very same reason 1) is not a problem.  
> > 
> > Hmm. I'm never that keen on drivers doing that if we can avoid it but perhaps we
> > can't here.  
> 
> Well, I dropped the caching of values for read_raw. I think it got rid 
> of these particular problems. The issue 1) is still there for buffered 
> mode but I guess we just need to live with it. On the bright side, 
> missing a sample once in a blue moon is not fatal for most of the 
> use-cases I can think of right now. (Besides, there is no general fix as 
> you said so worrying about the unknown use-cases right now does not feel 
> like the sanest thing. I have enough of worrying with the things that 
> really are a problem...)
> 
> >> +	/*
> >> +	 * Delay to allow IC to initialize. We don't care if we delay
> >> +	 * for more than 1 ms so msleep() is Ok. We just don't want to
> >> +	 * block  
> > 
> > The msleep bit is kind of obvious for a reset. I'd not bother documenting that
> > detail.  
> 
> Well, the documentation is to suppress review comments regarding 1mS 
> msleep :) And, I can't blame reviewers as the checkpatch is picking this 
> up too. Hence I think it's Ok to tell that: "Yes, I know the sleep is 
> likely to last longer than the requested 1 mS but it does not matter for 
> our use-case so we still consciously chose to use msleep()."
I understand how you ended up with it, but meh, reviewers have seen those
warnings lots of times already!
:)  
..


> > Fallback compatibles require that on a failure to match ID we still let the driver
> > carry on.  However we can print something in the log to say we don't recognise
> > the device.  The intent is that at future part can be supported by old kernels just
> > be having the dt list multiple compatibles if the device really are backwards
> > compatible with parts already supported.  
> 
> Makes sense. Besides, we should be able to trust the dt has correct 
> compatibles - I'm not sure we should do these runtime checks for part 
> IDs at all. I dropped the error - return and changed the print to warn.

Experience says the checks are useful.  Lots of boards have turned up with the
wrong part, so warning at least is a nice to have!

We had to argue a bit with the DT maintainers to get them to let us have
the warnings ;)

Jonathan

Powered by blists - more mailing lists