All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] dt-bindings: add the regulator optional properties
@ 2016-09-04 22:11 ` Caesar Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Caesar Wang @ 2016-09-04 22:11 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: linux-rockchip, dbasehore, Douglas Anderson, Heiko Stuebner,
	Brian Norris, Caesar Wang, Rob Herring, linux-input, devicetree,
	linux-kernel, Dmitry Torokhov, Mark Rutland

Add the regulator properties that will be used to power on/off
the regulator.

Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: linux-input@vger.kernel.org
---

 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index 488edcb..e648e44 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -17,6 +17,9 @@ Required properties:
 - interrupt-parent: the phandle for the interrupt controller
 - interrupts: interrupt line
 
+Optional properties:
+- power-supply: phandle of the regulator that provides the supply voltage.
+
 Example:
 
 	i2c-hid-dev@2c {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 1/2] dt-bindings: add the regulator optional properties
@ 2016-09-04 22:11 ` Caesar Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Caesar Wang @ 2016-09-04 22:11 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Mark Rutland, devicetree-u79uwXL29TY76Z2rM5mHXA,
	dbasehore-F7+t8E8rja9g9hUCZPvPmw, Heiko Stuebner, Brian Norris,
	Douglas Anderson, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Rob Herring,
	linux-input-u79uwXL29TY76Z2rM5mHXA, Dmitry Torokhov, Caesar Wang

Add the regulator properties that will be used to power on/off
the regulator.

Signed-off-by: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---

 Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
index 488edcb..e648e44 100644
--- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
+++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
@@ -17,6 +17,9 @@ Required properties:
 - interrupt-parent: the phandle for the interrupt controller
 - interrupts: interrupt line
 
+Optional properties:
+- power-supply: phandle of the regulator that provides the supply voltage.
+
 Example:
 
 	i2c-hid-dev@2c {
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] HID: i2c-hid: support the regulator
@ 2016-09-04 22:11   ` Caesar Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Caesar Wang @ 2016-09-04 22:11 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: linux-rockchip, dbasehore, Douglas Anderson, Heiko Stuebner,
	Brian Norris, Caesar Wang, linux-input, Mika Westerberg,
	Dmitry Torokhov, Benjamin Tissoires, Benson Leung, Guohua Zhong,
	Zhonghui",
	linux-kernel

From: Brian Norris <briannorris@chromium.org>

In order to allow supporting the HID based devices that need power on/off
the regulator. We try to get a power-supply property from the
device tree.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Signed-off-by: Caesar Wang <wxt@rock-chips.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: linux-input@vger.kernel.org

---

 drivers/hid/i2c-hid/i2c-hid.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2..07cc7aa 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -38,6 +38,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/i2c/i2c-hid.h>
 
@@ -152,6 +153,7 @@ struct i2c_hid {
 
 	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
+	struct regulator	*supply;
 };
 
 static int __i2c_hid_command(struct i2c_client *client,
@@ -968,6 +970,21 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
+	ihid->supply = devm_regulator_get(&client->dev, "power");
+	if (IS_ERR(ihid->supply)) {
+		ret = PTR_ERR(ihid->supply);
+		dev_err(&client->dev, "Failed to get power regulator: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = regulator_enable(ihid->supply);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to enable power regulator: %d\n",
+			ret);
+		return ret;
+	}
+
 	if (client->dev.of_node) {
 		ret = i2c_hid_of_probe(client, &ihid->pdata);
 		if (ret)
@@ -1100,6 +1117,8 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->desc)
 		gpiod_put(ihid->desc);
 
+	regulator_disable(ihid->supply);
+
 	kfree(ihid);
 
 	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
@@ -1152,6 +1171,11 @@ static int i2c_hid_suspend(struct device *dev)
 		else
 			hid_warn(hid, "Failed to enable irq wake: %d\n",
 				wake_status);
+	} else {
+		ret = regulator_disable(ihid->supply);
+		if (ret < 0)
+			hid_warn(hid, "Failed to disable power supply: %d\n",
+				 ret);
 	}
 
 	return 0;
@@ -1165,7 +1189,12 @@ static int i2c_hid_resume(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int wake_status;
 
-	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
+	if (!device_may_wakeup(&client->dev)) {
+		ret = regulator_enable(ihid->supply);
+		if (ret < 0)
+			hid_warn(hid, "Failed to enable power supply: %d\n",
+				 ret);
+	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(ihid->irq);
 		if (!wake_status)
 			ihid->irq_wake_enabled = false;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] HID: i2c-hid: support the regulator
@ 2016-09-04 22:11   ` Caesar Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Caesar Wang @ 2016-09-04 22:11 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Guohua Zhong, dbasehore-F7+t8E8rja9g9hUCZPvPmw, Heiko Stuebner,
	Dmitry Torokhov, Brian Norris, Douglas Anderson,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Benjamin Tissoires, linux-input-u79uwXL29TY76Z2rM5mHXA,
	Benson Leung, Mika Westerberg, Zhonghui",
	Caesar Wang

From: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

In order to allow supporting the HID based devices that need power on/off
the regulator. We try to get a power-supply property from the
device tree.

Signed-off-by: Brian Norris <briannorris-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Caesar Wang <wxt-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
Cc: Jiri Kosina <jikos-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

---

 drivers/hid/i2c-hid/i2c-hid.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index b3ec4f2..07cc7aa 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -38,6 +38,7 @@
 #include <linux/acpi.h>
 #include <linux/of.h>
 #include <linux/gpio/consumer.h>
+#include <linux/regulator/consumer.h>
 
 #include <linux/i2c/i2c-hid.h>
 
@@ -152,6 +153,7 @@ struct i2c_hid {
 
 	bool			irq_wake_enabled;
 	struct mutex		reset_lock;
+	struct regulator	*supply;
 };
 
 static int __i2c_hid_command(struct i2c_client *client,
@@ -968,6 +970,21 @@ static int i2c_hid_probe(struct i2c_client *client,
 	if (!ihid)
 		return -ENOMEM;
 
+	ihid->supply = devm_regulator_get(&client->dev, "power");
+	if (IS_ERR(ihid->supply)) {
+		ret = PTR_ERR(ihid->supply);
+		dev_err(&client->dev, "Failed to get power regulator: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = regulator_enable(ihid->supply);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to enable power regulator: %d\n",
+			ret);
+		return ret;
+	}
+
 	if (client->dev.of_node) {
 		ret = i2c_hid_of_probe(client, &ihid->pdata);
 		if (ret)
@@ -1100,6 +1117,8 @@ static int i2c_hid_remove(struct i2c_client *client)
 	if (ihid->desc)
 		gpiod_put(ihid->desc);
 
+	regulator_disable(ihid->supply);
+
 	kfree(ihid);
 
 	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
@@ -1152,6 +1171,11 @@ static int i2c_hid_suspend(struct device *dev)
 		else
 			hid_warn(hid, "Failed to enable irq wake: %d\n",
 				wake_status);
+	} else {
+		ret = regulator_disable(ihid->supply);
+		if (ret < 0)
+			hid_warn(hid, "Failed to disable power supply: %d\n",
+				 ret);
 	}
 
 	return 0;
@@ -1165,7 +1189,12 @@ static int i2c_hid_resume(struct device *dev)
 	struct hid_device *hid = ihid->hid;
 	int wake_status;
 
-	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
+	if (!device_may_wakeup(&client->dev)) {
+		ret = regulator_enable(ihid->supply);
+		if (ret < 0)
+			hid_warn(hid, "Failed to enable power supply: %d\n",
+				 ret);
+	} else if (ihid->irq_wake_enabled) {
 		wake_status = disable_irq_wake(ihid->irq);
 		if (!wake_status)
 			ihid->irq_wake_enabled = false;
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] dt-bindings: add the regulator optional properties
  2016-09-04 22:11 ` Caesar Wang
  (?)
  (?)
@ 2016-09-12 16:16 ` Rob Herring
  -1 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2016-09-12 16:16 UTC (permalink / raw)
  To: Caesar Wang
  Cc: Jiri Kosina, linux-rockchip, dbasehore, Douglas Anderson,
	Heiko Stuebner, Brian Norris, linux-input, devicetree,
	linux-kernel, Dmitry Torokhov, Mark Rutland

On Mon, Sep 05, 2016 at 06:11:55AM +0800, Caesar Wang wrote:
> Add the regulator properties that will be used to power on/off
> the regulator.
> 
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: linux-input@vger.kernel.org
> ---
> 
>  Documentation/devicetree/bindings/input/hid-over-i2c.txt | 3 +++
>  1 file changed, 3 insertions(+)

I find this binding a bit questionable. The compatible should describe 
the actual device, not just the protocol it uses.

> diff --git a/Documentation/devicetree/bindings/input/hid-over-i2c.txt b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> index 488edcb..e648e44 100644
> --- a/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> +++ b/Documentation/devicetree/bindings/input/hid-over-i2c.txt
> @@ -17,6 +17,9 @@ Required properties:
>  - interrupt-parent: the phandle for the interrupt controller
>  - interrupts: interrupt line
>  
> +Optional properties:
> +- power-supply: phandle of the regulator that provides the supply voltage.

This needs to be actual supplies for devices. What if a device has 2 
supplies?

Add a device compatible string and make this property specific to that 
device, then it's fine.

Rob

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] HID: i2c-hid: support the regulator
  2016-09-04 22:11   ` Caesar Wang
  (?)
@ 2016-09-14  7:36   ` Benjamin Tissoires
  2016-09-14  7:55       ` Brian Norris
  -1 siblings, 1 reply; 10+ messages in thread
From: Benjamin Tissoires @ 2016-09-14  7:36 UTC (permalink / raw)
  To: Caesar Wang
  Cc: Jiri Kosina, linux-rockchip, dbasehore, Douglas Anderson,
	Heiko Stuebner, Brian Norris, linux-input, Mika Westerberg,
	Dmitry Torokhov, Benson Leung, Guohua Zhong, Zhonghui",
	linux-kernel

On Sep 05 2016 or thereabouts, Caesar Wang wrote:
> From: Brian Norris <briannorris@chromium.org>
> 
> In order to allow supporting the HID based devices that need power on/off
> the regulator. We try to get a power-supply property from the
> device tree.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: linux-input@vger.kernel.org
> 
> ---
> 
>  drivers/hid/i2c-hid/i2c-hid.c | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index b3ec4f2..07cc7aa 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -38,6 +38,7 @@
>  #include <linux/acpi.h>
>  #include <linux/of.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
>  
>  #include <linux/i2c/i2c-hid.h>
>  
> @@ -152,6 +153,7 @@ struct i2c_hid {
>  
>  	bool			irq_wake_enabled;
>  	struct mutex		reset_lock;
> +	struct regulator	*supply;
>  };
>  
>  static int __i2c_hid_command(struct i2c_client *client,
> @@ -968,6 +970,21 @@ static int i2c_hid_probe(struct i2c_client *client,
>  	if (!ihid)
>  		return -ENOMEM;
>  
> +	ihid->supply = devm_regulator_get(&client->dev, "power");
> +	if (IS_ERR(ihid->supply)) {

I am not familiar with regulators, but what if (like 99% of the
available i2c-hid devices) there is no regulator attached to the device?

Will the pointer be null? Will there be a dummy regulator?

It seems at first sight that you are adding a requirement on the devices
which is not part of the spec, and which will break every existing
systems but yours. Again, I might be wrong, so please provide more
information.

Cheers,
Benjamin

> +		ret = PTR_ERR(ihid->supply);
> +		dev_err(&client->dev, "Failed to get power regulator: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_enable(ihid->supply);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to enable power regulator: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	if (client->dev.of_node) {
>  		ret = i2c_hid_of_probe(client, &ihid->pdata);
>  		if (ret)
> @@ -1100,6 +1117,8 @@ static int i2c_hid_remove(struct i2c_client *client)
>  	if (ihid->desc)
>  		gpiod_put(ihid->desc);
>  
> +	regulator_disable(ihid->supply);
> +
>  	kfree(ihid);
>  
>  	acpi_dev_remove_driver_gpios(ACPI_COMPANION(&client->dev));
> @@ -1152,6 +1171,11 @@ static int i2c_hid_suspend(struct device *dev)
>  		else
>  			hid_warn(hid, "Failed to enable irq wake: %d\n",
>  				wake_status);
> +	} else {
> +		ret = regulator_disable(ihid->supply);
> +		if (ret < 0)
> +			hid_warn(hid, "Failed to disable power supply: %d\n",
> +				 ret);
>  	}
>  
>  	return 0;
> @@ -1165,7 +1189,12 @@ static int i2c_hid_resume(struct device *dev)
>  	struct hid_device *hid = ihid->hid;
>  	int wake_status;
>  
> -	if (device_may_wakeup(&client->dev) && ihid->irq_wake_enabled) {
> +	if (!device_may_wakeup(&client->dev)) {
> +		ret = regulator_enable(ihid->supply);
> +		if (ret < 0)
> +			hid_warn(hid, "Failed to enable power supply: %d\n",
> +				 ret);
> +	} else if (ihid->irq_wake_enabled) {
>  		wake_status = disable_irq_wake(ihid->irq);
>  		if (!wake_status)
>  			ihid->irq_wake_enabled = false;
> -- 
> 1.9.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] HID: i2c-hid: support the regulator
@ 2016-09-14  7:55       ` Brian Norris
  0 siblings, 0 replies; 10+ messages in thread
From: Brian Norris @ 2016-09-14  7:55 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Caesar Wang, Jiri Kosina, linux-rockchip, dbasehore,
	Douglas Anderson, Heiko Stuebner, linux-input, Mika Westerberg,
	Dmitry Torokhov, Benson Leung, Guohua Zhong, Zhonghui",
	linux-kernel

Hi Benjamin,

On Wed, Sep 14, 2016 at 09:36:03AM +0200, Benjamin Tissoires wrote:
> On Sep 05 2016 or thereabouts, Caesar Wang wrote:
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index b3ec4f2..07cc7aa 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/of.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > @@ -152,6 +153,7 @@ struct i2c_hid {
> >  
> >  	bool			irq_wake_enabled;
> >  	struct mutex		reset_lock;
> > +	struct regulator	*supply;
> >  };
> >  
> >  static int __i2c_hid_command(struct i2c_client *client,
> > @@ -968,6 +970,21 @@ static int i2c_hid_probe(struct i2c_client *client,
> >  	if (!ihid)
> >  		return -ENOMEM;
> >  
> > +	ihid->supply = devm_regulator_get(&client->dev, "power");
> > +	if (IS_ERR(ihid->supply)) {
> 
> I am not familiar with regulators, but what if (like 99% of the
> available i2c-hid devices) there is no regulator attached to the device?
> 
> Will the pointer be null? Will there be a dummy regulator?
> 
> It seems at first sight that you are adding a requirement on the devices
> which is not part of the spec, and which will break every existing
> systems but yours. Again, I might be wrong, so please provide more
> information.

The default behavior of regulator_get() is to provide a dummy regulator
if none is found. So the pointer is never NULL, and it won't break
devices without a regulator. If you don't want a dummy regulator you
would use regulator_get_optional() instead, and you would then need to
handle ERR_PTR(-ENODEV) specifically.

Brian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] HID: i2c-hid: support the regulator
@ 2016-09-14  7:55       ` Brian Norris
  0 siblings, 0 replies; 10+ messages in thread
From: Brian Norris @ 2016-09-14  7:55 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Jiri Kosina, Heiko Stuebner, Dmitry Torokhov,
	dbasehore-F7+t8E8rja9g9hUCZPvPmw, Douglas Anderson,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Guohua Zhong,
	linux-input-u79uwXL29TY76Z2rM5mHXA, Benson Leung,
	Mika Westerberg, Zhonghui",
	Caesar Wang

Hi Benjamin,

On Wed, Sep 14, 2016 at 09:36:03AM +0200, Benjamin Tissoires wrote:
> On Sep 05 2016 or thereabouts, Caesar Wang wrote:
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index b3ec4f2..07cc7aa 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/acpi.h>
> >  #include <linux/of.h>
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/regulator/consumer.h>
> >  
> >  #include <linux/i2c/i2c-hid.h>
> >  
> > @@ -152,6 +153,7 @@ struct i2c_hid {
> >  
> >  	bool			irq_wake_enabled;
> >  	struct mutex		reset_lock;
> > +	struct regulator	*supply;
> >  };
> >  
> >  static int __i2c_hid_command(struct i2c_client *client,
> > @@ -968,6 +970,21 @@ static int i2c_hid_probe(struct i2c_client *client,
> >  	if (!ihid)
> >  		return -ENOMEM;
> >  
> > +	ihid->supply = devm_regulator_get(&client->dev, "power");
> > +	if (IS_ERR(ihid->supply)) {
> 
> I am not familiar with regulators, but what if (like 99% of the
> available i2c-hid devices) there is no regulator attached to the device?
> 
> Will the pointer be null? Will there be a dummy regulator?
> 
> It seems at first sight that you are adding a requirement on the devices
> which is not part of the spec, and which will break every existing
> systems but yours. Again, I might be wrong, so please provide more
> information.

The default behavior of regulator_get() is to provide a dummy regulator
if none is found. So the pointer is never NULL, and it won't break
devices without a regulator. If you don't want a dummy regulator you
would use regulator_get_optional() instead, and you would then need to
handle ERR_PTR(-ENODEV) specifically.

Brian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] HID: i2c-hid: support the regulator
  2016-09-14  7:55       ` Brian Norris
  (?)
@ 2016-09-14  8:02       ` Brian Norris
  2016-09-14 14:31         ` Benjamin Tissoires
  -1 siblings, 1 reply; 10+ messages in thread
From: Brian Norris @ 2016-09-14  8:02 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Caesar Wang, Jiri Kosina, linux-rockchip, dbasehore,
	Douglas Anderson, Heiko Stuebner, linux-input, Mika Westerberg,
	Dmitry Torokhov, Benson Leung, Guohua Zhong, Zhonghui",
	linux-kernel

On Wed, Sep 14, 2016 at 03:55:05PM +0800, Brian Norris wrote:
> The default behavior of regulator_get() is to provide a dummy regulator
> if none is found. So the pointer is never NULL, and it won't break
> devices without a regulator. If you don't want a dummy regulator you
> would use regulator_get_optional() instead, and you would then need to
> handle ERR_PTR(-ENODEV) specifically.

One caveat to the never-NULL comment above that I just noticed:

If CONFIG_REGULATOR=n, then regulator_get() actually returns NULL (see
include/linux/regulator/consumer.h), but it also specifically has a
comment right next to that NULL return, saying:

        /* Nothing except the stubbed out regulator API should be
         * looking at the value except to check if it is an error
         * value. Drivers are free to handle NULL specifically by
         * skipping all regulator API calls, but they don't have to.
         * Drivers which don't, should make sure they properly handle
         * corner cases of the API, such as regulator_get_voltage()
         * returning 0.
         */

So, we still don't need to handle the NULL case specially.

Brian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] HID: i2c-hid: support the regulator
  2016-09-14  8:02       ` Brian Norris
@ 2016-09-14 14:31         ` Benjamin Tissoires
  0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Tissoires @ 2016-09-14 14:31 UTC (permalink / raw)
  To: Brian Norris
  Cc: Caesar Wang, Jiri Kosina, linux-rockchip, dbasehore,
	Douglas Anderson, Heiko Stuebner, linux-input, Mika Westerberg,
	Dmitry Torokhov, Benson Leung, Guohua Zhong, Zhonghui",
	linux-kernel

On Sep 14 2016 or thereabouts, Brian Norris wrote:
> On Wed, Sep 14, 2016 at 03:55:05PM +0800, Brian Norris wrote:
> > The default behavior of regulator_get() is to provide a dummy regulator
> > if none is found. So the pointer is never NULL, and it won't break
> > devices without a regulator. If you don't want a dummy regulator you
> > would use regulator_get_optional() instead, and you would then need to
> > handle ERR_PTR(-ENODEV) specifically.
> 
> One caveat to the never-NULL comment above that I just noticed:
> 
> If CONFIG_REGULATOR=n, then regulator_get() actually returns NULL (see
> include/linux/regulator/consumer.h), but it also specifically has a
> comment right next to that NULL return, saying:
> 
>         /* Nothing except the stubbed out regulator API should be
>          * looking at the value except to check if it is an error
>          * value. Drivers are free to handle NULL specifically by
>          * skipping all regulator API calls, but they don't have to.
>          * Drivers which don't, should make sure they properly handle
>          * corner cases of the API, such as regulator_get_voltage()
>          * returning 0.
>          */
> 
> So, we still don't need to handle the NULL case specially.

Well, all the other regulator calls are either regulator_enable() or
regulator_disable(), which in this case (CONFIG_REGULATOR=n) are
returning 0.

So I think the whole patch is safe in its current form. Thanks for the
explanations.

Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 
> Brian

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-09-14 14:31 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-04 22:11 [PATCH 1/2] dt-bindings: add the regulator optional properties Caesar Wang
2016-09-04 22:11 ` Caesar Wang
2016-09-04 22:11 ` [PATCH 2/2] HID: i2c-hid: support the regulator Caesar Wang
2016-09-04 22:11   ` Caesar Wang
2016-09-14  7:36   ` Benjamin Tissoires
2016-09-14  7:55     ` Brian Norris
2016-09-14  7:55       ` Brian Norris
2016-09-14  8:02       ` Brian Norris
2016-09-14 14:31         ` Benjamin Tissoires
2016-09-12 16:16 ` [PATCH 1/2] dt-bindings: add the regulator optional properties Rob Herring

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.