[PATCH v5 0/2] Add LTRF216A Driver

[PATCH v5 0/2] Add LTRF216A Driver

[Shreeya Patel]

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


Changes in v5
  - Add power management support.
  - Add reset functionality.
  - Use readx_poll_timeout() to get data.
  - Cleanup some of the redundant code.
  - Update int_time_fac after I2C write is successful.
  - Rename mutex to lock.
  - Use Reverse Xmas tree pattern for all variable definitions.
  - Improve error handling messages and add error codes.
  - Add one more MODULE_AUTHOR.
  - Remove cleardata which was reading data for infrared light.
  - Remove patch for deprecated vendor prefix [PATCH v4 3/3].
  - Remove deprecated string from DT binding document.

Changes in v4
  - Add more descriptive comment for mutex lock
  - Fix mutex locking in read_raw()
  - Use i2c_smbus_read_i2c_block_data()

Changes in v3
  - Use u16 instead of u8 for int_time_fac
  - Reorder headers in ltrf216a.c file
  - Remove int_time_mapping table and use int_time_available
  - Fix indentation in the bindings file.

Changes in v2
  - Add support for 25ms and 50ms integration time.
  - Rename some of the macros as per names given in datasheet
  - Add a comment for the mutex lock
  - Use read_avail callback instead of attributes and set the
    appropriate _available bit.
  - Use FIELD_PREP() at appropriate places.
  - Add a constant lookup table for integration time and reg val
  - Use BIT() macro for magic numbers.
  - Improve error handling at few places.
  - Use get_unaligned_le24() and div_u64()
  - Use probe_new() callback and devm functions
  - Return errors in probe using dev_err_probe()
  - Use DEFINE_SIMPLE_DEV_PM_OPS()
  - Correct the formula for lux to use 0.45 instead of 0.8
  - Add interrupt and power supply property in DT bindings
  - Add vendor prefix name as per the alphabetical order.


Shreeya Patel (2):
  dt-bindings: Document ltrf216a light sensor bindings
  iio: light: Add support for ltrf216a sensor

 .../bindings/iio/light/liteon,ltrf216a.yaml   |  50 ++
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/ltrf216a.c                  | 441 ++++++++++++++++++
 4 files changed, 502 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
 create mode 100644 drivers/iio/light/ltrf216a.c

-- 
2.30.2

[PATCH v5 1/2] 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>
---
Changes in v5
  - Remove deprecated string 'ltr' from the bindings.

Changes in v3
  - Fix indentation in the example section

Changes in v2
  - Take over the maintainership for the bindings
  - Add interrupt and power supply property in DT bindings

 .../bindings/iio/light/liteon,ltrf216a.yaml   | 50 +++++++++++++++++++
 1 file changed, 50 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..f256ff2e744c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
@@ -0,0 +1,50 @@
+# 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:
+  - Shreeya Patel <shreeya.patel@collabora.com>
+
+description:
+  Ambient light sensing with an i2c interface.
+
+properties:
+  compatible:
+    const:
+      - liteon,ltrf216a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor.
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@53 {
+            compatible = "liteon,ltrf216a";
+            reg = <0x53>;
+            vdd-supply = <&vdd_regulator>;
+            interrupt-parent = <&gpio0>;
+            interrupts = <5 IRQ_TYPE_LEVEL_LOW>;
+        };
+    };
-- 
2.30.2

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

[Rob Herring]

On Wed, 08 Jun 2022 17:05:52 +0530, Shreeya Patel wrote:
> Add devicetree bindings for ltrf216a ambient light sensor.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
> Changes in v5
>   - Remove deprecated string 'ltr' from the bindings.
> 
> Changes in v3
>   - Fix indentation in the example section
> 
> Changes in v2
>   - Take over the maintainership for the bindings
>   - Add interrupt and power supply property in DT bindings
> 
>  .../bindings/iio/light/liteon,ltrf216a.yaml   | 50 +++++++++++++++++++
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml: properties:compatible:const: ['liteon,ltrf216a'] is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml: ignoring, error in schema: properties: compatible: const
Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.example.dtb:0:0: /example-0/i2c/light-sensor@53: failed to match any schema with compatible: ['liteon,ltrf216a']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v5 2/2] 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: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.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>
---
Note :-

This patch generates the below mentioned warnings due to not documenting
the 'ltr' string in vendors-prefix.yaml and liteon,ltrf216a.yaml files.
The thread for the discussion of not documenting 'ltr' as deprecated
prefix can be found here.
https://lore.kernel.org/lkml/20220511094024.175994-2-shreeya.patel@collabora.com/

There are released devices which uses ltr216a light sensor and exposes the
vendor prefix name as 'ltr' through ACPI. Hence, we would like to add
this string under compatible property which would help probe the light sensor
driver.

WARNING: DT compatible string "ltr,ltrf216a" appears un-documented
-- check ./Documentation/devicetree/bindings/
#474: FILE: drivers/iio/light/ltrf216a.c:421:
+	{ .compatible = "ltr,ltrf216a", },

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


Changes in v5
  - Add power management support.
  - Add reset functionality.
  - Use readx_poll_timeout() to get data.
  - Cleanup some of the redundant code.
  - Update int_time_fac after I2C write is successful.
  - Rename mutex to lock.
  - Use Reverse Xmas tree pattern for all variable definitions.
  - Improve error handling messages and add error codes.
  - Add one more MODULE_AUTHOR.
  - Remove cleardata which was reading data for infrared light.

Changes in v4
  - Add more descriptive comment for mutex lock
  - Fix mutex locking in read_raw()
  - Use i2c_smbus_read_i2c_block_data()

Changes in v3
  - Use u16 instead of u8 for int_time_fac
  - Reorder headers in ltrf216a.c file
  - Remove int_time_mapping table and use int_time_available

Changes in v2
  - Add support for 25ms and 50ms integration time.
  - Rename some of the macros as per names given in datasheet
  - Add a comment for the mutex lock
  - Use read_avail callback instead of attributes and set the
    appropriate _available bit.
  - Use FIELD_PREP() at appropriate places.
  - Add a constant lookup table for integration time and reg val
  - Use BIT() macro for magic numbers.
  - Improve error handling at few places.
  - Use get_unaligned_le24() and div_u64()
  - Use probe_new() callback and devm functions
  - Return errors in probe using dev_err_probe()
  - Use DEFINE_SIMPLE_DEV_PM_OPS()
  - Correct the formula for lux to use 0.45 instead of 0.8

 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/ltrf216a.c | 441 +++++++++++++++++++++++++++++++++++
 3 files changed, 452 insertions(+)
 create mode 100644 drivers/iio/light/ltrf216a.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index a62c7b4b8678..6c95431d12f6 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -332,6 +332,16 @@ config LTR501
 	  This driver can also be built as a module.  If so, the module
 	  will be called ltr501.
 
+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 LV0104CS
 	tristate "LV0104CS Ambient Light Sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index d10912faf964..6f23817fae6f 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LTRF216A)		+= ltrf216a.o
 obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_MAX44009)		+= max44009.o
diff --git a/drivers/iio/light/ltrf216a.c b/drivers/iio/light/ltrf216a.c
new file mode 100644
index 000000000000..20a72105645e
--- /dev/null
+++ b/drivers/iio/light/ltrf216a.c
@@ -0,0 +1,441 @@
+// 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/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/iopoll.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <asm/unaligned.h>
+
+#define LTRF216A_DRV_NAME "ltrf216a"
+
+#define LTRF216A_ALS_RESET_MASK         BIT(4)
+#define LTRF216A_ALS_DATA_STATUS	BIT(3)
+#define LTRF216A_ALS_ENABLE_MASK	BIT(1)
+#define LTRF216A_MAIN_CTRL		0x00
+#define LTRF216A_ALS_MEAS_RES		0x04
+#define LTRF216A_MAIN_STATUS		0x07
+#define LTRF216A_CLEAR_DATA_0		0x0A
+#define LTRF216A_ALS_DATA_0		0x0D
+#define LTRF216A_ALS_READ_DATA_DELAY	20000
+
+static const int ltrf216a_int_time_available[][2] = {
+	{0, 400000},
+	{0, 200000},
+	{0, 100000},
+	{0, 50000},
+	{0, 25000},
+};
+
+static const int ltrf216a_int_time_reg[][2] = {
+	{400, 0x03},
+	{200, 0x13},
+	{100, 0x22},
+	{50, 0x31},
+	{25, 0x40},
+};
+
+/* Window Factor is needed when device is under Window glass
+ * with coated tinted ink. This is to compensate the light loss
+ * due to the lower transmission rate of the window glass.
+ */
+#define LTRF216A_WIN_FAC	1
+
+struct ltrf216a_data {
+	struct i2c_client *client;
+	u32 int_time;
+	u16 int_time_fac;
+	u8 als_gain_fac;
+	/*
+	 * Ensure cached value of integration time is consistent
+	 * with hardware setting and remains constant during a
+	 * measurement of Lux.
+	 */
+	struct mutex lock;
+};
+
+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),
+		.info_mask_separate_available =
+			BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
+
+static int ltrf216a_init(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	/* enable sensor */
+	ret |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
+	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);
+	if (ret < 0)
+		dev_err(&data->client->dev,
+			"Error writing to LTRF216A_MAIN_CTRL while enabling the sensor: %d\n", ret);
+
+	return ret;
+}
+
+static void ltrf216a_reset(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int regval = FIELD_PREP(LTRF216A_ALS_RESET_MASK, 1);
+
+	/* reset sensor, chip fails to respond to this, so ignore any errors */
+	i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, regval);
+
+	/* reset time */
+	usleep_range(1000, 2000);
+}
+
+static int ltrf216a_disable(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
+	if (ret < 0)
+		dev_err(&data->client->dev,
+			"Error writing to LTRF216A_MAIN_CTRL while disabling the sensor: %d\n",
+			ret);
+
+	return ret;
+}
+
+static void als_ltrf216a_disable(void *data)
+{
+	struct iio_dev *indio_dev = data;
+
+	ltrf216a_disable(indio_dev);
+}
+
+static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
+{
+	unsigned int i;
+	u8 reg_val;
+	int ret;
+
+	for (i = 0; i < ARRAY_SIZE(ltrf216a_int_time_available); i++) {
+		if (ltrf216a_int_time_available[i][1] == itime)
+			break;
+	}
+	if (i == ARRAY_SIZE(ltrf216a_int_time_available))
+		return -EINVAL;
+
+	reg_val = ltrf216a_int_time_reg[i][1];
+
+	ret = i2c_smbus_write_byte_data(data->client, LTRF216A_ALS_MEAS_RES, reg_val);
+	if (ret < 0) {
+		dev_err(&data->client->dev,
+			"Error writing to LTRF216A_ALS_MEAS_RES: %d\n", ret);
+		return ret;
+	}
+
+	data->int_time_fac = ltrf216a_int_time_reg[i][0];
+	data->int_time = itime;
+
+	return 0;
+}
+
+static int ltrf216a_get_int_time(struct ltrf216a_data *data, int *val, int *val2)
+{
+	*val = 0;
+	*val2 = data->int_time;
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+#ifdef CONFIG_PM
+static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
+{
+	struct device *dev = &data->client->dev;
+	int ret = 0, suspended;
+
+	if (on) {
+		suspended = pm_runtime_suspended(dev);
+		ret = pm_runtime_get_sync(dev);
+
+		/* Allow one integration cycle before allowing a reading */
+		if (suspended)
+			msleep(ltrf216a_int_time_reg[0][0]);
+	} else {
+		pm_runtime_mark_last_busy(dev);
+		ret = pm_runtime_put_autosuspend(dev);
+	}
+
+	return ret;
+}
+#else
+static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
+{
+	return 0;
+}
+#endif
+
+int ltrf216a_check_for_data(struct i2c_client *client)
+{
+	int ret;
+
+	ret = i2c_smbus_read_byte_data(client, LTRF216A_MAIN_STATUS);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read LTRF216A_MAIN_STATUS register: %d\n", ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
+{
+	int ret, val;
+	u8 buf[3];
+
+	ret = readx_poll_timeout(ltrf216a_check_for_data, data->client, val,
+				 val & LTRF216A_ALS_DATA_STATUS, LTRF216A_ALS_READ_DATA_DELAY,
+				 LTRF216A_ALS_READ_DATA_DELAY * 25);
+	if (ret) {
+		dev_err(&data->client->dev, "Timed out waiting for valid data: %d\n", ret);
+		return ret;
+	}
+
+	ret = i2c_smbus_read_i2c_block_data(data->client, addr, sizeof(buf), buf);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "Error reading measurement data: %d\n", ret);
+		return ret;
+	}
+
+	return get_unaligned_le24(&buf[0]);
+}
+
+static int ltrf216a_get_lux(struct ltrf216a_data *data)
+{
+	int greendata;
+	u64 lux, div;
+
+	ltrf216a_set_power_state(data, true);
+
+	greendata = ltrf216a_read_data(data, LTRF216A_ALS_DATA_0);
+	if (greendata < 0)
+		return greendata;
+
+	ltrf216a_set_power_state(data, false);
+
+	lux = greendata * 45 * LTRF216A_WIN_FAC * 100;
+	div = data->als_gain_fac * data->int_time_fac * 100;
+
+	return div_u64(lux, div);
+}
+
+static int ltrf216a_read_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;
+
+	mutex_lock(&data->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = ltrf216a_get_lux(data);
+		if (ret < 0)
+			break;
+		*val = ret;
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = ltrf216a_get_int_time(data, val, val2);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&data->lock);
+
+	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->lock);
+		ret = ltrf216a_set_int_time(data, val2);
+		mutex_unlock(&data->lock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ltrf216a_read_available(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   const int **vals, int *type, int *length,
+				   long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(ltrf216a_int_time_available) * 2;
+		*vals = (const int *)ltrf216a_int_time_available;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ltrf216a_info = {
+	.read_raw	= ltrf216a_read_raw,
+	.write_raw	= ltrf216a_write_raw,
+	.read_avail	= ltrf216a_read_available,
+};
+
+static int ltrf216a_probe(struct i2c_client *client)
+{
+	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->lock);
+
+	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;
+
+	/* reset sensor, chip fails to respond to this, so ignore any errors */
+	ltrf216a_reset(indio_dev);
+
+	ret = pm_runtime_set_active(&client->dev);
+	if (ret)
+		goto error_power_down;
+
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev, 5000);
+	pm_runtime_use_autosuspend(&client->dev);
+
+	ltrf216a_set_power_state(data, true);
+
+	ret = ltrf216a_init(indio_dev);
+	if (ret < 0) {
+		dev_err_probe(&client->dev, ret, "ltrf216a chip init failed\n");
+		goto error_power_down;
+	}
+
+	data->int_time = 100000;
+	data->int_time_fac = 100;
+	data->als_gain_fac = 3;
+
+	ret = devm_add_action_or_reset(&client->dev, als_ltrf216a_disable, indio_dev);
+	if (ret < 0)
+		goto error_power_down;
+
+	ret = devm_iio_device_register(&client->dev, indio_dev);
+	if (ret)
+		goto error_power_down;
+
+error_power_down:
+	ltrf216a_set_power_state(data, false);
+
+	return ret;
+}
+
+static int ltrf216a_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	ltrf216a_disable(indio_dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int ltrf216a_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return ltrf216a_disable(indio_dev);
+}
+
+static int ltrf216a_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return ltrf216a_init(indio_dev);
+}
+#endif
+
+static const struct dev_pm_ops ltrf216a_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
+			   ltrf216a_runtime_resume, NULL)
+};
+
+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 = "liteon,ltrf216a", },
+	{ .compatible = "ltr,ltrf216a", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, ltrf216a_of_match);
+
+static struct i2c_driver ltrf216a_driver = {
+	.driver = {
+		.name = LTRF216A_DRV_NAME,
+		.pm = pm_sleep_ptr(&ltrf216a_pm_ops),
+		.of_match_table = ltrf216a_of_match,
+	},
+	.probe_new	= ltrf216a_probe,
+	.remove         = ltrf216a_remove,
+	.id_table	= ltrf216a_id,
+};
+module_i2c_driver(ltrf216a_driver);
+
+MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
+MODULE_AUTHOR("Shreeya Patel <shreeya.patel@collabora.com>");
+MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

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

[Andy Shevchenko]

On Wed, Jun 8, 2022 at 1:37 PM Shreeya Patel
<shreeya.patel@collabora.com> wrote:
>
> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>
> Add initial support for ltrf216a ambient light sensor.
>
> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf

https?

...

> +#define LTRF216A_ALS_READ_DATA_DELAY   20000

What units?

...

> +/* Window Factor is needed when device is under Window glass

the device

> + * with coated tinted ink. This is to compensate the light loss

for the?

> + * due to the lower transmission rate of the window glass.
> + */

/*
 * Multi-line comments should look
 * like this very example. Find the difference.
 */

...

> +static int ltrf216a_init(struct iio_dev *indio_dev)
> +{
> +       struct ltrf216a_data *data = iio_priv(indio_dev);
> +       int ret = 0;

Useless assignment.

> +
> +       /* enable sensor */
> +       ret |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);

This is bad code. Use another variable with distinguashable name.

> +       ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);

Can this driver utilize regmap I2C?

> +       if (ret < 0)
> +               dev_err(&data->client->dev,
> +                       "Error writing to LTRF216A_MAIN_CTRL while enabling the sensor: %d\n", ret);
> +
> +       return ret;
> +}

...

> +static int ltrf216a_disable(struct iio_dev *indio_dev)
> +{
> +       struct ltrf216a_data *data = iio_priv(indio_dev);
> +       int ret = 0;

Useless assignment.

> +       ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
> +       if (ret < 0)

> +               dev_err(&data->client->dev,
> +                       "Error writing to LTRF216A_MAIN_CTRL while disabling the sensor: %d\n",
> +                       ret);

With a temporary variable for the device this may be located on one line.
Same for the similar cases.

> +       return ret;
> +}

...

> +#ifdef CONFIG_PM

Why? Can't it be hidden by using pm_sleep_ptr() or alike?

> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
> +{
> +       struct device *dev = &data->client->dev;
> +       int ret = 0, suspended;

Useless assignment. Please, go thru all your code and drop these
potentially dangerous assignments.

> +
> +       if (on) {
> +               suspended = pm_runtime_suspended(dev);
> +               ret = pm_runtime_get_sync(dev);
> +
> +               /* Allow one integration cycle before allowing a reading */
> +               if (suspended)
> +                       msleep(ltrf216a_int_time_reg[0][0]);
> +       } else {
> +               pm_runtime_mark_last_busy(dev);
> +               ret = pm_runtime_put_autosuspend(dev);
> +       }
> +
> +       return ret;
> +}
> +#else
> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
> +{
> +       return 0;
> +}
> +#endif
> +
> +int ltrf216a_check_for_data(struct i2c_client *client)
> +{
> +       int ret;
> +
> +       ret = i2c_smbus_read_byte_data(client, LTRF216A_MAIN_STATUS);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "Failed to read LTRF216A_MAIN_STATUS register: %d\n", ret);

> +               return ret;

Dup.

> +       }
> +
> +       return ret;
> +}

...

> +#ifdef CONFIG_PM_SLEEP

Oh, please no.

> +#endif

...

> +static const struct dev_pm_ops ltrf216a_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +       SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
> +                          ltrf216a_runtime_resume, NULL)
> +};

Use pm_sleep_ptr() and corresponding top-level macros.

-- 
With Best Regards,
Andy Shevchenko

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

[Shreeya Patel]

On 08/06/22 21:46, Andy Shevchenko wrote:
> On Wed, Jun 8, 2022 at 1:37 PM Shreeya Patel
> <shreeya.patel@collabora.com> wrote:
>> From: Zhigang Shi <Zhigang.Shi@liteon.com>
>>
>> Add initial support for ltrf216a ambient light sensor.
>>
>> Datasheet: gitlab.steamos.cloud/shreeya/iio/-/blob/main/LTRF216A.pdf
> https?
>
> ...
>
>> +#define LTRF216A_ALS_READ_DATA_DELAY   20000
> What units?
>
> ...
>
>> +/* Window Factor is needed when device is under Window glass
> the device
>
>> + * with coated tinted ink. This is to compensate the light loss
> for the?
>
>> + * due to the lower transmission rate of the window glass.
>> + */
> /*
>   * Multi-line comments should look
>   * like this very example. Find the difference.
>   */
>
> ...
>
>> +static int ltrf216a_init(struct iio_dev *indio_dev)
>> +{
>> +       struct ltrf216a_data *data = iio_priv(indio_dev);
>> +       int ret = 0;
> Useless assignment.
>
>> +
>> +       /* enable sensor */
>> +       ret |= FIELD_PREP(LTRF216A_ALS_ENABLE_MASK, 1);
> This is bad code. Use another variable with distinguashable name.
>
>> +       ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, ret);
> Can this driver utilize regmap I2C?

Thanks for all your comments and yes we can use the regmap I2C
but the plan is to get the basic version merged and then I'll be sending
patches for any enhancements that we'd like to do.



Thanks,
Shreeya Patel

>
>> +       if (ret < 0)
>> +               dev_err(&data->client->dev,
>> +                       "Error writing to LTRF216A_MAIN_CTRL while enabling the sensor: %d\n", ret);
>> +
>> +       return ret;
>> +}
> ...
>
>> +static int ltrf216a_disable(struct iio_dev *indio_dev)
>> +{
>> +       struct ltrf216a_data *data = iio_priv(indio_dev);
>> +       int ret = 0;
> Useless assignment.
>
>> +       ret = i2c_smbus_write_byte_data(data->client, LTRF216A_MAIN_CTRL, 0);
>> +       if (ret < 0)
>> +               dev_err(&data->client->dev,
>> +                       "Error writing to LTRF216A_MAIN_CTRL while disabling the sensor: %d\n",
>> +                       ret);
> With a temporary variable for the device this may be located on one line.
> Same for the similar cases.
>
>> +       return ret;
>> +}
> ...
>
>> +#ifdef CONFIG_PM
> Why? Can't it be hidden by using pm_sleep_ptr() or alike?
>
>> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
>> +{
>> +       struct device *dev = &data->client->dev;
>> +       int ret = 0, suspended;
> Useless assignment. Please, go thru all your code and drop these
> potentially dangerous assignments.
>
>> +
>> +       if (on) {
>> +               suspended = pm_runtime_suspended(dev);
>> +               ret = pm_runtime_get_sync(dev);
>> +
>> +               /* Allow one integration cycle before allowing a reading */
>> +               if (suspended)
>> +                       msleep(ltrf216a_int_time_reg[0][0]);
>> +       } else {
>> +               pm_runtime_mark_last_busy(dev);
>> +               ret = pm_runtime_put_autosuspend(dev);
>> +       }
>> +
>> +       return ret;
>> +}
>> +#else
>> +static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
>> +{
>> +       return 0;
>> +}
>> +#endif
>> +
>> +int ltrf216a_check_for_data(struct i2c_client *client)
>> +{
>> +       int ret;
>> +
>> +       ret = i2c_smbus_read_byte_data(client, LTRF216A_MAIN_STATUS);
>> +       if (ret < 0) {
>> +               dev_err(&client->dev, "Failed to read LTRF216A_MAIN_STATUS register: %d\n", ret);
>> +               return ret;
> Dup.
>
>> +       }
>> +
>> +       return ret;
>> +}
> ...
>
>> +#ifdef CONFIG_PM_SLEEP
> Oh, please no.
>
>> +#endif
> ...
>
>> +static const struct dev_pm_ops ltrf216a_pm_ops = {
>> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> +                               pm_runtime_force_resume)
>> +       SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
>> +                          ltrf216a_runtime_resume, NULL)
>> +};
> Use pm_sleep_ptr() and corresponding top-level macros.
>

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

[kernel test robot]

Hi Shreeya,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on v5.19-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Shreeya-Patel/Add-LTRF216A-Driver/20220608-194016
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20220609/202206090943.Zvfv6OBd-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project b92436efcb7813fc481b30f2593a4907568d917a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4809a7f42af19c3da77457e1aaf37ddd171fa779
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Shreeya-Patel/Add-LTRF216A-Driver/20220608-194016
        git checkout 4809a7f42af19c3da77457e1aaf37ddd171fa779
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/iio/adc/ drivers/iio/light/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/iio/light/ltrf216a.c:193:5: warning: no previous prototype for function 'ltrf216a_check_for_data' [-Wmissing-prototypes]
   int ltrf216a_check_for_data(struct i2c_client *client)
       ^
   drivers/iio/light/ltrf216a.c:193:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int ltrf216a_check_for_data(struct i2c_client *client)
   ^
   static 
   1 warning generated.


vim +/ltrf216a_check_for_data +193 drivers/iio/light/ltrf216a.c

   192	
 > 193	int ltrf216a_check_for_data(struct i2c_client *client)
   194	{
   195		int ret;
   196	
   197		ret = i2c_smbus_read_byte_data(client, LTRF216A_MAIN_STATUS);
   198		if (ret < 0) {
   199			dev_err(&client->dev, "Failed to read LTRF216A_MAIN_STATUS register: %d\n", ret);
   200			return ret;
   201		}
   202	
   203		return ret;
   204	}
   205	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Shreeya,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on v5.19-rc1 next-20220608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Shreeya-Patel/Add-LTRF216A-Driver/20220608-194016
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20220609/202206091041.MeYKMR6P-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/4809a7f42af19c3da77457e1aaf37ddd171fa779
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Shreeya-Patel/Add-LTRF216A-Driver/20220608-194016
        git checkout 4809a7f42af19c3da77457e1aaf37ddd171fa779
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/iio/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

>> drivers/iio/light/ltrf216a.c:193:5: warning: no previous prototype for 'ltrf216a_check_for_data' [-Wmissing-prototypes]
     193 | int ltrf216a_check_for_data(struct i2c_client *client)
         |     ^~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/device.h:25,
                    from include/linux/acpi.h:15,
                    from include/linux/i2c.h:13,
                    from drivers/iio/light/ltrf216a.c:14:
>> drivers/iio/light/ltrf216a.c:409:28: error: 'ltrf216a_runtime_suspend' undeclared here (not in a function); did you mean 'pm_runtime_suspend'?
     409 |         SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
         |                            ^~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/pm.h:329:28: note: in definition of macro 'RUNTIME_PM_OPS'
     329 |         .runtime_suspend = suspend_fn, \
         |                            ^~~~~~~~~~
   drivers/iio/light/ltrf216a.c:409:9: note: in expansion of macro 'SET_RUNTIME_PM_OPS'
     409 |         SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
         |         ^~~~~~~~~~~~~~~~~~
>> drivers/iio/light/ltrf216a.c:410:28: error: 'ltrf216a_runtime_resume' undeclared here (not in a function); did you mean 'ltrf216a_int_time_reg'?
     410 |                            ltrf216a_runtime_resume, NULL)
         |                            ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/pm.h:330:27: note: in definition of macro 'RUNTIME_PM_OPS'
     330 |         .runtime_resume = resume_fn, \
         |                           ^~~~~~~~~
   drivers/iio/light/ltrf216a.c:409:9: note: in expansion of macro 'SET_RUNTIME_PM_OPS'
     409 |         SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
         |         ^~~~~~~~~~~~~~~~~~


vim +409 drivers/iio/light/ltrf216a.c

   405	
   406	static const struct dev_pm_ops ltrf216a_pm_ops = {
   407		SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
   408					pm_runtime_force_resume)
 > 409		SET_RUNTIME_PM_OPS(ltrf216a_runtime_suspend,
 > 410				   ltrf216a_runtime_resume, NULL)
   411	};
   412	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v5 0/2] Add LTRF216A Driver

[Jonathan Cameron]

On Wed,  8 Jun 2022 17:05:51 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> This patchset adds support for ltrf216a Ambient Light Sensor
> and documents the DT bindings for the same.
> 
As Andy already gave some valuable feedback and you have
a few autobuilder checks to fix up (plus I'm low on time this week)
I'll wait for v6 before taking another look.

Thanks,

Jonathan

> 
> Changes in v5
>   - Add power management support.
>   - Add reset functionality.
>   - Use readx_poll_timeout() to get data.
>   - Cleanup some of the redundant code.
>   - Update int_time_fac after I2C write is successful.
>   - Rename mutex to lock.
>   - Use Reverse Xmas tree pattern for all variable definitions.
>   - Improve error handling messages and add error codes.
>   - Add one more MODULE_AUTHOR.
>   - Remove cleardata which was reading data for infrared light.
>   - Remove patch for deprecated vendor prefix [PATCH v4 3/3].
>   - Remove deprecated string from DT binding document.
> 
> Changes in v4
>   - Add more descriptive comment for mutex lock
>   - Fix mutex locking in read_raw()
>   - Use i2c_smbus_read_i2c_block_data()
> 
> Changes in v3
>   - Use u16 instead of u8 for int_time_fac
>   - Reorder headers in ltrf216a.c file
>   - Remove int_time_mapping table and use int_time_available
>   - Fix indentation in the bindings file.
> 
> Changes in v2
>   - Add support for 25ms and 50ms integration time.
>   - Rename some of the macros as per names given in datasheet
>   - Add a comment for the mutex lock
>   - Use read_avail callback instead of attributes and set the
>     appropriate _available bit.
>   - Use FIELD_PREP() at appropriate places.
>   - Add a constant lookup table for integration time and reg val
>   - Use BIT() macro for magic numbers.
>   - Improve error handling at few places.
>   - Use get_unaligned_le24() and div_u64()
>   - Use probe_new() callback and devm functions
>   - Return errors in probe using dev_err_probe()
>   - Use DEFINE_SIMPLE_DEV_PM_OPS()
>   - Correct the formula for lux to use 0.45 instead of 0.8
>   - Add interrupt and power supply property in DT bindings
>   - Add vendor prefix name as per the alphabetical order.
> 
> 
> Shreeya Patel (2):
>   dt-bindings: Document ltrf216a light sensor bindings
>   iio: light: Add support for ltrf216a sensor
> 
>  .../bindings/iio/light/liteon,ltrf216a.yaml   |  50 ++
>  drivers/iio/light/Kconfig                     |  10 +
>  drivers/iio/light/Makefile                    |   1 +
>  drivers/iio/light/ltrf216a.c                  | 441 ++++++++++++++++++
>  4 files changed, 502 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
>  create mode 100644 drivers/iio/light/ltrf216a.c
>