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

[Mike Looijmans <mike.looijmans@topic.nl>]

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