[PATCH 1/2] dt-bindings: usb: tps6598x: Add wakeup property

[PATCH 1/2] dt-bindings: usb: tps6598x: Add wakeup property

[Jun Nie]

Add wakeup property description. People can enable it with adding
the property.

Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
 Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
index fef4acdc4773..348a715d61f4 100644
--- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
@@ -23,6 +23,8 @@ properties:
   reg:
     maxItems: 1
 
+  wakeup-source: true
+
   interrupts:
     maxItems: 1
 
@@ -48,6 +50,7 @@ examples:
         tps6598x: tps6598x@38 {
             compatible = "ti,tps6598x";
             reg = <0x38>;
+            wakeup-source;
 
             interrupt-parent = <&msmgpio>;
             interrupts = <107 IRQ_TYPE_LEVEL_LOW>;
-- 
2.34.1

[PATCH 2/2] usb: typec: tipd: Support wakeup

[Jun Nie]

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);
+	}
+
 	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

Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Add wakeup property

[Bryan O'Donoghue]

On 05/01/2023 07:50, Jun Nie wrote:
> Add wakeup property description. People can enable it with adding
> the property.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>   Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> index fef4acdc4773..348a715d61f4 100644
> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
> @@ -23,6 +23,8 @@ properties:
>     reg:
>       maxItems: 1
>   
> +  wakeup-source: true
> +
>     interrupts:
>       maxItems: 1
>   
> @@ -48,6 +50,7 @@ examples:
>           tps6598x: tps6598x@38 {
>               compatible = "ti,tps6598x";
>               reg = <0x38>;
> +            wakeup-source;
>   
>               interrupt-parent = <&msmgpio>;
>               interrupts = <107 IRQ_TYPE_LEVEL_LOW>;

LGTM

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Re: [PATCH 2/2] usb: typec: tipd: Support wakeup

[Bryan O'Donoghue]

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>

Re: [PATCH 2/2] usb: typec: tipd: Support wakeup

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH 2/2] usb: typec: tipd: Support wakeup

[Jun Nie]

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>

Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Add wakeup property

[Krzysztof Kozlowski]

On 05/01/2023 08:50, Jun Nie wrote:
> Add wakeup property description. People can enable it with adding
> the property.
> 
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 3 +++


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof