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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 23-03-2020 14:47, Andy Shevchenko wrote:
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.

Didn't mention it explicitly, but no comment means "I agree".



...

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!

...

Hmm, reading the datasheet again about the power modes is now confusing me. It's been awhile since I read that, and I don't think that I got it right....

there are two power setting registers, and it's not quite clear to me any more what I'm supposed to do with them...

I think the intention is that I just set ACC_PWR_CTRL to "4" after reset/probe, and leave it there, and use the ACC_PWR_CONF register to go in and out of suspend state. This affects the temperature sensor as well.


I'll need a bit of caffeine before I get to v5.



+#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;



--
Mike Looijmans



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux