Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor
From: Shreeya Patel
Date: Mon Apr 11 2022 - 13:07:10 EST
On 27/03/22 20:00, Jonathan Cameron wrote:
Hi Jonathan,
I have a question inline related to one of your comments.
On Fri, 25 Mar 2022 16:00:14 +0530
Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx> wrote:
Hi Zhigang, Shreeya,
Comments inline.
Thanks,
Jonathan
From: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
Add initial support for ltrf216a ambient light sensor.
Datasheet :-
https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
We now have a Datasheet tag, so make this part of the tag block so automated
tools can find it easily:
Datasheet: https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
Co-developed-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
Signed-off-by: Zhigang Shi <Zhigang.Shi@xxxxxxxxxx>
---
drivers/iio/light/Kconfig | 10 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/ltrf216a.c | 334 +++++++++++++++++++++++++++++++++++
3 files changed, 345 insertions(+)
create mode 100644 drivers/iio/light/ltrf216a.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index a62c7b4b8678..08fa383a8ca7 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -318,6 +318,16 @@ config SENSORS_LM3533
changes. The ALS-control output values can be set per zone for the
three current output channels.
+config LTRF216A
+ tristate "Liteon LTRF216A Light Sensor"
+ depends on I2C
+ help
+ If you say Y or M here, you get support for Liteon LTRF216A
+ Ambient Light Sensor.
+
+ If built as a dynamically linked module, it will be called
+ ltrf216a.
+
config LTR501
tristate "LTR-501ALS-01 light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index d10912faf964..8fa91b9fe5b6 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
obj-$(CONFIG_ISL29125) += isl29125.o
obj-$(CONFIG_JSA1212) += jsa1212.o
obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
+obj-$(CONFIG_LTRF216A) += ltrf216a.o
obj-$(CONFIG_LTR501) += ltr501.o
obj-$(CONFIG_LV0104CS) += lv0104cs.o
obj-$(CONFIG_MAX44000) += max44000.o
diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
new file mode 100644
index 000000000000..99295358a7fe
--- /dev/null
+++ b/drivers/iio/light/ltrf216a.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * LTRF216A Ambient Light Sensor
+ *
+ * Copyright (C) 2021 Lite-On Technology Corp (Singapore)
+ * Author: Shi Zhigang <Zhigang.Shi@xxxxxxxxxx>
+ *
+ * IIO driver for LTRF216A (7-bit I2C slave address 0x53).
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
mod_devicetable.h for the id tables
+#include <linux/pm.h>
+#include <linux/delay.h>
+
+#define LTRF216A_DRV_NAME "ltrf216a"
+
+#define LTRF216A_MAIN_CTRL 0x00
+
+#define LTRF216A_ALS_MEAS_RATE 0x04
MEAS_RES seems more appropriate from datasheet.
As mentioned below, also add defines for all the fields
of the registers you will access and where the fields are
obvious numerical things, add defines for the values they
can take.
+#define LTRF216A_MAIN_STATUS 0x07
+#define LTRF216A_CLEAR_DATA_0 0x0A
+
+#define LTRF216A_ALS_DATA_0 0x0D
+
+static const int int_time_mapping[] = { 400000, 200000, 100000 };
+
+struct ltrf216a_data {
+ struct i2c_client *client;
+ u32 int_time;
+ u8 int_time_fac;
+ u8 als_gain_fac;
+ struct mutex mutex;
All locks need a comment to say exactly what they are protecting.
+};
+
+/* open air. need to update based on TP transmission rate. */
+#define WIN_FAC 1
+
+static const struct iio_chan_spec ltrf216a_channels[] = {
+ {
+ .type = IIO_LIGHT,
+ .info_mask_separate =
+ BIT(IIO_CHAN_INFO_PROCESSED) |
+ BIT(IIO_CHAN_INFO_INT_TIME),
+ }
+};
+
+static IIO_CONST_ATTR_INT_TIME_AVAIL("0.1 0.2 0.4");
+
+static struct attribute *ltrf216a_attributes[] = {
+ &iio_const_attr_integration_time_available.dev_attr.attr,
please use the read_avail callback and set the appropriate
_available bit.
That allows in kernel access to this information + is probably
shorter in this case as you won't have an attribute group etc
to deal wtih.
+ NULL
+};
+
+static const struct attribute_group ltrf216a_attribute_group = {
+ .attrs = ltrf216a_attributes,
+};
+
+static int ltrf216a_init(struct iio_dev *indio_dev)
+{
+ int ret;
+ struct ltrf216a_data *data = iio_priv(indio_dev);
+
+ ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_CTRL);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Error reading LTRF216A_MAIN_CTRL\n");
+ return ret;
+ }
+
+ /* enable sensor */
+ ret |= 0x02;
Needs a #define and preferably use
ret |= FIELD_PREP()...
+ ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ltrf216a_disable(struct iio_dev *indio_dev)
+{
+ int ret;
+ struct ltrf216a_data *data = iio_priv(indio_dev);
+
+ ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
+ if (ret < 0)
+ dev_err(&data->client->dev, "Error writing LTRF216A_MAIN_CTRL\n");
+
+ return ret;
+}
+
+static int ltrf216a_set_it_time(struct ltrf216a_data *data, int itime)
+{
+ int i, ret, index = -1;
+ u8 reg;
+
+ for (i = 0; i < ARRAY_SIZE(int_time_mapping); i++) {
+ if (int_time_mapping[i] == itime) {
+ index = i;
+ break;
+ }
+ }
+ /* Make sure integration time index is valid */
+ if (index < 0)
+ return -EINVAL;
+
+ if (index == 0) {
Switch statement seems more appropriate than this stack of if else
+ reg = 0x03;
reg isn't a great name as I assume this is the value, not the address
which was my first thought... Perhaps reg_val?
+ data->int_time_fac = 4;
+ } else if (index == 1) {
+ reg = 0x13;
+ data->int_time_fac = 2;
+ } else {
+ reg = (index << 4) | 0x02;
Unless I'm missing something index == 2 if we get here.
So why the calculation? I'd suggest defining the two fields and using
FIELD_PREP() to set up each part probably to one of a set of
#define LTRF216A_ALS_MEAS_RATE_
+ data->int_time_fac = 1;
+ }
+
+ ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg);
Called MEAS_RES on the datasheet, why this name for the register?
+ if (ret < 0)
+ return ret;
+
+ data->int_time = itime;
+
+ return 0;
+}
+
+static int ltrf216a_get_it_time(struct ltrf216a_data *data, int *val, int *val2)
+{
+ *val = 0;
+ *val2 = data->int_time;
I'd put this inline as it avoids a question I had at the call site on why
you passed *val in as it would always be 0.
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
+{
+ int ret;
+ int tries = 25;
+ int val_0, val_1, val_2;
+
+ while (tries--) {
+ ret = i2c_smbus_read_byte_data(data->client, LTRF216A_MAIN_STATUS);
+ if (ret < 0)
+ return ret;
+ if (ret & 0x08)
That 0x08 is a magic number and also better defined using BIT(3)
Anyhow, use a define for that.
+ break;
+ msleep(20);
+ }
+
+ val_0 = i2c_smbus_read_byte_data(data->client, addr);
All of these can return errors so you should check.
Device doesn't support any larger reads?
+ val_1 = i2c_smbus_read_byte_data(data->client, addr + 1);
+ val_2 = i2c_smbus_read_byte_data(data->client, addr + 2);
+ ret = (val_2 << 16) + (val_1 << 8) + val_0;
This is a le24_to_cpu() conversion.
Preferred choice would be to use something like
u8 buf[3];
int i;
for (i = 0; i < 3; i++) {
ret = i2c_smbus_read_byte_data(data->client, addr);
if (ret < 0)
return ret;
buf[i] = ret;
}
return le24_to_cpu(buf);
We do not have any le24_to_cpu() function in current kernel source code.
I was thinking to use le32_to_cpu() instead but it requires an argument of
type __le32 and our case we storing the values in u8 buf[3] so I'm not
really sure if it's possible to use le32_to_cpu() or any other function.
Thanks,
Shreeya Patel