[PATCH v5 0/7] iio: light: AL3010 introduction

[PATCH v5 0/7] iio: light: AL3010 introduction

[David Heidelberg]

These patches implement support for simple iio light sensor AL3010 and
also improves and align formating of AL3320a which is origin of al3010
driver.

AL3010 is used in many devices, mostly tegra2/3 based.
This effort is aimed for reducing number of out-of-tree patches for
tegra tablets and phones.

This sensor is tested on Nexus 7 (2012, grouper/tilapia).

David Heidelberg (7):
  dt-bindings: iio: light: add support for Dyna-Image AL3320A
  dt-bindings: iio: light: add support for Dyna-Image AL3010
  iio: light: al3320a slightly improve code formatting
  iio: light: add Dyna-Image AL3010 driver
  iio: light: al3320a implement suspend support
  iio: light: al3320a implement devm_add_action_or_reset
  iio: light: al3320a allow module autoload and polish

 .../devicetree/bindings/iio/light/al3010.yaml |  43 ++++
 .../bindings/iio/light/al3320a.yaml           |  43 ++++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 drivers/iio/light/Kconfig                     |  10 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/al3010.c                    | 240 ++++++++++++++++++
 drivers/iio/light/al3320a.c                   |  72 ++++--
 7 files changed, 394 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/light/al3010.yaml
 create mode 100644 Documentation/devicetree/bindings/iio/light/al3320a.yaml
 create mode 100644 drivers/iio/light/al3010.c

-- 
2.25.0

[PATCH v5 1/7] dt-bindings: iio: light: add support for Dyna-Image AL3320A

[David Heidelberg]

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 <david@ixit.cz>
---
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

diff --git a/Documentation/devicetree/bindings/iio/light/al3320a.yaml b/Documentation/devicetree/bindings/iio/light/al3320a.yaml
new file mode 100644
index 000000000000..cdc8d98d114c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/al3320a.yaml
@@ -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 <david@ixit.cz>
+
+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>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 9cd52d9e1f7f..8d2ebf3d0aa1 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -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,.*":
-- 
2.25.0

[PATCH v5 2/7] dt-bindings: iio: light: add support for Dyna-Image AL3010

[David Heidelberg]

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 <david@ixit.cz>
---
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

diff --git a/Documentation/devicetree/bindings/iio/light/al3010.yaml b/Documentation/devicetree/bindings/iio/light/al3010.yaml
new file mode 100644
index 000000000000..fdd1e5fa9c74
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/al3010.yaml
@@ -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 <david@ixit.cz>
+
+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>;
+        };
+    };
-- 
2.25.0

[PATCH v5 3/7] iio: light: al3320a slightly improve code formatting

[David Heidelberg]

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

Signed-off-by: David Heidelberg <david@ixit.cz>
---
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(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index a21aa99e74e4..89b935b4fe9e 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -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;
 
@@ -133,7 +133,7 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		ret = (ret & AL3320A_GAIN_MASK) >> AL3320A_GAIN_SHIFT;
+		ret = FIELD_GET(AL3320A_GAIN_MASK, ret)
 		*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;
 	}
-- 
2.25.0

[PATCH v5 4/7] iio: light: add Dyna-Image AL3010 driver

[David Heidelberg]

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 <david@ixit.cz>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
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

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 9968f982fbc7..43d9b830279d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -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
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c98d1cefb861..88bb93550fcc 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -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
diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
new file mode 100644
index 000000000000..8f8f7f18d620
--- /dev/null
+++ b/drivers/iio/light/al3010.c
@@ -0,0 +1,240 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AL3010 - Dyna Image Ambient Light Sensor
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Copyright (c) 2016, Dyna-Image Corp.
+ *
+ * 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 <daniel.baluta@nxp.com>");
+MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.0

[PATCH v5 5/7] iio: light: al3320a implement suspend support

[David Heidelberg]

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

Signed-off-by: David Heidelberg <david@ixit.cz>
---
 drivers/iio/light/al3320a.c | 30 +++++++++++++++++++++++++-----
 1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 89b935b4fe9e..affa4c6c199a 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -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)
+{
+	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,
-- 
2.25.0

[PATCH v5 6/7] iio: light: al3320a implement devm_add_action_or_reset

[David Heidelberg]

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 <david@ixit.cz>
---
 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,
 };
 
-- 
2.25.0

[PATCH v5 7/7] iio: light: al3320a allow module autoload and polish

[David Heidelberg]

- allow autoloading when build as module and defined inside DT

Signed-off-by: David Heidelberg <david@ixit.cz>
---
v4
 - dropping of init.h header moved to previous patches

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

diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
index 49e73e24fff6..e7c4899a4313 100644
--- a/drivers/iio/light/al3320a.c
+++ b/drivers/iio/light/al3320a.c
@@ -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,
-- 
2.25.0

Re: [PATCH v5 4/7] iio: light: add Dyna-Image AL3010 driver

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:58 +0100
David Heidelberg <david@ixit.cz> wrote:

> 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 <david@ixit.cz>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>

Hi David,

Code looks fine, but you need to change the module author and
I think it makes sense to add your own copyright notice to reflect the
work you did on this driver!

Thanks,

Jonathan

> ---
> 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
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 9968f982fbc7..43d9b830279d 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -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
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c98d1cefb861..88bb93550fcc 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -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
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> new file mode 100644
> index 000000000000..8f8f7f18d620
> --- /dev/null
> +++ b/drivers/iio/light/al3010.c
> @@ -0,0 +1,240 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AL3010 - Dyna Image Ambient Light Sensor
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Copyright (c) 2016, Dyna-Image Corp.

Also update this.

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

> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@nxp.com>");

You want to update this!

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

[PATCH v6] iio: light: add Dyna-Image AL3010 driver

[David Heidelberg]

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 <david@ixit.cz>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Signed-off-by: David Heidelberg <david@ixit.cz>
---
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

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 9968f982fbc7..43d9b830279d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -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
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c98d1cefb861..88bb93550fcc 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -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
diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
new file mode 100644
index 000000000000..84001d04d29d
--- /dev/null
+++ b/drivers/iio/light/al3010.c
@@ -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 <daniel.baluta@nxp.com>");
+MODULE_AUTHOR("David Heidelberg <david@ixit.cz>");
+MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
+MODULE_LICENSE("GPL v2");
-- 
2.25.0

Re: [PATCH v5 1/7] dt-bindings: iio: light: add support for Dyna-Image AL3320A

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:55 +0100
David Heidelberg <david@ixit.cz> wrote:

> 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 <david@ixit.cz>
> ---
> 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
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/al3320a.yaml b/Documentation/devicetree/bindings/iio/light/al3320a.yaml
> new file mode 100644
> index 000000000000..cdc8d98d114c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/al3320a.yaml
I missed this previously but it's preferred for the filename to include the
manufacturer bit.  I'll fix up including the reference to the filename below.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Ideally I'd like to add a DT reviewed by, but I think you've addressed
all the previous comments.

Thanks,

Jonathan


> @@ -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 <david@ixit.cz>
> +
> +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>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 9cd52d9e1f7f..8d2ebf3d0aa1 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -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,.*":

Re: [PATCH v5 2/7] dt-bindings: iio: light: add support for Dyna-Image AL3010

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:56 +0100
David Heidelberg <david@ixit.cz> wrote:

> 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 <david@ixit.cz>
Applied with name tweaked as per patch 1.

Thanks,

Jonathan

> ---
> 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
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/al3010.yaml b/Documentation/devicetree/bindings/iio/light/al3010.yaml
> new file mode 100644
> index 000000000000..fdd1e5fa9c74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/al3010.yaml
> @@ -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 <david@ixit.cz>
> +
> +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>;
> +        };
> +    };

Re: [PATCH v5 3/7] iio: light: al3320a slightly improve code formatting

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:57 +0100
David Heidelberg <david@ixit.cz> wrote:

> - modified to be in part with al3010 driver
> - cleanup using bitfield, no functionality change intended
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
One issue below. Fixed up and applied.

Thanks,

Jonathan

> ---
> 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(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index a21aa99e74e4..89b935b4fe9e 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -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;
>  
> @@ -133,7 +133,7 @@ static int al3320a_read_raw(struct iio_dev *indio_dev,
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = (ret & AL3320A_GAIN_MASK) >> AL3320A_GAIN_SHIFT;
> +		ret = FIELD_GET(AL3320A_GAIN_MASK, ret)
Missing semicolon.  I fixed up.


>  		*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;
>  	}

Re: [PATCH v6] iio: light: add Dyna-Image AL3010 driver

[Jonathan Cameron]

On Fri, 14 Feb 2020 23:09:17 +0100
David Heidelberg <david@ixit.cz> wrote:

> 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 <david@ixit.cz>
> Tested-by: Dmitry Osipenko <digetx@gmail.com>
> Tested-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
> Signed-off-by: David Heidelberg <david@ixit.cz>
Hi.

I'm not generally keen on partial reposts of series, but in this
case it's so trivial I'll pick it up anyway.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
> 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
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 9968f982fbc7..43d9b830279d 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -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
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index c98d1cefb861..88bb93550fcc 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -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
> diff --git a/drivers/iio/light/al3010.c b/drivers/iio/light/al3010.c
> new file mode 100644
> index 000000000000..84001d04d29d
> --- /dev/null
> +++ b/drivers/iio/light/al3010.c
> @@ -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 <daniel.baluta@nxp.com>");
> +MODULE_AUTHOR("David Heidelberg <david@ixit.cz>");
> +MODULE_DESCRIPTION("AL3010 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v5 5/7] iio: light: al3320a implement suspend support

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:59 +0100
David Heidelberg <david@ixit.cz> wrote:

> AL3320a is fairly simple chip, so for suspend is enough to disable and
> later enable it again.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
Applied with a small tweak mentioned below.

Thanks,

Jonathan

> ---
>  drivers/iio/light/al3320a.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 89b935b4fe9e..affa4c6c199a 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -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)
> +{
> +	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);
Build test showed this is missing a static.
Added.
> +
>  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,

Re: [PATCH v5 5/7] iio: light: al3320a implement suspend support

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:11:59 +0100
David Heidelberg <david@ixit.cz> wrote:

> AL3320a is fairly simple chip, so for suspend is enough to disable and
> later enable it again.
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
> ---
>  drivers/iio/light/al3320a.c | 30 +++++++++++++++++++++++++-----
>  1 file changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 89b935b4fe9e..affa4c6c199a 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -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);

data doesn't exist.  I've fixed up this as well, but please take a quick
look.

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

Re: [PATCH v5 6/7] iio: light: al3320a implement devm_add_action_or_reset

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:12:00 +0100
David Heidelberg <david@ixit.cz> 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 <david@ixit.cz>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  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,
>  };
>  

Re: [PATCH v5 7/7] iio: light: al3320a allow module autoload and polish

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:12:01 +0100
David Heidelberg <david@ixit.cz> wrote:

> - allow autoloading when build as module and defined inside DT
> 
> Signed-off-by: David Heidelberg <david@ixit.cz>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan

> ---
> v4
>  - dropping of init.h header moved to previous patches
> 
>  drivers/iio/light/al3320a.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/light/al3320a.c b/drivers/iio/light/al3320a.c
> index 49e73e24fff6..e7c4899a4313 100644
> --- a/drivers/iio/light/al3320a.c
> +++ b/drivers/iio/light/al3320a.c
> @@ -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,

Re: [PATCH v5 5/7] iio: light: al3320a implement suspend support

[David Heidelberg]

On Sat, Feb 15, 2020 at 17:15, Jonathan Cameron <jic23@kernel.org> 
wrote:
> 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

Re: [PATCH v5 5/7] iio: light: al3320a implement suspend support

[Jonathan Cameron]

On Sat, 15 Feb 2020 18:34:12 +0100
David Heidelberg <david@ixit.cz> wrote:

> On Sat, Feb 15, 2020 at 17:15, Jonathan Cameron <jic23@kernel.org> 
> wrote:
> > 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.
Yup :) Got that one as well.

Jonathan

> 
> Sorry for inconvience.
> 
> David
> 
> 
> 

Re: [PATCH v5 6/7] iio: light: al3320a implement devm_add_action_or_reset

[Jonathan Cameron]

On Tue, 11 Feb 2020 20:12:00 +0100
David Heidelberg <david@ixit.cz> 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 <david@ixit.cz>

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

> ---
>  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,
>  };
>  

Re: [PATCH v5 6/7] iio: light: al3320a implement devm_add_action_or_reset

[David Heidelberg]

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


On Sat, Feb 15, 2020 at 18:38, Jonathan Cameron <jic23@kernel.org> 
wrote:
> On Tue, 11 Feb 2020 20:12:00 +0100
> David Heidelberg <david@ixit.cz> 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 <david@ixit.cz>
> 
> This doesn't build as is.  I've fixed up but please take a close
> look at the result.
> 
>>  ---
>>   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,
>>   };
>> 
> 

Re: [PATCH v5 1/7] dt-bindings: iio: light: add support for Dyna-Image AL3320A

[Rob Herring]

On Tue, 11 Feb 2020 20:11:55 +0100, David Heidelberg wrote:
> 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 <david@ixit.cz>
> ---
> 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
> 

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

Re: [PATCH v5 2/7] dt-bindings: iio: light: add support for Dyna-Image AL3010

[Rob Herring]

On Tue, 11 Feb 2020 20:11:56 +0100, David Heidelberg wrote:
> 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 <david@ixit.cz>
> ---
> 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
> 

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

Re: [PATCH v5 6/7] iio: light: al3320a implement devm_add_action_or_reset

[Jonathan Cameron]

On Sat, 15 Feb 2020 20:42:59 +0100
David Heidelberg <david@ixit.cz> wrote:

> 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.
Gah. I messed that up.  Anyhow, now fixed and pushed out with changes
in the right patch.

Thanks,

Jonathan

> 
> 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
> 
> 
> On Sat, Feb 15, 2020 at 18:38, Jonathan Cameron <jic23@kernel.org> 
> wrote:
> > On Tue, 11 Feb 2020 20:12:00 +0100
> > David Heidelberg <david@ixit.cz> 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 <david@ixit.cz>  
> > 
> > This doesn't build as is.  I've fixed up but please take a close
> > look at the result.
> >   
> >>  ---
> >>   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,
> >>   };
> >>   
> >   
> 
>