Add support for the MCP9600 thermocouple EMF converter
Subsystems (2)
cover Add support for the MCP9600 thermocouple EMF converter
Add support for the MCP9600 thermocouple EMF converter.
Andrew Hepp (2):
dt-bindings: iio: Add MCP9600 thermocouple EMF converter
iio: temperature: Add MCP9600 thermocouple EMF converter
.../iio/temperature/microchip,mcp9600.yaml | 70 +++++++++ drivers/iio/temperature/Kconfig | 10 ++ drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/mcp9600.c | 145 ++++++++++++++++++ 4 files changed, 226 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml create mode 100644 drivers/iio/temperature/mcp9600.c
Signature · 1 line
2.30.2
PATCH 1/2 dt-bindings: iio: Add MCP9600 thermocouple EMF converter
Add support for the MCP9600 thermocouple EMF converter.
Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
---
Changes for v6:
- none
Changes for v5:
- remove "bindings" from subject
- change unevaluatedProperties to additionalProperties Changes for v4:
- use descriptive names for open/short circuit interrupts
- remove vdd regulator description
- remove unused import
- use generic sensor name in example
- don't use literal style for doc description Changes for v3:
- Added dt-bindings
--- .../iio/temperature/microchip,mcp9600.yaml | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml new file +70 -0
@@ -0,0 +1,70 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/temperature/microchip,mcp9600.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Microchip MCP9600 thermocouple EMF converter + +maintainers: + - Andrew Hepp <[email protected]> + +description: + https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf + +properties: + compatible: + const: microchip,mcp9600 + + reg: + maxItems: 1 + + interrupts: + minItems: 1 + maxItems: 6 + + interrupt-names: + minItems: 1 + maxItems: 6 + items: + enum: + - open-circuit + - short-circuit + - alert1 + - alert2 + - alert3 + - alert4 + + thermocouple-type: + $ref: /schemas/types.yaml#/definitions/uint32 + description: + Type of thermocouple (THERMOCOUPLE_TYPE_K if omitted). + Use defines in dt-bindings/iio/temperature/thermocouple.h. + Supported types are B, E, J, K, N, R, S, T. + + vdd-supply: true + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + #include <dt-bindings/iio/temperature/thermocouple.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + temperature-sensor@60 { + compatible = "microchip,mcp9600"; + reg = <0x60>; + interrupt-parent = <&gpio>; + interrupts = <25 IRQ_TYPE_EDGE_RISING>; + interrupt-names = "open-circuit"; + thermocouple-type = <THERMOCOUPLE_TYPE_K>; + vdd-supply = <&vdd>; + }; + };
Signature · 1 line
2.30.2
Re: [PATCH 1/2] dt-bindings: iio: Add MCP9600 thermocouple EMF converter
Quoted 8 lines · Andrew Hepp
Add support for the MCP9600 thermocouple EMF converter.
Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
Signed-off-by Andrew Hepp <[email protected]>Reviewed-by Krzysztof Kozlowski <[email protected]>---
Changes for v6:
- none
Fix your subject - this is v6, not v1.
You still need to fix build failures for driver...
Best regards,
Krzysztof
PATCH 2/2 iio: temperature: Add MCP9600 thermocouple EMF converter
In reply to Andrew HeppAdd support for the MCP9600 thermocouple EMF converter.
Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
---
Changes for v6:
- read_word_swapped instead of block_data Changes for v5:
- remove "driver" from subject
Changes for v4:
- none
Changes for v3:
- none
Changes for v2:
- remove unused sysfs include
- remove unused scan fields from channel
- warn rather than fail when probing unknown device
- register device through devm
- clean up style and prints
--- drivers/iio/temperature/Kconfig | 10 +++ drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/mcp9600.c | 145 ++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 drivers/iio/temperature/mcp9600.c
drivers/iio/temperature/Kconfig +10 -0
@@ -158,4 +158,14 @@ config MAX31865 This driver can also be build as a module. If so, the module will be called max31865. +config MCP9600 + tristate "MCP9600 thermocouple EMF converter" + depends on I2C + help + If you say yes here you get support for MCP9600 + thermocouple EMF converter connected via I2C. + + This driver can also be built as a module. If so, the module + will be called mcp9600. + endmenu
drivers/iio/temperature/Makefile +1 -0
@@ -10,6 +10,7 @@ obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o obj-$(CONFIG_MAX30208) += max30208.o obj-$(CONFIG_MAX31856) += max31856.o obj-$(CONFIG_MAX31865) += max31865.o +obj-$(CONFIG_MCP9600) += mcp9600.o obj-$(CONFIG_MLX90614) += mlx90614.o obj-$(CONFIG_MLX90632) += mlx90632.o obj-$(CONFIG_TMP006) += tmp006.o
drivers/iio/temperature/mcp9600.c new file +145 -0
145 lines not on this page · raw message
Signature · 1 line
2.30.2
Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter
This looks really good. I have some small comments, and I apologize for only having them so late in the review cycle.
Quoted 1 line · Andrew Hepp
Add support for the MCP9600 thermocouple EMF converter.
Would be nice to have a very short description of the capabilities of the sensor in the commit message.
Quoted 19 lines
Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
Signed-off-by Andrew Hepp <[email protected]>---
[...]drivers/iio/temperature/mcp9600.c new file +7
-0@@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +static const struct iio_chan_spec mcp9600_channels[] = { + { + .type =IIO_TEMP , + .address =MCP9600_HOT_JUNCTION , + .info_mask_separate = +BIT (IIO_CHAN_INFO_RAW ) |BIT (IIO_CHAN_INFO_SCALE ),
drivers/iio/temperature/mcp9600.c line 40
Quoted 10 lines
drivers/iio/temperature/mcp9600.c new file +10
-0+ }, + { + .type = IIO_TEMP, + .address = MCP9600_COLD_JUNCTION, + .channel2 = IIO_MOD_TEMP_AMBIENT, + .modified = 1, + .info_mask_separate = +BIT (IIO_CHAN_INFO_RAW ) |BIT (IIO_CHAN_INFO_SCALE ), + }, + IIO_CHAN_SOFT_TIMESTAMP(2),
If you do not have supported for buffered capture there is no need to include a timestamp in the channel spec. There is no way to read it without buffered support.
drivers/iio/temperature/mcp9600.c line 51
Quoted 11 lines
drivers/iio/temperature/mcp9600.c new file +11
-0+}; + +struct mcp9600_data { +struct i2c_client *client; +struct mutex read_lock;/* lock to prevent concurrent reads */ +}; + +static int mcp9600_read (struct mcp9600_data *data, +struct iio_chan_spec const *chan,int *val) +{ +__be16 buf;
buf does not seem to be used.
drivers/iio/temperature/mcp9600.c line 54
Quoted 3 lines
drivers/iio/temperature/mcp9600.c new file +3
-0+int ret; + +mutex_lock (&data->read_lock);
Do you actually need the custom lock? i2c_smbus_read_word_swapped itself should provide locking and there is only a single operation under your custom lock, which will already be atomic.
Quoted 10 lines
drivers/iio/temperature/mcp9600.c new file +10
-0+ ret =i2c_smbus_read_word_swapped (data->client, chan->address); +mutex_unlock (&data->read_lock); + +if (ret <0 ) +return ret; + *val = ret; + +return 0 ; +} +
drivers/iio/temperature/mcp9600.c line 99
Quoted 10 lines
drivers/iio/temperature/mcp9600.c new file +9
-0+static int mcp9600_probe (struct i2c_client *client) +{ +struct iio_dev *indio_dev; +struct mcp9600_data *data; +int ret; + + ret =i2c_smbus_read_byte_data (client,MCP9600_DEVICE_ID ); +if (ret <0 ) +return ret;
Might as well throw an error message in here for better diagnostics.
return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter
Quoted 1 line · Lars-Peter Clausen
This looks really good. I have some small comments, and I apologize for only having them so late in the review cycle.
No worries at all! I really appreciate the time and effort you, Jonathan, and Krzysztof have put into reviewing this.
Quoted 6 lines · Andrew Hepp
Quoted 1 line
Add support for the MCP9600 thermocouple EMF converter.
Would be nice to have a very short description of the capabilities of the sensor in the commit message.
That seems like a good idea! Should the message be about the capabilities of the sensor, or the capabilities of the driver? The sensor supports a lot of advanced features that the driver currently doesn't support.
Currently I'm leaning towards
"Add support for the MCP9600 thermocouple EMF converter. The sensor has integrated cold junction compensation and a typical accuracy of 0.5 degrees Celsius. The driver supports a resolution of 0.0625 degrees Celsius."
Quoted 22 lines
Quoted 22 lines
Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf
Signed-off-by Andrew Hepp <[email protected]>---
[...]drivers/iio/temperature/mcp9600.c new file +10
-0@@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +static const struct iio_chan_spec mcp9600_channels[] = { + { + .type = IIO_TEMP, + .address = MCP9600_HOT_JUNCTION, + .info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), + }, + { + .type = IIO_TEMP,
Quoted 8 lines
Quoted 7 lines
+ .address =
MCP9600_COLD_JUNCTION,
+ .channel2 =IIO_MOD_TEMP_AMBIENT,
+ .modified = 1,
+ .info_mask_separate =
+BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), + },
+IIO_CHAN_SOFT_TIMESTAMP(2),If you do not have supported for buffered capture there is no need to include a timestamp in the channel spec. There is no way to read it without buffered support.
Ack
Quoted 4 lines
Quoted 4 lines
drivers/iio/temperature/mcp9600.c new file +4
-0+}; + +struct mcp9600_data { + struct i2c_client *client;
Quoted 8 lines
drivers/iio/temperature/mcp9600.c line 51
Quoted 7 lines
+ struct mutex
read_lock; /* lock to prevent concurrent reads */drivers/iio/temperature/mcp9600.c new file +6
-0+}; + +static int mcp9600_read (struct mcp9600_data *data, + struct iio_chan_spec const *chan, int *val) +{ + __be16 buf;buf does not seem to be used.
Oops, sorry about that, I'll make sure to build with warnings as errors next submission. I tested the module after changing from i2c_smbus_read_block_data but looks like I got a bit ahead of myself submitting.
Quoted 3 lines
Quoted 2 lines
+ int ret; >> +
+mutex_lock(&data->read_lock);Do you actually need the custom lock?
i2c_smbus_read_word_swappeditself should provide locking and there is only a single operation under your custom lock, which will already be atomic.
That seems like a convincing argument to me. It certainly doesn't seem like the lock is doing anything, since i2c_smbus_read_word_swapped provides locking.
Quoted 23 lines
Quoted 10 lines
+ ret =
i2c_smbus_read_word_swapped(data->client, chan->address); +mutex_unlock(&data->read_lock);drivers/iio/temperature/mcp9600.c new file +8
-0+ + if (ret < 0) + return ret; + *val = ret; + + return 0; +} +drivers/iio/temperature/mcp9600.c line 99
Quoted 10 lines
drivers/iio/temperature/mcp9600.c new file +9
-0+static int mcp9600_probe (struct i2c_client *client) +{ + struct iio_dev *indio_dev; + struct mcp9600_data *data; + int ret; + + ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID); + if (ret < 0) + return ret;Might as well throw an error message in here for better diagnostics.
Quoted 3 lines
return
dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
I think this is how I did it in my original submission, but it sounds like the preferred way of doing things is to warn without returning an error, in order to support fallback compatibilities?
Jonathan Cameron
maintainerRe: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter
Quoted 20 lines · On Thu, 23 Mar 2023 11:20:55 -0700 Andrew Hepp <[email protected]> wrote:
Quoted 2 lines · Lars-Peter Clausen
This looks really good. I have some small comments, and I apologize for only having them so late in the review cycle.
No worries at all! I really appreciate the time and effort you, Jonathan, and Krzysztof have put into reviewing this.
Quoted 7 lines · Andrew Hepp
Quoted 1 line
Add support for the MCP9600 thermocouple EMF converter.
Would be nice to have a very short description of the capabilities of the sensor in the commit message.
That seems like a good idea! Should the message be about the capabilities of the sensor, or the capabilities of the driver? The sensor supports a lot of advanced features that the driver currently doesn't support.
Quoted 7 lines
Currently I'm leaning towards
"Add support for the MCP9600 thermocouple EMF converter. The sensor has integrated cold junction compensation and a typical accuracy of 0.5 degrees Celsius. The driver supports a resolution of 0.0625 degrees Celsius."
Might be worth calling out what EMF stands for as well. Otherwise that is fine.
One follow up below. I took another look at the driver and other than the points Lars has raised, this looks good to me now.
Thanks,
Jonathan
Quoted 88 lines
Quoted 26 lines
Quoted 26 lines
Datasheet:
https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdfSigned-off-by Andrew Hepp <[email protected]>---
[...]drivers/iio/temperature/mcp9600.c new file +12
-0@@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +static const struct iio_chan_spec mcp9600_channels[] = { + { + .type = IIO_TEMP, + .address = MCP9600_HOT_JUNCTION, + .info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), + }, + { + .type = IIO_TEMP, + .address = MCP9600_COLD_JUNCTION, + .channel2 = IIO_MOD_TEMP_AMBIENT,Quoted 8 lines
Quoted 5 lines
+ .modified = 1,
+ .info_mask_separate =
+BIT(IIO_CHAN_INFO_RAW)|BIT(IIO_CHAN_INFO_SCALE), + },
+IIO_CHAN_SOFT_TIMESTAMP(2),If you do not have supported for buffered capture there is no need to include a timestamp in the channel spec. There is no way to read it without buffered support.
Ack
Quoted 4 lines
Quoted 4 lines
drivers/iio/temperature/mcp9600.c new file +4
-0+}; + +struct mcp9600_data { + struct i2c_client *client;Quoted 8 lines
drivers/iio/temperature/mcp9600.c line 51
Quoted 7 lines
+ struct mutex
read_lock; /* lock to prevent concurrent reads */drivers/iio/temperature/mcp9600.c new file +6
-0+}; + +static int mcp9600_read (struct mcp9600_data *data, + struct iio_chan_spec const *chan, int *val) +{ + __be16 buf;buf does not seem to be used.
Oops, sorry about that, I'll make sure to build with warnings as errors next submission. I tested the module after changing from
i2c_smbus_read_block_databut looks like I got a bit ahead of myself submitting.Quoted 5 lines
Quoted 2 lines
+ int ret; >> +
+mutex_lock(&data->read_lock);Do you actually need the custom lock?
i2c_smbus_read_word_swappeditself should provide locking and there is only a single operation under your custom lock, which will already be atomic.That seems like a convincing argument to me. It certainly doesn't seem like the lock is doing anything, since
i2c_smbus_read_word_swappedprovides locking.Quoted 23 lines
Quoted 10 lines
+ ret =
i2c_smbus_read_word_swapped(data->client, chan->address); +mutex_unlock(&data->read_lock);drivers/iio/temperature/mcp9600.c new file +8
-0+ + if (ret < 0) + return ret; + *val = ret; + + return 0; +} +drivers/iio/temperature/mcp9600.c line 99
Quoted 10 lines
drivers/iio/temperature/mcp9600.c new file +9
-0+static int mcp9600_probe (struct i2c_client *client) +{ + struct iio_dev *indio_dev; + struct mcp9600_data *data; + int ret; + + ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID); + if (ret < 0) + return ret;Might as well throw an error message in here for better diagnostics.
Quoted 7 lines
Quoted 3 lines
return
dev_err_probe(&client->dev, ret, "Failed to read device ID\n");I think this is how I did it in my original submission, but it sounds like the preferred way of doing things is to warn without returning an error, in order to support fallback compatibilities?
A failure to read the DEVICE_ID register at all is worth a print as this is the first time the driver will try to use the bus so if the device isn't there (or isn't responding) it would be good to shout about it.
Jonathan
kernel test robot
build botRe: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter
In reply to Andrew HeppThank you for the patch! Perhaps something to improve:
Hi Andrew,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jic23-iio/togreg]
[also build test WARNING on robh/for-next linus/master v6.3-rc2 next-20230317] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andrew-Hepp/dt-bindings-iio-Add-MCP9600-thermocouple-EMF-converter/20230320-024950 base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg patch link: https://lore.kernel.org/r/20230319184728.49232-3-andrew.hepp%40ahepp.dev patch subject: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20230320/[email protected]/config) compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/dc26dd0d9cb47654a6910bf35d8531b90ae88ece
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andrew-Hepp/dt-bindings-iio-Add-MCP9600-thermocouple-EMF-converter/20230320-024950 git checkout dc26dd0d9cb47654a6910bf35d8531b90ae88ece
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 olddefconfig
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/iio/temperature/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/iio/temperature/mcp9600.c: In function 'mcp9600_read':
>> drivers/iio/temperature/mcp9600.c:51:16: warning: unused variable 'buf' [-Wunused-variable]
51 | __be16 buf;
| ^~~vim +/buf +51 drivers/iio/temperature/mcp9600.c
47
48 static int mcp9600_read(struct mcp9600_data *data,
49 struct iio_chan_spec const *chan, int *val)
50 {
> 51 __be16 buf;
52 int ret;
53
54 mutex_lock(&data->read_lock);
55 ret = i2c_smbus_read_word_swapped(data->client, chan->address);
56 mutex_unlock(&data->read_lock);
57
58 if (ret < 0)
59 return ret;
60 *val = ret;
61
62 return 0;
63 }
64 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
My apologies, I forgot to add "v6" to the subject line.