From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Crt Mori <cmo@melexis.com>
Cc: Jonathan Cameron <jic23@kernel.org>,
linux-iio <linux-iio@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] iio:temperature:mlx90632: Adding extended calibration option
Date: Sat, 8 Aug 2020 13:44:17 +0300 [thread overview]
Message-ID: <CAHp75VemS+QJ+m05KVNobYyzbqB-hcBdbLK1YbiaZWbAFJ9xfw@mail.gmail.com> (raw)
In-Reply-To: <20200807232104.1256119-3-cmo@melexis.com>
On Sat, Aug 8, 2020 at 2:24 AM Crt Mori <cmo@melexis.com> wrote:
>
> For some time the market wants medical grade accuracy in medical range,
> while still retaining the declared accuracy outside of the medical range
> within the same sensor. That is why we created extended calibration
> which is automatically switched to when object temperature is too high.
>
> This patch also introduces the object_ambient_temperature variable which
> is needed for more accurate calculation of the object infra-red
> footprint as sensor's ambient temperature might be totally different
> than what the ambient temperature is at object and that is why we can
> have some more errors which can be eliminated. Currently this temperature
> is fixed at 25, but the interface to adjust it by user (with external
> sensor or just IR measurement of the other object which acts as ambient),
> will be introduced in another commit.
Thank you for an update!
My comments below.
...
> struct mlx90632_data {
> struct i2c_client *client;
> struct mutex lock; /* Multiple reads for single measurement */
> struct regmap *regmap;
> u16 emissivity;
> + u8 mtyp; /* measurement type - to enable extended range calculations */
> + u32 object_ambient_temperature;
> };
As a separate patch to have a kernel doc for this, there are plenty of
examples in the kernel, but I will show below an example
/**
* struct foo - private data for the MLX90632 device
* @client: I2C client of the device
* @bar: ...
...
*/
struct foo {
struct i2c_client *client;
... bar;
...
};
...
> + if ((type != MLX90632_MTYP_MEDICAL) && (type != MLX90632_MTYP_EXTENDED))
> + return -EINVAL;
So, just to point out what the difference between & and && (even for
boolean): the former one forces both sides of operand to be executed,
while && checks for only first in case of false.
...
> +static int mlx90632_read_ambient_raw_extended(struct regmap *regmap,
> + s16 *ambient_new_raw, s16 *ambient_old_raw)
> +{
> + unsigned int read_tmp;
> + int ret;
> +
> + ret = regmap_read(regmap, MLX90632_RAM_3(17), &read_tmp);
> + if (ret < 0)
> + return ret;
> + *ambient_new_raw = (s16)read_tmp;
Again the same question, do you need all these castings?
> + ret = regmap_read(regmap, MLX90632_RAM_3(18), &read_tmp);
These 17, 18 and 19 should be defined with descriptive names.
> + if (ret < 0)
> + return ret;
> + *ambient_old_raw = (s16)read_tmp;
> +
> + return 0;
> +}
...
> + int tries = 4;
> + while (tries-- > 0) {
> + ret = mlx90632_perform_measurement(data);
> + if (ret < 0)
> + goto read_unlock;
> +
> + if (ret == 19)
> + break;
> + }
> + if (tries < 0) {
> + ret = -ETIMEDOUT;
> + goto read_unlock;
> + }
Again, please use
unsigned int tries = 4; // or should be 5?
do {
...
} while (ret != ..._ LAST_CHANNEL && --tries);
if (!tries)
...
It will really make the code easier to read.
...
> +static s64 mlx90632_preprocess_temp_obj_extended(s16 object_new_raw, s16 ambient_new_raw,
> + s16 ambient_old_raw, s16 Ka)
> +{
> + s64 VR_IR, kKa, tmp;
> +
> + kKa = ((s64)Ka * 1000LL) >> 10ULL;
> + VR_IR = (s64)ambient_old_raw * 1000000LL +
> + kKa * div64_s64((s64)ambient_new_raw * 1000LL,
> + MLX90632_REF_3);
> + tmp = div64_s64(
> + div64_s64((s64) object_new_raw * 1000000000000LL, MLX90632_REF_12),
> + VR_IR);
> + return div64_s64(tmp << 19ULL, 1000LL);
> +}
It would be nice to have a comment on this voodoo arithmetics.
--
With Best Regards,
Andy Shevchenko
next prev parent reply other threads:[~2020-08-08 10:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-07 23:21 [PATCH v3 0/2] iio: temperature: mlx90632: Add extended calibration calculations Crt Mori
2020-08-07 23:21 ` [PATCH v3 1/2] iio:temperature:mlx90632: Reduce number of equal calulcations Crt Mori
2020-08-08 10:31 ` Andy Shevchenko
2020-08-07 23:21 ` [PATCH v3 2/2] iio:temperature:mlx90632: Adding extended calibration option Crt Mori
2020-08-08 10:44 ` Andy Shevchenko [this message]
2020-08-08 11:02 ` Crt Mori
2020-08-08 10:29 ` [PATCH v3 0/2] iio: temperature: mlx90632: Add extended calibration calculations Andy Shevchenko
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=CAHp75VemS+QJ+m05KVNobYyzbqB-hcBdbLK1YbiaZWbAFJ9xfw@mail.gmail.com \
--to=andy.shevchenko@gmail.com \
--cc=cmo@melexis.com \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/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).