| Message ID | 1473027116-13892-2-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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
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
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
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;