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.
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 <[email protected]>
---
drivers/iio/light/Kconfig | 12 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/apds9306.c | 1326 ++++++++++++++++++++++++++++++++++
3 files changed, 1339 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..ae79d3b1129d
--- /dev/null
+++ b/drivers/iio/light/apds9306.c
@@ -0,0 +1,1326 @@
+// 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 <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/events.h>
+#include <linux/iio/sysfs.h>
+
+#include <asm/unaligned.h>
+
+#define APDS9306_MAIN_CTRL 0x00
+#define APDS9306_ALS_MEAS_RATE 0x04
+#define APDS9306_ALS_GAIN 0x05
+#define APDS9306_PART_ID 0x06
+#define APDS9306_MAIN_STATUS 0x07
+#define APDS9306_CLEAR_DATA_0 0x0A
+#define APDS9306_CLEAR_DATA_1 0x0B
+#define APDS9306_CLEAR_DATA_2 0x0C
+#define APDS9306_ALS_DATA_0 0x0D
+#define APDS9306_ALS_DATA_1 0x0E
+#define APDS9306_ALS_DATA_2 0x0F
+#define APDS9306_INT_CFG 0x19
+#define APDS9306_INT_PERSISTENCE 0x1A
+#define APDS9306_ALS_THRES_UP_0 0x21
+#define APDS9306_ALS_THRES_UP_1 0x22
+#define APDS9306_ALS_THRES_UP_2 0x23
+#define APDS9306_ALS_THRES_LOW_0 0x24
+#define APDS9306_ALS_THRES_LOW_1 0x25
+#define APDS9306_ALS_THRES_LOW_2 0x26
+#define APDS9306_ALS_THRES_VAR 0x27
+
+#define APDS9306_ALS_INT_STAT_MASK BIT(4)
+#define APDS9306_ALS_DATA_STAT_MASK BIT(3)
+
+#define APDS9306_ALS_THRES_VAL_MAX 0xFFFFF
+#define APDS9306_ALS_THRES_VAR_VAL_MAX 7
+#define APDS9306_ALS_PERSIST_VAL_MAX 15
+#define APDS9306_ALS_READ_DATA_DELAY_US 20000
+
+enum apds9306_power_states {
+ STANDBY,
+ ACTIVE,
+};
+
+/**
+ * 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.
+ *
+ * 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
+ * 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,
+ }, {
+ .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;
+ 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 struct iio_event_spec apds9306_event_spec_als[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
+ }, {
+ .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
+ .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
+ BIT(IIO_EV_INFO_ENABLE),
+ }, {
+ .type = IIO_EV_TYPE_THRESH,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+static struct iio_event_spec apds9306_event_spec_clear[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .mask_separate = BIT(IIO_EV_INFO_ENABLE),
+ },
+};
+
+#define APDS9306_CHANNEL(_type) \
+ .type = _type, \
+ .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
+ BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+ .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
+ BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
+
+static struct iio_chan_spec apds9306_channels_with_events[] = {
+ {
+ APDS9306_CHANNEL(IIO_LIGHT)
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .event_spec = apds9306_event_spec_als,
+ .num_event_specs = ARRAY_SIZE(apds9306_event_spec_als),
+ }, {
+ APDS9306_CHANNEL(IIO_INTENSITY)
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .modified = 1,
+ .event_spec = apds9306_event_spec_clear,
+ .num_event_specs = ARRAY_SIZE(apds9306_event_spec_clear),
+ },
+};
+
+static struct iio_chan_spec apds9306_channels_without_events[] = {
+ {
+ APDS9306_CHANNEL(IIO_LIGHT)
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ }, {
+ APDS9306_CHANNEL(IIO_INTENSITY)
+ .channel2 = IIO_MOD_LIGHT_CLEAR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .modified = 1,
+ },
+};
+
+/* INT_PERSISTENCE available */
+IIO_CONST_ATTR(thresh_either_period_available, "[0 1 15]");
+
+/* ALS_THRESH_VAR available */
+IIO_CONST_ATTR(thresh_adaptive_either_values_available, "[0 1 7]");
+
+static struct attribute *apds9306_event_attributes[] = {
+ &iio_const_attr_thresh_either_period_available.dev_attr.attr,
+ &iio_const_attr_thresh_adaptive_either_values_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group apds9306_event_attr_group = {
+ .attrs = apds9306_event_attributes,
+};
+
+static const struct regmap_range apds9306_readable_ranges[] = {
+ regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_THRES_VAR)
+};
+
+static const struct regmap_range apds9306_writable_ranges[] = {
+ regmap_reg_range(APDS9306_MAIN_CTRL, APDS9306_ALS_GAIN),
+ regmap_reg_range(APDS9306_INT_CFG, APDS9306_ALS_THRES_VAR)
+};
+
+static const struct regmap_range apds9306_volatile_ranges[] = {
+ regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS),
+ regmap_reg_range(APDS9306_CLEAR_DATA_0, APDS9306_ALS_DATA_2)
+};
+
+static const struct regmap_range apds9306_precious_ranges[] = {
+ regmap_reg_range(APDS9306_MAIN_STATUS, APDS9306_MAIN_STATUS)
+};
+
+static const struct regmap_access_table apds9306_readable_table = {
+ .yes_ranges = apds9306_readable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(apds9306_readable_ranges)
+};
+
+static const struct regmap_access_table apds9306_writable_table = {
+ .yes_ranges = apds9306_writable_ranges,
+ .n_yes_ranges = ARRAY_SIZE(apds9306_writable_ranges)
+};
+
+static const struct regmap_access_table apds9306_volatile_table = {
+ .yes_ranges = apds9306_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(apds9306_volatile_ranges)
+};
+
+static const struct regmap_access_table apds9306_precious_table = {
+ .yes_ranges = apds9306_precious_ranges,
+ .n_yes_ranges = ARRAY_SIZE(apds9306_precious_ranges)
+};
+
+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.
+ */
+};
+
+static const struct reg_field apds9306_regfield_sw_reset =
+ REG_FIELD(APDS9306_MAIN_CTRL, 4, 4);
+
+static const struct reg_field apds9306_regfield_en =
+ REG_FIELD(APDS9306_MAIN_CTRL, 1, 1);
+
+static const struct reg_field apds9306_regfield_intg_time =
+ REG_FIELD(APDS9306_ALS_MEAS_RATE, 4, 6);
+
+static const struct reg_field apds9306_regfield_repeat_rate =
+ REG_FIELD(APDS9306_ALS_MEAS_RATE, 0, 2);
+
+static const struct reg_field apds9306_regfield_scale =
+ REG_FIELD(APDS9306_ALS_GAIN, 0, 2);
+
+static const struct reg_field apds9306_regfield_int_src =
+ REG_FIELD(APDS9306_INT_CFG, 4, 5);
+
+static const struct reg_field apds9306_regfield_int_thresh_var_en =
+ REG_FIELD(APDS9306_INT_CFG, 3, 3);
+
+static const struct reg_field apds9306_regfield_int_en =
+ REG_FIELD(APDS9306_INT_CFG, 2, 2);
+
+static const struct reg_field apds9306_regfield_int_persist_val =
+ REG_FIELD(APDS9306_INT_PERSISTENCE, 4, 7);
+
+static const struct reg_field apds9306_regfield_int_thresh_var_val =
+ REG_FIELD(APDS9306_ALS_THRES_VAR, 0, 2);
+
+static int apds9306_regfield_init(struct apds9306_data *data)
+{
+ struct device *dev = data->dev;
+ struct regmap *regmap = data->regmap;
+
+ data->regfield_sw_reset = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_sw_reset);
+ if (IS_ERR(data->regfield_sw_reset))
+ return PTR_ERR(data->regfield_sw_reset);
+
+ data->regfield_en = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_en);
+ if (IS_ERR(data->regfield_en))
+ return PTR_ERR(data->regfield_en);
+
+ data->regfield_intg_time = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_intg_time);
+ if (IS_ERR(data->regfield_intg_time))
+ return PTR_ERR(data->regfield_intg_time);
+
+ data->regfield_repeat_rate = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_repeat_rate);
+ if (IS_ERR(data->regfield_repeat_rate))
+ return PTR_ERR(data->regfield_repeat_rate);
+
+ data->regfield_scale = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_scale);
+ if (IS_ERR(data->regfield_scale))
+ return PTR_ERR(data->regfield_scale);
+
+ data->regfield_int_src = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_int_src);
+ if (IS_ERR(data->regfield_int_src))
+ return PTR_ERR(data->regfield_int_src);
+
+ data->regfield_int_thresh_var_en = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_int_thresh_var_en);
+ if (IS_ERR(data->regfield_int_thresh_var_en))
+ return PTR_ERR(data->regfield_int_thresh_var_en);
+
+ data->regfield_int_en = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_int_en);
+ if (IS_ERR(data->regfield_int_en))
+ return PTR_ERR(data->regfield_int_en);
+
+ data->regfield_int_persist_val = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_int_persist_val);
+ if (IS_ERR(data->regfield_int_persist_val))
+ return PTR_ERR(data->regfield_int_en);
+
+ data->regfield_int_thresh_var_val = devm_regmap_field_alloc(dev, regmap,
+ apds9306_regfield_int_thresh_var_val);
+ if (IS_ERR(data->regfield_int_thresh_var_val))
+ return PTR_ERR(data->regfield_int_thresh_var_en);
+
+ return 0;
+}
+
+static int apds9306_power_state(struct apds9306_data *data,
+ enum apds9306_power_states state)
+{
+ int ret;
+
+ /* Reset not included as it causes ugly I2C bus error */
+ switch (state) {
+ case STANDBY:
+ return regmap_field_write(data->regfield_en, 0);
+ case ACTIVE:
+ ret = regmap_field_write(data->regfield_en, 1);
+ if (ret)
+ return ret;
+ /* 5ms wake up time */
+ usleep_range(5000, 10000);
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int apds9306_runtime_power(struct apds9306_data *data, int en)
+{
+ struct device *dev = data->dev;
+ int ret;
+
+ if (en) {
+ ret = pm_runtime_resume_and_get(dev);
+ if (ret < 0) {
+ dev_err(dev, "runtime resume failed: %d\n", ret);
+ return ret;
+ }
+ return 0;
+ }
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
+ return 0;
+}
+
+static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
+{
+ struct device *dev = data->dev;
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ int ret, delay, intg_time, status = 0;
+ u8 buff[3];
+
+ ret = apds9306_runtime_power(data, 1);
+ if (ret)
+ return ret;
+
+ intg_time = iio_gts_find_int_time_by_sel(&data->gts,
+ data->intg_time_idx);
+ if (intg_time < 0)
+ delay = apds9306_repeat_rate_period[data->repeat_rate_idx];
+
+ /*
+ * Whichever is greater - integration time period or
+ * sampling period.
+ */
+ delay = max(intg_time,
+ apds9306_repeat_rate_period[data->repeat_rate_idx]);
+
+
+ /*
+ * Clear stale data flag that might have been set by the interrupt
+ * handler if it got data available flag set in the status reg.
+ */
+ data->read_data_available = 0;
+
+ /*
+ * If this function runs parallel with the interrupt handler, either
+ * this reads and clears the status registers or the interrupt handler
+ * does. The interrupt handler sets a flag for read data available
+ * in our private structure which we read here.
+ */
+ ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS,
+ status, (status & (APDS9306_ALS_DATA_STAT_MASK |
+ APDS9306_ALS_INT_STAT_MASK)) ||
+ data->read_data_available,
+ APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
+
+ if (ret)
+ return ret;
+
+ /* If we reach here before the interrupt handler we push an event */
+ if ((status & APDS9306_ALS_INT_STAT_MASK)) {
+ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+ data->int_ch,
+ IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
+ iio_get_time_ns(indio_dev));
+ }
+
+ if ((status & APDS9306_ALS_DATA_STAT_MASK) ||
+ data->read_data_available) {
+ ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
+ if (ret) {
+ dev_err(dev, "read data failed\n");
+ return ret;
+ }
+ *val = get_unaligned_le24(&buff);
+ }
+
+ return apds9306_runtime_power(data, 0);
+}
+
+static int apds9306_intg_time_get(struct apds9306_data *data, int *val2)
+{
+ int ret;
+
+ ret = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+ if (ret < 0)
+ return ret;
+
+ *val2 = ret;
+
+ return 0;
+}
+
+static int apds9306_intg_time_set_hw(struct apds9306_data *data, int sel)
+{
+ int ret;
+
+ ret = regmap_field_write(data->regfield_intg_time, sel);
+ if (ret)
+ return ret;
+
+ data->intg_time_idx = sel;
+
+ return ret;
+}
+
+static int apds9306_gain_set_hw(struct apds9306_data *data, int sel)
+{
+ int ret;
+
+ ret = regmap_field_write(data->regfield_scale, sel);
+ if (ret)
+ return ret;
+
+ data->gain_idx = sel;
+
+ return ret;
+}
+
+static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
+{
+ struct device *dev = data->dev;
+ int ret, intg_old, gain_old, gain_new, gain_new_closest;
+ bool ok;
+
+ if (!iio_gts_valid_time(&data->gts, val2)) {
+ dev_err(dev, "Unsupported integration time %u\n", val2);
+ return ret;
+ }
+
+ intg_old = iio_gts_find_int_time_by_sel(&data->gts,
+ data->intg_time_idx);
+ if (ret < 0)
+ return ret;
+
+ if (intg_old == val2)
+ return 0;
+
+ gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
+ if (gain_old < 0)
+ return gain_old;
+
+ ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
+ intg_old, val2, &gain_new);
+ if (gain_new < 0) {
+ dev_err(dev, "Unsupported gain with time\n");
+ return gain_new;
+ }
+
+ gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
+ if (gain_new_closest < 0) {
+ gain_new_closest = iio_gts_get_min_gain(&data->gts);
+ if (gain_new_closest < 0)
+ return gain_new_closest < 0;
+ }
+ if (!ok)
+ dev_dbg(dev, "Unable to find optimum gain, setting minimum");
+
+ ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
+ if (ret < 0)
+ return ret;
+
+ ret = apds9306_intg_time_set_hw(data, ret);
+ if (ret)
+ return ret;
+
+ ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
+ if (ret < 0)
+ return ret;
+
+ return apds9306_gain_set_hw(data, ret);
+}
+
+static int apds9306_sampling_freq_get(struct apds9306_data *data, int *val,
+ int *val2)
+{
+ if (data->repeat_rate_idx > ARRAY_SIZE(apds9306_repeat_rate_freq))
+ return -EINVAL;
+
+ *val = apds9306_repeat_rate_freq[data->repeat_rate_idx][0];
+ *val2 = apds9306_repeat_rate_freq[data->repeat_rate_idx][1];
+
+ return 0;
+}
+
+static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
+ int val2)
+{
+ int i, ret = -EINVAL;
+
+ for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
+ if (apds9306_repeat_rate_freq[i][0] == val &&
+ apds9306_repeat_rate_freq[i][1] == val2) {
+ ret = regmap_field_write(data->regfield_repeat_rate, i);
+ if (ret)
+ return ret;
+ data->repeat_rate_idx = i;
+ return ret;
+ }
+
+ return ret;
+}
+
+static int apds9306_scale_get(struct apds9306_data *data, int *val, int *val2)
+{
+ int gain, intg;
+
+ gain = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
+ if (gain < 0)
+ return gain;
+
+ intg = iio_gts_find_int_time_by_sel(&data->gts, data->intg_time_idx);
+ if (intg < 0)
+ return intg;
+
+ return iio_gts_get_scale(&data->gts, gain, intg, val, val2);
+}
+
+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! */
+ 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);
+}
+
+static int apds9306_event_period_get(struct apds9306_data *data, int *val)
+{
+ int period, ret;
+
+ ret = regmap_field_read(data->regfield_int_persist_val, &period);
+ if (ret)
+ return ret;
+
+ if (period > APDS9306_ALS_PERSIST_VAL_MAX)
+ return -EINVAL;
+
+ *val = period;
+
+ return ret;
+}
+
+static int apds9306_event_period_set(struct apds9306_data *data, int val)
+{
+ if (val < 0 || val > APDS9306_ALS_PERSIST_VAL_MAX)
+ return -EINVAL;
+
+ return regmap_field_write(data->regfield_int_persist_val, val);
+}
+
+static int apds9306_event_thresh_get(struct apds9306_data *data, int dir,
+ int *val)
+{
+ int var, ret;
+ u8 buff[3];
+
+ if (dir == IIO_EV_DIR_RISING)
+ var = APDS9306_ALS_THRES_UP_0;
+ else if (dir == IIO_EV_DIR_FALLING)
+ var = APDS9306_ALS_THRES_LOW_0;
+ else
+ return -EINVAL;
+
+ ret = regmap_bulk_read(data->regmap, var, buff, sizeof(buff));
+ if (ret)
+ return ret;
+ *val = get_unaligned_le24(&buff);
+ return 0;
+}
+
+static int apds9306_event_thresh_set(struct apds9306_data *data, int dir,
+ int val)
+{
+ int var;
+ u8 buff[3];
+
+ if (dir == IIO_EV_DIR_RISING)
+ var = APDS9306_ALS_THRES_UP_0;
+ else if (dir == IIO_EV_DIR_FALLING)
+ var = APDS9306_ALS_THRES_LOW_0;
+ else
+ return -EINVAL;
+
+ if (val < 0 || val > APDS9306_ALS_THRES_VAL_MAX)
+ return -EINVAL;
+
+ put_unaligned_le24(val, buff);
+ return regmap_bulk_write(data->regmap, var, buff, sizeof(buff));
+}
+
+static int apds9306_event_thresh_adaptive_get(struct apds9306_data *data,
+ int *val)
+{
+ int thad, ret;
+
+ ret = regmap_field_read(data->regfield_int_thresh_var_val, &thad);
+ if (ret)
+ return ret;
+
+ if (thad > APDS9306_ALS_THRES_VAR_VAL_MAX)
+ return -EINVAL;
+
+ *val = thad;
+
+ return ret;
+}
+
+static int apds9306_event_thresh_adaptive_set(struct apds9306_data *data,
+ int val)
+{
+ if (val < 0 || val > APDS9306_ALS_THRES_VAR_VAL_MAX)
+ return -EINVAL;
+
+ return regmap_field_write(data->regfield_int_thresh_var_val, val);
+}
+
+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.
+ */
+ 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;
+ }
+};
+
+static int apds9306_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type, int *length,
+ long mask)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ return iio_gts_avail_times(&data->gts, vals, type, length);
+ case IIO_CHAN_INFO_SCALE:
+ return iio_gts_all_avail_scales(&data->gts, vals, type,
+ length);
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ *length = ARRAY_SIZE(apds9306_repeat_rate_freq) * 2;
+ *vals = (const int *)apds9306_repeat_rate_freq;
+ *type = IIO_VAL_INT_PLUS_MICRO;
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int apds9306_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SCALE:
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_CHAN_INFO_INT_TIME:
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int apds9306_write_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;
+
+ mutex_lock(&data->mutex);
+ switch (mask) {
+ case IIO_CHAN_INFO_INT_TIME:
+ if (!val)
+ ret = apds9306_intg_time_set(data, val2);
+ else
+ ret = -EINVAL;
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ ret = apds9306_scale_set(data, val, val2);
+ break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = apds9306_sampling_freq_set(data, val, val2);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ mutex_unlock(&data->mutex);
+
+ return ret;
+}
+
+static irqreturn_t apds9306_irq_handler(int irq, void *priv)
+{
+ struct iio_dev *indio_dev = priv;
+ struct apds9306_data *data = iio_priv(indio_dev);
+ int ret, status;
+
+ /*
+ * The interrupt line is released and the interrupt flag is
+ * cleared as a result of reading the status register. All the
+ * status flags are cleared as a result of this read.
+ */
+ ret = regmap_read(data->regmap, APDS9306_MAIN_STATUS, &status);
+ if (ret < 0) {
+ dev_err(data->dev, "status reg read failed\n");
+ return IRQ_HANDLED;
+ }
+
+ if ((status & APDS9306_ALS_INT_STAT_MASK)) {
+ iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+ data->int_ch, IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_EITHER), iio_get_time_ns(indio_dev));
+ }
+
+ /*
+ * If a one-shot read through sysfs is underway at the same time
+ * as this interrupt handler is executing and a read data available
+ * flag was set, this flag is set to inform read_poll_timeout()
+ * to exit.
+ */
+ if ((status & APDS9306_ALS_DATA_STAT_MASK))
+ data->read_data_available = 1;
+
+ return IRQ_HANDLED;
+}
+
+static int apds9306_read_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ mutex_lock(&data->mutex);
+ if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+ ret = apds9306_event_period_get(data, val);
+ else
+ ret = apds9306_event_thresh_get(data, dir, val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+ case IIO_EV_TYPE_THRESH_ADAPTIVE:
+ mutex_lock(&data->mutex);
+ ret = apds9306_event_thresh_adaptive_get(data, val);
+ mutex_unlock(&data->mutex);
+ if (ret)
+ return ret;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int apds9306_write_event(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ mutex_lock(&data->mutex);
+ if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+ ret = apds9306_event_period_set(data, val);
+ else
+ ret = apds9306_event_thresh_set(data, dir, val);
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_TYPE_THRESH_ADAPTIVE:
+ mutex_lock(&data->mutex);
+ ret = apds9306_event_thresh_adaptive_set(data, val);
+ mutex_unlock(&data->mutex);
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int apds9306_read_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+ unsigned int int_en;
+ int ret;
+
+ mutex_lock(&data->mutex);
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ ret = regmap_field_read(data->regfield_int_en, &int_en);
+ if (ret)
+ break;
+ if (chan->type == IIO_LIGHT)
+ ret = int_en & data->int_ch;
+ else if (chan->type == IIO_INTENSITY)
+ ret = int_en & !data->int_ch;
+ else
+ ret = -EINVAL;
+ break;
+ case IIO_EV_TYPE_THRESH_ADAPTIVE:
+ ret = regmap_field_read(data->regfield_int_thresh_var_en,
+ &int_en);
+ if (ret)
+ break;
+ ret = int_en;
+ break;
+ default:
+ ret = -EINVAL;
+ }
+ mutex_unlock(&data->mutex);
+ return ret;
+}
+
+static int apds9306_write_event_config(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ int state)
+{
+ struct apds9306_data *data = iio_priv(indio_dev);
+ int ret, int_sel;
+
+ state = !!state;
+ switch (type) {
+ case IIO_EV_TYPE_THRESH:
+ /*
+ * If interrupt is enabled, the channel is set before enabling
+ * the interrupt. In case of disable, no need to switch
+ * channels. In case of different channel is selected while
+ * interrupt in on, just changing the channel.
+ */
+ if (state) {
+ if (chan->type == IIO_LIGHT)
+ int_sel = 1;
+ else if (chan->type == IIO_INTENSITY)
+ int_sel = 0;
+ else
+ return -EINVAL;
+
+ ret = regmap_field_write(data->regfield_int_src,
+ int_sel);
+ if (ret)
+ return ret;
+ data->int_ch = int_sel;
+ }
+ ret = regmap_field_read(data->regfield_int_en, &int_sel);
+ if (ret)
+ return ret;
+ if (int_sel == state)
+ return 0;
+ mutex_lock(&data->mutex);
+ ret = regmap_field_write(data->regfield_int_en, state);
+ if (!ret)
+ ret = apds9306_runtime_power(data, state);
+ mutex_unlock(&data->mutex);
+ return ret;
+ case IIO_EV_TYPE_THRESH_ADAPTIVE:
+ return regmap_field_write(data->regfield_int_thresh_var_en,
+ state);
+ default:
+ return -EINVAL;
+ }
+}
+
+#define APDS9306_IIO_INFO \
+ .read_avail = apds9306_read_avail, \
+ .read_raw = apds9306_read_raw, \
+ .write_raw = apds9306_write_raw, \
+ .write_raw_get_fmt = apds9306_write_raw_get_fmt,
+
+static const struct iio_info apds9306_info_no_events = {
+ APDS9306_IIO_INFO
+};
+
+static const struct iio_info apds9306_info = {
+ APDS9306_IIO_INFO
+ .event_attrs = &apds9306_event_attr_group,
+ .read_event_value = apds9306_read_event,
+ .write_event_value = apds9306_write_event,
+ .read_event_config = apds9306_read_event_config,
+ .write_event_config = apds9306_write_event_config,
+};
+
+static int apds9306_init_iio_gts(struct apds9306_data *data)
+{
+ int i, ret, part_id;
+
+ ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < ARRAY_SIZE(apds9306_gts_mul); i++) {
+ if (part_id == apds9306_gts_mul[i].part_id)
+ return devm_iio_init_iio_gts(data->dev,
+ apds9306_gts_mul[i].max_scale_int,
+ apds9306_gts_mul[i].max_scale_nano,
+ apds9306_gains, ARRAY_SIZE(apds9306_gains),
+ apds9306_itimes, ARRAY_SIZE(apds9306_itimes),
+ &data->gts);
+ }
+
+ return -ENXIO;
+}
+
+static void apds9306_powerdown(void *ptr)
+{
+ struct apds9306_data *data = (struct apds9306_data *)ptr;
+ int ret;
+
+ /* Disable interrupts */
+ ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
+ if (ret)
+ return;
+ ret = regmap_field_write(data->regfield_int_en, 0);
+ if (ret)
+ return;
+ /* Put the device in standby mode */
+ apds9306_power_state(data, STANDBY);
+}
+
+static int apds9306_init_device(struct apds9306_data *data)
+{
+ struct device *dev = data->dev;
+ int ret;
+
+ ret = apds9306_power_state(data, ACTIVE);
+ if (ret)
+ return ret;
+
+ ret = pm_runtime_set_active(dev);
+ if (ret)
+ return ret;
+
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
+
+ pm_runtime_set_autosuspend_delay(dev, 5000);
+ pm_runtime_use_autosuspend(dev);
+
+ /* Init GTS multiplier values according to Part ID */
+ ret = apds9306_init_iio_gts(data);
+ if (ret)
+ return ret;
+
+ /* Integration time: 100 msec */
+ ret = apds9306_intg_time_set_hw(data, 2);
+ if (ret)
+ return ret;
+
+ /* Sampling frequency: 100 msec, 10 Hz */
+ ret = apds9306_sampling_freq_set(data, 10, 0);
+ if (ret)
+ return ret;
+
+ /* Scale: x3 */
+ ret = apds9306_gain_set_hw(data, 1);
+ if (ret)
+ return ret;
+
+ /* Interrupt source channel: als */
+ ret = regmap_field_write(data->regfield_int_src, 1);
+ if (ret)
+ return ret;
+ data->int_ch = 1;
+
+ /* Interrupts disabled */
+ ret = regmap_field_write(data->regfield_int_en, 0);
+ if (ret)
+ return ret;
+
+ /* Threshold Variance disabled */
+ return regmap_field_write(data->regfield_int_thresh_var_en, 0);
+}
+
+static int apds9306_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct apds9306_data *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+
+ mutex_init(&data->mutex);
+
+ data->regmap = devm_regmap_init_i2c(client, &apds9306_regmap);
+ if (IS_ERR(data->regmap))
+ return dev_err_probe(dev, PTR_ERR(data->regmap),
+ "regmap initialization failed\n");
+
+ data->dev = dev;
+ i2c_set_clientdata(client, indio_dev);
+
+ ret = apds9306_regfield_init(data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "regfield initialization failed\n");
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+ indio_dev->name = "apds9306";
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ if (client->irq) {
+ indio_dev->info = &apds9306_info;
+ indio_dev->channels = apds9306_channels_with_events;
+ indio_dev->num_channels =
+ ARRAY_SIZE(apds9306_channels_with_events);
+ ret = devm_request_threaded_irq(dev, client->irq, NULL,
+ apds9306_irq_handler, IRQF_ONESHOT,
+ "apds9306_event", indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to assign interrupt.\n");
+ } else {
+ indio_dev->info = &apds9306_info_no_events;
+ indio_dev->channels = apds9306_channels_without_events;
+ indio_dev->num_channels =
+ ARRAY_SIZE(apds9306_channels_without_events);
+ }
+
+ ret = apds9306_init_device(data);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to init device\n");
+
+ ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to add action on driver unwind\n");
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static int apds9306_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct apds9306_data *data = iio_priv(indio_dev);
+
+ return apds9306_power_state(data, STANDBY);
+}
+
+static int apds9306_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct apds9306_data *data = iio_priv(indio_dev);
+
+ return apds9306_power_state(data, ACTIVE);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
+ apds9306_runtime_suspend,
+ apds9306_runtime_resume,
+ NULL);
+
+static const struct of_device_id apds9306_of_match[] = {
+ { .compatible = "avago,apds9306" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, apds9306_of_match);
+
+static struct i2c_driver apds9306_driver = {
+ .driver = {
+ .name = "apds9306",
+ .pm = pm_ptr(&apds9306_pm_ops),
+ .of_match_table = apds9306_of_match,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ },
+ .probe = apds9306_probe,
+};
+module_i2c_driver(apds9306_driver);
+
+MODULE_AUTHOR("Subhajit Ghosh <[email protected]>");
+MODULE_DESCRIPTION("APDS9306 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_GTS_HELPER);
--
2.34.1
On 27/10/2023 09:45, 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.
>
> v1 -> v2
> - Renamed probe_new to probe
> - Removed module id table
This is fourth version of this patchset, not second, so v4.
Best regards,
Krzysztof
On 27/10/23 18:43, Krzysztof Kozlowski wrote:
> On 27/10/2023 09:45, 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.
>>
>> v1 -> v2
>> - Renamed probe_new to probe
>> - Removed module id table
>
> This is fourth version of this patchset, not second, so v4.
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
1. This patch set adds support for APDS9306 Light sensor,
2. In v0 of this patch series I was told to combine the avago,apds9300.yaml
and avago,apds9960.yaml as they look similar, then add my apds9306
support into it.
3. I assumed that the squashing operation of apds9300 and apds9960 schemas
are a separate operation and submitted a separate patch to do that, please
follow the link
Link: https://lore.kernel.org/all/20231019-hurry-eagle-0ffa95b1a026@spud/
4. Conor reviewed the patch and said that it would be better that I handle all
these operations in apds9306 driver (this) patch series rather than submitting
a new patch.
"Ahh apologies then. The best course of action would likely be to include
the patch merging the two bindings in your series adding the third user."
5. As per this patch series -- RFC->v0->v1-v2
I have formatted the commit messages wrongly which might be the source of all the
confusion. I'll fix it. Please let me know the best course of action, I am not well
versed with this process. Thank you for reviewing.
Regards,
Subhajit Ghosh
On 27/10/2023 10:42, Subhajit Ghosh wrote:
> 4. Conor reviewed the patch and said that it would be better that I handle all
> these operations in apds9306 driver (this) patch series rather than submitting
> a new patch.
> "Ahh apologies then. The best course of action would likely be to include
> the patch merging the two bindings in your series adding the third user."
> 5. As per this patch series -- RFC->v0->v1-v2
RFC was the first version sent to mailing list. So after RFC there is
second version - v2. This is v4.
>
> I have formatted the commit messages wrongly which might be the source of all the
> confusion. I'll fix it. Please let me know the best course of action, I am not well
> versed with this process. Thank you for reviewing.
Best regards,
Krzysztof
On Fri, Oct 27, 2023 at 06:15:45PM +1030, 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.
Please, do not send so often this big patches (~1.5kLoC!), let reviewers a bit
of time, please.
Since I have my message postponed, I finish review on the v1, and I believe
100% of those comments will be applicable here.
--
With Best Regards,
Andy Shevchenko
> Please, do not send so often this big patches (~1.5kLoC!), let reviewers a bit
> of time, please.
>
> Since I have my message postponed, I finish review on the v1, and I believe
> 100% of those comments will be applicable here.
>
Hi Andy,
v1 broke the kernel build, which was bit embarrassing as I skipped compilation on
latest kernel and only tested on my embedded test bench, so I submitted v2 that early,
sorry about that, I understand, won't happen again. I am not in a hurry either!
Regards,
Subhajit Ghosh
On 27/10/23 21:34, Krzysztof Kozlowski wrote:
> On 27/10/2023 10:42, Subhajit Ghosh wrote:
>> 4. Conor reviewed the patch and said that it would be better that I handle all
>> these operations in apds9306 driver (this) patch series rather than submitting
>> a new patch.
>> "Ahh apologies then. The best course of action would likely be to include
>> the patch merging the two bindings in your series adding the third user."
>> 5. As per this patch series -- RFC->v0->v1-v2
>
> RFC was the first version sent to mailing list. So after RFC there is
> second version - v2. This is v4.
Acknowledging all your other comments. Appreciate your time and effort in reviewing
this. One last question on this - So what version should I use for the patchset
which I will submit next - "v3" or "v5" in the Subject of the emails?
Regards,
Subhajit Ghosh
Hi Subhajit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 611da07b89fdd53f140d7b33013f255bf0ed8f34]
url: https://github.com/intel-lab-lkp/linux/commits/Subhajit-Ghosh/dt-bindings-iio-light-Avago-APDS9306/20231027-154954
base: 611da07b89fdd53f140d7b33013f255bf0ed8f34
patch link: https://lore.kernel.org/r/20231027074545.6055-3-subhajit.ghosh%40tweaklogic.com
patch subject: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20231028/[email protected]/config)
compiler: alpha-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231028/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/iio/light/apds9306.c:110: warning: cannot understand function prototype: 'const int apds9306_repeat_rate_freq[][2] = '
>> drivers/iio/light/apds9306.c:123: warning: cannot understand function prototype: 'const int apds9306_repeat_rate_period[] = '
vim +110 drivers/iio/light/apds9306.c
106
107 /**
108 * apds9306_repeat_rate_freq - Sampling Frequency in uHz
109 */
> 110 static const int apds9306_repeat_rate_freq[][2] = {
111 {40, 0},
112 {20, 0},
113 {10, 0},
114 {5, 0},
115 {2, 0},
116 {1, 0},
117 {0, 500000},
118 };
119
120 /**
121 * apds9306_repeat_rate_period - Sampling period in uSec
122 */
> 123 static const int apds9306_repeat_rate_period[] = {
124 25000, 50000, 100000, 200000, 500000, 1000000, 2000000
125 };
126 static_assert(ARRAY_SIZE(apds9306_repeat_rate_freq) ==
127 ARRAY_SIZE(apds9306_repeat_rate_period));
128
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, 27 Oct 2023 22:12:11 +1030
Subhajit Ghosh <[email protected]> wrote:
> On 27/10/23 21:34, Krzysztof Kozlowski wrote:
> > On 27/10/2023 10:42, Subhajit Ghosh wrote:
> >> 4. Conor reviewed the patch and said that it would be better that I handle all
> >> these operations in apds9306 driver (this) patch series rather than submitting
> >> a new patch.
> >> "Ahh apologies then. The best course of action would likely be to include
> >> the patch merging the two bindings in your series adding the third user."
> >> 5. As per this patch series -- RFC->v0->v1-v2
> >
> > RFC was the first version sent to mailing list. So after RFC there is
> > second version - v2. This is v4.
>
> Acknowledging all your other comments. Appreciate your time and effort in reviewing
> this. One last question on this - So what version should I use for the patchset
> which I will submit next - "v3" or "v5" in the Subject of the emails?
Go with v5 and play it safe given possible confusion.
Numbering when there has previously been one or more RFC versions is always rather
confusing, but we tend not to have a v0! So RFC-> v1 -> v2 -> V3 would have
been fine in my opinion, or
RFC -> V2 -> v3
With a note in v2 cover letter saying changes from RFC to make it clear there
was not a separate v1.
Jonathan
>
> Regards,
> Subhajit Ghosh
>
On Fri, 27 Oct 2023 18:15:45 +1030
Subhajit Ghosh <[email protected]> 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 <[email protected]>
More comments inline.
> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> new file mode 100644
> index 000000000000..ae79d3b1129d
> --- /dev/null
> +++ b/drivers/iio/light/apds9306.c
> @@ -0,0 +1,1326 @@
...
> +#define APDS9306_INT_CFG 0x19
> +#define APDS9306_INT_PERSISTENCE 0x1A
> +#define APDS9306_ALS_THRES_UP_0 0x21
> +#define APDS9306_ALS_THRES_UP_1 0x22
> +#define APDS9306_ALS_THRES_UP_2 0x23
> +#define APDS9306_ALS_THRES_LOW_0 0x24
> +#define APDS9306_ALS_THRES_LOW_1 0x25
> +#define APDS9306_ALS_THRES_LOW_2 0x26
> +#define APDS9306_ALS_THRES_VAR 0x27
Naming doesn't make it clear what is a field and what is a register address
_REG or _ADDR as a postfix for register addresses tends to make that clear.
> +
> +#define APDS9306_ALS_INT_STAT_MASK BIT(4)
> +#define APDS9306_ALS_DATA_STAT_MASK BIT(3)
Naming doesn't make it clear which register these are in and
it definitely should.
> +
> +#define APDS9306_ALS_THRES_VAL_MAX 0xFFFFF
> +#define APDS9306_ALS_THRES_VAR_VAL_MAX 7
> +#define APDS9306_ALS_PERSIST_VAL_MAX 15
> +#define APDS9306_ALS_READ_DATA_DELAY_US 20000
> +
> +enum apds9306_power_states {
> + STANDBY,
> + ACTIVE,
Effectively on vs off: I'd just use a boolean.
> +};
> +/**
> + * 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;
As mentioned below, I'm not sure why you need to cache these instead
of just reading the fields back from the regmap cache or the device.
> + u8 int_ch;
> +};
> +
...
> +static struct iio_event_spec apds9306_event_spec_als[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> + }, {
> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
This matches an entry above for type. Don't have separate entries.
> + },
> +};
> +
> +static struct iio_event_spec apds9306_event_spec_clear[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +#define APDS9306_CHANNEL(_type) \
> + .type = _type, \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
Scale on the intensity channel is interesting... What are the units?
There tend not to be any well defined units for intensity (as opposed
to illuminance). There may be gain on the signal, but it won't be in untils
that map directly to a scale userspace should apply. This is one of the
rare reasons for using the HARDWARE_GAIN element of the ABI.
A tricky corner however as relationship between raw value and hardwaregain
is not tightly defined (as it can be really weird!)
> +
> +static struct iio_chan_spec apds9306_channels_without_events[] = {
> + {
> + APDS9306_CHANNEL(IIO_LIGHT)
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + }, {
> + APDS9306_CHANNEL(IIO_INTENSITY)
> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .modified = 1,
> + },
> +};
> +
> +static int apds9306_runtime_power(struct apds9306_data *data, int en)
Why int? bool seems a better fit.
> +{
> + struct device *dev = data->dev;
> + int ret;
> +
> + if (en) {
> + ret = pm_runtime_resume_and_get(dev);
> + if (ret < 0) {
> + dev_err(dev, "runtime resume failed: %d\n", ret);
> + return ret;
> + }
> + return 0;
The two paths are entirely unrelated so split power up from power down as two
different functions. Only the power up can actually fail.
> + }
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
Feels like we should suggest a generic version of this given it's such a common pair
I guess the biggest problem would be naming it.
pm_runtime_mark_and_put_autosuspend() maybe?
Anyhow, that's a question for another day.
> + return 0;
> +}
> +
> +static int apds9306_read_data(struct apds9306_data *data, int *val, int reg)
> +{
> + struct device *dev = data->dev;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + int ret, delay, intg_time, status = 0;
> + u8 buff[3];
> +
> + ret = apds9306_runtime_power(data, 1);
> + if (ret)
> + return ret;
> +
> + intg_time = iio_gts_find_int_time_by_sel(&data->gts,
> + data->intg_time_idx);
Feels like caching the intg_time is potentially useful unlike the idx
which can be gotten easily from the register cache. I haven't checked
how complex looking it up like this is though, so may not be worth bothering.
> + if (intg_time < 0)
> + delay = apds9306_repeat_rate_period[data->repeat_rate_idx];
> +
> + /*
> + * Whichever is greater - integration time period or
> + * sampling period.
> + */
> + delay = max(intg_time,
> + apds9306_repeat_rate_period[data->repeat_rate_idx]);
> +
> +
> + /*
> + * Clear stale data flag that might have been set by the interrupt
> + * handler if it got data available flag set in the status reg.
> + */
> + data->read_data_available = 0;
> +
> + /*
> + * If this function runs parallel with the interrupt handler, either
> + * this reads and clears the status registers or the interrupt handler
> + * does. The interrupt handler sets a flag for read data available
> + * in our private structure which we read here.
> + */
> + ret = regmap_read_poll_timeout(data->regmap, APDS9306_MAIN_STATUS,
> + status, (status & (APDS9306_ALS_DATA_STAT_MASK |
> + APDS9306_ALS_INT_STAT_MASK)) ||
> + data->read_data_available,
> + APDS9306_ALS_READ_DATA_DELAY_US, delay * 2);
> +
> + if (ret)
> + return ret;
> +
> + /* If we reach here before the interrupt handler we push an event */
> + if ((status & APDS9306_ALS_INT_STAT_MASK)) {
> + iio_push_event(indio_dev, IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> + data->int_ch,
> + IIO_EV_TYPE_THRESH, IIO_EV_DIR_EITHER),
> + iio_get_time_ns(indio_dev));
> + }
> +
> + if ((status & APDS9306_ALS_DATA_STAT_MASK) ||
> + data->read_data_available) {
If these aren't true isn't it an error that we should report?
if (!(status & APDS9306_ALS_DATA_STAT_MASK) &&
!data->read_data_available)
return -EIO; //maybe indicated timed out?
ret = regmap
> + ret = regmap_bulk_read(data->regmap, reg, buff, sizeof(buff));
> + if (ret) {
> + dev_err(dev, "read data failed\n");
> + return ret;
> + }
> + *val = get_unaligned_le24(&buff);
> + }
> +
> + return apds9306_runtime_power(data, 0);
The fact this looks like it can fail is an artefact of combining power up and power down.
apds9306_runtime_power_down(data);
return 0;
would make it clear that it can't.
> +}
> +
> +
> +static int apds9306_intg_time_set_hw(struct apds9306_data *data, int sel)
> +{
> + int ret;
> +
> + ret = regmap_field_write(data->regfield_intg_time, sel);
Given regmap comes with excellent caching and you have that turned on,
what is the advantage of having your own cache of the value?
I'd just use regmap_field_read() to get it back where you need it.
Having made that change, this wrapper becomes unnecessary and yuo can
just call regmap_field_write() directly wherever it is currently called.
Same applies for all the other caching of status in the driver. If you
can rely on the generic register caching it normally ends ups simpler
than rolling your own.
> + if (ret)
> + return ret;
> +
> + data->intg_time_idx = sel;
> +
> + return ret;
> +}
> +static int apds9306_sampling_freq_set(struct apds9306_data *data, int val,
> + int val2)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(apds9306_repeat_rate_freq); i++)
> + if (apds9306_repeat_rate_freq[i][0] == val &&
> + apds9306_repeat_rate_freq[i][1] == val2) {
Align the second line of this with the first one
if (apds9306_repeat_rate_freq[i][0] == val &&
apds9306_repeat_rate_freq[i][1] == val2) {
> + ret = regmap_field_write(data->regfield_repeat_rate, i);
> + if (ret)
> + return ret;
> + data->repeat_rate_idx = i;
> + return ret;
> + }
> +
> + return ret;
> +}
> +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?
> + 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);
> +}
> +
> +static int apds9306_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info,
> + int val, int val2)
> +{
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + mutex_lock(&data->mutex);
Lock taken in all 'real' paths as the default won't actually happen.
So take it outside the switch and use guard() to all direct returns.
> + if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
> + ret = apds9306_event_period_set(data, val);
> + else
> + ret = apds9306_event_thresh_set(data, dir, val);
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + mutex_lock(&data->mutex);
> + ret = apds9306_event_thresh_adaptive_set(data, val);
> + mutex_unlock(&data->mutex);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int apds9306_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct apds9306_data *data = iio_priv(indio_dev);
> + unsigned int int_en;
> + int ret;
> +
> + mutex_lock(&data->mutex);
guard()
and direct returns to simplify this.
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + ret = regmap_field_read(data->regfield_int_en, &int_en);
> + if (ret)
> + break;
> + if (chan->type == IIO_LIGHT)
> + ret = int_en & data->int_ch;
> + else if (chan->type == IIO_INTENSITY)
> + ret = int_en & !data->int_ch;
> + else
> + ret = -EINVAL;
> + break;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + ret = regmap_field_read(data->regfield_int_thresh_var_en,
> + &int_en);
> + if (ret)
> + break;
> + ret = int_en;
> + break;
> + default:
> + ret = -EINVAL;
> + }
> + mutex_unlock(&data->mutex);
> + return ret;
> +}
> +
> +static int apds9306_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + int state)
> +{
> + struct apds9306_data *data = iio_priv(indio_dev);
> + int ret, int_sel;
> +
> + state = !!state;
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + /*
> + * If interrupt is enabled, the channel is set before enabling
> + * the interrupt. In case of disable, no need to switch
> + * channels. In case of different channel is selected while
> + * interrupt in on, just changing the channel.
> + */
> + if (state) {
> + if (chan->type == IIO_LIGHT)
> + int_sel = 1;
> + else if (chan->type == IIO_INTENSITY)
> + int_sel = 0;
> + else
> + return -EINVAL;
> +
> + ret = regmap_field_write(data->regfield_int_src,
> + int_sel);
> + if (ret)
> + return ret;
> + data->int_ch = int_sel;
> + }
> + ret = regmap_field_read(data->regfield_int_en, &int_sel);
> + if (ret)
> + return ret;
> + if (int_sel == state)
Not obvious why you are comparing the enabled state of a channel with the
interrupt selection. Which might not be set by here anyway.
> + return 0;
> + mutex_lock(&data->mutex);
> + ret = regmap_field_write(data->regfield_int_en, state);
> + if (!ret)
> + ret = apds9306_runtime_power(data, state);
This bit is confusing in a way that would be cleaned up by...
scoped_guard(mutex, &data->mutex) {
ret = regmap_field_write();
if (ret)
return ret;
return apds9360_runtime_power();
}
or simply
guard(mutex)(&data->mutex);
ret = regmap_field_write();
if (ret)
return ret;
...
but I think you need to add brackets to whole case statement for that
to work as expected.
> + mutex_unlock(&data->mutex);
> + return ret;
> + case IIO_EV_TYPE_THRESH_ADAPTIVE:
> + return regmap_field_write(data->regfield_int_thresh_var_en,
> + state);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define APDS9306_IIO_INFO \
> + .read_avail = apds9306_read_avail, \
> + .read_raw = apds9306_read_raw, \
> + .write_raw = apds9306_write_raw, \
> + .write_raw_get_fmt = apds9306_write_raw_get_fmt,
I'd not bother with this macro. It saves very little actual
code and makes things a tiny bit harder to read.
> +
> +static const struct iio_info apds9306_info_no_events = {
> + APDS9306_IIO_INFO
> +};
> +
> +static const struct iio_info apds9306_info = {
> + APDS9306_IIO_INFO
> + .event_attrs = &apds9306_event_attr_group,
> + .read_event_value = apds9306_read_event,
> + .write_event_value = apds9306_write_event,
> + .read_event_config = apds9306_read_event_config,
> + .write_event_config = apds9306_write_event_config,
> +};
> +static void apds9306_powerdown(void *ptr)
> +{
> + struct apds9306_data *data = (struct apds9306_data *)ptr;
> + int ret;
> +
> + /* Disable interrupts */
As below. The field names hopefully make that obvious so the comment not needed
and just becomes something that can become wrong as the driver evolves.
> + ret = regmap_field_write(data->regfield_int_thresh_var_en, 0);
> + if (ret)
> + return;
> + ret = regmap_field_write(data->regfield_int_en, 0);
> + if (ret)
> + return;
> + /* Put the device in standby mode */
This one is even more obvious.
> + apds9306_power_state(data, STANDBY);
> +}
> +
> +static int apds9306_init_device(struct apds9306_data *data)
> +{
> + struct device *dev = data->dev;
> + int ret;
> +
> + ret = apds9306_power_state(data, ACTIVE);
> + if (ret)
> + return ret;
> +
> + ret = pm_runtime_set_active(dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_set_autosuspend_delay(dev, 5000);
> + pm_runtime_use_autosuspend(dev);
this lot of runtime pm stuff isn't initializing the device, so I don't
see it as making sense in here. I'd push it out to the caller with
the power up before init and the autosuspend etc after.
I'll note that I'd expect to see a a pm_runtime_put_autosuspend()
at the end of probe to put device to sleep soon after loading.
> +
> + /* Init GTS multiplier values according to Part ID */
> + ret = apds9306_init_iio_gts(data);
> + if (ret)
> + return ret;
> +
> + /* Integration time: 100 msec */
> + ret = apds9306_intg_time_set_hw(data, 2);
Define as below.
> + if (ret)
> + return ret;
> +
> + /* Sampling frequency: 100 msec, 10 Hz */
> + ret = apds9306_sampling_freq_set(data, 10, 0);
> + if (ret)
> + return ret;
> +
> + /* Scale: x3 */
> + ret = apds9306_gain_set_hw(data, 1);
As per note below, this is one where the value written isn't 'obvious'.
A good indication that the code could be made more readable.
ret = apds9306_gain_set_hw(data, APDS9306_GSEL_3X);
for example.
> + if (ret)
> + return ret;
> +
> + /* Interrupt source channel: als */
> + ret = regmap_field_write(data->regfield_int_src, 1);
I've not checked the datasheet, but this feels like the 1 is
probably a selection value that should have a define like you
have for the GSEL values. Do that and hopefully the comment
is no longer necessary.
> + if (ret)
> + return ret;
> + data->int_ch = 1;
> +
> + /* Interrupts disabled */
> + ret = regmap_field_write(data->regfield_int_en, 0);
> + if (ret)
> + return ret;
> +
> + /* Threshold Variance disabled */
Ideal situation is that the code itself makes it obvious what is going on,
reducing need for comments. Long experience tells me that comments that
add little information and just an opportunity for code rot!
This is a good example. Setting a field called int_thresh_var_en to 0
seems pretty clear without the comment.
Consider all the other similar cases and just keep the ones that non obvious.
In some cases that means using some defines to make the code obvious.
> + return regmap_field_write(data->regfield_int_thresh_var_en, 0);
> +}
> +
> +static int apds9306_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct apds9306_data *data = iio_priv(indio_dev);
As below.
> +
> + return apds9306_power_state(data, STANDBY);
> +}
> +
> +static int apds9306_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct apds9306_data *data = iio_priv(indio_dev);
You could combine these lines without significant loss of readability.
struct apds9306_data *data = iio_priv(dev_get_drvdata(dev));
> +
> + return apds9306_power_state(data, ACTIVE);
> +}
> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(apds9306_pm_ops,
> + apds9306_runtime_suspend,
> + apds9306_runtime_resume,
> + NULL);
> +static struct i2c_driver apds9306_driver = {
> + .driver = {
> + .name = "apds9306",
> + .pm = pm_ptr(&apds9306_pm_ops),
> + .of_match_table = apds9306_of_match,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
Async probe may make sense, but it's not necessarily an obvious choice.
Add a brief comment on why.
> + },
> + .probe = apds9306_probe,
> +};
> +module_i2c_driver(apds9306_driver);
> +
> +MODULE_AUTHOR("Subhajit Ghosh <[email protected]>");
> +MODULE_DESCRIPTION("APDS9306 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_GTS_HELPER);
On 10/28/23 18:20, Jonathan Cameron wrote:
> On Fri, 27 Oct 2023 18:15:45 +1030
> Subhajit Ghosh <[email protected]> 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 <[email protected]>
>
...
>
>> +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! ~~
Hi dee Ho peeps,
On 10/29/23 17:51, Matti Vaittinen wrote:
> On 10/28/23 18:20, Jonathan Cameron wrote:
>> On Fri, 27 Oct 2023 18:15:45 +1030
>> Subhajit Ghosh <[email protected]> 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 <[email protected]>
>>
>
> ...
>
>>
>>> +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!
>
I think I have a rough understanding. I did a Kunit test which goes
through all the available scales values from the
gts->avail_all_scales_table and all integration times, and feeds them to
the logic below. It seems the first culprit is hit by:
val = 0, val2 = 125025502.
Problem is that the 125025502 is rounded. The exact linearized NANO
scale resulting from time multiplier 128, gain multiplier 1 is
125025502.5 - which means we will see rounding.
>>
>>> + if (val2 % 10)
>>> + val2 += 1;
For a while I was unsure if this check works for all cases because I see
linearized scales:
250051005 - multipliers 1x, 64x
83350335 - multipliers 3x, 64x and 6x, 32x
27783445 - multipliers 9x, 64x.
For those we will get + 1 added to val2 even though there is no
rounding. It appears this is not a problem because the
iio_gts_get_gain() (which is used to figure out the required total gain
to get the desired scale) does not require the scale to be formed by
exact multiples of gain.
Still, the check:
>>> + if (val2 % 10)
>>> + val2 += 1;
seems a bit misleading because only the scales where the NANO accuracy
is not enough need the + 1. I would at least ask for a comment
explaining this a bit better :)
Another quick'n dirty way is to simply check if requested scale is one
of the magic scales where the NANO accuracy is not enough:
41675167(.5)
125025502(.5)
20837583(.75)
13891722(.5)
and handle the rounding only for those (instead of the val2 % 10) -
still with appropriate comment.
I think it would be very nice if the gts-helpers could do the rounding
when computing the available scales, but that'd require some thinking.
Fixup patch is still very welcome ;)
I did avoid this issue with BU27* drivers earlier because I was able to
select the maximum scale so that the NANO accuracy was enough since I
used these helpers for the intensity channels.
>>> +
>>> + 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);
>>> +}
>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 10/30/23 12:21, Matti Vaittinen wrote:
> Hi dee Ho peeps,
>
> On 10/29/23 17:51, Matti Vaittinen wrote:
>> On 10/28/23 18:20, Jonathan Cameron wrote:
>>> On Fri, 27 Oct 2023 18:15:45 +1030
>>> Subhajit Ghosh <[email protected]> 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.
...
>>>> +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!
>>
>
> I think I have a rough understanding. I did a Kunit test which goes
> through all the available scales values from the
> gts->avail_all_scales_table and all integration times, and feeds them to
> the logic below. It seems the first culprit is hit by:
> val = 0, val2 = 125025502.
>
> Problem is that the 125025502 is rounded. The exact linearized NANO
> scale resulting from time multiplier 128, gain multiplier 1 is
> 125025502.5 - which means we will see rounding.
>
>>>
>>>> + if (val2 % 10)
>>>> + val2 += 1;
>
> For a while I was unsure if this check works for all cases because I see
> linearized scales:
> 250051005 - multipliers 1x, 64x
> 83350335 - multipliers 3x, 64x and 6x, 32x
> 27783445 - multipliers 9x, 64x.
>
> For those we will get + 1 added to val2 even though there is no
> rounding. It appears this is not a problem because the
> iio_gts_get_gain() (which is used to figure out the required total gain
> to get the desired scale) does not require the scale to be formed by
> exact multiples of gain.
...
> I think it would be very nice if the gts-helpers could do the rounding
> when computing the available scales, but that'd require some thinking.
> Fixup patch is still very welcome ;)
I did some further experimenting. Basically, I did a "hack" which always
rounds up the available-scales values if division results a remainder.
This way the values advertised by the available_scales did find the
matching table.
It is a tiny bit icky because for example the scale 6945861.25 becomes
6945862 in available-scales. Also, I assume that if we "hack" just the
available-scales and don't fix the rest of the logic, setting 6945862
will read back as 6945861 (I haven't tested this though). Also, the
20837583.75 will be 20837583 in available-scales but 20837582 when read
back, resulting small error. (I haven't tested this either but I assume
the current GTS code is flooring the 20837583.75 to 20837583.
I am wondering if changing the iio_gts_get_gain() to do rounding instead
of flooring and changing also the iio_gts_total_gain_to_scale() to
something like:
int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
int *scale_int, int *scale_nano)
{
u64 tmp;
int rem;
tmp = gts->max_scale;
rem = do_div(tmp, total_gain);
if (total_gain > 1 && rem >= total_gain / 2)
tmp += 1ULL;
return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
}
would do the trick. It's just that I'm a bit afraid of touching the
iio_gts_get_gain() - by the very least I need to fire up the GTS tests
which I implemented but are not in-tree due to the test-device
dependency... :/
Any thoughts?
>>>> +
>>>> + 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);
>>>> +}
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 31/10/23 17:41, Matti Vaittinen wrote:
> On 10/30/23 12:21, Matti Vaittinen wrote:
>> Hi dee Ho peeps,
>>
>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>> On 10/28/23 18:20, Jonathan Cameron wrote:
>>>> On Fri, 27 Oct 2023 18:15:45 +1030
>>>> Subhajit Ghosh <[email protected]> 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.
>
> ...
>
>>>>> +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!
>>>
>>
>> I think I have a rough understanding. I did a Kunit test which goes through all the available scales values from the gts->avail_all_scales_table and all integration times, and feeds them to the logic below. It seems the first culprit is hit by:
>> val = 0, val2 = 125025502.
>>
>> Problem is that the 125025502 is rounded. The exact linearized NANO scale resulting from time multiplier 128, gain multiplier 1 is 125025502.5 - which means we will see rounding.
>>
>>>>
>>>>> + if (val2 % 10)
>>>>> + val2 += 1;
>>
>> For a while I was unsure if this check works for all cases because I see linearized scales:
>> 250051005 - multipliers 1x, 64x
>> 83350335 - multipliers 3x, 64x and 6x, 32x
>> 27783445 - multipliers 9x, 64x.
>>
>> For those we will get + 1 added to val2 even though there is no rounding. It appears this is not a problem because the iio_gts_get_gain() (which is used to figure out the required total gain to get the desired scale) does not require the scale to be formed by exact multiples of gain.
>
> ...
>
>> I think it would be very nice if the gts-helpers could do the rounding when computing the available scales, but that'd require some thinking. Fixup patch is still very welcome ;)
>
> I did some further experimenting. Basically, I did a "hack" which always rounds up the available-scales values if division results a remainder. This way the values advertised by the available_scales did find the matching table.
>
> It is a tiny bit icky because for example the scale 6945861.25 becomes 6945862 in available-scales. Also, I assume that if we "hack" just the available-scales and don't fix the rest of the logic, setting 6945862 will read back as 6945861 (I haven't tested this though). Also, the 20837583.75 will be 20837583 in available-scales but 20837582 when read back, resulting small error. (I haven't tested this either but I assume the current GTS code is flooring the 20837583.75 to 20837583.
>
> I am wondering if changing the iio_gts_get_gain() to do rounding instead of flooring and changing also the iio_gts_total_gain_to_scale() to something like:
>
> int iio_gts_total_gain_to_scale(struct iio_gts *gts, int total_gain,
> int *scale_int, int *scale_nano)
> {
> u64 tmp;
> int rem;
>
> tmp = gts->max_scale;
>
> rem = do_div(tmp, total_gain);
> if (total_gain > 1 && rem >= total_gain / 2)
> tmp += 1ULL;
>
> return iio_gts_delinearize(tmp, NANO, scale_int, scale_nano);
> }
>
> would do the trick. It's just that I'm a bit afraid of touching the iio_gts_get_gain() - by the very least I need to fire up the GTS tests which I implemented but are not in-tree due to the test-device dependency... :/
>
> Any thoughts?
>
Hi Matti,
Sorry, got busy with my full time job.
It's nice to see that you have found the issue without my test results:)
Please find below my tests -
root@stm32mp1:/sys/bus/iio/devices/iio:device1# cat scale_available
14.009712000 4.669904000 2.334952000 1.751214000 1.556634666 0.875607000 0.778317333 0.583738000 0.437803500 0.291869000 0.218901750 0.194579333 0.145934500 0.109450875 0.097289666 0.072967250 0.048644833 0.036483625
0.024322416 0.018241812 0.012161208 0.006080604
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.875607000 > scale ## This works
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.097289666 > scale ## This fails
root@stm32mp1:/sys/bus/iio/devices/iio:device1# echo 0.097289667 > scale ## However if I add 1, it works! I figured, its a rounding issue so used this trick: "if (val2 % 10) val2 += 1;"
I am sorry, I haven't gone through the full gts internals and only used your driver as a reference to understand it's implementation. I do not have any thoughts on top of my head now but let me go through the code.
Regards,
Subhajit Ghosh
>
>> +static struct iio_event_spec apds9306_event_spec_als[] = {
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_RISING,
>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> + }, {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_FALLING,
>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>> + }, {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
>> + }, {
>> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE),
>> + }, {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> This matches an entry above for type. Don't have separate entries.
>> + },
>> +};
>> +
>> +static struct iio_event_spec apds9306_event_spec_clear[] = {
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> + },
>> +};
>> +
>> +#define APDS9306_CHANNEL(_type) \
>> + .type = _type, \
>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
>> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>
> Scale on the intensity channel is interesting... What are the units?
> There tend not to be any well defined units for intensity (as opposed
> to illuminance). There may be gain on the signal, but it won't be in untils
> that map directly to a scale userspace should apply. This is one of the
> rare reasons for using the HARDWARE_GAIN element of the ABI.
>
> A tricky corner however as relationship between raw value and hardwaregain
> is not tightly defined (as it can be really weird!)
Hi Jonathan,
Thank you for taking time for reviewing and clearing all my tiny doubts and
queries especially for the dt and versioning part. Much appreciated.
In the above case, should I not expose scale for the "clear" channel? Rather,
how should I expose the "clear" channel to userspace?
Regards,
Subhajit Ghosh
>
>> +
>> +static struct iio_chan_spec apds9306_channels_without_events[] = {
>> + {
>> + APDS9306_CHANNEL(IIO_LIGHT)
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + }, {
>> + APDS9306_CHANNEL(IIO_INTENSITY)
>> + .channel2 = IIO_MOD_LIGHT_CLEAR,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>> + .modified = 1,
>> + },
>> +};
>
On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> On 10/30/23 12:21, Matti Vaittinen wrote:
> > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > On 10/28/23 18:20, Jonathan Cameron wrote:
...
> tmp = gts->max_scale;
>
> rem = do_div(tmp, total_gain);
> if (total_gain > 1 && rem >= total_gain / 2)
> tmp += 1ULL;
...which is NIH DIV_ROUND_CLOSEST_ULL()
--
With Best Regards,
Andy Shevchenko
On 10/31/23 12:38, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
>
> ...
>
>> tmp = gts->max_scale;
>>
>> rem = do_div(tmp, total_gain);
>> if (total_gain > 1 && rem >= total_gain / 2)
>> tmp += 1ULL;
>
> ...which is NIH DIV_ROUND_CLOSEST_ULL()
Thanks for the hint Andy. I do very much prefer using stuff like the
DIV_ROUND_CLOSEST_ULL().
I will use this - do you have other suggestions for me, especially
regarding the division u64 / u64?
I would appreciate if you found the time and energy to look at:
https://lore.kernel.org/all/[email protected]/
as well. I feel like I am doing something someone else has already done.
(Yes, DIV_ROUND_CLOSEST_ULL() can be used there too).
Anyways, Thanks for this!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On 10/31/23 12:38, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
>
> ...
>
>> tmp = gts->max_scale;
>>
>> rem = do_div(tmp, total_gain);
>> if (total_gain > 1 && rem >= total_gain / 2)
>> tmp += 1ULL;
>
> ...which is NIH DIV_ROUND_CLOSEST_ULL()
There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
tmp + total_gain / 2;
before division - which in theory may overflow.
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
> On 10/31/23 12:38, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> > > On 10/30/23 12:21, Matti Vaittinen wrote:
> > > > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > > > On 10/28/23 18:20, Jonathan Cameron wrote:
...
> > > tmp = gts->max_scale;
> > >
> > > rem = do_div(tmp, total_gain);
> > > if (total_gain > 1 && rem >= total_gain / 2)
> > > tmp += 1ULL;
> >
> > ...which is NIH DIV_ROUND_CLOSEST_ULL()
>
> There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
>
> tmp + total_gain / 2;
>
> before division - which in theory may overflow.
Then you can fix it there for everybody, no?
--
With Best Regards,
Andy Shevchenko
On 10/31/23 15:42, Andy Shevchenko wrote:
> On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
>> On 10/31/23 12:38, Andy Shevchenko wrote:
>>> On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
>>>> On 10/30/23 12:21, Matti Vaittinen wrote:
>>>>> On 10/29/23 17:51, Matti Vaittinen wrote:
>>>>>> On 10/28/23 18:20, Jonathan Cameron wrote:
>
> ...
>
>>>> tmp = gts->max_scale;
>>>>
>>>> rem = do_div(tmp, total_gain);
>>>> if (total_gain > 1 && rem >= total_gain / 2)
>>>> tmp += 1ULL;
>>>
>>> ...which is NIH DIV_ROUND_CLOSEST_ULL()
>>
>> There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
>>
>> tmp + total_gain / 2;
>>
>> before division - which in theory may overflow.
>
> Then you can fix it there for everybody, no?
Now that I know of this macro - Maybe. It's just always scary to touch
things which seem like fundamental building blocks and which may be used
by many. Odds are something breaks, so I tend to be very conservative
when suggesting changes to widely used stuff. Especially when I have no
idea when and why the API has been added - and if the thing I'm trying
to "fix" has been a deliberate choice.
Yours,
-- Matti.
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Wed, Nov 01, 2023 at 08:16:32AM +0200, Matti Vaittinen wrote:
> On 10/31/23 15:42, Andy Shevchenko wrote:
> > On Tue, Oct 31, 2023 at 02:07:46PM +0200, Matti Vaittinen wrote:
> > > On 10/31/23 12:38, Andy Shevchenko wrote:
> > > > On Tue, Oct 31, 2023 at 09:11:37AM +0200, Matti Vaittinen wrote:
> > > > > On 10/30/23 12:21, Matti Vaittinen wrote:
> > > > > > On 10/29/23 17:51, Matti Vaittinen wrote:
> > > > > > > On 10/28/23 18:20, Jonathan Cameron wrote:
...
> > > > > tmp = gts->max_scale;
> > > > >
> > > > > rem = do_div(tmp, total_gain);
> > > > > if (total_gain > 1 && rem >= total_gain / 2)
> > > > > tmp += 1ULL;
> > > >
> > > > ...which is NIH DIV_ROUND_CLOSEST_ULL()
> > >
> > > There is a difference though. The DIV_ROUND_CLOSEST_ULL() does
> > >
> > > tmp + total_gain / 2;
> > >
> > > before division - which in theory may overflow.
> >
> > Then you can fix it there for everybody, no?
>
> Now that I know of this macro - Maybe. It's just always scary to touch
> things which seem like fundamental building blocks and which may be used by
> many. Odds are something breaks, so I tend to be very conservative when
> suggesting changes to widely used stuff. Especially when I have no idea when
> and why the API has been added - and if the thing I'm trying to "fix" has
> been a deliberate choice.
Welcome to the club of the div overflow in macros discussion:
https://lore.kernel.org/linux-clk/[email protected]/T/#t
--
With Best Regards,
Andy Shevchenko
Hi Subhajit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 611da07b89fdd53f140d7b33013f255bf0ed8f34]
url: https://github.com/intel-lab-lkp/linux/commits/Subhajit-Ghosh/dt-bindings-iio-light-Avago-APDS9306/20231027-154954
base: 611da07b89fdd53f140d7b33013f255bf0ed8f34
patch link: https://lore.kernel.org/r/20231027074545.6055-3-subhajit.ghosh%40tweaklogic.com
patch subject: [PATCH v2 2/2] iio: light: Add support for APDS9306 Light Sensor
config: powerpc64-allyesconfig (https://download.01.org/0day-ci/archive/20231105/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231105/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/iio/light/apds9306.c:598:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
598 | return ret;
| ^~~
drivers/iio/light/apds9306.c:593:9: note: initialize the variable 'ret' to silence this warning
593 | int ret, intg_old, gain_old, gain_new, gain_new_closest;
| ^
| = 0
1 warning generated.
vim +/ret +598 drivers/iio/light/apds9306.c
589
590 static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
591 {
592 struct device *dev = data->dev;
593 int ret, intg_old, gain_old, gain_new, gain_new_closest;
594 bool ok;
595
596 if (!iio_gts_valid_time(&data->gts, val2)) {
597 dev_err(dev, "Unsupported integration time %u\n", val2);
> 598 return ret;
599 }
600
601 intg_old = iio_gts_find_int_time_by_sel(&data->gts,
602 data->intg_time_idx);
603 if (ret < 0)
604 return ret;
605
606 if (intg_old == val2)
607 return 0;
608
609 gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
610 if (gain_old < 0)
611 return gain_old;
612
613 ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
614 intg_old, val2, &gain_new);
615 if (gain_new < 0) {
616 dev_err(dev, "Unsupported gain with time\n");
617 return gain_new;
618 }
619
620 gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
621 if (gain_new_closest < 0) {
622 gain_new_closest = iio_gts_get_min_gain(&data->gts);
623 if (gain_new_closest < 0)
624 return gain_new_closest < 0;
625 }
626 if (!ok)
627 dev_dbg(dev, "Unable to find optimum gain, setting minimum");
628
629 ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
630 if (ret < 0)
631 return ret;
632
633 ret = apds9306_intg_time_set_hw(data, ret);
634 if (ret)
635 return ret;
636
637 ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
638 if (ret < 0)
639 return ret;
640
641 return apds9306_gain_set_hw(data, ret);
642 }
643
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Sun, Nov 05, 2023 at 10:22:07PM +0800, kernel test robot wrote:
> >> drivers/iio/light/apds9306.c:598:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
> 598 | return ret;
> | ^~~
> drivers/iio/light/apds9306.c:593:9: note: initialize the variable 'ret' to silence this warning
> 593 | int ret, intg_old, gain_old, gain_new, gain_new_closest;
> | ^
> | = 0
> 1 warning generated.
Bad advice, just use correct error code instead of ret.
> 590 static int apds9306_intg_time_set(struct apds9306_data *data, int val2)
> 591 {
> 592 struct device *dev = data->dev;
> 593 int ret, intg_old, gain_old, gain_new, gain_new_closest;
> 594 bool ok;
> 595
> 596 if (!iio_gts_valid_time(&data->gts, val2)) {
> 597 dev_err(dev, "Unsupported integration time %u\n", val2);
> > 598 return ret;
> 599 }
> 600
> 601 intg_old = iio_gts_find_int_time_by_sel(&data->gts,
> 602 data->intg_time_idx);
> 603 if (ret < 0)
> 604 return ret;
> 605
> 606 if (intg_old == val2)
> 607 return 0;
> 608
> 609 gain_old = iio_gts_find_gain_by_sel(&data->gts, data->gain_idx);
> 610 if (gain_old < 0)
> 611 return gain_old;
> 612
> 613 ret = iio_gts_find_new_gain_by_old_gain_time(&data->gts, gain_old,
> 614 intg_old, val2, &gain_new);
> 615 if (gain_new < 0) {
> 616 dev_err(dev, "Unsupported gain with time\n");
> 617 return gain_new;
> 618 }
> 619
> 620 gain_new_closest = iio_find_closest_gain_low(&data->gts, gain_new, &ok);
> 621 if (gain_new_closest < 0) {
> 622 gain_new_closest = iio_gts_get_min_gain(&data->gts);
> 623 if (gain_new_closest < 0)
> 624 return gain_new_closest < 0;
> 625 }
> 626 if (!ok)
> 627 dev_dbg(dev, "Unable to find optimum gain, setting minimum");
> 628
> 629 ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
> 630 if (ret < 0)
> 631 return ret;
> 632
> 633 ret = apds9306_intg_time_set_hw(data, ret);
> 634 if (ret)
> 635 return ret;
> 636
> 637 ret = iio_gts_find_sel_by_gain(&data->gts, gain_new_closest);
> 638 if (ret < 0)
> 639 return ret;
> 640
> 641 return apds9306_gain_set_hw(data, ret);
> 642 }
--
With Best Regards,
Andy Shevchenko
On Tue, 31 Oct 2023 19:08:08 +1030
Subhajit Ghosh <[email protected]> wrote:
> >
> >> +static struct iio_event_spec apds9306_event_spec_als[] = {
> >> + {
> >> + .type = IIO_EV_TYPE_THRESH,
> >> + .dir = IIO_EV_DIR_RISING,
> >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> >> + }, {
> >> + .type = IIO_EV_TYPE_THRESH,
> >> + .dir = IIO_EV_DIR_FALLING,
> >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
> >> + }, {
> >> + .type = IIO_EV_TYPE_THRESH,
> >> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
> >> + }, {
> >> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
> >> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
> >> + BIT(IIO_EV_INFO_ENABLE),
> >> + }, {
> >> + .type = IIO_EV_TYPE_THRESH,
> >> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > This matches an entry above for type. Don't have separate entries.
> >> + },
> >> +};
> >> +
> >> +static struct iio_event_spec apds9306_event_spec_clear[] = {
> >> + {
> >> + .type = IIO_EV_TYPE_THRESH,
> >> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> >> + },
> >> +};
> >> +
> >> +#define APDS9306_CHANNEL(_type) \
> >> + .type = _type, \
> >> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
> >> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> >> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
> >> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> >
> > Scale on the intensity channel is interesting... What are the units?
> > There tend not to be any well defined units for intensity (as opposed
> > to illuminance). There may be gain on the signal, but it won't be in untils
> > that map directly to a scale userspace should apply. This is one of the
> > rare reasons for using the HARDWARE_GAIN element of the ABI.
> >
> > A tricky corner however as relationship between raw value and hardwaregain
> > is not tightly defined (as it can be really weird!)
> Hi Jonathan,
>
> Thank you for taking time for reviewing and clearing all my tiny doubts and
> queries especially for the dt and versioning part. Much appreciated.
>
> In the above case, should I not expose scale for the "clear" channel? Rather,
> how should I expose the "clear" channel to userspace?
What is the scale? What units to you get after applying it?
I suspect it's not well defined.
Not sure what we've done in similar cases in the past. Perhaps have
a look at those. There may be precedence for still using scale.
I'm about to jump on a long haul flight (delayed - sigh) so don't have
time to look myself!
Jonathan
>
> Regards,
> Subhajit Ghosh
>
> >
> >> +
> >> +static struct iio_chan_spec apds9306_channels_without_events[] = {
> >> + {
> >> + APDS9306_CHANNEL(IIO_LIGHT)
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> + }, {
> >> + APDS9306_CHANNEL(IIO_INTENSITY)
> >> + .channel2 = IIO_MOD_LIGHT_CLEAR,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> >> + .modified = 1,
> >> + },
> >> +};
> >
>
>
On 6/11/23 21:43, Jonathan Cameron wrote:
> On Tue, 31 Oct 2023 19:08:08 +1030
> Subhajit Ghosh <[email protected]> wrote:
>
>>>
>>>> +static struct iio_event_spec apds9306_event_spec_als[] = {
>>>> + {
>>>> + .type = IIO_EV_TYPE_THRESH,
>>>> + .dir = IIO_EV_DIR_RISING,
>>>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>>>> + }, {
>>>> + .type = IIO_EV_TYPE_THRESH,
>>>> + .dir = IIO_EV_DIR_FALLING,
>>>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE),
>>>> + }, {
>>>> + .type = IIO_EV_TYPE_THRESH,
>>>> + .mask_shared_by_all = BIT(IIO_EV_INFO_PERIOD),
>>>> + }, {
>>>> + .type = IIO_EV_TYPE_THRESH_ADAPTIVE,
>>>> + .mask_shared_by_all = BIT(IIO_EV_INFO_VALUE) |
>>>> + BIT(IIO_EV_INFO_ENABLE),
>>>> + }, {
>>>> + .type = IIO_EV_TYPE_THRESH,
>>>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>> This matches an entry above for type. Don't have separate entries.
>>>> + },
>>>> +};
>>>> +
>>>> +static struct iio_event_spec apds9306_event_spec_clear[] = {
>>>> + {
>>>> + .type = IIO_EV_TYPE_THRESH,
>>>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>>>> + },
>>>> +};
>>>> +
>>>> +#define APDS9306_CHANNEL(_type) \
>>>> + .type = _type, \
>>>> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
>>>> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
>>>> + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ), \
>>>
>>> Scale on the intensity channel is interesting... What are the units?
>>> There tend not to be any well defined units for intensity (as opposed
>>> to illuminance). There may be gain on the signal, but it won't be in untils
>>> that map directly to a scale userspace should apply. This is one of the
>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
>>>
>>> A tricky corner however as relationship between raw value and hardwaregain
>>> is not tightly defined (as it can be really weird!)
>> Hi Jonathan,
>>
>> Thank you for taking time for reviewing and clearing all my tiny doubts and
>> queries especially for the dt and versioning part. Much appreciated.
>>
>> In the above case, should I not expose scale for the "clear" channel? Rather,
>> how should I expose the "clear" channel to userspace?
> What is the scale? What units to you get after applying it?
The scale is in Lux. The output after applying is Lux.
>
> I suspect it's not well defined.
>
> Not sure what we've done in similar cases in the past. Perhaps have
> a look at those. There may be precedence for still using scale.
Sure, I'll look it up.
>
> I'm about to jump on a long haul flight (delayed - sigh) so don't have
> time to look myself!
>
No worries. I will update notes and application in next version.
Have a good trip to where ever you are going. Are you coming to Australia
by any chance?
Thanks for the reply.
Regards,
Subhajit Ghosh
On 11/6/23 14:04, Subhajit Ghosh wrote:
> On 6/11/23 21:43, Jonathan Cameron wrote:
>> On Tue, 31 Oct 2023 19:08:08 +1030
>> Subhajit Ghosh <[email protected]> wrote:
>>
>>>> Scale on the intensity channel is interesting... What are the units?
>>>> There tend not to be any well defined units for intensity (as opposed
>>>> to illuminance). There may be gain on the signal, but it won't be
>>>> in untils
>>>> that map directly to a scale userspace should apply. This is one of
>>>> the
>>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
>>>>
>>>> A tricky corner however as relationship between raw value and
>>>> hardwaregain
>>>> is not tightly defined (as it can be really weird!)
>>> Hi Jonathan,
>>>
>>> Thank you for taking time for reviewing and clearing all my tiny
>>> doubts and
>>> queries especially for the dt and versioning part. Much appreciated.
>>>
>>> In the above case, should I not expose scale for the "clear" channel?
>>> Rather,
>>> how should I expose the "clear" channel to userspace?
>> What is the scale? What units to you get after applying it?
> The scale is in Lux. The output after applying is Lux.
Hi Subhajit,
I am by no means an expert here but maybe you could check if the channel
should be of type 'illuminance'? (To me 'Lux' sounds like an unit of
illuminance rather than intensity).
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Mon, 6 Nov 2023 14:10:43 +0200
Matti Vaittinen <[email protected]> wrote:
> On 11/6/23 14:04, Subhajit Ghosh wrote:
> > On 6/11/23 21:43, Jonathan Cameron wrote:
> >> On Tue, 31 Oct 2023 19:08:08 +1030
> >> Subhajit Ghosh <[email protected]> wrote:
> >>
> >>>> Scale on the intensity channel is interesting... What are the units?
> >>>> There tend not to be any well defined units for intensity (as opposed
> >>>> to illuminance). There may be gain on the signal, but it won't be
> >>>> in untils
> >>>> that map directly to a scale userspace should apply. This is one of
> >>>> the
> >>>> rare reasons for using the HARDWARE_GAIN element of the ABI.
> >>>>
> >>>> A tricky corner however as relationship between raw value and
> >>>> hardwaregain
> >>>> is not tightly defined (as it can be really weird!)
> >>> Hi Jonathan,
> >>>
> >>> Thank you for taking time for reviewing and clearing all my tiny
> >>> doubts and
> >>> queries especially for the dt and versioning part. Much appreciated.
> >>>
> >>> In the above case, should I not expose scale for the "clear" channel?
> >>> Rather,
> >>> how should I expose the "clear" channel to userspace?
> >> What is the scale? What units to you get after applying it?
> > The scale is in Lux. The output after applying is Lux.
>
> Hi Subhajit,
>
> I am by no means an expert here but maybe you could check if the channel
> should be of type 'illuminance'? (To me 'Lux' sounds like an unit of
> illuminance rather than intensity).
Absolutely. Light measurements are 'weird'. Lux is a measurement of
light as if the human eye were looking at it. Unfortunately light sensors
don't have the same sensitivity curves as the eye so instead they tend
to do it either by some careful choice of filters, a horrible approximation
based on assumption of day light, or most commonly by combining the inputs
of several different light sensors.
Clear normally means sensitive to both visible light and infrared which
means you need to remove the infrared part to get closer to human
eye response. Thus in other light sensors, we can't assign standard
units to the intensity channels (and so don't provide a scale).
We just don't have enough information to work out what they are measuring.
In theory we could provide the full sensitivity curve (usually somewhere
in the datasheet) but they can be complex multi-peak things so we don't
so far.
Jonathan
>
> Yours,
> -- Matti
>