Messages in this thread | | | Date | Fri, 27 Oct 2023 13:49:07 +0300 | From | Matti Vaittinen <> | 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@tweaklogic.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@tweaklogic.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! ~~
| |