lkml.org 
[lkml]   [2014]   [Oct]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2] Input: add i2c/smbus driver for elan touchpad
On Fri, Oct 03, 2014 at 10:06:51PM +0100, Dmitry Torokhov wrote:
> [adding devicetree folks...]
>
> On Fri, Oct 03, 2014 at 02:03:57PM -0700, Dmitry Torokhov wrote:
> > Hi Duson,
> >
> > On Tue, Jan 07, 2014 at 11:08:03AM +0800, Duson Lin wrote:
> > > This driver adds support for elan i2c/smbus touchpad found on some
> > > laptops PC
> > >
> >
> > I was looking at your original patch together with varous other patchs
> > for the device in ChromeOS trees and I ended up with the following
> > version. Please take a look and shout if you see something wrong.
> >
> > The SMbus portion is untested as I do not have hardware; I2C does work
> > on actual hardware.
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> > Input: driver for Elan I2C/SMbus touchpad
> >
> > From: Duson Lin <dusonlin@emc.com.tw>
> >
> > This driver supports Elan I2C/SMbous touchpads found in some laptops and
> > also in many Chromebooks.
> >
> > Signed-off-by: Duson Lin <dusonlin@emc.com.tw>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > .../devicetree/bindings/input/elan_i2c.txt | 34 +
> > .../devicetree/bindings/vendor-prefixes.txt | 1
> > drivers/input/mouse/Kconfig | 30 +
> > drivers/input/mouse/Makefile | 5
> > drivers/input/mouse/elan_i2c.h | 86 ++
> > drivers/input/mouse/elan_i2c_core.c | 1129 ++++++++++++++++++++
> > drivers/input/mouse/elan_i2c_i2c.c | 612 +++++++++++
> > drivers/input/mouse/elan_i2c_smbus.c | 514 +++++++++
> > 8 files changed, 2411 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/elan_i2c.txt
> > create mode 100644 drivers/input/mouse/elan_i2c.h
> > create mode 100644 drivers/input/mouse/elan_i2c_core.c
> > create mode 100644 drivers/input/mouse/elan_i2c_i2c.c
> > create mode 100644 drivers/input/mouse/elan_i2c_smbus.c
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan_i2c.txt b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > new file mode 100644
> > index 0000000..0a6ee7d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/elan_i2c.txt
> > @@ -0,0 +1,34 @@
> > +Elantech I2C Touchpad
> > +
> > +Required properties:
> > +- compatible: must be "elan,i2c_touchpad".

s/_/-/ in DT strings and property names plase.

The name seems very generic, and you mention the driver supports a
variety of touchpads. Please choose a specific device name for the
compatible string. Other devices can have that as a fallback entry in
addition to a more specific string.

> > +- reg: I2C address of the chip.
> > +- interrupt-parent: a phandle for the gpio controller (see interrupt
> > + binding[0]).
> > +- interrupts: (gpio) interrupt to which the chip is connected (see interrupt
> > + binding[0]).

If this is an interrupt from the PoV of this device, it doesn't matter
if the other end is connected to a hardwired interrupt controller or a
gpio. So drop the mention of GPIOs frmom the interrupt properties.

> > +
> > +Optional properties:
> > +- wakeup-source: touchpad should be configured as a wakeup source.

s/should be configured as/can be used as/

> > +- pinctrl-names: should be "default" (see pinctrl binding [1]).
> > +- pinctrl-0: a phandle pointing to the pin settings for the device (see
> > + pinctrl binding [1]).
> > +- vcc-supply: a phandle for the regulator supplying 3.3V power.
> > +
> > +[0]: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> > +[1]: Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > +
> > +Example:
> > + &i2c1 {
> > + /* ... */
> > +
> > + touchpad@15 {
> > + compatible = "elan,i2c_touchpad";
> > + reg = <0x15>;
> > + interrupt-parent = <&gpio4>;
> > + interrupts = <0x0 IRQ_TYPE_EDGE_FALLING>;
> > + wakeup-source;
> > + };
> > +
> > + /* ... */
> > + };

Otherwise this looks ok.

Mark.


\
 
 \ /
  Last update: 2014-10-06 12:01    [W:0.057 / U:1.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site