| Message ID | 20230319184728.49232-3-andrew.hepp@ahepp.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for the MCP9600 thermocouple EMF converter | expand |
This looks really good. I have some small comments, and I apologize for only having them so late in the review cycle. On 3/19/23 11:47, Andrew Hepp wrote: > 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. > > Datasheet: https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf > Signed-off-by: Andrew Hepp <andrew.hepp@ahepp.dev> > --- > [...] > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > new file mode 100644 > index 000000000000..b6d8ffb90c36 > --- /dev/null > +++ b/drivers/iio/temperature/mcp9600.c > @@ -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, > + .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. > +}; > + > +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. > + 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. > + ret = i2c_smbus_read_word_swapped(data->client, chan->address); > + mutex_unlock(&data->read_lock); > + > + if (ret < 0) > + return ret; > + *val = ret; > + > + return 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");
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/202303200531.buTbR2TA-lkp@intel.com/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 <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303200531.buTbR2TA-lkp@intel.com/
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
On 3/19/23 11:59 AM, Lars-Peter Clausen wrote: > 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. > > On 3/19/23 11:47, Andrew Hepp wrote: >> 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." >> >> Datasheet: >> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf >> Signed-off-by: Andrew Hepp <andrew.hepp@ahepp.dev> >> --- >> [...] >> diff --git a/drivers/iio/temperature/mcp9600.c >> b/drivers/iio/temperature/mcp9600.c >> new file mode 100644 >> index 000000000000..b6d8ffb90c36 >> --- /dev/null >> +++ b/drivers/iio/temperature/mcp9600.c >> @@ -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, >> + .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 >> +}; >> + >> +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. 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. >> + 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. 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. >> + ret = i2c_smbus_read_word_swapped(data->client, chan->address); >> + mutex_unlock(&data->read_lock); >> + >> + if (ret < 0) >> + return ret; >> + *val = ret; >> + >> + return 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"); > > 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?
On Thu, 23 Mar 2023 11:20:55 -0700 Andrew Hepp <andrew.hepp@ahepp.dev> wrote: > On 3/19/23 11:59 AM, Lars-Peter Clausen wrote: > > 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. > > > > > On 3/19/23 11:47, Andrew Hepp wrote: > >> 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." > 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 > >> > >> Datasheet: > >> https://ww1.microchip.com/downloads/en/DeviceDoc/MCP960X-Data-Sheet-20005426.pdf > >> Signed-off-by: Andrew Hepp <andrew.hepp@ahepp.dev> > >> --- > >> [...] > >> diff --git a/drivers/iio/temperature/mcp9600.c > >> b/drivers/iio/temperature/mcp9600.c > >> new file mode 100644 > >> index 000000000000..b6d8ffb90c36 > >> --- /dev/null > >> +++ b/drivers/iio/temperature/mcp9600.c > >> @@ -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, > >> + .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 > > >> +}; > >> + > >> +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. > > 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. > > >> + 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. > > 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. > > >> + ret = i2c_smbus_read_word_swapped(data->client, chan->address); > >> + mutex_unlock(&data->read_lock); > >> + > >> + if (ret < 0) > >> + return ret; > >> + *val = ret; > >> + > >> + return 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"); > > > > > > 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
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig index ed384f33e0c7..ea2ce364b2e9 100644 --- a/drivers/iio/temperature/Kconfig +++ b/drivers/iio/temperature/Kconfig @@ -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 diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile index dfec8c6d3019..9330d4a39598 100644 --- a/drivers/iio/temperature/Makefile +++ b/drivers/iio/temperature/Makefile @@ -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 diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c new file mode 100644 index 000000000000..b6d8ffb90c36 --- /dev/null +++ b/drivers/iio/temperature/mcp9600.c @@ -0,0 +1,145 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * mcp9600.c - Support for Microchip MCP9600 thermocouple EMF converter + * + * Copyright (c) 2022 Andrew Hepp + * Author: <andrew.hepp@ahepp.dev> + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> + +#include <linux/iio/iio.h> + +/* MCP9600 registers */ +#define MCP9600_HOT_JUNCTION 0x0 +#define MCP9600_COLD_JUNCTION 0x2 +#define MCP9600_DEVICE_ID 0x20 + +/* MCP9600 device id value */ +#define MCP9600_DEVICE_ID_MCP9600 0x40 + +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, + .modified = 1, + .info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), + }, + IIO_CHAN_SOFT_TIMESTAMP(2), +}; + +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; + int ret; + + mutex_lock(&data->read_lock); + ret = i2c_smbus_read_word_swapped(data->client, chan->address); + mutex_unlock(&data->read_lock); + + if (ret < 0) + return ret; + *val = ret; + + return 0; +} + +static int mcp9600_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + struct mcp9600_data *data = iio_priv(indio_dev); + int ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = mcp9600_read(data, chan, val); + if (ret) + return ret; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = 62; + *val2 = 500000; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static const struct iio_info mcp9600_info = { + .read_raw = mcp9600_read_raw, +}; + +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; + if (ret != MCP9600_DEVICE_ID_MCP9600) + dev_warn(&client->dev, "Expected ID %x, got %x\n", + MCP9600_DEVICE_ID_MCP9600, ret); + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + data = iio_priv(indio_dev); + data->client = client; + mutex_init(&data->read_lock); + + indio_dev->info = &mcp9600_info; + indio_dev->name = "mcp9600"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = mcp9600_channels; + indio_dev->num_channels = ARRAY_SIZE(mcp9600_channels); + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static const struct i2c_device_id mcp9600_id[] = { + { "mcp9600" }, + {} +}; +MODULE_DEVICE_TABLE(i2c, mcp9600_id); + +static const struct of_device_id mcp9600_of_match[] = { + { .compatible = "microchip,mcp9600" }, + {} +}; +MODULE_DEVICE_TABLE(of, mcp9600_of_match); + +static struct i2c_driver mcp9600_driver = { + .driver = { + .name = "mcp9600", + .of_match_table = mcp9600_of_match, + }, + .probe_new = mcp9600_probe, + .id_table = mcp9600_id +}; +module_i2c_driver(mcp9600_driver); + +MODULE_AUTHOR("Andrew Hepp <andrew.hepp@ahepp.dev>"); +MODULE_DESCRIPTION("Microchip MCP9600 thermocouple EMF converter driver"); +MODULE_LICENSE("GPL");
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 <andrew.hepp@ahepp.dev> --- 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