[PATCH v9 0/2] Add LTRF216A Driver

[PATCH v9 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 v9
  - Add LTRF216A_MAIN_STATUS register in volatile function.
  - Update the datasheet link.

Changes in v8
  - Add caching mechanism to restore register state after h/w resume.
  - Add callback functions and disable locking in regmap config.
  - Update mutex comment as per it's current scope in the driver.
  - Add Shreeya as author of the driver.
  - Make some minor cleanups.

Changes in v7
  - Add regmap support.
  - Fix runtime power management implementation.
  - Fix the ordering of devm and non-devm functions.
  - Use DEFINE_RUNTIME_DEV_PM_OPS macro
  - Fix the error reported by kernel test robot for bindings.

Changes in v6
  - Fix some errors reported by kernel test robot.
  - Add protocol details for the datasheet link.
  - Remove useless assignments.
  - Add unit details for read data delay macro.
  - Use pm_sleep_ptr().

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   |  49 ++
 drivers/iio/light/Kconfig                     |  11 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/ltrf216a.c                  | 523 ++++++++++++++++++
 4 files changed, 584 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
 create mode 100644 drivers/iio/light/ltrf216a.c

-- 
2.30.2

[PATCH v9 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 v7
  - Fix the error reported by kernel test robot.

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   | 49 +++++++++++++++++++
 1 file changed, 49 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..7de1b0e721ca
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
@@ -0,0 +1,49 @@
+# 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

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

[Shreeya Patel]

Add initial support for ltrf216a ambient light sensor.

Datasheet: https://gitlab.collabora.com/shreeya/iio/-/blob/master/LTRF216A.pdf
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Co-developed-by: Zhigang Shi <Zhigang.Shi@liteon.com>
Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
Co-developed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
Signed-off-by: Shreeya Patel <shreeya.patel@collabora.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 v9
  - Add LTRF216A_MAIN_STATUS register in volatile function.
  - Update the datasheet link.

Changes in v8
  - Add caching mechanism to restore register state after h/w resume.
  - Add callback functions and disable locking in regmap config.
  - Update mutex comment as per it's current scope in the driver.
  - Add Shreeya as author of the driver.
  - Make some minor cleanups.

Changes in v7
  - Add regmap support.
  - Fix runtime power management implementation.
  - Fix the ordering of devm and non-devm functions.
  - Use DEFINE_RUNTIME_DEV_PM_OPS macro

Changes in v6
  - Fix some errors reported by kernel test robot.
  - Add protocol details for the datasheet link.
  - Remove useless assignments.
  - Add unit details for read data delay macro.
  - Use pm_sleep_ptr().

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    |  11 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/ltrf216a.c | 523 +++++++++++++++++++++++++++++++++++
 3 files changed, 535 insertions(+)
 create mode 100644 drivers/iio/light/ltrf216a.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 8537e88f02e3..7cf6e8490123 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -331,6 +331,17 @@ 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
+	select REGMAP_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..22916eea97b8
--- /dev/null
+++ b/drivers/iio/light/ltrf216a.c
@@ -0,0 +1,523 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * LTRF216A Ambient Light Sensor
+ *
+ * Copyright (C) 2022 Collabora, Ltd.
+ * Author: Shreeya Patel <shreeya.patel@collabora.com>
+ *
+ * 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/regmap.h>
+
+#include <linux/iio/iio.h>
+
+#include <asm/unaligned.h>
+
+#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_ALS_GAIN		0x05
+#define LTRF216A_PART_ID		0x06
+#define LTRF216A_MAIN_STATUS		0x07
+#define LTRF216A_ALS_CLEAR_DATA_0	0x0a
+#define LTRF216A_ALS_CLEAR_DATA_1	0x0b
+#define LTRF216A_ALS_CLEAR_DATA_2	0x0c
+#define LTRF216A_ALS_DATA_0		0x0d
+#define LTRF216A_ALS_DATA_1		0x0e
+#define LTRF216A_ALS_DATA_2		0x0f
+#define LTRF216A_INT_CFG		0x19
+#define LTRF216A_INT_PST		0x1a
+#define LTRF216A_ALS_THRES_UP_0		0x21
+#define LTRF216A_ALS_THRES_UP_1		0x22
+#define LTRF216A_ALS_THRES_UP_2		0x23
+#define LTRF216A_ALS_THRES_LOW_0	0x24
+#define LTRF216A_ALS_THRES_LOW_1	0x25
+#define LTRF216A_ALS_THRES_LOW_2	0x26
+#define LTRF216A_ALS_READ_DATA_DELAY_US	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 the device is under Window glass
+ * with coated tinted ink. This is to compensate for the light loss
+ * due to the lower transmission rate of the window glass and helps
+ * in calculating lux.
+ */
+#define LTRF216A_WIN_FAC	1
+
+struct ltrf216a_data {
+	struct regmap *regmap;
+	struct i2c_client *client;
+	u32 int_time;
+	u16 int_time_fac;
+	u8 als_gain_fac;
+	/*
+	 * Protects regmap accesses and makes sure integration time
+	 * remains constant during the 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 void ltrf216a_reset(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+
+	/* reset sensor, chip fails to respond to this, so ignore any errors */
+	regmap_write(data->regmap, LTRF216A_MAIN_CTRL, LTRF216A_ALS_RESET_MASK);
+
+	/* reset time */
+	usleep_range(1000, 2000);
+}
+
+static int ltrf216a_enable(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	/* enable sensor */
+	ret = regmap_set_bits(data->regmap,
+			      LTRF216A_MAIN_CTRL, LTRF216A_ALS_ENABLE_MASK);
+	if (ret) {
+		dev_err(dev, "failed to enable sensor: %d\n", ret);
+		return ret;
+	}
+
+	/* sleep for one integration cycle after enabling the device */
+	msleep(ltrf216a_int_time_reg[0][0]);
+
+	return 0;
+}
+
+static int ltrf216a_disable(struct iio_dev *indio_dev)
+{
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	ret = regmap_write(data->regmap, LTRF216A_MAIN_CTRL, 0);
+	if (ret)
+		dev_err(dev, "failed to disable sensor: %d\n", ret);
+
+	return ret;
+}
+
+static int ltrf216a_set_int_time(struct ltrf216a_data *data, int itime)
+{
+	struct device *dev = &data->client->dev;
+	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 = regmap_write(data->regmap, LTRF216A_ALS_MEAS_RES, reg_val);
+	if (ret) {
+		dev_err(dev, "failed to set integration time: %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;
+}
+
+static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
+{
+	struct device *dev = &data->client->dev;
+	int ret;
+
+	if (on) {
+		ret = pm_runtime_resume_and_get(dev);
+		if (ret) {
+			dev_err(dev, "failed to resume runtime PM: %d\n", ret);
+			return ret;
+		}
+	} else {
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+	}
+
+	return ret;
+}
+
+static int ltrf216a_read_data(struct ltrf216a_data *data, u8 addr)
+{
+	struct device *dev = &data->client->dev;
+	int ret, val;
+	u8 buf[3];
+
+	ret = regmap_read_poll_timeout(data->regmap, LTRF216A_MAIN_STATUS,
+				       val, val & LTRF216A_ALS_DATA_STATUS,
+				       LTRF216A_ALS_READ_DATA_DELAY_US,
+				       LTRF216A_ALS_READ_DATA_DELAY_US * 50);
+	if (ret) {
+		dev_err(dev, "failed to wait for measurement data: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_bulk_read(data->regmap, addr, buf, sizeof(buf));
+	if (ret) {
+		dev_err(dev, "failed to read measurement data: %d\n", ret);
+		return ret;
+	}
+
+	return get_unaligned_le24(&buf[0]);
+}
+
+static int ltrf216a_get_lux(struct ltrf216a_data *data)
+{
+	int ret, greendata;
+	u64 lux, div;
+
+	ret = ltrf216a_set_power_state(data, true);
+	if (ret)
+		return ret;
+
+	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;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->lock);
+		ret = ltrf216a_get_lux(data);
+		mutex_unlock(&data->lock);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_INT_TIME:
+		mutex_lock(&data->lock);
+		ret = ltrf216a_get_int_time(data, val, val2);
+		mutex_unlock(&data->lock);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+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 bool ltrf216a_readable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTRF216A_MAIN_CTRL:
+	case LTRF216A_ALS_MEAS_RES:
+	case LTRF216A_ALS_GAIN:
+	case LTRF216A_PART_ID:
+	case LTRF216A_MAIN_STATUS:
+	case LTRF216A_ALS_CLEAR_DATA_0:
+	case LTRF216A_ALS_CLEAR_DATA_1:
+	case LTRF216A_ALS_CLEAR_DATA_2:
+	case LTRF216A_ALS_DATA_0:
+	case LTRF216A_ALS_DATA_1:
+	case LTRF216A_ALS_DATA_2:
+	case LTRF216A_INT_CFG:
+	case LTRF216A_INT_PST:
+	case LTRF216A_ALS_THRES_UP_0:
+	case LTRF216A_ALS_THRES_UP_1:
+	case LTRF216A_ALS_THRES_UP_2:
+	case LTRF216A_ALS_THRES_LOW_0:
+	case LTRF216A_ALS_THRES_LOW_1:
+	case LTRF216A_ALS_THRES_LOW_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltrf216a_writable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTRF216A_MAIN_CTRL:
+	case LTRF216A_ALS_MEAS_RES:
+	case LTRF216A_ALS_GAIN:
+	case LTRF216A_INT_CFG:
+	case LTRF216A_INT_PST:
+	case LTRF216A_ALS_THRES_UP_0:
+	case LTRF216A_ALS_THRES_UP_1:
+	case LTRF216A_ALS_THRES_UP_2:
+	case LTRF216A_ALS_THRES_LOW_0:
+	case LTRF216A_ALS_THRES_LOW_1:
+	case LTRF216A_ALS_THRES_LOW_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltrf216a_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LTRF216A_MAIN_STATUS:
+	case LTRF216A_ALS_CLEAR_DATA_0:
+	case LTRF216A_ALS_CLEAR_DATA_1:
+	case LTRF216A_ALS_CLEAR_DATA_2:
+	case LTRF216A_ALS_DATA_0:
+	case LTRF216A_ALS_DATA_1:
+	case LTRF216A_ALS_DATA_2:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static bool ltrf216a_precious_reg(struct device *dev, unsigned int reg)
+{
+	return reg == LTRF216A_MAIN_STATUS;
+}
+
+static const struct regmap_config ltrf216a_regmap_config = {
+	.name = "ltrf216a",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = LTRF216A_ALS_THRES_LOW_2,
+	.readable_reg = ltrf216a_readable_reg,
+	.writeable_reg = ltrf216a_writable_reg,
+	.volatile_reg = ltrf216a_volatile_reg,
+	.precious_reg = ltrf216a_precious_reg,
+	.disable_locking = true,
+};
+
+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);
+
+	data->regmap = devm_regmap_init_i2c(client, &ltrf216a_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+				     "regmap initialization failed\n");
+
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	mutex_init(&data->lock);
+
+	indio_dev->info = &ltrf216a_info;
+	indio_dev->name = "ltrf216a";
+	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 = regmap_reinit_cache(data->regmap, &ltrf216a_regmap_config);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to reinit regmap cache\n");
+
+	ret = devm_pm_runtime_enable(&client->dev);
+	if (ret)
+		return dev_err_probe(&client->dev, ret,
+				     "failed to enable runtime PM\n");
+
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+
+	if (!IS_ENABLED(CONFIG_PM)) {
+		ret = ltrf216a_enable(indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	data->int_time = 100000;
+	data->int_time_fac = 100;
+	data->als_gain_fac = 3;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static int ltrf216a_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = ltrf216a_disable(indio_dev);
+	if (ret)
+		return ret;
+
+	regcache_cache_only(data->regmap, true);
+
+	return 0;
+}
+
+static int ltrf216a_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct ltrf216a_data *data = iio_priv(indio_dev);
+	int ret;
+
+	regcache_cache_only(data->regmap, false);
+	regcache_mark_dirty(data->regmap);
+	ret = regcache_sync(data->regmap);
+	if (ret)
+		goto cache_only;
+
+	ret = ltrf216a_enable(indio_dev);
+	if (ret)
+		goto cache_only;
+
+	return 0;
+
+cache_only:
+	regcache_cache_only(data->regmap, true);
+
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(ltrf216a_pm_ops, ltrf216a_runtime_suspend,
+				 ltrf216a_runtime_resume, NULL);
+
+static const struct i2c_device_id ltrf216a_id[] = {
+	{ "ltrf216a" },
+	{}
+};
+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",
+		.pm = pm_ptr(&ltrf216a_pm_ops),
+		.of_match_table = ltrf216a_of_match,
+	},
+	.probe_new = ltrf216a_probe,
+	.id_table = ltrf216a_id,
+};
+module_i2c_driver(ltrf216a_driver);
+
+MODULE_AUTHOR("Shreeya Patel <shreeya.patel@collabora.com>");
+MODULE_AUTHOR("Shi Zhigang <Zhigang.Shi@liteon.com>");
+MODULE_DESCRIPTION("LTRF216A ambient light sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.30.2

Re: [PATCH v9 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 linus/master v5.19-rc6 next-20220715]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shreeya-Patel/Add-LTRF216A-Driver/20220716-210741
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: s390-randconfig-r023-20220717 (https://download.01.org/0day-ci/archive/20220717/202207170630.lcfpHu8v-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 07022e6cf9b5b3baa642be53d0b3c3f1c403dbfd)
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
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/6db354952f0013dd461d2338761ef8ae3399f2d8
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Shreeya-Patel/Add-LTRF216A-Driver/20220716-210741
        git checkout 6db354952f0013dd461d2338761ef8ae3399f2d8
        # 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=s390 SHELL=/bin/bash 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 >>):

   In file included from drivers/iio/light/ltrf216a.c:19:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from drivers/iio/light/ltrf216a.c:19:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from drivers/iio/light/ltrf216a.c:19:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
>> drivers/iio/light/ltrf216a.c:188:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
           if (on) {
               ^~
   drivers/iio/light/ltrf216a.c:199:9: note: uninitialized use occurs here
           return ret;
                  ^~~
   drivers/iio/light/ltrf216a.c:188:2: note: remove the 'if' if its condition is always true
           if (on) {
           ^~~~~~~~
   drivers/iio/light/ltrf216a.c:186:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   13 warnings generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for DRM_DP_AUX_BUS
   Depends on HAS_IOMEM && DRM && OF
   Selected by
   - DRM_MSM && HAS_IOMEM && DRM && (ARCH_QCOM || SOC_IMX5 || COMPILE_TEST && COMMON_CLK && IOMMU_SUPPORT && (QCOM_OCMEM || QCOM_OCMEM && (QCOM_LLCC || QCOM_LLCC && (QCOM_COMMAND_DB || QCOM_COMMAND_DB


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

   182	
   183	static int ltrf216a_set_power_state(struct ltrf216a_data *data, bool on)
   184	{
   185		struct device *dev = &data->client->dev;
   186		int ret;
   187	
 > 188		if (on) {
   189			ret = pm_runtime_resume_and_get(dev);
   190			if (ret) {
   191				dev_err(dev, "failed to resume runtime PM: %d\n", ret);
   192				return ret;
   193			}
   194		} else {
   195			pm_runtime_mark_last_busy(dev);
   196			pm_runtime_put_autosuspend(dev);
   197		}
   198	
   199		return ret;
   200	}
   201	

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

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

[Jonathan Cameron]

On Fri, 15 Jul 2022 16:46:26 +0530
Shreeya Patel <shreeya.patel@collabora.com> wrote:

> Add initial support for ltrf216a ambient light sensor.
> 
> Datasheet: https://gitlab.collabora.com/shreeya/iio/-/blob/master/LTRF216A.pdf
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Co-developed-by: Zhigang Shi <Zhigang.Shi@liteon.com>
> Signed-off-by: Zhigang Shi <Zhigang.Shi@liteon.com>
> Co-developed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>


The first of the two 0-day warnings confuses me. It might be spurious due
to some unrelated issue, but I'm not certain of that...

Otherwise, a few more comments around PM. The way you've done it isn't
something I've seen before + I think you leave the device powered up in
!CONFIG_PM after remove which isn't ideal.

Thanks,

Jonathan


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

This one was caught by 0-day.  ret = 0; or perhaps better, just return
directly in the two branches rather than having a single exit point.

> +
> +	if (on) {
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret) {
> +			dev_err(dev, "failed to resume runtime PM: %d\n", ret);
> +			return ret;
> +		}
> +	} else {
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +	}
> +
> +	return ret;
> +}
> +


> +
> +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);
> +
> +	data->regmap = devm_regmap_init_i2c(client, &ltrf216a_regmap_config);
> +	if (IS_ERR(data->regmap))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> +				     "regmap initialization failed\n");
> +
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	mutex_init(&data->lock);
> +
> +	indio_dev->info = &ltrf216a_info;
> +	indio_dev->name = "ltrf216a";
> +	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 = regmap_reinit_cache(data->regmap, &ltrf216a_regmap_config);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to reinit regmap cache\n");
> +
> +	ret = devm_pm_runtime_enable(&client->dev);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret,
> +				     "failed to enable runtime PM\n");
> +
> +	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
> +	if (!IS_ENABLED(CONFIG_PM)) {
> +		ret = ltrf216a_enable(indio_dev);

What turns this off again?  I'd expect to see a devm_add_action_or_reset()
to do that in the !CONFIG_PM case.

This is also an unusual pattern. As far as I can tell it works.
Normal trick for ensuring !CONFIG_PM works is to:

1) Unconditionally turn device on.
2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
   due to runtime pm.
3) Then call pm_runtime_set_active() so the state tracking matches.
4) Call 	
  pm_runtime_mark_last_busy(dev);
  pm_runtime_put_autosuspend(dev);
  (here you have a function to do this anyway)
  to let runtime_pm use same path as normal to autosuspend

the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
is left turned on.  Is there something I'm missing that makes that cycle
inappropriate here?  The main reason to do this is it then looks exactly
like any other runtime_pm calls elsewhere in the driver, so easier to review.


> +		if (ret)
> +			return ret;
> +	}
> +
> +	data->int_time = 100000;
> +	data->int_time_fac = 100;
> +	data->als_gain_fac = 3;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +

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

[Dmitry Osipenko]

On 7/18/22 20:25, Jonathan Cameron wrote:
> What turns this off again?  I'd expect to see a devm_add_action_or_reset()
> to do that in the !CONFIG_PM case.
> 
> This is also an unusual pattern. As far as I can tell it works.
> Normal trick for ensuring !CONFIG_PM works is to:
> 
> 1) Unconditionally turn device on.
> 2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
>    due to runtime pm.

If CONFIG_PM is disabled, do we really need to care about the power
management on removal?

> 3) Then call pm_runtime_set_active() so the state tracking matches.

We can add pm_runtime_set_active() before h/w is touched for more
consistency. On Steam Deck supplies are always enabled, but this may be
not true for other devices.

> 4) Call 	
>   pm_runtime_mark_last_busy(dev);
>   pm_runtime_put_autosuspend(dev);
>   (here you have a function to do this anyway)
>   to let runtime_pm use same path as normal to autosuspend
> 
> the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
> is left turned on.  Is there something I'm missing that makes that cycle
> inappropriate here?  The main reason to do this is it then looks exactly
> like any other runtime_pm calls elsewhere in the driver, so easier to review.

It's appropriate, although caring about PM when it's disabled in kernel
config could be unnecessary, IMO. It was my suggestion to keep the h/w
enabled on driver's removal with !CONFIG_PM, minimizing the code.

-- 
Best regards,
Dmitry

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

[Jonathan Cameron]

On Tue, 19 Jul 2022 14:56:51 +0300
Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:

> On 7/18/22 20:25, Jonathan Cameron wrote:
> > What turns this off again?  I'd expect to see a devm_add_action_or_reset()
> > to do that in the !CONFIG_PM case.
> > 
> > This is also an unusual pattern. As far as I can tell it works.
> > Normal trick for ensuring !CONFIG_PM works is to:
> > 
> > 1) Unconditionally turn device on.
> > 2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
> >    due to runtime pm.  
> 
> If CONFIG_PM is disabled, do we really need to care about the power
> management on removal?
> 

Best effort + in general if we do something probe(), we want to do the
reverse in remove().  Sure it's not super important, but it's a nice
to have.  This tends to get 'fixed' by people revisiting the driver
after it has merged.

> > 3) Then call pm_runtime_set_active() so the state tracking matches.  
> 
> We can add pm_runtime_set_active() before h/w is touched for more
> consistency. On Steam Deck supplies are always enabled, but this may be
> not true for other devices.

Generally set it wherever you 'enable' the device as you are indicating
the state after that has happened. That might be really early though.

> 
> > 4) Call 	
> >   pm_runtime_mark_last_busy(dev);
> >   pm_runtime_put_autosuspend(dev);
> >   (here you have a function to do this anyway)
> >   to let runtime_pm use same path as normal to autosuspend
> > 
> > the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
> > is left turned on.  Is there something I'm missing that makes that cycle
> > inappropriate here?  The main reason to do this is it then looks exactly
> > like any other runtime_pm calls elsewhere in the driver, so easier to review.  
> 
> It's appropriate, although caring about PM when it's disabled in kernel
> config could be unnecessary, IMO. It was my suggestion to keep the h/w
> enabled on driver's removal with !CONFIG_PM, minimizing the code.
> 
For the cost of about 4-8 lines of code, I think it's worth having, but can
also see why you decided against.

Jonathan

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

[Dmitry Osipenko]

On 7/19/22 15:19, Jonathan Cameron wrote:
> On Tue, 19 Jul 2022 14:56:51 +0300
> Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote:
> 
>> On 7/18/22 20:25, Jonathan Cameron wrote:
>>> What turns this off again?  I'd expect to see a devm_add_action_or_reset()
>>> to do that in the !CONFIG_PM case.
>>>
>>> This is also an unusual pattern. As far as I can tell it works.
>>> Normal trick for ensuring !CONFIG_PM works is to:
>>>
>>> 1) Unconditionally turn device on.
>>> 2) Register unconditional device off devm_callback. Very rarely harmful even if device already off
>>>    due to runtime pm.  
>>
>> If CONFIG_PM is disabled, do we really need to care about the power
>> management on removal?
>>
> 
> Best effort + in general if we do something probe(), we want to do the
> reverse in remove().  Sure it's not super important, but it's a nice
> to have.  This tends to get 'fixed' by people revisiting the driver
> after it has merged.
> 
>>> 3) Then call pm_runtime_set_active() so the state tracking matches.  
>>
>> We can add pm_runtime_set_active() before h/w is touched for more
>> consistency. On Steam Deck supplies are always enabled, but this may be
>> not true for other devices.
> 
> Generally set it wherever you 'enable' the device as you are indicating
> the state after that has happened. That might be really early though.
> 
>>
>>> 4) Call 	
>>>   pm_runtime_mark_last_busy(dev);
>>>   pm_runtime_put_autosuspend(dev);
>>>   (here you have a function to do this anyway)
>>>   to let runtime_pm use same path as normal to autosuspend
>>>
>>> the upshot of this is that if !CONFIG_PM 3 and 4 do nothing and device
>>> is left turned on.  Is there something I'm missing that makes that cycle
>>> inappropriate here?  The main reason to do this is it then looks exactly
>>> like any other runtime_pm calls elsewhere in the driver, so easier to review.  
>>
>> It's appropriate, although caring about PM when it's disabled in kernel
>> config could be unnecessary, IMO. It was my suggestion to keep the h/w
>> enabled on driver's removal with !CONFIG_PM, minimizing the code.
>>
> For the cost of about 4-8 lines of code, I think it's worth having, but can
> also see why you decided against.

Alright, thank you for the review. Shreeya will address it all and
prepare the v10.

-- 
Best regards,
Dmitry

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

[Rob Herring]

On Fri, 15 Jul 2022 16:46:25 +0530, Shreeya Patel wrote:
> Add devicetree bindings for ltrf216a ambient light sensor.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
> ---
> 
> Changes in v7
>   - Fix the error reported by kernel test robot.
> 
> 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   | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/liteon,ltrf216a.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>