Hi Bjorn,
On 04/21/2016 03:37 PM, Bjorn Andersson wrote:
On Thu 31 Mar 18:47 PDT 2016, Andrew Duggan wrote:
On 03/31/2016 12:14 PM, Bjorn Andersson wrote:
On Thu 31 Mar 11:19 PDT 2016, Dmitry Torokhov wrote:
Hi Bjorn,
On Wed, Mar 30, 2016 at 09:57:29AM -0700, Bjorn Andersson wrote:
From: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
Support the two supplies - vdd and vio - to make it possible to control
power to the Synaptics chip.
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxxxxxx>
Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
---
.../devicetree/bindings/input/rmi4/rmi_i2c.txt | 7 ++++
drivers/input/rmi4/rmi_i2c.c | 45 ++++++++++++++++++++++
Would not we need pretty much the same changes for SPI devices? Can this
be done in core?
Yes, I believe it needs the exact same steps.
I did a initial quick hack on v1 of the patchset and back then it was
possible, when I rebased it a few weeks back I kept ending up in getting
interrupts with the power off.
Looking at the code this is likely because in the resume paths the IRQ
is enabled before we jump to rmi_driver_resume(), so putting this in the
core I ended up calling rmi_process_interrupt_requests() before powering
up the chip.
Actually, I don't think the irq needs to be enabled before calling
rmi_driver_resume(). Typically, the functions are just reading and writing
to registers and do not need to handle interrupts. We could probably call to
rmi_driver_resume() before enabling the irq. I can double check that there
are not any exceptions to this.
I finally got back to giving this a spin.
The problem is that we register the transport device with the driver,
which triggers the rmi_driver probe() which resolves the resources. We
then continue on and call rmi_i2c_init_irq() which will (implicitly)
enable the irq. So if the rmi_driver probe() does not finish in a serial
fashion we will enable interrupts before we have fully initialized the
core.
I don't know if this causes other issues, but with the required delay
after enabling the regulators we always get an interrupt before the
rmi_driver probe() function is finished.
I have not observed any issues related to timing, but it looks like on
the systems which I have tested on rmi_driver() seems to be completing
synchronously before the init_irq() call. I was making the assumption
that rmi_driver() would have completed by the time
rmi_register_transport_device() returned. But, based on your description
and looking into the base driver code I see that the probe can be
deferred and that assumption isn't always true.
I have also considered adding a power callback to the core so that the
transport drivers can set the power independently of suspend and resume. One
example would be to shut off power to a touchpad if a mouse is connected. If
we do need to have the irq enabled before calling rmi_driver_resume() we
could still move regulator support to the core and call the power callback
from the transport drivers.
I see no (sane) way of waiting for the rmi_driver to finish probeing;
there could be cases where it's powered by a regulator (or reset gpio)
that is not yet probed. EPROBE_DEFER will handle this, but we can't wait
for it in the transport driver.
Do we need to wait for rmi_driver to finish probing? What about setting
a flag at the end of rmi_driver_probe() which
rmi_process_interrupt_requests() can check before processing interrupts.
If rmi_driver hasn't finished probing it could just return.
I therefor think these physical resources should be handled in the
context of the transport layer, to make sure we don't have temporal
dependencies to the other layers.
I'm fine with enabling the regulators in the transport driver's probe
function before calling rmi_register_transport_device() to make sure the
device is powered on. What about exporting common functions from
rmi_driver.c which implement common regulator functionality which can
then be called by the transports? To avoid duplication between rmi_i2c
and rmi_spi.
Or we should not have the rmi_driver as a separate device driver at all
- it could be a "library" that runs in the context of the transport
device.
I would have to look into this further to understand the impact on the
bus architecture of merging the physical and transport drivers. But, I
don't think this particular issue warrants such a change. But, if having
them as separate devices does cause a lot of other problems, it might
be possible to merge them.
Andrew
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
