Re: [PATCH v2 1/5] dt-bindings: input: Add binding for bma150 sensor

[Jonathan Bakker]

On 2019-02-18 11:18 a.m., Rob Herring wrote:
> On Sat, Feb 02, 2019 at 04:18:02PM +0100, PaweÅ Chmiel wrote:
>> From: Jonathan Bakker <xc-racer2@xxxxxxx>
>>
>> Add device tree bindings for Bosch BMA150 Accelerometer Sensor
>>
>> Changes from v1:
>>  - Add properties for all of bma150_cfg
>>  - Correct IRQ type in example
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@xxxxxxx>
>> Signed-off-by: PaweÅ Chmiel <pawel.mikolaj.chmiel@xxxxxxxxx>
>> ---
>>  .../bindings/input/bosch,bma150.txt           | 38 +++++++++++++++++++
>>  include/dt-bindings/input/bma150.h            | 22 +++++++++++
>>  include/linux/bma150.h                        | 13 +------
>>  3 files changed, 62 insertions(+), 11 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/input/bosch,bma150.txt
>>  create mode 100644 include/dt-bindings/input/bma150.h
>>
>> diff --git a/Documentation/devicetree/bindings/input/bosch,bma150.txt b/Documentation/devicetree/bindings/input/bosch,bma150.txt
>> new file mode 100644
>> index 000000000000..f644d132f79c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/bosch,bma150.txt
>> @@ -0,0 +1,38 @@
>> +* Bosch BMA150 Accelerometer Sensor
>> +
>> +Also works for the SMB380 and BMA023 accelerometers
>> +
>> +Required properties:
>> +- compatible : Should be "bosch,bma150"
>> +- reg : The I2C address of the sensor
>> +
>> +Optional properties:
>> +- interrupt-parent : should be the phandle for the interrupt controller
> 
> This is implied and can be dropped.
> 
Ok, sounds good.
>> +- interrupts : Interrupt mapping for IRQ.  If not present device will be polled
> 
>> +- any-motion-int : bool for if the any motion interrupt should be enabled
>> +- hg-int : bool for if the high-G interrupt should be enabled
>> +- lg-int : bool for if the low-G interrupt should be enabled
>> +- any-motion-cfg : array of integers for any motion duration and threshold
>> +- hg-cfg : array of integers for high-G hysterisis, duration, and threshold
>> +- lg-cfg : array of integers for low-G hysterisis, duration, and threshold
>> +- range : configuration of range, one of BMA150_RANGE_* as defined in [1]
>> +- bandwidth : refresh rate of device, one of BMA150_BW_* as defined in [1]
> 
> These all need vendor prefixes if they stay.
> 
> What determines all this configuration? It seems like a user may want to 
> change at run-time in which case sysfs would be more appropriate.
> 
> I don't recall seeing other accelerometers with these, but seems these 
> could apply to other accelerometers. In which case, they should be 
> common.
> 
From my understanding of the pre-existing non-DT driver and the datasheet (available at https://media.digikey.com/pdf/Data%20Sheets/Bosch/BMA150.pdf), the *-int and *-cfg are for configuring of what will cause the chip to send an interrupt.  A sysfs property would probably work for them as well although I don't know if they can be adjusted on the fly or not.  My device doesn't have the interrupt line hooked up and instead uses the polling method, so the only tunables I can test are the range and bandwith which are used as a type of digital smoothing.

As I personally use the defaults and there are no current in-tree users, I am fine with dropping all of this configuration and going with the defaults that are said to provide "generic sensitivity performance".  If someone wants to add it in the future, they can add the sysfs and/or DT properties.  The similar bma180 IIO driver hardcodes everything.

However, looking through all this again, instead of making this driver DT-aware, it might make more sense to expand the bma180 IIO driver to add support for bma150.  Most of the accelerometer drivers are already in iio plus it would add support for the temperature part of the sensor.  Would it then make sense to remove this driver entirely or can the two co-exist?
>> +
>> +Example:
>> +
>> +bma150@38 {
> 
> accelerometer@38
Got it.
> 
>> +	compatible = "bosch,bma150";
>> +	reg = <0x38>;
>> +	interrupt-parent = <&gph0>;
>> +	interrupts = <1 IRQ_TYPE_EDGE_RISING>;
>> +	any-motion-int;
>> +	hg-int;
>> +	lg-int;
>> +	any-motion-cfg = <0 0>;
>> +	hg-cfg = <0 150 160>;
>> +	lg-cfg = <0 150 20>;
>> +	range = <BMA150_RANGE_2G>;
>> +	bandwidth = <BMA150_BW_50HZ>;
>> +};
>> +
>> +[1] include/dt-bindings/input/bma150.h
>> diff --git a/include/dt-bindings/input/bma150.h b/include/dt-bindings/input/bma150.h
>> new file mode 100644
>> index 000000000000..fb38ca787f0f
>> --- /dev/null
>> +++ b/include/dt-bindings/input/bma150.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * This header provides bindings for the BMA150 accelerometer
>> + */
>> +#ifndef _DT_BINDINGS_INPUT_BMA150_H
>> +#define _DT_BINDINGS_INPUT_BMA150_H
>> +
>> +/* Range */
>> +#define BMA150_RANGE_2G		0
>> +#define BMA150_RANGE_4G		1
>> +#define BMA150_RANGE_8G		2
>> +
>> +/* Refresh rate */
>> +#define BMA150_BW_25HZ		0
>> +#define BMA150_BW_50HZ		1
>> +#define BMA150_BW_100HZ		2
>> +#define BMA150_BW_190HZ		3
>> +#define BMA150_BW_375HZ		4
>> +#define BMA150_BW_750HZ		5
>> +#define BMA150_BW_1500HZ	6
>> +
>> +#endif /* _DT_BINDINGS_INPUT_BMA150_H */
>> diff --git a/include/linux/bma150.h b/include/linux/bma150.h
>> index 97ade7cdc870..b85266a9c35c 100644
>> --- a/include/linux/bma150.h
>> +++ b/include/linux/bma150.h
>> @@ -20,19 +20,10 @@
>>  #ifndef _BMA150_H_
>>  #define _BMA150_H_
>>  
>> -#define BMA150_DRIVER		"bma150"
>> +#include <dt-bindings/input/bma150.h>
>>  
>> -#define BMA150_RANGE_2G		0
>> -#define BMA150_RANGE_4G		1
>> -#define BMA150_RANGE_8G		2
>> +#define BMA150_DRIVER		"bma150"
>>  
>> -#define BMA150_BW_25HZ		0
>> -#define BMA150_BW_50HZ		1
>> -#define BMA150_BW_100HZ		2
>> -#define BMA150_BW_190HZ		3
>> -#define BMA150_BW_375HZ		4
>> -#define BMA150_BW_750HZ		5
>> -#define BMA150_BW_1500HZ	6
>>  
>>  struct bma150_cfg {
>>  	bool any_motion_int;		/* Set to enable any-motion interrupt */
>> -- 
>> 2.17.1
>>
Thanks,
Jonathan