Re: [PATCH v4] iio: accel: Add support for the Bosch-Sensortec BMI088

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Mon, Mar 23, 2020 at 01:33:58PM +0100, Mike Looijmans wrote:
> On 23-03-2020 12:31, Andy Shevchenko wrote:
> > On Mon, Mar 23, 2020 at 10:28:30AM +0100, Mike Looijmans wrote:
> > > The BMI088 is a combined module with both accelerometer and gyroscope.
> > > This adds the accelerometer driver support for the SPI interface.
> > > The gyroscope part is already supported by the BMG160 driver.
> > 
> > 
> > Thank you, the comment about shared buffer given to v3 still applies.
> > Also see below.

Since you didn't comment on many, I assume you are in favor to follow.
Please, comment if it's not the case.

...

> As most of the method body depends on that "bool" argument, I would actually
> just split it into separate "enable" and "disable" methods. Simpler to read
> and understand, and probably doesn't make a difference in compiled size
> either.

It's even better!

...

> > > +#ifndef BMI088_ACCEL_H
> > > +#define BMI088_ACCEL_H
> > > +
> > > +extern const struct regmap_config bmi088_regmap_conf;
> > > +extern const struct dev_pm_ops bmi088_accel_pm_ops;
> > 
> > Do you need extern?
> 
> probably not.
> 
> > 
> > > +int bmi088_accel_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > > +			    const char *name, bool block_supported);
> > > +int bmi088_accel_core_remove(struct device *dev);
> > 
> > This needs
> > 
> > #include <linux/types.h>
> > 
> > struct device;
> > struct regmap;
> > 
> 
> Hmm, and "struct regmap_config" as well I guess (see above)

Oh, it requires headers then

So,

#include <linux/pm.h>
#include <linux/regmap.h>
#include <linux/types.h>

struct device;

-- 
With Best Regards,
Andy Shevchenko