Re: [PATCH 00/10] Input: synaptics-rmi4: Synaptics RMI4 Driver rebased on 4.3

On 11/26/2015 02:41 AM, Benjamin Tissoires wrote:

Hi Andrew,

On Nov 25 2015 or thereabouts, Andrew Duggan wrote:

quoted

This is a new patch series which squashes all of the development
history of the RMI4 driver into patches based on functionality. The
first patch adds the core RMI4 functionality needed by all RMI4 devices
and then the additional patches add transport and function drivers for
supporting various devices.

Thanks for this hard work. I did not do a full review of the series yet,
but caught some general design questions that I am writing here.

quoted

Touchpads which are currently using hid-rmi should have the same
functionality, but now knowledge of RMI is handled in the core instead

I tried applying the series on top of a 4.4-rc2, and git am failed. I
think it's good to keep this one in the series, but we might want to
postpone its application when rmi4 hits Linus' tree so Jiri will be able
to take it without conflicting with Dmitry's tree. Well, Jiri and Dmitry
can sort this out :)

I forgot there were in changes to hid-rmi in 4.4. I can fix it up to work with whatever version the core ends up being applied to.

quoted

of in hid-rmi. These patches also provide basic finger reporting for a
wide range of RMI4 touchscreens connected to I2C and SPI busses.
However, additional work may need to be done to implement product specific
features.

I tried to include all of the feedback I received from the previous
patches I posted. However, I did not come up with a satisfactory solution
for allowing function drivers to be built as modules. I think it is fine
to allow function drivers to be enabled or disabled in the core at build

I agree. I do not like the implementation however (I know, I contributed
a lot to it). How about we keep a static array of struct
rmi_function_handler? If we add a .registered file in struct
rmi_function_handler, we could simplify the registering/unregistering of
the functions more easily by looping through the array.

Sounds like a good idea. Right you it takes several lines in multiple files to register and unregister function drivers and its really easy to forget something.

quoted

time. However, if supporting function drivers as modules is a must have
for upstreaming I can continue to try to find a solution. Also, I went
ahead and removed support for polling.

Thanks for removing polling.

I have 2 other general concerns for rmi_bus:
- interrupts:
the current implementation has 2 type of interrupt handling depending on
the transport driver: either generic irqs or specific ones that are
triggered by the transport driver.

I think it would make sense to remove the generic irq handling in rmi4_bus
and let the transport driver handle it. This way, it will be easier for
the transport driver to decide whether or not forwarding the interrupts to
the bus.

This is what is done with the HID bus for the record. The transport
driver calls hid_input_report() when an irq has been processed.

The driver was handling the IRQs originally to avoid duplicating a lot of code in each of the transport devices. But, now that the custom GPIO code has been removed the reasoning for handling it in rmi_driver is less compelling. Plus, now we have SMBus and HID transports which don't handle the IRQs directly. It might also be better to enable and disable IRQs in the transport drivers pm callbacks.

- suspend/resume:
with the smbus implementation, we have quite some troubles with the
suspend/resume part. The reason is that both the rmi_bus and the serio
bus are registering to the pm subsystem and this leads to races between
them. Our current solution is to disable the pm registering in rmi_bus
and let the transport driver handle it at his level (which is in fine
triggered by the serio pm resume actually, not its own pm registration).

The interaction between the SMBus and the PS/2 interface does complicate things. I have not noticed any similar issues with PS/2 + HID/I2C. Probably, because PS/2 + HID/I2C was designed to work without any driver interaction.

I wonder if having the pm functions directly in the bus is a good idea
and if we should not let the transport driver handle those. We should
still keep the rmi specific functions in rmi_core/bus, I am just talking
about the registration of the bus to the subsystem.

I guess we could just call rmi_driver_suspend/resume() from the transports and have it loop through all of the function drivers and call their suspend/resume functions. So far it has just made sense to use the pm subsystem instead of doing things manually. But, none of the other transports have dependencies on devices in separate driver stacks.

[just thinking out loud: maybe we encounter problems of ordering due to
the fact that the bus is registered to the pm subsystem. Maybe if the
device itself in the transport driver registers, there will be guarantee
that the serio resume is called before the rmi_smbus one, which would
solve our problems].

Well if rmi_smbus can register itself with the pm subsystem, could we just make the transport device the parent of the bus device? Would that ensure that all of the pm callbacks get called together? Sounds like some more investigating is needed for figuring out how best to handle suspend/resume.

Thanks,
Andrew

Cheers,
Benjamin

quoted

Thanks,
Andrew

Andrew Duggan (10):
   Input: synaptics-rmi4: Add support for Synaptics RMI4 devices
   Input: synaptics-rmi4: Add I2C transport driver
   Input: synaptics-rmi4: Add device tree support for RMI4 I2C devices
   Input: synaptics-rmi4: Add support for 2D sensors and F11
   Input: synaptics-rmi4: Add device tree support for 2d sensors and F11
   Input: synaptics-rmi4: Add support for F12
   Input: synaptics-rmi4: Add support for F30
   Input: synaptics-rmi4: Add SPI transport driver
   Input: synaptics-rmi4: Add device tree support to the SPI transport
     driver
   HID: rmi: Make hid-rmi a transport driver for synaptics-rmi4

  .../bindings/input/rmi4/rmi_2d_sensor.txt          |   55 +
  .../devicetree/bindings/input/rmi4/rmi_f01.txt     |   40 +
  .../devicetree/bindings/input/rmi4/rmi_i2c.txt     |   53 +
  .../devicetree/bindings/input/rmi4/rmi_spi.txt     |   57 +
  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
  drivers/hid/hid-rmi.c                              |  922 ++-----------
  drivers/input/Kconfig                              |    2 +
  drivers/input/Makefile                             |    2 +
  drivers/input/rmi4/Kconfig                         |   94 ++
  drivers/input/rmi4/Makefile                        |   15 +
  drivers/input/rmi4/rmi_2d_sensor.c                 |  323 +++++
  drivers/input/rmi4/rmi_2d_sensor.h                 |   88 ++
  drivers/input/rmi4/rmi_bus.c                       |  419 ++++++
  drivers/input/rmi4/rmi_bus.h                       |  195 +++
  drivers/input/rmi4/rmi_driver.c                    | 1112 ++++++++++++++++
  drivers/input/rmi4/rmi_driver.h                    |  126 ++
  drivers/input/rmi4/rmi_f01.c                       |  570 ++++++++
  drivers/input/rmi4/rmi_f11.c                       | 1354 ++++++++++++++++++++
  drivers/input/rmi4/rmi_f12.c                       |  487 +++++++
  drivers/input/rmi4/rmi_f30.c                       |  419 ++++++
  drivers/input/rmi4/rmi_i2c.c                       |  270 ++++
  drivers/input/rmi4/rmi_spi.c                       |  464 +++++++
  include/linux/rmi.h                                |  415 ++++++
  include/uapi/linux/input.h                         |    1 +
  24 files changed, 6638 insertions(+), 846 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_2d_sensor.txt
  create mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_f01.txt
  create mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
  create mode 100644 Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
  create mode 100644 drivers/input/rmi4/Kconfig
  create mode 100644 drivers/input/rmi4/Makefile
  create mode 100644 drivers/input/rmi4/rmi_2d_sensor.c
  create mode 100644 drivers/input/rmi4/rmi_2d_sensor.h
  create mode 100644 drivers/input/rmi4/rmi_bus.c
  create mode 100644 drivers/input/rmi4/rmi_bus.h
  create mode 100644 drivers/input/rmi4/rmi_driver.c
  create mode 100644 drivers/input/rmi4/rmi_driver.h
  create mode 100644 drivers/input/rmi4/rmi_f01.c
  create mode 100644 drivers/input/rmi4/rmi_f11.c
  create mode 100644 drivers/input/rmi4/rmi_f12.c
  create mode 100644 drivers/input/rmi4/rmi_f30.c
  create mode 100644 drivers/input/rmi4/rmi_i2c.c
  create mode 100644 drivers/input/rmi4/rmi_spi.c
  create mode 100644 include/linux/rmi.h

-- 
2.5.0