Re: [PATCH v2 5/6] iio: light: ROHM BU27034 Ambient Light Sensor

[Matti Vaittinen]

On 3/4/23 22:17, Jonathan Cameron wrote:
> On Thu, 2 Mar 2023 12:58:59 +0200
> Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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?

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.

> > + * 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?


> > +			/*
> > +			 * As Jonathan put it, if caller requests for
>
> Probably don't reference me directly in a driver.  Keep the 'blame' for the
> email threads :)  Comment is fine otherwise.

Okay, okay :) I just liked the expression and didn't want to take the 
credit for it ;)

> > +
> > +	/*
> > +	 * The new integration time can be supported while keeping the scale of
> > +	 * channels intact by tuning the gains.
>
> This comment is in a path that is hit event if we go through the warnings
> above that say this isn't true.

Oh! Valid point! This changed when I allowed gain to be changed - need 
to drop the comment. Thanks!

> > +
> > +static int bu27034_calc_lux(struct bu27034_data *data, __le16 *res, int *val)
>
> As you are going to put it in the buffer, make val a fixed size integer.
> The current approach of calculate in an int and copy to a u32 is a bit nasty.
> Of course if there is a chance of a large enough value you'll have to be careful
> for the unsigned to signed conversion on 32 bit platforms. I doubt there is, but
> a comment saying why not would be great in the code that is hit from read_raw()

This same ..._calc_lux() is used also from the read_raw. I'd rather keep 
this using ints so there would be no need for involving u32 in 
read_raw() path. However, you are correct in that the current calling 
from buffered read where we pass pointer to u32 here - is nasty. I'll 
add temporary int in the calling function there, and add casting with a 
comment when storing value to scan.lux. Thanks for pointing this out! 
There was also missing an error return problem - see below.

> > +
> > +	if (d1_d0_ratio_scaled < 87)
> > +		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 0);
> > +	else if (d1_d0_ratio_scaled < 100)
> > +		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 1);
> > +	else
> > +		*val = bu27034_fixp_calc_lx(ch0, ch1, gain0, gain1, meastime, 2);
> > +

The bu27034_fixp_calc_lx() might return -EINVAL - which we missed here. 
I'll fix also this one.

> > +
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		if (chan->type != IIO_LIGHT)
> > +			return -EINVAL;
> > +
> > +		/* Don't mess with measurement enabling while buffering */
> > +		ret = iio_device_claim_direct_mode(idev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		mutex_lock(&data->mutex);
>
> See below. I would factor out the rest of this so that you can
> unconditionally unlock and then check the return value.

Yes. moving measurement start/stop in bu27034_get_lux() makes this much 
nicer. I wonder how I didn't see that myself - thanks.

> > +
> > +	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().

> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to access sensor\n");
> > +
> > +	part_id &= BU27034_MASK_PART_ID;
>
> FIELD_GET() even when it's lower bits as then there is no need for
> a reviewer to confirm that it is the lower bits.
> Then you can just do
>
> 	if (FIELD_GET(BU27034_MASK_PART_ID, reg) != BU27034_ID)
>
> > +
> > +	if (part_id != BU27034_ID)
> > +		dev_warn(dev, "unsupported device 0x%x\n", part_id);
>
> I'd adjust that to "unknown device" or "unrecognised device" as it might
> well be supported just fine based on the compatible fallback, we just have
> no way of knowing if it is.

Hmm. Won't promise but maybe I have the time to finish v3 tonight. We 
can then continue discussions towards the v4 :)

Yours,
	-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~