| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <00FC9A978A94B7418C33AFAE8A35ED49DE6FF3@DFLE09.ent.ti.com>
Date: Thu, 14 Aug 2014 17:44:04 +0000
From: "Murphy, Dan" <dmurphy@...com>
To: Mark Rutland <mark.rutland@....com>
CC: "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"madcatxster@...oid-pointer.net" <madcatxster@...oid-pointer.net>,
"simon@...gewell.org" <simon@...gewell.org>,
"elias.vds@...il.com" <elias.vds@...il.com>,
"dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>
Subject: Re: [PATCH v5] input: drv260x: Add TI drv260x haptics driver
Mark
On 08/13/2014 04:20 AM, Mark Rutland wrote:
> Hi Dan,
>
> Apologies for the delay.
>
No worries. Thanks for the comments
> 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@...com>
>> ---
>>
>> 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.
Will change this.
>
>> + - 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?
>
OK yeah. I will update these to call out ERM vs LRA libraries and add a check in the code
to make sure there is no mis-match of lib to 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.
SGTM. Will update
>
>> + 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?
>
I removed the default and change to empty as that is what the
default mode is is an empty library.
> [...]
>
>> +/**
>> + * 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?
Moved
Thanks Again
Dan
>
> Thanks,
> Mark.
>
--
------------------
Dan Murphy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists