Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support

[Jonathan Cameron]

On Mon, 29 Aug 2022 06:30:52 +0000
"Regus, Ciprian" <Ciprian.Regus@xxxxxxxxxx> wrote:

> In reply to one of Andy's questions.
> 
> > On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@xxxxxxxxxx>
> > wrote:  
> > > On Mon, 22 Aug 2022 15:51:05 +0300
> > > Ciprian Regus <ciprian.regus@xxxxxxxxxx> wrote:  
> > 
> > In reply to Jonathan's comments to answer his question and add more
> > comments from me.
> > 
> > ...
> >   
> > > > Datasheet: https://www.analog.com/media/en/technical-  
> > documentation/data-sheets/2499fe.pdf  
> > > >
> > > > Signed-off-by: Ciprian Regus <ciprian.regus@xxxxxxxxxx>  
> > 
> > Tag block mustn't have the blank line(s).
> > 
> > ...
> >   
> > > >  #include <linux/iio/iio.h>
> > > >  #include <linux/iio/driver.h>
> > > >  #include <linux/module.h>
> > > > +#include <linux/property.h>  
> > > why?  
> > 
> > device_get_match_data() requires it.
> > 
> > But why not sort them?
> >   
> > > >  #include <linux/mod_devicetable.h>  
> > 
> > ...
> >   
> > > > -             *val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);  
> > >
> > > Old code here is less than ideal, should be reading into 3 bytes then using
> > > the be24 accesors. Please fix whilst here.  You will need multiple paths here
> > > depending on size.
> > >  
> > > > +             *val = (be32_to_cpu(st->buf) >> st->sub_lsb) -
> > > > +                     BIT(ddata->chip_info->resolution + 1);  
> > 
> > Shouldn't this use some kind of sign_extend()?  
> The BIT(ddata->chip_info->resolution + 1) subtraction part is already doing the sign extension,
> since that bit (which is the most significant one) will be 0 if the result is < 0, and 1 otherwise.

This wins the award for one of the strangest formats I've yet seen.
The format is normal 2s complement 24 bit, but with a bonus upper bit to allow representation of
ever so slightly more than 24 bits.  Specifically on the postive side the out of range value.
0x7fffff + 1 = 0x800000 (which would normally be considered wrapped around to most negative value.

and on the negative side as well
0x800000 - 1 = 0x7fffff (normally most positive value).
It does this by throwing in an additional bit for sign which only tells us something useful
if in these two edge conditions (which are really an out of range error).
That bit corresponds int his case to
0x1000000 and is set for postive values only.

Thus our VS+ value becomes 0x01800000
and VS- value becomes      0x007fffff 
(extended to 32 bits for reasons that will become clear)
Applying 2's complement subtraction of the magic value above.

0x01800000 - 0x01000000 = 0x00800000   (2^23)
0x007fffff - 0x01000000 = 0xff7fffff   (-(2^23 + 1))

So it is indeed sign extended by the above.

Perhaps a few comments to set people off along the right path to what is
going on here would be useful?

Jonathan


> 
> Regards,
> Ciprian
> >
> > Also with a temporary variable for chip info this line can be single.
> > 
> >    struct ... *ci = ddata->chip_info;
> > 
> >    ...BIT(ci->resolution + 1)
> > 
> > ...
> >   
> > > > +     u32 resolution;  
> > 
> > Keep this in a way that the "longer lines go first".
> > 
> > ...
> >   
> > > > +     resolution = st->common_ddata.chip_info->resolution;
> > > > +     st->sub_lsb = 31 - (resolution + 1);
> > > > +     st->recv_size = resolution / BITS_PER_BYTE + 1;  
> > 
> > BITS_TO_BYTES()
> > 
> > ...
> >   
> > > >  static const struct i2c_device_id ltc2497_id[] = {
> > > > -     { "ltc2497", 0 },
> > > > +     { "ltc2497", TYPE_LTC2497 },
> > > > +     { "ltc2499", TYPE_LTC2499 },  
> > 
> > Use pointers here like you have done for the OF table.
> >   
> > > >       { }
> > > >  };  
> > 
> > ...
> >   
> > > > +enum chip_type {
> > > > +     TYPE_LTC2496,
> > > > +     TYPE_LTC2497,
> > > > +     TYPE_LTC2499  
> > 
> > Keep trailing comma.
> >   
> > > > +};  
> > 
> > --
> > With Best Regards,
> > Andy Shevchenko