linux-kernel - RE: [PATCH 2/3] drivers: iio: adc: LTC2499 support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Aug 2022 06:30:52 +0000
From:   "Regus, Ciprian" <Ciprian.Regus@...log.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "Linux Kernel Mailing List" <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>
Subject: RE: [PATCH 2/3] drivers: iio: adc: LTC2499 support

In reply to one of Andy's questions.

> On Mon, Aug 22, 2022 at 11:13 PM Jonathan Cameron <jic23@...nel.org>
> wrote:
> > On Mon, 22 Aug 2022 15:51:05 +0300
> > Ciprian Regus <ciprian.regus@...log.com> 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@...log.com>
> 
> 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.

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