Re: [PATCH v5] input: drv260x: Add TI drv260x haptics driver
From: Mark Rutland <mark.rutland@arm.com>
Date: 2014-08-13 09:20:20
Also in:
linux-devicetree, lkml
Hi Dan, Apologies for the delay. On Thu, Jul 31, 2014 at 08:14:49PM +0100, Dan Murphy wrote:
quoted hunk ↗ jump to hunk
Add the TI drv260x haptics/vibrator driver. This device uses the input force feedback to produce a wave form to driver an ERM or LRA actuator device. The initial driver supports the devices real time playback mode. But the device has additional wave patterns in ROM. This functionality will be added in future patchsets. Product data sheet is located here: http://www.ti.com/product/drv2605 Signed-off-by: Dan Murphy <redacted> --- v5 - Move register defines to c file and rm header file, error check init in probe, fixed identation, remove empty labels in probe and just return on fail and removed the remove callback and function. Did not factor out the suspend into a stop function. - https://patchwork.kernel.org/patch/4649631/ v4 - Convert regulator to devm, added error checking where required, updated bindings doc, moved dt parsing to separate call and made platform data the first data point, moved vibrator enable and mode programming from play entry to worker thread, added user check and input mutex in suspend/resume calls, moved platform data to individual file and into include platform-data directory, removed file names from file headers - https://patchwork.kernel.org/patch/4642221/ v3 - Updated binding doc, changed to memless device, updated input alloc to devm, removed mutex locking, add sanity checks for mode and library - https://patchwork.kernel.org/patch/4635421/ v2 - Fixed binding doc and patch headline - https://patchwork.kernel.org/patch/4619641/ .../devicetree/bindings/input/ti,drv260x.txt | 50 ++ drivers/input/misc/Kconfig | 9 + drivers/input/misc/Makefile | 1 + drivers/input/misc/drv260x.c | 697 ++++++++++++++++++++ include/dt-bindings/input/ti-drv260x.h | 35 + include/linux/platform_data/drv260x-pdata.h | 29 + 6 files changed, 821 insertions(+) create mode 100644 Documentation/devicetree/bindings/input/ti,drv260x.txt create mode 100644 drivers/input/misc/drv260x.c create mode 100644 include/dt-bindings/input/ti-drv260x.h create mode 100644 include/linux/platform_data/drv260x-pdata.hdiff --git a/Documentation/devicetree/bindings/input/ti,drv260x.txt b/Documentation/devicetree/bindings/input/ti,drv260x.txt new file mode 100644 index 0000000..8e6970d --- /dev/null +++ b/Documentation/devicetree/bindings/input/ti,drv260x.txt@@ -0,0 +1,50 @@ +Texas Instruments - drv260x Haptics driver family + +The drv260x family serial control bus communicates through I2C protocols + +Required properties: + - compatible - One of: + "ti,drv2604" - DRV2604 + "ti,drv2605" - DRV2605 + "ti,drv2605l" - DRV2605L + - reg - I2C slave address + - supply- Required supply regulators are: + "vbat" - battery voltage
This looks a bit odd, and seems to imply the name would be supply-vbat, which doesn't sound right. I assume this should be vbat-supply? If so, just have this as: - vbat-supply - regulator supplying battery voltage.
+ - mode - Power up mode of the chip (defined in include/dt-bindings/input/ti-drv260x.h) + DRV260X_LRA_MODE - Linear Resonance Actuator mode (Piezoelectric) + DRV260X_LRA_NO_CAL_MODE - This is a LRA Mode but there is no calibration + sequence during init. And the device is configured for real + time playback mode (RTP mode). + DRV260X_ERM_MODE - Eccentric Rotating Mass mode (Rotary vibrator) + - library-sel - These are ROM based waveforms pre-programmed into the IC. + This should be set to set the library to use at power up. + (defined in include/dt-bindings/input/ti-drv260x.h) + DRV260X_LIB_A - Pre-programmed Library + DRV260X_LIB_B - Pre-programmed Library + DRV260X_LIB_C - Pre-programmed Library + DRV260X_LIB_D - Pre-programmed Library + DRV260X_LIB_E - Pre-programmed Library + DRV260X_LIB_F - Pre-programmed Library
In the datasheet these seem to be ERM libraries A-E, then LRA library (not F). How does the library selection interact with the mode? Surely it only makes sense to select an ERM library in ERM mode?
+ +Optional properties: + - enable-gpio - gpio pin to enable/disable the device. + - vib_rated_voltage - The rated voltage of the actuator in millivolts. + If this is not set then the value will be defaulted to + 3.2 v. + - vib_overdrive_voltage - The overdrive voltage of the actuator in millivolts. + If this is not set then the value will be defaulted to + 3.2 v.
It looks like you forogt to s/_/-/ here when the code was last updated. These would be better suffixed with -mv given they are in millivolts rather than volts.
+ enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>; + mode = <DRV260X_LRA_MODE>; + library-sel = <DRV260X_LIB_SEL_DEFAULT>;
This value is not described above. What happens if DRV260X_LIB_SEL_DEFAULT is selected? [...]
+/**
+ * Rated and Overdriver Voltages:
+ * Calculated using the formula r = v * 255 / 5.6
+ * where r is what will be written to the register
+ * and v is the rated or overdriver voltage of the actuator
+ **/
+#define DRV260X_DEF_RATED_VOLT 0x90
+#define DRV260X_DEF_OD_CLAMP_VOLT 0x90
+
+static int drv260x_calculate_voltage(int voltage)
+{
+ return (voltage * 255 / 5600);
+}
+Shouldn't the comment be attached to the function rather than the defines? Thanks, Mark.