Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1260048 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Linus Walleij <linus.walleij@linaro.org> - 2015-10-31 21:50 +0100
    Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Andrew Duggan <aduggan@synaptics.com> - 2015-11-02 23:20 +0100
      Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Linus Walleij <linus.walleij@linaro.org> - 2015-11-03 11:30 +0100
      Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Linus Walleij <linus.walleij@linaro.org> - 2015-11-03 15:10 +0100
        Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Andrew Duggan <aduggan@synaptics.com> - 2015-11-04 01:40 +0100
          Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Benjamin Tissoires <benjamin.tissoires@gmail.com> - 2015-11-04 09:30 +0100
          Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the  existing rmi4 branch Linus Walleij <linus.walleij@linaro.org> - 2015-11-04 15:00 +0100

#1260048 — Re: [PATCH 00/11] Input: synaptics-rmi4: various fixes for the existing rmi4 branch

On Thu, Jul 23, 2015 at 7:10 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:

> Dmitry, the Sony guys are currently working on upstreaming their work.
> Their Xperia are using a Synaptics I2C touchscreen. They are able to
> boot with an upstream kernel a lot of parts now, and they will
> eventually need the upstream touch controller.

I have a Synaptics touchscreen thing since 2010 waiting in
drivers/staging/ste_rmi4.

Is there *anything* I can do to help further this?

It just seems like a massive body of code that doesn't get the
right love, Benjamin have you considered just merging this into
drivers/staging so it is simple for everyone to participate, or are there
infrastructural blockers?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1261019

On 10/31/2015 01:41 PM, Linus Walleij wrote:
> On Thu, Jul 23, 2015 at 7:10 PM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>
>> Dmitry, the Sony guys are currently working on upstreaming their work.
>> Their Xperia are using a Synaptics I2C touchscreen. They are able to
>> boot with an upstream kernel a lot of parts now, and they will
>> eventually need the upstream touch controller.
> I have a Synaptics touchscreen thing since 2010 waiting in
> drivers/staging/ste_rmi4.
>
> Is there *anything* I can do to help further this?
>
> It just seems like a massive body of code that doesn't get the
> right love, Benjamin have you considered just merging this into
> drivers/staging so it is simple for everyone to participate, or are there
> infrastructural blockers?
>
> Yours,
> Linus Walleij

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've held off on 
posting more patches to the list since there are previous patches still 
outstanding and I didn't want to inundate the list. But, at this point I 
think I would appreciate someone volunteering to review it. I can post 
some or all of it to the list if that's the best place for that to happen.

I've added support for newer devices (Function 12), connected it up to 
hid-rmi to support HID touchpads, and reworked the SPI transport driver. 
I'm able to get basic support for touch working for a variety of devices 
RMI4 over I2C on a Nexus 4, RMI4 over SPI on a Nexus 9, and RMI4 over 
HID on various touchpads. With Benjamin's SMBus work it won't take much 
to also support SMBus touchpads.

I think the biggest blocker right now is just the fact that it is a 
large body of code and there hasn't been enough continuous activity to 
on it. So whenever a new patch is posted, reviewers have to go through 
the time consuming process of relearning the code. But, there are a lot 
of RMI4 devices out there without an upstreamed driver so I think it is 
worth doing.

Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261369

On Mon, Nov 2, 2015 at 11:14 PM, Andrew Duggan <aduggan@synaptics.com> wrote:

> I think the biggest blocker right now is just the fact that it is a large
> body of code and there hasn't been enough continuous activity to on it. So
> whenever a new patch is posted, reviewers have to go through the time
> consuming process of relearning the code. But, there are a lot of RMI4
> devices out there without an upstreamed driver so I think it is worth doing.

OK I've offloaded Dmitry with reviews of this code in the past and I
can do it again.

I'd like to know what to review though and it needs to be posted to
the mailing list with a cover letter.

In the meantime I can clone your git and see if I can get it working
on my Ux500 TVK with the RMI4 Synaptics touchscreen.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261515

On Mon, Nov 2, 2015 at 11:14 PM, Andrew Duggan <aduggan@synaptics.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@80110000 {
            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.

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

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

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

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.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1261989

On 11/03/2015 06:01 AM, Linus Walleij wrote:
> On Mon, Nov 2, 2015 at 11:14 PM, Andrew Duggan <aduggan@synaptics.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@80110000 {
>              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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262126

On Wed, Nov 4, 2015 at 1:38 AM, Andrew Duggan <aduggan@synaptics.com> wrote:
> On 11/03/2015 06:01 AM, Linus Walleij wrote:
>>
>> On Mon, Nov 2, 2015 at 11:14 PM, Andrew Duggan <aduggan@synaptics.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@80110000 {
>>              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!
>

Guys,

sorry for being a little bit unresponsive on this matter. I just moved
across the Atlantic (again :-P ) and am in a rough setup for a few
days (hopefully not weeks). I don't currently have any Synaptics RMI4
over SMBus device with me, but Lyude (AKA Chandler) does have access
to the ones we have in Westford.

In addition to the patches I published in this series (the ones Andrew
has), our latest tree
(https://github.com/bentiss/linux/commits/synaptics-rmi4-smbus-v4.3-rc6%2B)
contains the SMBus support and some attempts to fix suspend/resume
with SMBus touchpads.

The problem we have with suspend/resume on SMBus is that the device is
both seen as a PS/2 device (which we unbind) and the SMBus one.
Problem is, during resume, the serial port is reset, which disable the
SMBus part.

Our solution currently consists in disabling the PM resume function
from the rmi_driver code, and export it through an internal API. Once
the low level driver receive the resume callback (either from the PM
code or the serial driver which just busted our init through its own
reset), this low level driver calls the rmi_driver resume function
which does all the RMI4 resume handling.

I think this should be needed for i2c-hid devices too given that most
of them also enumerates as PS/2.

I always refrained to send this resume fixes because we were not able
to test the rmi4_i2c driver as it doesn't register itself with the PM
subsystem.

These are hopefully the only changes that needs to be done on the
rmi4_core part for having rmi_smbus working.

Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1262292

On Wed, Nov 4, 2015 at 1:38 AM, Andrew Duggan <aduggan@synaptics.com> wrote:

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

I think you should also squash all fixes into the logical steps in the previous
patch series or just squash the entire history and send it out in totally
new, logical chunks of functionality.

The credits can be preserved by just stacking on some signed-off-by
in the end of the patch. Author: is usually set to whoever wrote the
majority of the code, but I suspect few care too much as long as
they're mentioned.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web