Date:   Sun, 12 Mar 2023 15:36:55 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Matti Vaittinen <mazziesaccount@gmail.com>
Cc:     Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Paul Gazzillo <paul@pgazz.com>,
        Zhigang Shi <Zhigang.Shi@liteon.com>,
        Shreeya Patel <shreeya.patel@collabora.com>,
        Dmitry Osipenko <dmitry.osipenko@collabora.com>,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org
Subject: Re: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor
Message-ID: <20230312153655.052d5730@jic23-huawei>
In-Reply-To: <c16d372f-a122-16d6-ad08-1fbffb01d9ff@gmail.com>
References: <cover.1677750859.git.mazziesaccount@gmail.com>
        <874d59be98703bb58a98fea72138de5b94d71a52.1677750859.git.mazziesaccount@gmail.com>
        <20230304201720.2d554f07@jic23-huawei>
        <c16d372f-a122-16d6-ad08-1fbffb01d9ff@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Sun, 5 Mar 2023 14:22:51 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 3/4/23 22:17, Jonathan Cameron wrote:
> > On Thu, 2 Mar 2023 12:58:59 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> +/*
> >> + * The BU27034 does not have interrupt or any other mechanism of triggering
> >> + * the data read when measurement has finished. Hence we poll the VALID bit in
> >> + * a thread. We will try to wake the thread BU27034_MEAS_WAIT_PREMATURE_MS
> >> + * milliseconds before the expected sampling time to prevent the drifting. Eg,
> >> + * If we constantly wake up a bit too late we would eventually skip a sample.  
> > 
> > Lazier approach would be to just sent the sampling frequency at twice the
> > expected frequency and you'll never miss a sample unless you the wake up is
> > delayed massively for some reason.  Particularly 'fresh' data might not matter
> > enough that half a cycle late is a problem.  
> 
> Hmm. Do I read this right - You suggest we drop the polling loop for 
> valid bit and just always sleep for int_time / 2 if data was not valid?

Yes.  There are costs to both methods, but the advantage here is that the chance
of being so late you miss becomes much less.

> 
> I don't know. That would probably make the time-stamps for buffered 
> results to be jumping quite a bit - especially with the longer 
> integration times.

True enough.  They are probably fairly noisy either way but this would make them
worse.

> 
> >> + * And because the sleep can't wake up _exactly_ at given time this would be
> >> + * inevitable even if the sensor clock would be perfectly phase-locked to CPU
> >> + * clock - which we can't say is the case.
> >> + *
> >> + * This is still fragile. No matter how big advance do we have, we will still
> >> + * risk of losing a sample because things can in a rainy-day skenario be
> >> + * delayed a lot. Yet, more we reserve the time for polling, more we also lose
> >> + * the performance by spending cycles polling the register. So, selecting this
> >> + * value is a balancing dance between severity of wasting CPU time and severity
> >> + * of losing samples.
> >> + *
> >> + * In most cases losing the samples is not _that_ crucial because light levels
> >> + * tend to change slowly.
> >> + */
> >> +#define BU27034_MEAS_WAIT_PREMATURE_MS	5
> >> +#define BU27034_DATA_WAIT_TIME_US	1000
> >> +#define BU27034_TOTAL_DATA_WAIT_TIME_US (BU27034_MEAS_WAIT_PREMATURE_MS * 1000)  
> >   
> >> +static const struct iio_chan_spec bu27034_channels[] = {
> >> +	{
> >> +		.type = IIO_LIGHT,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> >> +				      BIT(IIO_CHAN_INFO_SCALE),  
> > 
> > What is this scale for?  
> 
> The scale is to inform users that we return data using milli lux.
> 
> > Given the channel is computed from various different inputs, is there a
> > clear definition of how it is scaled?  What does a write to it mean?  
> 
> Nothing. writing anything else but milli lux scale fails with -EINVAL.
> 
> I guess I am doing something in an unusual way here :) Do you have a 
> suggestion for me?

Return data in lux?  Or return it as INFO_RAW - thus making it clear
that the reading is not in expected units and a conversion must be
applied by userspace.  SCALE is not applied to PROCESSED by userspace.

In the rare case where you do get SCALE and PROCESSED it's there to allow
for changes in the underlying signal measurement that are eaten up in the
computation needed to get to PROCESSED - that is they have no visible
affect (beyond range changes etc).


...

> >> +
> >> +	ret = regmap_read(regmap, BU27034_REG_SYSTEM_CONTROL, &part_id);  
> > 
> > As it's not all of the register I'd rename the temporary variable to
> > val or reg or something along those lines.  
> 
> I still like having the variable named part_id - as it makes the check 
> obvious. What I did was adding another temporary variable 'reg' and doing:
> 
> part_id = FIELD_GET(BU27034_MASK_PART_ID, reg);
> 
> and then using the part_id in if() and dev_warn().

Looks good.


Jonathan