iio: light: AL3010 introduction

Documentation/devicetree/bindings/iio/light/al3320a.yaml new file +43 -0

@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/al3320a.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dyna-Image AL3320A sensor
+
+maintainers:
+  - David Heidelberg <[email protected]>
+
+properties:
+  compatible:
+    const: dynaimage,al3320a
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@1c {
+            compatible = "dynaimage,al3320a";
+            reg = <0x1c>;
+            vdd-supply = <&vdd_reg>;
+            interrupts = <0 99 4>;
+        };
+    };

Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0

index 9cd52d9e1f7f..8d2ebf3d0aa1 100644
@@ -267,6 +267,8 @@ patternProperties:
     description: Dragino Technology Co., Limited
   "^dserve,.*":
     description: dServe Technology B.V.
+  "^dynaimage,.*":
+    description: Dyna-Image
   "^ea,.*":
     description: Embedded Artists AB
   "^ebs-systart,.*":

This commit add dt-bindings support to al3320a driver and vendor-prefix dynaimage.
Partly based on unmerged commit:
"iio: Add Dyna-Image AP3223 ambient light and proximity driver"

Signed-off-by David Heidelberg <[email protected]>

---
v5
- drop requirement on interrups and vdd-supply
- s/al3320a@1c/light-sensor@1c/
- dual license also under BSD

 .../bindings/iio/light/al3320a.yaml           | 43 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/al3320a.yaml

The Dyna-Image AL3010 is a 16-bit digital ambient light sensor which provides a multiple gain function with linear response over a dynamic range 1216/4863/19452/77806.

Signed-off-by David Heidelberg <[email protected]>

---
v5
- drop requirement on interrups and vdd-supply
- s/al3010@1c/light-sensor@1c/
- dual license also under BSD

 .../devicetree/bindings/iio/light/al3010.yaml | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/al3010.yaml

Documentation/devicetree/bindings/iio/light/al3010.yaml new file +43 -0

new file mode 100644
index 000000000000..fdd1e5fa9c74
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/al3010.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Dyna-Image AL3010 sensor
+
+maintainers:
+  - David Heidelberg <[email protected]>
+
+properties:
+  compatible:
+    const: dynaimage,al3010
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  vdd-supply:
+    description: Regulator that provides power to the sensor
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@1c {
+            compatible = "dynaimage,al3010";
+            reg = <0x1c>;
+            vdd-supply = <&vdd_reg>;
+            interrupts = <0 99 4>;
+        };
+    };

The Dyna-Image AL3010 is a 16-bit digital ambient light sensor which provides a multiple gain function with linear response over a dynamic range 1216/4863/19452/77806.

Signed-off-by David Heidelberg <[email protected]>

---
v5
- drop requirement on interrups and vdd-supply
- s/al3010@1c/light-sensor@1c/
- dual license also under BSD

 .../devicetree/bindings/iio/light/al3010.yaml | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/al3010.yaml

- modified to be in part with al3010 driver
- cleanup using bitfield, no functionality change intended

Signed-off-by David Heidelberg <[email protected]>

---
v4
- use GENMASK() and bitfields
- sort headers
- drop init.h unused header

 drivers/iio/light/al3320a.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

drivers/iio/light/al3320a.c +5 -5

index a21aa99e74e4..89b935b4fe9e 100644
@@ -9,9 +9,9 @@
  * TODO: interrupt support, thresholds
  */
 
-#include <linux/module.h>
-#include <linux/init.h>
+#include <linux/bitfield.h>
 #include <linux/i2c.h>
+#include <linux/module.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>

@@ -36,8 +36,7 @@
 #define AL3320A_CONFIG_DISABLE		0x00
 #define AL3320A_CONFIG_ENABLE		0x01
 
-#define AL3320A_GAIN_SHIFT		1
-#define AL3320A_GAIN_MASK		(BIT(2) | BIT(1))
+#define AL3320A_GAIN_MASK		GENMASK(2, 1)
 
 /* chip params default values */
 #define AL3320A_DEFAULT_MEAN_TIME	4

@@ -90,7 +89,8 @@ static int al3320a_init(struct al3320a_data *data)
 		return ret;
 
 	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG_RANGE,
-					AL3320A_RANGE_3 << AL3320A_GAIN_SHIFT);
+					FIELD_PREP(AL3320A_GAIN_MASK,
+						   AL3320A_RANGE_3));
 	if (ret < 0)
 		return ret;
 

drivers/iio/light/al3320a.c +6 -4

 		*val = al3320a_scales[ret][0];
 		*val2 = al3320a_scales[ret][1];
 

@@ -152,11 +152,13 @@ static int al3320a_write_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_SCALE:
 		for (i = 0; i < ARRAY_SIZE(al3320a_scales); i++) {
-			if (val == al3320a_scales[i][0] &&
-			    val2 == al3320a_scales[i][1])
-				return i2c_smbus_write_byte_data(data->client,
+			if (val != al3320a_scales[i][0] ||
+			    val2 != al3320a_scales[i][1])
+				continue;
+
+			return i2c_smbus_write_byte_data(data->client,
 					AL3320A_REG_CONFIG_RANGE,
-					i << AL3320A_GAIN_SHIFT);
+					FIELD_PREP(AL3320A_GAIN_MASK, i));
 		}
 		break;
 	}

Based on:
- 3320A in-kernel driver
- https://www.spinics.net/lists/linux-iio/msg25145.html
- https://lore.kernel.org/patchwork/patch/684179/

I decided to keep it aside of AL3320A due to different approach and much simpler design of 3010.

Tested on Nexus 7 2012 (grouper/tilapia).

Tested-by David Heidelberg <[email protected]>

Tested-by Dmitry Osipenko <[email protected]>

Tested-by Michał Mirosław <[email protected]>

Reviewed-by Dmitry Osipenko <[email protected]>

Signed-off-by David Heidelberg <[email protected]>

---
v4:
- SQUASHed: iio: light: al3010 implement suspend support
- switched from _remove to devm_add_action_or_reset
- implement bitfields FIELD_PREP & FIELD_GET, no functionality change

 drivers/iio/light/Kconfig  |  10 ++
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/al3010.c | 240 +++++++++++++++++++++++++++++++++++++
 3 files changed, 251 insertions(+)
 create mode 100644 drivers/iio/light/al3010.c

drivers/iio/light/Kconfig +10 -0

index 9968f982fbc7..43d9b830279d 100644
@@ -43,6 +43,16 @@ config ADUX1020
 	 To compile this driver as a module, choose M here: the
 	 module will be called adux1020.
 
+config AL3010
+	tristate "AL3010 ambient light sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Dyna Image AL3010
+	  ambient light sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called al3010.
+
 config AL3320A
 	tristate "AL3320A ambient light sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index c98d1cefb861..88bb93550fcc 100644
@@ -7,6 +7,7 @@
 obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
 obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
 obj-$(CONFIG_ADUX1020)		+= adux1020.o
+obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o

drivers/iio/light/al3010.c new file +0 -0

new file mode 100644
index 000000000000..8f8f7f18d620

drivers/iio/light/al3010.c new file +8 -0

+ *
+ * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
+ *
+ * TODO: interrupt support, thresholds
+ * When the driver will get support for interrupt handling, then interrupt
+ * will need to be disabled before turning sensor OFF in order to avoid
+ * potential races with the interrupt handling.
+ */

drivers/iio/light/al3010.c new file +2 -0

+MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL v2");

Based on:
- 3320A in-kernel driver
- https://www.spinics.net/lists/linux-iio/msg25145.html
- https://lore.kernel.org/patchwork/patch/684179/

I decided to keep it aside of AL3320A due to different approach and much simpler design of 3010.

Tested on Nexus 7 2012 (grouper/tilapia).

Tested-by David Heidelberg <[email protected]>

Tested-by Dmitry Osipenko <[email protected]>

Tested-by Michał Mirosław <[email protected]>

Reviewed-by Dmitry Osipenko <[email protected]>

Signed-off-by David Heidelberg <[email protected]>

---
v4:
- SQUASHed: iio: light: al3010 implement suspend support
- switched from _remove to devm_add_action_or_reset
- implement bitfields FIELD_PREP & FIELD_GET, no functionality change v6:
- add copyright and change driver author field

 drivers/iio/light/Kconfig  |  10 ++
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/al3010.c | 242 +++++++++++++++++++++++++++++++++++++
 3 files changed, 253 insertions(+)
 create mode 100644 drivers/iio/light/al3010.c

drivers/iio/light/Kconfig +10 -0

index 9968f982fbc7..43d9b830279d 100644
@@ -43,6 +43,16 @@ config ADUX1020
 	 To compile this driver as a module, choose M here: the
 	 module will be called adux1020.
 
+config AL3010
+	tristate "AL3010 ambient light sensor"
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Dyna Image AL3010
+	  ambient light sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called al3010.
+
 config AL3320A
 	tristate "AL3320A ambient light sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index c98d1cefb861..88bb93550fcc 100644
@@ -7,6 +7,7 @@
 obj-$(CONFIG_ACPI_ALS)		+= acpi-als.o
 obj-$(CONFIG_ADJD_S311)		+= adjd_s311.o
 obj-$(CONFIG_ADUX1020)		+= adux1020.o
+obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o

drivers/iio/light/al3010.c new file +242 -0

new file mode 100644
index 000000000000..84001d04d29d
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AL3010 - Dyna Image Ambient Light Sensor
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Copyright (c) 2016, Dyna-Image Corp.
+ * Copyright (c) 2020, David Heidelberg, Michał Mirosław, Dmitry Osipenko
+ *
+ * IIO driver for AL3010 (7-bit I2C slave address 0x1C).
+ *
+ * TODO: interrupt support, thresholds
+ * When the driver will get support for interrupt handling, then interrupt
+ * will need to be disabled before turning sensor OFF in order to avoid
+ * potential races with the interrupt handling.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define AL3010_DRV_NAME "al3010"
+
+#define AL3010_REG_SYSTEM		0x00
+#define AL3010_REG_DATA_LOW		0x0c
+#define AL3010_REG_CONFIG		0x10
+
+#define AL3010_CONFIG_DISABLE		0x00
+#define AL3010_CONFIG_ENABLE		0x01
+
+#define AL3010_GAIN_MASK		GENMASK(6,4)
+
+#define AL3010_SCALE_AVAILABLE "1.1872 0.2968 0.0742 0.018"
+
+enum al3xxxx_range {
+	AL3XXX_RANGE_1, /* 77806 lx */
+	AL3XXX_RANGE_2, /* 19542 lx */
+	AL3XXX_RANGE_3, /*  4863 lx */
+	AL3XXX_RANGE_4  /*  1216 lx */
+};
+
+static const int al3010_scales[][2] = {
+	{0, 1187200}, {0, 296800}, {0, 74200}, {0, 18600}
+};
+
+struct al3010_data {
+	struct i2c_client *client;
+};
+
+static const struct iio_chan_spec al3010_channels[] = {
+	{
+		.type	= IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static IIO_CONST_ATTR(in_illuminance_scale_available, AL3010_SCALE_AVAILABLE);
+
+static struct attribute *al3010_attributes[] = {
+	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group al3010_attribute_group = {
+	.attrs = al3010_attributes,
+};
+
+static int al3010_set_pwr(struct i2c_client *client, bool pwr)
+{
+	u8 val = pwr ? AL3010_CONFIG_ENABLE : AL3010_CONFIG_DISABLE;
+	return i2c_smbus_write_byte_data(client, AL3010_REG_SYSTEM, val);
+}
+
+static void al3010_set_pwr_off(void *_data)
+{
+	struct al3010_data *data = _data;
+
+	al3010_set_pwr(data->client, false);
+}
+
+static int al3010_init(struct al3010_data *data)
+{
+	int ret;
+
+	ret = al3010_set_pwr(data->client, true);
+
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, AL3010_REG_CONFIG,
+					FIELD_PREP(AL3010_GAIN_MASK,
+						   AL3XXX_RANGE_3));
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int al3010_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	struct al3010_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		/*
+		 * ALS ADC value is stored in two adjacent registers:
+		 * - low byte of output is stored at AL3010_REG_DATA_LOW
+		 * - high byte of output is stored at AL3010_REG_DATA_LOW + 1
+		 */
+		ret = i2c_smbus_read_word_data(data->client,
+					       AL3010_REG_DATA_LOW);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		ret = i2c_smbus_read_byte_data(data->client,
+					       AL3010_REG_CONFIG);
+		if (ret < 0)
+			return ret;
+
+		ret = FIELD_GET(AL3010_GAIN_MASK, ret);
+		*val = al3010_scales[ret][0];
+		*val2 = al3010_scales[ret][1];
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	}
+	return -EINVAL;
+}
+
+static int al3010_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long mask)
+{
+	struct al3010_data *data = iio_priv(indio_dev);
+	int i;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		for (i = 0; i < ARRAY_SIZE(al3010_scales); i++) {
+			if (val != al3010_scales[i][0] ||
+			    val2 != al3010_scales[i][1])
+				continue;
+
+			return i2c_smbus_write_byte_data(data->client,
+					AL3010_REG_CONFIG,
+					FIELD_PREP(AL3010_GAIN_MASK, i));
+		}
+		break;
+	}
+	return -EINVAL;
+}
+
+static const struct iio_info al3010_info = {
+	.read_raw	= al3010_read_raw,
+	.write_raw	= al3010_write_raw,
+	.attrs		= &al3010_attribute_group,
+};
+
+static int al3010_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct al3010_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;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &al3010_info;
+	indio_dev->name = AL3010_DRV_NAME;
+	indio_dev->channels = al3010_channels;
+	indio_dev->num_channels = ARRAY_SIZE(al3010_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = al3010_init(data);
+	if (ret < 0) {
+		dev_err(&client->dev, "al3010 chip init failed\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(&client->dev,
+					al3010_set_pwr_off,
+					data);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static int __maybe_unused al3010_suspend(struct device *dev)
+{
+	return al3010_set_pwr(to_i2c_client(dev), false);
+}
+
+static int __maybe_unused al3010_resume(struct device *dev)
+{
+	return al3010_set_pwr(to_i2c_client(dev), true);
+}
+
+SIMPLE_DEV_PM_OPS(al3010_pm_ops, al3010_suspend, al3010_resume);
+
+static const struct i2c_device_id al3010_id[] = {
+	{"al3010", },
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, al3010_id);
+
+static const struct of_device_id al3010_of_match[] = {
+	{ .compatible = "dynaimage,al3010", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, al3010_of_match);
+
+static struct i2c_driver al3010_driver = {
+	.driver = {
+		.name = AL3010_DRV_NAME,
+		.of_match_table = al3010_of_match,
+		.pm = &al3010_pm_ops,
+	},
+	.probe		= al3010_probe,
+	.id_table	= al3010_id,
+};
+module_i2c_driver(al3010_driver);
+
+MODULE_AUTHOR("Daniel Baluta <[email protected]>");
+MODULE_AUTHOR("David Heidelberg <[email protected]>");
+MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL v2");

AL3320a is fairly simple chip, so for suspend is enough to disable and later enable it again.

Signed-off-by David Heidelberg <[email protected]>

---
 drivers/iio/light/al3320a.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

drivers/iio/light/al3320a.c +13 -5

index 89b935b4fe9e..affa4c6c199a 100644
@@ -7,6 +7,9 @@
  * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
  *
  * TODO: interrupt support, thresholds
+ * When the driver will get support for interrupt handling, then interrupt
+ * will need to be disabled before turning sensor OFF in order to avoid
+ * potential races with the interrupt handling.
  */
 
 #include <linux/bitfield.h>

@@ -78,13 +81,18 @@ static const struct attribute_group al3320a_attribute_group = {
 	.attrs = al3320a_attributes,
 };
 
+static int al3320a_set_pwr(struct i2c_client *client, bool pwr)
+{
+	u8 val = pwr ? AL3320A_CONFIG_ENABLE : AL3320A_CONFIG_DISABLE;
+	return i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG, val);
+}
+
 static int al3320a_init(struct al3320a_data *data)
 {
 	int ret;
 
-	/* power on */
-	ret = i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG,
-					AL3320A_CONFIG_ENABLE);
+	ret = al3320a_set_pwr(data->client, true);
+
 	if (ret < 0)
 		return ret;
 

@@ -203,10 +211,21 @@ static int al3320a_probe(struct i2c_client *client,
 
 static int al3320a_remove(struct i2c_client *client)
 {
-	return i2c_smbus_write_byte_data(client, AL3320A_REG_CONFIG,
-					 AL3320A_CONFIG_DISABLE);
+	return al3320a_set_pwr(client, false);
 }
 
+static int __maybe_unused al3320a_suspend(struct device *dev)

drivers/iio/light/al3320a.c +2 -0

+
 static const struct i2c_device_id al3320a_id[] = {
 	{"al3320a", 0},
 	{}

@@ -216,6 +235,7 @@ MODULE_DEVICE_TABLE(i2c, al3320a_id);
 static struct i2c_driver al3320a_driver = {
 	.driver = {
 		.name = AL3320A_DRV_NAME,
+		.pm = &al3320a_pm_ops,
 	},
 	.probe		= al3320a_probe,
 	.remove		= al3320a_remove,

SIMPLE_DEV_PM_OPS

Quoted 1 line · Jonathan Cameron

SIMPLE_DEV_PM_OPS

Hello Jonathan,

sorry about al3320a, I didn't compile test it :(

Missing static for SIMPLE_DEV_PM_OPS is also valid for AL3010.

Sorry for inconvience.

David

AL3320a is fairly simple chip, so for suspend is enough to disable and later enable it again.

Signed-off-by David Heidelberg <[email protected]>

---
 drivers/iio/light/al3320a.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

drivers/iio/light/al3320a.c +7 -0

index 89b935b4fe9e..affa4c6c199a 100644
@@ -7,6 +7,9 @@
  * IIO driver for AL3320A (7-bit I2C slave address 0x1C).
  *
  * TODO: interrupt support, thresholds
+ * When the driver will get support for interrupt handling, then interrupt
+ * will need to be disabled before turning sensor OFF in order to avoid
+ * potential races with the interrupt handling.
  */
 
 #include <linux/bitfield.h>

@@ -78,13 +81,18 @@ static const struct attribute_group al3320a_attribute_group = {
 	.attrs = al3320a_attributes,
 };
 
+static int al3320a_set_pwr(struct i2c_client *client, bool pwr)
+{
+	u8 val = pwr ? AL3320A_CONFIG_ENABLE : AL3320A_CONFIG_DISABLE;
+	return i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG, val);

drivers/iio/light/al3320a.c +15 -2

+
 	if (ret < 0)
 		return ret;
 

@@ -203,10 +211,21 @@ static int al3320a_probe(struct i2c_client *client,
 
 static int al3320a_remove(struct i2c_client *client)
 {
-	return i2c_smbus_write_byte_data(client, AL3320A_REG_CONFIG,
-					 AL3320A_CONFIG_DISABLE);
+	return al3320a_set_pwr(client, false);
 }
 
+static int __maybe_unused al3320a_suspend(struct device *dev)
+{
+	return al3320a_set_pwr(to_i2c_client(dev), false);
+}
+
+static int __maybe_unused al3320a_resume(struct device *dev)
+{
+	return al3320a_set_pwr(to_i2c_client(dev), true);
+}
+
+SIMPLE_DEV_PM_OPS(al3320a_pm_ops, al3320a_suspend, al3320a_resume);
+
 static const struct i2c_device_id al3320a_id[] = {
 	{"al3320a", 0},
 	{}

@@ -216,6 +235,7 @@ MODULE_DEVICE_TABLE(i2c, al3320a_id);
 static struct i2c_driver al3320a_driver = {
 	.driver = {
 		.name = AL3320A_DRV_NAME,
+		.pm = &al3320a_pm_ops,
 	},
 	.probe		= al3320a_probe,
 	.remove		= al3320a_remove,

Use devm_add_action_or_reset to automatically disable the device and allow you to get rid of the remove function entirely.

Signed-off-by David Heidelberg <[email protected]>

---
 drivers/iio/light/al3320a.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

drivers/iio/light/al3320a.c +14 -6

index affa4c6c199a..49e73e24fff6 100644
@@ -87,6 +87,13 @@ static int al3320a_set_pwr(struct i2c_client *client, bool pwr)
 	return i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG, val);
 }
 
+static void al3320a_set_pwr_off(void *_data)
+{
+	struct al3320a_data *data = _data;
+
+	al3320a_set_pwr(data->client, false);
+}
+
 static int al3320a_init(struct al3320a_data *data)
 {
 	int ret;

@@ -206,12 +213,14 @@ static int al3320a_probe(struct i2c_client *client,
 		dev_err(&client->dev, "al3320a chip init failed\n");
 		return ret;
 	}
-	return devm_iio_device_register(&client->dev, indio_dev);
-}
 
-static int al3320a_remove(struct i2c_client *client)
-{
-	return al3320a_set_pwr(client, false);
+	ret = devm_add_action_or_reset(&client->dev,
+					al3320a_set_pwr_off,
+					data);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static int __maybe_unused al3320a_suspend(struct device *dev)

@@ -238,7 +247,6 @@ static struct i2c_driver al3320a_driver = {
 		.pm = &al3320a_pm_ops,
 	},
 	.probe		= al3320a_probe,
-	.remove		= al3320a_remove,
 	.id_table	= al3320a_id,
 };
 

Use devm_add_action_or_reset to automatically disable the device and allow you to get rid of the remove function entirely.

Signed-off-by David Heidelberg <[email protected]>

---
 drivers/iio/light/al3320a.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

drivers/iio/light/al3320a.c +14 -6

index affa4c6c199a..49e73e24fff6 100644
@@ -87,6 +87,13 @@ static int al3320a_set_pwr(struct i2c_client *client, bool pwr)
 	return i2c_smbus_write_byte_data(data->client, AL3320A_REG_CONFIG, val);
 }
 
+static void al3320a_set_pwr_off(void *_data)
+{
+	struct al3320a_data *data = _data;
+
+	al3320a_set_pwr(data->client, false);
+}
+
 static int al3320a_init(struct al3320a_data *data)
 {
 	int ret;

@@ -206,12 +213,14 @@ static int al3320a_probe(struct i2c_client *client,
 		dev_err(&client->dev, "al3320a chip init failed\n");
 		return ret;
 	}
-	return devm_iio_device_register(&client->dev, indio_dev);
-}
 
-static int al3320a_remove(struct i2c_client *client)
-{
-	return al3320a_set_pwr(client, false);
+	ret = devm_add_action_or_reset(&client->dev,
+					al3320a_set_pwr_off,
+					data);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 static int __maybe_unused al3320a_suspend(struct device *dev)

@@ -238,7 +247,6 @@ static struct i2c_driver al3320a_driver = {
 		.pm = &al3320a_pm_ops,
 	},
 	.probe		= al3320a_probe,
-	.remove		= al3320a_remove,
 	.id_table	= al3320a_id,
 };
 

Quoted 4 lines · On Tue, 11 Feb 2020 20:12:00 +0100 David Heidelberg <[email protected]> wrote:

Use devm_add_action_or_reset to automatically disable the device and allow you to get rid of the remove function entirely.

Signed-off-by David Heidelberg <[email protected]>

This doesn't build as is. I've fixed up but please take a close look at the result.

Quoted 54 lines

---

  drivers/iio/light/al3320a.c | 20 ++++++++++++++------
  1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/al3320a.c
b/drivers/iio/light/al3320a.c
index affa4c6c199a..49e73e24fff6 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -87,6 +87,13 @@ static int al3320a_set_pwr(struct i2c_client
*client, bool pwr)
return i2c_smbus_write_byte_data(data->client,
AL3320A_REG_CONFIG, val);
}

+static void al3320a_set_pwr_off(void *_data)
+{
+ struct al3320a_data *data = _data;
+
+ al3320a_set_pwr(data->client, false);
+}
+
static int al3320a_init(struct al3320a_data *data)
{
int ret;
@@ -206,12 +213,14 @@ static int al3320a_probe(struct i2c_client
*client,
dev_err(&client->dev, "al3320a chip init failed\n"); return ret;
}
- return devm_iio_device_register(&client->dev, indio_dev);
-}

-static int al3320a_remove(struct i2c_client *client) -{
- return al3320a_set_pwr(client, false);
+ ret = devm_add_action_or_reset(&client->dev,
+ al3320a_set_pwr_off,
+ data);
+ if (ret < 0)
+ return ret;
+
+ return devm_iio_device_register(&client->dev, indio_dev);
}

static int __maybe_unused al3320a_suspend(struct device *dev)
@@ -238,7 +247,6 @@ static struct i2c_driver al3320a_driver = {
.pm = &al3320a_pm_ops,
},
.probe = al3320a_probe,
- .remove = al3320a_remove,
.id_table = al3320a_id,
};

Thank you for fixes, it does look good.
Only thing is fixes which should go into commit "iio: light: al3320a implement suspend support"[1] when into "iio: light: al3320a implement devm_add_action_or_reset" [2].

+ return i2c_smbus_write_byte_data(client, AL3320A_REG_CONFIG, val);

and

+static SIMPLE_DEV_PM_OPS(al3320a_pm_ops, al3320a_suspend, al3320a_resume);

Thank you very much for merging and next time
I'll spend more time with review and doing final build test.

David

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=d74856400b2dcb2e0eab2132c779408809566431
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=4b1b42bcd993258628fabc5e35f20a202b1e9f7b

Quoted 66 lines · Jonathan Cameron

Quoted 4 lines · On Tue, 11 Feb 2020 20:12:00 +0100 David Heidelberg <[email protected]> wrote:

Use devm_add_action_or_reset to automatically disable the device and allow you to get rid of the remove function entirely.

Signed-off-by David Heidelberg <[email protected]>

This doesn't build as is. I've fixed up but please take a close look at the result.

Quoted 54 lines

---

  drivers/iio/light/al3320a.c | 20 ++++++++++++++------
  1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/al3320a.c
b/drivers/iio/light/al3320a.c
index affa4c6c199a..49e73e24fff6 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -87,6 +87,13 @@ static int al3320a_set_pwr(struct i2c_client
*client, bool pwr)
return i2c_smbus_write_byte_data(data->client,
AL3320A_REG_CONFIG, val);
}

+static void al3320a_set_pwr_off(void *_data)
+{
+ struct al3320a_data *data = _data;
+
+ al3320a_set_pwr(data->client, false);
+}
+
static int al3320a_init(struct al3320a_data *data)
{
int ret;
@@ -206,12 +213,14 @@ static int al3320a_probe(struct i2c_client
*client,
dev_err(&client->dev, "al3320a chip init failed\n"); return ret;
}
- return devm_iio_device_register(&client->dev, indio_dev);
-}

-static int al3320a_remove(struct i2c_client *client) -{
- return al3320a_set_pwr(client, false);
+ ret = devm_add_action_or_reset(&client->dev,
+ al3320a_set_pwr_off,
+ data);
+ if (ret < 0)
+ return ret;
+
+ return devm_iio_device_register(&client->dev, indio_dev);
}

static int __maybe_unused al3320a_suspend(struct device *dev)
@@ -238,7 +247,6 @@ static struct i2c_driver al3320a_driver = {
.pm = &al3320a_pm_ops,
},
.probe = al3320a_probe,
- .remove = al3320a_remove,
.id_table = al3320a_id,
};

- allow autoloading when build as module and defined inside DT

Signed-off-by David Heidelberg <[email protected]>

---
v4
- dropping of init.h header moved to previous patches

 drivers/iio/light/al3320a.c | 8 ++++++++
 1 file changed, 8 insertions(+)

drivers/iio/light/al3320a.c +8 -0

index 49e73e24fff6..e7c4899a4313 100644
@@ -15,6 +15,7 @@
 #include <linux/bitfield.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>

@@ -241,9 +242,16 @@ static const struct i2c_device_id al3320a_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, al3320a_id);
 
+static const struct of_device_id al3320a_of_match[] = {
+	{ .compatible = "dynaimage,al3320a", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, al3320a_of_match);
+
 static struct i2c_driver al3320a_driver = {
 	.driver = {
 		.name = AL3320A_DRV_NAME,
+		.of_match_table = al3320a_of_match,
 		.pm = &al3320a_pm_ops,
 	},
 	.probe		= al3320a_probe,