* [PATCH v1 0/2] iio: temperature: add support for tmp117 @ 2021-03-20 6:45 Puranjay Mohan 2021-03-20 6:45 ` [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-03-20 6:45 ` [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 0 siblings, 2 replies; 9+ messages in thread From: Puranjay Mohan @ 2021-03-20 6:45 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel Cc: Puranjay Mohan Add the dt-bindings and the driver for tmp117 sensor. Changes since v0: 1. Correct Yaml syntax. 2. Change IIO_CHAN_INFO_OFFSET to IIO_CHAN_INFO_CALIBBIAS. 3. Implement IIO_CHAN_INFO_SCALE. 4. Use devm_iio_device_register(). 5. Remove unused headers like delay.h Puranjay Mohan (2): dt-bindings: iio: temperature: Add DT bindings for TMP117 iio: temperature: add driver support for ti tmp117 .../bindings/iio/temperature/ti,tmp117.yaml | 27 +++ drivers/iio/temperature/Kconfig | 10 + drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/tmp117.c | 182 ++++++++++++++++++ 4 files changed, 220 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml create mode 100644 drivers/iio/temperature/tmp117.c -- 2.30.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-03-20 6:45 [PATCH v1 0/2] iio: temperature: add support for tmp117 Puranjay Mohan @ 2021-03-20 6:45 ` Puranjay Mohan 2021-03-20 17:51 ` Jonathan Cameron 2021-03-22 0:15 ` Rob Herring 2021-03-20 6:45 ` [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 1 sibling, 2 replies; 9+ messages in thread From: Puranjay Mohan @ 2021-03-20 6:45 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel Cc: Puranjay Mohan Add devicetree binding document for TMP117, a digital temperature sensor. Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> --- .../bindings/iio/temperature/ti,tmp117.yaml | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml new file mode 100644 index 000000000..927660461 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml @@ -0,0 +1,27 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/iio/temperature/ti,tmp117.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" +description: | + TI TMP117 - Digital temperature sensor with integrated NV memory that supports + I2C interface. + https://www.ti.com/lit/gpn/tmp1 +examples: | + tmp117@48 { + compatible = "ti,tmp117"; + reg = <0x48>; + }; +maintainers: + - "Puranjay Mohan <puranjay12@gmail.com>" +properties: + compatible: + enum: + - "ti,tmp117" + reg: + maxItems: 1 +required: + - compatible + - reg +title: "TI TMP117 - Digital temperature sensor with integrated NV memory" + -- 2.30.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-03-20 6:45 ` [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan @ 2021-03-20 17:51 ` Jonathan Cameron 2021-03-22 0:15 ` Rob Herring 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2021-03-20 17:51 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, devicetree, knaack.h, linux-iio, linux-kernel On Sat, 20 Mar 2021 12:15:08 +0530 Puranjay Mohan <puranjay12@gmail.com> wrote: > Add devicetree binding document for TMP117, a digital temperature sensor. Contents looks fine, but the ordering is unusual. Vast majority of bindings I've seen have document in an order that makes sense for human readers (not alphabetical!) Use whitespace to help readability as well. Something like: > @@ -0,0 +1,27 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/iio/temperature/ti,tmp117.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > +title: "TI TMP117 - Digital temperature sensor with integrated NV memory" > +description: | > + TI TMP117 - Digital temperature sensor with integrated NV memory that supports Avoid repeating info in the title. > + I2C interface. > + https://www.ti.com/lit/gpn/tmp1 > +maintainers: > + - "Puranjay Mohan <puranjay12@gmail.com>" > +properties: > + compatible: > + enum: > + - "ti,tmp117" > + reg: > + maxItems: 1 > +required: > + - compatible > + - reg > +examples: | > + tmp117@48 { > + compatible = "ti,tmp117"; > + reg = <0x48>; > + }; > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > --- > .../bindings/iio/temperature/ti,tmp117.yaml | 27 +++++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > > diff --git a/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > new file mode 100644 > index 000000000..927660461 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > @@ -0,0 +1,27 @@ > +# SPDX-License-Identifier: GPL-2.0 > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/iio/temperature/ti,tmp117.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > +description: | > + TI TMP117 - Digital temperature sensor with integrated NV memory that supports > + I2C interface. > + https://www.ti.com/lit/gpn/tmp1 > +examples: | > + tmp117@48 { > + compatible = "ti,tmp117"; > + reg = <0x48>; > + }; > +maintainers: > + - "Puranjay Mohan <puranjay12@gmail.com>" > +properties: > + compatible: > + enum: > + - "ti,tmp117" > + reg: > + maxItems: 1 > +required: > + - compatible > + - reg > +title: "TI TMP117 - Digital temperature sensor with integrated NV memory" > + ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-03-20 6:45 ` [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-03-20 17:51 ` Jonathan Cameron @ 2021-03-22 0:15 ` Rob Herring 1 sibling, 0 replies; 9+ messages in thread From: Rob Herring @ 2021-03-22 0:15 UTC (permalink / raw) To: Puranjay Mohan Cc: linux-kernel, knaack.h, linux-iio, jic23, alexandru.ardelean, devicetree On Sat, 20 Mar 2021 12:15:08 +0530, Puranjay Mohan wrote: > Add devicetree binding document for TMP117, a digital temperature sensor. > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > --- > .../bindings/iio/temperature/ti,tmp117.yaml | 27 +++++++++++++++++++ > 1 file changed, 27 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml: examples: 'tmp117@48 {\n compatible = "ti,tmp117";\n reg = <0x48>;\n};\n' is not of type 'array' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml: examples: 'tmp117@48 {\n compatible = "ti,tmp117";\n reg = <0x48>;\n};\n' is not of type 'array' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml: 'additionalProperties' is a required property /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml: ignoring, error in schema: examples warning: no schema found in file: ./Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml Error: Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dts:20.5-6 syntax error FATAL ERROR: Unable to parse input tree make[1]: *** [scripts/Makefile.lib:349: Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml] Error 1 make: *** [Makefile:1380: dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1456094 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 2021-03-20 6:45 [PATCH v1 0/2] iio: temperature: add support for tmp117 Puranjay Mohan 2021-03-20 6:45 ` [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan @ 2021-03-20 6:45 ` Puranjay Mohan 2021-03-20 18:15 ` Jonathan Cameron ` (2 more replies) 1 sibling, 3 replies; 9+ messages in thread From: Puranjay Mohan @ 2021-03-20 6:45 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel Cc: Puranjay Mohan TMP117 is a Digital temperature sensor with integrated NV memory. Add support for tmp117 driver in iio subsystem. Datasheet:-https://www.ti.com/lit/gpn/tmp117 Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> --- drivers/iio/temperature/Kconfig | 10 ++ drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/tmp117.c | 182 +++++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+) create mode 100644 drivers/iio/temperature/tmp117.c diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig index f1f2a1499..c5482983f 100644 --- a/drivers/iio/temperature/Kconfig +++ b/drivers/iio/temperature/Kconfig @@ -97,6 +97,16 @@ config TMP007 This driver can also be built as a module. If so, the module will be called tmp007. +config TMP117 + tristate "TMP117 Digital temperature sensor with integrated NV memory" + depends on I2C + help + If you say yes here you get support for the Texas Instruments + TMP117 Digital temperature sensor with integrated NV memory. + + This driver can also be built as a module. If so, the module will + be called tmp117. + config TSYS01 tristate "Measurement Specialties TSYS01 temperature sensor using I2C bus connection" depends on I2C diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile index 90c113115..e3392c4b2 100644 --- a/drivers/iio/temperature/Makefile +++ b/drivers/iio/temperature/Makefile @@ -12,5 +12,6 @@ obj-$(CONFIG_MLX90614) += mlx90614.o obj-$(CONFIG_MLX90632) += mlx90632.o obj-$(CONFIG_TMP006) += tmp006.o obj-$(CONFIG_TMP007) += tmp007.o +obj-$(CONFIG_TMP117) += tmp117.o obj-$(CONFIG_TSYS01) += tsys01.o obj-$(CONFIG_TSYS02D) += tsys02d.o diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c new file mode 100644 index 000000000..194820700 --- /dev/null +++ b/drivers/iio/temperature/tmp117.c @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * tmp117.c - Digital temperature sensor with integrated NV memory + * + * Copyright (c) 2021 Puranjay Mohan <puranjay12@gmail.com> + * + * Driver for the Texas Instruments TMP117 Temperature Sensor + * + * (7-bit I2C slave address (0x48 - 0x4B), changeable via ADD pins) + * + * Note: This driver assumes that the sensor has been calibrated beforehand. + */ + +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/pm.h> +#include <linux/of.h> +#include <linux/irq.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/events.h> + +#define TMP117_REG_TEMP 0x0 +#define TMP117_REG_CFGR 0x1 +#define TMP117_REG_HIGH_LIM 0x2 +#define TMP117_REG_LOW_LIM 0x3 +#define TMP117_REG_EEPROM_UL 0x4 +#define TMP117_REG_EEPROM1 0x5 +#define TMP117_REG_EEPROM2 0x6 +#define TMP117_REG_TEMP_OFFSET 0x7 +#define TMP117_REG_EEPROM3 0x8 +#define TMP117_REG_DEVICE_ID 0xF + +#define TMP117_SCALE 7812500 /* in uCelsius*/ +#define TMP117_RESOLUTION 78125 +#define TMP117_DEVICE_ID 0x0117 + +struct tmp117_data { + struct i2c_client *client; + struct mutex lock; +}; + +static int tmp117_read_reg(struct tmp117_data *data, u8 reg) +{ + return i2c_smbus_read_word_swapped(data->client, reg); +} + +static int tmp117_write_reg(struct tmp117_data *data, u8 reg, int val) +{ + return i2c_smbus_write_word_swapped(data->client, reg, val); +} + +static int tmp117_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int *val, + int *val2, long mask) +{ + struct tmp117_data *data = iio_priv(indio_dev); + u16 tmp, off; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + tmp = tmp117_read_reg(data, TMP117_REG_TEMP); + *val = tmp; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_CALIBBIAS: + off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET); + *val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 10000000; + *val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 10000000; + return IIO_VAL_INT_PLUS_MICRO; + + case IIO_CHAN_INFO_SCALE: + *val = 0; + *val2 = TMP117_SCALE; + return IIO_VAL_INT_PLUS_NANO; + + default: + return -EINVAL; + } +} + +static int tmp117_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *channel, int val, + int val2, long mask) +{ + struct tmp117_data *data = iio_priv(indio_dev); + u16 off; + + switch (mask) { + case IIO_CHAN_INFO_CALIBBIAS: + off = ((val * 10000000) + (val2 * 10)) + / (int32_t)TMP117_RESOLUTION; + return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off); + + default: + return -EINVAL; + } +} + +static const struct iio_chan_spec tmp117_channels[] = { + { + .type = IIO_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), + }, +}; + +static const struct iio_info tmp117_info = { + .read_raw = tmp117_read_raw, + .write_raw = tmp117_write_raw, +}; + +static bool tmp117_identify(struct i2c_client *client) +{ + int dev_id; + + dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); + if (dev_id < 0) + return false; + + return (dev_id == TMP117_DEVICE_ID); +} + +static int tmp117_probe(struct i2c_client *client, + const struct i2c_device_id *tmp117_id) +{ + struct tmp117_data *data; + struct iio_dev *indio_dev; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) + return -EOPNOTSUPP; + + if (!tmp117_identify(client)) { + dev_err(&client->dev, "TMP117 not found\n"); + return -ENODEV; + } + + 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->lock); + + indio_dev->name = "tmp117"; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &tmp117_info; + + indio_dev->channels = tmp117_channels; + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static const struct of_device_id tmp117_of_match[] = { + { .compatible = "ti,tmp117", }, + { }, +}; +MODULE_DEVICE_TABLE(of, tmp117_of_match); + +static const struct i2c_device_id tmp117_id[] = { + { "tmp117", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, tmp117_id); + +static struct i2c_driver tmp117_driver = { + .driver = { + .name = "tmp117", + .of_match_table = of_match_ptr(tmp117_of_match), + }, + .probe = tmp117_probe, + .id_table = tmp117_id, +}; +module_i2c_driver(tmp117_driver); + +MODULE_AUTHOR("Puranjay Mohan <puranjay12@gmail.com>"); +MODULE_DESCRIPTION("TI TMP117 Temperature sensor driver"); +MODULE_LICENSE("GPL"); -- 2.30.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 2021-03-20 6:45 ` [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan @ 2021-03-20 18:15 ` Jonathan Cameron 2021-03-21 5:07 ` Lars-Peter Clausen [not found] ` <CAHp75VdYU1VPGEHWii77wXnwp55aUd8FZ54yhO31fs_qzmTBSQ@mail.gmail.com> 2 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2021-03-20 18:15 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, devicetree, knaack.h, linux-iio, linux-kernel On Sat, 20 Mar 2021 12:15:09 +0530 Puranjay Mohan <puranjay12@gmail.com> wrote: > TMP117 is a Digital temperature sensor with integrated NV memory. > > Add support for tmp117 driver in iio subsystem. > > Datasheet:-https://www.ti.com/lit/gpn/tmp117 > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> Hi Puranjay, Pretty clean driver. A few comments inline. From a process point of view, I'd advise against sending out a new version of a patch set until the previous one has sat on the list for a few days at least. It will save you time as you can handle multiple reviews in one go. I don't mind if you prefer to do a fast turn around though as I'm very good at ignoring emails ;) Jonathan > --- > drivers/iio/temperature/Kconfig | 10 ++ > drivers/iio/temperature/Makefile | 1 + > drivers/iio/temperature/tmp117.c | 182 +++++++++++++++++++++++++++++++ > 3 files changed, 193 insertions(+) > create mode 100644 drivers/iio/temperature/tmp117.c > > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig > index f1f2a1499..c5482983f 100644 > --- a/drivers/iio/temperature/Kconfig > +++ b/drivers/iio/temperature/Kconfig > @@ -97,6 +97,16 @@ config TMP007 > This driver can also be built as a module. If so, the module will > be called tmp007. > > +config TMP117 > + tristate "TMP117 Digital temperature sensor with integrated NV memory" > + depends on I2C > + help > + If you say yes here you get support for the Texas Instruments > + TMP117 Digital temperature sensor with integrated NV memory. > + > + This driver can also be built as a module. If so, the module will > + be called tmp117. > + > config TSYS01 > tristate "Measurement Specialties TSYS01 temperature sensor using I2C bus connection" > depends on I2C > diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile > index 90c113115..e3392c4b2 100644 > --- a/drivers/iio/temperature/Makefile > +++ b/drivers/iio/temperature/Makefile > @@ -12,5 +12,6 @@ obj-$(CONFIG_MLX90614) += mlx90614.o > obj-$(CONFIG_MLX90632) += mlx90632.o > obj-$(CONFIG_TMP006) += tmp006.o > obj-$(CONFIG_TMP007) += tmp007.o > +obj-$(CONFIG_TMP117) += tmp117.o > obj-$(CONFIG_TSYS01) += tsys01.o > obj-$(CONFIG_TSYS02D) += tsys02d.o > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c > new file mode 100644 > index 000000000..194820700 > --- /dev/null > +++ b/drivers/iio/temperature/tmp117.c > @@ -0,0 +1,182 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * tmp117.c - Digital temperature sensor with integrated NV memory > + * > + * Copyright (c) 2021 Puranjay Mohan <puranjay12@gmail.com> > + * > + * Driver for the Texas Instruments TMP117 Temperature Sensor > + * > + * (7-bit I2C slave address (0x48 - 0x4B), changeable via ADD pins) > + * > + * Note: This driver assumes that the sensor has been calibrated beforehand. > + */ > + > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/module.h> > +#include <linux/pm.h> no power management yet. > +#include <linux/of.h> > +#include <linux/irq.h> No interrupts yet. > + > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> I don't think you are using anything from that header. > +#include <linux/iio/events.h> You don't support any events yet. > + > +#define TMP117_REG_TEMP 0x0 > +#define TMP117_REG_CFGR 0x1 > +#define TMP117_REG_HIGH_LIM 0x2 > +#define TMP117_REG_LOW_LIM 0x3 > +#define TMP117_REG_EEPROM_UL 0x4 > +#define TMP117_REG_EEPROM1 0x5 > +#define TMP117_REG_EEPROM2 0x6 > +#define TMP117_REG_TEMP_OFFSET 0x7 > +#define TMP117_REG_EEPROM3 0x8 > +#define TMP117_REG_DEVICE_ID 0xF > + > +#define TMP117_SCALE 7812500 /* in uCelsius*/ Always good to embed the units in the name so it's easy to see what it is at usepoint. I also suspect this is actually in nano Celsius. TMP117_SCALE_UC or MICRO_C > +#define TMP117_RESOLUTION 78125 Define one of scale or resolution in terms of the other though with suggested change to calibbias scaling this may not be used anyway. > +#define TMP117_DEVICE_ID 0x0117 > + > +struct tmp117_data { > + struct i2c_client *client; > + struct mutex lock; Locks always need documentation to explain what their scope is. In this case the lock is never actually locked so writing that doc would have made it fairly obvious it wasn't useful! > +}; > + > +static int tmp117_read_reg(struct tmp117_data *data, u8 reg) > +{ > + return i2c_smbus_read_word_swapped(data->client, reg); These two wrappers don't add anything significant. Just put the i2c calls direct inline. > +} > + > +static int tmp117_write_reg(struct tmp117_data *data, u8 reg, int val) > +{ > + return i2c_smbus_write_word_swapped(data->client, reg, val); > +} > + > +static int tmp117_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > + struct tmp117_data *data = iio_priv(indio_dev); > + u16 tmp, off; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + tmp = tmp117_read_reg(data, TMP117_REG_TEMP); > + *val = tmp; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_CALIBBIAS: > + off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET); > + *val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 10000000; > + *val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 10000000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 0; > + *val2 = TMP117_SCALE; > + return IIO_VAL_INT_PLUS_NANO; > + > + default: > + return -EINVAL; > + } > +} > + > +static int tmp117_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int val, > + int val2, long mask) > +{ > + struct tmp117_data *data = iio_priv(indio_dev); > + u16 off; > + > + switch (mask) { > + case IIO_CHAN_INFO_CALIBBIAS: > + off = ((val * 10000000) + (val2 * 10)) > + / (int32_t)TMP117_RESOLUTION; > + return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off); Calibbias is not normally in an particular defined units. Most devices implementing it apply this value via some DAC effecting the reference voltage or similar. The method to set it is often device specific. Here it's applied in the same units as _RAW which is nice and simple I think it would be more logical to just have have it take an integer value rather than try and map it as a tuning of the output after application of scale. > + > + default: > + return -EINVAL; > + } > +} > + > +static const struct iio_chan_spec tmp117_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), > + }, > +}; > + > +static const struct iio_info tmp117_info = { > + .read_raw = tmp117_read_raw, > + .write_raw = tmp117_write_raw, > +}; > + > +static bool tmp117_identify(struct i2c_client *client) As mentioned below, I'd have this return 0 for a match and negative for other cases as they you can distinguish between the read failing and the value being wrong. > +{ > + int dev_id; > + > + dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > + if (dev_id < 0) > + return false; > + > + return (dev_id == TMP117_DEVICE_ID); > +} > + > +static int tmp117_probe(struct i2c_client *client, > + const struct i2c_device_id *tmp117_id) It's taking many years, but ideally new I2C drivers should use the probe_new callback instead of probe. > +{ > + struct tmp117_data *data; > + struct iio_dev *indio_dev; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) > + return -EOPNOTSUPP; > + > + if (!tmp117_identify(client)) { Probably better to move the error value assignment into the identify function. That will let you distinguish between error on the read and wrong value. > + dev_err(&client->dev, "TMP117 not found\n"); > + return -ENODEV; > + } > + > + 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->lock); > + > + indio_dev->name = "tmp117"; > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &tmp117_info; > + > + indio_dev->channels = tmp117_channels; > + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static const struct of_device_id tmp117_of_match[] = { > + { .compatible = "ti,tmp117", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, tmp117_of_match); > + > +static const struct i2c_device_id tmp117_id[] = { > + { "tmp117", 0 }, > + { } > +}; > +MODULE_DEVICE_TABLE(i2c, tmp117_id); > + > +static struct i2c_driver tmp117_driver = { > + .driver = { > + .name = "tmp117", > + .of_match_table = of_match_ptr(tmp117_of_match), Don't use of_match_ptr. 2 reasons: 1) If you build without CONFIG_OF you'll get a warning because the tmp117_of_match table is unused. 2) It stops use of other firmware probing methods that use the of_match_table The main one of those is ACPI based probing that use the magic PRP0001 ACPI ID. .of_match_table = tmp117_of_match, > + }, > + .probe = tmp117_probe, > + .id_table = tmp117_id, > +}; > +module_i2c_driver(tmp117_driver); > + > +MODULE_AUTHOR("Puranjay Mohan <puranjay12@gmail.com>"); > +MODULE_DESCRIPTION("TI TMP117 Temperature sensor driver"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 2021-03-20 6:45 ` [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 2021-03-20 18:15 ` Jonathan Cameron @ 2021-03-21 5:07 ` Lars-Peter Clausen 2021-03-21 5:23 ` Lars-Peter Clausen [not found] ` <CAHp75VdYU1VPGEHWii77wXnwp55aUd8FZ54yhO31fs_qzmTBSQ@mail.gmail.com> 2 siblings, 1 reply; 9+ messages in thread From: Lars-Peter Clausen @ 2021-03-21 5:07 UTC (permalink / raw) To: Puranjay Mohan, alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel On 3/20/21 7:45 AM, Puranjay Mohan wrote: > TMP117 is a Digital temperature sensor with integrated NV memory. > > Add support for tmp117 driver in iio subsystem. > > Datasheet:-https://www.ti.com/lit/gpn/tmp117 > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> This looks good to me. Just two small bits I overlooked during the first review, sorry for that. > +}; > + > [...] > +static int tmp117_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > + struct tmp117_data *data = iio_priv(indio_dev); > + u16 tmp, off; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + tmp = tmp117_read_reg(data, TMP117_REG_TEMP); > + *val = tmp; No need for tmp here. Just directly assign to val. > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_CALIBBIAS: > + off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET); > + *val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 10000000; > + *val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 10000000; > + return IIO_VAL_INT_PLUS_MICRO; > + > + case IIO_CHAN_INFO_SCALE: > + *val = 0; > + *val2 = TMP117_SCALE; > + return IIO_VAL_INT_PLUS_NANO; > + > + default: > + return -EINVAL; > + } > +} > + > +static int tmp117_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int val, > + int val2, long mask) > +{ > + struct tmp117_data *data = iio_priv(indio_dev); > + u16 off; > + > + switch (mask) { > + case IIO_CHAN_INFO_CALIBBIAS: > + off = ((val * 10000000) + (val2 * 10)) > + / (int32_t)TMP117_RESOLUTION; This needs some input validation. Writing a too large or too small value will cause an overflow/underflow and a bogus value will be written to the register. You can either reject invalid values by returning -EINVAL or clamp them into the right range. Up to you how you want to handle this. > + return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off); > + > + default: > + return -EINVAL; > + } > +} ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 2021-03-21 5:07 ` Lars-Peter Clausen @ 2021-03-21 5:23 ` Lars-Peter Clausen 0 siblings, 0 replies; 9+ messages in thread From: Lars-Peter Clausen @ 2021-03-21 5:23 UTC (permalink / raw) To: Puranjay Mohan, alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel On 3/21/21 6:07 AM, Lars-Peter Clausen wrote: > On 3/20/21 7:45 AM, Puranjay Mohan wrote: >> TMP117 is a Digital temperature sensor with integrated NV memory. >> >> Add support for tmp117 driver in iio subsystem. >> >> Datasheet:-https://www.ti.com/lit/gpn/tmp117 >> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > > This looks good to me. Just two small bits I overlooked during the > first review, sorry for that. > >> +}; >> + >> [...] >> +static int tmp117_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *channel, int *val, >> + int *val2, long mask) >> +{ >> + struct tmp117_data *data = iio_priv(indio_dev); >> + u16 tmp, off; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + tmp = tmp117_read_reg(data, TMP117_REG_TEMP); >> + *val = tmp; > No need for tmp here. Just directly assign to val. Actually thinking about this, does the current implementation work correctly for negative temperatures? I think there are two options to fix it. Either cast to s16 or use the sign_extend32() function. Have a look at how the tmp006 driver handles this. It is a good example, including the error checking. In general you should check if your I2C read failed and return an error in that case rather than a bogus value for the measurement. Same for when reading the calibration offset. Another thing. IIO reports temperature values in milli degrees Celsius. I believe in the current implementation the scale is so that it will report in degrees Celsius instead. ^ permalink raw reply [flat|nested] 9+ messages in thread
[parent not found: <CAHp75VdYU1VPGEHWii77wXnwp55aUd8FZ54yhO31fs_qzmTBSQ@mail.gmail.com>]
* Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 [not found] ` <CAHp75VdYU1VPGEHWii77wXnwp55aUd8FZ54yhO31fs_qzmTBSQ@mail.gmail.com> @ 2021-03-21 11:27 ` Andy Shevchenko 0 siblings, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2021-03-21 11:27 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel (Seems it didn't make mailing list) On Sat, Mar 20, 2021 at 10:55 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Saturday, March 20, 2021, Puranjay Mohan <puranjay12@gmail.com> wrote: >> >> TMP117 is a Digital temperature sensor with integrated NV memory. >> >> Add support for tmp117 driver in iio subsystem. >> >> Datasheet:-https://www.ti.com/lit/gpn/tmp117 >> > > No blank line is needed here > > >> >> Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> >> --- >> drivers/iio/temperature/Kconfig | 10 ++ >> drivers/iio/temperature/Makefile | 1 + >> drivers/iio/temperature/tmp117.c | 182 +++++++++++++++++++++++++++++++ >> 3 files changed, 193 insertions(+) >> create mode 100644 drivers/iio/temperature/tmp117.c >> >> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig >> index f1f2a1499..c5482983f 100644 >> --- a/drivers/iio/temperature/Kconfig >> +++ b/drivers/iio/temperature/Kconfig >> @@ -97,6 +97,16 @@ config TMP007 >> This driver can also be built as a module. If so, the module will >> be called tmp007. >> >> +config TMP117 >> + tristate "TMP117 Digital temperature sensor with integrated NV memory" >> + depends on I2C >> + help >> + If you say yes here you get support for the Texas Instruments >> + TMP117 Digital temperature sensor with integrated NV memory. >> + >> + This driver can also be built as a module. If so, the module will >> + be called tmp117. >> + >> config TSYS01 >> tristate "Measurement Specialties TSYS01 temperature sensor using I2C bus connection" >> depends on I2C >> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile >> index 90c113115..e3392c4b2 100644 >> --- a/drivers/iio/temperature/Makefile >> +++ b/drivers/iio/temperature/Makefile >> @@ -12,5 +12,6 @@ obj-$(CONFIG_MLX90614) += mlx90614.o >> obj-$(CONFIG_MLX90632) += mlx90632.o >> obj-$(CONFIG_TMP006) += tmp006.o >> obj-$(CONFIG_TMP007) += tmp007.o >> +obj-$(CONFIG_TMP117) += tmp117.o >> obj-$(CONFIG_TSYS01) += tsys01.o >> obj-$(CONFIG_TSYS02D) += tsys02d.o >> diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c >> new file mode 100644 >> index 000000000..194820700 >> --- /dev/null >> +++ b/drivers/iio/temperature/tmp117.c >> @@ -0,0 +1,182 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * tmp117.c - Digital temperature sensor with integrated NV memory > > > File name inside the file is redundant, remove it > >> >> + * >> + * Copyright (c) 2021 Puranjay Mohan <puranjay12@gmail.com> >> + * >> + * Driver for the Texas Instruments TMP117 Temperature Sensor >> + * >> + * (7-bit I2C slave address (0x48 - 0x4B), changeable via ADD pins) >> + * >> + * Note: This driver assumes that the sensor has been calibrated beforehand. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/i2c.h> >> +#include <linux/module.h> >> +#include <linux/pm.h> > > >> >> +#include <linux/of.h> > > > You should use mod_devicetable.h rather than of.h. > >> >> +#include <linux/irq.h> >> + >> +#include <linux/iio/iio.h> >> +#include <linux/iio/sysfs.h> >> +#include <linux/iio/events.h> > > > Can you consider to use constants and /or formulas from units.h? > >> >> +#define TMP117_REG_TEMP 0x0 >> +#define TMP117_REG_CFGR 0x1 >> +#define TMP117_REG_HIGH_LIM 0x2 >> +#define TMP117_REG_LOW_LIM 0x3 >> +#define TMP117_REG_EEPROM_UL 0x4 >> +#define TMP117_REG_EEPROM1 0x5 >> +#define TMP117_REG_EEPROM2 0x6 >> +#define TMP117_REG_TEMP_OFFSET 0x7 >> +#define TMP117_REG_EEPROM3 0x8 >> +#define TMP117_REG_DEVICE_ID 0xF >> + >> +#define TMP117_SCALE 7812500 /* in uCelsius*/ >> +#define TMP117_RESOLUTION 78125 >> +#define TMP117_DEVICE_ID 0x0117 >> + >> +struct tmp117_data { >> + struct i2c_client *client; >> + struct mutex lock; >> +}; >> + >> +static int tmp117_read_reg(struct tmp117_data *data, u8 reg) >> +{ >> + return i2c_smbus_read_word_swapped(data->client, reg); >> +} >> + >> +static int tmp117_write_reg(struct tmp117_data *data, u8 reg, int val) >> +{ >> + return i2c_smbus_write_word_swapped(data->client, reg, val); >> +} >> + >> +static int tmp117_read_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *channel, int *val, >> + int *val2, long mask) >> +{ >> + struct tmp117_data *data = iio_priv(indio_dev); >> + u16 tmp, off; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_RAW: >> + tmp = tmp117_read_reg(data, TMP117_REG_TEMP); >> + *val = tmp; >> + return IIO_VAL_INT; >> + >> + case IIO_CHAN_INFO_CALIBBIAS: >> + off = tmp117_read_reg(data, TMP117_REG_TEMP_OFFSET); >> + *val = ((int16_t)off * (int32_t)TMP117_RESOLUTION) / 10000000; >> + *val2 = ((int16_t)off * (int32_t)TMP117_RESOLUTION) % 10000000; > > > Often when you do explicit casting it means something was not well thought through. Consider to justify why you do explicit casting in each case and drop where it’s not necessarily needed. > >> >> + return IIO_VAL_INT_PLUS_MICRO; >> + >> + case IIO_CHAN_INFO_SCALE: >> + *val = 0; >> + *val2 = TMP117_SCALE; >> + return IIO_VAL_INT_PLUS_NANO; >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static int tmp117_write_raw(struct iio_dev *indio_dev, >> + struct iio_chan_spec const *channel, int val, >> + int val2, long mask) >> +{ >> + struct tmp117_data *data = iio_priv(indio_dev); >> + u16 off; >> + >> + switch (mask) { >> + case IIO_CHAN_INFO_CALIBBIAS: >> + off = ((val * 10000000) + (val2 * 10)) >> + / (int32_t)TMP117_RESOLUTION; >> + return tmp117_write_reg(data, TMP117_REG_TEMP_OFFSET, off); >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static const struct iio_chan_spec tmp117_channels[] = { >> + { >> + .type = IIO_TEMP, >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | >> + BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), >> + }, >> +}; >> + >> +static const struct iio_info tmp117_info = { >> + .read_raw = tmp117_read_raw, >> + .write_raw = tmp117_write_raw, >> +}; >> + >> +static bool tmp117_identify(struct i2c_client *client) >> +{ >> + int dev_id; >> + >> + dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); >> + if (dev_id < 0) >> + return false; >> + >> + return (dev_id == TMP117_DEVICE_ID); >> +} >> + >> +static int tmp117_probe(struct i2c_client *client, >> + const struct i2c_device_id *tmp117_id) >> +{ >> + struct tmp117_data *data; >> + struct iio_dev *indio_dev; >> + >> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) >> + return -EOPNOTSUPP; >> + >> + if (!tmp117_identify(client)) { >> + dev_err(&client->dev, "TMP117 not found\n"); >> + return -ENODEV; >> + } >> + >> + 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->lock); >> + >> + indio_dev->name = "tmp117"; >> + indio_dev->modes = INDIO_DIRECT_MODE; >> + indio_dev->info = &tmp117_info; >> + >> + indio_dev->channels = tmp117_channels; >> + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); >> + >> + return devm_iio_device_register(&client->dev, indio_dev); >> +} >> + >> +static const struct of_device_id tmp117_of_match[] = { >> + { .compatible = "ti,tmp117", }, > > >> >> + { }, > > > No comma for terminator line > >> >> +}; >> +MODULE_DEVICE_TABLE(of, tmp117_of_match); >> + >> +static const struct i2c_device_id tmp117_id[] = { >> + { "tmp117", 0 }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(i2c, tmp117_id); >> + >> +static struct i2c_driver tmp117_driver = { >> + .driver = { >> + .name = "tmp117", >> + .of_match_table = of_match_ptr(tmp117_of_match), >> + }, >> + .probe = tmp117_probe, >> + .id_table = tmp117_id, >> +}; >> +module_i2c_driver(tmp117_driver); >> + >> +MODULE_AUTHOR("Puranjay Mohan <puranjay12@gmail.com>"); >> +MODULE_DESCRIPTION("TI TMP117 Temperature sensor driver"); >> +MODULE_LICENSE("GPL"); >> -- >> 2.30.1 >> > > > -- > With Best Regards, > Andy Shevchenko > > -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-03-22 0:16 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-20 6:45 [PATCH v1 0/2] iio: temperature: add support for tmp117 Puranjay Mohan
2021-03-20 6:45 ` [PATCH v1 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan
2021-03-20 17:51 ` Jonathan Cameron
2021-03-22 0:15 ` Rob Herring
2021-03-20 6:45 ` [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan
2021-03-20 18:15 ` Jonathan Cameron
2021-03-21 5:07 ` Lars-Peter Clausen
2021-03-21 5:23 ` Lars-Peter Clausen
[not found] ` <CAHp75VdYU1VPGEHWii77wXnwp55aUd8FZ54yhO31fs_qzmTBSQ@mail.gmail.com>
2021-03-21 11:27 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.