Message-ID: <20160506005832.GB1256@tuxbot>
Date:	Thu, 5 May 2016 17:58:32 -0700
From:	Bjorn Andersson <bjorn.andersson@...aro.org>
To:	Andrew Duggan <aduggan@...aptics.com>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Christopher Heiny <cheiny@...aptics.com>,
	linux-input@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Bjorn Andersson <bjorn.andersson@...ymobile.com>
Subject: Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

On Thu 05 May 13:55 PDT 2016, Andrew Duggan wrote:

> Hi Bjorn,
> 
> On 04/21/2016 03:37 PM, Bjorn Andersson wrote:
[..]
> >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.
> 

Note that the required resources could be provided by a kernel module
that is loaded at some future time, or never. So we can't really stall
the transport probe().

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

For the DT binding documents I think it's best to just duplicate the
information, as long as we don't see more common properties between a
growing number of transports (unlikely).


For the implementation we need a context to operate on before probing
the common code, as far as I can see the two places are per transport or
in struct rmi_transport_dev.

The latter would allow us to provide a few common helper functions.


That part that I can see would make it worth adding this is the delay
(Tpowerup?), especially if we need to do something more advanced here.

I've found various numbers of Tpowerup (10ms to 150ms) and some
implementations out there leave the regulators on during sleep, while
others cut the power.

So it seems we will be up for some level of additional logic here, that
warrants the de-duplication.

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

I get the feeling that there is an expected 1:1 relationship between the
transport and core driver, making the use of the driver model for
separation overkill and a potential cause of issues.

I agree this might not warrant the churn of a rewrite, but I'm concern
about the above helpers just working around an artificial issue.

Regards,
Bjorn

Powered by blists - more mailing lists