Re: [PATCH v2] iio: light: lv0104cs: Add support for LV0104CS light sensor

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 24 Feb 2018 20:38:08 -0600
Jeff LaBundy <jeff@labundy.com> wrote:

<snip>
> > > +static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
> > > +				int *val, int *val2)
> > > +{
> > > +	u8 regval = LV0104CS_REGVAL_MEASURE;
> > > +	u16 adc_output;
> > > +	int ret;
> > > +
> > > +	regval |= lv0104cs_scales[lv0104cs->scale].regval;
> > > +	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
> > > +	ret = lv0104cs_write_reg(lv0104cs, regval);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* wait for integration time to pass (with margin) */
> > > +	switch (lv0104cs->int_time) {
> > > +	case LV0104CS_INTEG_12_5MS:
> > > +		msleep(50);
> > > +		break;
> > > +
> > > +	case LV0104CS_INTEG_100MS:
> > > +		msleep(150);
> > > +		break;
> > > +
> > > +	case LV0104CS_INTEG_200MS:
> > > +		msleep(250);
> > > +		break;
> > > +
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/* convert ADC output to lux */
> > > +	switch (lv0104cs->scale) {
> > > +	case LV0104CS_SCALE_0_25X:  
> > Hmm.  Given how simple the scale application is and the fact
> > that we aren't trying dynamic control (which makes this complex)
> > we 'could' go for the IIO default option of providing the raw
> > value and letting userspace deal with the calibration.
> > 
> > However, this is a slow device (fairly anyway) so the
> > cost of conversion is trivial and we are unlikely to ever want
> > to bother with a buffered interface on this I think things
> > are fine as you have them.  
> 
> Sure thing, I'll leave this as-is. The reason for having chosen
> PROCESSED is because the device's ADC output already includes the
> effects of CALIBSCALE, so the device technically does not offer
> a RAW output on its own. Therefore I have presented a PROCESSED
> value that includes the SCALE that was in place when the measurement
> was captured.
It's normal for a device to already include the effect of a
calibscale as they are often done by tweaking a DAC in the
analog section of the sensor circuit.  So don't let that
argue against providing a _RAW value.

The answer is still fine though, just thought I'd comment on the
reasoning ;)

<snip>