usb: typec: tps6598x: Export some power supply properties

PATCH Superseded Guido Günther 2 participants 6 messages Dec 1, 2020

Subsystems (2)

USB USB Type-C class

lore

Share & tools

lore

Guido Günther

author

2020-12-01 13:00 UTC #0 lore raw

v4 cover usb: typec: tps6598x: Export some power supply properties

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 v3
- As per review comments from Andy Shevchenko
https://lore.kernel.org/linux-usb/CAHp75VeLZtm85Y=3QMkPGb332wn05-zr-_mrrwXvnqLhazR1Gg@mail.gmail.com/
- Use positive conditionals
- Add reviewed by from Heikki Krogerus

    https://lore.kernel.org/linux-usb/[email protected]/T/#u
    https://lore.kernel.org/linux-usb/[email protected]/T/#u

- Fix typc vs typec typo in commit message

changes from v2
- As per kernel test robot
https://lore.kernel.org/linux-usb/[email protected]/
- 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/[email protected]/
- select POWER_SUPPLY
- use POWER_SUPPLY_USB_TYPE_PD when a PD contract got negotiated

To: Heikki Krogerus <[email protected]>,Greg Kroah-Hartman <[email protected]>,[email protected],[email protected],Andy Shevchenko <[email protected]>

Guido Günther (2):
usb: typec: 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(-)

Signature · 1 line

2.29.2

Guido Günther

author

2020-12-01 13:00 UTC #1 lore raw

PATCH v4 1/2 usb: typec: tps6598x: Select USB_ROLE_SWITCH and REGMAP_I2C

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

Quoted 1 line

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 <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

---
 drivers/usb/typec/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/usb/typec/Kconfig +2 -2

index 6c5908a37ee8..772b07e9f188 100644
@@ -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.

Signature · 1 line

2.29.2

Guido Günther

author

2020-12-01 13:00 UTC #2 lore raw

PATCH v4 2/2 usb: typec: tps6598x: Export some power supply properties

In reply to Guido Günther

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

Signed-off-by Guido Günther <[email protected]>

Reviewed-by Heikki Krogerus <[email protected]>

---
 drivers/usb/typec/Kconfig    |   1 +
 drivers/usb/typec/tps6598x.c | 105 +++++++++++++++++++++++++++++++++++
 2 files changed, 106 insertions(+)

drivers/usb/typec/Kconfig +1 -0

index 772b07e9f188..365f905a8e49 100644
@@ -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

drivers/usb/typec/tps6598x.c +105 -0

149 lines not on this page · raw message

Signature · 1 line

2.29.2

Andy Shevchenko

2020-12-01 13:53 UTC #3 lore raw

Quoted 2 lines · Guido Günther

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

...

drivers/usb/typec/tps6598x.c static const struct regmap_config tps6598x_regmap_config = { · line 545

Quoted 7 lines

drivers/usb/typec/tps6598x.c +6 -0

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

I'm so sorry by not noticing this one...

drivers/usb/typec/tps6598x.c static const struct regmap_config tps6598x_regmap_config = { · line 555

Quoted 10 lines

drivers/usb/typec/tps6598x.c +10 -0

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

...followed by this.

Please, use devm_kasprintf() instead.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Guido Günther

author

2020-12-01 13:57 UTC #4 lore raw

Hi Andy,

Quoted 29 lines · Andy Shevchenko

Quoted 2 lines · Guido Günther
This allows downstream supplies and userspace to detect whether external power is supplied.
...
drivers/usb/typec/tps6598x.c static const struct regmap_config tps6598x_regmap_config = { · line 545
Quoted 7 lines
drivers/usb/typec/tps6598x.c +6 -0
+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;
I'm so sorry by not noticing this one...
drivers/usb/typec/tps6598x.c static const struct regmap_config tps6598x_regmap_config = { · line 555
Quoted 10 lines
drivers/usb/typec/tps6598x.c +10 -0
+       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);
...followed by this.

Quoted 1 line

Please, use devm_kasprintf() instead.

Will do. I'll let the series sit for a couple of days before sending a v5.
Cheers,
-- Guido

Quoted 5 lines

Signature · 3 lines
-- 
With Best Regards,
Andy Shevchenko

Andy Shevchenko

Reviewed-by on the v4 series

2020-12-01 13:55 UTC #5 lore raw

In reply to Guido Günther

Quoted 9 lines · 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?

From my perspective the patches are okay (after addressing one more comment), FWIW

Reviewed-by Andy Shevchenko <[email protected]>

Quoted 35 lines

changes from v3
- As per review comments from Andy Shevchenko
https://lore.kernel.org/linux-usb/CAHp75VeLZtm85Y=3QMkPGb332wn05-zr-_mrrwXvnqLhazR1Gg@mail.gmail.com/
- Use positive conditionals
- Add reviewed by from Heikki Krogerus
    https://lore.kernel.org/linux-usb/[email protected]/T/#u
    https://lore.kernel.org/linux-usb/[email protected]/T/#u
- Fix typc vs typec typo in commit message
changes from v2
- As per kernel test robot
https://lore.kernel.org/linux-usb/[email protected]/
- 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/[email protected]/
- select POWER_SUPPLY
- use POWER_SUPPLY_USB_TYPE_PD when a PD contract got negotiated
To: Heikki Krogerus <[email protected]>,Greg Kroah-Hartman <[email protected]>,[email protected],[email protected],Andy Shevchenko <[email protected]>
Guido Günther (2):
usb: typec: 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

Signature · 2 lines

With Best Regards,
Andy Shevchenko