Date:   Fri, 27 Oct 2023 13:49:07 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Subhajit Ghosh <subhajit.ghosh@...aklogic.com>,
        Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Paul Gazzillo <paul@...zz.com>
Cc:     Matt Ranostay <matt@...ostay.sg>,
        Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@...s.com>,
        linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] iio: light: Add support for APDS9306 Light Sensor

Hi Subhajit,

It's nice to see the GTS helpers are used (and hopefully helpful). I 
didn't have the time to go through everything with full focus - so 
please just tell me if some of my questions are silly :) The comments I 
marked as 'nit' aren't really important - feel free to use your 
judgement on them :)

On 10/26/23 17:35, Subhajit Ghosh 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.
> 
> 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@...aklogic.com>
> ---
>   drivers/iio/light/Kconfig    |   12 +
>   drivers/iio/light/Makefile   |    1 +
>   drivers/iio/light/apds9306.c | 1334 ++++++++++++++++++++++++++++++++++
>   3 files changed, 1347 insertions(+)
>   create mode 100644 drivers/iio/light/apds9306.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 45edba797e4c..04e7d10f1470 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -73,6 +73,18 @@ config APDS9300
>   	  To compile this driver as a module, choose M here: the
>   	  module will be called apds9300.
>   
> +config APDS9306
> +	tristate "Avago APDS9306 Ambient Light Sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	select IIO_GTS_HELPER
> +	help
> +	  If you say Y or M here, you get support for Avago APDS9306
> +	  Ambient Light Sensor.
> +
> +	  If built as a dynamically linked module, it will be called
> +	  apds9306.
> +
>   config APDS9960
>   	tristate "Avago APDS9960 gesture/RGB/ALS/proximity sensor"
>   	select REGMAP_I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c0db4c4c36ec..ab94eac04db0 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_ADUX1020)		+= adux1020.o
>   obj-$(CONFIG_AL3010)		+= al3010.o
>   obj-$(CONFIG_AL3320A)		+= al3320a.o
>   obj-$(CONFIG_APDS9300)		+= apds9300.o
> +obj-$(CONFIG_APDS9306)		+= apds9306.o
>   obj-$(CONFIG_APDS9960)		+= apds9960.o
>   obj-$(CONFIG_AS73211)		+= as73211.o
>   obj-$(CONFIG_BH1750)		+= bh1750.o
> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> new file mode 100644
> index 000000000000..352893913a29
> --- /dev/null
> +++ b/drivers/iio/light/apds9306.c
> @@ -0,0 +1,1334 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
> + * I2C Address: 0x52
> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> + *
> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@...aklogic.com>
> + */
> +

...

> +
> +/**
> + * struct part_id_gts_multiplier - Part no. & corresponding gts multiplier
> + * @part_id: Part ID of the device
> + * @max_scale_int: Multiplier for iio_init_iio_gts()
> + * @max_scale_nano: Multiplier for iio_init_iio_gts()
> + */
> +struct part_id_gts_multiplier {
> +	int part_id;
> +	int max_scale_int;
> +	int max_scale_nano;
> +};
> +
> +/*
> + * As per the datasheet, at HW Gain = 3x, Integration time 100mS (32x),
> + * typical 2000 ADC counts are observed for 49.8 uW per sq cm (340.134 lux)
> + * for apds9306 and 43 uW per sq cm (293.69 lux) for apds9306-065.
> + * Assuming lux per count is linear across all integration time ranges.

I love this comment. Still, even with this I managed to get confused :) 
It might be beneficial to mention that the minimum gain is 1x from both 
the integration time and gain. (Not mandatory, I was just trying to 
figure out why this was so difficult for me to follow).

> + * Lux = (raw + offset) * scale; offset can be any value by userspace.
> + * HG = Hardware Gain; ITG = Gain by changing integration time.
> + * Scale table by IIO GTS Helpers = (1 / HG) * (1 / ITG) * Multiplier.
> + *
> + * The Lux values provided in the datasheet are at ITG=32x and HG=3x,
> + * at typical 2000 count.
> + *
> + * Lux per ADC count at 3x and 32x for apds9306 = 340.134 / 2000
> + * Lux per ADC count at 3x and 32x for apds9306-065 = 293.69 / 2000
> + *
> + * The Multiplier for the scale table provided to userspace:
> + * IIO GTS scale Multiplier for apds9306 = (340.134 / 2000) * 32 * 3

'nit'
Could you please show also the result of the computation (16.326432)...

> + * IIO GTS scale Multiplier for apds9306-065 = (293.69 / 2000) * 32 * 3
> + */
> +static struct part_id_gts_multiplier apds9306_gts_mul[] = {
> +	{
> +		.part_id = 0xB1,
> +		.max_scale_int = 16,
> +		.max_scale_nano = 3264320,

'nit'
... to make it easier to see what we have here corresponds to the values 
in comment above.

> +	}, {
> +		.part_id = 0xB3,
> +		.max_scale_int = 14,
> +		.max_scale_nano = 9712000,
> +	},
> +};
> +
> +/**
> + * apds9306_repeat_rate_freq - Sampling Frequency in uHz
> + */
> +static const int apds9306_repeat_rate_freq[][2] = {
> +	{40, 0},
> +	{20, 0},
> +	{10, 0},
> +	{5,  0},
> +	{2,  0},
> +	{1,  0},
> +	{0, 500000},
> +};
> +
> +/**
> + * apds9306_repeat_rate_period - Sampling period in uSec
> + */
> +static const int apds9306_repeat_rate_period[] = {
> +	25000, 50000, 100000, 200000, 500000, 1000000, 2000000
> +};
> +static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
> +	      ARRAY_SIZE(apds9306_repeat_rate_period));
> +
> +/**
> + * struct apds9306_data - apds9306 private data and registers definitions
> + *
> + * All regfield definitions are named exactly according to datasheet for easy
> + * search
> + *
> + * @dev:	Pointer to the device structure
> + * @gts:	IIO Gain Time Scale structure
> + * @mutex:	Lock for protecting register access, adc reads and power
> + * @regmap:	Regmap structure pointer
> + * @regfield_sw_reset:	Reg: MAIN_CTRL, Field: SW_Reset
> + * @regfield_en:	Reg: MAIN_CTRL, Field: ALS_EN
> + * @regfield_intg_time:	Reg: ALS_MEAS_RATE, Field: ALS Resolution/Bit Width
> + * @regfield_repeat_rate:	Reg: ALS_MEAS_RATE, Field: ALS Measurement Rate
> + * @regfield_scale:	Reg: ALS_GAIN, Field: ALS Gain Range
> + * @regfield_int_src:	Reg: INT_CFG, Field: ALS Interrupt Source
> + * @regfield_int_thresh_var_en:	Reg: INT_CFG, Field: ALS Var Interrupt Mode
> + * @regfield_int_en:	Reg: INT_CFG, Field: ALS Interrupt Enable
> + * @regfield_int_persist_val:	Reg: INT_PERSISTENCE, Field: ALS_PERSIST
> + * @regfield_int_thresh_var_val:	Reg: ALS_THRSH_VAR, Field: ALS_THRES_VAR
> + * @nlux_per_count:	nano lux per ADC count for a particular model
> + * @read_data_available:	Flag set by IRQ handler for ADC data available
> + * @intg_time_idx:	Array index for integration times
> + * @repeat_rate_idx:	Array index for sampling frequency
> + * @gain_idx:	Array index for gain
> + * @int_ch:	Currently selected Interrupt channel
> + */
> +struct apds9306_data {
> +	struct device *dev;
> +	struct iio_gts gts;
> +	/*
> +	 * Protects device settings changes where some calculations are required
> +	 * before or after setting or getting the raw settings values from regmap
> +	 * writes or reads respectively.
> +	 */
> +	struct mutex mutex;
> +
> +	struct regmap *regmap;
> +	struct regmap_field *regfield_sw_reset;
> +	struct regmap_field *regfield_en;
> +	struct regmap_field *regfield_intg_time;
> +	struct regmap_field *regfield_repeat_rate;
> +	struct regmap_field *regfield_scale;
> +	struct regmap_field *regfield_int_src;
> +	struct regmap_field *regfield_int_thresh_var_en;
> +	struct regmap_field *regfield_int_en;
> +	struct regmap_field *regfield_int_persist_val;
> +	struct regmap_field *regfield_int_thresh_var_val;
> +
> +	int nlux_per_count;
> +	int read_data_available;
> +	u8 intg_time_idx;
> +	u8 repeat_rate_idx;
> +	u8 gain_idx;

'nit'
I'm not sure caching the time and gain idx in the driver data is that 
beneficial? I assume you use regmap cache amyways. For me caching these 
add a bit of complexity when trying to ensure they are not used 
'uninitialized' for not that obvious benefit.

> +	u8 int_ch;
> +};
> +
> +/*
> + * Available scales with gain 1x - 18x, timings 3.125, 25, 50, 100, 200,
> + * 400 mS
> + * Time impacts to gain: 1x, 8x, 16x, 32x, 64x, 128x > + */
> +
> +#define APDS9306_GSEL_1X	0x00
> +#define APDS9306_GSEL_3X	0x01
> +#define APDS9306_GSEL_6X	0x02
> +#define APDS9306_GSEL_9X	0x03
> +#define APDS9306_GSEL_18X	0x04
> +
> +static const struct iio_gain_sel_pair apds9306_gains[] = {
> +	GAIN_SCALE_GAIN(1, APDS9306_GSEL_1X),
> +	GAIN_SCALE_GAIN(3, APDS9306_GSEL_3X),
> +	GAIN_SCALE_GAIN(6, APDS9306_GSEL_6X),
> +	GAIN_SCALE_GAIN(9, APDS9306_GSEL_9X),
> +	GAIN_SCALE_GAIN(18, APDS9306_GSEL_18X),
> +};
> +
> +#define APDS9306_MEAS_MODE_400MS	0x00
> +#define APDS9306_MEAS_MODE_200MS	0x01
> +#define APDS9306_MEAS_MODE_100MS	0x02
> +#define APDS9306_MEAS_MODE_50MS		0x03
> +#define APDS9306_MEAS_MODE_25MS		0x04
> +#define APDS9306_MEAS_MODE_3125US	0x05
> +
> +static const struct iio_itime_sel_mul apds9306_itimes[] = {
> +	GAIN_SCALE_ITIME_US(400000, APDS9306_MEAS_MODE_400MS, 128),
> +	GAIN_SCALE_ITIME_US(200000, APDS9306_MEAS_MODE_200MS, 64),
> +	GAIN_SCALE_ITIME_US(100000, APDS9306_MEAS_MODE_100MS, 32),
> +	GAIN_SCALE_ITIME_US(50000, APDS9306_MEAS_MODE_50MS, 16),
> +	GAIN_SCALE_ITIME_US(25000, APDS9306_MEAS_MODE_25MS, 8),
> +	GAIN_SCALE_ITIME_US(3125, APDS9306_MEAS_MODE_3125US, 1),
> +};
> +

...

> +
> +static const struct regmap_config apds9306_regmap = {
> +	.name = "apds9306_regmap",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.rd_table = &apds9306_readable_table,
> +	.wr_table = &apds9306_writable_table,
> +	.volatile_table = &apds9306_volatile_table,
> +	.precious_table = &apds9306_precious_table,
> +	.max_register = APDS9306_ALS_THRES_VAR,
> +	.cache_type = REGCACHE_RBTREE,
> +	/*
> +	 * Leaving the regmap lock enabled as regfield accesses are everywhere
> +	 * which are read modify writes and data mutex is not used in the
> +	 * interrupt handler.
> +	 */

To my eye this comment looks a bit misplaced without the
	.disable_locking = false,
- which is a no-op here. I think what you wrote in the comment is true 
(default assumption) for many drivers - maybe the comment is not needed?

> +};

...

> +static int apds9306_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct apds9306_data *data = iio_priv(indio_dev);
> +	int ret, reg;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (chan->channel2 == IIO_MOD_LIGHT_CLEAR)
> +			reg = APDS9306_CLEAR_DATA_0;
> +		else
> +			reg = APDS9306_ALS_DATA_0;
> +		/*
> +		 * Changing device parameters during adc operation, resets
> +		 * the ADC which has to avoided.
> +		 */

Would you need to grab the mutex here? I think you want also prevent 
changing gain/time during the computations.

> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +		ret = apds9306_read_data(data, val, reg);
> +		iio_device_release_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = apds9306_intg_time_get(data, val2);
> +		if (ret)
> +			return ret;
> +		*val = 0;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = apds9306_sampling_freq_get(data, val, val2);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = apds9306_scale_get(data, val, val2);
> +		if (ret)
> +			return ret;
> +		return IIO_VAL_INT_PLUS_NANO;
> +	default:
> +		return -EINVAL;
> +	}
> +};
> +

Thanks for the nice driver!

Yours,
	-- Matti


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

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

Powered by blists - more mailing lists