[PATCH v3 0/2] usb: typec: tps6598x: Export some power supply properties

[PATCH v3 0/2] usb: typec: tps6598x: Export some power supply properties

[Guido Günther]

This allows downstream supplies and userspace to detect whether external power
is supplied.

The Librem 5 has the tp65982 in front of bq25980 charge controller.  Since that
is capable of sinking and sourcing power the online property helps to decide
what to do. It also makes upower happy.

There will be follow up patches providing more properties but these need some
more time to cook and i wanted to check if this is the right way to go?

changes from v2
  - As per kernel test robot
    https://lore.kernel.org/linux-usb/202011271005.zJVawX74-lkp@intel.com/
    - Flip USB_ROLE_SWITCH and REGMAP_I2C from 'depends on' to 'select'
      This matches tcpm and avoids a config symbol recursion which went
      unnoticed on my arm64 build but trips up x86_64.

changes from v1
  - As per review comments from Heikki Krogerus
    https://lore.kernel.org/linux-usb/20201126123552.GP1008337@kuha.fi.intel.com/
    - select POWER_SUPPLY
    - use POWER_SUPPLY_USB_TYPE_PD when a PD contract got negotiated

To: Heikki Krogerus <heikki.krogerus@linux.intel.com>,Greg Kroah-Hartman <gregkh@linuxfoundation.org>,linux-usb@vger.kernel.org,linux-kernel@vger.kernel.org

Guido Günther (2):
  usb: typc: tps6598x: Select USB_ROLE_SWITCH and REGMAP_I2C
  usb: typec: tps6598x: Export some power supply properties

 drivers/usb/typec/Kconfig    |   5 +-
 drivers/usb/typec/tps6598x.c | 105 +++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+), 2 deletions(-)

-- 
2.29.2

[PATCH v3 1/2] usb: typc: tps6598x: Select USB_ROLE_SWITCH and REGMAP_I2C

[Guido Günther]

This is more in line with what tcpm does and will be needed
to avoid recursive dependency like

 > drivers/power/supply/Kconfig:2:error: recursive dependency detected!
   drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by TYPEC_TPS6598X
   drivers/usb/typec/Kconfig:64: symbol TYPEC_TPS6598X depends on REGMAP_I2C
   drivers/base/regmap/Kconfig:19: symbol REGMAP_I2C is selected by CHARGER_ADP5061
   drivers/power/supply/Kconfig:93: symbol CHARGER_ADP5061 depends on POWER_SUPPLY
   For a resolution refer to Documentation/kbuild/kconfig-language.rst
   subsection "Kconfig recursive dependency limitations"

when selecting POWER_SUPPLY.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 6c5908a37ee8..772b07e9f188 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -64,8 +64,8 @@ config TYPEC_HD3SS3220
 config TYPEC_TPS6598X
 	tristate "TI TPS6598x USB Power Delivery controller driver"
 	depends on I2C
-	depends on REGMAP_I2C
-	depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
+	select REGMAP_I2C
+	select USB_ROLE_SWITCH
 	help
 	  Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power
 	  Delivery controller.
-- 
2.29.2

Re: [PATCH v3 1/2] usb: typc: tps6598x: Select USB_ROLE_SWITCH and REGMAP_I2C

[Heikki Krogerus]

On Fri, Nov 27, 2020 at 01:53:28PM +0100, Guido Günther wrote:
> This is more in line with what tcpm does and will be needed
> to avoid recursive dependency like
> 
>  > drivers/power/supply/Kconfig:2:error: recursive dependency detected!
>    drivers/power/supply/Kconfig:2: symbol POWER_SUPPLY is selected by TYPEC_TPS6598X
>    drivers/usb/typec/Kconfig:64: symbol TYPEC_TPS6598X depends on REGMAP_I2C
>    drivers/base/regmap/Kconfig:19: symbol REGMAP_I2C is selected by CHARGER_ADP5061
>    drivers/power/supply/Kconfig:93: symbol CHARGER_ADP5061 depends on POWER_SUPPLY
>    For a resolution refer to Documentation/kbuild/kconfig-language.rst
>    subsection "Kconfig recursive dependency limitations"
> 
> when selecting POWER_SUPPLY.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>

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

> ---
>  drivers/usb/typec/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 6c5908a37ee8..772b07e9f188 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -64,8 +64,8 @@ config TYPEC_HD3SS3220
>  config TYPEC_TPS6598X
>  	tristate "TI TPS6598x USB Power Delivery controller driver"
>  	depends on I2C
> -	depends on REGMAP_I2C
> -	depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
> +	select REGMAP_I2C
> +	select USB_ROLE_SWITCH
>  	help
>  	  Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power
>  	  Delivery controller.
> -- 
> 2.29.2

thanks,

-- 
heikki

[PATCH v3 2/2] usb: typec: tps6598x: Export some power supply properties

[Guido Günther]

This allows downstream supplies and userspace to detect
whether external power is supplied.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 drivers/usb/typec/Kconfig    |   1 +
 drivers/usb/typec/tps6598x.c | 105 +++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index 772b07e9f188..365f905a8e49 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -64,6 +64,7 @@ config TYPEC_HD3SS3220
 config TYPEC_TPS6598X
 	tristate "TI TPS6598x USB Power Delivery controller driver"
 	depends on I2C
+	select POWER_SUPPLY
 	select REGMAP_I2C
 	select USB_ROLE_SWITCH
 	help
diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
index 3db33bb622c3..8163306f849e 100644
--- a/drivers/usb/typec/tps6598x.c
+++ b/drivers/usb/typec/tps6598x.c
@@ -9,6 +9,7 @@
 #include <linux/i2c.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/power_supply.h>
 #include <linux/regmap.h>
 #include <linux/interrupt.h>
 #include <linux/usb/typec.h>
@@ -55,6 +56,7 @@ enum {
 };
 
 /* TPS_REG_POWER_STATUS bits */
+#define TPS_POWER_STATUS_CONNECTION	BIT(0)
 #define TPS_POWER_STATUS_SOURCESINK	BIT(1)
 #define TPS_POWER_STATUS_PWROPMODE(p)	(((p) & GENMASK(3, 2)) >> 2)
 
@@ -96,8 +98,25 @@ struct tps6598x {
 	struct typec_partner *partner;
 	struct usb_pd_identity partner_identity;
 	struct usb_role_switch *role_sw;
+	struct typec_capability typec_cap;
+
+	struct power_supply *psy;
+	struct power_supply_desc psy_desc;
+	enum power_supply_usb_type usb_type;
+};
+
+static enum power_supply_property tps6598x_psy_props[] = {
+	POWER_SUPPLY_PROP_USB_TYPE,
+	POWER_SUPPLY_PROP_ONLINE,
 };
 
+static enum power_supply_usb_type tps6598x_psy_usb_types[] = {
+	POWER_SUPPLY_USB_TYPE_C,
+	POWER_SUPPLY_USB_TYPE_PD,
+};
+
+static const char *tps6598x_psy_name_prefix = "tps6598x-source-psy-";
+
 /*
  * Max data bytes for Data1, Data2, and other registers. See ch 1.3.2:
  * https://www.ti.com/lit/ug/slvuan1a/slvuan1a.pdf
@@ -248,6 +267,8 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
 	if (desc.identity)
 		typec_partner_set_identity(tps->partner);
 
+	power_supply_changed(tps->psy);
+
 	return 0;
 }
 
@@ -260,6 +281,7 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
 	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
 	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
 	tps6598x_set_data_role(tps, TPS_STATUS_DATAROLE(status), false);
+	power_supply_changed(tps->psy);
 }
 
 static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
@@ -467,6 +489,85 @@ static const struct regmap_config tps6598x_regmap_config = {
 	.max_register = 0x7F,
 };
 
+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 (!(pwr_status & TPS_POWER_STATUS_CONNECTION) ||
+	    !(pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
+		val->intval = 0;
+	} else {
+		val->intval = 1;
+	}
+	return 0;
+}
+
+static int tps6598x_psy_get_prop(struct power_supply *psy,
+				 enum power_supply_property psp,
+				 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)
+			val->intval = POWER_SUPPLY_USB_TYPE_PD;
+		else
+			val->intval = POWER_SUPPLY_USB_TYPE_C;
+		break;
+	case POWER_SUPPLY_PROP_ONLINE:
+		ret = tps6598x_psy_get_online(tps, val);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static int devm_tps6598_psy_register(struct tps6598x *tps)
+{
+	struct power_supply_config psy_cfg = {};
+	const char *port_dev_name = dev_name(tps->dev);
+	size_t psy_name_len = strlen(tps6598x_psy_name_prefix) +
+				     strlen(port_dev_name) + 1;
+	char *psy_name;
+
+	psy_cfg.drv_data = tps;
+	psy_cfg.fwnode = dev_fwnode(tps->dev);
+	psy_name = devm_kzalloc(tps->dev, psy_name_len, GFP_KERNEL);
+	if (!psy_name)
+		return -ENOMEM;
+
+	snprintf(psy_name, psy_name_len, "%s%s", tps6598x_psy_name_prefix,
+		 port_dev_name);
+	tps->psy_desc.name = psy_name;
+	tps->psy_desc.type = POWER_SUPPLY_TYPE_USB;
+	tps->psy_desc.usb_types = tps6598x_psy_usb_types;
+	tps->psy_desc.num_usb_types = ARRAY_SIZE(tps6598x_psy_usb_types);
+	tps->psy_desc.properties = tps6598x_psy_props;
+	tps->psy_desc.num_properties = ARRAY_SIZE(tps6598x_psy_props);
+	tps->psy_desc.get_property = tps6598x_psy_get_prop;
+
+	tps->usb_type = POWER_SUPPLY_USB_TYPE_C;
+
+	tps->psy = devm_power_supply_register(tps->dev, &tps->psy_desc,
+					       &psy_cfg);
+	return PTR_ERR_OR_ZERO(tps->psy);
+}
+
 static int tps6598x_probe(struct i2c_client *client)
 {
 	struct typec_capability typec_cap = { };
@@ -560,6 +661,10 @@ static int tps6598x_probe(struct i2c_client *client)
 		goto err_role_put;
 	}
 
+	ret = devm_tps6598_psy_register(tps);
+	if (ret)
+		return ret;
+
 	tps->port = typec_register_port(&client->dev, &typec_cap);
 	if (IS_ERR(tps->port)) {
 		ret = PTR_ERR(tps->port);
-- 
2.29.2

Re: [PATCH v3 2/2] usb: typec: tps6598x: Export some power supply properties

[Heikki Krogerus]

On Fri, Nov 27, 2020 at 01:53:29PM +0100, Guido Günther wrote:
> This allows downstream supplies and userspace to detect
> whether external power is supplied.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>

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

> ---
>  drivers/usb/typec/Kconfig    |   1 +
>  drivers/usb/typec/tps6598x.c | 105 +++++++++++++++++++++++++++++++++++
>  2 files changed, 106 insertions(+)
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 772b07e9f188..365f905a8e49 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -64,6 +64,7 @@ config TYPEC_HD3SS3220
>  config TYPEC_TPS6598X
>  	tristate "TI TPS6598x USB Power Delivery controller driver"
>  	depends on I2C
> +	select POWER_SUPPLY
>  	select REGMAP_I2C
>  	select USB_ROLE_SWITCH
>  	help
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 3db33bb622c3..8163306f849e 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -9,6 +9,7 @@
>  #include <linux/i2c.h>
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/power_supply.h>
>  #include <linux/regmap.h>
>  #include <linux/interrupt.h>
>  #include <linux/usb/typec.h>
> @@ -55,6 +56,7 @@ enum {
>  };
>  
>  /* TPS_REG_POWER_STATUS bits */
> +#define TPS_POWER_STATUS_CONNECTION	BIT(0)
>  #define TPS_POWER_STATUS_SOURCESINK	BIT(1)
>  #define TPS_POWER_STATUS_PWROPMODE(p)	(((p) & GENMASK(3, 2)) >> 2)
>  
> @@ -96,8 +98,25 @@ struct tps6598x {
>  	struct typec_partner *partner;
>  	struct usb_pd_identity partner_identity;
>  	struct usb_role_switch *role_sw;
> +	struct typec_capability typec_cap;
> +
> +	struct power_supply *psy;
> +	struct power_supply_desc psy_desc;
> +	enum power_supply_usb_type usb_type;
> +};
> +
> +static enum power_supply_property tps6598x_psy_props[] = {
> +	POWER_SUPPLY_PROP_USB_TYPE,
> +	POWER_SUPPLY_PROP_ONLINE,
>  };
>  
> +static enum power_supply_usb_type tps6598x_psy_usb_types[] = {
> +	POWER_SUPPLY_USB_TYPE_C,
> +	POWER_SUPPLY_USB_TYPE_PD,
> +};
> +
> +static const char *tps6598x_psy_name_prefix = "tps6598x-source-psy-";
> +
>  /*
>   * Max data bytes for Data1, Data2, and other registers. See ch 1.3.2:
>   * https://www.ti.com/lit/ug/slvuan1a/slvuan1a.pdf
> @@ -248,6 +267,8 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
>  	if (desc.identity)
>  		typec_partner_set_identity(tps->partner);
>  
> +	power_supply_changed(tps->psy);
> +
>  	return 0;
>  }
>  
> @@ -260,6 +281,7 @@ static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
>  	typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
>  	typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
>  	tps6598x_set_data_role(tps, TPS_STATUS_DATAROLE(status), false);
> +	power_supply_changed(tps->psy);
>  }
>  
>  static int tps6598x_exec_cmd(struct tps6598x *tps, const char *cmd,
> @@ -467,6 +489,85 @@ static const struct regmap_config tps6598x_regmap_config = {
>  	.max_register = 0x7F,
>  };
>  
> +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 (!(pwr_status & TPS_POWER_STATUS_CONNECTION) ||
> +	    !(pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
> +		val->intval = 0;
> +	} else {
> +		val->intval = 1;
> +	}
> +	return 0;
> +}
> +
> +static int tps6598x_psy_get_prop(struct power_supply *psy,
> +				 enum power_supply_property psp,
> +				 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)
> +			val->intval = POWER_SUPPLY_USB_TYPE_PD;
> +		else
> +			val->intval = POWER_SUPPLY_USB_TYPE_C;
> +		break;
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		ret = tps6598x_psy_get_online(tps, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static int devm_tps6598_psy_register(struct tps6598x *tps)
> +{
> +	struct power_supply_config psy_cfg = {};
> +	const char *port_dev_name = dev_name(tps->dev);
> +	size_t psy_name_len = strlen(tps6598x_psy_name_prefix) +
> +				     strlen(port_dev_name) + 1;
> +	char *psy_name;
> +
> +	psy_cfg.drv_data = tps;
> +	psy_cfg.fwnode = dev_fwnode(tps->dev);
> +	psy_name = devm_kzalloc(tps->dev, psy_name_len, GFP_KERNEL);
> +	if (!psy_name)
> +		return -ENOMEM;
> +
> +	snprintf(psy_name, psy_name_len, "%s%s", tps6598x_psy_name_prefix,
> +		 port_dev_name);
> +	tps->psy_desc.name = psy_name;
> +	tps->psy_desc.type = POWER_SUPPLY_TYPE_USB;
> +	tps->psy_desc.usb_types = tps6598x_psy_usb_types;
> +	tps->psy_desc.num_usb_types = ARRAY_SIZE(tps6598x_psy_usb_types);
> +	tps->psy_desc.properties = tps6598x_psy_props;
> +	tps->psy_desc.num_properties = ARRAY_SIZE(tps6598x_psy_props);
> +	tps->psy_desc.get_property = tps6598x_psy_get_prop;
> +
> +	tps->usb_type = POWER_SUPPLY_USB_TYPE_C;
> +
> +	tps->psy = devm_power_supply_register(tps->dev, &tps->psy_desc,
> +					       &psy_cfg);
> +	return PTR_ERR_OR_ZERO(tps->psy);
> +}
> +
>  static int tps6598x_probe(struct i2c_client *client)
>  {
>  	struct typec_capability typec_cap = { };
> @@ -560,6 +661,10 @@ static int tps6598x_probe(struct i2c_client *client)
>  		goto err_role_put;
>  	}
>  
> +	ret = devm_tps6598_psy_register(tps);
> +	if (ret)
> +		return ret;
> +
>  	tps->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(tps->port)) {
>  		ret = PTR_ERR(tps->port);
> -- 
> 2.29.2

thanks,

-- 
heikki

Re: [PATCH v3 2/2] usb: typec: tps6598x: Export some power supply properties

[Andy Shevchenko]

On Fri, Nov 27, 2020 at 2:57 PM Guido Günther <agx@sigxcpu.org> wrote:
>
> This allows downstream supplies and userspace to detect
> whether external power is supplied.

> +       if (!(pwr_status & TPS_POWER_STATUS_CONNECTION) ||
> +           !(pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
> +               val->intval = 0;
> +       } else {
> +               val->intval = 1;
> +       }

Can we please use positive conditionals (which usually are easier to read)?

       if ((pwr_status & TPS_POWER_STATUS_CONNECTION) &&
           (pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
               val->intval = 1;
       } else {
               val->intval = 0;
       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] usb: typec: tps6598x: Export some power supply properties

[Guido Günther]

Hi,
On Mon, Nov 30, 2020 at 08:35:26PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 27, 2020 at 2:57 PM Guido Günther <agx@sigxcpu.org> wrote:
> >
> > This allows downstream supplies and userspace to detect
> > whether external power is supplied.
> 
> > +       if (!(pwr_status & TPS_POWER_STATUS_CONNECTION) ||
> > +           !(pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
> > +               val->intval = 0;
> > +       } else {
> > +               val->intval = 1;
> > +       }
> 
> Can we please use positive conditionals (which usually are easier to
> read)?

Make sense. Fixed in v4.
 -- Guido

> 
>        if ((pwr_status & TPS_POWER_STATUS_CONNECTION) &&
>            (pwr_status & TPS_POWER_STATUS_SOURCESINK)) {
>                val->intval = 1;
>        } else {
>                val->intval = 0;
>        }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
>