Re: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor

[Matti Vaittinen]

On 10/28/23 18:20, Jonathan Cameron wrote:
> On Fri, 27 Oct 2023 18:15:45 +1030
> Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote:
>
> > Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor with als
> > and clear channels with i2c interface. Hardware interrupt configuration is
> > optional. It is a low power device with 20 bit resolution and has
> > configurable adaptive interrupt mode and interrupt persistence mode.
> > The device also features inbuilt hardware gain, multiple integration time
> > selection options and sampling frequency selection options.
>
> Hi Subhajit,
>
>
> >
> Change log below the ---
>
> We don't generally want to end up with this information in the git log
> and anything above the --- is used for the commit message.
>
> Note that if you want to keep notes in your local git it is fine adding
>
> Signed-of-by...
>
> ---
>
> Version notes
> etc
>
>
> As then git am will drop them anyway when your patches are picked up.
>
>
> > v1 -> v2
> > - Renamed probe_new to probe
> > - Removed module id table
> >
> > v0 -> v1
> > - Fixed errors as per previous review
> > - Longer commit messages and descriptions
> > - Updated scale calculations as per iio gts scheme to export proper scale
> >    values and tables to userspace
> > - Removed processed attribute for the same channel for which raw is
> >    provided, instead, exporting proper scale and scale table to userspace so
> >    that userspace can do "(raw + offset) * scale" and derive Lux values
> > - Fixed IIO attribute range syntax
> > - Keeping the regmap lock enabled as the driver uses unlocked regfield
> >    accesses from interrupt handler
> > - Several levels of cleanups by placing guard mutexes in proper places and
> >    returning immediately in case of an error
> > - Using iio_device_claim_direct_mode() during raw reads so that
> >    configurations could not be changed during an adc conversion period
> > - In case of a powerdown error, returning immediately
> > - Removing the definition of direction of the hardware interrupt and
> >    leaving it on to device tree
> > - Adding the powerdown callback after doing device initialization
> > - Removed the regcache_cache_only() implementation
> >
> > Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>

...

> > +static int apds9306_scale_set(struct apds9306_data *data, int val, int val2)
> > +{
> > +	int i, ret, time_sel, gain_sel;
> > +
> > +	/* Rounding up the last digit by one, otherwise matching table fails! */
>
> Interesting.  Sounds like a question for Matti?

Sounds odd indeed. I assume this happens when scale setting is requested 
using one of the exact values advertised by the available scales from 
the GTS? This does not feel right and the +1 does not ring a bell to me. 
I need to investigate what's going on. It would help if you could 
provide the values used as val and val2 for the setting.

This will take a while from me though - I'll try to get to this next 
week. Thanks for pointing out the anomaly!

> > +	if (val2 % 10)
> > +		val2 += 1;
> > +
> > +	ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> > +				     data->intg_time_idx, val, val2, &gain_sel);
> > +	if (ret) {
> > +		for (i = 0; i < data->gts.num_itime; i++) {
> > +			time_sel = data->gts.itime_table[i].sel;
> > +
> > +			if (time_sel == data->intg_time_idx)
> > +				continue;
> > +
> > +			ret = iio_gts_find_gain_sel_for_scale_using_time(&data->gts,
> > +						time_sel, val, val2, &gain_sel);
> > +			if (!ret)
> > +				break;
> > +		}
> > +		if (ret)
> > +			return -EINVAL;
> > +
> > +		ret = apds9306_intg_time_set_hw(data, time_sel);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return apds9306_gain_set_hw(data, gain_sel);
> > +}

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

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