Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 0/2] iio: light: Support AMS AS73211 digital XYZ sensor
@ 2020-07-31  7:01 Christian Eggers
  2020-07-31  7:01 ` [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
  2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
  0 siblings, 2 replies; 9+ messages in thread
From: Christian Eggers @ 2020-07-31  7:01 UTC (permalink / raw)
  To: Rob Herring, Andy Shevchenko, Jonathan Cameron, Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio, devicetree, linux-kernel, Christian Eggers

his series adds support for the AMS AS73211 digital XYZ sensor.

Changes in v4:
---------------
- Integrated 2nd review from Andy Shevchenko
- Use more devm_ functions in as73211_probe()

Changes in v3:
---------------
- Integrated comments from Andy Shevchenko
- Integrated comments from Jonathan Cameron

Changes in v2:
---------------
- Fix $id in dt binding
- Document full I2C address range in "reg" property
- Move "buffer" member out of "struct as73211_data"
- Fix sparse warnings by using correct data types




^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support
  2020-07-31  7:01 [PATCH v4 0/2] iio: light: Support AMS AS73211 digital XYZ sensor Christian Eggers
@ 2020-07-31  7:01 ` Christian Eggers
  2020-07-31 22:45   ` Rob Herring
  2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
  1 sibling, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-07-31  7:01 UTC (permalink / raw)
  To: Rob Herring, Andy Shevchenko, Jonathan Cameron, Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio, devicetree, linux-kernel, Christian Eggers

Add DT bindings for AMS AS73211 XYZ True Color Sensor.

Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 .../bindings/iio/light/ams,as73211.yaml       | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as73211.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/ams,as73211.yaml b/Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
new file mode 100644
index 000000000000..0e8cd02759b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ams,as73211.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS AS73211 JENCOLOR(R) Digital XYZ Sensor
+
+maintainers:
+  - Christian Eggers <ceggers@arri.de>
+
+description: |
+  XYZ True Color Sensor with I2C Interface
+  https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
+
+properties:
+  compatible:
+    enum:
+      - ams,as73211
+
+  reg:
+    description:
+      I2C address of the device (0x74...0x77).
+    maxItems: 1
+
+  interrupts:
+    description:
+      Interrupt specifier for the READY interrupt generated by the device.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        as73211@74 {
+            compatible = "ams,as73211";
+            reg = <0x74>;
+            pinctrl-names = "default";
+            pinctrl-0 = <&pinctrl_color_sensor>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <19 IRQ_TYPE_EDGE_RISING>; /* READY */
+        };
+    };
+...
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31  7:01 [PATCH v4 0/2] iio: light: Support AMS AS73211 digital XYZ sensor Christian Eggers
  2020-07-31  7:01 ` [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
@ 2020-07-31  7:01 ` Christian Eggers
  2020-07-31  7:34   ` Andy Shevchenko
  2020-08-01 15:29   ` Jonathan Cameron
  1 sibling, 2 replies; 9+ messages in thread
From: Christian Eggers @ 2020-07-31  7:01 UTC (permalink / raw)
  To: Rob Herring, Andy Shevchenko, Jonathan Cameron, Jonathan Cameron
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	linux-iio, devicetree, linux-kernel, Christian Eggers

Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.

This driver has no built-in trigger. In order for making triggered
measurements, an external (software) trigger driver like
iio-trig-hrtimer or iio-trig-sysfs is required.

The sensor supports single and continuous measurement modes. The latter
is not used by design as this would require tight timing synchronization
between hardware and driver without much benefit.

Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
Signed-off-by: Christian Eggers <ceggers@arri.de>
---
 MAINTAINERS                 |   7 +
 drivers/iio/light/Kconfig   |  15 +
 drivers/iio/light/Makefile  |   1 +
 drivers/iio/light/as73211.c | 780 ++++++++++++++++++++++++++++++++++++
 4 files changed, 803 insertions(+)
 create mode 100644 drivers/iio/light/as73211.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 068d6e94122b..673570414147 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -934,6 +934,13 @@ S:	Supported
 F:	arch/arm64/boot/dts/amd/amd-seattle-xgbe*.dtsi
 F:	drivers/net/ethernet/amd/xgbe/
 
+AMS AS73211 DRIVER
+M:	Christian Eggers <ceggers@arri.de>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
+F:	drivers/iio/light/as73211.c
+
 ANALOG DEVICES INC AD5686 DRIVER
 M:	Michael Hennerich <Michael.Hennerich@analog.com>
 L:	linux-pm@vger.kernel.org
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 182bd18c4bb2..cade6dc0305b 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -86,6 +86,21 @@ config APDS9960
 	  To compile this driver as a module, choose M here: the
 	  module will be called apds9960
 
+config AS73211
+	tristate "AMS AS73211 XYZ color sensor"
+	depends on I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	 If you say yes here you get support for the AMS AS73211
+	 JENCOLOR(R) Digital XYZ Sensor.
+
+	 For triggered measurements, you will need an additional trigger driver
+	 like IIO_HRTIMER_TRIGGER or IIO_SYSFS_TRIGGER.
+
+	 This driver can also be built as a module.  If so, the module
+	 will be called as73211.
+
 config BH1750
 	tristate "ROHM BH1750 ambient light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index d1c8aa30b9a8..ea376deaca54 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_AL3010)		+= al3010.o
 obj-$(CONFIG_AL3320A)		+= al3320a.o
 obj-$(CONFIG_APDS9300)		+= apds9300.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
+obj-$(CONFIG_AS73211)		+= as73211.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
 obj-$(CONFIG_BH1780)		+= bh1780.o
 obj-$(CONFIG_CM32181)		+= cm32181.o
diff --git a/drivers/iio/light/as73211.c b/drivers/iio/light/as73211.c
new file mode 100644
index 000000000000..cfd7c81c87b2
--- /dev/null
+++ b/drivers/iio/light/as73211.c
@@ -0,0 +1,780 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor
+ *
+ * Author: Christian Eggers <ceggers@arri.de>
+ *
+ * Copyright (c) 2020 ARRI Lighting
+ *
+ * Color light sensor with 16-bit channels for x, y, z and temperature);
+ * 7-bit I2C slave address 0x74 .. 0x77.
+ *
+ * Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
+ */
+
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+
+#define AS73211_DRV_NAME "as73211"
+
+/* AS73211 configuration registers */
+#define AS73211_REG_OSR    0x0
+#define AS73211_REG_AGEN   0x2
+#define AS73211_REG_CREG1  0x6
+#define AS73211_REG_CREG2  0x7
+#define AS73211_REG_CREG3  0x8
+
+/* AS73211 output register bank */
+#define AS73211_OUT_OSR_STATUS    0
+#define AS73211_OUT_TEMP          1
+#define AS73211_OUT_MRES1         2
+#define AS73211_OUT_MRES2         3
+#define AS73211_OUT_MRES3         4
+
+#define AS73211_OSR_SS            BIT(7)
+#define AS73211_OSR_PD            BIT(6)
+#define AS73211_OSR_SW_RES        BIT(3)
+#define AS73211_OSR_DOS_MASK      GENMASK(2, 0)
+#define AS73211_OSR_DOS_CONFIG    FIELD_PREP(AS73211_OSR_DOS_MASK, 0x2)
+#define AS73211_OSR_DOS_MEASURE   FIELD_PREP(AS73211_OSR_DOS_MASK, 0x3)
+
+#define AS73211_AGEN_DEVID_MASK   GENMASK(7, 4)
+#define AS73211_AGEN_DEVID(x)     FIELD_PREP(AS73211_AGEN_DEVID_MASK, (x))
+#define AS73211_AGEN_MUT_MASK     GENMASK(3, 0)
+#define AS73211_AGEN_MUT(x)       FIELD_PREP(AS73211_AGEN_MUT_MASK, (x))
+
+#define AS73211_CREG1_GAIN_MASK   GENMASK(7, 4)
+#define AS73211_CREG1_TIME_MASK   GENMASK(3, 0)
+
+#define AS73211_CREG3_CCLK_MASK   GENMASK(1, 0)
+
+#define AS73211_OSR_STATUS_OUTCONVOF  BIT(15)
+#define AS73211_OSR_STATUS_MRESOF     BIT(14)
+#define AS73211_OSR_STATUS_ADCOF      BIT(13)
+#define AS73211_OSR_STATUS_LDATA      BIT(12)
+#define AS73211_OSR_STATUS_NDATA      BIT(11)
+#define AS73211_OSR_STATUS_NOTREADY   BIT(10)
+
+#define AS73211_SAMPLE_TIME_NUM       15
+#define AS73211_SAMPLE_TIME_MAX_MS    BIT(AS73211_SAMPLE_TIME_NUM - 1)
+
+static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000, 8192000 };
+static const int as73211_hardwaregain_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128,
+						   256, 512, 1024, 2048 };
+
+/**
+ * struct as73211_data - Instance data for one AS73211
+ * @client: I2C client.
+ * @osr:    Cached Operational State Register.
+ * @creg1:  Cached Configuration Register 1.
+ * @creg2:  Cached Configuration Register 2.
+ * @creg3:  Cached Configuration Register 3.
+ * @mutex:  Keeps cached registers in synch with the device.
+ * @completion: Completion to wait for interrupt.
+ * @int_time_avail: Available integration times (depend on sampling frequency).
+ */
+struct as73211_data {
+	struct i2c_client *client;
+	u8 osr;
+	u8 creg1;
+	u8 creg2;
+	u8 creg3;
+	struct mutex mutex;
+	struct completion completion;
+	int int_time_avail[AS73211_SAMPLE_TIME_NUM * 2];
+};
+
+#define AS73211_COLOR_CHANNEL(_color, _si, _addr) { \
+	.type = IIO_INTENSITY, \
+	.modified = 1, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), \
+	.info_mask_shared_by_type = \
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+		BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
+		BIT(IIO_CHAN_INFO_INT_TIME), \
+	.info_mask_shared_by_type_available = \
+		BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+		BIT(IIO_CHAN_INFO_HARDWAREGAIN) | \
+		BIT(IIO_CHAN_INFO_INT_TIME), \
+	.channel2 = IIO_MOD_##_color, \
+	.address = _addr, \
+	.scan_index = _si, \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+#define AS73211_OFFSET_TEMP (-66.9)
+#define AS73211_SCALE_TEMP  0.05
+#define AS73211_SCALE_X 277071108  /* nW/m^2 */
+#define AS73211_SCALE_Y 298384270  /* nW/m^2 */
+#define AS73211_SCALE_Z 160241927  /* nW/m^2 */
+
+/* Channel order MUST match devices result register order */
+#define AS73211_SCAN_INDEX_TEMP 0
+#define AS73211_SCAN_INDEX_X    1
+#define AS73211_SCAN_INDEX_Y    2
+#define AS73211_SCAN_INDEX_Z    3
+#define AS73211_SCAN_INDEX_TS   4
+
+#define AS73211_SCAN_MASK_COLOR ( \
+	BIT(AS73211_SCAN_INDEX_X) |   \
+	BIT(AS73211_SCAN_INDEX_Y) |   \
+	BIT(AS73211_SCAN_INDEX_Z))
+
+#define AS73211_SCAN_MASK_ALL (    \
+	BIT(AS73211_SCAN_INDEX_TEMP) | \
+	AS73211_SCAN_MASK_COLOR)
+
+static const struct iio_chan_spec as73211_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_OFFSET) |
+			BIT(IIO_CHAN_INFO_SCALE),
+		.address = AS73211_OUT_TEMP,
+		.scan_index = AS73211_SCAN_INDEX_TEMP,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		}
+	},
+	AS73211_COLOR_CHANNEL(X, AS73211_SCAN_INDEX_X, AS73211_OUT_MRES1),
+	AS73211_COLOR_CHANNEL(Y, AS73211_SCAN_INDEX_Y, AS73211_OUT_MRES2),
+	AS73211_COLOR_CHANNEL(Z, AS73211_SCAN_INDEX_Z, AS73211_OUT_MRES3),
+	IIO_CHAN_SOFT_TIMESTAMP(AS73211_SCAN_INDEX_TS),
+};
+
+/* integration time in units of 1024 clock cycles */
+static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data)
+{
+	/* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
+	return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
+}
+
+static unsigned int as73211_integration_time_us(struct as73211_data *data,
+						 unsigned int integration_time_1024cyc)
+{
+	/*
+	 * f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz)
+	 * t_cycl is configured in CREG1 in powers of 2 (x 1024 cycles)
+	 * t_int_us = 1 / (f_samp) * t_cycl * US_PER_SEC
+	 *          = 1 / (2^CREG3_CCLK * 1,024,000) * 2^CREG1_CYCLES * 1,024 * US_PER_SEC
+	 *          = 2^(-CREG3_CCLK) * 2^CREG1_CYCLES * 1,000
+	 *            in order to get rid of negative exponents, we extend the
+	 *            "fraction" by 2^3 (3 == CREG3_CCLK,max)
+	 *          = 2^(3-CREG3_CCLK) * 2^CREG1_CYCLES * 125
+	 */
+	return BIT(3 - FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) *
+		integration_time_1024cyc * 125;
+}
+
+static void as73211_integration_time_calc_avail(struct as73211_data *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(data->int_time_avail); i++) {
+		unsigned int time_us = as73211_integration_time_us(data, BIT(i));
+
+		data->int_time_avail[i * 2] = time_us / USEC_PER_SEC;
+		data->int_time_avail[i * 2 + 1] = time_us % USEC_PER_SEC;
+	}
+}
+
+static unsigned int as73211_gain(struct as73211_data *data)
+{
+	/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
+	return BIT(13 - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
+}
+
+/* must be called with as73211_data::mutex held. */
+static int as73211_req_data(struct as73211_data *data)
+{
+	unsigned int time_us = as73211_integration_time_us(data,
+							    as73211_integration_time_1024cyc(data));
+	struct device *dev = &data->client->dev;
+	union i2c_smbus_data smbus_data;
+	u16 osr_status;
+	int ret;
+
+	if (data->client->irq)
+		reinit_completion(&data->completion);
+
+	/*
+	 * During measurement, there should be no traffic on the i2c bus as the
+	 * electrical noise would disturb the measurement process.
+	 */
+	i2c_lock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
+
+	data->osr &= ~AS73211_OSR_DOS_MASK;
+	data->osr |= AS73211_OSR_DOS_MEASURE | AS73211_OSR_SS;
+
+	smbus_data.byte = data->osr;
+	ret = __i2c_smbus_xfer(data->client->adapter, data->client->addr,
+			data->client->flags, I2C_SMBUS_WRITE,
+			AS73211_REG_OSR, I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (ret < 0) {
+		i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
+		return ret;
+	}
+
+	/*
+	 * Reset AS73211_OSR_SS (is self clearing) in order to avoid unintentional
+	 * triggering of further measurements later.
+	 */
+	data->osr &= ~AS73211_OSR_SS;
+
+	/*
+	 * Add some extra margin for the timeout. sensor timing is not as precise
+	 * as our one ...
+	 */
+	time_us += time_us / 8;
+	if (data->client->irq) {
+		ret = wait_for_completion_timeout(&data->completion, usecs_to_jiffies(time_us));
+		if (!ret) {
+			dev_err(dev, "timeout waiting for READY IRQ\n");
+			i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
+			return -ETIMEDOUT;
+		}
+	} else {
+		/* Wait integration time */
+		usleep_range(time_us, 2 * time_us);
+	}
+
+	i2c_unlock_bus(data->client->adapter, I2C_LOCK_SEGMENT);
+
+	ret = i2c_smbus_read_word_data(data->client, AS73211_OUT_OSR_STATUS);
+	if (ret < 0)
+		return ret;
+
+	osr_status = ret;
+	if (osr_status != (AS73211_OSR_DOS_MEASURE | AS73211_OSR_STATUS_NDATA)) {
+		if (osr_status & AS73211_OSR_SS) {
+			dev_err(dev, "%s() Measurement has not stopped\n", __func__);
+			return -ETIME;
+		}
+		if (osr_status & AS73211_OSR_STATUS_NOTREADY) {
+			dev_err(dev, "%s() Data is not ready\n", __func__);
+			return -ENODATA;
+		}
+		if (!(osr_status & AS73211_OSR_STATUS_NDATA)) {
+			dev_err(dev, "%s() New new data available\n", __func__);
+			return -ENODATA;
+		}
+		if (osr_status & AS73211_OSR_STATUS_LDATA) {
+			dev_err(dev, "%s() Result buffer overrun\n", __func__);
+			return -ENOBUFS;
+		}
+		if (osr_status & AS73211_OSR_STATUS_ADCOF) {
+			dev_err(dev, "%s() ADC overflow\n", __func__);
+			return -EOVERFLOW;
+		}
+		if (osr_status & AS73211_OSR_STATUS_MRESOF) {
+			dev_err(dev, "%s() Measurement result overflow\n", __func__);
+			return -EOVERFLOW;
+		}
+		if (osr_status & AS73211_OSR_STATUS_OUTCONVOF) {
+			dev_err(dev, "%s() Timer overflow\n", __func__);
+			return -EOVERFLOW;
+		}
+		dev_err(dev, "%s() Unexpected status value\n", __func__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int as73211_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct as73211_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		int ret;
+
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		ret = as73211_req_data(data);
+		if (ret < 0) {
+			iio_device_release_direct_mode(indio_dev);
+			return ret;
+		}
+
+		ret = i2c_smbus_read_word_data(data->client, chan->address);
+		iio_device_release_direct_mode(indio_dev);
+		if (ret < 0)
+			return ret;
+
+		*val = ret;
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_OFFSET:
+		*val = AS73211_OFFSET_TEMP;
+		*val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) * 1000000;
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			*val = AS73211_SCALE_TEMP;
+			*val2 = (AS73211_SCALE_TEMP - (int)AS73211_SCALE_TEMP) * 1000000;
+			return IIO_VAL_INT_PLUS_MICRO;
+
+		case IIO_INTENSITY: {
+			unsigned int scale;
+
+			switch (chan->channel2) {
+			case IIO_MOD_X:
+				scale = AS73211_SCALE_X;
+				break;
+			case IIO_MOD_Y:
+				scale = AS73211_SCALE_Y;
+				break;
+			case IIO_MOD_Z:
+				scale = AS73211_SCALE_Z;
+				break;
+			default:
+				return -EINVAL;
+			}
+			scale /= as73211_gain(data);
+			scale /= as73211_integration_time_1024cyc(data);
+			*val = scale;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}}
+
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		/* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */
+		*val = BIT(FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) * 1024000;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = as73211_gain(data);
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_INT_TIME: {
+		unsigned int time_us;
+
+		mutex_lock(&data->mutex);
+		time_us = as73211_integration_time_us(data, as73211_integration_time_1024cyc(data));
+		mutex_unlock(&data->mutex);
+		*val = time_us / USEC_PER_SEC;
+		*val2 = time_us % USEC_PER_SEC;
+		return IIO_VAL_INT_PLUS_MICRO;
+	}}
+
+	return -EINVAL;
+}
+
+static int as73211_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length, long mask)
+{
+	struct as73211_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*length = ARRAY_SIZE(as73211_samp_freq_avail);
+		*vals = as73211_samp_freq_avail;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*length = ARRAY_SIZE(as73211_hardwaregain_avail);
+		*vals = as73211_hardwaregain_avail;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(data->int_time_avail);
+		*vals = data->int_time_avail;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	}
+
+	return -EINVAL;
+}
+
+static int _as73211_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan __always_unused,
+			       int val, int val2, long mask)
+{
+	struct as73211_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ: {
+		int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */;  /* 1024, 2048, ... */
+
+		/* val must be 1024 * 2^x */
+		if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val ||
+				!is_power_of_2(freq_kHz) || val2)
+			return -EINVAL;
+
+		/* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz (=2^10)) */
+		reg_bits = ilog2(freq_kHz) - 10;
+		if (!FIELD_FIT(AS73211_CREG3_CCLK_MASK, reg_bits))
+			return -EINVAL;
+
+		data->creg3 &= ~AS73211_CREG3_CCLK_MASK;
+		data->creg3 |= FIELD_PREP(AS73211_CREG3_CCLK_MASK, reg_bits);
+		as73211_integration_time_calc_avail(data);
+
+		ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_CREG3, data->creg3);
+		if (ret < 0)
+			return ret;
+
+		return 0;
+	}
+	case IIO_CHAN_INFO_HARDWAREGAIN: {
+		unsigned int reg_bits;
+
+		if (val < 0 || !is_power_of_2(val) || val2)
+			return -EINVAL;
+
+		/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
+		reg_bits = 13 - ilog2(val);
+		if (!FIELD_FIT(AS73211_CREG1_GAIN_MASK, reg_bits))
+			return -EINVAL;
+
+		data->creg1 &= ~AS73211_CREG1_GAIN_MASK;
+		data->creg1 |= FIELD_PREP(AS73211_CREG1_GAIN_MASK, reg_bits);
+
+		ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_CREG1, data->creg1);
+		if (ret < 0)
+			return ret;
+
+		return 0;
+	}
+	case IIO_CHAN_INFO_INT_TIME: {
+		int val_us = val * USEC_PER_SEC + val2;
+		int time_ms;
+		int reg_bits;
+
+		/* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */
+		int f_samp_1_024mhz = BIT(FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3));
+
+		/*
+		 * time_ms = time_us * US_PER_MS * f_samp_1_024mhz / MHZ_PER_HZ
+		 *         = time_us * f_samp_1_024mhz / 1000
+		 */
+		time_ms = (val_us * f_samp_1_024mhz) / 1000;  /* 1 ms, 2 ms, ... (power of two) */
+		if (time_ms < 0 || !is_power_of_2(time_ms) || time_ms > AS73211_SAMPLE_TIME_MAX_MS)
+			return -EINVAL;
+
+		reg_bits = ilog2(time_ms);
+		if (!FIELD_FIT(AS73211_CREG1_TIME_MASK, reg_bits))
+			return -EINVAL;  /* not possible due to previous tests */
+
+		data->creg1 &= ~AS73211_CREG1_TIME_MASK;
+		data->creg1 |= FIELD_PREP(AS73211_CREG1_TIME_MASK, reg_bits);
+
+		ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_CREG1, data->creg1);
+		if (ret < 0)
+			return ret;
+
+		return 0;
+	}}
+
+	return -EINVAL;
+}
+
+static int as73211_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct as73211_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	/* Need to switch to config mode ... */
+	if ((data->osr & AS73211_OSR_DOS_MASK) != AS73211_OSR_DOS_CONFIG) {
+		data->osr &= ~AS73211_OSR_DOS_MASK;
+		data->osr |= AS73211_OSR_DOS_CONFIG;
+
+		ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_OSR, data->osr);
+		if (ret < 0)
+			goto error_release;
+	}
+
+	ret = _as73211_write_raw(indio_dev, chan, val, val2, mask);
+
+error_release:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static irqreturn_t as73211_ready_handler(int irq __always_unused, void *priv)
+{
+	struct as73211_data *data = iio_priv(priv);
+
+	complete(&data->completion);
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t as73211_trigger_handler(int irq __always_unused, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct as73211_data *data = iio_priv(indio_dev);
+	struct {
+		__le16 chan[4];
+		s64 ts __aligned(8);
+	} scan;
+	int data_result, ret;
+
+	mutex_lock(&data->mutex);
+
+	data_result = as73211_req_data(data);
+
+	if (*indio_dev->active_scan_mask == AS73211_SCAN_MASK_ALL) {
+		/* Optimization for reading all (color + temperature) channels */
+		u8 addr = as73211_channels[0].address;
+		struct i2c_msg msgs[] = {
+			{
+				.addr = data->client->addr,
+				.flags = 0,
+				.len = 1,
+				.buf = &addr,
+			},
+			{
+				.addr = data->client->addr,
+				.flags = I2C_M_RD,
+				.len = sizeof(scan.chan),
+				.buf = (u8 *)&scan.chan,
+			},
+		};
+
+		ret = i2c_transfer(data->client->adapter, msgs, ARRAY_SIZE(msgs));
+		if (ret < 0)
+			goto done;
+	} else {
+		/* Optimization for reading only color channels */
+
+		/* AS73211 starts reading at address 2 */
+		ret = i2c_master_recv(data->client,
+				(char *)&scan.chan[1], 3 * sizeof(scan.chan[1]));
+		if (ret < 0)
+			goto done;
+	}
+
+	if (data_result) {
+		/*
+		 * Saturate all channels (useful for overflows). Temperature channel
+		 * is not affected by overflows.
+		 */
+		scan.chan[1] = cpu_to_le16(U16_MAX);
+		scan.chan[2] = cpu_to_le16(U16_MAX);
+		scan.chan[3] = cpu_to_le16(U16_MAX);
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
+
+done:
+	mutex_unlock(&data->mutex);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static const struct iio_info as73211_info = {
+	.read_raw = as73211_read_raw,
+	.read_avail = as73211_read_avail,
+	.write_raw = as73211_write_raw,
+};
+
+static int as73211_power(struct iio_dev *indio_dev, bool state)
+{
+	struct as73211_data *data = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (state)
+		data->osr &= ~AS73211_OSR_PD;
+	else
+		data->osr |= AS73211_OSR_PD;
+
+	ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_OSR, data->osr);
+
+	mutex_unlock(&data->mutex);
+
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int as73211_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct as73211_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	mutex_init(&data->mutex);
+	init_completion(&data->completion);
+
+	indio_dev->dev.parent = dev;
+	indio_dev->info = &as73211_info;
+	indio_dev->name = AS73211_DRV_NAME;
+	indio_dev->channels = as73211_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as73211_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
+	if (ret < 0)
+		return ret;
+	data->osr = ret;
+
+	/* reset device */
+	data->osr |= AS73211_OSR_SW_RES;
+	ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_OSR, data->osr);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_OSR);
+	if (ret < 0)
+		return ret;
+	data->osr = ret;
+
+	/*
+	 * Reading AGEN is only possible after reset (AGEN is not available if
+	 * device is in measurement mode).
+	 */
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_AGEN);
+	if (ret < 0)
+		return ret;
+
+	/* At the time of writing this driver, only DEVID 2 and MUT 1 is known. */
+	if ((ret & AS73211_AGEN_DEVID_MASK) != AS73211_AGEN_DEVID(2) ||
+	    (ret & AS73211_AGEN_MUT_MASK) != AS73211_AGEN_MUT(1))
+		return -ENODEV;
+
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_CREG1);
+	if (ret < 0)
+		return ret;
+	data->creg1 = ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_CREG2);
+	if (ret < 0)
+		return ret;
+	data->creg2 = ret;
+
+	ret = i2c_smbus_read_byte_data(data->client, AS73211_REG_CREG3);
+	if (ret < 0)
+		return ret;
+	data->creg3 = ret;
+	as73211_integration_time_calc_avail(data);
+
+	/* enable device */
+	ret = as73211_power(indio_dev, true);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, as73211_trigger_handler, NULL);
+	if (ret < 0)
+		goto powerdown;
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(&client->dev, client->irq,
+				NULL,
+				as73211_ready_handler,
+				IRQF_ONESHOT,
+				client->name, indio_dev);
+		if (ret < 0)
+			goto powerdown;
+	}
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret < 0)
+		goto powerdown;
+
+	return 0;
+
+powerdown:
+	as73211_power(indio_dev, false);
+	return ret;
+}
+
+static int as73211_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	as73211_power(indio_dev, false);
+
+	return 0;
+}
+
+static int __maybe_unused as73211_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return as73211_power(indio_dev, false);
+}
+
+static int __maybe_unused as73211_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+
+	return as73211_power(indio_dev, true);
+}
+
+static SIMPLE_DEV_PM_OPS(as73211_pm_ops, as73211_suspend, as73211_resume);
+
+static const struct of_device_id as73211_of_match[] = {
+	{ .compatible = "ams,as73211" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, as73211_of_match);
+
+static const struct i2c_device_id as73211_id[] = {
+	{ "as73211", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, as73211_id);
+
+static struct i2c_driver as73211_driver = {
+	.driver = {
+		.name           = AS73211_DRV_NAME,
+		.of_match_table = as73211_of_match,
+		.pm             = &as73211_pm_ops,
+	},
+	.probe_new  = as73211_probe,
+	.remove     = as73211_remove,
+	.id_table   = as73211_id,
+};
+module_i2c_driver(as73211_driver);
+
+MODULE_AUTHOR("Christian Eggers <ceggers@arri.de>");
+MODULE_DESCRIPTION("AS73211 XYZ True Color Sensor driver");
+MODULE_LICENSE("GPL");
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
@ 2020-07-31  7:34   ` Andy Shevchenko
  2020-07-31 10:52     ` Christian Eggers
  2020-08-01 15:29   ` Jonathan Cameron
  1 sibling, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-31  7:34 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Rob Herring, Jonathan Cameron, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	devicetree, Linux Kernel Mailing List

On Fri, Jul 31, 2020 at 10:03 AM Christian Eggers <ceggers@arri.de> wrote:
>
> Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.
>
> This driver has no built-in trigger. In order for making triggered
> measurements, an external (software) trigger driver like
> iio-trig-hrtimer or iio-trig-sysfs is required.
>
> The sensor supports single and continuous measurement modes. The latter
> is not used by design as this would require tight timing synchronization
> between hardware and driver without much benefit.

Thanks for an update. My comments below.

...

> +static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000, 8192000 };

This looks related to the below mentioned 1.024MHz.

Perhaps add a definition above and comment here?

#define AS73211_BASE_FREQ_1024KHZ   1024000

/* Available sample frequencies are power of two multiplier by 1.024MHz */
(Rephrase it better)

> +static const int as73211_hardwaregain_avail[] = { 1, 2, 4, 8, 16, 32, 64, 128,
> +                                                  256, 512, 1024, 2048 };

Indentation. Better is
... foo = {
  bar, baz,
};

And in both cases leave comma at the last value.

...

> +/**
> + * struct as73211_data - Instance data for one AS73211
> + * @client: I2C client.
> + * @osr:    Cached Operational State Register.
> + * @creg1:  Cached Configuration Register 1.
> + * @creg2:  Cached Configuration Register 2.
> + * @creg3:  Cached Configuration Register 3.

> + * @mutex:  Keeps cached registers in synch with the device.

sync

> + * @completion: Completion to wait for interrupt.
> + * @int_time_avail: Available integration times (depend on sampling frequency).
> + */

...

> +/* integration time in units of 1024 clock cycles */
Unify this with below one. Or the other way around, i.o.w. join one of
them into the other.

> +static unsigned int as73211_integration_time_1024cyc(struct as73211_data *data)
> +{
> +       /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
> +       return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
> +}

...

> +static unsigned int as73211_integration_time_us(struct as73211_data *data,
> +                                                unsigned int integration_time_1024cyc)
> +{
> +       /*
> +        * f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz)
> +        * t_cycl is configured in CREG1 in powers of 2 (x 1024 cycles)
> +        * t_int_us = 1 / (f_samp) * t_cycl * US_PER_SEC
> +        *          = 1 / (2^CREG3_CCLK * 1,024,000) * 2^CREG1_CYCLES * 1,024 * US_PER_SEC
> +        *          = 2^(-CREG3_CCLK) * 2^CREG1_CYCLES * 1,000

> +        *            in order to get rid of negative exponents, we extend the
> +        *            "fraction" by 2^3 (3 == CREG3_CCLK,max)

In the parentheses swap left and right parts for better reading.

Perhaps shift left to have formulas separated from text visually.

> +        *          = 2^(3-CREG3_CCLK) * 2^CREG1_CYCLES * 125

Okay, 125 = 1000/2^3.

> +        */
> +       return BIT(3 - FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) *
> +               integration_time_1024cyc * 125;
> +}

...

> +               data->int_time_avail[i * 2] = time_us / USEC_PER_SEC;

I would do + 0, but it's up to you (complete style preference).

> +               data->int_time_avail[i * 2 + 1] = time_us % USEC_PER_SEC;

...

> +       unsigned int time_us = as73211_integration_time_us(data,
> +                                                           as73211_integration_time_1024cyc(data));

One line?

...

> +               /* f_samp is configured in CREG3 in powers of 2 (x 1.024 MHz) */
> +               *val = BIT(FIELD_GET(AS73211_CREG3_CCLK_MASK, data->creg3)) * 1024000;

As above mentioned, definition can be used.

...


> +               int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */;  /* 1024, 2048, ... */
> +
> +               /* val must be 1024 * 2^x */
> +               if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val ||
> +                               !is_power_of_2(freq_kHz) || val2)
> +                       return -EINVAL;

Please, define HZ_PER_KHZ locally. It will really help when we move
these definitions to a global level.

...

> +               /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
> +               reg_bits = 13 - ilog2(val);

13 is the second time in the code. Deserves a descriptive definition.

...

> +       indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +       if (indio_dev == NULL)

if (!indio_dev)

> +               return -ENOMEM;

...

> +       indio_dev->dev.parent = dev;

Doesn't IIO core do this for you?

...

> +       /* At the time of writing this driver, only DEVID 2 and MUT 1 is known. */

are known

...

> +       /* enable device */

This is confusing and by the fact useless.

...

> +       ret = devm_iio_device_register(dev, indio_dev);
> +       if (ret < 0)
> +               goto powerdown;
> +
> +       return 0;

> +powerdown:
> +       as73211_power(indio_dev, false);
> +       return ret;

devm_*() is tricky. Here you broke ordering heavily. So, consider to
add this under devm_add_action_or_reset().

...

> +static int as73211_remove(struct i2c_client *client)
> +{
> +       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +       as73211_power(indio_dev, false);
> +
> +       return 0;
> +}

And as a result of the above this will be gone.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31  7:34   ` Andy Shevchenko
@ 2020-07-31 10:52     ` Christian Eggers
  2020-07-31 15:41       ` Andy Shevchenko
  0 siblings, 1 reply; 9+ messages in thread
From: Christian Eggers @ 2020-07-31 10:52 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Rob Herring, Jonathan Cameron, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	devicetree, Linux Kernel Mailing List

Hi Andy,

thanks for further review. If nobody else sends comments, I'll
publish the next version tonight. Maybe we could clarify the questions
below in time.

Best regards
Christian


> W=1 (not V=1) runs kernel doc validation script.
without V=1, I get nothing. Neither excess nor missing members
are reported on my system.


On Friday, 31 July 2020, 09:34:02 CEST, Andy Shevchenko wrote:
> On Fri, Jul 31, 2020 at 10:03 AM Christian Eggers <ceggers@arri.de> wrote:
> > +static const int as73211_samp_freq_avail[] = { 1024000, 2048000, 4096000,
> > 8192000 };
> This looks related to the below mentioned 1.024MHz.
> 
> Perhaps add a definition above and comment here?
> 
> #define AS73211_BASE_FREQ_1024KHZ   1024000
added similar define in v5. The array looks like the following now

static const int as73211_samp_freq_avail[] = {
	AS73211_SAMPLE_FREQ_BASE,
	AS73211_SAMPLE_FREQ_BASE * 2,
	AS73211_SAMPLE_FREQ_BASE * 4,
	AS73211_SAMPLE_FREQ_BASE * 8
};


> > +/* integration time in units of 1024 clock cycles */
> 
> Unify this with below one. Or the other way around, i.o.w. join one of
> them into the other.
> 
> > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data
> > *data) +{
> > +       /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
> > +       return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
> > +}
I'm not sure, whether this is possible. as73211_integration_time_1024cyc()
returns the current setting from hardware. as73211_integration_time_us()
calculates the resulting time. But as73211_integration_time_us() is also
called in as73211_integration_time_calc_avail() inside the loop.

> > +       unsigned int time_us = as73211_integration_time_us(data,
> > +                                                          as73211_integration_time_1024cyc(data));
> One line?
checkpatch complains... ignore?


> > +               int reg_bits, freq_kHz = val / 1000 /* HZ_PER_KHZ */;  /*
> > 1024, 2048, ... */ +
> > +               /* val must be 1024 * 2^x */
> > +               if (val < 0 || (freq_kHz * 1000 /* HZ_PER_KHZ */) != val
> > ||
> > +                               !is_power_of_2(freq_kHz) || val2)
> > +                       return -EINVAL;
> 
> Please, define HZ_PER_KHZ locally. It will really help when we move
> these definitions to a global level.
ok

> 
> ...
> 
> > +               /* gain can be calculated from CREG1 as 2^(13 -
> > CREG1_GAIN) */ +               reg_bits = 13 - ilog2(val);
> 
> 13 is the second time in the code. Deserves a descriptive definition.
I'm unsure how to solve this. Possible values for gain:

CREG1[7:4]  | gain
-----------------------------
0           | 2048x
1           | 1024x
2           |  512x
...         |  ...
13          |    1x

#define AS73211_CREG1_GAIN_1_NON_SHIFTED 13  // this define is CREG1 related, but not shifted to the right position

static unsigned int as73211_gain(struct as73211_data *data)
{
	/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
	return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
}

---- or ----

#define AS73211_CREG1_GAIN_1 FIELD_PREP(AS73211_CREG1_GAIN_MASK, 13)

static unsigned int as73211_gain(struct as73211_data *data)
{
	/* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
	return BIT(FIELD_GET(AS73211_CREG1_GAIN_MASK, AS73211_CREG1_GAIN_1) - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
}


> > +       indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +       if (indio_dev == NULL)
> 
> if (!indio_dev)
> 
> > +               return -ENOMEM;
> 
> ...
> 
> > +       indio_dev->dev.parent = dev;
> 
> Doesn't IIO core do this for you?
devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc().
I already looked around, but I didn't find. And after debugging
v5.4, devm_iio_device_alloc() definitely doesn't do it.

> > +       ret = devm_iio_device_register(dev, indio_dev);
> > +       if (ret < 0)
> > +               goto powerdown;
> > +
> > +       return 0;
> > 
> > +powerdown:
> > +       as73211_power(indio_dev, false);
> > +       return ret;
> 
> devm_*() is tricky. Here you broke ordering heavily. So, consider to
> add this under devm_add_action_or_reset().
Sorry, my mistake! I already felt that something may be wrong here...




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31 10:52     ` Christian Eggers
@ 2020-07-31 15:41       ` Andy Shevchenko
  2020-07-31 16:19         ` Jonathan Cameron
  0 siblings, 1 reply; 9+ messages in thread
From: Andy Shevchenko @ 2020-07-31 15:41 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Rob Herring, Jonathan Cameron, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	devicetree, Linux Kernel Mailing List

On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <ceggers@arri.de> wrote:

> > W=1 (not V=1) runs kernel doc validation script.
> without V=1, I get nothing. Neither excess nor missing members
> are reported on my system.

It's strange.

...

> > Perhaps add a definition above and comment here?
> >
> > #define AS73211_BASE_FREQ_1024KHZ   1024000
> added similar define in v5. The array looks like the following now
>
> static const int as73211_samp_freq_avail[] = {

>         AS73211_SAMPLE_FREQ_BASE,

' * 1'

>         AS73211_SAMPLE_FREQ_BASE * 2,
>         AS73211_SAMPLE_FREQ_BASE * 4,
>         AS73211_SAMPLE_FREQ_BASE * 8
> };

...

> > > +/* integration time in units of 1024 clock cycles */
> >
> > Unify this with below one. Or the other way around, i.o.w. join one of
> > them into the other.
> >
> > > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data
> > > *data) +{
> > > +       /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
> > > +       return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
> > > +}
> I'm not sure, whether this is possible. as73211_integration_time_1024cyc()
> returns the current setting from hardware. as73211_integration_time_us()
> calculates the resulting time. But as73211_integration_time_us() is also
> called in as73211_integration_time_calc_avail() inside the loop.

What I meant is solely comments to be joined, not the code.

...

> > > +       unsigned int time_us = as73211_integration_time_us(data,
> > > +                                                          as73211_integration_time_1024cyc(data));
> > One line?

> checkpatch complains... ignore?

Hmm... is it over 100? Or you are using some old tools to work with
the kernel...

...

> > > +               /* gain can be calculated from CREG1 as 2^(13 -
> > > CREG1_GAIN) */ +               reg_bits = 13 - ilog2(val);
> >
> > 13 is the second time in the code. Deserves a descriptive definition.

> I'm unsure how to solve this. Possible values for gain:
>
> CREG1[7:4]  | gain
> -----------------------------
> 0           | 2048x
> 1           | 1024x
> 2           |  512x
> ...         |  ...
> 13          |    1x
>
> #define AS73211_CREG1_GAIN_1_NON_SHIFTED 13  // this define is CREG1 related, but not shifted to the right position
>
> static unsigned int as73211_gain(struct as73211_data *data)
> {
>         /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
>         return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
> }

This way (w/o _NON_SHIFTED suffix) if both 13:s in the code are of the
same meaning.

...

> > > +       indio_dev->dev.parent = dev;
> >
> > Doesn't IIO core do this for you?
> devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc().
> I already looked around, but I didn't find. And after debugging
> v5.4, devm_iio_device_alloc() definitely doesn't do it.

Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9.

Recently IIO gained some features among which I think the one that
assigns parent devices.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31 15:41       ` Andy Shevchenko
@ 2020-07-31 16:19         ` Jonathan Cameron
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2020-07-31 16:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Christian Eggers, Rob Herring, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	devicetree, Linux Kernel Mailing List

On Fri, 31 Jul 2020 18:41:47 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Jul 31, 2020 at 1:52 PM Christian Eggers <ceggers@arri.de> wrote:
> 
> > > W=1 (not V=1) runs kernel doc validation script.  
> > without V=1, I get nothing. Neither excess nor missing members
> > are reported on my system.  
> 
> It's strange.
> 
> ...
> 
> > > Perhaps add a definition above and comment here?
> > >
> > > #define AS73211_BASE_FREQ_1024KHZ   1024000  
> > added similar define in v5. The array looks like the following now
> >
> > static const int as73211_samp_freq_avail[] = {  
> 
> >         AS73211_SAMPLE_FREQ_BASE,  
> 
> ' * 1'
> 
> >         AS73211_SAMPLE_FREQ_BASE * 2,
> >         AS73211_SAMPLE_FREQ_BASE * 4,
> >         AS73211_SAMPLE_FREQ_BASE * 8
> > };  
> 
> ...
> 
> > > > +/* integration time in units of 1024 clock cycles */  
> > >
> > > Unify this with below one. Or the other way around, i.o.w. join one of
> > > them into the other.
> > >  
> > > > +static unsigned int as73211_integration_time_1024cyc(struct as73211_data
> > > > *data) +{
> > > > +       /* integration time in CREG1 is in powers of 2 (x 1024 cycles) */
> > > > +       return BIT(FIELD_GET(AS73211_CREG1_TIME_MASK, data->creg1));
> > > > +}  
> > I'm not sure, whether this is possible. as73211_integration_time_1024cyc()
> > returns the current setting from hardware. as73211_integration_time_us()
> > calculates the resulting time. But as73211_integration_time_us() is also
> > called in as73211_integration_time_calc_avail() inside the loop.  
> 
> What I meant is solely comments to be joined, not the code.
> 
> ...
> 
       unsigned int time_us = as73211_integration_time_us(data, as73211_integration_time_1024cyc(data));  
> > > One line?  
> 
> > checkpatch complains... ignore?  
> 
> Hmm... is it over 100? Or you are using some old tools to work with
> the kernel...

It's over a 100... (about 103 by the handy scale at the top of my email client :)

> 
> ...
> 
> > > > +               /* gain can be calculated from CREG1 as 2^(13 -
> > > > CREG1_GAIN) */ +               reg_bits = 13 - ilog2(val);  
> > >
> > > 13 is the second time in the code. Deserves a descriptive definition.  
> 
> > I'm unsure how to solve this. Possible values for gain:
> >
> > CREG1[7:4]  | gain
> > -----------------------------
> > 0           | 2048x
> > 1           | 1024x
> > 2           |  512x
> > ...         |  ...
> > 13          |    1x
> >
> > #define AS73211_CREG1_GAIN_1_NON_SHIFTED 13  // this define is CREG1 related, but not shifted to the right position
> >
> > static unsigned int as73211_gain(struct as73211_data *data)
> > {
> >         /* gain can be calculated from CREG1 as 2^(13 - CREG1_GAIN) */
> >         return BIT(AS73211_CREG1_GAIN_1_NON_SHIFTED - FIELD_GET(AS73211_CREG1_GAIN_MASK, data->creg1));
> > }  
> 
> This way (w/o _NON_SHIFTED suffix) if both 13:s in the code are of the
> same meaning.
> 
> ...
> 
> > > > +       indio_dev->dev.parent = dev;  
> > >
> > > Doesn't IIO core do this for you?  
> > devm_iio_device_alloc() doesn't pass 'dev' to iio_device_alloc().
> > I already looked around, but I didn't find. And after debugging
> > v5.4, devm_iio_device_alloc() definitely doesn't do it.  
> 
> Why are you talking about v5.4? We are in v5.8 cycle contributing to v5.9.

This will be 5.10 now.  5.9 cycle for new stuff via IIO effectively closed
last week. 

> 
> Recently IIO gained some features among which I think the one that
> assigns parent devices.
> 
yup. This should be in linux-next now for the coming merge window (which probably opens on Sunday).

Note that we have lots of time to tidy up loose ends for this one as it will only sit
in my tree for next few weeks if I do pick it up.

Jonathan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support
  2020-07-31  7:01 ` [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
@ 2020-07-31 22:45   ` Rob Herring
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2020-07-31 22:45 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Peter Meerwald-Stadler, Hartmut Knaack, Jonathan Cameron,
	Rob Herring, Andy Shevchenko, devicetree, Lars-Peter Clausen,
	Jonathan Cameron, linux-iio, linux-kernel

On Fri, 31 Jul 2020 09:01:13 +0200, Christian Eggers wrote:
> Add DT bindings for AMS AS73211 XYZ True Color Sensor.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> ---
>  .../bindings/iio/light/ams,as73211.yaml       | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4 2/2] iio: light: as73211: New driver
  2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
  2020-07-31  7:34   ` Andy Shevchenko
@ 2020-08-01 15:29   ` Jonathan Cameron
  1 sibling, 0 replies; 9+ messages in thread
From: Jonathan Cameron @ 2020-08-01 15:29 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Rob Herring, Andy Shevchenko, Jonathan Cameron, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-iio,
	devicetree, linux-kernel

On Fri, 31 Jul 2020 09:01:14 +0200
Christian Eggers <ceggers@arri.de> wrote:

> Support for AMS AS73211 JENCOLOR(R) Digital XYZ Sensor.
> 
> This driver has no built-in trigger. In order for making triggered
> measurements, an external (software) trigger driver like
> iio-trig-hrtimer or iio-trig-sysfs is required.
> 
> The sensor supports single and continuous measurement modes. The latter
> is not used by design as this would require tight timing synchronization
> between hardware and driver without much benefit.
> 
> Datasheet: https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-b302-c2fd-e30a-c98df87616df
> Signed-off-by: Christian Eggers <ceggers@arri.de>

A question inline about flipping to config mode mid capture.

> ---
>  MAINTAINERS                 |   7 +
>  drivers/iio/light/Kconfig   |  15 +
>  drivers/iio/light/Makefile  |   1 +
>  drivers/iio/light/as73211.c | 780 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 803 insertions(+)
>  create mode 100644 drivers/iio/light/as73211.c
> 
...
> +
> +static int as73211_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct as73211_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
> +
> +	/* Need to switch to config mode ... */

Is this safe whilst we are doing a capture?
You may want to claim_direct_mode for write_raw to ensure we don't get such a
race.

> +	if ((data->osr & AS73211_OSR_DOS_MASK) != AS73211_OSR_DOS_CONFIG) {
> +		data->osr &= ~AS73211_OSR_DOS_MASK;
> +		data->osr |= AS73211_OSR_DOS_CONFIG;
> +
> +		ret = i2c_smbus_write_byte_data(data->client, AS73211_REG_OSR, data->osr);
> +		if (ret < 0)
> +			goto error_release;
> +	}
> +
> +	ret = _as73211_write_raw(indio_dev, chan, val, val2, mask);
> +
> +error_release:
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31  7:01 [PATCH v4 0/2] iio: light: Support AMS AS73211 digital XYZ sensor Christian Eggers
2020-07-31  7:01 ` [PATCH v4 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
2020-07-31 22:45   ` Rob Herring
2020-07-31  7:01 ` [PATCH v4 2/2] iio: light: as73211: New driver Christian Eggers
2020-07-31  7:34   ` Andy Shevchenko
2020-07-31 10:52     ` Christian Eggers
2020-07-31 15:41       ` Andy Shevchenko
2020-07-31 16:19         ` Jonathan Cameron
2020-08-01 15:29   ` Jonathan Cameron

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git