On 10/8/23 18:48, Subhajit Ghosh wrote:
Driver support for Avago (Broadcom) APDS9306
Ambient Light Sensor with als and clear channels.
This driver exposes raw values for both the channels and
processed(lux) values for the als channel.
Support for both with or without hardware interrupt
configurations are provided.
Datasheet at https://docs.broadcom.com/doc/AV02-4755EN
Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
Hi Subhajit,
To my eyes this driver looks nice. Just spotted two minor things.
---
drivers/iio/light/Kconfig | 12 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/apds9306.c | 1381 ++++++++++++++++++++++++++++++++++
3 files changed, 1394 insertions(+)
create mode 100644 drivers/iio/light/apds9306.c
...
+
+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;
Returning the truth value on purpose? :)
+ }
+ 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 get_device_id_lux_per_count(struct apds9306_data *data)
+{
+ int ret, part_id;
+
+ ret = regmap_read(data->regmap, APDS9306_PART_ID, &part_id);
+ if (ret)
+ return ret;
+
+ if (part_id == apds9306_part_id_nlux_per_count[0].part_id)
+ data->nlux_per_count =
+ apds9306_part_id_nlux_per_count[0].nlux_per_count;
+ else if (part_id == apds9306_part_id_nlux_per_count[1].part_id)
+ data->nlux_per_count =
+ apds9306_part_id_nlux_per_count[1].nlux_per_count;
+ else
+ return -ENXIO;
I think we should be able to differentiate between the IC variants by DT
compatible. (Commented that on bindings patch). Not sure if we need to
support cases where the sensor is instantiated without device-tree. I am
not super happy when code requires the part-id to be known if we have
separate compatibles for variants. Can we in dt-case just print a
warning if the part-ID is not what we expect - and proceed assuming the
nlux_per_count based on the DT information? (Sometimes we see new
variants with same part-IDs - or many part-IDs with no SW changes
needed. Hence maintaining the part-ID lists may be tedious). This is
just some pondering though, no strong requirements from my side
+
+ return 0;
+}
+
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
