[PATCH 0/7] TPS6598x PD tracing and other improvements

[PATCH 0/7] TPS6598x PD tracing and other improvements

[Sebastian Krzyszkowiak]

This is a series developed for the Librem 5 phone, which uses TPS65982
as its USB-C controller. Implemented are Power Delivery sink contract
tracing and exporting negotiated power values as power supply properties,
fixes for data role swapping, status register caching and a debugfs entry
for querying customer use word of the firmware running on the controller.

Angus Ainslie (3):
  usb: typec: tipd: set the data role on tps IRQ
  usb: typec: tipd: Add trace event for SINK PD contract
  usb: typec: tipd: Provide POWER_SUPPLY_PROP_{CURRENT,VOLTAGE}_MAX

Guido Günther (2):
  usb: typec: tipd: Only update power status on IRQ
  usb: typec: tipd: Add debugfs entries for customer use word

Sebastian Krzyszkowiak (2):
  usb: typec: tipd: Provide POWER_SUPPLY_PROP_PRESENT
  usb: typec: tipd: Fail probe when the controller is in BOOT mode

 drivers/usb/typec/tipd/core.c     | 263 ++++++++++++++++++++++++++----
 drivers/usb/typec/tipd/tps6598x.h |  30 ++++
 drivers/usb/typec/tipd/trace.h    |  38 +++++
 3 files changed, 302 insertions(+), 29 deletions(-)

-- 
2.35.1

[PATCH 1/7] usb: typec: tipd: Only update power status on IRQ

[Sebastian Krzyszkowiak]

From: Guido Günther <agx@sigxcpu.org>

Instead of refetching power status cache it and only update it when a
change is signalled via irq. This simplifies tracing and adding more
supply properties in follow up patches.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 16b4560216ba..dfbba5ae9487 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -93,6 +93,8 @@ struct tps6598x {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	enum power_supply_usb_type usb_type;
+
+	u16 pwr_status;
 };
 
 static enum power_supply_property tps6598x_psy_props[] = {
@@ -230,17 +232,12 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
 {
 	struct typec_partner_desc desc;
 	enum typec_pwr_opmode mode;
-	u16 pwr_status;
 	int ret;
 
 	if (tps->partner)
 		return 0;
 
-	ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
-	if (ret < 0)
-		return ret;
-
-	mode = TPS_POWER_STATUS_PWROPMODE(pwr_status);
+	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
 
 	desc.usb_pd = mode == TYPEC_PWR_MODE_PD;
 	desc.accessory = TYPEC_ACCESSORY_NONE; /* XXX: handle accessories */
@@ -455,6 +452,7 @@ static bool tps6598x_read_power_status(struct tps6598x *tps)
 		dev_err(tps->dev, "failed to read power status: %d\n", ret);
 		return false;
 	}
+	tps->pwr_status = pwr_status;
 	trace_tps6598x_power_status(pwr_status);
 
 	return true;
@@ -601,15 +599,8 @@ static const struct regmap_config tps6598x_regmap_config = {
 static int tps6598x_psy_get_online(struct tps6598x *tps,
 				   union power_supply_propval *val)
 {
-	int ret;
-	u16 pwr_status;
-
-	ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
-	if (ret < 0)
-		return ret;
-
-	if (TPS_POWER_STATUS_CONNECTION(pwr_status) &&
-	    TPS_POWER_STATUS_SOURCESINK(pwr_status)) {
+	if (TPS_POWER_STATUS_CONNECTION(tps->pwr_status) &&
+	    TPS_POWER_STATUS_SOURCESINK(tps->pwr_status)) {
 		val->intval = 1;
 	} else {
 		val->intval = 0;
@@ -622,15 +613,11 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
 				 union power_supply_propval *val)
 {
 	struct tps6598x *tps = power_supply_get_drvdata(psy);
-	u16 pwr_status;
 	int ret = 0;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_USB_TYPE:
-		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
-		if (ret < 0)
-			return ret;
-		if (TPS_POWER_STATUS_PWROPMODE(pwr_status) == TYPEC_PWR_MODE_PD)
+		if (TPS_POWER_STATUS_PWROPMODE(tps->pwr_status) == TYPEC_PWR_MODE_PD)
 			val->intval = POWER_SUPPLY_USB_TYPE_PD;
 		else
 			val->intval = POWER_SUPPLY_USB_TYPE_C;
@@ -837,6 +824,11 @@ static int tps6598x_probe(struct i2c_client *client)
 	fwnode_handle_put(fwnode);
 
 	if (status & TPS_STATUS_PLUG_PRESENT) {
+		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
+		if (ret < 0) {
+			dev_err(tps->dev, "failed to read power status: %d\n", ret);
+			goto err_role_put;
+		}
 		ret = tps6598x_connect(tps, status);
 		if (ret)
 			dev_err(&client->dev, "failed to register partner\n");
-- 
2.35.1

Re: [PATCH 1/7] usb: typec: tipd: Only update power status on IRQ

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:12PM +0100, Sebastian Krzyszkowiak wrote:
> From: Guido Günther <agx@sigxcpu.org>
> 
> Instead of refetching power status cache it and only update it when a
> change is signalled via irq. This simplifies tracing and adding more
> supply properties in follow up patches.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 16b4560216ba..dfbba5ae9487 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -93,6 +93,8 @@ struct tps6598x {
>  	struct power_supply *psy;
>  	struct power_supply_desc psy_desc;
>  	enum power_supply_usb_type usb_type;
> +
> +	u16 pwr_status;
>  };
>  
>  static enum power_supply_property tps6598x_psy_props[] = {
> @@ -230,17 +232,12 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
>  {
>  	struct typec_partner_desc desc;
>  	enum typec_pwr_opmode mode;
> -	u16 pwr_status;
>  	int ret;
>  
>  	if (tps->partner)
>  		return 0;
>  
> -	ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
> -	if (ret < 0)
> -		return ret;
> -
> -	mode = TPS_POWER_STATUS_PWROPMODE(pwr_status);
> +	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
>  
>  	desc.usb_pd = mode == TYPEC_PWR_MODE_PD;
>  	desc.accessory = TYPEC_ACCESSORY_NONE; /* XXX: handle accessories */
> @@ -455,6 +452,7 @@ static bool tps6598x_read_power_status(struct tps6598x *tps)
>  		dev_err(tps->dev, "failed to read power status: %d\n", ret);
>  		return false;
>  	}
> +	tps->pwr_status = pwr_status;
>  	trace_tps6598x_power_status(pwr_status);
>  
>  	return true;
> @@ -601,15 +599,8 @@ static const struct regmap_config tps6598x_regmap_config = {
>  static int tps6598x_psy_get_online(struct tps6598x *tps,
>  				   union power_supply_propval *val)
>  {
> -	int ret;
> -	u16 pwr_status;
> -
> -	ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
> -	if (ret < 0)
> -		return ret;
> -
> -	if (TPS_POWER_STATUS_CONNECTION(pwr_status) &&
> -	    TPS_POWER_STATUS_SOURCESINK(pwr_status)) {
> +	if (TPS_POWER_STATUS_CONNECTION(tps->pwr_status) &&
> +	    TPS_POWER_STATUS_SOURCESINK(tps->pwr_status)) {
>  		val->intval = 1;
>  	} else {
>  		val->intval = 0;
> @@ -622,15 +613,11 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
>  				 union power_supply_propval *val)
>  {
>  	struct tps6598x *tps = power_supply_get_drvdata(psy);
> -	u16 pwr_status;
>  	int ret = 0;
>  
>  	switch (psp) {
>  	case POWER_SUPPLY_PROP_USB_TYPE:
> -		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &pwr_status);
> -		if (ret < 0)
> -			return ret;
> -		if (TPS_POWER_STATUS_PWROPMODE(pwr_status) == TYPEC_PWR_MODE_PD)
> +		if (TPS_POWER_STATUS_PWROPMODE(tps->pwr_status) == TYPEC_PWR_MODE_PD)
>  			val->intval = POWER_SUPPLY_USB_TYPE_PD;
>  		else
>  			val->intval = POWER_SUPPLY_USB_TYPE_C;
> @@ -837,6 +824,11 @@ static int tps6598x_probe(struct i2c_client *client)
>  	fwnode_handle_put(fwnode);
>  
>  	if (status & TPS_STATUS_PLUG_PRESENT) {
> +		ret = tps6598x_read16(tps, TPS_REG_POWER_STATUS, &tps->pwr_status);
> +		if (ret < 0) {
> +			dev_err(tps->dev, "failed to read power status: %d\n", ret);
> +			goto err_role_put;
> +		}
>  		ret = tps6598x_connect(tps, status);
>  		if (ret)
>  			dev_err(&client->dev, "failed to register partner\n");
> -- 
> 2.35.1

-- 
heikki

[PATCH 2/7] usb: typec: tipd: set the data role on tps IRQ

[Sebastian Krzyszkowiak]

From: Angus Ainslie <angus@akkea.ca>

Don't immediately set the data role, only set it in response to the
negotiated value notification from the tps6589x. Otherwise data role
switch fails for DRP.

We only use values cached from the IRQ instead of poking I2C all
the time.

The update is moved in a function that will become more useful in later
commits.

Fixes: 18a6c866bb19 ("usb: typec: tps6598x: Add USB role switching logic")
Signed-off-by: Angus Ainslie <angus@akkea.ca>
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index dfbba5ae9487..f387786ff95e 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -94,6 +94,7 @@ struct tps6598x {
 	struct power_supply_desc psy_desc;
 	enum power_supply_usb_type usb_type;
 
+	u32 data_status;
 	u16 pwr_status;
 };
 
@@ -271,6 +272,15 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
 	return 0;
 }
 
+static int
+tps6598x_update_data_status(struct tps6598x *tps, u32 status)
+{
+	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status),
+			       !!(tps->data_status & TPS_DATA_STATUS_DATA_CONNECTION));
+	trace_tps6598x_data_status(tps->data_status);
+	return 0;
+}
+
 static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 {
 	if (!IS_ERR(tps->partner))
@@ -370,8 +380,6 @@ static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
 		goto out_unlock;
 	}
 
-	tps6598x_set_data_role(tps, role, true);
-
 out_unlock:
 	mutex_unlock(&tps->lock);
 
@@ -437,6 +445,7 @@ static bool tps6598x_read_data_status(struct tps6598x *tps)
 		dev_err(tps->dev, "failed to read data status: %d\n", ret);
 		return false;
 	}
+	tps->data_status = data_status;
 	trace_tps6598x_data_status(data_status);
 
 	return true;
@@ -497,10 +506,13 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
 		if (!tps6598x_read_power_status(tps))
 			goto err_clear_ints;
 
-	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
+	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE) {
 		if (!tps6598x_read_data_status(tps))
 			goto err_clear_ints;
 
+		tps6598x_update_data_status(tps, status);
+	}
+
 	/* Handle plug insert or removal */
 	if (event & APPLE_CD_REG_INT_PLUG_EVENT)
 		tps6598x_handle_plug_event(tps, status);
@@ -544,10 +556,13 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		if (!tps6598x_read_power_status(tps))
 			goto err_clear_ints;
 
-	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE)
+	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
 		if (!tps6598x_read_data_status(tps))
 			goto err_clear_ints;
 
+		tps6598x_update_data_status(tps, status);
+	}
+
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
 		tps6598x_handle_plug_event(tps, status);
-- 
2.35.1

Re: [PATCH 2/7] usb: typec: tipd: set the data role on tps IRQ

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:13PM +0100, Sebastian Krzyszkowiak wrote:
> From: Angus Ainslie <angus@akkea.ca>
> 
> Don't immediately set the data role, only set it in response to the
> negotiated value notification from the tps6589x. Otherwise data role
> switch fails for DRP.
> 
> We only use values cached from the IRQ instead of poking I2C all
> the time.
> 
> The update is moved in a function that will become more useful in later
> commits.
> 
> Fixes: 18a6c866bb19 ("usb: typec: tps6598x: Add USB role switching logic")
> Signed-off-by: Angus Ainslie <angus@akkea.ca>
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>

Fixes tag but but no Cc: stable... tag?

Is there some reason why you don't have the stable tag, i.e. why
shouldn't this be added to the stable kernels?

> ---
>  drivers/usb/typec/tipd/core.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index dfbba5ae9487..f387786ff95e 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -94,6 +94,7 @@ struct tps6598x {
>  	struct power_supply_desc psy_desc;
>  	enum power_supply_usb_type usb_type;
>  
> +	u32 data_status;
>  	u16 pwr_status;
>  };
>  
> @@ -271,6 +272,15 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
>  	return 0;
>  }
>  
> +static int
> +tps6598x_update_data_status(struct tps6598x *tps, u32 status)
> +{
> +	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status),
> +			       !!(tps->data_status & TPS_DATA_STATUS_DATA_CONNECTION));
> +	trace_tps6598x_data_status(tps->data_status);
> +	return 0;
> +}
> +
>  static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
>  {
>  	if (!IS_ERR(tps->partner))
> @@ -370,8 +380,6 @@ static int tps6598x_dr_set(struct typec_port *port, enum typec_data_role role)
>  		goto out_unlock;
>  	}
>  
> -	tps6598x_set_data_role(tps, role, true);
> -
>  out_unlock:
>  	mutex_unlock(&tps->lock);
>  
> @@ -437,6 +445,7 @@ static bool tps6598x_read_data_status(struct tps6598x *tps)
>  		dev_err(tps->dev, "failed to read data status: %d\n", ret);
>  		return false;
>  	}
> +	tps->data_status = data_status;
>  	trace_tps6598x_data_status(data_status);
>  
>  	return true;
> @@ -497,10 +506,13 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  		if (!tps6598x_read_power_status(tps))
>  			goto err_clear_ints;
>  
> -	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE)
> +	if (event & APPLE_CD_REG_INT_DATA_STATUS_UPDATE) {
>  		if (!tps6598x_read_data_status(tps))
>  			goto err_clear_ints;
>  
> +		tps6598x_update_data_status(tps, status);
> +	}
> +
>  	/* Handle plug insert or removal */
>  	if (event & APPLE_CD_REG_INT_PLUG_EVENT)
>  		tps6598x_handle_plug_event(tps, status);
> @@ -544,10 +556,13 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		if (!tps6598x_read_power_status(tps))
>  			goto err_clear_ints;
>  
> -	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE)
> +	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
>  		if (!tps6598x_read_data_status(tps))
>  			goto err_clear_ints;
>  
> +		tps6598x_update_data_status(tps, status);
> +	}
> +
>  	/* Handle plug insert or removal */
>  	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
>  		tps6598x_handle_plug_event(tps, status);
> -- 
> 2.35.1

-- 
heikki

[PATCH 3/7] usb: typec: tipd: Add trace event for SINK PD contract

[Sebastian Krzyszkowiak]

From: Angus Ainslie <angus@akkea.ca>

This allows to trace the negotiated contract as sink. It's
only updated when the contract is actually established.

Co-developed-by: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Angus Ainslie <angus@akkea.ca>
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c     | 63 ++++++++++++++++++++++++++++++-
 drivers/usb/typec/tipd/tps6598x.h | 30 +++++++++++++++
 drivers/usb/typec/tipd/trace.h    | 38 +++++++++++++++++++
 3 files changed, 129 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index f387786ff95e..80b4a9870caf 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -34,6 +34,7 @@
 #define TPS_REG_STATUS			0x1a
 #define TPS_REG_SYSTEM_CONF		0x28
 #define TPS_REG_CTRL_CONF		0x29
+#define TPS_REG_ACTIVE_CONTRACT		0x34
 #define TPS_REG_POWER_STATUS		0x3f
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 #define TPS_REG_DATA_STATUS		0x5f
@@ -93,6 +94,7 @@ struct tps6598x {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	enum power_supply_usb_type usb_type;
+	struct tps6598x_pdo terms;
 
 	u32 data_status;
 	u16 pwr_status;
@@ -528,6 +530,47 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
 	return IRQ_NONE;
 }
 
+static int tps6598x_get_active_pd_contract(struct tps6598x *tps)
+{
+	u64 contract;
+	int type;
+	int max_power;
+	int ret = 0;
+
+	ret = tps6598x_block_read(tps, TPS_REG_ACTIVE_CONTRACT, &contract, 6);
+	if (ret)
+		return ret;
+
+	contract &= 0xFFFFFFFFFFFF;
+	type = TPS_PDO_CONTRACT_TYPE(contract);
+	memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
+
+	/* If there's no PD it decodes to all 0 */
+	switch (type) {
+	case TPS_PDO_CONTRACT_FIXED:
+		tps->terms.max_voltage = TPS_PDO_FIXED_CONTRACT_VOLTAGE(contract);
+		tps->terms.max_current = TPS_PDO_FIXED_CONTRACT_MAX_CURRENT(contract);
+		break;
+	case TPS_PDO_CONTRACT_BATTERY:
+		tps->terms.max_voltage = TPS_PDO_BAT_CONTRACT_MAX_VOLTAGE(contract);
+		max_power = TPS_PDO_BAT_CONTRACT_MAX_POWER(contract);
+		tps->terms.max_current = 1000 * 1000 * max_power / tps->terms.max_voltage;
+		break;
+	case TPS_PDO_CONTRACT_VARIABLE:
+		tps->terms.max_voltage = TPS_PDO_VAR_CONTRACT_MAX_VOLTAGE(contract);
+		tps->terms.max_current = TPS_PDO_VAR_CONTRACT_MAX_CURRENT(contract);
+		break;
+	default:
+		dev_warn(tps->dev, "Unknown contract type: %d\n", type);
+		return -EINVAL;
+	}
+
+	tps->terms.pdo = contract;
+	trace_tps6598x_pdo(&tps->terms);
+
+	return 0;
+}
+
 static irqreturn_t tps6598x_interrupt(int irq, void *data)
 {
 	struct tps6598x *tps = data;
@@ -563,6 +606,14 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 		tps6598x_update_data_status(tps, status);
 	}
 
+	if ((event1 | event2) & TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER) {
+		ret = tps6598x_get_active_pd_contract(tps);
+		if (ret) {
+			dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
+			goto err_clear_ints;
+		}
+	}
+
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
 		tps6598x_handle_plug_event(tps, status);
@@ -751,10 +802,11 @@ static int tps6598x_probe(struct i2c_client *client)
 
 		irq_handler = cd321x_interrupt;
 	} else {
-		/* Enable power status, data status and plug event interrupts */
+		/* Enable interrupts used by this driver */
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
 			TPS_REG_INT_DATA_STATUS_UPDATE |
-			TPS_REG_INT_PLUG_EVENT;
+			TPS_REG_INT_PLUG_EVENT |
+			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER;
 	}
 
 	/* Make sure the controller has application firmware running */
@@ -847,6 +899,13 @@ static int tps6598x_probe(struct i2c_client *client)
 		ret = tps6598x_connect(tps, status);
 		if (ret)
 			dev_err(&client->dev, "failed to register partner\n");
+
+		if ((TPS_POWER_STATUS_PWROPMODE(tps->pwr_status) == TYPEC_PWR_MODE_PD) &&
+		    (!(status & TPS_STATUS_PORTROLE))) {
+			ret = tps6598x_get_active_pd_contract(tps);
+			if (ret)
+				dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
+		}
 	}
 
 	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
index 527857549d69..546cd4881f1f 100644
--- a/drivers/usb/typec/tipd/tps6598x.h
+++ b/drivers/usb/typec/tipd/tps6598x.h
@@ -199,4 +199,34 @@
 #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
 #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
 
+
+/* PDO decoding as in https://www.ti.com/lit/an/slva842/slva842.pdf */
+#define TPS_PDO_CONTRACT_TYPE(x)	FIELD_GET(GENMASK(31, 30), x)
+#define TPS_PDO_CONTRACT_FIXED	0
+#define TPS_PDO_CONTRACT_BATTERY	1
+#define TPS_PDO_CONTRACT_VARIABLE	2
+
+#define TPS_PDO_FIXED_CONTRACT_DETAILS		GENMASK(29, 25)
+#define TPS_PDO_FIXED_CONTRACT_DR_POWER		BIT(29)
+#define TPS_PDO_FIXED_CONTRACT_USB_SUSPEND	BIT(28)
+#define TPS_PDO_FIXED_CONTRACT_EXTERNAL_PWR	BIT(27)
+#define TPS_PDO_FIXED_CONTRACT_USB_COMMS	BIT(26)
+#define TPS_PDO_FIXED_CONTRACT_DR_DATA		BIT(25)
+
+#define TPS_PDO_FIXED_CONTRACT_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
+#define TPS_PDO_FIXED_CONTRACT_MAX_CURRENT(x)	(FIELD_GET(GENMASK(9, 0), x) * 10000)
+#define TPS_PDO_VAR_CONTRACT_MAX_VOLTAGE(x)	(FIELD_GET(GENMASK(29, 20), x) * 50000)
+#define TPS_PDO_VAR_CONTRACT_MIN_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
+#define TPS_PDO_VAR_CONTRACT_MAX_CURRENT(x)	(FIELD_GET(GENMASK(9, 0), x) * 10000)
+#define TPS_PDO_BAT_CONTRACT_MAX_VOLTAGE(x)	(FIELD_GET(GENMASK(29, 20), x) * 50000)
+#define TPS_PDO_BAT_CONTRACT_MIN_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
+#define TPS_PDO_BAT_CONTRACT_MAX_POWER(x)	(FIELD_GET(GENMASK(9, 0), x) * 250000)
+
+struct tps6598x_pdo {
+	u32 pdo;
+	int max_voltage; /* uV */
+	int max_current; /* uA */
+	int max_power;   /* uW */
+};
+
 #endif /* __TPS6598X_H__ */
diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
index 12cad1bde7cc..148e2ef3157b 100644
--- a/drivers/usb/typec/tipd/trace.h
+++ b/drivers/usb/typec/tipd/trace.h
@@ -194,6 +194,20 @@
 	(data_status & TPS_DATA_STATUS_DP_CONNECTION ? \
 	 show_data_status_dp_pin_assignment(data_status) : "")
 
+#define show_pdo_contract_type(pdo) \
+	__print_symbolic(TPS_PDO_CONTRACT_TYPE(pdo), \
+		{ TPS_PDO_CONTRACT_FIXED, "fixed" }, \
+		{ TPS_PDO_CONTRACT_BATTERY, "battery" }, \
+		{ TPS_PDO_CONTRACT_VARIABLE, "variable" })
+
+#define show_pdo_contract_details(pdo) \
+	__print_flags(pdo & TPS_PDO_FIXED_CONTRACT_DETAILS, "|", \
+		{ TPS_PDO_FIXED_CONTRACT_DR_POWER, "dr-power" }, \
+		{ TPS_PDO_FIXED_CONTRACT_USB_SUSPEND, "usb-suspend" }, \
+		{ TPS_PDO_FIXED_CONTRACT_EXTERNAL_PWR, "ext-power" }, \
+		{ TPS_PDO_FIXED_CONTRACT_USB_COMMS, "usb-comms" }, \
+		{ TPS_PDO_FIXED_CONTRACT_DR_DATA, "dr-data" })
+
 TRACE_EVENT(tps6598x_irq,
 	    TP_PROTO(u64 event1,
 		     u64 event2),
@@ -296,6 +310,30 @@ TRACE_EVENT(tps6598x_data_status,
 		    )
 );
 
+TRACE_EVENT(tps6598x_pdo,
+	    TP_PROTO(struct tps6598x_pdo *pdo),
+	    TP_ARGS(pdo),
+
+	    TP_STRUCT__entry(
+			     __field(u32, pdo)
+			     __field(int, max_current)
+			     __field(int, max_voltage)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->pdo = pdo->pdo;
+			   __entry->max_current = pdo->max_current;
+			   __entry->max_voltage = pdo->max_voltage;
+			   ),
+
+	    TP_printk("%s supply, max %duA, %duV, details: %s",
+		      show_pdo_contract_type(__entry->pdo),
+		      __entry->max_current,
+		      __entry->max_voltage,
+		      show_pdo_contract_details(__entry->pdo)
+		    )
+);
+
 #endif /* _TPS6598X_TRACE_H_ */
 
 /* This part must be outside protection */
-- 
2.35.1

Re: [PATCH 3/7] usb: typec: tipd: Add trace event for SINK PD contract

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:14PM +0100, Sebastian Krzyszkowiak wrote:
> From: Angus Ainslie <angus@akkea.ca>
> 
> This allows to trace the negotiated contract as sink. It's
> only updated when the contract is actually established.

With the PDOs (and also RDO) I think we'll rely on the sysfs interface
that I mentioned once it's ready, but since this is only trace stuff, I
guess it's okay to take this in the mean time.

I in any case noticed one issue below...

> Co-developed-by: Guido Günther <agx@sigxcpu.org>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Signed-off-by: Angus Ainslie <angus@akkea.ca>
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/usb/typec/tipd/core.c     | 63 ++++++++++++++++++++++++++++++-
>  drivers/usb/typec/tipd/tps6598x.h | 30 +++++++++++++++
>  drivers/usb/typec/tipd/trace.h    | 38 +++++++++++++++++++
>  3 files changed, 129 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index f387786ff95e..80b4a9870caf 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -34,6 +34,7 @@
>  #define TPS_REG_STATUS			0x1a
>  #define TPS_REG_SYSTEM_CONF		0x28
>  #define TPS_REG_CTRL_CONF		0x29
> +#define TPS_REG_ACTIVE_CONTRACT		0x34
>  #define TPS_REG_POWER_STATUS		0x3f
>  #define TPS_REG_RX_IDENTITY_SOP		0x48
>  #define TPS_REG_DATA_STATUS		0x5f
> @@ -93,6 +94,7 @@ struct tps6598x {
>  	struct power_supply *psy;
>  	struct power_supply_desc psy_desc;
>  	enum power_supply_usb_type usb_type;
> +	struct tps6598x_pdo terms;
>  
>  	u32 data_status;
>  	u16 pwr_status;
> @@ -528,6 +530,47 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>  	return IRQ_NONE;
>  }
>  
> +static int tps6598x_get_active_pd_contract(struct tps6598x *tps)
> +{
> +	u64 contract;
> +	int type;
> +	int max_power;
> +	int ret = 0;
> +
> +	ret = tps6598x_block_read(tps, TPS_REG_ACTIVE_CONTRACT, &contract, 6);
> +	if (ret)
> +		return ret;
> +
> +	contract &= 0xFFFFFFFFFFFF;
> +	type = TPS_PDO_CONTRACT_TYPE(contract);
> +	memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
> +
> +	/* If there's no PD it decodes to all 0 */
> +	switch (type) {
> +	case TPS_PDO_CONTRACT_FIXED:
> +		tps->terms.max_voltage = TPS_PDO_FIXED_CONTRACT_VOLTAGE(contract);
> +		tps->terms.max_current = TPS_PDO_FIXED_CONTRACT_MAX_CURRENT(contract);
> +		break;
> +	case TPS_PDO_CONTRACT_BATTERY:
> +		tps->terms.max_voltage = TPS_PDO_BAT_CONTRACT_MAX_VOLTAGE(contract);
> +		max_power = TPS_PDO_BAT_CONTRACT_MAX_POWER(contract);
> +		tps->terms.max_current = 1000 * 1000 * max_power / tps->terms.max_voltage;
> +		break;
> +	case TPS_PDO_CONTRACT_VARIABLE:
> +		tps->terms.max_voltage = TPS_PDO_VAR_CONTRACT_MAX_VOLTAGE(contract);
> +		tps->terms.max_current = TPS_PDO_VAR_CONTRACT_MAX_CURRENT(contract);
> +		break;
> +	default:
> +		dev_warn(tps->dev, "Unknown contract type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	tps->terms.pdo = contract;
> +	trace_tps6598x_pdo(&tps->terms);
> +
> +	return 0;
> +}
> +
>  static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  {
>  	struct tps6598x *tps = data;
> @@ -563,6 +606,14 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  		tps6598x_update_data_status(tps, status);
>  	}
>  
> +	if ((event1 | event2) & TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER) {
> +		ret = tps6598x_get_active_pd_contract(tps);
> +		if (ret) {
> +			dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
> +			goto err_clear_ints;
> +		}
> +	}
> +
>  	/* Handle plug insert or removal */
>  	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
>  		tps6598x_handle_plug_event(tps, status);
> @@ -751,10 +802,11 @@ static int tps6598x_probe(struct i2c_client *client)
>  
>  		irq_handler = cd321x_interrupt;
>  	} else {
> -		/* Enable power status, data status and plug event interrupts */
> +		/* Enable interrupts used by this driver */
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>  			TPS_REG_INT_DATA_STATUS_UPDATE |
> -			TPS_REG_INT_PLUG_EVENT;
> +			TPS_REG_INT_PLUG_EVENT |
> +			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER;
>  	}
>  
>  	/* Make sure the controller has application firmware running */
> @@ -847,6 +899,13 @@ static int tps6598x_probe(struct i2c_client *client)
>  		ret = tps6598x_connect(tps, status);
>  		if (ret)
>  			dev_err(&client->dev, "failed to register partner\n");
> +
> +		if ((TPS_POWER_STATUS_PWROPMODE(tps->pwr_status) == TYPEC_PWR_MODE_PD) &&
> +		    (!(status & TPS_STATUS_PORTROLE))) {
> +			ret = tps6598x_get_active_pd_contract(tps);
> +			if (ret)
> +				dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
> +		}
>  	}
>  
>  	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> diff --git a/drivers/usb/typec/tipd/tps6598x.h b/drivers/usb/typec/tipd/tps6598x.h
> index 527857549d69..546cd4881f1f 100644
> --- a/drivers/usb/typec/tipd/tps6598x.h
> +++ b/drivers/usb/typec/tipd/tps6598x.h
> @@ -199,4 +199,34 @@
>  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_A    BIT(2)
>  #define TPS_DATA_STATUS_DP_SPEC_PIN_ASSIGNMENT_B    (BIT(2) | BIT(1))
>  
> +
> +/* PDO decoding as in https://www.ti.com/lit/an/slva842/slva842.pdf */
> +#define TPS_PDO_CONTRACT_TYPE(x)	FIELD_GET(GENMASK(31, 30), x)
> +#define TPS_PDO_CONTRACT_FIXED	0
> +#define TPS_PDO_CONTRACT_BATTERY	1
> +#define TPS_PDO_CONTRACT_VARIABLE	2
> +
> +#define TPS_PDO_FIXED_CONTRACT_DETAILS		GENMASK(29, 25)
> +#define TPS_PDO_FIXED_CONTRACT_DR_POWER		BIT(29)
> +#define TPS_PDO_FIXED_CONTRACT_USB_SUSPEND	BIT(28)
> +#define TPS_PDO_FIXED_CONTRACT_EXTERNAL_PWR	BIT(27)
> +#define TPS_PDO_FIXED_CONTRACT_USB_COMMS	BIT(26)
> +#define TPS_PDO_FIXED_CONTRACT_DR_DATA		BIT(25)

Those are already defined in include/linux/usb/pd.h

> +#define TPS_PDO_FIXED_CONTRACT_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
> +#define TPS_PDO_FIXED_CONTRACT_MAX_CURRENT(x)	(FIELD_GET(GENMASK(9, 0), x) * 10000)
> +#define TPS_PDO_VAR_CONTRACT_MAX_VOLTAGE(x)	(FIELD_GET(GENMASK(29, 20), x) * 50000)
> +#define TPS_PDO_VAR_CONTRACT_MIN_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
> +#define TPS_PDO_VAR_CONTRACT_MAX_CURRENT(x)	(FIELD_GET(GENMASK(9, 0), x) * 10000)
> +#define TPS_PDO_BAT_CONTRACT_MAX_VOLTAGE(x)	(FIELD_GET(GENMASK(29, 20), x) * 50000)
> +#define TPS_PDO_BAT_CONTRACT_MIN_VOLTAGE(x)	(FIELD_GET(GENMASK(19, 10), x) * 50000)
> +#define TPS_PDO_BAT_CONTRACT_MAX_POWER(x)	(FIELD_GET(GENMASK(9, 0), x) * 250000)

And I believe the same with these too.

> +struct tps6598x_pdo {
> +	u32 pdo;
> +	int max_voltage; /* uV */
> +	int max_current; /* uA */
> +	int max_power;   /* uW */
> +};
> +
>  #endif /* __TPS6598X_H__ */
> diff --git a/drivers/usb/typec/tipd/trace.h b/drivers/usb/typec/tipd/trace.h
> index 12cad1bde7cc..148e2ef3157b 100644
> --- a/drivers/usb/typec/tipd/trace.h
> +++ b/drivers/usb/typec/tipd/trace.h
> @@ -194,6 +194,20 @@
>  	(data_status & TPS_DATA_STATUS_DP_CONNECTION ? \
>  	 show_data_status_dp_pin_assignment(data_status) : "")
>  
> +#define show_pdo_contract_type(pdo) \
> +	__print_symbolic(TPS_PDO_CONTRACT_TYPE(pdo), \
> +		{ TPS_PDO_CONTRACT_FIXED, "fixed" }, \
> +		{ TPS_PDO_CONTRACT_BATTERY, "battery" }, \
> +		{ TPS_PDO_CONTRACT_VARIABLE, "variable" })
> +
> +#define show_pdo_contract_details(pdo) \
> +	__print_flags(pdo & TPS_PDO_FIXED_CONTRACT_DETAILS, "|", \
> +		{ TPS_PDO_FIXED_CONTRACT_DR_POWER, "dr-power" }, \
> +		{ TPS_PDO_FIXED_CONTRACT_USB_SUSPEND, "usb-suspend" }, \
> +		{ TPS_PDO_FIXED_CONTRACT_EXTERNAL_PWR, "ext-power" }, \
> +		{ TPS_PDO_FIXED_CONTRACT_USB_COMMS, "usb-comms" }, \
> +		{ TPS_PDO_FIXED_CONTRACT_DR_DATA, "dr-data" })
> +
>  TRACE_EVENT(tps6598x_irq,
>  	    TP_PROTO(u64 event1,
>  		     u64 event2),
> @@ -296,6 +310,30 @@ TRACE_EVENT(tps6598x_data_status,
>  		    )
>  );
>  
> +TRACE_EVENT(tps6598x_pdo,
> +	    TP_PROTO(struct tps6598x_pdo *pdo),
> +	    TP_ARGS(pdo),
> +
> +	    TP_STRUCT__entry(
> +			     __field(u32, pdo)
> +			     __field(int, max_current)
> +			     __field(int, max_voltage)
> +			     ),
> +
> +	    TP_fast_assign(
> +			   __entry->pdo = pdo->pdo;
> +			   __entry->max_current = pdo->max_current;
> +			   __entry->max_voltage = pdo->max_voltage;
> +			   ),
> +
> +	    TP_printk("%s supply, max %duA, %duV, details: %s",
> +		      show_pdo_contract_type(__entry->pdo),
> +		      __entry->max_current,
> +		      __entry->max_voltage,
> +		      show_pdo_contract_details(__entry->pdo)
> +		    )
> +);
> +
>  #endif /* _TPS6598X_TRACE_H_ */
>  
>  /* This part must be outside protection */

thanks,

-- 
heikki

[PATCH 4/7] usb: typec: tipd: Provide POWER_SUPPLY_PROP_{CURRENT,VOLTAGE}_MAX

[Sebastian Krzyszkowiak]

From: Angus Ainslie <angus@akkea.ca>

This helps downstream supplies to adjust their input limits.

Signed-off-by: Angus Ainslie <angus@akkea.ca>
Signed-off-by: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 76 ++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 80b4a9870caf..f3e8f1183f5b 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -39,6 +39,11 @@
 #define TPS_REG_RX_IDENTITY_SOP		0x48
 #define TPS_REG_DATA_STATUS		0x5f
 
+#define TPS_USB_500mA	  500000
+#define TPS_TYPEC_1500mA 1500000
+#define TPS_TYPEC_3000mA 3000000
+#define TPS_USB_5V	 5000000
+
 /* TPS_REG_SYSTEM_CONF bits */
 #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
 
@@ -103,6 +108,8 @@ struct tps6598x {
 static enum power_supply_property tps6598x_psy_props[] = {
 	POWER_SUPPLY_PROP_USB_TYPE,
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_CURRENT_MAX,
+	POWER_SUPPLY_PROP_VOLTAGE_MAX,
 };
 
 static enum power_supply_usb_type tps6598x_psy_usb_types[] = {
@@ -294,6 +301,8 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 	typec_set_orientation(tps->port, TYPEC_ORIENTATION_NONE);
 	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status), false);
 
+	memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
+
 	power_supply_changed(tps->psy);
 }
 
@@ -577,6 +586,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	u64 event1;
 	u64 event2;
 	u32 status;
+	bool psy_changed = false;
 	int ret;
 
 	mutex_lock(&tps->lock);
@@ -595,10 +605,13 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 	if (!tps6598x_read_status(tps, &status))
 		goto err_clear_ints;
 
-	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
+	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) {
 		if (!tps6598x_read_power_status(tps))
 			goto err_clear_ints;
 
+		psy_changed = true;
+	}
+
 	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
 		if (!tps6598x_read_data_status(tps))
 			goto err_clear_ints;
@@ -612,12 +625,18 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 			dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
 			goto err_clear_ints;
 		}
+		psy_changed = true;
 	}
 
 	/* Handle plug insert or removal */
 	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
 		tps6598x_handle_plug_event(tps, status);
 
+	if ((event1 | event2) & TPS_REG_INT_HARD_RESET) {
+		memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
+		psy_changed = true;
+	}
+
 err_clear_ints:
 	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
 	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
@@ -625,6 +644,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
 err_unlock:
 	mutex_unlock(&tps->lock);
 
+	if (psy_changed)
+		power_supply_changed(tps->psy);
+
 	if (event1 | event2)
 		return IRQ_HANDLED;
 	return IRQ_NONE;
@@ -671,6 +693,49 @@ static int tps6598x_psy_get_online(struct tps6598x *tps,
 	} else {
 		val->intval = 0;
 	}
+
+	return 0;
+}
+
+static int tps6598x_psy_get_max_current(struct tps6598x *tps,
+					union power_supply_propval *val)
+{
+	enum typec_pwr_opmode mode;
+
+	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
+	switch (mode) {
+	case TYPEC_PWR_MODE_1_5A:
+		val->intval = TPS_TYPEC_1500mA;
+		break;
+	case TYPEC_PWR_MODE_3_0A:
+		val->intval = TPS_TYPEC_3000mA;
+		break;
+	case TYPEC_PWR_MODE_PD:
+		val->intval = tps->terms.max_current ?: TPS_USB_500mA;
+		break;
+	default:
+	case TYPEC_PWR_MODE_USB:
+		val->intval = TPS_USB_500mA;
+	}
+	return 0;
+}
+
+static int tps6598x_psy_get_max_voltage(struct tps6598x *tps,
+					union power_supply_propval *val)
+{
+	enum typec_pwr_opmode mode;
+
+	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
+	switch (mode) {
+	case TYPEC_PWR_MODE_PD:
+		val->intval = tps->terms.max_voltage ?: TPS_USB_5V;
+		break;
+	default:
+	case TYPEC_PWR_MODE_1_5A:
+	case TYPEC_PWR_MODE_3_0A:
+	case TYPEC_PWR_MODE_USB:
+		val->intval = TPS_USB_5V;
+	}
 	return 0;
 }
 
@@ -691,6 +756,12 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		ret = tps6598x_psy_get_online(tps, val);
 		break;
+	case POWER_SUPPLY_PROP_CURRENT_MAX:
+		ret = tps6598x_psy_get_max_current(tps, val);
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
+		ret = tps6598x_psy_get_max_voltage(tps, val);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -806,7 +877,8 @@ static int tps6598x_probe(struct i2c_client *client)
 		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
 			TPS_REG_INT_DATA_STATUS_UPDATE |
 			TPS_REG_INT_PLUG_EVENT |
-			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER;
+			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER |
+			TPS_REG_INT_HARD_RESET;
 	}
 
 	/* Make sure the controller has application firmware running */
-- 
2.35.1

Re: [PATCH 4/7] usb: typec: tipd: Provide POWER_SUPPLY_PROP_{CURRENT,VOLTAGE}_MAX

[Heikki Krogerus]

Hi,

On Thu, Mar 17, 2022 at 04:45:15PM +0100, Sebastian Krzyszkowiak wrote:
> From: Angus Ainslie <angus@akkea.ca>
> 
> This helps downstream supplies to adjust their input limits.
> 
> Signed-off-by: Angus Ainslie <angus@akkea.ca>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
> ---
>  drivers/usb/typec/tipd/core.c | 76 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 80b4a9870caf..f3e8f1183f5b 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -39,6 +39,11 @@
>  #define TPS_REG_RX_IDENTITY_SOP		0x48
>  #define TPS_REG_DATA_STATUS		0x5f
>  
> +#define TPS_USB_500mA	  500000
> +#define TPS_TYPEC_1500mA 1500000
> +#define TPS_TYPEC_3000mA 3000000
> +#define TPS_USB_5V	 5000000
> +
>  /* TPS_REG_SYSTEM_CONF bits */
>  #define TPS_SYSCONF_PORTINFO(c)		((c) & 7)
>  
> @@ -103,6 +108,8 @@ struct tps6598x {
>  static enum power_supply_property tps6598x_psy_props[] = {
>  	POWER_SUPPLY_PROP_USB_TYPE,
>  	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_CURRENT_MAX,
> +	POWER_SUPPLY_PROP_VOLTAGE_MAX,
>  };
>  
>  static enum power_supply_usb_type tps6598x_psy_usb_types[] = {
> @@ -294,6 +301,8 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
>  	typec_set_orientation(tps->port, TYPEC_ORIENTATION_NONE);
>  	tps6598x_set_data_role(tps, TPS_STATUS_TO_TYPEC_DATAROLE(status), false);
>  
> +	memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
> +
>  	power_supply_changed(tps->psy);
>  }
>  
> @@ -577,6 +586,7 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	u64 event1;
>  	u64 event2;
>  	u32 status;
> +	bool psy_changed = false;
>  	int ret;
>  
>  	mutex_lock(&tps->lock);
> @@ -595,10 +605,13 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  	if (!tps6598x_read_status(tps, &status))
>  		goto err_clear_ints;
>  
> -	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE)
> +	if ((event1 | event2) & TPS_REG_INT_POWER_STATUS_UPDATE) {
>  		if (!tps6598x_read_power_status(tps))
>  			goto err_clear_ints;
>  
> +		psy_changed = true;
> +	}
> +
>  	if ((event1 | event2) & TPS_REG_INT_DATA_STATUS_UPDATE) {
>  		if (!tps6598x_read_data_status(tps))
>  			goto err_clear_ints;
> @@ -612,12 +625,18 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  			dev_err(tps->dev, "failed to read pd contract: %d\n", ret);
>  			goto err_clear_ints;
>  		}
> +		psy_changed = true;

Can data status change alone really indicate that the contract has
changed?

>  	}
>  
>  	/* Handle plug insert or removal */
>  	if ((event1 | event2) & TPS_REG_INT_PLUG_EVENT)
>  		tps6598x_handle_plug_event(tps, status);
>  
> +	if ((event1 | event2) & TPS_REG_INT_HARD_RESET) {
> +		memset(&tps->terms, 0, sizeof(struct tps6598x_pdo));
> +		psy_changed = true;
> +	}
> +
>  err_clear_ints:
>  	tps6598x_write64(tps, TPS_REG_INT_CLEAR1, event1);
>  	tps6598x_write64(tps, TPS_REG_INT_CLEAR2, event2);
> @@ -625,6 +644,9 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>  err_unlock:
>  	mutex_unlock(&tps->lock);
>  
> +	if (psy_changed)
> +		power_supply_changed(tps->psy);
> +
>  	if (event1 | event2)
>  		return IRQ_HANDLED;
>  	return IRQ_NONE;
> @@ -671,6 +693,49 @@ static int tps6598x_psy_get_online(struct tps6598x *tps,
>  	} else {
>  		val->intval = 0;
>  	}
> +
> +	return 0;
> +}
> +
> +static int tps6598x_psy_get_max_current(struct tps6598x *tps,
> +					union power_supply_propval *val)
> +{
> +	enum typec_pwr_opmode mode;
> +
> +	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
> +	switch (mode) {
> +	case TYPEC_PWR_MODE_1_5A:
> +		val->intval = TPS_TYPEC_1500mA;
> +		break;
> +	case TYPEC_PWR_MODE_3_0A:
> +		val->intval = TPS_TYPEC_3000mA;
> +		break;
> +	case TYPEC_PWR_MODE_PD:
> +		val->intval = tps->terms.max_current ?: TPS_USB_500mA;
> +		break;
> +	default:
> +	case TYPEC_PWR_MODE_USB:
> +		val->intval = TPS_USB_500mA;
> +	}
> +	return 0;
> +}
> +
> +static int tps6598x_psy_get_max_voltage(struct tps6598x *tps,
> +					union power_supply_propval *val)
> +{
> +	enum typec_pwr_opmode mode;
> +
> +	mode = TPS_POWER_STATUS_PWROPMODE(tps->pwr_status);
> +	switch (mode) {
> +	case TYPEC_PWR_MODE_PD:
> +		val->intval = tps->terms.max_voltage ?: TPS_USB_5V;
> +		break;
> +	default:
> +	case TYPEC_PWR_MODE_1_5A:
> +	case TYPEC_PWR_MODE_3_0A:
> +	case TYPEC_PWR_MODE_USB:
> +		val->intval = TPS_USB_5V;
> +	}
>  	return 0;
>  }
>  
> @@ -691,6 +756,12 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_ONLINE:
>  		ret = tps6598x_psy_get_online(tps, val);
>  		break;
> +	case POWER_SUPPLY_PROP_CURRENT_MAX:
> +		ret = tps6598x_psy_get_max_current(tps, val);
> +		break;
> +	case POWER_SUPPLY_PROP_VOLTAGE_MAX:
> +		ret = tps6598x_psy_get_max_voltage(tps, val);
> +		break;
>  	default:
>  		ret = -EINVAL;
>  		break;
> @@ -806,7 +877,8 @@ static int tps6598x_probe(struct i2c_client *client)
>  		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>  			TPS_REG_INT_DATA_STATUS_UPDATE |
>  			TPS_REG_INT_PLUG_EVENT |
> -			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER;
> +			TPS_REG_INT_NEW_CONTRACT_AS_CONSUMER |
> +			TPS_REG_INT_HARD_RESET;
>  	}
>  
>  	/* Make sure the controller has application firmware running */
> -- 
> 2.35.1

thanks,

-- 
heikki

[PATCH 5/7] usb: typec: tipd: Provide POWER_SUPPLY_PROP_PRESENT

[Sebastian Krzyszkowiak]

This allows userspace and downstream supplies to know whether
something is plugged in.

Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index f3e8f1183f5b..874528b02a99 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -108,6 +108,7 @@ struct tps6598x {
 static enum power_supply_property tps6598x_psy_props[] = {
 	POWER_SUPPLY_PROP_USB_TYPE,
 	POWER_SUPPLY_PROP_ONLINE,
+	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_CURRENT_MAX,
 	POWER_SUPPLY_PROP_VOLTAGE_MAX,
 };
@@ -756,6 +757,9 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_ONLINE:
 		ret = tps6598x_psy_get_online(tps, val);
 		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = !!tps->partner;
+		break;
 	case POWER_SUPPLY_PROP_CURRENT_MAX:
 		ret = tps6598x_psy_get_max_current(tps, val);
 		break;
-- 
2.35.1

Re: [PATCH 5/7] usb: typec: tipd: Provide POWER_SUPPLY_PROP_PRESENT

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:16PM +0100, Sebastian Krzyszkowiak wrote:
> This allows userspace and downstream supplies to know whether
> something is plugged in.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index f3e8f1183f5b..874528b02a99 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -108,6 +108,7 @@ struct tps6598x {
>  static enum power_supply_property tps6598x_psy_props[] = {
>  	POWER_SUPPLY_PROP_USB_TYPE,
>  	POWER_SUPPLY_PROP_ONLINE,
> +	POWER_SUPPLY_PROP_PRESENT,
>  	POWER_SUPPLY_PROP_CURRENT_MAX,
>  	POWER_SUPPLY_PROP_VOLTAGE_MAX,
>  };
> @@ -756,6 +757,9 @@ static int tps6598x_psy_get_prop(struct power_supply *psy,
>  	case POWER_SUPPLY_PROP_ONLINE:
>  		ret = tps6598x_psy_get_online(tps, val);
>  		break;
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = !!tps->partner;
> +		break;
>  	case POWER_SUPPLY_PROP_CURRENT_MAX:
>  		ret = tps6598x_psy_get_max_current(tps, val);
>  		break;
> -- 
> 2.35.1

-- 
heikki

[PATCH 6/7] usb: typec: tipd: Add debugfs entries for customer use word

[Sebastian Krzyszkowiak]

From: Guido Günther <agx@sigxcpu.org>

This allows to verify that a sane firmware is on the device.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 65 +++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 874528b02a99..d3c70aaf1a0c 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -6,6 +6,7 @@
  * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
  */
 
+#include <linux/debugfs.h>
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
@@ -22,6 +23,7 @@
 /* Register offsets */
 #define TPS_REG_VID			0x00
 #define TPS_REG_MODE			0x03
+#define TPS_REG_CUSTOMER_USE		0x06
 #define TPS_REG_CMD1			0x08
 #define TPS_REG_DATA1			0x09
 #define TPS_REG_INT_EVENT1		0x14
@@ -99,10 +101,15 @@ struct tps6598x {
 	struct power_supply *psy;
 	struct power_supply_desc psy_desc;
 	enum power_supply_usb_type usb_type;
+
 	struct tps6598x_pdo terms;
 
 	u32 data_status;
 	u16 pwr_status;
+#ifdef CONFIG_DEBUG_FS
+	struct dentry *dev_dentry;
+	struct dentry *customer_user_dentry;
+#endif
 };
 
 static enum power_supply_property tps6598x_psy_props[] = {
@@ -239,6 +246,62 @@ static void tps6598x_set_data_role(struct tps6598x *tps,
 	typec_set_data_role(tps->port, role);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static struct dentry *rootdir;
+
+static int tps6598x_debug_customer_use_show(struct seq_file *s, void *v)
+{
+	struct tps6598x *tps = (struct tps6598x *)s->private;
+	u64 mode64;
+	int ret;
+
+	mutex_lock(&tps->lock);
+
+	ret =  tps6598x_block_read(tps, TPS_REG_CUSTOMER_USE, &mode64, sizeof(mode64));
+	if (!ret)
+		seq_printf(s, "0x%016llx\n", mode64);
+
+	mutex_unlock(&tps->lock);
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(tps6598x_debug_customer_use);
+
+static void tps6598x_debugfs_init(struct tps6598x *tps)
+{
+	struct dentry *dentry;
+
+	if (!rootdir)
+		rootdir = debugfs_create_dir("tps6598x", NULL);
+
+	dentry = debugfs_create_dir(dev_name(tps->dev), rootdir);
+	if (IS_ERR(dentry))
+		return;
+	tps->dev_dentry = dentry;
+
+	dentry = debugfs_create_file("customer_use",
+				     S_IFREG | 0444, tps->dev_dentry,
+				     tps, &tps6598x_debug_customer_use_fops);
+	if (IS_ERR(dentry))
+		return;
+	tps->customer_user_dentry = dentry;
+}
+
+static void tps6598x_debugfs_exit(struct tps6598x *tps)
+{
+	debugfs_remove(tps->customer_user_dentry);
+	debugfs_remove(tps->dev_dentry);
+	debugfs_remove(rootdir);
+	rootdir = NULL;
+}
+
+#else
+
+static void tps6598x_debugfs_init(const struct tps6598x *tps) { }
+static void tps6598x_debugfs_exit(const struct tps6598x *tps) { }
+
+#endif
+
 static int tps6598x_connect(struct tps6598x *tps, u32 status)
 {
 	struct typec_partner_desc desc;
@@ -995,6 +1058,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	}
 
 	i2c_set_clientdata(client, tps);
+	tps6598x_debugfs_init(tps);
 
 	return 0;
 
@@ -1011,6 +1075,7 @@ static int tps6598x_remove(struct i2c_client *client)
 {
 	struct tps6598x *tps = i2c_get_clientdata(client);
 
+	tps6598x_debugfs_exit(tps);
 	tps6598x_disconnect(tps, 0);
 	typec_unregister_port(tps->port);
 	usb_role_switch_put(tps->role_sw);
-- 
2.35.1

Re: [PATCH 6/7] usb: typec: tipd: Add debugfs entries for customer use word

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:17PM +0100, Sebastian Krzyszkowiak wrote:
> From: Guido Günther <agx@sigxcpu.org>
> 
> This allows to verify that a sane firmware is on the device.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 65 +++++++++++++++++++++++++++++++++++
>  1 file changed, 65 insertions(+)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 874528b02a99..d3c70aaf1a0c 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -6,6 +6,7 @@
>   * Author: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/i2c.h>
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> @@ -22,6 +23,7 @@
>  /* Register offsets */
>  #define TPS_REG_VID			0x00
>  #define TPS_REG_MODE			0x03
> +#define TPS_REG_CUSTOMER_USE		0x06
>  #define TPS_REG_CMD1			0x08
>  #define TPS_REG_DATA1			0x09
>  #define TPS_REG_INT_EVENT1		0x14
> @@ -99,10 +101,15 @@ struct tps6598x {
>  	struct power_supply *psy;
>  	struct power_supply_desc psy_desc;
>  	enum power_supply_usb_type usb_type;
> +
>  	struct tps6598x_pdo terms;
>  
>  	u32 data_status;
>  	u16 pwr_status;
> +#ifdef CONFIG_DEBUG_FS
> +	struct dentry *dev_dentry;
> +	struct dentry *customer_user_dentry;
> +#endif
>  };
>  
>  static enum power_supply_property tps6598x_psy_props[] = {
> @@ -239,6 +246,62 @@ static void tps6598x_set_data_role(struct tps6598x *tps,
>  	typec_set_data_role(tps->port, role);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +static struct dentry *rootdir;
> +
> +static int tps6598x_debug_customer_use_show(struct seq_file *s, void *v)
> +{
> +	struct tps6598x *tps = (struct tps6598x *)s->private;
> +	u64 mode64;
> +	int ret;
> +
> +	mutex_lock(&tps->lock);
> +
> +	ret =  tps6598x_block_read(tps, TPS_REG_CUSTOMER_USE, &mode64, sizeof(mode64));
> +	if (!ret)
> +		seq_printf(s, "0x%016llx\n", mode64);
> +
> +	mutex_unlock(&tps->lock);
> +
> +	return 0;
> +}
> +DEFINE_SHOW_ATTRIBUTE(tps6598x_debug_customer_use);
> +
> +static void tps6598x_debugfs_init(struct tps6598x *tps)
> +{
> +	struct dentry *dentry;
> +
> +	if (!rootdir)
> +		rootdir = debugfs_create_dir("tps6598x", NULL);
> +
> +	dentry = debugfs_create_dir(dev_name(tps->dev), rootdir);
> +	if (IS_ERR(dentry))
> +		return;
> +	tps->dev_dentry = dentry;
> +
> +	dentry = debugfs_create_file("customer_use",
> +				     S_IFREG | 0444, tps->dev_dentry,
> +				     tps, &tps6598x_debug_customer_use_fops);
> +	if (IS_ERR(dentry))
> +		return;
> +	tps->customer_user_dentry = dentry;
> +}
> +
> +static void tps6598x_debugfs_exit(struct tps6598x *tps)
> +{
> +	debugfs_remove(tps->customer_user_dentry);
> +	debugfs_remove(tps->dev_dentry);
> +	debugfs_remove(rootdir);
> +	rootdir = NULL;
> +}
> +
> +#else
> +
> +static void tps6598x_debugfs_init(const struct tps6598x *tps) { }
> +static void tps6598x_debugfs_exit(const struct tps6598x *tps) { }
> +
> +#endif
> +
>  static int tps6598x_connect(struct tps6598x *tps, u32 status)
>  {
>  	struct typec_partner_desc desc;
> @@ -995,6 +1058,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	}
>  
>  	i2c_set_clientdata(client, tps);
> +	tps6598x_debugfs_init(tps);
>  
>  	return 0;
>  
> @@ -1011,6 +1075,7 @@ static int tps6598x_remove(struct i2c_client *client)
>  {
>  	struct tps6598x *tps = i2c_get_clientdata(client);
>  
> +	tps6598x_debugfs_exit(tps);
>  	tps6598x_disconnect(tps, 0);
>  	typec_unregister_port(tps->port);
>  	usb_role_switch_put(tps->role_sw);
> -- 
> 2.35.1

-- 
heikki

[PATCH 7/7] usb: typec: tipd: Fail probe when the controller is in BOOT mode

[Sebastian Krzyszkowiak]

BOOT mode means that the device isn't operational because of missing
firmware, so there's no reason to try to continue in this condition
(probe will fail in a different place anyway).

Aside of that, the warning that used to be emited about "dead-battery
condition" was misleading, as dead-battery condition is a different
thing that's unrelated to operation mode.

Therefore, assume that BOOT mode is not a supported mode of operation.

Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
---
 drivers/usb/typec/tipd/core.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index d3c70aaf1a0c..c818cc40139d 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -729,8 +729,6 @@ static int tps6598x_check_mode(struct tps6598x *tps)
 	case TPS_MODE_APP:
 		return 0;
 	case TPS_MODE_BOOT:
-		dev_warn(tps->dev, "dead-battery condition\n");
-		return 0;
 	case TPS_MODE_BIST:
 	case TPS_MODE_DISC:
 	default:
-- 
2.35.1

Re: [PATCH 7/7] usb: typec: tipd: Fail probe when the controller is in BOOT mode

[Heikki Krogerus]

On Thu, Mar 17, 2022 at 04:45:18PM +0100, Sebastian Krzyszkowiak wrote:
> BOOT mode means that the device isn't operational because of missing
> firmware, so there's no reason to try to continue in this condition
> (probe will fail in a different place anyway).
> 
> Aside of that, the warning that used to be emited about "dead-battery
> condition" was misleading, as dead-battery condition is a different
> thing that's unrelated to operation mode.
> 
> Therefore, assume that BOOT mode is not a supported mode of operation.
> 
> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/tipd/core.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index d3c70aaf1a0c..c818cc40139d 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -729,8 +729,6 @@ static int tps6598x_check_mode(struct tps6598x *tps)
>  	case TPS_MODE_APP:
>  		return 0;
>  	case TPS_MODE_BOOT:
> -		dev_warn(tps->dev, "dead-battery condition\n");
> -		return 0;
>  	case TPS_MODE_BIST:
>  	case TPS_MODE_DISC:
>  	default:
> -- 
> 2.35.1

thanks,

-- 
heikki

Re: [PATCH 0/7] TPS6598x PD tracing and other improvements

[Heikki Krogerus]

Hi,

On Thu, Mar 17, 2022 at 04:45:11PM +0100, Sebastian Krzyszkowiak wrote:
> This is a series developed for the Librem 5 phone, which uses TPS65982
> as its USB-C controller. Implemented are Power Delivery sink contract
> tracing and exporting negotiated power values as power supply properties,
> fixes for data role swapping, status register caching and a debugfs entry
> for querying customer use word of the firmware running on the controller.
> 
> Angus Ainslie (3):
>   usb: typec: tipd: set the data role on tps IRQ
>   usb: typec: tipd: Add trace event for SINK PD contract
>   usb: typec: tipd: Provide POWER_SUPPLY_PROP_{CURRENT,VOLTAGE}_MAX
> 
> Guido Günther (2):
>   usb: typec: tipd: Only update power status on IRQ
>   usb: typec: tipd: Add debugfs entries for customer use word
> 
> Sebastian Krzyszkowiak (2):
>   usb: typec: tipd: Provide POWER_SUPPLY_PROP_PRESENT
>   usb: typec: tipd: Fail probe when the controller is in BOOT mode
> 
>  drivers/usb/typec/tipd/core.c     | 263 ++++++++++++++++++++++++++----
>  drivers/usb/typec/tipd/tps6598x.h |  30 ++++
>  drivers/usb/typec/tipd/trace.h    |  38 +++++
>  3 files changed, 302 insertions(+), 29 deletions(-)

These look pretty good to me. I'll see if I can test these on Monday -
I finally have access to a machine again that actually has TI PD
controller. But I will give my comments then in any case, if there is
anything to comment.

But related to patch 3/7, there is a series in works that would expose
the PDOs in sysfs [1]. I was wondering have you guys noticed it, and
would that actually work also in your case?

[1] https://lore.kernel.org/linux-usb/20220203144657.16527-1-heikki.krogerus@linux.intel.com/

Br,

-- 
heikki