Message-ID: <20200816100917.4f3308b8@archlinux>
Date:   Sun, 16 Aug 2020 10:09:17 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Crt Mori <cmo@...exis.com>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] iio:temperature:mlx90632: Adding extended
 calibration option

On Tue, 11 Aug 2020 09:53:55 +0200
Crt Mori <cmo@...exis.com> wrote:

> On Sun, 9 Aug 2020 at 23:05, Crt Mori <cmo@...exis.com> wrote:
> >
> > On Sun, 9 Aug 2020 at 15:32, Jonathan Cameron <jic23@...nel.org> wrote:  
> > >
> > > On Sat, 8 Aug 2020 23:57:59 +0200
> > > Crt Mori <cmo@...exis.com> wrote:
> > >  
> > > > Hi,
> > > > I am very sorry you missed them, I thought you saw it (reply on v3 of
> > > > the patch). Maybe something happened to that mail, as it contained
> > > > link to datasheet, so I will omit it now.
> > > >
> > > > Except for the order, only the remarks below are still open (did you
> > > > get the polling trail I did?)
> > > >
> > > > On Sat, 8 Aug 2020 at 22:04, Andy Shevchenko <andy.shevchenko@...il.com> wrote:  
> > > > >
> > > > > On Sat, Aug 8, 2020 at 3:11 PM Crt Mori <cmo@...exis.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?
> > > > >  
> > > >
> > > > These castings are in fact needed. You read unsigned integer, but the
> > > > return value is signed integer. Without the cast it did not extend the
> > > > signed bit, but just wrote the value to signed. Also I find it more
> > > > obvious with casts, that I did not "accidentally" convert to signed.  
> > >
> > > Should we perhaps be making this explicit for the cases where we
> > > are sign extending?  That doesn't include these two as the lvalue
> > > is s16, but does include some of the others.
> > >
> > > sign_extend32(read_tmp, 15)
> > >  
> >
> > So for you lines like
> > s32 read;
> > read = (read + (s16)read_tmp) / 2;
> >
> > would actually be better as:
> > read = (read + sign_extend32(read_tmp, 15)) / 2;
> >
> > Hm, strange. I would read that more align the read_tmp to 32 bit than
> > the value you have in read_tmp is actually a signed 16 bit integer...
> >  
> 
> OK, I did some trails without the casts and had deja-vu from the first
> series of patches I submitted.  I noticed that without a cast the
> value that ends up in variable is not extended to signed, but it is
> unsigned value truncated. This same finding leads to have these casts
> already in current ambient and object raw read functions.
> 
> So now only debate is if sign_extend32 is useful in this case, as read
> in the current case is 32 bit (before it was also 16 bit).
> 
> My preference is to leave unified across the driver.

It is fairly obvious to me what is going on as things stand, but
if others are being confused, the sign_extend32 does make it explicit
that this is all about sign extension.

> 
> > > >  
> > > > > ...
> > > > >  
> > > > > > +       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?
> > > > >  
> > > > When we started 0 to 19 were all open for access, from userspace, then
> > > > only 1 and 2 were used with calculations, and now we use 17, 18 and
> > > > 19. Matter of fact is, I can't provide a descriptive name as it
> > > > depends on DSP version and as you can see now within the same DSP
> > > > version, also on the ID part. While RAM3 vs RAM1 and RAM2 could be
> > > > named RAM_OBJECT1, RAM_OBJECT2, RAM_AMBIENT, knowing our development
> > > > that might not be true in the next configuration, so I rather keep the
> > > > naming as in the datasheet.  
> > > Normal solution for that is to version the defines as well.
> > >
> > > MLX90632_FW3_RAM_1_AMBIENT etc
> > > When a new version changes this, then you introduced new defines to
> > > support that firmware.
> > >  
> >
> > OK will add those, but it is ending up as:
> > MLX90632_RAM_DSP5_AMBIENT
> > MLX90632_RAM_DSP5_EXTENDED_AMBIENT
> > MLX90632_RAM_DSP5_OBJECT_1
> > MLX90632_RAM_DSP5_EXTENDED_OBJECT_1
> > MLX90632_RAM_DSP5_OBJECT_2
> > MLX90632_RAM_DSP5_EXTENDED_OBJECT_2
> >
> > ok?  
That's fine.

> > > >  
> > > > > ...
> > > > >  
> > > > > > +       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.  
> > > >
> > > > OK, will fix that ordering in v5, but will wait till we solve also
> > > > above discussions to avoid adding new versions.
> > > >  
> > > > >
> > > > > --
> > > > > With Best Regards,
> > > > > Andy Shevchenko  
> > > >
> > > > And about that voodoo stuff with numbers:
> > > >
> > > > Honestly, the equation is in the datasheet[1] and this is just making
> > > > floating point to fixed point with proper intermediate scaling
> > > > (initially I had defines of TENTOX, but that was not desired). There
> > > > is no better explanation of this voodoo.  
> > >
> > > We all love fixed point arithmetic :)
> > >
> > > Jonathan  

Powered by blists - more mailing lists