[PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[Abdel Alkuor]

as6200 is high accuracy temperature sensor of -/+ 0.4C degree
with a range between -40C to 125C degrees

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
Changes in v2:
  - Add vdd-supply

 .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml

diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
new file mode 100644
index 000000000000..a1817795cdca
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS AS6200 Temperature Sensor
+
+maintainers:
+  - Abdel Alkuor <alkuor@gmail.com>
+
+description: |
+  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
+
+properties:
+  compatible:
+    const: ams,as6200
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        temperature-sensor@48 {
+            compatible = "ams,as6200";
+            reg = <0x48>;
+            vdd-supply = <&vdd>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
+        };
+    };
+...
-- 
2.34.1

[PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

[Abdel Alkuor]

as6200 is a high accuracy temperature sensor of 0.0625C with a range
between -40 to 125 Celsius degrees.

The driver implements the alert trigger event in comparator mode where
consecutive faults are converted into periods, high/low temperature
thresholds require to be above/below the set limit for n seconds for
the alert to be triggered/cleared. The alert is only cleared when the
current temperature is below the low temperature threshold for n seconds.

The driver supports the following:
- show available sampling frequencey
- read/write sampling frequency
- read raw temperature
- read scaling factor
- read/write temperature period that needs to be met for low/high
  temperature thresholds to trigger an alert
- show available temperature period thresholds
- buffer trigger
- temperature alert event trigger

Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf

Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
Changes in v2:
  - Add AS6200 in Kconfig in alphabetical order
  - Add as6200.o in Makefile in alphabetical order
  - Fix spelling Auther -> Author
  - Group iio/*.h
  - Arrange *.h into alpabetical order
  - Prefix defines with AS6200
  - Add mask bits for temperature
  - Remove dev, iio_dev, data from as6200 struct
  - Add a comment for the lock usage
  - Convert as6200_samp_freq into 2d array
  - Fix spaces around curly brackets
  - Remove unneccessry "," after NULL terminated arrays
  - Add an array for consecutive faults conversion from samples to periods
  - Remove custom consecutive faults attribute and use IIO_EV_INFO_PERIOD instead
  - Rename as6200_temp_channels to as6200_channels
  - Remove unneccessry zerod indices from as6200_channels
  - Add available sampling frequency attribute
  - Use regmap_update_bits instead of as6200_modify_config_reg
  - Remove as6200_read_config_reg
  - Retrieve driver priv data using iio_priv
  - Retrieve all temperature readings using the new temp mask
  - Use the scaling factor directly instead of using macro
  - Use mutex guard in the event handler
  - Return IRQ_NONE instead of returning an error in as6200_event_handler
  - Remove lock from the trigger handler and put the data on the stack
  - Add custom attribute to show the avaiable temp periods
  - Add in_tempY_threshold_either_period and in_temp_period_available to iio sysfs
    documentation
  - Use as6200 as the name of iio device
  - Drop as6200_remove and use devm_iio_device_register
  - Populate i2c_set_clientdata with indio_dev
  - grab and enable a regulator in probe
  - Update commit message

 Documentation/ABI/testing/sysfs-bus-iio |   4 +-
 drivers/iio/temperature/Kconfig         |  12 +-
 drivers/iio/temperature/Makefile        |   1 +
 drivers/iio/temperature/as6200.c        | 493 ++++++++++++++++++++++++
 4 files changed, 508 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iio/temperature/as6200.c

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 19cde14f3869..cc96e6592377 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1174,6 +1174,7 @@ What:		/sys/.../events/in_voltageY_roc_rising_period
 What:		/sys/.../events/in_voltageY_roc_falling_period
 What:		/sys/.../events/in_tempY_thresh_rising_period
 What:		/sys/.../events/in_tempY_thresh_falling_period
+What:		/sys/.../events/in_tempY_thresh_either_period
 What:		/sys/.../events/in_tempY_roc_rising_period
 What:		/sys/.../events/in_tempY_roc_falling_period
 What:		/sys/.../events/in_accel_x&y&z_mag_falling_period
@@ -2016,11 +2017,12 @@ Description:
 		equal to X micrometers.
 
 What:		/sys/bus/iio/devices/iio:deviceX/events/in_illuminance_period_available
+What:		/sys/bus/iio/devices/iio:deviceX/events/in_temp_period_available
 Date:		November 2019
 KernelVersion:	5.4
 Contact:	linux-iio@vger.kernel.org
 Description:
-		List of valid periods (in seconds) for which the light intensity
+		List of valid periods (in seconds) for which the condition
 		must be above the threshold level before interrupt is asserted.
 
 What:		/sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index ed384f33e0c7..a0ffbc77e623 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -4,6 +4,17 @@
 #
 menu "Temperature sensors"
 
+config AS6200
+       tristate "AS6200 temperature sensor"
+       depends on I2C
+       select REGMAP_I2C
+       help
+         If you say yes here you get support for AS6200
+         temperature sensor chip connected via I2C.
+
+         This driver can also be built as a module.  If so, the module
+         will be called as6200.
+
 config IQS620AT_TEMP
 	tristate "Azoteq IQS620AT temperature sensor"
 	depends on MFD_IQS62X || COMPILE_TEST
@@ -157,5 +168,4 @@ config MAX31865
 
 	  This driver can also be build as a module. If so, the module
 	  will be called max31865.
-
 endmenu
diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
index dfec8c6d3019..e3d4be6770d2 100644
--- a/drivers/iio/temperature/Makefile
+++ b/drivers/iio/temperature/Makefile
@@ -3,6 +3,7 @@
 # Makefile for industrial I/O temperature drivers
 #
 
+obj-$(CONFIG_AS6200) += as6200.o
 obj-$(CONFIG_IQS620AT_TEMP) += iqs620at-temp.o
 obj-$(CONFIG_LTC2983) += ltc2983.o
 obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
new file mode 100644
index 000000000000..7fcc785871d8
--- /dev/null
+++ b/drivers/iio/temperature/as6200.c
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AMS AS6200 Temperature sensor
+ *
+ * Author: Abdel Alkuor <alkuor@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kstrtox.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+
+#define AS6200_TVAL_REG		0x0
+#define AS6200_CONFIG_REG	0x1
+#define AS6200_TLOW_REG		0x2
+#define AS6200_THIGH_REG	0x3
+
+#define AS6200_CONFIG_AL	BIT(5)
+#define AS6200_CONFIG_CR	GENMASK(7, 6)
+#define AS6200_CONFIG_SM	BIT(8)
+#define AS6200_CONFIG_IM	BIT(9)
+#define AS6200_CONFIG_POL	BIT(10)
+#define AS6200_CONFIG_CF	GENMASK(12, 11)
+
+#define AS6200_TEMP_MASK	GENMASK(15, 4)
+
+struct as6200 {
+	struct regmap *regmap;
+	struct mutex lock; /* Prevent concurrent temp fault processing */
+};
+
+static const int as6200_samp_freq[4][2] = {
+	{ 0, 250000 },
+	{ 1, 0 },
+	{ 4, 0 },
+	{ 8, 0 }
+};
+
+/* Consective faults converted to period */
+static const int as6200_temp_thresh_periods[4][4][2] = {
+	{ { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
+	{ { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
+	{ { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
+	{ { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }
+};
+
+static const struct iio_event_spec as6200_temp_event[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_PERIOD),
+	}
+};
+
+static const struct iio_chan_spec as6200_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE) |
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
+		.scan_type = {
+			.sign = 's',
+			.realbits = 12,
+			.storagebits = 16,
+			.shift = 4,
+		},
+		.event_spec = as6200_temp_event,
+		.num_event_specs = ARRAY_SIZE(as6200_temp_event),
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct regmap_config as6200_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0x7F,
+};
+
+static int as6200_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long mask)
+{
+	struct as6200 *as = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(as->regmap, AS6200_TVAL_REG, &reg);
+		if (ret)
+			return ret;
+
+		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, reg), 11);
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		*val2 = 62500;
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &reg);
+		if (ret)
+			return ret;
+
+		reg = FIELD_GET(AS6200_CONFIG_CR, reg);
+		*val = as6200_samp_freq[reg][0];
+		*val2 = as6200_samp_freq[reg][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int as6200_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*length = ARRAY_SIZE(as6200_samp_freq) * 2;
+		*vals = (int *)as6200_samp_freq;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int as6200_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val,
+			    int val2,
+			    long mask)
+{
+	struct as6200 *as = iio_priv(indio_dev);
+	int cr;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		for (cr = 0; cr < ARRAY_SIZE(as6200_samp_freq); cr++) {
+			if (val == as6200_samp_freq[cr][0] &&
+			    val2 == as6200_samp_freq[cr][1])
+				break;
+		}
+
+		if (cr == ARRAY_SIZE(as6200_samp_freq))
+			return -EINVAL;
+
+		return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
+					  AS6200_CONFIG_CR,
+					  FIELD_PREP(AS6200_CONFIG_CR, cr));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int as6200_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info,
+				   int *val, int *val2)
+{
+	struct as6200 *as = iio_priv(indio_dev);
+	unsigned int reg;
+	unsigned int tmp;
+	int ret;
+	u8 cf;
+	u8 cr;
+
+	switch (dir) {
+	case IIO_EV_DIR_FALLING:
+		reg = AS6200_TLOW_REG;
+		break;
+	case IIO_EV_DIR_RISING:
+		reg = AS6200_THIGH_REG;
+		break;
+	case IIO_EV_DIR_EITHER:
+		reg = AS6200_CONFIG_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_read(as->regmap, reg, &tmp);
+	if (ret)
+		return ret;
+
+	if (info == IIO_EV_INFO_VALUE) {
+		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
+		ret = IIO_VAL_INT;
+	} else {
+		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
+		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
+		*val = as6200_temp_thresh_periods[cr][cf][0];
+		*val2 = as6200_temp_thresh_periods[cr][cf][1];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+	}
+
+	return ret;
+}
+
+static int as6200_write_event_value(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int val, int val2)
+{
+	struct as6200 *as = iio_priv(indio_dev);
+	unsigned int tmp;
+	unsigned int reg;
+	int ret;
+	u8 cr;
+	u8 cf;
+
+	switch (dir) {
+	case IIO_EV_DIR_FALLING:
+		reg = AS6200_TLOW_REG;
+		break;
+	case IIO_EV_DIR_RISING:
+		reg = AS6200_THIGH_REG;
+		break;
+	case IIO_EV_TYPE_THRESH:
+		reg = AS6200_CONFIG_REG;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (info == IIO_EV_INFO_VALUE) {
+		/*
+		 * range without applying the scaling
+		 * factor of 0.06250
+		 */
+		if (val > 2000 || val < -640)
+			return -EINVAL;
+
+		tmp = FIELD_PREP(AS6200_TEMP_MASK, val);
+	} else {
+		ret = regmap_read(as->regmap, reg, &tmp);
+		if (ret)
+			return ret;
+
+		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
+
+		for (cf = 0; cf < ARRAY_SIZE(as6200_temp_thresh_periods); cf++) {
+			if (val == as6200_temp_thresh_periods[cr][cf][0] &&
+			    val2 == as6200_temp_thresh_periods[cr][cf][1])
+				break;
+		}
+
+		if (cf == ARRAY_SIZE(as6200_temp_thresh_periods))
+			return -EINVAL;
+
+		tmp &= ~AS6200_CONFIG_CF;
+		tmp |= FIELD_PREP(AS6200_CONFIG_CF, cf);
+	}
+
+	return regmap_write(as->regmap, reg, tmp);
+}
+
+static irqreturn_t as6200_event_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct as6200 *as = iio_priv(indio_dev);
+	unsigned int alert;
+	enum iio_event_direction dir;
+	int ret;
+
+	guard(mutex)(&as->lock);
+
+	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
+	if (ret)
+		return IRQ_NONE;
+
+	alert = FIELD_GET(AS6200_CONFIG_AL, alert);
+
+	dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+
+	iio_push_event(indio_dev,
+		       IIO_EVENT_CODE(IIO_TEMP, 0, 0,
+				      dir,
+				      IIO_EV_TYPE_THRESH,
+				      0, 0, 0),
+		       iio_get_time_ns(indio_dev));
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t as6200_trigger_handler(int irq, void *private)
+{
+	struct iio_poll_func *pf = private;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct as6200 *as = iio_priv(indio_dev);
+	int ret;
+	u8 data[16];
+
+	ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
+	if (!ret)
+		iio_push_to_buffers_with_timestamp(indio_dev, data,
+						   iio_get_time_ns(indio_dev));
+
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static ssize_t
+in_temp_period_available_show(struct device *dev,
+			      struct device_attribute *attr,
+			      char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct as6200 *as = iio_priv(indio_dev);
+	int len = 0;
+	unsigned int cr;
+	u8 cf;
+	int ret;
+
+	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &cr);
+	if (ret)
+		return ret;
+
+	cr = FIELD_GET(AS6200_CONFIG_CR, cr);
+
+	for (cf = 0; cf < ARRAY_SIZE(as6200_temp_thresh_periods); cf++)
+		len += sprintf(buf + len, "%d.%06d ",
+			       as6200_temp_thresh_periods[cr][cf][0],
+			       as6200_temp_thresh_periods[cr][cf][1]);
+
+	len += sprintf(buf + len, "\n");
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RO(in_temp_period_available, 0);
+
+static struct attribute *as6200_event_attributes[] = {
+	&iio_dev_attr_in_temp_period_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group as6200_event_attribute_group = {
+	.attrs = as6200_event_attributes,
+};
+
+static const struct iio_info as6200_temp_info = {
+	.event_attrs = &as6200_event_attribute_group,
+	.read_raw = &as6200_read_raw,
+	.read_avail = &as6200_read_avail,
+	.write_raw = &as6200_write_raw,
+	.read_event_value = &as6200_read_event_value,
+	.write_event_value = &as6200_write_event_value,
+};
+
+static int as6200_probe(struct i2c_client *client)
+{
+	struct as6200 *as;
+	struct iio_dev *indio_dev;
+	int ret;
+	struct device *dev = &client->dev;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+		return -EINVAL;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*as));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	as = iio_priv(indio_dev);
+
+	as->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
+	if (IS_ERR(as->regmap))
+		return PTR_ERR(as->regmap);
+
+	mutex_init(&as->lock);
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = as6200_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as6200_channels);
+	indio_dev->name = "as6200";
+	indio_dev->info = &as6200_temp_info;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      NULL,
+					      as6200_trigger_handler,
+					      NULL);
+	if (ret)
+		return ret;
+
+	if (client->irq) {
+		ret = devm_request_threaded_irq(dev,
+						client->irq,
+						NULL,
+						&as6200_event_handler,
+						IRQF_ONESHOT,
+						client->name,
+						indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Could not get and enable regulator %d\n",
+				     ret);
+
+	i2c_set_clientdata(client, indio_dev);
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int __maybe_unused as6200_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
+
+	if (client->irq)
+		disable_irq(client->irq);
+
+	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
+				  AS6200_CONFIG_SM, AS6200_CONFIG_SM);
+}
+
+static int __maybe_unused as6200_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
+
+	if (client->irq)
+		enable_irq(client->irq);
+
+	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
+				  AS6200_CONFIG_SM, 0);
+}
+
+static const struct dev_pm_ops as6200_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
+};
+
+static const struct i2c_device_id as6200_id_table[] = {
+	{ "as6200" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, as6200_id_table);
+
+static const struct of_device_id as6200_of_match[] = {
+	{ .compatible = "ams,as6200" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, as6200_of_match);
+
+static struct i2c_driver as6200_driver = {
+	.driver = {
+		.name = "as6200",
+		.pm = pm_sleep_ptr(&as6200_pm_ops),
+		.of_match_table = as6200_of_match,
+	},
+	.probe = as6200_probe,
+	.id_table = as6200_id_table,
+};
+module_i2c_driver(as6200_driver);
+
+MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
+MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

[Jonathan Cameron]

On Fri,  1 Dec 2023 23:16:51 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> as6200 is a high accuracy temperature sensor of 0.0625C with a range
> between -40 to 125 Celsius degrees.
> 
> The driver implements the alert trigger event in comparator mode where
> consecutive faults are converted into periods, high/low temperature
> thresholds require to be above/below the set limit for n seconds for
> the alert to be triggered/cleared. The alert is only cleared when the
> current temperature is below the low temperature threshold for n seconds.
> 
> The driver supports the following:
> - show available sampling frequencey
> - read/write sampling frequency
> - read raw temperature
> - read scaling factor
> - read/write temperature period that needs to be met for low/high
>   temperature thresholds to trigger an alert
> - show available temperature period thresholds
> - buffer trigger
> - temperature alert event trigger

Hi Abdel,

A few comments inline. Looking good in general.

> 
> Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> 

No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
as that breaks some existing tooling.

> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
>  
>  What:		/sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index ed384f33e0c7..a0ffbc77e623 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -4,6 +4,17 @@
>  #
>  menu "Temperature sensors"
>  
> +config AS6200
> +       tristate "AS6200 temperature sensor"
> +       depends on I2C
> +       select REGMAP_I2C
> +       help
> +         If you say yes here you get support for AS6200
> +         temperature sensor chip connected via I2C.
> +
> +         This driver can also be built as a module.  If so, the module
> +         will be called as6200.
> +
>  config IQS620AT_TEMP
>  	tristate "Azoteq IQS620AT temperature sensor"
>  	depends on MFD_IQS62X || COMPILE_TEST
> @@ -157,5 +168,4 @@ config MAX31865
>  
>  	  This driver can also be build as a module. If so, the module
>  	  will be called max31865.
> -
Stray change.

>  endmenu

> diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 000000000000..7fcc785871d8
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,493 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for AMS AS6200 Temperature sensor
> + *
> + * Author: Abdel Alkuor <alkuor@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kstrtox.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define AS6200_TVAL_REG		0x0
> +#define AS6200_CONFIG_REG	0x1
> +#define AS6200_TLOW_REG		0x2
> +#define AS6200_THIGH_REG	0x3
> +
> +#define AS6200_CONFIG_AL	BIT(5)
> +#define AS6200_CONFIG_CR	GENMASK(7, 6)
> +#define AS6200_CONFIG_SM	BIT(8)
> +#define AS6200_CONFIG_IM	BIT(9)
> +#define AS6200_CONFIG_POL	BIT(10)
> +#define AS6200_CONFIG_CF	GENMASK(12, 11)
> +
> +#define AS6200_TEMP_MASK	GENMASK(15, 4)
> +
> +struct as6200 {
> +	struct regmap *regmap;
> +	struct mutex lock; /* Prevent concurrent temp fault processing */

Why does it matter? What might cause such processing?

> +};
> +
> +static const int as6200_samp_freq[4][2] = {
> +	{ 0, 250000 },
> +	{ 1, 0 },
> +	{ 4, 0 },
> +	{ 8, 0 }
> +};
> +
> +/* Consective faults converted to period */
> +static const int as6200_temp_thresh_periods[4][4][2] = {
> +	{ { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> +	{ { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
> +	{ { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
> +	{ { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }

I'd suggest naming the first column at least (which is CR I think?)

So define an enum and 
enum {
	AS6200_CR_0_25HZ = 0,
	AS6200_CR_1HZ = 1,
	AS6200_CR_4HZ = 2,
	AS6200_CR_8HZ = 3,
};
And use that for the samp freq entries above, so that they clearly relate
to the rows of this arram
	[AS6200_CR_0_25HZ] = { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
You could take it further and use an enum for CF as well.

	[AS6200_CR_0_25HZ] = {
		[AS6200_CF_1] = { 4, 0 },
		[AS6200_CF_2] = { 8, 0 },
		[AS6200_CF_4] = { 16, 0 },
		[AS6200_CF_6] = { 24, 0 },
	},
	[AS6200_CR_1_HZ] = {
		[AS6200_CF_1] = { 1, 0 },
		[AS6200_CF_2] = { 2, 0 },
	...	
	}
	etc which makes it clear where all the numbers come.

> +};


> +static int as6200_read_event_value(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   enum iio_event_type type,
> +				   enum iio_event_direction dir,
> +				   enum iio_event_info info,
> +				   int *val, int *val2)
> +{
> +	struct as6200 *as = iio_priv(indio_dev);
> +	unsigned int reg;
> +	unsigned int tmp;
> +	int ret;
> +	u8 cf;
> +	u8 cr;
> +
> +	switch (dir) {
> +	case IIO_EV_DIR_FALLING:
> +		reg = AS6200_TLOW_REG;
> +		break;
> +	case IIO_EV_DIR_RISING:
> +		reg = AS6200_THIGH_REG;
> +		break;
> +	case IIO_EV_DIR_EITHER:
> +		reg = AS6200_CONFIG_REG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_read(as->regmap, reg, &tmp);
> +	if (ret)
> +		return ret;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> +		ret = IIO_VAL_INT;
return here.

> +	} else {
> +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> +		*val = as6200_temp_thresh_periods[cr][cf][0];
> +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> +		ret = IIO_VAL_INT_PLUS_MICRO;

and here.  If there is nothing more to be done, it simplifies the code
flow being read to just return as quick as possible.

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

> +static irqreturn_t as6200_event_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct as6200 *as = iio_priv(indio_dev);
> +	unsigned int alert;
> +	enum iio_event_direction dir;
> +	int ret;
> +
> +	guard(mutex)(&as->lock);
What data are we protecting here?

> +
> +	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	alert = FIELD_GET(AS6200_CONFIG_AL, alert);
> +
> +	dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +
> +	iio_push_event(indio_dev,
> +		       IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> +				      dir,
> +				      IIO_EV_TYPE_THRESH,
> +				      0, 0, 0),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> +{
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct as6200 *as = iio_priv(indio_dev);
> +	int ret;
> +	u8 data[16];

I'd make this much more explicit and make sure you zero it to avoid leaking
data.

	struct data {
		u8 channel;
		s64 timestamp __aligned(8);
	};

	memset(&data, 0, sizeof(data)); /* Ensures the holes are zero filled */

Also, avoid the casting mess and read into a local variable that is an unsigned int
and copy it to the struct data if no error.
> +
> +	ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
> +	if (!ret)
Whilst it's more lines, greatly prefer to see error paths out of line and good
paths inline (so no if (!ret))

	if (ret)
		goto done;

	iio_push...

done:
	...

May seem silly but when reviewing a lot of code, keeping things looking "normal"
is a great benefit!

> +		iio_push_to_buffers_with_timestamp(indio_dev, data,
> +						   iio_get_time_ns(indio_dev));
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +
> +static int __maybe_unused as6200_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> +
> +	if (client->irq)
> +		disable_irq(client->irq);
> +
> +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> +				  AS6200_CONFIG_SM, AS6200_CONFIG_SM);
> +}
> +
> +static int __maybe_unused as6200_resume(struct device *dev)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> +
> +	if (client->irq)
> +		enable_irq(client->irq);

I would normally expect suspend and resume to be mirror images. If that doesn't
make sense for some reason and we do need to do the irq handling
before the register write in both cases then add a comment.

> +
> +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> +				  AS6200_CONFIG_SM, 0);
> +}
> +
> +static const struct dev_pm_ops as6200_pm_ops = {

DEFINE_SIMPLE_DEV_PM_OPS()


> +	SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> +};
> +
> +static const struct i2c_device_id as6200_id_table[] = {
> +	{ "as6200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> +
> +static const struct of_device_id as6200_of_match[] = {
> +	{ .compatible = "ams,as6200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, as6200_of_match);
> +
> +static struct i2c_driver as6200_driver = {
> +	.driver = {
> +		.name = "as6200",
> +		.pm = pm_sleep_ptr(&as6200_pm_ops),
> +		.of_match_table = as6200_of_match,
> +	},
> +	.probe = as6200_probe,
> +	.id_table = as6200_id_table,
> +};
> +module_i2c_driver(as6200_driver);
> +
> +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> +MODULE_LICENSE("GPL");

Re: [PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

[Abdel Alkuor]

On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> On Fri,  1 Dec 2023 23:16:51 -0500
> Abdel Alkuor <alkuor@gmail.com> wrote:
> 
> > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > between -40 to 125 Celsius degrees.
> > 
> > The driver implements the alert trigger event in comparator mode where
> > consecutive faults are converted into periods, high/low temperature
> > thresholds require to be above/below the set limit for n seconds for
> > the alert to be triggered/cleared. The alert is only cleared when the
> > current temperature is below the low temperature threshold for n seconds.
> > 
> > The driver supports the following:
> > - show available sampling frequencey
> > - read/write sampling frequency
> > - read raw temperature
> > - read scaling factor
> > - read/write temperature period that needs to be met for low/high
> >   temperature thresholds to trigger an alert
> > - show available temperature period thresholds
> > - buffer trigger
> > - temperature alert event trigger
> 
> Hi Abdel,
> 
> A few comments inline. Looking good in general.
>
Hi Jonathon,

Thank you for your time. I have a couple _silly_ questions about the tag
and returning from if else. Other than that, your comments will be addressed
in v3.
> > 
> > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > 
> 
> No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
> as that breaks some existing tooling.
> 
Understood. 

P.S. when running checkpatch.pl on this patch, I get the following warning:

WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead

should I use Link instead?
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> >  
> >  What:		/sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
> > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> > index ed384f33e0c7..a0ffbc77e623 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -4,6 +4,17 @@
> >  #
> >  menu "Temperature sensors"
> >  
> > +config AS6200
> > +       tristate "AS6200 temperature sensor"
> > +       depends on I2C
> > +       select REGMAP_I2C
> > +       help
> > +         If you say yes here you get support for AS6200
> > +         temperature sensor chip connected via I2C.
> > +
> > +         This driver can also be built as a module.  If so, the module
> > +         will be called as6200.
> > +
> >  config IQS620AT_TEMP
> >  	tristate "Azoteq IQS620AT temperature sensor"
> >  	depends on MFD_IQS62X || COMPILE_TEST
> > @@ -157,5 +168,4 @@ config MAX31865
> >  
> >  	  This driver can also be build as a module. If so, the module
> >  	  will be called max31865.
> > -
> Stray change.
>
Ops, I fixed it locally and forgot to regenerate a patch after. Will be fixed
in v3
> >  endmenu
> 
> > diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> > new file mode 100644
> > index 000000000000..7fcc785871d8
> > --- /dev/null
> > +++ b/drivers/iio/temperature/as6200.c
> > @@ -0,0 +1,493 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for AMS AS6200 Temperature sensor
> > + *
> > + * Author: Abdel Alkuor <alkuor@gmail.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kstrtox.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#define AS6200_TVAL_REG		0x0
> > +#define AS6200_CONFIG_REG	0x1
> > +#define AS6200_TLOW_REG		0x2
> > +#define AS6200_THIGH_REG	0x3
> > +
> > +#define AS6200_CONFIG_AL	BIT(5)
> > +#define AS6200_CONFIG_CR	GENMASK(7, 6)
> > +#define AS6200_CONFIG_SM	BIT(8)
> > +#define AS6200_CONFIG_IM	BIT(9)
> > +#define AS6200_CONFIG_POL	BIT(10)
> > +#define AS6200_CONFIG_CF	GENMASK(12, 11)
> > +
> > +#define AS6200_TEMP_MASK	GENMASK(15, 4)
> > +
> > +struct as6200 {
> > +	struct regmap *regmap;
> > +	struct mutex lock; /* Prevent concurrent temp fault processing */
> 
> Why does it matter? What might cause such processing?
> 
Good point. I had a misunderstanding how the interrupts work and I did
some reading and realized that the interrupt is disabled until the
buttom half of the interrupt is completed as oneshot type is used.
I'll remove it in v3.
> > +};
> > +
> > +static const int as6200_samp_freq[4][2] = {
> > +	{ 0, 250000 },
> > +	{ 1, 0 },
> > +	{ 4, 0 },
> > +	{ 8, 0 }
> > +};
> > +
> > +/* Consective faults converted to period */
> > +static const int as6200_temp_thresh_periods[4][4][2] = {
> > +	{ { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> > +	{ { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
> > +	{ { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
> > +	{ { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }
> 
> I'd suggest naming the first column at least (which is CR I think?)
> 
> So define an enum and 
> enum {
> 	AS6200_CR_0_25HZ = 0,
> 	AS6200_CR_1HZ = 1,
> 	AS6200_CR_4HZ = 2,
> 	AS6200_CR_8HZ = 3,
> };
> And use that for the samp freq entries above, so that they clearly relate
> to the rows of this arram
> 	[AS6200_CR_0_25HZ] = { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> You could take it further and use an enum for CF as well.
> 
> 	[AS6200_CR_0_25HZ] = {
> 		[AS6200_CF_1] = { 4, 0 },
> 		[AS6200_CF_2] = { 8, 0 },
> 		[AS6200_CF_4] = { 16, 0 },
> 		[AS6200_CF_6] = { 24, 0 },
> 	},
> 	[AS6200_CR_1_HZ] = {
> 		[AS6200_CF_1] = { 1, 0 },
> 		[AS6200_CF_2] = { 2, 0 },
> 	...	
> 	}
> 	etc which makes it clear where all the numbers come.
> 
> > +};
>
Good suggestion. I'll fix it in v3.
> 
> > +static int as6200_read_event_value(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   enum iio_event_type type,
> > +				   enum iio_event_direction dir,
> > +				   enum iio_event_info info,
> > +				   int *val, int *val2)
> > +{
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	unsigned int reg;
> > +	unsigned int tmp;
> > +	int ret;
> > +	u8 cf;
> > +	u8 cr;
> > +
> > +	switch (dir) {
> > +	case IIO_EV_DIR_FALLING:
> > +		reg = AS6200_TLOW_REG;
> > +		break;
> > +	case IIO_EV_DIR_RISING:
> > +		reg = AS6200_THIGH_REG;
> > +		break;
> > +	case IIO_EV_DIR_EITHER:
> > +		reg = AS6200_CONFIG_REG;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_read(as->regmap, reg, &tmp);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (info == IIO_EV_INFO_VALUE) {
> > +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > +		ret = IIO_VAL_INT;
> return here.
> 
> > +	} else {
> > +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > +		*val = as6200_temp_thresh_periods[cr][cf][0];
> > +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> > +		ret = IIO_VAL_INT_PLUS_MICRO;
> 
> and here.  If there is nothing more to be done, it simplifies the code
> flow being read to just return as quick as possible.
>
I did it as you mentioned, and when running check_patch.pl, it gives back a
warning that else is not needed here because of the return in the if
statement. So I opted into using ret instead, should I remove the else or ignore
the warning?
> > +	}
> > +
> > +	return ret;
> > +}
> 
> > +static irqreturn_t as6200_event_handler(int irq, void *private)
> > +{
> > +	struct iio_dev *indio_dev = private;
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	unsigned int alert;
> > +	enum iio_event_direction dir;
> > +	int ret;
> > +
> > +	guard(mutex)(&as->lock);
> What data are we protecting here?
> 
No data actually. As I mentioned prior, will be dropped in v3.
> > +
> > +	ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
> > +	if (ret)
> > +		return IRQ_NONE;
> > +
> > +	alert = FIELD_GET(AS6200_CONFIG_AL, alert);
> > +
> > +	dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +
> > +	iio_push_event(indio_dev,
> > +		       IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> > +				      dir,
> > +				      IIO_EV_TYPE_THRESH,
> > +				      0, 0, 0),
> > +		       iio_get_time_ns(indio_dev));
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> > +{
> > +	struct iio_poll_func *pf = private;
> > +	struct iio_dev *indio_dev = pf->indio_dev;
> > +	struct as6200 *as = iio_priv(indio_dev);
> > +	int ret;
> > +	u8 data[16];
> 
> I'd make this much more explicit and make sure you zero it to avoid leaking
> data.
> 
> 	struct data {
> 		u8 channel;
> 		s64 timestamp __aligned(8);
> 	};
> 
> 	memset(&data, 0, sizeof(data)); /* Ensures the holes are zero filled */
>
My system 1 thinking got me here as I tested it and all holes
were set to 0. Will be fixed in v3.
> Also, avoid the casting mess and read into a local variable that is an unsigned int
> and copy it to the struct data if no error.
> > +
> > +	ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
> > +	if (!ret)
> Whilst it's more lines, greatly prefer to see error paths out of line and good
> paths inline (so no if (!ret))
> 
> 	if (ret)
> 		goto done;
> 
> 	iio_push...
> 
> done:
> 	...
> 
> May seem silly but when reviewing a lot of code, keeping things looking "normal"
> is a great benefit!
>
Makes sense. Will be fixed in v3.
> > +		iio_push_to_buffers_with_timestamp(indio_dev, data,
> > +						   iio_get_time_ns(indio_dev));
> > +
> > +	iio_trigger_notify_done(indio_dev->trig);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> ...
> 
> > +
> > +static int __maybe_unused as6200_suspend(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > +	if (client->irq)
> > +		disable_irq(client->irq);
> > +
> > +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > +				  AS6200_CONFIG_SM, AS6200_CONFIG_SM);
> > +}
> > +
> > +static int __maybe_unused as6200_resume(struct device *dev)
> > +{
> > +	struct i2c_client *client = to_i2c_client(dev);
> > +	struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > +	if (client->irq)
> > +		enable_irq(client->irq);
> 
> I would normally expect suspend and resume to be mirror images. If that doesn't
> make sense for some reason and we do need to do the irq handling
> before the register write in both cases then add a comment.
No reason. Will be fixed in v3.
> 
> > +
> > +	return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > +				  AS6200_CONFIG_SM, 0);
> > +}
> > +
> > +static const struct dev_pm_ops as6200_pm_ops = {
> 
> DEFINE_SIMPLE_DEV_PM_OPS()
>
Will be fixed in v3.
> 
> > +	SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> > +};
> > +
> > +static const struct i2c_device_id as6200_id_table[] = {
> > +	{ "as6200" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> > +
> > +static const struct of_device_id as6200_of_match[] = {
> > +	{ .compatible = "ams,as6200" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, as6200_of_match);
> > +
> > +static struct i2c_driver as6200_driver = {
> > +	.driver = {
> > +		.name = "as6200",
> > +		.pm = pm_sleep_ptr(&as6200_pm_ops),
> > +		.of_match_table = as6200_of_match,
> > +	},
> > +	.probe = as6200_probe,
> > +	.id_table = as6200_id_table,
> > +};
> > +module_i2c_driver(as6200_driver);
> > +
> > +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> > +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> > +MODULE_LICENSE("GPL");
> 

Re: [PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

[Jonathan Cameron]

On Mon, 4 Dec 2023 21:16:34 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> > On Fri,  1 Dec 2023 23:16:51 -0500
> > Abdel Alkuor <alkuor@gmail.com> wrote:
> >   
> > > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > > between -40 to 125 Celsius degrees.
> > > 
> > > The driver implements the alert trigger event in comparator mode where
> > > consecutive faults are converted into periods, high/low temperature
> > > thresholds require to be above/below the set limit for n seconds for
> > > the alert to be triggered/cleared. The alert is only cleared when the
> > > current temperature is below the low temperature threshold for n seconds.
> > > 
> > > The driver supports the following:
> > > - show available sampling frequencey
> > > - read/write sampling frequency
> > > - read raw temperature
> > > - read scaling factor
> > > - read/write temperature period that needs to be met for low/high
> > >   temperature thresholds to trigger an alert
> > > - show available temperature period thresholds
> > > - buffer trigger
> > > - temperature alert event trigger  
> > 
> > Hi Abdel,
> > 
> > A few comments inline. Looking good in general.
> >  
> Hi Jonathon,
> 
> Thank you for your time. I have a couple _silly_ questions about the tag
> and returning from if else. Other than that, your comments will be addressed
> in v3.
> > > 
> > > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > >   
> > 
> > No blank line here.  Tags block (and Datasheet is a tag) never has blank lines
> > as that breaks some existing tooling.
> >   
> Understood. 
> 
> P.S. when running checkpatch.pl on this patch, I get the following warning:
> 
> WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead
> 
> should I use Link instead?

Nah. Just ignore checkpatch. :)

Also, if just accepting a comment, no need to say so.  Assumption of reviewer
is that if you keep quiet on a particular comment you will have it fixed in next
version.  Crop out that section of the email and only keep the bits that
you want the reviewer to see your comments on.

Hopefully I've cropped it appropriately!

> > > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > > +	if (info == IIO_EV_INFO_VALUE) {
> > > +		*val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > > +		ret = IIO_VAL_INT;  
> > return here.
> >   
> > > +	} else {
> > > +		cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > > +		cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > > +		*val = as6200_temp_thresh_periods[cr][cf][0];
> > > +		*val2 = as6200_temp_thresh_periods[cr][cf][1];
> > > +		ret = IIO_VAL_INT_PLUS_MICRO;  
> > 
> > and here.  If there is nothing more to be done, it simplifies the code
> > flow being read to just return as quick as possible.
> >  
> I did it as you mentioned, and when running check_patch.pl, it gives back a
> warning that else is not needed here because of the return in the if
> statement. So I opted into using ret instead, should I remove the else or ignore
> the warning?

Dropping the else is fine or take the view it's wrong and ignore it.
Checkpatch is providing hints and suggestions on where to double check.
There is no requirement to accept it's suggestions if you feel the
code ends up less readable.

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

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]

On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:
> as6200 is high accuracy temperature sensor of -/+ 0.4C degree

Is +/- 0.4 degrees really "high accuracy"?

> with a range between -40C to 125C degrees
> 
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
> Changes in v2:
>   - Add vdd-supply
> 
>  .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> new file mode 100644
> index 000000000000..a1817795cdca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS AS6200 Temperature Sensor
> +
> +maintainers:
> +  - Abdel Alkuor <alkuor@gmail.com>
> +
> +description: |

Please add the text from your commit message (although perhaps dropping
the "high accuracy" section) here.

Otherwise, this looks okay to me.

Thanks,
Conor.

> +  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> +
> +properties:
> +  compatible:
> +    const: ams,as6200
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        temperature-sensor@48 {
> +            compatible = "ams,as6200";
> +            reg = <0x48>;
> +            vdd-supply = <&vdd>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
> +        };
> +    };
> +...
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[Abdel Alkuor]

On Sun, Dec 03, 2023 at 11:24:31AM +0000, Conor Dooley wrote:
> On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:
> > as6200 is high accuracy temperature sensor of -/+ 0.4C degree
>
Hi Conor,
> Is +/- 0.4 degrees really "high accuracy"?
>
That's what the datasheet says :D. I'll remove it.
> > with a range between -40C to 125C degrees
> > 
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> > ---
> > Changes in v2:
> >   - Add vdd-supply
> > 
> >  .../bindings/iio/temperature/ams,as6200.yaml  | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > new file mode 100644
> > index 000000000000..a1817795cdca
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/temperature/ams,as6200.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/temperature/ams,as6200.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: AMS AS6200 Temperature Sensor
> > +
> > +maintainers:
> > +  - Abdel Alkuor <alkuor@gmail.com>
> > +
> > +description: |
> 
> Please add the text from your commit message (although perhaps dropping
> the "high accuracy" section) here.
> 
> Otherwise, this looks okay to me.
Sounds good. Will add it in v3.
> 
> Thanks,
> Conor.
> 
Thanks,
Abdel
> > +  https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> > +
> > +properties:
> > +  compatible:
> > +    const: ams,as6200
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        temperature-sensor@48 {
> > +            compatible = "ams,as6200";
> > +            reg = <0x48>;
> > +            vdd-supply = <&vdd>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <17 IRQ_TYPE_EDGE_BOTH>;
> > +        };
> > +    };
> > +...
> > -- 
> > 2.34.1
> > 

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[Jonathan Cameron]

On Mon, 4 Dec 2023 21:19:22 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:

> On Sun, Dec 03, 2023 at 11:24:31AM +0000, Conor Dooley wrote:
> > On Fri, Dec 01, 2023 at 11:16:50PM -0500, Abdel Alkuor wrote:  
> > > as6200 is high accuracy temperature sensor of -/+ 0.4C degree  
> >  
> Hi Conor,
> > Is +/- 0.4 degrees really "high accuracy"?
> >  
> That's what the datasheet says :D. I'll remove it.

I'd not noticed this.  Why is this an IIO driver rather than an hwmon one?

Mostly we do that only for very high accuracy or weird temperature sensors
(infrared ones for example or things with complex thermocouple handling).
Simpler devices meant for hardware monitoring type applications typically go
in hwmon.

We have the iio to hwmon bridge driver for things that naturally have uses
where IIO features are needed, but which get used for hwmon sometimes.
Not sure this is enough IIO focused though, so perhaps list out why you
think it should be in IIO?

Jonathan

Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add AMS AS6200

[Abdel Alkuor]

On Wed, Dec 06, 2023 at 05:40:50PM +0000, Jonathan Cameron wrote:
> On Mon, 4 Dec 2023 21:19:22 -0500
> Abdel Alkuor <alkuor@gmail.com> wrote:
>
Hi Jonathan,
> I'd not noticed this.  Why is this an IIO driver rather than an hwmon one?
> 
> Mostly we do that only for very high accuracy or weird temperature sensors
> (infrared ones for example or things with complex thermocouple handling).
> Simpler devices meant for hardware monitoring type applications typically go
> in hwmon.
> 
> We have the iio to hwmon bridge driver for things that naturally have uses
> where IIO features are needed, but which get used for hwmon sometimes.
> Not sure this is enough IIO focused though, so perhaps list out why you
> think it should be in IIO?
No specific reason. I thought IIO is the de facto for such sensors. I'll
use hwmon instead.

Thanks the clarification.

Abdel