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
