From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Puranjay Mohan <puranjay12@gmail.com>
Cc: Alexandru Ardelean <alexandru.ardelean@analog.com>,
Jonathan Cameron <jic23@kernel.org>,
devicetree <devicetree@vger.kernel.org>,
Hartmut Knaack <knaack.h@gmx.de>,
linux-iio <linux-iio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v2 2/2] iio: temperature: add driver support for ti tmp117
Date: Thu, 1 Apr 2021 12:36:51 +0300 [thread overview]
Message-ID: <CAHp75Vcmyvi4V2D0cnmobTGHCS_ci2FC=cBF8tEoG9v51o8RmQ@mail.gmail.com> (raw)
In-Reply-To: <20210401091648.87421-3-puranjay12@gmail.com>
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
next prev parent reply other threads:[~2021-04-01 9:38 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHp75Vcmyvi4V2D0cnmobTGHCS_ci2FC=cBF8tEoG9v51o8RmQ@mail.gmail.com' \
--to=andy.shevchenko@gmail.com \
--cc=alexandru.ardelean@analog.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=puranjay12@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).