| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 21 Mar 2021 13:27:05 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Puranjay Mohan <puranjay12@...il.com>
Cc: "alexandru.ardelean@...log.com" <alexandru.ardelean@...log.com>,
"jic23@...nel.org" <jic23@...nel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"knaack.h@....de" <knaack.h@....de>,
"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 2/2] iio: temperature: add driver support for ti tmp117
(Seems it didn't make mailing list)
On Sat, Mar 20, 2021 at 10:55 PM Andy Shevchenko
<andy.shevchenko@...il.com> wrote:
>
>
>
> On Saturday, March 20, 2021, Puranjay Mohan <puranjay12@...il.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@...il.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@...il.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@...il.com>");
>> +MODULE_DESCRIPTION("TI TMP117 Temperature sensor driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.30.1
>>
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists