* [PATCH v2 0/2] iio: temperature: add support for tmp117 @ 2021-04-01 9:16 Puranjay Mohan 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-04-01 9:16 ` [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 0 siblings, 2 replies; 12+ messages in thread From: Puranjay Mohan @ 2021-04-01 9:16 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel, lars Cc: Puranjay Mohan Add the dt-bindings and the driver for tmp117 sensor. Changes since v1: 1. Remove unused headers 2. Add error checking in i2c read/write. 3. Correct DT bindings. 4. Correct implementation to return tmp in milli celcius. 5. Remove unused mutex lock. 6. Modify MAINTAINERS. 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 | 34 ++++ MAINTAINERS | 7 + drivers/iio/temperature/Kconfig | 10 + drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/tmp117.c | 179 ++++++++++++++++++ 5 files changed, 231 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] 12+ messages in thread
* [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-04-01 9:16 [PATCH v2 0/2] iio: temperature: add support for tmp117 Puranjay Mohan @ 2021-04-01 9:16 ` Puranjay Mohan 2021-04-01 14:56 ` Rob Herring ` (2 more replies) 2021-04-01 9:16 ` [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 1 sibling, 3 replies; 12+ messages in thread From: Puranjay Mohan @ 2021-04-01 9:16 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel, lars 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 | 34 +++++++++++++++++++ 1 file changed, 34 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..1ead22317 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml @@ -0,0 +1,34 @@ +# 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 + 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 + +additionalProperties: false + +examples: + - | + tmp117@48 { + compatible = "ti,tmp117"; + reg = <0x48>; + }; -- 2.30.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan @ 2021-04-01 14:56 ` Rob Herring 2021-04-01 15:13 ` Jonathan Cameron 2021-04-01 16:09 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2021-04-01 14:56 UTC (permalink / raw) To: Puranjay Mohan Cc: linux-kernel, alexandru.ardelean, knaack.h, devicetree, lars, jic23, linux-iio On Thu, 01 Apr 2021 14:46:47 +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 | 34 +++++++++++++++++++ > 1 file changed, 34 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: Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dts:21.13-26: Warning (reg_format): /example-0/tmp117@48:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1) Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: Warning (pci_device_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: Warning (simple_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format' Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format' /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.example.dt.yaml: example-0: tmp117@48:reg:0: [72] is too short From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml See https://patchwork.ozlabs.org/patch/1460920 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] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-04-01 14:56 ` Rob Herring @ 2021-04-01 15:13 ` Jonathan Cameron 2021-04-01 16:09 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: Jonathan Cameron @ 2021-04-01 15:13 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, devicetree, knaack.h, linux-iio, linux-kernel, lars On Thu, 1 Apr 2021 14:46:47 +0530 Puranjay Mohan <puranjay12@gmail.com> 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 | 34 +++++++++++++++++++ > 1 file changed, 34 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..1ead22317 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > @@ -0,0 +1,34 @@ > +# 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 > + 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 > + > +additionalProperties: false > + > +examples: > + - | > + tmp117@48 { > + compatible = "ti,tmp117"; > + reg = <0x48>; > + }; For the example embed it in an appropriate i2c description e.g. i2c { #address-cells = <1>; #size-cells = <0>; tmp117@48 { compatible = "ti,tmp117"; reg = <0x48>; }; }; ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-04-01 14:56 ` Rob Herring 2021-04-01 15:13 ` Jonathan Cameron @ 2021-04-01 16:09 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2021-04-01 16:09 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel, lars On Thu, Apr 01, 2021 at 02:46:47PM +0530, Puranjay Mohan wrote: > Add devicetree binding document for TMP117, a digital temperature sensor. Please run checkpatch.pl and fix trailing WS. > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> > --- > .../bindings/iio/temperature/ti,tmp117.yaml | 34 +++++++++++++++++++ > 1 file changed, 34 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..1ead22317 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml > @@ -0,0 +1,34 @@ > +# 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 > + I2C interface. > + https://www.ti.com/lit/gpn/tmp1 > + > +maintainers: > + - "Puranjay Mohan <puranjay12@gmail.com>" Don't need quotes. > + > +properties: > + compatible: > + enum: > + - "ti,tmp117" Don't need quotes. Blank line here. > + reg: > + maxItems: 1 blank line here. > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + tmp117@48 { > + compatible = "ti,tmp117"; > + reg = <0x48>; You could just add to trivial-devices.yaml. > + }; > -- > 2.30.1 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-01 9:16 [PATCH v2 0/2] iio: temperature: add support for tmp117 Puranjay Mohan 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan @ 2021-04-01 9:16 ` Puranjay Mohan 2021-04-01 9:36 ` Andy Shevchenko 2021-04-02 8:13 ` Lars-Peter Clausen 1 sibling, 2 replies; 12+ messages in thread From: Puranjay Mohan @ 2021-04-01 9:16 UTC (permalink / raw) To: alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel, lars 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> --- MAINTAINERS | 7 ++ drivers/iio/temperature/Kconfig | 10 ++ drivers/iio/temperature/Makefile | 1 + drivers/iio/temperature/tmp117.c | 179 +++++++++++++++++++++++++++++++ 4 files changed, 197 insertions(+) create mode 100644 drivers/iio/temperature/tmp117.c diff --git a/MAINTAINERS b/MAINTAINERS index 60ed2963e..c9b806b63 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -16666,6 +16666,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h F: include/linux/soc/ti/ti_sci_inta_msi.h F: include/linux/soc/ti/ti_sci_protocol.h +TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER +M: Puranjay Mohan <puranjay12@gmail.com> +L: linux-iio@vger.kernel.org +S: Supported +F: Documentation/devicetree/bindings/iio/temperature/ti,tmp117.yaml +F: drivers/iio/temperature/tmp117.c + THANKO'S RAREMONO AM/FM/SW RADIO RECEIVER USB DRIVER M: Hans Verkuil <hverkuil@xs4all.nl> L: linux-media@vger.kernel.org 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..83c0b856f --- /dev/null +++ b/drivers/iio/temperature/tmp117.c @@ -0,0 +1,179 @@ +// 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/iio/iio.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_RESOLUTION_10UC 78125 +#define TMP117_DEVICE_ID 0x0117 + +struct tmp117_data { + struct i2c_client *client; +}; + +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); + s32 ret; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + ret = i2c_smbus_read_word_swapped(data->client, + TMP117_REG_TEMP); + if (ret < 0) + return ret; + *val = sign_extend32(ret, 15); + return IIO_VAL_INT; + + case IIO_CHAN_INFO_CALIBBIAS: + ret = i2c_smbus_read_word_swapped(data->client, + TMP117_REG_TEMP_OFFSET); + if (ret < 0) + return ret; + *val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) + / 10000; + *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) + % 10000; + return IIO_VAL_INT_PLUS_MICRO; + + case IIO_CHAN_INFO_SCALE: + /* Conversion from 10s of uC to mC + * as IIO reports temperature in mC + */ + *val = TMP117_RESOLUTION_10UC / 10000; + *val2 = (TMP117_RESOLUTION_10UC % 10000) * 100; + return IIO_VAL_INT_PLUS_MICRO; + + 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); + s16 off; + + switch (mask) { + case IIO_CHAN_INFO_CALIBBIAS: + off = (s16)val; + return i2c_smbus_write_word_swapped(data->client, + 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 int 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 dev_id; + if (dev_id != TMP117_DEVICE_ID) { + dev_err(&client->dev, "TMP117 not found\n"); + return -ENODEV; + } + return 0; +} + +static int tmp117_probe(struct i2c_client *client, + const struct i2c_device_id *tmp117_id) +{ + struct tmp117_data *data; + struct iio_dev *indio_dev; + int ret; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) + return -EOPNOTSUPP; + + ret = tmp117_identify(client); + if (ret < 0) + return 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->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 = tmp117_of_match, + }, + .probe_new = 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] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-01 9:16 ` [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan @ 2021-04-01 9:36 ` Andy Shevchenko 2021-04-02 8:14 ` Lars-Peter Clausen 2021-04-02 8:13 ` Lars-Peter Clausen 1 sibling, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2021-04-01 9:36 UTC (permalink / raw) To: Puranjay Mohan Cc: Alexandru Ardelean, Jonathan Cameron, devicetree, Hartmut Knaack, linux-iio, Linux Kernel Mailing List, Lars-Peter Clausen On Thu, Apr 1, 2021 at 12:19 PM Puranjay Mohan <puranjay12@gmail.com> wrote: > > TMP117 is a Digital temperature sensor with integrated NV memory. > > Add support for tmp117 driver in iio subsystem. + blank line > Datasheet:-https://www.ti.com/lit/gpn/tmp117 Make it a tag, i.e. remove the following blank line and use a space after colon. > > Signed-off-by: Puranjay Mohan <puranjay12@gmail.com> ... > +/* > + * tmp117.c - Digital temperature sensor with integrated NV memory It's useless and provokes an unneeded churn when having a file name inside the file. Please, drop it for good. > + * > + * Copyright (c) 2021 Puranjay Mohan <puranjay12@gmail.com> > + * > + * Driver for the Texas Instruments TMP117 Temperature Sensor > + * Redundant blank line. > + * (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> Missed: bitops.h //sign_extend32() types.h // s32 > + > +#include <linux/iio/iio.h> ... > +struct tmp117_data { > + struct i2c_client *client; > +}; Doesn't make any sense to have a separate structure for just one pointer member. Use that pointer directly. ... > + case IIO_CHAN_INFO_CALIBBIAS: > + ret = i2c_smbus_read_word_swapped(data->client, > + TMP117_REG_TEMP_OFFSET); > + if (ret < 0) > + return ret; > + *val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > + / 10000; One line > + *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > + % 10000; One line. I'll be honest, I do not like these explicit castings at all. Can you revisit and try to refactor that you won't need them? For example, I can't understand how ret can be higher than 16 bit since we checked on negative values beforehand. > + return IIO_VAL_INT_PLUS_MICRO; > + > + case IIO_CHAN_INFO_SCALE: > + /* Conversion from 10s of uC to mC > + * as IIO reports temperature in mC > + */ > + *val = TMP117_RESOLUTION_10UC / 10000; > + *val2 = (TMP117_RESOLUTION_10UC % 10000) * 100; > + return IIO_VAL_INT_PLUS_MICRO; You use 10000 many times, can you give it an appropriate name (via #define)? ... > + s16 off; > + case IIO_CHAN_INFO_CALIBBIAS: > + off = (s16)val; Redundant explicit casting. > + return i2c_smbus_write_word_swapped(data->client, > + TMP117_REG_TEMP_OFFSET, off); ... > +static const struct of_device_id tmp117_of_match[] = { > + { .compatible = "ti,tmp117", }, > + { }, No need to comma in terminator line(s). > +}; -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-01 9:36 ` Andy Shevchenko @ 2021-04-02 8:14 ` Lars-Peter Clausen 2021-04-03 13:52 ` Andy Shevchenko 0 siblings, 1 reply; 12+ messages in thread From: Lars-Peter Clausen @ 2021-04-02 8:14 UTC (permalink / raw) To: Andy Shevchenko, Puranjay Mohan Cc: Alexandru Ardelean, Jonathan Cameron, devicetree, Hartmut Knaack, linux-iio, Linux Kernel Mailing List On 4/1/21 11:36 AM, Andy Shevchenko wrote: > [...] >> + case IIO_CHAN_INFO_SCALE: >> + /* Conversion from 10s of uC to mC >> + * as IIO reports temperature in mC >> + */ >> + *val = TMP117_RESOLUTION_10UC / 10000; >> + *val2 = (TMP117_RESOLUTION_10UC % 10000) * 100; >> + return IIO_VAL_INT_PLUS_MICRO; > You use 10000 many times, can you give it an appropriate name (via #define)? #define TENTHOUSAND 10000 ;) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-02 8:14 ` Lars-Peter Clausen @ 2021-04-03 13:52 ` Andy Shevchenko 0 siblings, 0 replies; 12+ messages in thread From: Andy Shevchenko @ 2021-04-03 13:52 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Puranjay Mohan, Alexandru Ardelean, Jonathan Cameron, devicetree, Hartmut Knaack, linux-iio, Linux Kernel Mailing List On Fri, Apr 2, 2021 at 11:14 AM Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 4/1/21 11:36 AM, Andy Shevchenko wrote: > > [...] > >> + case IIO_CHAN_INFO_SCALE: > >> + /* Conversion from 10s of uC to mC > >> + * as IIO reports temperature in mC > >> + */ > >> + *val = TMP117_RESOLUTION_10UC / 10000; > >> + *val2 = (TMP117_RESOLUTION_10UC % 10000) * 100; > >> + return IIO_VAL_INT_PLUS_MICRO; > > You use 10000 many times, can you give it an appropriate name (via #define)? > #define TENTHOUSAND 10000 ;) Actually, besides April fool's day, this can have a value if 1000 is defined as mC _PER_uC or so. Oh, wait. We have it already. #define MILLIDEGREE_PER_DEGREE 1000 -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-01 9:16 ` [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 2021-04-01 9:36 ` Andy Shevchenko @ 2021-04-02 8:13 ` Lars-Peter Clausen 2021-04-03 14:58 ` Puranjay Mohan 1 sibling, 1 reply; 12+ messages in thread From: Lars-Peter Clausen @ 2021-04-02 8:13 UTC (permalink / raw) To: Puranjay Mohan, alexandru.ardelean, jic23, devicetree, knaack.h, linux-iio, linux-kernel On 4/1/21 11:16 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> Nice and clean driver. Just some comments about the CALIBBIAS. > [...] > +#define TMP117_RESOLUTION_10UC 78125 Isn't the unit here 100 uC? > +#define TMP117_DEVICE_ID 0x0117 > + > +struct tmp117_data { > + struct i2c_client *client; > +}; > + > +static int tmp117_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *channel, int *val, > + int *val2, long mask) > +{ > [...] > + case IIO_CHAN_INFO_CALIBBIAS: > + ret = i2c_smbus_read_word_swapped(data->client, > + TMP117_REG_TEMP_OFFSET); > + if (ret < 0) > + return ret; > + *val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > + / 10000; > + *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > + % 10000; If I understand this right CALBBIAS is written in one unit, but read in another unit. E.g. if you do `echo 100 > ..._calibbias` and then `cat ..._calibbias` you'd read a different value back. I think that would be quite unexpected behavior. The unit should be the same. Here in the read function you can just return the register value. Just make sure to properly sign extend like for the RAW property. > + return IIO_VAL_INT_PLUS_MICRO; > [...] > +} > + > +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); > + s16 off; > + > + switch (mask) { > + case IIO_CHAN_INFO_CALIBBIAS: > + off = (s16)val; This should have some input validation to avoid writing bogus values to the register when the value is out of range. You can either reject out of range values or clamp them into the valid range (using the clamp() macro). > + return i2c_smbus_write_word_swapped(data->client, > + TMP117_REG_TEMP_OFFSET, off); > + > + default: > + return -EINVAL; > + } > +} > + [...] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-02 8:13 ` Lars-Peter Clausen @ 2021-04-03 14:58 ` Puranjay Mohan 2021-04-03 15:03 ` Lars-Peter Clausen 0 siblings, 1 reply; 12+ messages in thread From: Puranjay Mohan @ 2021-04-03 14:58 UTC (permalink / raw) To: Lars-Peter Clausen Cc: alexandru.ardelean, Jonathan Cameron, devicetree, knaack.h, linux-iio, Linux Kernel Mailing List On Fri, Apr 2, 2021 at 1:43 PM Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 4/1/21 11:16 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> > > Nice and clean driver. Just some comments about the CALIBBIAS. > > > [...] > > +#define TMP117_RESOLUTION_10UC 78125 > Isn't the unit here 100 uC? it is 7.8125 milli degrees_C so 78125 x 10^-4 milli degrees_C which is 78125 x 10^-4 x 10^3 micro degrees_C so it becomes 78125 x 10^-1 micro degrees_C = 78125 10_microdegrees_C. Did it in detail so I remember it in the future. I guess you thought it as 0.78125 millidegrees_C? > > +#define TMP117_DEVICE_ID 0x0117 > > + > > +struct tmp117_data { > > + struct i2c_client *client; > > +}; > > + > > +static int tmp117_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *channel, int *val, > > + int *val2, long mask) > > +{ > > [...] > > + case IIO_CHAN_INFO_CALIBBIAS: > > + ret = i2c_smbus_read_word_swapped(data->client, > > + TMP117_REG_TEMP_OFFSET); > > + if (ret < 0) > > + return ret; > > + *val = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > > + / 10000; > > + *val2 = ((int16_t)ret * (int32_t)TMP117_RESOLUTION_10UC) > > + % 10000; > > If I understand this right CALBBIAS is written in one unit, but read in > another unit. E.g. if you do `echo 100 > ..._calibbias` and then `cat > ..._calibbias` you'd read a different value back. > yes, reading it will return the value in milli degrees_C. but to write it should be in the register format according to the datasheet. > I think that would be quite unexpected behavior. The unit should be the > same. Here in the read function you can just return the register value. Okay, if you feel that would be right then I will do it. > Just make sure to properly sign extend like for the RAW property. > > > + return IIO_VAL_INT_PLUS_MICRO; > > [...] > > +} > > + > > +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); > > + s16 off; > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_CALIBBIAS: > > + off = (s16)val; > This should have some input validation to avoid writing bogus values to > the register when the value is out of range. You can either reject out > of range values or clamp them into the valid range (using the clamp() > macro). the maximum value which this register takes is 0xffff, so it should get clamped automatically when casting to s16? I might be wrong here. > > + return i2c_smbus_write_word_swapped(data->client, > > + TMP117_REG_TEMP_OFFSET, off); > > + > > + default: > > + return -EINVAL; > > + } > > +} > > + > [...] -- Thanks and Regards Yours Truly, Puranjay Mohan ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 2021-04-03 14:58 ` Puranjay Mohan @ 2021-04-03 15:03 ` Lars-Peter Clausen 0 siblings, 0 replies; 12+ messages in thread From: Lars-Peter Clausen @ 2021-04-03 15:03 UTC (permalink / raw) To: Puranjay Mohan Cc: alexandru.ardelean, Jonathan Cameron, devicetree, knaack.h, linux-iio, Linux Kernel Mailing List On 4/3/21 4:58 PM, Puranjay Mohan wrote: > On Fri, Apr 2, 2021 at 1:43 PM Lars-Peter Clausen <lars@metafoo.de> wrote: >> On 4/1/21 11:16 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> >> Nice and clean driver. Just some comments about the CALIBBIAS. >> >>> [...] >>> +#define TMP117_RESOLUTION_10UC 78125 >> Isn't the unit here 100 uC? > it is 7.8125 milli degrees_C so 78125 x 10^-4 milli degrees_C > which is 78125 x 10^-4 x 10^3 micro degrees_C > so it becomes 78125 x 10^-1 micro degrees_C = 78125 10_microdegrees_C. > Did it in detail so I remember it in the future. I guess you thought > it as 0.78125 millidegrees_C? Ah, I get it, it is a tenth micro degree, not tens of micro degrees, sorry got confused. > [...] > >> I think that would be quite unexpected behavior. The unit should be the >> same. Here in the read function you can just return the register value. > Okay, if you feel that would be right then I will do it. Yea, I think reading and writing in different units would be a bit confusing. >> Just make sure to properly sign extend like for the RAW property. >> >>> + return IIO_VAL_INT_PLUS_MICRO; >>> [...] >>> +} >>> + >>> +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); >>> + s16 off; >>> + >>> + switch (mask) { >>> + case IIO_CHAN_INFO_CALIBBIAS: >>> + off = (s16)val; >> This should have some input validation to avoid writing bogus values to >> the register when the value is out of range. You can either reject out >> of range values or clamp them into the valid range (using the clamp() >> macro). > the maximum value which this register takes is 0xffff, so it should > get clamped automatically when casting to s16? > I might be wrong here. Casting will truncate the upper bits. So something like 0x12345 gets turned into 0x2345. > >>> + return i2c_smbus_write_word_swapped(data->client, >>> + TMP117_REG_TEMP_OFFSET, off); >>> + >>> + default: >>> + return -EINVAL; >>> + } >>> +} >>> + >> [...] > > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-03 15:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-01 9:16 [PATCH v2 0/2] iio: temperature: add support for tmp117 Puranjay Mohan 2021-04-01 9:16 ` [PATCH v2 1/2] dt-bindings: iio: temperature: Add DT bindings for TMP117 Puranjay Mohan 2021-04-01 14:56 ` Rob Herring 2021-04-01 15:13 ` Jonathan Cameron 2021-04-01 16:09 ` Rob Herring 2021-04-01 9:16 ` [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117 Puranjay Mohan 2021-04-01 9:36 ` Andy Shevchenko 2021-04-02 8:14 ` Lars-Peter Clausen 2021-04-03 13:52 ` Andy Shevchenko 2021-04-02 8:13 ` Lars-Peter Clausen 2021-04-03 14:58 ` Puranjay Mohan 2021-04-03 15:03 ` Lars-Peter Clausen
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).