[2/2] usb: typec: tipd: Support wakeup
Commit Message
Enable wakeup when pluging or unpluging USB cable. It is up to other
components to hold system in active mode, such as display, so that
user can receive the notification.
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
drivers/usb/typec/tipd/core.c | 38 +++++++++++++++++++++++++++++++++++
1 file changed, 38 insertions(+)
Comments
On 05/01/2023 07:50, Jun Nie wrote:
> Enable wakeup when pluging or unpluging USB cable. It is up to other
> components to hold system in active mode, such as display, so that
> user can receive the notification.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> drivers/usb/typec/tipd/core.c | 38 +++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 46a4d8b128f0..485b90c13078 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -95,6 +95,7 @@ struct tps6598x {
> struct power_supply_desc psy_desc;
> enum power_supply_usb_type usb_type;
>
> + int wakeup;
> u16 pwr_status;
> };
>
> @@ -846,6 +847,12 @@ static int tps6598x_probe(struct i2c_client *client)
> i2c_set_clientdata(client, tps);
> fwnode_handle_put(fwnode);
>
> + tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
> + if (tps->wakeup) {
> + device_init_wakeup(&client->dev, true);
> + enable_irq_wake(client->irq);
> + }
Does the ordering of device_init_wakeup() and enable_irq_wake() matter ?
The sequence in drivers/usb/typec/tcpm/tcpci_maxim.c is
enable_irq_wake() and then device_init_wakeup() ?
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
On Thu, Jan 05, 2023 at 03:50:58PM +0800, Jun Nie wrote:
> Enable wakeup when pluging or unpluging USB cable. It is up to other
> components to hold system in active mode, such as display, so that
> user can receive the notification.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/tipd/core.c | 38 +++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 46a4d8b128f0..485b90c13078 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -95,6 +95,7 @@ struct tps6598x {
> struct power_supply_desc psy_desc;
> enum power_supply_usb_type usb_type;
>
> + int wakeup;
> u16 pwr_status;
> };
>
> @@ -846,6 +847,12 @@ static int tps6598x_probe(struct i2c_client *client)
> i2c_set_clientdata(client, tps);
> fwnode_handle_put(fwnode);
>
> + tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
> + if (tps->wakeup) {
> + device_init_wakeup(&client->dev, true);
> + enable_irq_wake(client->irq);
> + }
> +
> return 0;
>
> err_disconnect:
> @@ -870,6 +877,36 @@ static void tps6598x_remove(struct i2c_client *client)
> usb_role_switch_put(tps->role_sw);
> }
>
> +static int __maybe_unused tps6598x_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tps6598x *tps = i2c_get_clientdata(client);
> +
> + if (tps->wakeup) {
> + disable_irq(client->irq);
> + enable_irq_wake(client->irq);
> + }
> +
> + return 0;
> +}
> +
> +static int __maybe_unused tps6598x_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct tps6598x *tps = i2c_get_clientdata(client);
> +
> + if (tps->wakeup) {
> + disable_irq_wake(client->irq);
> + enable_irq(client->irq);
> + }
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops tps6598x_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(tps6598x_suspend, tps6598x_resume)
> +};
> +
> static const struct of_device_id tps6598x_of_match[] = {
> { .compatible = "ti,tps6598x", },
> { .compatible = "apple,cd321x", },
> @@ -886,6 +923,7 @@ MODULE_DEVICE_TABLE(i2c, tps6598x_id);
> static struct i2c_driver tps6598x_i2c_driver = {
> .driver = {
> .name = "tps6598x",
> + .pm = &tps6598x_pm_ops,
> .of_match_table = tps6598x_of_match,
> },
> .probe_new = tps6598x_probe,
> --
> 2.34.1
thanks,
Bryan O'Donoghue <bryan.odonoghue@linaro.org> 于2023年1月5日周四 19:05写道：
>
> On 05/01/2023 07:50, Jun Nie wrote:
> > Enable wakeup when pluging or unpluging USB cable. It is up to other
> > components to hold system in active mode, such as display, so that
> > user can receive the notification.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> > drivers/usb/typec/tipd/core.c | 38 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> > index 46a4d8b128f0..485b90c13078 100644
> > --- a/drivers/usb/typec/tipd/core.c
> > +++ b/drivers/usb/typec/tipd/core.c
> > @@ -95,6 +95,7 @@ struct tps6598x {
> > struct power_supply_desc psy_desc;
> > enum power_supply_usb_type usb_type;
> >
> > + int wakeup;
> > u16 pwr_status;
> > };
> >
> > @@ -846,6 +847,12 @@ static int tps6598x_probe(struct i2c_client *client)
> > i2c_set_clientdata(client, tps);
> > fwnode_handle_put(fwnode);
> >
> > + tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
> > + if (tps->wakeup) {
> > + device_init_wakeup(&client->dev, true);
> > + enable_irq_wake(client->irq);
> > + }
>
> Does the ordering of device_init_wakeup() and enable_irq_wake() matter ?
>
> The sequence in drivers/usb/typec/tcpm/tcpci_maxim.c is
> enable_irq_wake() and then device_init_wakeup() ?
With reading related code, I believe it is better to put device_init_wakeup()
before enable_irq_wake() logically. Though it shall not matter in real world.
device_init_wakeup() register the wakeup source and setup sysfs etc, which
is puerly software side infrastructure. enable_irq_wake() setup interrupt
configuration, including software and hardware sides. I assume there is no
suspend/resume process involves wakeup event before probe function finish
in real world. If there is, device_init_wakeup() should come before before
enable_irq_wake() strictly.
- Jun
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
@@ -95,6 +95,7 @@ struct tps6598x {
struct power_supply_desc psy_desc;
enum power_supply_usb_type usb_type;
+ int wakeup;
u16 pwr_status;
};
@@ -846,6 +847,12 @@ static int tps6598x_probe(struct i2c_client *client)
i2c_set_clientdata(client, tps);
fwnode_handle_put(fwnode);
+ tps->wakeup = device_property_read_bool(tps->dev, "wakeup-source");
+ if (tps->wakeup) {
+ device_init_wakeup(&client->dev, true);
+ enable_irq_wake(client->irq);
+ }
+
return 0;
err_disconnect:
@@ -870,6 +877,36 @@ static void tps6598x_remove(struct i2c_client *client)
usb_role_switch_put(tps->role_sw);
}
+static int __maybe_unused tps6598x_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tps6598x *tps = i2c_get_clientdata(client);
+
+ if (tps->wakeup) {
+ disable_irq(client->irq);
+ enable_irq_wake(client->irq);
+ }
+
+ return 0;
+}
+
+static int __maybe_unused tps6598x_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct tps6598x *tps = i2c_get_clientdata(client);
+
+ if (tps->wakeup) {
+ disable_irq_wake(client->irq);
+ enable_irq(client->irq);
+ }
+
+ return 0;
+}
+
+static const struct dev_pm_ops tps6598x_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(tps6598x_suspend, tps6598x_resume)
+};
+
static const struct of_device_id tps6598x_of_match[] = {
{ .compatible = "ti,tps6598x", },
{ .compatible = "apple,cd321x", },
@@ -886,6 +923,7 @@ MODULE_DEVICE_TABLE(i2c, tps6598x_id);
static struct i2c_driver tps6598x_i2c_driver = {
.driver = {
.name = "tps6598x",
+ .pm = &tps6598x_pm_ops,
.of_match_table = tps6598x_of_match,
},
.probe_new = tps6598x_probe,