Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200808121026.1300375-1-cmo@melexis.com> <20200808121026.1300375-3-cmo@melexis.com>
In-Reply-To: <20200808121026.1300375-3-cmo@melexis.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Sat, 8 Aug 2020 23:03:51 +0300
Message-ID: <CAHp75VfWk7pCy4Osv0uY0UH4yFS=PRGbE1CNCakuRFTE33SDJg@mail.gmail.com>
Subject: Re: [PATCH v4 2/4] iio:temperature:mlx90632: Adding extended
 calibration option
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 8, 2020 at 3:11 PM 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.

The kernel doc patch should go before this patch.

...

> +       *ambient_new_raw = (s16)read_tmp;

> +       *ambient_old_raw = (s16)read_tmp;

Sorry, did I miss your answer about these castings all over the patch?

...

> +       ret = regmap_read(regmap, MLX90632_RAM_1(17), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(17), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_1(18), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(18), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_1(19), &read_tmp);
> +       ret = regmap_read(regmap, MLX90632_RAM_2(19), &read_tmp);

What so special about these magic 17, 18, 19? Can you provide definitions?

...

> +       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;
> +       }

Please avoid ping-pong type of changes in the same series (similar way
as for kernel doc), which means don't introduce something you are
going to change later on. Patch to move to do {} while () should go
before this one.

-- 
With Best Regards,
Andy Shevchenko