Re: [PATCH v5] input: drv260x: Add TI drv260x haptics driver

From: Mark Rutland
Date: Wed Aug 13 2014 - 05:20:26 EST

Hi Dan,

Apologies for the delay.

On Thu, Jul 31, 2014 at 08:14:49PM +0100, Dan Murphy wrote:
> 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 <dmurphy@xxxxxx>
> ---
>
> 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.h
>
> diff --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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/