linux-kernel - Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the existing rmi4 branch

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5639539F.2050406@synaptics.com>
Date:	Tue, 3 Nov 2015 16:38:55 -0800
From:	Andrew Duggan <aduggan@...aptics.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Benjamin Tissoires <benjamin.tissoires@...il.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Christopher Heiny <cheiny@...aptics.com>,
	Stephen Chandler Paul <cpaul@...hat.com>,
	Linux Input <linux-input@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the
 existing rmi4 branch

On 11/03/2015 06:01 AM, Linus Walleij wrote:
> On Mon, Nov 2, 2015 at 11:14 PM, Andrew Duggan <aduggan@...aptics.com> wrote:
>
>> I have been continuing to work on the synaptics-rmi4 driver and was just
>> trying to figure out what the next step should be. I recently uploaded my
>> changes here https://github.com/aduggan/linux.
> I just tested this patch set on the Ux500 with a TVK UI board.
>
> I added this:
>
>          i2c@...10000 {
>              synaptics@4b {
>                  /* Synaptics RMI4 TM1217 touchscreen */
>                  compatible = "syna,rmi-i2c";
>                  #address-cells = <1>;
>                  #size-cells = <0>;
>                  reg = <0x4b>;
>                  pinctrl-names = "default";
>                  pinctrl-0 = <&synaptics_tvk_mode>;
>                  interrupt-parent = <&gpio2>;
>                  interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
>                  syna,sensor-name="TM1217";
>
>                  rmi-f01@1 {
>                      reg = <0x1>;
>                      syna,nosleep = <1>;
>                  };
>                  rmi-f11@11 {
>                      reg = <0x11>;
>                      syna,f11-flip-x = <1>;
>                      syna,sensor-type = <1>;
>                  };
>              };
>          };
>
> Bootlog:
> [    2.143127] rmi_f01 sensor00.fn01: found RMI device, manufacturer:
> Synaptics, product: TM1217
> [    2.155242] input: Synaptics RMI4 Touch Sensor as
> /devices/sensor00/input/input2
> [    2.165466] rmi_i2c 3-004b: registered rmi i2c driver at 0x4b.
>
> Doing cat /dev/input/event2 gives noise on screen, yay.
>
> Some quick questions I see immediately:
>
> - The DT examples in Documentation/devicetree/bindings/input/*
>    omit
>    #address-cells = <1>;
>    #size-cells = <0>;
>    for the I2C device children (i.e. the function nodes), it needs to look
>    like my example above to work.

The devices which I have tested on haven't needed #address-cells or 
#size-cells. It looks like they are inheriting values defined at higher 
levels. I can add them to the example for completeness and update the 
documentation to say that they may be needed.

> - All things boolean like syna,nosleep and syna,f11-flip-x
>    should just be like:
>    syna,nosleep;
>    syna,f11-flip-x;
>    in the device tree. Use of_property_read_bool() for these.

That makes sense. I will switch them to booleans.

> - syna,sensor-name = "FOO";
>    Why?
>    The bootlog clearly states that f01 can autodetect the sensor
>    type. And f01 is always compiled in, right? So just cut this
>    binding and handling, if the two don't match it's just
>    super-confusing.

The sensor name in the platform data is used by some debug messages 
before F01 is loaded. But, I agree it can be confusing having multiple 
names for the device. Especially, when the sensor name is the same as 
the product id. I'll see if it makes sense to use another name like the 
transport device's name in the logs.

> - /proc/interrupts say this:
>   206:        114          0  nmk2-64-95  20 Edge      sensor00
>   sensor00? Unhelpful. Why can't it say "TM1217", give take
>   an instance number, with the detected sensor name?

Currently, rmi_dev's device name is also being set before F01 is loaded. 
I'll see if we can simply set it later when we have the product id from F01.

> But to me the code seems overall pretty mature. It just works.
> I'll be happy to give a more detailed review if you post it.

Great! I'll look into the issues you highlighted above and start posting 
patches soon. Thanks for reviewing!

Andrew

> Yours,
> Linus Walleij

--
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/