[PATCH 0/3] Add LTRF216A Driver

[PATCH 0/3] Add LTRF216A Driver

[Shreeya Patel]

This patchset adds support for ltrf216a Ambient Light Sensor
and documents the DT bindings for the same.

Shreeya Patel (3):
  dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix
  dt-bindings: Document ltrf216a light sensor bindings
  iio: light: Add support for ltrf216a sensor

 .../bindings/iio/light/liteon,ltrf216a.yaml   |  42 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   3 +
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/ltrf216a.c                  | 334 ++++++++++++++++++
 5 files changed, 390 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
 create mode 100644 drivers/iio/light/ltrf216a.c

-- 
2.30.2

[PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Shreeya Patel]

'liteon' is the correct vendor prefix for devices released by
LITE-ON Technology Corp. But one of the released device which uses
ltr216a light sensor exposes the vendor prefix name as 'ltr' through
ACPI.

Hence, add 'ltr' as a deprecated vendor prefix which would suppress the
following warning in case the compatible string used in ltrf216a driver
is "ltr,ltrf216a"

WARNING: DT compatible string vendor "ltr" appears un-documented --
check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
364: FILE: drivers/iio/light/ltrf216a.c:313:
+    { .compatible = "ltr,ltrf216a" },


Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index baed2b007d0e..e78091d51443 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -691,6 +691,9 @@ patternProperties:
     description: Linx Technologies
   "^liteon,.*":
     description: LITE-ON Technology Corp.
+  "^ltr,.*":
+    description: LITE-ON Technology Corp.
+    deprecated: true
   "^litex,.*":
     description: LiteX SoC builder
   "^lltc,.*":
-- 
2.30.2

Re: [PATCH 1/3] dt-bindings: vendor-prefixes: Add 'ltr' as deprecated vendor prefix

[Krzysztof Kozlowski]

On 25/03/2022 11:30, Shreeya Patel wrote:
> 'liteon' is the correct vendor prefix for devices released by
> LITE-ON Technology Corp. But one of the released device which uses
> ltr216a light sensor exposes the vendor prefix name as 'ltr' through
> ACPI.
> 
> Hence, add 'ltr' as a deprecated vendor prefix which would suppress the
> following warning in case the compatible string used in ltrf216a driver
> is "ltr,ltrf216a"
> 
> WARNING: DT compatible string vendor "ltr" appears un-documented --
> check ./Documentation/devicetree/bindings/vendor-prefixes.yaml
> 364: FILE: drivers/iio/light/ltrf216a.c:313:
> +    { .compatible = "ltr,ltrf216a" },
> 
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index baed2b007d0e..e78091d51443 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -691,6 +691,9 @@ patternProperties:
>      description: Linx Technologies
>    "^liteon,.*":
>      description: LITE-ON Technology Corp.
> +  "^ltr,.*":
> +    description: LITE-ON Technology Corp.
> +    deprecated: true

Keep in alphabetical order. With the change:
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof

[PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Shreeya Patel]

Add devicetree bindings for ltrf216a ambient light sensor

Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
---
 .../bindings/iio/light/liteon,ltrf216a.yaml   | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
new file mode 100644
index 000000000000..275d86a0353a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LTRF216A Ambient Light Sensor
+
+maintainers:
+  - Zhigang Shi <Zhigang.Shi@liteon.com>
+
+description: |
+  Ambient sensing with an i2c interface.
+
+properties:
+  compatible:
+    enum:
+      - liteon,ltrf216a
+      - ltr,ltrf216a
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@53 {
+                compatible = "ltr,ltrf216a";
+                reg = <0x53>;
+        };
+    };
+...
-- 
2.30.2

Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Krzysztof Kozlowski]

On 25/03/2022 11:30, Shreeya Patel wrote:
> Add devicetree bindings for ltrf216a ambient light sensor

Full stop.

> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
>  .../bindings/iio/light/liteon,ltrf216a.yaml   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> new file mode 100644
> index 000000000000..275d86a0353a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTRF216A Ambient Light Sensor
> +
> +maintainers:
> +  - Zhigang Shi <Zhigang.Shi@liteon.com>
> +
> +description: |
> +  Ambient sensing with an i2c interface.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - liteon,ltrf216a
> +      - ltr,ltrf216a

Make it oneOf and then you can add "deprecated" to "ltr", because I
assume it is deprecated since it is using deprecated vendor prefix and
you keep it only because there is a real world device with it.

> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@53 {
> +                compatible = "ltr,ltrf216a";

Don't use deprecated compatible/vendor prefix.

> +                reg = <0x53>;
> +        };
> +    };
> +...


Best regards,
Krzysztof

Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Jonathan Cameron]

On Fri, 25 Mar 2022 16:00:13 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Add devicetree bindings for ltrf216a ambient light sensor
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Hi Shreeya,

As we are making this Zhigang Shi's problem to maintain, I'm 
looking for an ack.  Bit mean otherwise :)

Except for the deprecated part this could just have gone in
trivial-bindings.yaml.

I guess you don't need it for your existing board, but best
practice would probably include ensuring whatever supplies
the device needs are here so that platforms that don't enable
them by default can turn them on.

Also, there is an interrupt according to the datasheet linked
from patch 3 and that should definitely be in the binding
even if the driver isn't using it.

Jonathan


> ---
>  .../bindings/iio/light/liteon,ltrf216a.yaml   | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> new file mode 100644
> index 000000000000..275d86a0353a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LTRF216A Ambient Light Sensor
> +
> +maintainers:
> +  - Zhigang Shi <Zhigang.Shi@liteon.com>
> +
> +description: |
> +  Ambient sensing with an i2c interface.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - liteon,ltrf216a
> +      - ltr,ltrf216a
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@53 {
> +                compatible = "ltr,ltrf216a";
> +                reg = <0x53>;
> +        };
> +    };
> +...

Re: [PATCH 2/3] dt-bindings: Document ltrf216a light sensor bindings

[Gabriel Krisman Bertazi]

Jonathan Cameron <jic23@kernel.org> writes:

> On Fri, 25 Mar 2022 16:00:13 +0530
> Shreeya Patel <shreeya.patel@collabora.com> wrote:
>
>> Add devicetree bindings for ltrf216a ambient light sensor
>> 
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Hi Shreeya,
>
> As we are making this Zhigang Shi's problem to maintain, I'm 
> looking for an ack.  Bit mean otherwise :)

Alternatively, Shreeya could take over the maintainership of this
schema, since she wrote it. :)

>
> Except for the deprecated part this could just have gone in
> trivial-bindings.yaml.
>
> I guess you don't need it for your existing board, but best
> practice would probably include ensuring whatever supplies
> the device needs are here so that platforms that don't enable
> them by default can turn them on.
>
> Also, there is an interrupt according to the datasheet linked
> from patch 3 and that should definitely be in the binding
> even if the driver isn't using it.
>
> Jonathan
>
>
>> ---
>>  .../bindings/iio/light/liteon,ltrf216a.yaml   | 42 +++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
>> new file mode 100644
>> index 000000000000..275d86a0353a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
>> @@ -0,0 +1,42 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/light/liteon,ltrf216a.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: LTRF216A Ambient Light Sensor
>> +
>> +maintainers:
>> +  - Zhigang Shi <Zhigang.Shi@liteon.com>
>> +
>> +description: |
>> +  Ambient sensing with an i2c interface.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - liteon,ltrf216a
>> +      - ltr,ltrf216a
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        light-sensor@53 {
>> +                compatible = "ltr,ltrf216a";
>> +                reg = <0x53>;
>> +        };
>> +    };
>> +...
>

-- 
Gabriel Krisman Bertazi

[PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

From: Zhigang Shi <Zhigang.Shi@liteon.com>

Add initial support for ltrf216a ambient light sensor.

Datasheet :-
https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf


Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
---
 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@liteon.com>
+ *
+ * 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>
+#include <linux/pm.h>
+#include <linux/delay.h>
+
+#define LTRF216A_DRV_NAME "ltrf216a"
+
+#define LTRF216A_MAIN_CTRL		0x00
+
+#define LTRF216A_ALS_MEAS_RATE		0x04
+#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;
+};
+
+/* 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,
+	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;
+	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) {
+		reg = 0x03;
+		data->int_time_fac = 4;
+	} else if (index == 1) {
+		reg = 0x13;
+		data->int_time_fac = 2;
+	} else {
+		reg = (index << 4) | 0x02;
+		data->int_time_fac = 1;
+	}
+
+	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg);
+	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;
+
+	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)
+			break;
+		msleep(20);
+	}
+
+	val_0 = i2c_smbus_read_byte_data(data->client, addr);
+	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;
+
+	return ret;
+}
+
+static int ltrf216a_get_lux(struct ltrf216a_data *data)
+{
+	int greendata, cleardata, lux;
+
+	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
+	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
+
+	if (greendata < 0 || cleardata < 0)
+		lux = 0;
+	else
+		lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;
+
+	return lux;
+}
+
+static int ltrf216a_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, int *val,
+			     int *val2, long mask)
+{
+	int ret;
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+
+	mutex_lock(&data->mutex);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = ltrf216a_get_lux(data);
+		*val = ret;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = ltrf216a_get_it_time(data, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int ltrf216a_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan, int val,
+			      int val2, long mask)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val != 0)
+			return -EINVAL;
+		mutex_lock(&data->mutex);
+		ret = ltrf216a_set_it_time(data, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ltrf216a_info = {
+	.read_raw	= ltrf216a_read_raw,
+	.write_raw	= ltrf216a_write_raw,
+	.attrs		= &ltrf216a_attribute_group,
+};
+
+static int ltrf216a_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct ltrf216a_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	mutex_init(&data->mutex);
+
+	indio_dev->info = &ltrf216a_info;
+	indio_dev->name = LTRF216A_DRV_NAME;
+	indio_dev->channels = ltrf216a_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = ltrf216a_init(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "ltrf216a chip init failed\n");
+		return ret;
+	}
+	data->int_time = 100000;
+	data->int_time_fac = 1;
+	data->als_gain_fac = 3;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to register iio dev\n");
+		goto err_init;
+	}
+
+	return 0;
+err_init:
+	ltrf216a_disable(indio_dev);
+	return ret;
+}
+
+static int ltrf216a_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	ltrf216a_disable(indio_dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ltrf216a_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return ltrf216a_disable(indio_dev);
+}
+
+static int ltrf216a_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return ltrf216a_init(indio_dev);
+}
+
+static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume);
+#define LTRF216A_PM_OPS (&ltrf216a_pm_ops)
+#else
+#define LTRF216A_PM_OPS NULL
+#endif
+
+static const struct i2c_device_id ltrf216a_id[] = {
+	{ LTRF216A_DRV_NAME, 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
+
+static const struct of_device_id ltrf216a_of_match[] = {
+	{ .compatible = "ltr,ltrf216a", },
+	{ .compatible = "liteon,ltrf216a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
+
+static struct i2c_driver ltrf216a_driver = {
+	.driver = {
+		.name = LTRF216A_DRV_NAME,
+		.pm = LTRF216A_PM_OPS,
+		.of_match_table = ltrf216a_of_match,
+	},
+	.probe		= ltrf216a_probe,
+	.remove		= ltrf216a_remove,
+	.id_table	= ltrf216a_id,
+};
+
+module_i2c_driver(ltrf216a_driver);
+
+MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
+MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.30.2

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Krzysztof Kozlowski]

On 25/03/2022 11:30, Shreeya Patel wrote:
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
> 
> Add initial support for ltrf216a ambient light sensor.
> 
> Datasheet :-
> https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
> 
> 
> Co-developed-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
> ---
>  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@liteon.com>
> + *
> + * 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>
> +#include <linux/pm.h>
> +#include <linux/delay.h>
> +
> +#define LTRF216A_DRV_NAME "ltrf216a"
> +
> +#define LTRF216A_MAIN_CTRL		0x00
> +
> +#define LTRF216A_ALS_MEAS_RATE		0x04
> +#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;
> +};
> +
> +/* 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,
> +	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;
> +	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) {
> +		reg = 0x03;
> +		data->int_time_fac = 4;
> +	} else if (index == 1) {
> +		reg = 0x13;
> +		data->int_time_fac = 2;
> +	} else {
> +		reg = (index << 4) | 0x02;
> +		data->int_time_fac = 1;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RATE, reg);
> +	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;
> +
> +	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)
> +			break;
> +		msleep(20);
> +	}
> +
> +	val_0 = i2c_smbus_read_byte_data(data->client, addr);
> +	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;
> +
> +	return ret;
> +}
> +
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata, lux;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +	if (greendata < 0 || cleardata < 0)
> +		lux = 0;
> +	else
> +		lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;
> +
> +	return lux;
> +}
> +
> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	int ret;
> +	struct ltrf216a_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = ltrf216a_get_lux(data);
> +		*val = ret;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = ltrf216a_get_it_time(data, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int ltrf216a_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct ltrf216a_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0)
> +			return -EINVAL;
> +		mutex_lock(&data->mutex);
> +		ret = ltrf216a_set_it_time(data, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ltrf216a_info = {
> +	.read_raw	= ltrf216a_read_raw,
> +	.write_raw	= ltrf216a_write_raw,
> +	.attrs		= &ltrf216a_attribute_group,
> +};
> +
> +static int ltrf216a_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct ltrf216a_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	mutex_init(&data->mutex);
> +
> +	indio_dev->info = &ltrf216a_info;
> +	indio_dev->name = LTRF216A_DRV_NAME;
> +	indio_dev->channels = ltrf216a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = ltrf216a_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "ltrf216a chip init failed\n");
> +		return ret;
> +	}
> +	data->int_time = 100000;
> +	data->int_time_fac = 1;
> +	data->als_gain_fac = 3;
> +
> +	ret = iio_device_register(indio_dev);

Use devm- function, assuming no issues with removal-steps (disable will
be before iio unregister).

> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to register iio dev\n");
> +		goto err_init;
> +	}
> +
> +	return 0;

Blank line.

> +err_init:
> +	ltrf216a_disable(indio_dev);
> +	return ret;
> +}
> +



Best regards,
Krzysztof

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Jonathan Cameron]

On Fri, 25 Mar 2022 16:00:14 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

Hi Zhigang, Shreeya,

Comments inline.

Thanks,

Jonathan
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
> 
> 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@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
> ---
>  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@liteon.com>
> + *
> + * 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);

> +
> +	return ret;
> +}
> +
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata, lux;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +	if (greendata < 0 || cleardata < 0)

I'd rather see the error passed all the way up than eaten here.

> +		lux = 0;
> +	else
> +		lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;

This feels like it would be better reported as
IIO_VAL_FRACTIONAL;
Won't make any difference if the only user is sysfs, but if anyone
ever wants to use the in kernel access to this device they will get
access to better precision.

Also possible the core handling for IIO_VAL_FRACTIONAL will do a better
job on retaining precision.

> +
> +	return lux;
> +}
> +
> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	int ret;
> +	struct ltrf216a_data *data = iio_priv(indio_dev);
> +
> +	mutex_lock(&data->mutex);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = ltrf216a_get_lux(data);
> +		*val = ret;
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = ltrf216a_get_it_time(data, val, val2);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int ltrf216a_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int val,
> +			      int val2, long mask)
> +{
> +	struct ltrf216a_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0)
> +			return -EINVAL;
> +		mutex_lock(&data->mutex);
> +		ret = ltrf216a_set_it_time(data, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info ltrf216a_info = {
> +	.read_raw	= ltrf216a_read_raw,
> +	.write_raw	= ltrf216a_write_raw,
> +	.attrs		= &ltrf216a_attribute_group,
> +};
> +
> +static int ltrf216a_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)

Whilst it's taking a long time to clean out the old
approach, probe_new() callback preferred for new i2c drivers.

> +{
> +	struct ltrf216a_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	mutex_init(&data->mutex);
> +
> +	indio_dev->info = &ltrf216a_info;
> +	indio_dev->name = LTRF216A_DRV_NAME;
> +	indio_dev->channels = ltrf216a_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = ltrf216a_init(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "ltrf216a chip init failed\n");
> +		return ret;

As below, slightly preference for dev_err_probe().
It doesn't really matter when deferral isn't a possibility but using
that function ends up slightly neater.

> +	}
blank line here.  Adding separation so we have
"function call
error handler

next thing.."

Just makes things a tiny bit easier to read quickly.


> +	data->int_time = 100000;
> +	data->int_time_fac = 1;
> +	data->als_gain_fac = 3;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to register iio dev\n");
Once using devm, this can be
		return dev_err_probe(&client->dev, ret,
				     "failed to register iio dev\n");

Which is neater but only use this function to print errors for things
that happen in probe.
 
> +		goto err_init;
> +	}
> +
> +	return 0;
> +err_init:
> +	ltrf216a_disable(indio_dev);
> +	return ret;
> +}
> +
> +static int ltrf216a_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
> +	ltrf216a_disable(indio_dev);

As Kryzstof mentioned, use
devm versions and where they don't exist you can use
devm_add_action_or_reset() (e.g. for this disable)

That way we don't need error handling or a remove function.
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int ltrf216a_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return ltrf216a_disable(indio_dev);
> +}
> +
> +static int ltrf216a_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +
> +	return ltrf216a_init(indio_dev);
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume);
> +#define LTRF216A_PM_OPS (&ltrf216a_pm_ops)
> +#else
> +#define LTRF216A_PM_OPS NULL
> +#endif

Recently some 'magic' macros were added to pm.h that allow you to just
specify this stuff without guards 

static DEFINE_SIMPLE_PM_OPS(ltrf216a_pm_ops, lt..._)

and use

.pm = pm_sleep_ptr(&ltrf216a_pm_ops),

below

and then the compiler can see enough to remove the unused code
if CONFIG_PM_SLEEP isn't defined without the need for messy
__maybe_unused etc.

We are part way through driving this change through IIO and
less of the way along with the rest of the kernel, but it's
a nice cleanup so please apply it to new code like this.

> +
> +static const struct i2c_device_id ltrf216a_id[] = {
> +	{ LTRF216A_DRV_NAME, 0},

Space after 0

> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
> +
> +static const struct of_device_id ltrf216a_of_match[] = {
> +	{ .compatible = "ltr,ltrf216a", },
> +	{ .compatible = "liteon,ltrf216a", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
> +
> +static struct i2c_driver ltrf216a_driver = {
> +	.driver = {
> +		.name = LTRF216A_DRV_NAME,
> +		.pm = LTRF216A_PM_OPS,
> +		.of_match_table = ltrf216a_of_match,
> +	},
> +	.probe		= ltrf216a_probe,
> +	.remove		= ltrf216a_remove,
> +	.id_table	= ltrf216a_id,
> +};
> +
> +module_i2c_driver(ltrf216a_driver);
> +
> +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
> +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

On 27/03/22 20:00, Jonathan Cameron wrote:

Hi Jonathan,

Thanks for your detailed review. I am working on v2 with the modifications
suggested by you.

Just one comment inline.

> On Fri, 25 Mar 2022 16:00:14 +0530
> Shreeya Patel <shreeya.patel@collabora.com> wrote:
>
> Hi Zhigang, Shreeya,
>
> Comments inline.
>
> Thanks,
>
> Jonathan
>> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>>
>> 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@collabora.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
>> ---
>>   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@liteon.com>
>> + *
>> + * 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_

I think the calculation here is to set the default value when the
integration time = 1. In this case, reg value will be 34 (0x22) which
is the default value of ALS_MEAS_RATE register.

I will still confirm it once from Zhigang before sending a v2.

>> +		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);
>
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
>> +{
>> +	int greendata, cleardata, lux;
>> +
>> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
>> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
>> +
>> +	if (greendata < 0 || cleardata < 0)
> I'd rather see the error passed all the way up than eaten here.
>
>> +		lux = 0;
>> +	else
>> +		lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;
> This feels like it would be better reported as
> IIO_VAL_FRACTIONAL;
> Won't make any difference if the only user is sysfs, but if anyone
> ever wants to use the in kernel access to this device they will get
> access to better precision.
>
> Also possible the core handling for IIO_VAL_FRACTIONAL will do a better
> job on retaining precision.
>
>> +
>> +	return lux;
>> +}
>> +
>> +static int ltrf216a_read_raw(struct iio_dev *indio_dev,
>> +			     struct iio_chan_spec const *chan, int *val,
>> +			     int *val2, long mask)
>> +{
>> +	int ret;
>> +	struct ltrf216a_data *data = iio_priv(indio_dev);
>> +
>> +	mutex_lock(&data->mutex);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +		ret = ltrf216a_get_lux(data);
>> +		*val = ret;
>> +		ret = IIO_VAL_INT;
>> +		break;
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		ret = ltrf216a_get_it_time(data, val, val2);
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ltrf216a_write_raw(struct iio_dev *indio_dev,
>> +			      struct iio_chan_spec const *chan, int val,
>> +			      int val2, long mask)
>> +{
>> +	struct ltrf216a_data *data = iio_priv(indio_dev);
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_INT_TIME:
>> +		if (val != 0)
>> +			return -EINVAL;
>> +		mutex_lock(&data->mutex);
>> +		ret = ltrf216a_set_it_time(data, val2);
>> +		mutex_unlock(&data->mutex);
>> +		return ret;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static const struct iio_info ltrf216a_info = {
>> +	.read_raw	= ltrf216a_read_raw,
>> +	.write_raw	= ltrf216a_write_raw,
>> +	.attrs		= &ltrf216a_attribute_group,
>> +};
>> +
>> +static int ltrf216a_probe(struct i2c_client *client,
>> +			  const struct i2c_device_id *id)
> Whilst it's taking a long time to clean out the old
> approach, probe_new() callback preferred for new i2c drivers.
>
>> +{
>> +	struct ltrf216a_data *data;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	i2c_set_clientdata(client, indio_dev);
>> +	data->client = client;
>> +
>> +	mutex_init(&data->mutex);
>> +
>> +	indio_dev->info = &ltrf216a_info;
>> +	indio_dev->name = LTRF216A_DRV_NAME;
>> +	indio_dev->channels = ltrf216a_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(ltrf216a_channels);
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> +	ret = ltrf216a_init(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "ltrf216a chip init failed\n");
>> +		return ret;
> As below, slightly preference for dev_err_probe().
> It doesn't really matter when deferral isn't a possibility but using
> that function ends up slightly neater.
>
>> +	}
> blank line here.  Adding separation so we have
> "function call
> error handler
>
> next thing.."
>
> Just makes things a tiny bit easier to read quickly.
>
>
>> +	data->int_time = 100000;
>> +	data->int_time_fac = 1;
>> +	data->als_gain_fac = 3;
>> +
>> +	ret = iio_device_register(indio_dev);
>> +	if (ret < 0) {
>> +		dev_err(&client->dev, "failed to register iio dev\n");
> Once using devm, this can be
> 		return dev_err_probe(&client->dev, ret,
> 				     "failed to register iio dev\n");
>
> Which is neater but only use this function to print errors for things
> that happen in probe.
>   
>> +		goto err_init;
>> +	}
>> +
>> +	return 0;
>> +err_init:
>> +	ltrf216a_disable(indio_dev);
>> +	return ret;
>> +}
>> +
>> +static int ltrf216a_remove(struct i2c_client *client)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> +	iio_device_unregister(indio_dev);
>> +	ltrf216a_disable(indio_dev);
> As Kryzstof mentioned, use
> devm versions and where they don't exist you can use
> devm_add_action_or_reset() (e.g. for this disable)
>
> That way we don't need error handling or a remove function.
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int ltrf216a_suspend(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> +	return ltrf216a_disable(indio_dev);
>> +}
>> +
>> +static int ltrf216a_resume(struct device *dev)
>> +{
>> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> +
>> +	return ltrf216a_init(indio_dev);
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_suspend, ltrf216a_resume);
>> +#define LTRF216A_PM_OPS (&ltrf216a_pm_ops)
>> +#else
>> +#define LTRF216A_PM_OPS NULL
>> +#endif
> Recently some 'magic' macros were added to pm.h that allow you to just
> specify this stuff without guards
>
> static DEFINE_SIMPLE_PM_OPS(ltrf216a_pm_ops, lt..._)
>
> and use
>
> .pm = pm_sleep_ptr(&ltrf216a_pm_ops),
>
> below
>
> and then the compiler can see enough to remove the unused code
> if CONFIG_PM_SLEEP isn't defined without the need for messy
> __maybe_unused etc.
>
> We are part way through driving this change through IIO and
> less of the way along with the rest of the kernel, but it's
> a nice cleanup so please apply it to new code like this.
>
>> +
>> +static const struct i2c_device_id ltrf216a_id[] = {
>> +	{ LTRF216A_DRV_NAME, 0},
> Space after 0
>
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ltrf216a_id);
>> +
>> +static const struct of_device_id ltrf216a_of_match[] = {
>> +	{ .compatible = "ltr,ltrf216a", },
>> +	{ .compatible = "liteon,ltrf216a", },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
>> +
>> +static struct i2c_driver ltrf216a_driver = {
>> +	.driver = {
>> +		.name = LTRF216A_DRV_NAME,
>> +		.pm = LTRF216A_PM_OPS,
>> +		.of_match_table = ltrf216a_of_match,
>> +	},
>> +	.probe		= ltrf216a_probe,
>> +	.remove		= ltrf216a_remove,
>> +	.id_table	= ltrf216a_id,
>> +};
>> +
>> +module_i2c_driver(ltrf216a_driver);
>> +
>> +MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
>> +MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
>> +MODULE_LICENSE("GPL v2");
>

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Jonathan Cameron]

On Wed, 30 Mar 2022 01:33:19 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> On 27/03/22 20:00, Jonathan Cameron wrote:
> 
> Hi Jonathan,
> 
> Thanks for your detailed review. I am working on v2 with the modifications
> suggested by you.
> 
> Just one comment inline.
> 

...

> >> +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_  
> 
> I think the calculation here is to set the default value when the
> integration time = 1. 

1 isn't a possible value in int_time_available.

I guess you mean 100ms in which case if this were a switch statement

	switch (index) {
	case 0: /* 400msec */
		reg = 0x03;
		data->int_time_fac = 4;
		break;
	case 1: /* 200msec */
		reg = 0x13;
		data->int_time_fac = 2;
		break;
	case 2: /* 100sec */
		reg = 0x22;
		data->int_time_fac = 1;
		break; 
	}

btw from datasheet, 50ms and 25ms also seem possible, why not support them?

Note the switch might be better handled as a constant look up table of appropriate
structures.

> In this case, reg value will be 34 (0x22) which
> is the default value of ALS_MEAS_RATE register.

> 
> I will still confirm it once from Zhigang before sending a v2.
> 
> >> +		data->int_time_fac = 1;
> >> +	}
> >> +
>
Jonathan

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Shreeya Patel]

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@collabora.com> wrote:
>
> Hi Zhigang, Shreeya,
>
> Comments inline.
>
> Thanks,
>
> Jonathan
>> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>>
>> 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@collabora.com>
>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
>> ---
>>   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@liteon.com>
>> + *
>> + * 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

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Gabriel Krisman Bertazi]

Shreeya Patel <shreeya.patel@collabora.com> writes:

>>> +	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.

I guess you could make it a 32-bit buffer, keep the most
significant byte zeroed and return le32_to_cpu:

  u8 buf[4];

  memset(buf, 0x0, sizeof(buf));

  for (i = 0; i < 3; i++) {
 	ret = i2c_smbus_read_byte_data(data->client, addr);
 	if (ret < 0)
 		return ret;
	buf[i] = ret;
  }
  return le32_to_cpu(buf);

-- 
Gabriel Krisman Bertazi

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Jonathan Cameron]

On Tue, 12 Apr 2022 10:06:18 -0400
Gabriel Krisman Bertazi <krisman@collabora.com> wrote:

> Shreeya Patel <shreeya.patel@collabora.com> writes:
> 
> >>> +	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.  
> 
> I guess you could make it a 32-bit buffer, keep the most
> significant byte zeroed and return le32_to_cpu:
> 
>   u8 buf[4];
> 
>   memset(buf, 0x0, sizeof(buf));
> 
>   for (i = 0; i < 3; i++) {
>  	ret = i2c_smbus_read_byte_data(data->client, addr);
>  	if (ret < 0)
>  		return ret;
> 	buf[i] = ret;
>   }
>   return le32_to_cpu(buf);

> 
I was being silly.  It's not aligned for obvious reasons that we don't do
24bit alignment, so you need
get_unaligned_le24()

Jonathan

Re: [PATCH 3/3] iio: light: Add support for ltrf216a sensor

[Gabriel Krisman Bertazi]

Shreeya Patel <shreeya.patel@collabora.com> writes:

> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> Add initial support for ltrf216a ambient light sensor.
>
> Datasheet :-
> https://gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTR-F216A-QT.pdf
> +	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)

ltrf216a_set_int_time instad of it_time?  although, ltr501 also uses
"it" instead of "int" on the function name..

> +
> +static int ltrf216a_get_lux(struct ltrf216a_data *data)
> +{
> +	int greendata, cleardata, lux;
> +
> +	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
> +	cleardata = ltrf216a_read_data(data, LTRF216A_CLEAR_DATA_0);
> +
> +	if (greendata < 0 || cleardata < 0)
> +		lux = 0;
> +	else
> +		lux = greendata * 8 * WIN_FAC / data->als_gain_fac / data->int_time_fac / 10;

This could be rewritten to avoid most of the divisions.

But it also doesn't fit the calculation shown in page 20 on the
datasheet.

I suspect that 8 was calculated from a specific Window Factor (~1.77),
which is specific to one device, but I'm not sure.  The datasheet
formula is:

lux = (ALS_DATA_X * 0.45 * window_factor) / (gain * int_time)

Shouldn't WIN_FAC be a configurable parameter, instead of constant?

-- 
Gabriel Krisman Bertazi