* [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter
@ 2023-03-19 18:47 Andrew Hepp
2023-03-19 18:47 ` [PATCH 1/2] dt-bindings: iio: Add " Andrew Hepp
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Andrew Hepp @ 2023-03-19 18:47 UTC (permalink / raw)
To: devicetree, linux-iio
Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron, Andrew Hepp
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
--
2.30.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] dt-bindings: iio: Add MCP9600 thermocouple EMF converter 2023-03-19 18:47 [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter Andrew Hepp @ 2023-03-19 18:47 ` Andrew Hepp 2023-03-20 6:45 ` Krzysztof Kozlowski 2023-03-19 18:47 ` [PATCH 2/2] iio: temperature: " Andrew Hepp 2023-03-19 18:52 ` [PATCH 0/2] Add support for the " Andrew Hepp 2 siblings, 1 reply; 9+ messages in thread From: Andrew Hepp @ 2023-03-19 18:47 UTC (permalink / raw) To: devicetree, linux-iio Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron, Andrew Hepp, Krzysztof Kozlowski 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> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- 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 diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml new file mode 100644 index 000000000000..d2cafa38a544 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml @@ -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 <andrew.hepp@ahepp.dev> + +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>; + }; + }; -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: Add MCP9600 thermocouple EMF converter 2023-03-19 18:47 ` [PATCH 1/2] dt-bindings: iio: Add " Andrew Hepp @ 2023-03-20 6:45 ` Krzysztof Kozlowski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2023-03-20 6:45 UTC (permalink / raw) To: Andrew Hepp, devicetree, linux-iio Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron On 19/03/2023 19:47, Andrew Hepp wrote: > 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> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > Changes for v6: > - none Fix your subject - this is v6, not v1. You still need to fix build failures for driver... Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter 2023-03-19 18:47 [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter Andrew Hepp 2023-03-19 18:47 ` [PATCH 1/2] dt-bindings: iio: Add " Andrew Hepp @ 2023-03-19 18:47 ` Andrew Hepp 2023-03-19 18:59 ` Lars-Peter Clausen 2023-03-19 21:24 ` kernel test robot 2023-03-19 18:52 ` [PATCH 0/2] Add support for the " Andrew Hepp 2 siblings, 2 replies; 9+ messages in thread From: Andrew Hepp @ 2023-03-19 18:47 UTC (permalink / raw) To: devicetree, linux-iio Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron, 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 <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 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"); -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter 2023-03-19 18:47 ` [PATCH 2/2] iio: temperature: " Andrew Hepp @ 2023-03-19 18:59 ` Lars-Peter Clausen 2023-03-23 18:20 ` Andrew Hepp 2023-03-19 21:24 ` kernel test robot 1 sibling, 1 reply; 9+ messages in thread From: Lars-Peter Clausen @ 2023-03-19 18:59 UTC (permalink / raw) To: Andrew Hepp, devicetree, linux-iio Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron 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"); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter 2023-03-19 18:59 ` Lars-Peter Clausen @ 2023-03-23 18:20 ` Andrew Hepp 2023-03-26 16:26 ` Jonathan Cameron 0 siblings, 1 reply; 9+ messages in thread From: Andrew Hepp @ 2023-03-23 18:20 UTC (permalink / raw) To: Lars-Peter Clausen, devicetree, linux-iio Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter 2023-03-23 18:20 ` Andrew Hepp @ 2023-03-26 16:26 ` Jonathan Cameron 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2023-03-26 16:26 UTC (permalink / raw) To: Andrew Hepp Cc: Lars-Peter Clausen, devicetree, linux-iio, Rob Herring, Krzysztof Kozlowski 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: temperature: Add MCP9600 thermocouple EMF converter 2023-03-19 18:47 ` [PATCH 2/2] iio: temperature: " Andrew Hepp 2023-03-19 18:59 ` Lars-Peter Clausen @ 2023-03-19 21:24 ` kernel test robot 1 sibling, 0 replies; 9+ messages in thread From: kernel test robot @ 2023-03-19 21:24 UTC (permalink / raw) To: Andrew Hepp, devicetree, linux-iio Cc: oe-kbuild-all, Rob Herring, Krzysztof Kozlowski, Jonathan Cameron, Andrew Hepp 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 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter 2023-03-19 18:47 [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter Andrew Hepp 2023-03-19 18:47 ` [PATCH 1/2] dt-bindings: iio: Add " Andrew Hepp 2023-03-19 18:47 ` [PATCH 2/2] iio: temperature: " Andrew Hepp @ 2023-03-19 18:52 ` Andrew Hepp 2 siblings, 0 replies; 9+ messages in thread From: Andrew Hepp @ 2023-03-19 18:52 UTC (permalink / raw) To: devicetree, linux-iio; +Cc: Rob Herring, Krzysztof Kozlowski, Jonathan Cameron My apologies, I forgot to add "v6" to the subject line. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-03-26 16:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-19 18:47 [PATCH 0/2] Add support for the MCP9600 thermocouple EMF converter Andrew Hepp 2023-03-19 18:47 ` [PATCH 1/2] dt-bindings: iio: Add " Andrew Hepp 2023-03-20 6:45 ` Krzysztof Kozlowski 2023-03-19 18:47 ` [PATCH 2/2] iio: temperature: " Andrew Hepp 2023-03-19 18:59 ` Lars-Peter Clausen 2023-03-23 18:20 ` Andrew Hepp 2023-03-26 16:26 ` Jonathan Cameron 2023-03-19 21:24 ` kernel test robot 2023-03-19 18:52 ` [PATCH 0/2] Add support for the " Andrew Hepp
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).