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

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

Hi Andrew,

On Nov 25 2015 or thereabouts, Andrew Duggan wrote:
> 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.

> 
> 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 :)

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

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

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

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.

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

Cheers,
Benjamin

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