drivers: sensor: mcp9808: fix various problems and improve test

Correct handling of device encoded temperature values, which combine a
12-bit 2s complement signed value with a separate sign bit.  Rework
conversion between device and sensor temperature representations to
support negative temperatures in both domains.

Use a much simpler trigger configuration where the alert is driven by
comparator output, rather than as an interrupt that requires a pair of
I2C transactions to read and clear the flag.

Refactor the trigger infrastructure to use the setup/handle/process
idiom, which reduces duplicated code and to correctly detect alerts
present when the triggers are set.

Completely replace the sample with something that demonstrates
updating upper and lower threshold values to track moving
temperatures.

Signed-off-by: Peter A. Bigot <[email protected]>

Author: pabigot

drivers/sensor/mcp9808/mcp9808.c

@@ -1,6 +1,5 @@
-/* sensor_mcp9808.c - Driver for MCP9808 temperature sensor */
-
 /*
+ * Copyright (c) 2019 Peter Bigot Consulting, LLC
  * Copyright (c) 2016 Intel Corporation
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -19,9 +18,11 @@
 
 LOG_MODULE_REGISTER(MCP9808, CONFIG_SENSOR_LOG_LEVEL);
 
-int mcp9808_reg_read(struct mcp9808_data *data, u8_t reg, u16_t *val)
+int mcp9808_reg_read(struct device *dev, u8_t reg, u16_t *val)
 {
-	int rc = i2c_write_read(data->i2c_master, data->i2c_slave_addr,
+	const struct mcp9808_data *data = dev->driver_data;
+	const struct mcp9808_config *cfg = dev->config->config_info;
+	int rc = i2c_write_read(data->i2c_master, cfg->i2c_addr,
 				&reg, sizeof(reg),
 				val, sizeof(*val));
 
@@ -38,56 +39,64 @@ static int mcp9808_sample_fetch(struct device *dev, enum sensor_channel chan)
 
 	__ASSERT_NO_MSG(chan == SENSOR_CHAN_ALL || chan == SENSOR_CHAN_AMBIENT_TEMP);
 
-	return mcp9808_reg_read(data, MCP9808_REG_TEMP_AMB, &data->reg_val);
+	return mcp9808_reg_read(dev, MCP9808_REG_TEMP_AMB, &data->reg_val);
 }
 
 static int mcp9808_channel_get(struct device *dev,
 			       enum sensor_channel chan,
 			       struct sensor_value *val)
 {
-	struct mcp9808_data *data = dev->driver_data;
+	const struct mcp9808_data *data = dev->driver_data;
+	int temp = mcp9808_temp_signed_from_reg(data->reg_val);
 
 	__ASSERT_NO_MSG(chan == SENSOR_CHAN_AMBIENT_TEMP);
 
-	val->val1 = (data->reg_val & MCP9808_TEMP_INT_MASK) >>
-		     MCP9808_TEMP_INT_SHIFT;
-	val->val2 = (data->reg_val & MCP9808_TEMP_FRAC_MASK) * 62500U;
-
-	if (data->reg_val & MCP9808_SIGN_BIT) {
-		val->val1 -= 256;
-	}
+	val->val1 = temp / MCP9808_TEMP_SCALE_CEL;
+	temp -= val->val1 * MCP9808_TEMP_SCALE_CEL;
+	val->val2 = (temp * 1000000) / MCP9808_TEMP_SCALE_CEL;
 
 	return 0;
 }
 
 static const struct sensor_driver_api mcp9808_api_funcs = {
 	.sample_fetch = mcp9808_sample_fetch,
 	.channel_get = mcp9808_channel_get,
+#ifdef CONFIG_MCP9808_TRIGGER
 	.attr_set = mcp9808_attr_set,
 	.trigger_set = mcp9808_trigger_set,
+#endif /* CONFIG_MCP9808_TRIGGER */
 };
 
 int mcp9808_init(struct device *dev)
 {
 	struct mcp9808_data *data = dev->driver_data;
+	const struct mcp9808_config *cfg = dev->config->config_info;
+	int rc = 0;
 
-	data->i2c_master =
-	       device_get_binding(DT_INST_0_MICROCHIP_MCP9808_BUS_NAME);
+	data->i2c_master = device_get_binding(cfg->i2c_bus);
 	if (!data->i2c_master) {
-		LOG_DBG("mcp9808: i2c master not found: %s",
-		    DT_INST_0_MICROCHIP_MCP9808_BUS_NAME);
+		LOG_DBG("mcp9808: i2c master not found: %s", cfg->i2c_bus);
 		return -EINVAL;
 	}
 
-	data->i2c_slave_addr = DT_INST_0_MICROCHIP_MCP9808_BASE_ADDRESS;
+#ifdef CONFIG_MCP9808_TRIGGER
+	rc = mcp9808_setup_interrupt(dev);
+#endif /* CONFIG_MCP9808_TRIGGER */
 
-	mcp9808_setup_interrupt(dev);
-
-	return 0;
+	return rc;
 }
 
-struct mcp9808_data mcp9808_data;
+static struct mcp9808_data mcp9808_data;
+static const struct mcp9808_config mcp9808_cfg = {
+	.i2c_bus = DT_INST_0_MICROCHIP_MCP9808_BUS_NAME,
+	.i2c_addr = DT_INST_0_MICROCHIP_MCP9808_BASE_ADDRESS,
+#ifdef CONFIG_MCP9808_TRIGGER
+	.alert_pin = DT_INST_0_MICROCHIP_MCP9808_INT_GPIOS_PIN,
+	.alert_flags = DT_INST_0_MICROCHIP_MCP9808_INT_GPIOS_FLAGS,
+	.alert_controller = DT_INST_0_MICROCHIP_MCP9808_INT_GPIOS_CONTROLLER,
+#endif /* CONFIG_MCP9808_TRIGGER */
+};
 
 DEVICE_AND_API_INIT(mcp9808, DT_INST_0_MICROCHIP_MCP9808_LABEL, mcp9808_init,
-		    &mcp9808_data, NULL, POST_KERNEL,
+		    &mcp9808_data, &mcp9808_cfg, POST_KERNEL,
 		    CONFIG_SENSOR_INIT_PRIORITY, &mcp9808_api_funcs);

drivers/sensor/mcp9808/mcp9808.h

@@ -1,4 +1,5 @@
 /*
+ * Copyright (c) 2019 Peter Bigot Consulting, LLC
  * Copyright (c) 2016 Intel Corporation
  *
  * SPDX-License-Identifier: Apache-2.0
@@ -21,41 +22,74 @@
 #define MCP9808_REG_CRITICAL		0x04
 #define MCP9808_REG_TEMP_AMB		0x05
 
-#define MCP9808_ALERT_INT		BIT(0)
-#define MCP9808_ALERT_CNT		BIT(3)
-#define MCP9808_INT_CLEAR		BIT(5)
-
-#define MCP9808_SIGN_BIT		BIT(12)
-#define MCP9808_TEMP_INT_MASK		0x0ff0
-#define MCP9808_TEMP_INT_SHIFT		4
-#define MCP9808_TEMP_FRAC_MASK		0x000f
-
-#define MCP9808_TEMP_MAX		0xffc
+/* 16 bits control configuration and state.
+ *
+ * * Bit 0 controls alert signal output mode
+ * * Bit 1 controls interrupt polarity
+ * * Bit 2 disables upper and lower threshold checking
+ * * Bit 3 enables alert signal output
+ * * Bit 4 records alert status
+ * * Bit 5 records interrupt status
+ * * Bit 6 locks the upper/lower window registers
+ * * Bit 7 locks the critical register
+ * * Bit 8 enters shutdown mode
+ * * Bits 9-10 control threshold hysteresis
+ */
+#define MCP9808_CFG_ALERT_MODE_INT	BIT(0)
+#define MCP9808_CFG_ALERT_ENA		BIT(3)
+#define MCP9808_CFG_ALERT_STATE		BIT(4)
+#define MCP9808_CFG_INT_CLEAR		BIT(5)
+
+/* 16 bits are used for temperature and state encoding:
+ * * Bits 0..11 encode the temperature in a 2s complement signed value
+ *   in Celsius with 1/16 Cel resolution
+ * * Bit 12 is set to indicate a negative temperature
+ * * Bit 13 is set to indicate a temperature below the lower threshold
+ * * Bit 14 is set to indicate a temperature above the upper threshold
+ * * Bit 15 is set to indicate a temperature above the critical threshold
+ */
+#define MCP9808_TEMP_SCALE_CEL		16 /* signed */
+#define MCP9808_TEMP_SIGN_BIT		BIT(12)
+#define MCP9808_TEMP_ABS_MASK		((u16_t)(MCP9808_TEMP_SIGN_BIT - 1U))
+#define MCP9808_TEMP_LWR_BIT		BIT(13)
+#define MCP9808_TEMP_UPR_BIT		BIT(14)
+#define MCP9808_TEMP_CRT_BIT		BIT(15)
 
 struct mcp9808_data {
 	struct device *i2c_master;
-	u16_t i2c_slave_addr;
 
 	u16_t reg_val;
 
-	struct gpio_callback gpio_cb;
+#ifdef CONFIG_MCP9808_TRIGGER
+	struct device *alert_gpio;
+	struct gpio_callback alert_cb;
+
+	struct device *dev;
+
+	struct sensor_trigger trig;
+	sensor_trigger_handler_t trigger_handler;
+#endif
 
 #ifdef CONFIG_MCP9808_TRIGGER_OWN_THREAD
 	struct k_sem sem;
 #endif
 
 #ifdef CONFIG_MCP9808_TRIGGER_GLOBAL_THREAD
 	struct k_work work;
-	struct device *dev;
 #endif
+};
 
+struct mcp9808_config {
+	const char *i2c_bus;
+	u16_t i2c_addr;
 #ifdef CONFIG_MCP9808_TRIGGER
-	struct sensor_trigger trig;
-	sensor_trigger_handler_t trigger_handler;
-#endif
+	u8_t alert_pin;
+	u8_t alert_flags;
+	const char *alert_controller;
+#endif /* CONFIG_MCP9808_TRIGGER */
 };
 
-int mcp9808_reg_read(struct mcp9808_data *data, u8_t reg, u16_t *val);
+int mcp9808_reg_read(struct device *dev, u8_t reg, u16_t *val);
 
 #ifdef CONFIG_MCP9808_TRIGGER
 int mcp9808_attr_set(struct device *dev, enum sensor_channel chan,
@@ -64,26 +98,37 @@ int mcp9808_attr_set(struct device *dev, enum sensor_channel chan,
 int mcp9808_trigger_set(struct device *dev,
 			const struct sensor_trigger *trig,
 			sensor_trigger_handler_t handler);
-void mcp9808_setup_interrupt(struct device *dev);
-#else
-static inline int mcp9808_attr_set(struct device *dev,
-				   enum sensor_channel chan,
-				   enum sensor_attribute attr,
-				   const struct sensor_value *val)
-{
-	return -ENOTSUP;
-}
+int mcp9808_setup_interrupt(struct device *dev);
+#endif /* CONFIG_MCP9808_TRIGGER */
 
-static inline int mcp9808_trigger_set(struct device *dev,
-				      const struct sensor_trigger *trig,
-				      sensor_trigger_handler_t handler)
+/* Encode a signed temperature in scaled Celsius to the format used in
+ * register values.
+ */
+static inline u16_t mcp9808_temp_reg_from_signed(int temp)
 {
-	return -ENOTSUP;
+	/* Get the 12-bit 2s complement value */
+	u16_t rv = temp & MCP9808_TEMP_ABS_MASK;
+
+	if (temp < 0) {
+		rv |= MCP9808_TEMP_SIGN_BIT;
+	}
+	return rv;
 }
 
-static void mcp9808_setup_interrupt(struct device *dev)
+/* Decode a register temperature value to a signed temperature in
+ * scaled Celsius.
+ */
+static inline int mcp9808_temp_signed_from_reg(u16_t reg)
 {
+	int rv = reg & MCP9808_TEMP_ABS_MASK;
+
+	if (reg & MCP9808_TEMP_SIGN_BIT) {
+		/* Convert 12-bit 2s complement to signed negative
+		 * value.
+		 */
+		rv = -(1U + (rv ^ MCP9808_TEMP_ABS_MASK));
+	}
+	return rv;
 }
-#endif /* CONFIG_MCP9808_TRIGGER */
 
 #endif /* ZEPHYR_DRIVERS_SENSOR_MCP9808_MCP9808_H_ */