Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220822125106.1106798-1-ciprian.regus@analog.com>
 <20220822125106.1106798-3-ciprian.regus@analog.com> <20220822203731.29c4c33b@jic23-huawei>
In-Reply-To: <20220822203731.29c4c33b@jic23-huawei>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Tue, 23 Aug 2022 18:21:56 +0300
Message-ID: <CAHp75Vft2pvXtxZcpvyV38cR9gOdDxDfCae8g5Uf2q6npWpb+Q@mail.gmail.com>
Subject: Re: [PATCH 2/3] drivers: iio: adc: LTC2499 support
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Ciprian Regus <ciprian.regus@analog.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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

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