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: <20210811051938.111191-1-puranjay12@gmail.com> <20210811051938.111191-3-puranjay12@gmail.com>
In-Reply-To: <20210811051938.111191-3-puranjay12@gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 11 Aug 2021 09:55:14 +0300
Message-ID: <CAHp75VeWSwk7g+HUQ21cRKFDBdHKv4M4mTjv6Ng5DnZz-ugj2Q@mail.gmail.com>
Subject: Re: [PATCH v11 2/2] iio: accel: Add driver support for ADXL355
To:     Puranjay Mohan <puranjay12@gmail.com>
Cc:     Michael Hennerich <Michael.Hennerich@analog.com>,
        Jonathan Cameron <jic23@kernel.org>,
        devicetree <devicetree@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        "Bogdan, Dragos" <Dragos.Bogdan@analog.com>,
        Darius <Darius.Berghe@analog.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Aug 11, 2021 at 8:19 AM Puranjay Mohan <puranjay12@gmail.com> wrote:
>
> ADXL355 is a 3-axis MEMS Accelerometer. It offers low noise density,
> low 0g offset drift, low power with selectable measurement ranges.
> It also features programmable high-pass and low-pass filters.

Thanks for an update! My comments below.
After addressing, feel free to add
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

...

> +#ifndef _ADXL355_H_
> +#define _ADXL355_H_
> +
> +#include <linux/regmap.h>

What I meant here is you need to add a forward declaration, i.e.

struct device;

line.

> +extern const struct regmap_access_table adxl355_readable_regs_tbl;
> +extern const struct regmap_access_table adxl355_writeable_regs_tbl;
> +
> +int adxl355_core_probe(struct device *dev, struct regmap *regmap,
> +                      const char *name);
> +
> +#endif /* _ADXL355_H_ */

...

> +#define MEGA(x) ((x) * 1000000)
> +#define TERA(x) ((x) * 1000000000000UL)

What I meant is to define just constants. It will be easier to move
this to a global space (I have a patch, btw, but it may be applied
later). So, just

#define MEGA  1000000UL
#define TERA 1000000000000UUL

And note that the second one should have a ULL suffix.

...

> +       ret = regmap_bulk_write(data->regmap,
> +                               adxl355_chans[chan].offset_reg,
> +                               data->transf_buf, 2);

ARRAY_SIZE() ?

> +       if (ret)
> +               goto set_opmode_unlock;

-- 
With Best Regards,
Andy Shevchenko