[PATCH] Input: synaptics-rmi4: Support regulator supplies

[PATCH] Input: synaptics-rmi4: Support regulator supplies

[Bjorn Andersson]

From: Bjorn Andersson <bjorn.andersson@sonymobile.com>

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@sonymobile.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 .../devicetree/bindings/input/rmi4/rmi_i2c.txt     |  7 ++++
 drivers/input/rmi4/rmi_i2c.c                       | 45 ++++++++++++++++++++++
 2 files changed, 52 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
index 95fa715c6046..a8c31f40f816 100644
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
+++ b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
@@ -22,6 +22,13 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - syna,reset-delay-ms: The number of milliseconds to wait after resetting the
 			device.
 
+- vdd-supply: VDD power supply.
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
+- vio-supply: VIO power supply
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
+
 Function Parameters:
 Parameters specific to RMI functions are contained in child nodes of the rmi device
  node. Documentation for the parameters of each function can be found in:
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index a96a326b53bd..a8c794daba04 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -11,6 +11,8 @@
 #include <linux/rmi.h>
 #include <linux/irq.h>
 #include <linux/of.h>
+#include <linux/delay.h>
+#include <linux/regulator/consumer.h>
 #include "rmi_driver.h"
 
 #define BUFFER_SIZE_INCREMENT 32
@@ -37,6 +39,8 @@ struct rmi_i2c_xport {
 
 	u8 *tx_buf;
 	size_t tx_buf_size;
+
+	struct regulator_bulk_data supplies[2];
 };
 
 #define RMI_PAGE_SELECT_REGISTER 0xff
@@ -246,6 +250,22 @@ static int rmi_i2c_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	rmi_i2c->supplies[0].supply = "vdd";
+	rmi_i2c->supplies[1].supply = "vio";
+	retval = devm_regulator_bulk_get(&client->dev,
+					 ARRAY_SIZE(rmi_i2c->supplies),
+					 rmi_i2c->supplies);
+	if (retval < 0)
+		return retval;
+
+	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
+				       rmi_i2c->supplies);
+	if (retval < 0)
+		return retval;
+
+	/* Allow the firmware to get ready */
+	msleep(10);
+
 	rmi_i2c->client = client;
 	mutex_init(&rmi_i2c->page_mutex);
 
@@ -286,6 +306,8 @@ static int rmi_i2c_remove(struct i2c_client *client)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 
 	rmi_unregister_transport_device(&rmi_i2c->xport);
+	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
+			       rmi_i2c->supplies);
 
 	return 0;
 }
@@ -308,6 +330,10 @@ static int rmi_i2c_suspend(struct device *dev)
 			dev_warn(dev, "Failed to enable irq for wake: %d\n",
 				ret);
 	}
+
+	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
+			       rmi_i2c->supplies);
+
 	return ret;
 }
 
@@ -317,6 +343,14 @@ static int rmi_i2c_resume(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
+				    rmi_i2c->supplies);
+	if (ret)
+		return ret;
+
+	/* Allow the firmware to get ready */
+	msleep(10);
+
 	enable_irq(rmi_i2c->irq);
 	if (device_may_wakeup(&client->dev)) {
 		ret = disable_irq_wake(rmi_i2c->irq);
@@ -346,6 +380,9 @@ static int rmi_i2c_runtime_suspend(struct device *dev)
 
 	disable_irq(rmi_i2c->irq);
 
+	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
+			       rmi_i2c->supplies);
+
 	return 0;
 }
 
@@ -355,6 +392,14 @@ static int rmi_i2c_runtime_resume(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
+				    rmi_i2c->supplies);
+	if (ret)
+		return ret;
+
+	/* Allow the firmware to get ready */
+	msleep(10);
+
 	enable_irq(rmi_i2c->irq);
 
 	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
-- 
2.5.0

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Dmitry Torokhov]

Hi Bjorn,

On Wed, Mar 30, 2016 at 09:57:29AM -0700, Bjorn Andersson wrote:
> From: Bjorn Andersson <bjorn.andersson@sonymobile.com>
> 
> 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@sonymobile.com>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  .../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?

Thanks.

>  2 files changed, 52 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> index 95fa715c6046..a8c31f40f816 100644
> --- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> +++ b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> @@ -22,6 +22,13 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>  - syna,reset-delay-ms: The number of milliseconds to wait after resetting the
>  			device.
>  
> +- vdd-supply: VDD power supply.
> +See Documentation/devicetree/bindings/regulator/regulator.txt
> +
> +- vio-supply: VIO power supply
> +See Documentation/devicetree/bindings/regulator/regulator.txt
> +
> +
>  Function Parameters:
>  Parameters specific to RMI functions are contained in child nodes of the rmi device
>   node. Documentation for the parameters of each function can be found in:
> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> index a96a326b53bd..a8c794daba04 100644
> --- a/drivers/input/rmi4/rmi_i2c.c
> +++ b/drivers/input/rmi4/rmi_i2c.c
> @@ -11,6 +11,8 @@
>  #include <linux/rmi.h>
>  #include <linux/irq.h>
>  #include <linux/of.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
>  #include "rmi_driver.h"
>  
>  #define BUFFER_SIZE_INCREMENT 32
> @@ -37,6 +39,8 @@ struct rmi_i2c_xport {
>  
>  	u8 *tx_buf;
>  	size_t tx_buf_size;
> +
> +	struct regulator_bulk_data supplies[2];
>  };
>  
>  #define RMI_PAGE_SELECT_REGISTER 0xff
> @@ -246,6 +250,22 @@ static int rmi_i2c_probe(struct i2c_client *client,
>  		return -ENODEV;
>  	}
>  
> +	rmi_i2c->supplies[0].supply = "vdd";
> +	rmi_i2c->supplies[1].supply = "vio";
> +	retval = devm_regulator_bulk_get(&client->dev,
> +					 ARRAY_SIZE(rmi_i2c->supplies),
> +					 rmi_i2c->supplies);
> +	if (retval < 0)
> +		return retval;
> +
> +	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> +				       rmi_i2c->supplies);
> +	if (retval < 0)
> +		return retval;
> +
> +	/* Allow the firmware to get ready */
> +	msleep(10);
> +
>  	rmi_i2c->client = client;
>  	mutex_init(&rmi_i2c->page_mutex);
>  
> @@ -286,6 +306,8 @@ static int rmi_i2c_remove(struct i2c_client *client)
>  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>  
>  	rmi_unregister_transport_device(&rmi_i2c->xport);
> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> +			       rmi_i2c->supplies);
>  
>  	return 0;
>  }
> @@ -308,6 +330,10 @@ static int rmi_i2c_suspend(struct device *dev)
>  			dev_warn(dev, "Failed to enable irq for wake: %d\n",
>  				ret);
>  	}
> +
> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> +			       rmi_i2c->supplies);
> +
>  	return ret;
>  }
>  
> @@ -317,6 +343,14 @@ static int rmi_i2c_resume(struct device *dev)
>  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>  	int ret;
>  
> +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> +				    rmi_i2c->supplies);
> +	if (ret)
> +		return ret;
> +
> +	/* Allow the firmware to get ready */
> +	msleep(10);
> +
>  	enable_irq(rmi_i2c->irq);
>  	if (device_may_wakeup(&client->dev)) {
>  		ret = disable_irq_wake(rmi_i2c->irq);
> @@ -346,6 +380,9 @@ static int rmi_i2c_runtime_suspend(struct device *dev)
>  
>  	disable_irq(rmi_i2c->irq);
>  
> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> +			       rmi_i2c->supplies);
> +
>  	return 0;
>  }
>  
> @@ -355,6 +392,14 @@ static int rmi_i2c_runtime_resume(struct device *dev)
>  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>  	int ret;
>  
> +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> +				    rmi_i2c->supplies);
> +	if (ret)
> +		return ret;
> +
> +	/* Allow the firmware to get ready */
> +	msleep(10);
> +
>  	enable_irq(rmi_i2c->irq);
>  
>  	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
> -- 
> 2.5.0
> 

-- 
Dmitry

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Bjorn Andersson]

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@sonymobile.com>
> > 
> > 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@sonymobile.com>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  .../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.

I assume it's done this way to allow incoming interrupts while functions
are being resumed. Andrew, can you comment on this?

Regards,
Bjorn

> Thanks.
> 
> >  2 files changed, 52 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> > index 95fa715c6046..a8c31f40f816 100644
> > --- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> > +++ b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
> > @@ -22,6 +22,13 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> >  - syna,reset-delay-ms: The number of milliseconds to wait after resetting the
> >  			device.
> >  
> > +- vdd-supply: VDD power supply.
> > +See Documentation/devicetree/bindings/regulator/regulator.txt
> > +
> > +- vio-supply: VIO power supply
> > +See Documentation/devicetree/bindings/regulator/regulator.txt
> > +
> > +
> >  Function Parameters:
> >  Parameters specific to RMI functions are contained in child nodes of the rmi device
> >   node. Documentation for the parameters of each function can be found in:
> > diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
> > index a96a326b53bd..a8c794daba04 100644
> > --- a/drivers/input/rmi4/rmi_i2c.c
> > +++ b/drivers/input/rmi4/rmi_i2c.c
> > @@ -11,6 +11,8 @@
> >  #include <linux/rmi.h>
> >  #include <linux/irq.h>
> >  #include <linux/of.h>
> > +#include <linux/delay.h>
> > +#include <linux/regulator/consumer.h>
> >  #include "rmi_driver.h"
> >  
> >  #define BUFFER_SIZE_INCREMENT 32
> > @@ -37,6 +39,8 @@ struct rmi_i2c_xport {
> >  
> >  	u8 *tx_buf;
> >  	size_t tx_buf_size;
> > +
> > +	struct regulator_bulk_data supplies[2];
> >  };
> >  
> >  #define RMI_PAGE_SELECT_REGISTER 0xff
> > @@ -246,6 +250,22 @@ static int rmi_i2c_probe(struct i2c_client *client,
> >  		return -ENODEV;
> >  	}
> >  
> > +	rmi_i2c->supplies[0].supply = "vdd";
> > +	rmi_i2c->supplies[1].supply = "vio";
> > +	retval = devm_regulator_bulk_get(&client->dev,
> > +					 ARRAY_SIZE(rmi_i2c->supplies),
> > +					 rmi_i2c->supplies);
> > +	if (retval < 0)
> > +		return retval;
> > +
> > +	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> > +				       rmi_i2c->supplies);
> > +	if (retval < 0)
> > +		return retval;
> > +
> > +	/* Allow the firmware to get ready */
> > +	msleep(10);
> > +
> >  	rmi_i2c->client = client;
> >  	mutex_init(&rmi_i2c->page_mutex);
> >  
> > @@ -286,6 +306,8 @@ static int rmi_i2c_remove(struct i2c_client *client)
> >  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
> >  
> >  	rmi_unregister_transport_device(&rmi_i2c->xport);
> > +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> > +			       rmi_i2c->supplies);
> >  
> >  	return 0;
> >  }
> > @@ -308,6 +330,10 @@ static int rmi_i2c_suspend(struct device *dev)
> >  			dev_warn(dev, "Failed to enable irq for wake: %d\n",
> >  				ret);
> >  	}
> > +
> > +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> > +			       rmi_i2c->supplies);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -317,6 +343,14 @@ static int rmi_i2c_resume(struct device *dev)
> >  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
> >  	int ret;
> >  
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> > +				    rmi_i2c->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Allow the firmware to get ready */
> > +	msleep(10);
> > +
> >  	enable_irq(rmi_i2c->irq);
> >  	if (device_may_wakeup(&client->dev)) {
> >  		ret = disable_irq_wake(rmi_i2c->irq);
> > @@ -346,6 +380,9 @@ static int rmi_i2c_runtime_suspend(struct device *dev)
> >  
> >  	disable_irq(rmi_i2c->irq);
> >  
> > +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
> > +			       rmi_i2c->supplies);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -355,6 +392,14 @@ static int rmi_i2c_runtime_resume(struct device *dev)
> >  	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
> >  	int ret;
> >  
> > +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
> > +				    rmi_i2c->supplies);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Allow the firmware to get ready */
> > +	msleep(10);
> > +
> >  	enable_irq(rmi_i2c->irq);
> >  
> >  	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
> > -- 
> > 2.5.0
> > 
> 
> -- 
> Dmitry

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Andrew Duggan]

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@sonymobile.com>
>>>
>>> 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@sonymobile.com>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>   .../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 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.

Andrew

> I assume it's done this way to allow incoming interrupts while functions
> are being resumed. Andrew, can you comment on this?
>
> Regards,
> Bjorn
>
>> Thanks.
>>
>>>   2 files changed, 52 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
>>> index 95fa715c6046..a8c31f40f816 100644
>>> --- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
>>> +++ b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
>>> @@ -22,6 +22,13 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
>>>   - syna,reset-delay-ms: The number of milliseconds to wait after resetting the
>>>   			device.
>>>   
>>> +- vdd-supply: VDD power supply.
>>> +See Documentation/devicetree/bindings/regulator/regulator.txt
>>> +
>>> +- vio-supply: VIO power supply
>>> +See Documentation/devicetree/bindings/regulator/regulator.txt
>>> +
>>> +
>>>   Function Parameters:
>>>   Parameters specific to RMI functions are contained in child nodes of the rmi device
>>>    node. Documentation for the parameters of each function can be found in:
>>> diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
>>> index a96a326b53bd..a8c794daba04 100644
>>> --- a/drivers/input/rmi4/rmi_i2c.c
>>> +++ b/drivers/input/rmi4/rmi_i2c.c
>>> @@ -11,6 +11,8 @@
>>>   #include <linux/rmi.h>
>>>   #include <linux/irq.h>
>>>   #include <linux/of.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/regulator/consumer.h>
>>>   #include "rmi_driver.h"
>>>   
>>>   #define BUFFER_SIZE_INCREMENT 32
>>> @@ -37,6 +39,8 @@ struct rmi_i2c_xport {
>>>   
>>>   	u8 *tx_buf;
>>>   	size_t tx_buf_size;
>>> +
>>> +	struct regulator_bulk_data supplies[2];
>>>   };
>>>   
>>>   #define RMI_PAGE_SELECT_REGISTER 0xff
>>> @@ -246,6 +250,22 @@ static int rmi_i2c_probe(struct i2c_client *client,
>>>   		return -ENODEV;
>>>   	}
>>>   
>>> +	rmi_i2c->supplies[0].supply = "vdd";
>>> +	rmi_i2c->supplies[1].supply = "vio";
>>> +	retval = devm_regulator_bulk_get(&client->dev,
>>> +					 ARRAY_SIZE(rmi_i2c->supplies),
>>> +					 rmi_i2c->supplies);
>>> +	if (retval < 0)
>>> +		return retval;
>>> +
>>> +	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +				       rmi_i2c->supplies);
>>> +	if (retval < 0)
>>> +		return retval;
>>> +
>>> +	/* Allow the firmware to get ready */
>>> +	msleep(10);
>>> +
>>>   	rmi_i2c->client = client;
>>>   	mutex_init(&rmi_i2c->page_mutex);
>>>   
>>> @@ -286,6 +306,8 @@ static int rmi_i2c_remove(struct i2c_client *client)
>>>   	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>>>   
>>>   	rmi_unregister_transport_device(&rmi_i2c->xport);
>>> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +			       rmi_i2c->supplies);
>>>   
>>>   	return 0;
>>>   }
>>> @@ -308,6 +330,10 @@ static int rmi_i2c_suspend(struct device *dev)
>>>   			dev_warn(dev, "Failed to enable irq for wake: %d\n",
>>>   				ret);
>>>   	}
>>> +
>>> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +			       rmi_i2c->supplies);
>>> +
>>>   	return ret;
>>>   }
>>>   
>>> @@ -317,6 +343,14 @@ static int rmi_i2c_resume(struct device *dev)
>>>   	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>>>   	int ret;
>>>   
>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +				    rmi_i2c->supplies);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Allow the firmware to get ready */
>>> +	msleep(10);
>>> +
>>>   	enable_irq(rmi_i2c->irq);
>>>   	if (device_may_wakeup(&client->dev)) {
>>>   		ret = disable_irq_wake(rmi_i2c->irq);
>>> @@ -346,6 +380,9 @@ static int rmi_i2c_runtime_suspend(struct device *dev)
>>>   
>>>   	disable_irq(rmi_i2c->irq);
>>>   
>>> +	regulator_bulk_disable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +			       rmi_i2c->supplies);
>>> +
>>>   	return 0;
>>>   }
>>>   
>>> @@ -355,6 +392,14 @@ static int rmi_i2c_runtime_resume(struct device *dev)
>>>   	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
>>>   	int ret;
>>>   
>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(rmi_i2c->supplies),
>>> +				    rmi_i2c->supplies);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	/* Allow the firmware to get ready */
>>> +	msleep(10);
>>> +
>>>   	enable_irq(rmi_i2c->irq);
>>>   
>>>   	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
>>> -- 
>>> 2.5.0
>>>
>> -- 
>> Dmitry

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Bjorn Andersson]

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@sonymobile.com>
> >>>
> >>>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@sonymobile.com>
> >>>Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>---
> >>>  .../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 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.


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.

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.

Regards,
Bjorn

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Andrew Duggan]

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@sonymobile.com>
>>>>>
>>>>> 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@sonymobile.com>
>>>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>>> ---
>>>>>   .../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

Re: [PATCH] Input: synaptics-rmi4: Support regulator supplies

[Bjorn Andersson]

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

[PATCH v2 0/3] input: rmi4: Regulator supply support

[Bjorn Andersson]

The first version of the regulator support patch suffered from being
implemented in the transport driver, as a work around for resource availability
racing (EPROBE_DEFER of the core driver) with the interrupt handler.

After reconsidering the solutions discussed following that I concluded that the
interrupt management is not really part of the transport, neither conceptually
or electrically. I therefor here suggest (patch 1/3) to move the interrupt
registration and handling to the core rmi driver.

This solves the potential race of interrupts being delivered in the transport
driver before the core driver have been given a chance to recover from probe
deferral.

Patch 2/3 then add the necessary code for acquiring regulator handles and
enabling these.

Patch 3/3 removes the set_page() done in the transport drivers, as we can't
rely on the chip becoming available at any time during the initialization/probe
phase.

Bjorn Andersson (3):
  input: rmi4: Move IRQ handling to rmi_driver
  input: rmi4: Acquire and enable VDD and VIO supplies
  input: rmi4: Remove set_page() call before core is initialized

 .../devicetree/bindings/input/rmi4/rmi_i2c.txt     |  6 ++
 .../devicetree/bindings/input/rmi4/rmi_spi.txt     |  6 ++
 drivers/input/rmi4/rmi_driver.c                    | 89 +++++++++++++++++++++-
 drivers/input/rmi4/rmi_i2c.c                       | 84 ++------------------
 drivers/input/rmi4/rmi_spi.c                       | 83 ++------------------
 include/linux/rmi.h                                | 11 ++-
 6 files changed, 118 insertions(+), 161 deletions(-)

-- 
2.5.0

[PATCH v2 1/3] input: rmi4: Move IRQ handling to rmi_driver

[Bjorn Andersson]

The attn IRQ is related to the chip, rather than the transport, so move
all handling of interrupts to the core driver. This also makes sure that
there are no races between interrupts and availability of the resources
used by the core driver.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/input/rmi4/rmi_driver.c | 69 +++++++++++++++++++++++++++++++++++---
 drivers/input/rmi4/rmi_i2c.c    | 73 +++--------------------------------------
 drivers/input/rmi4/rmi_spi.c    | 72 +++-------------------------------------
 include/linux/rmi.h             |  7 ++--
 4 files changed, 79 insertions(+), 142 deletions(-)

diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index faa295ec4f31..48c7991c9898 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -17,6 +17,7 @@
 #include <linux/bitmap.h>
 #include <linux/delay.h>
 #include <linux/fs.h>
+#include <linux/irq.h>
 #include <linux/kconfig.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
@@ -134,7 +135,7 @@ static void process_one_interrupt(struct rmi_driver_data *data,
 	}
 }
 
-int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
+static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
 {
 	struct rmi_driver_data *data = dev_get_drvdata(&rmi_dev->dev);
 	struct device *dev = &rmi_dev->dev;
@@ -179,7 +180,42 @@ int rmi_process_interrupt_requests(struct rmi_device *rmi_dev)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rmi_process_interrupt_requests);
+
+static irqreturn_t rmi_irq_fn(int irq, void *dev_id)
+{
+	struct rmi_device *rmi_dev = dev_id;
+	int ret;
+
+	ret = rmi_process_interrupt_requests(rmi_dev);
+	if (ret)
+		rmi_dbg(RMI_DEBUG_CORE, &rmi_dev->dev,
+			"Failed to process interrupt request: %d\n", ret);
+
+	return IRQ_HANDLED;
+}
+
+static int rmi_irq_init(struct rmi_device *rmi_dev)
+{
+	struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
+	int irq_flags = irq_get_trigger_type(pdata->irq);
+	int ret;
+
+	if (!irq_flags)
+		irq_flags = IRQF_TRIGGER_LOW;
+
+	ret = devm_request_threaded_irq(&rmi_dev->dev, pdata->irq, NULL,
+				        rmi_irq_fn, irq_flags | IRQF_ONESHOT,
+					dev_name(rmi_dev->xport->dev),
+					rmi_dev);
+	if (ret < 0) {
+		dev_warn(&rmi_dev->dev, "Failed to register interrupt %d\n",
+			 pdata->irq);
+
+		return ret;
+	}
+
+	return 0;
+}
 
 static int suspend_one_function(struct rmi_function *fn)
 {
@@ -786,8 +822,10 @@ err_put_fn:
 	return error;
 }
 
-int rmi_driver_suspend(struct rmi_device *rmi_dev)
+int rmi_driver_suspend(struct rmi_device *rmi_dev, bool enable_wake)
 {
+	struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
+	int irq = pdata->irq;
 	int retval = 0;
 
 	retval = rmi_suspend_functions(rmi_dev);
@@ -795,14 +833,33 @@ int rmi_driver_suspend(struct rmi_device *rmi_dev)
 		dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n",
 			retval);
 
+	disable_irq(irq);
+	if (enable_wake && device_may_wakeup(rmi_dev->xport->dev)) {
+		retval = enable_irq_wake(irq);
+		if (!retval)
+			dev_warn(&rmi_dev->dev,
+				 "Failed to enable irq for wake: %d\n",
+				 retval);
+	}
 	return retval;
 }
 EXPORT_SYMBOL_GPL(rmi_driver_suspend);
 
-int rmi_driver_resume(struct rmi_device *rmi_dev)
+int rmi_driver_resume(struct rmi_device *rmi_dev, bool clear_wake)
 {
+	struct rmi_device_platform_data *pdata = rmi_get_platform_data(rmi_dev);
+	int irq = pdata->irq;
 	int retval;
 
+	enable_irq(irq);
+	if (clear_wake && device_may_wakeup(rmi_dev->xport->dev)) {
+		retval = disable_irq_wake(irq);
+		if (!retval)
+			dev_warn(&rmi_dev->dev,
+				 "Failed to disable irq for wake: %d\n",
+				 retval);
+	}
+
 	retval = rmi_resume_functions(rmi_dev);
 	if (retval)
 		dev_warn(&rmi_dev->dev, "Failed to suspend functions: %d\n",
@@ -1003,6 +1060,10 @@ static int rmi_driver_probe(struct device *dev)
 		}
 	}
 
+	retval = rmi_irq_init(rmi_dev);
+	if (retval < 0)
+		goto err_destroy_functions;
+
 	if (data->f01_container->dev.driver)
 		/* Driver already bound, so enable ATTN now. */
 		return enable_sensor(rmi_dev);
diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index a96a326b53bd..18f6a9f4aeef 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -9,7 +9,6 @@
 
 #include <linux/i2c.h>
 #include <linux/rmi.h>
-#include <linux/irq.h>
 #include <linux/of.h>
 #include "rmi_driver.h"
 
@@ -33,8 +32,6 @@ struct rmi_i2c_xport {
 	struct mutex page_mutex;
 	int page;
 
-	int irq;
-
 	u8 *tx_buf;
 	size_t tx_buf_size;
 };
@@ -172,42 +169,6 @@ static const struct rmi_transport_ops rmi_i2c_ops = {
 	.read_block	= rmi_i2c_read_block,
 };
 
-static irqreturn_t rmi_i2c_irq(int irq, void *dev_id)
-{
-	struct rmi_i2c_xport *rmi_i2c = dev_id;
-	struct rmi_device *rmi_dev = rmi_i2c->xport.rmi_dev;
-	int ret;
-
-	ret = rmi_process_interrupt_requests(rmi_dev);
-	if (ret)
-		rmi_dbg(RMI_DEBUG_XPORT, &rmi_dev->dev,
-			"Failed to process interrupt request: %d\n", ret);
-
-	return IRQ_HANDLED;
-}
-
-static int rmi_i2c_init_irq(struct i2c_client *client)
-{
-	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
-	int irq_flags = irqd_get_trigger_type(irq_get_irq_data(rmi_i2c->irq));
-	int ret;
-
-	if (!irq_flags)
-		irq_flags = IRQF_TRIGGER_LOW;
-
-	ret = devm_request_threaded_irq(&client->dev, rmi_i2c->irq, NULL,
-			rmi_i2c_irq, irq_flags | IRQF_ONESHOT, client->name,
-			rmi_i2c);
-	if (ret < 0) {
-		dev_warn(&client->dev, "Failed to register interrupt %d\n",
-			rmi_i2c->irq);
-
-		return ret;
-	}
-
-	return 0;
-}
-
 #ifdef CONFIG_OF
 static const struct of_device_id rmi_i2c_of_match[] = {
 	{ .compatible = "syna,rmi4-i2c" },
@@ -235,8 +196,7 @@ static int rmi_i2c_probe(struct i2c_client *client,
 	if (!client->dev.of_node && client_pdata)
 		*pdata = *client_pdata;
 
-	if (client->irq > 0)
-		rmi_i2c->irq = client->irq;
+	pdata->irq = client->irq;
 
 	rmi_dbg(RMI_DEBUG_XPORT, &client->dev, "Probing %s.\n",
 			dev_name(&client->dev));
@@ -272,10 +232,6 @@ static int rmi_i2c_probe(struct i2c_client *client,
 		return retval;
 	}
 
-	retval = rmi_i2c_init_irq(client);
-	if (retval < 0)
-		return retval;
-
 	dev_info(&client->dev, "registered rmi i2c driver at %#04x.\n",
 			client->addr);
 	return 0;
@@ -297,17 +253,10 @@ static int rmi_i2c_suspend(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
-	ret = rmi_driver_suspend(rmi_i2c->xport.rmi_dev);
+	ret = rmi_driver_suspend(rmi_i2c->xport.rmi_dev, true);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
-	disable_irq(rmi_i2c->irq);
-	if (device_may_wakeup(&client->dev)) {
-		ret = enable_irq_wake(rmi_i2c->irq);
-		if (!ret)
-			dev_warn(dev, "Failed to enable irq for wake: %d\n",
-				ret);
-	}
 	return ret;
 }
 
@@ -317,15 +266,7 @@ static int rmi_i2c_resume(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
-	enable_irq(rmi_i2c->irq);
-	if (device_may_wakeup(&client->dev)) {
-		ret = disable_irq_wake(rmi_i2c->irq);
-		if (!ret)
-			dev_warn(dev, "Failed to disable irq for wake: %d\n",
-				ret);
-	}
-
-	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
+	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev, true);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
@@ -340,12 +281,10 @@ static int rmi_i2c_runtime_suspend(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
-	ret = rmi_driver_suspend(rmi_i2c->xport.rmi_dev);
+	ret = rmi_driver_suspend(rmi_i2c->xport.rmi_dev, false);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
-	disable_irq(rmi_i2c->irq);
-
 	return 0;
 }
 
@@ -355,9 +294,7 @@ static int rmi_i2c_runtime_resume(struct device *dev)
 	struct rmi_i2c_xport *rmi_i2c = i2c_get_clientdata(client);
 	int ret;
 
-	enable_irq(rmi_i2c->irq);
-
-	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev);
+	ret = rmi_driver_resume(rmi_i2c->xport.rmi_dev, false);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
diff --git a/drivers/input/rmi4/rmi_spi.c b/drivers/input/rmi4/rmi_spi.c
index 55bd1b34970c..f3e9e488635c 100644
--- a/drivers/input/rmi4/rmi_spi.c
+++ b/drivers/input/rmi4/rmi_spi.c
@@ -12,7 +12,6 @@
 #include <linux/rmi.h>
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
-#include <linux/irq.h>
 #include <linux/of.h>
 #include "rmi_driver.h"
 
@@ -44,8 +43,6 @@ struct rmi_spi_xport {
 	struct mutex page_mutex;
 	int page;
 
-	int irq;
-
 	u8 *rx_buf;
 	u8 *tx_buf;
 	int xfer_buf_size;
@@ -326,41 +323,6 @@ static const struct rmi_transport_ops rmi_spi_ops = {
 	.read_block	= rmi_spi_read_block,
 };
 
-static irqreturn_t rmi_spi_irq(int irq, void *dev_id)
-{
-	struct rmi_spi_xport *rmi_spi = dev_id;
-	struct rmi_device *rmi_dev = rmi_spi->xport.rmi_dev;
-	int ret;
-
-	ret = rmi_process_interrupt_requests(rmi_dev);
-	if (ret)
-		rmi_dbg(RMI_DEBUG_XPORT, &rmi_dev->dev,
-			"Failed to process interrupt request: %d\n", ret);
-
-	return IRQ_HANDLED;
-}
-
-static int rmi_spi_init_irq(struct spi_device *spi)
-{
-	struct rmi_spi_xport *rmi_spi = spi_get_drvdata(spi);
-	int irq_flags = irqd_get_trigger_type(irq_get_irq_data(rmi_spi->irq));
-	int ret;
-
-	if (!irq_flags)
-		irq_flags = IRQF_TRIGGER_LOW;
-
-	ret = devm_request_threaded_irq(&spi->dev, rmi_spi->irq, NULL,
-			rmi_spi_irq, irq_flags | IRQF_ONESHOT,
-			dev_name(&spi->dev), rmi_spi);
-	if (ret < 0) {
-		dev_warn(&spi->dev, "Failed to register interrupt %d\n",
-			rmi_spi->irq);
-		return ret;
-	}
-
-	return 0;
-}
-
 #ifdef CONFIG_OF
 static int rmi_spi_of_probe(struct spi_device *spi,
 			struct rmi_device_platform_data *pdata)
@@ -433,8 +395,7 @@ static int rmi_spi_probe(struct spi_device *spi)
 		return retval;
 	}
 
-	if (spi->irq > 0)
-		rmi_spi->irq = spi->irq;
+	pdata->irq = spi->irq;
 
 	rmi_spi->spi = spi;
 	mutex_init(&rmi_spi->page_mutex);
@@ -465,10 +426,6 @@ static int rmi_spi_probe(struct spi_device *spi)
 		return retval;
 	}
 
-	retval = rmi_spi_init_irq(spi);
-	if (retval < 0)
-		return retval;
-
 	dev_info(&spi->dev, "registered RMI SPI driver\n");
 	return 0;
 }
@@ -489,17 +446,10 @@ static int rmi_spi_suspend(struct device *dev)
 	struct rmi_spi_xport *rmi_spi = spi_get_drvdata(spi);
 	int ret;
 
-	ret = rmi_driver_suspend(rmi_spi->xport.rmi_dev);
+	ret = rmi_driver_suspend(rmi_spi->xport.rmi_dev, true);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
-	disable_irq(rmi_spi->irq);
-	if (device_may_wakeup(&spi->dev)) {
-		ret = enable_irq_wake(rmi_spi->irq);
-		if (!ret)
-			dev_warn(dev, "Failed to enable irq for wake: %d\n",
-				ret);
-	}
 	return ret;
 }
 
@@ -509,15 +459,7 @@ static int rmi_spi_resume(struct device *dev)
 	struct rmi_spi_xport *rmi_spi = spi_get_drvdata(spi);
 	int ret;
 
-	enable_irq(rmi_spi->irq);
-	if (device_may_wakeup(&spi->dev)) {
-		ret = disable_irq_wake(rmi_spi->irq);
-		if (!ret)
-			dev_warn(dev, "Failed to disable irq for wake: %d\n",
-				ret);
-	}
-
-	ret = rmi_driver_resume(rmi_spi->xport.rmi_dev);
+	ret = rmi_driver_resume(rmi_spi->xport.rmi_dev, true);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
@@ -532,12 +474,10 @@ static int rmi_spi_runtime_suspend(struct device *dev)
 	struct rmi_spi_xport *rmi_spi = spi_get_drvdata(spi);
 	int ret;
 
-	ret = rmi_driver_suspend(rmi_spi->xport.rmi_dev);
+	ret = rmi_driver_suspend(rmi_spi->xport.rmi_dev, false);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
-	disable_irq(rmi_spi->irq);
-
 	return 0;
 }
 
@@ -547,9 +487,7 @@ static int rmi_spi_runtime_resume(struct device *dev)
 	struct rmi_spi_xport *rmi_spi = spi_get_drvdata(spi);
 	int ret;
 
-	enable_irq(rmi_spi->irq);
-
-	ret = rmi_driver_resume(rmi_spi->xport.rmi_dev);
+	ret = rmi_driver_resume(rmi_spi->xport.rmi_dev, false);
 	if (ret)
 		dev_warn(dev, "Failed to resume device: %d\n", ret);
 
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index e0aca1476001..5944e6c2470d 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -204,9 +204,11 @@ struct rmi_device_platform_data_spi {
  * @reset_delay_ms - after issuing a reset command to the touch sensor, the
  * driver waits a few milliseconds to give the firmware a chance to
  * to re-initialize.  You can override the default wait period here.
+ * @irq: irq associated with the attn gpio line, or negative
  */
 struct rmi_device_platform_data {
 	int reset_delay_ms;
+	int irq;
 
 	struct rmi_device_platform_data_spi spi_data;
 
@@ -352,8 +354,7 @@ struct rmi_driver_data {
 
 int rmi_register_transport_device(struct rmi_transport_dev *xport);
 void rmi_unregister_transport_device(struct rmi_transport_dev *xport);
-int rmi_process_interrupt_requests(struct rmi_device *rmi_dev);
 
-int rmi_driver_suspend(struct rmi_device *rmi_dev);
-int rmi_driver_resume(struct rmi_device *rmi_dev);
+int rmi_driver_suspend(struct rmi_device *rmi_dev, bool enable_wake);
+int rmi_driver_resume(struct rmi_device *rmi_dev, bool clear_wake);
 #endif
-- 
2.5.0

[PATCH v2 2/3] input: rmi4: Acquire and enable VDD and VIO supplies

[Bjorn Andersson]

For systems where the rmi4 device is not powered by always-on regulators
we need to acquire handles to VDD and VIO and enable these.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v1:
- Added patch 1 and 3
- Moved regulator handling to the core driver

 .../devicetree/bindings/input/rmi4/rmi_i2c.txt       |  6 ++++++
 .../devicetree/bindings/input/rmi4/rmi_spi.txt       |  6 ++++++
 drivers/input/rmi4/rmi_driver.c                      | 20 ++++++++++++++++++++
 include/linux/rmi.h                                  |  4 +++-
 4 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
index 95fa715c6046..29aedd05cb81 100644
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
+++ b/Documentation/devicetree/bindings/input/rmi4/rmi_i2c.txt
@@ -22,6 +22,12 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - syna,reset-delay-ms: The number of milliseconds to wait after resetting the
 			device.
 
+- vdd-supply: VDD power supply.
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
+- vio-supply: VIO power supply
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
 Function Parameters:
 Parameters specific to RMI functions are contained in child nodes of the rmi device
  node. Documentation for the parameters of each function can be found in:
diff --git a/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt b/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
index a4ca7828f21d..71388bac4bc7 100644
--- a/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
+++ b/Documentation/devicetree/bindings/input/rmi4/rmi_spi.txt
@@ -22,6 +22,12 @@ See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 - spi-rx-delay-us: microsecond delay after a read transfer.
 - spi-tx-delay-us: microsecond delay after a write transfer.
 
+- vdd-supply: VDD power supply.
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
+- vio-supply: VIO power supply
+See Documentation/devicetree/bindings/regulator/regulator.txt
+
 Function Parameters:
 Parameters specific to RMI functions are contained in child nodes of the rmi device
  node. Documentation for the parameters of each function can be found in:
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 48c7991c9898..7e73539719cb 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -24,6 +24,7 @@
 #include <linux/of.h>
 #include <uapi/linux/input.h>
 #include <linux/rmi.h>
+#include <linux/regulator/consumer.h>
 #include "rmi_bus.h"
 #include "rmi_driver.h"
 
@@ -875,6 +876,9 @@ static int rmi_driver_remove(struct device *dev)
 
 	rmi_free_function_list(rmi_dev);
 
+	regulator_bulk_disable(ARRAY_SIZE(rmi_dev->supplies),
+			       rmi_dev->supplies);
+
 	return 0;
 }
 
@@ -938,6 +942,22 @@ static int rmi_driver_probe(struct device *dev)
 	data->rmi_dev = rmi_dev;
 	dev_set_drvdata(&rmi_dev->dev, data);
 
+	rmi_dev->supplies[0].supply = "vdd";
+	rmi_dev->supplies[1].supply = "vio";
+	retval = devm_regulator_bulk_get(rmi_dev->xport->dev,
+					 ARRAY_SIZE(rmi_dev->supplies),
+					 rmi_dev->supplies);
+	if (retval < 0)
+		return retval;
+
+	retval = regulator_bulk_enable(ARRAY_SIZE(rmi_dev->supplies),
+				       rmi_dev->supplies);
+	if (retval < 0)
+		return retval;
+
+	/* Allow the firmware to get ready */
+	msleep(10);
+
 	/*
 	 * Right before a warm boot, the sensor might be in some unusual state,
 	 * such as F54 diagnostics, or F34 bootloader mode after a firmware
diff --git a/include/linux/rmi.h b/include/linux/rmi.h
index 5944e6c2470d..38ff4c1d31d4 100644
--- a/include/linux/rmi.h
+++ b/include/linux/rmi.h
@@ -16,6 +16,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/types.h>
+#include <linux/regulator/consumer.h>
 
 #define NAME_BUFFER_SIZE 256
 
@@ -315,7 +316,7 @@ struct rmi_driver {
  * @number: Unique number for the device on the bus.
  * @driver: Pointer to associated driver
  * @xport: Pointer to the transport interface
- *
+ * @supplies: vdd and vdio regulator supplies
  */
 struct rmi_device {
 	struct device dev;
@@ -324,6 +325,7 @@ struct rmi_device {
 	struct rmi_driver *driver;
 	struct rmi_transport_dev *xport;
 
+	struct regulator_bulk_data supplies[2];
 };
 
 struct rmi_driver_data {
-- 
2.5.0

[PATCH v2 3/3] input: rmi4: Remove set_page() call before core is initialized

[Bjorn Andersson]

In the case of the chip not already being powered we can't call
set_page() before we're letting the core driver enable power to the
chip.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/input/rmi4/rmi_i2c.c | 11 ++---------
 drivers/input/rmi4/rmi_spi.c | 11 ++---------
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/input/rmi4/rmi_i2c.c b/drivers/input/rmi4/rmi_i2c.c
index 18f6a9f4aeef..3e32b320c8d2 100644
--- a/drivers/input/rmi4/rmi_i2c.c
+++ b/drivers/input/rmi4/rmi_i2c.c
@@ -215,15 +215,8 @@ static int rmi_i2c_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, rmi_i2c);
 
-	/*
-	 * Setting the page to zero will (a) make sure the PSR is in a
-	 * known state, and (b) make sure we can talk to the device.
-	 */
-	retval = rmi_set_page(rmi_i2c, 0);
-	if (retval) {
-		dev_err(&client->dev, "Failed to set page select to 0.\n");
-		return retval;
-	}
+	/* Invalidate current page to force rmi_set_page() on next access */
+	rmi_i2c->page = -1;
 
 	retval = rmi_register_transport_device(&rmi_i2c->xport);
 	if (retval) {
diff --git a/drivers/input/rmi4/rmi_spi.c b/drivers/input/rmi4/rmi_spi.c
index f3e9e488635c..36c6912685a7 100644
--- a/drivers/input/rmi4/rmi_spi.c
+++ b/drivers/input/rmi4/rmi_spi.c
@@ -410,15 +410,8 @@ static int rmi_spi_probe(struct spi_device *spi)
 	if (retval)
 		return retval;
 
-	/*
-	 * Setting the page to zero will (a) make sure the PSR is in a
-	 * known state, and (b) make sure we can talk to the device.
-	 */
-	retval = rmi_set_page(rmi_spi, 0);
-	if (retval) {
-		dev_err(&spi->dev, "Failed to set page select to 0.\n");
-		return retval;
-	}
+	/* Invalidate current page to force rmi_set_page() on next access */
+	rmi_spi->page = -1;
 
 	retval = rmi_register_transport_device(&rmi_spi->xport);
 	if (retval) {
-- 
2.5.0

Re: [PATCH v2 2/3] input: rmi4: Acquire and enable VDD and VIO supplies

[Rob Herring]

On Fri, May 06, 2016 at 09:40:07PM -0700, Bjorn Andersson wrote:
> For systems where the rmi4 device is not powered by always-on regulators
> we need to acquire handles to VDD and VIO and enable these.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v1:
> - Added patch 1 and 3
> - Moved regulator handling to the core driver
> 
>  .../devicetree/bindings/input/rmi4/rmi_i2c.txt       |  6 ++++++
>  .../devicetree/bindings/input/rmi4/rmi_spi.txt       |  6 ++++++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/input/rmi4/rmi_driver.c                      | 20 ++++++++++++++++++++
>  include/linux/rmi.h                                  |  4 +++-
>  4 files changed, 35 insertions(+), 1 deletion(-)

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Andrew Duggan]

Hi Bjorn,

On 05/06/2016 09:40 PM, Bjorn Andersson wrote:
> The first version of the regulator support patch suffered from being
> implemented in the transport driver, as a work around for resource availability
> racing (EPROBE_DEFER of the core driver) with the interrupt handler.
>
> After reconsidering the solutions discussed following that I concluded that the
> interrupt management is not really part of the transport, neither conceptually
> or electrically. I therefor here suggest (patch 1/3) to move the interrupt
> registration and handling to the core rmi driver.

My concern with moving interrupt processing to the core is that not all 
transports report attn to the rmi core using an irq. The HID and SMBus 
transports which are currently in development, reside a little higher in 
the stack and attention is reported using different mechanisms. We moved 
interrupt handling to the transport drivers so that they could handle 
the differences in how attn is reported.

This message has some of the previous discussion regarding interrupt 
processing:
https://lkml.org/lkml/2015/11/28/123

Similarly, not all transports will need support for regulators. 
Implementing both in the transport drivers avoids the EPROBE_DEFER 
racing and avoids adding checks in the core to see if it needs to handle 
interrupts and manage regulators.

Thanks,
Andrew

> This solves the potential race of interrupts being delivered in the transport
> driver before the core driver have been given a chance to recover from probe
> deferral.
>
> Patch 2/3 then add the necessary code for acquiring regulator handles and
> enabling these.
>
> Patch 3/3 removes the set_page() done in the transport drivers, as we can't
> rely on the chip becoming available at any time during the initialization/probe
> phase.
>
> Bjorn Andersson (3):
>    input: rmi4: Move IRQ handling to rmi_driver
>    input: rmi4: Acquire and enable VDD and VIO supplies
>    input: rmi4: Remove set_page() call before core is initialized
>
>   .../devicetree/bindings/input/rmi4/rmi_i2c.txt     |  6 ++
>   .../devicetree/bindings/input/rmi4/rmi_spi.txt     |  6 ++
>   drivers/input/rmi4/rmi_driver.c                    | 89 +++++++++++++++++++++-
>   drivers/input/rmi4/rmi_i2c.c                       | 84 ++------------------
>   drivers/input/rmi4/rmi_spi.c                       | 83 ++------------------
>   include/linux/rmi.h                                | 11 ++-
>   6 files changed, 118 insertions(+), 161 deletions(-)
>

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Bjorn Andersson]

On Mon 09 May 17:36 PDT 2016, Andrew Duggan wrote:

> Hi Bjorn,
> 
> On 05/06/2016 09:40 PM, Bjorn Andersson wrote:
> >The first version of the regulator support patch suffered from being
> >implemented in the transport driver, as a work around for resource availability
> >racing (EPROBE_DEFER of the core driver) with the interrupt handler.
> >
> >After reconsidering the solutions discussed following that I concluded that the
> >interrupt management is not really part of the transport, neither conceptually
> >or electrically. I therefor here suggest (patch 1/3) to move the interrupt
> >registration and handling to the core rmi driver.
> 
> My concern with moving interrupt processing to the core is that not all
> transports report attn to the rmi core using an irq. The HID and SMBus
> transports which are currently in development, reside a little higher in the
> stack and attention is reported using different mechanisms. We moved
> interrupt handling to the transport drivers so that they could handle the
> differences in how attn is reported.
> 

I suspected that to be the case.

> This message has some of the previous discussion regarding interrupt
> processing:
> https://lkml.org/lkml/2015/11/28/123
> 
> Similarly, not all transports will need support for regulators. Implementing
> both in the transport drivers avoids the EPROBE_DEFER racing and avoids
> adding checks in the core to see if it needs to handle interrupts and manage
> regulators.
> 

So either we duplicate the regulator support in spi/i2c or we make them
optional in the core driver. Sounds like you prefer the prior, i.e. v1
of my patch.

Regards,
Bjorn

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Andrew Duggan]

Hi Bjorn,

On 05/10/2016 08:49 AM, Bjorn Andersson wrote:
> On Mon 09 May 17:36 PDT 2016, Andrew Duggan wrote:
>
>> Hi Bjorn,
>>
>> On 05/06/2016 09:40 PM, Bjorn Andersson wrote:
>>> The first version of the regulator support patch suffered from being
>>> implemented in the transport driver, as a work around for resource availability
>>> racing (EPROBE_DEFER of the core driver) with the interrupt handler.
>>>
>>> After reconsidering the solutions discussed following that I concluded that the
>>> interrupt management is not really part of the transport, neither conceptually
>>> or electrically. I therefor here suggest (patch 1/3) to move the interrupt
>>> registration and handling to the core rmi driver.
>> My concern with moving interrupt processing to the core is that not all
>> transports report attn to the rmi core using an irq. The HID and SMBus
>> transports which are currently in development, reside a little higher in the
>> stack and attention is reported using different mechanisms. We moved
>> interrupt handling to the transport drivers so that they could handle the
>> differences in how attn is reported.
>>
> I suspected that to be the case.
>
>> This message has some of the previous discussion regarding interrupt
>> processing:
>> https://lkml.org/lkml/2015/11/28/123
>>
>> Similarly, not all transports will need support for regulators. Implementing
>> both in the transport drivers avoids the EPROBE_DEFER racing and avoids
>> adding checks in the core to see if it needs to handle interrupts and manage
>> regulators.
>>
> So either we duplicate the regulator support in spi/i2c or we make them
> optional in the core driver. Sounds like you prefer the prior, i.e. v1
> of my patch.

Yes, after all this I think it makes sense to put regulator support in 
the spi/i2c transports like in your v1 patch. I essentially duplicated 
the irq handling code in both transports so I would be ok with 
duplicating regulator support too. It doesn't seem like that much code. 
But, if this is too much duplication we could create some sort of common 
file and put the common irq and regulator support functions which could 
be called in the transports. Similar to how rmi_2d_sensor.c defines some 
common functions shared between rmi_f11 and rmi_f12.


Thanks,
Andrew

> Regards,
> Bjorn

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Bjorn Andersson]

On Wed 11 May 16:30 PDT 2016, Andrew Duggan wrote:

> Hi Bjorn,
> 
> On 05/10/2016 08:49 AM, Bjorn Andersson wrote:
[..]
> >So either we duplicate the regulator support in spi/i2c or we make them
> >optional in the core driver. Sounds like you prefer the prior, i.e. v1
> >of my patch.
> 
> Yes, after all this I think it makes sense to put regulator support in the
> spi/i2c transports like in your v1 patch. I essentially duplicated the irq
> handling code in both transports so I would be ok with duplicating regulator
> support too. It doesn't seem like that much code. But, if this is too much
> duplication we could create some sort of common file and put the common irq
> and regulator support functions which could be called in the transports.
> Similar to how rmi_2d_sensor.c defines some common functions shared between
> rmi_f11 and rmi_f12.
> 

Sounds reasonable, I'm okay with this. Did you have any comments on the
implementation I had in v1?

@Dmitry, do you want me to resend v1?

Regards,
Bjorn

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Andrew Duggan]

On 05/11/2016 08:05 PM, Bjorn Andersson wrote:
> On Wed 11 May 16:30 PDT 2016, Andrew Duggan wrote:
>
>> Hi Bjorn,
>>
>> On 05/10/2016 08:49 AM, Bjorn Andersson wrote:
> [..]
>>> So either we duplicate the regulator support in spi/i2c or we make them
>>> optional in the core driver. Sounds like you prefer the prior, i.e. v1
>>> of my patch.
>> Yes, after all this I think it makes sense to put regulator support in the
>> spi/i2c transports like in your v1 patch. I essentially duplicated the irq
>> handling code in both transports so I would be ok with duplicating regulator
>> support too. It doesn't seem like that much code. But, if this is too much
>> duplication we could create some sort of common file and put the common irq
>> and regulator support functions which could be called in the transports.
>> Similar to how rmi_2d_sensor.c defines some common functions shared between
>> rmi_f11 and rmi_f12.
>>
> Sounds reasonable, I'm okay with this. Did you have any comments on the
> implementation I had in v1?

I tested on a device which has an always on regulators so I didn't add 
anything to device tree for the device. But, it returned 0 when it 
didn't find anything which seems to be the correct behavior. Is there an 
easy way to avoid sleeping for 10ms when there are no regulators? Maybe 
check if both the supplies .consumer pointer is null?

Andrew

> @Dmitry, do you want me to resend v1?
>
> Regards,
> Bjorn

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Bjorn Andersson]

On Thu 12 May 17:52 PDT 2016, Andrew Duggan wrote:

> On 05/11/2016 08:05 PM, Bjorn Andersson wrote:
> >On Wed 11 May 16:30 PDT 2016, Andrew Duggan wrote:
> >
> >>Hi Bjorn,
> >>
> >>On 05/10/2016 08:49 AM, Bjorn Andersson wrote:
> >[..]
> >>>So either we duplicate the regulator support in spi/i2c or we make them
> >>>optional in the core driver. Sounds like you prefer the prior, i.e. v1
> >>>of my patch.
> >>Yes, after all this I think it makes sense to put regulator support in the
> >>spi/i2c transports like in your v1 patch. I essentially duplicated the irq
> >>handling code in both transports so I would be ok with duplicating regulator
> >>support too. It doesn't seem like that much code. But, if this is too much
> >>duplication we could create some sort of common file and put the common irq
> >>and regulator support functions which could be called in the transports.
> >>Similar to how rmi_2d_sensor.c defines some common functions shared between
> >>rmi_f11 and rmi_f12.
> >>
> >Sounds reasonable, I'm okay with this. Did you have any comments on the
> >implementation I had in v1?
> 
> I tested on a device which has an always on regulators so I didn't add
> anything to device tree for the device. But, it returned 0 when it didn't
> find anything which seems to be the correct behavior. Is there an easy way
> to avoid sleeping for 10ms when there are no regulators? Maybe check if both
> the supplies .consumer pointer is null?
> 

I did look at this as well, but unfortunately the regulators does not
come back as NULL, but rather as dummy regulators.

The delay matches Tpowerup (iirc) from the data sheet, which I assume is
firmware/hardware dependant. Should we provide a knob for that and
default the sleep to 0ms?

Regards,
Bjorn

Re: [PATCH v2 0/3] input: rmi4: Regulator supply support

[Andrew Duggan]

On 05/13/2016 03:29 PM, Bjorn Andersson wrote:
> On Thu 12 May 17:52 PDT 2016, Andrew Duggan wrote:
>
>> On 05/11/2016 08:05 PM, Bjorn Andersson wrote:
>>> On Wed 11 May 16:30 PDT 2016, Andrew Duggan wrote:
>>>
>>>> Hi Bjorn,
>>>>
>>>> On 05/10/2016 08:49 AM, Bjorn Andersson wrote:
>>> [..]
>>>>> So either we duplicate the regulator support in spi/i2c or we make them
>>>>> optional in the core driver. Sounds like you prefer the prior, i.e. v1
>>>>> of my patch.
>>>> Yes, after all this I think it makes sense to put regulator support in the
>>>> spi/i2c transports like in your v1 patch. I essentially duplicated the irq
>>>> handling code in both transports so I would be ok with duplicating regulator
>>>> support too. It doesn't seem like that much code. But, if this is too much
>>>> duplication we could create some sort of common file and put the common irq
>>>> and regulator support functions which could be called in the transports.
>>>> Similar to how rmi_2d_sensor.c defines some common functions shared between
>>>> rmi_f11 and rmi_f12.
>>>>
>>> Sounds reasonable, I'm okay with this. Did you have any comments on the
>>> implementation I had in v1?
>> I tested on a device which has an always on regulators so I didn't add
>> anything to device tree for the device. But, it returned 0 when it didn't
>> find anything which seems to be the correct behavior. Is there an easy way
>> to avoid sleeping for 10ms when there are no regulators? Maybe check if both
>> the supplies .consumer pointer is null?
>>
> I did look at this as well, but unfortunately the regulators does not
> come back as NULL, but rather as dummy regulators.
>
> The delay matches Tpowerup (iirc) from the data sheet, which I assume is
> firmware/hardware dependant. Should we provide a knob for that and
> default the sleep to 0ms?

Making  the default 0 and then setting an appropriate time out for 
devices which need it sounds like a good idea to me.

Thanks,
Andrew

> Regards,
> Bjorn