| Message ID | 20220730180500.152004-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bf7571c00dca0a9c5af3f5125ef5a89a40b13cd5 |
| Headers | show |
| Series | [1/2] extcon: usbc-tusb320: Factor out extcon into dedicated functions | expand |
Hi Marek,
I love your patch! Perhaps something to improve:
[auto build test WARNING on chanwoo-extcon/extcon-next]
[also build test WARNING on linus/master v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/extcon-usbc-tusb320-Factor-out-extcon-into-dedicated-functions/20220731-020545
base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
config: x86_64-randconfig-a012 (https://download.01.org/0day-ci/archive/20220731/202207310431.Rtpq2bqx-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 52cd00cabf479aa7eb6dbb063b7ba41ea57bce9e)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7bce9c0377f5e41fa29f57af40f436a48e2260b1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marek-Vasut/extcon-usbc-tusb320-Factor-out-extcon-into-dedicated-functions/20220731-020545
git checkout 7bce9c0377f5e41fa29f57af40f436a48e2260b1
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/extcon/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/extcon/extcon-usbc-tusb320.c:19: warning: expecting prototype for drivers/extcon/extcon-tusb320.c(). Prototype was for TUSB320_REG8() instead
vim +19 drivers/extcon/extcon-usbc-tusb320.c
18
> 19 #define TUSB320_REG8 0x8
20 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE GENMASK(7, 6)
21 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB 0x0
22 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A 0x1
23 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A 0x2
24 #define TUSB320_REG8_CURRENT_MODE_DETECT GENMASK(5, 4)
25 #define TUSB320_REG8_CURRENT_MODE_DETECT_DEF 0x0
26 #define TUSB320_REG8_CURRENT_MODE_DETECT_MED 0x1
27 #define TUSB320_REG8_CURRENT_MODE_DETECT_ACC 0x2
28 #define TUSB320_REG8_CURRENT_MODE_DETECT_HI 0x3
29 #define TUSB320_REG8_ACCESSORY_CONNECTED GENMASK(3, 2)
30 #define TUSB320_REG8_ACCESSORY_CONNECTED_NONE 0x0
31 #define TUSB320_REG8_ACCESSORY_CONNECTED_AUDIO 0x4
32 #define TUSB320_REG8_ACCESSORY_CONNECTED_ACC 0x5
33 #define TUSB320_REG8_ACCESSORY_CONNECTED_DEBUG 0x6
34 #define TUSB320_REG8_ACTIVE_CABLE_DETECTION BIT(0)
35
Hi Marek,
I love your patch! Perhaps something to improve:
[auto build test WARNING on chanwoo-extcon/extcon-next]
[also build test WARNING on linus/master v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Marek-Vasut/extcon-usbc-tusb320-Factor-out-extcon-into-dedicated-functions/20220731-020545
base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
config: sparc-randconfig-r031-20220731 (https://download.01.org/0day-ci/archive/20220731/202207310423.4ybeL2WB-lkp@intel.com/config)
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/7bce9c0377f5e41fa29f57af40f436a48e2260b1
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Marek-Vasut/extcon-usbc-tusb320-Factor-out-extcon-into-dedicated-functions/20220731-020545
git checkout 7bce9c0377f5e41fa29f57af40f436a48e2260b1
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/extcon/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/extcon/extcon-usbc-tusb320.c:19: warning: expecting prototype for drivers/extcon/extcon-tusb320.c(). Prototype was for TUSB320_REG8() instead
vim +19 drivers/extcon/extcon-usbc-tusb320.c
18
> 19 #define TUSB320_REG8 0x8
20 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE GENMASK(7, 6)
21 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB 0x0
22 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A 0x1
23 #define TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A 0x2
24 #define TUSB320_REG8_CURRENT_MODE_DETECT GENMASK(5, 4)
25 #define TUSB320_REG8_CURRENT_MODE_DETECT_DEF 0x0
26 #define TUSB320_REG8_CURRENT_MODE_DETECT_MED 0x1
27 #define TUSB320_REG8_CURRENT_MODE_DETECT_ACC 0x2
28 #define TUSB320_REG8_CURRENT_MODE_DETECT_HI 0x3
29 #define TUSB320_REG8_ACCESSORY_CONNECTED GENMASK(3, 2)
30 #define TUSB320_REG8_ACCESSORY_CONNECTED_NONE 0x0
31 #define TUSB320_REG8_ACCESSORY_CONNECTED_AUDIO 0x4
32 #define TUSB320_REG8_ACCESSORY_CONNECTED_ACC 0x5
33 #define TUSB320_REG8_ACCESSORY_CONNECTED_DEBUG 0x6
34 #define TUSB320_REG8_ACTIVE_CABLE_DETECTION BIT(0)
35
On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: > The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, > which can expose details collected by the TUSB320 in a far more > precise way than extcon. Since there are existing users in the > kernel and in DT which depend on the extcon interface, keep it > for now. > > Add TYPE-C interface and expose the supported supply current, > direction and connector polarity via the TYPE-C interface. > > Signed-off-by: Marek Vasut <marex@denx.de> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Yassine Oudjana <y.oudjana@protonmail.com> > To: linux-usb@vger.kernel.org > --- > drivers/extcon/Kconfig | 2 +- > drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+), 1 deletion(-) > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index c69d40ae5619a..7684b3afa6304 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -180,7 +180,7 @@ config EXTCON_USBC_CROS_EC > > config EXTCON_USBC_TUSB320 > tristate "TI TUSB320 USB-C extcon support" > - depends on I2C > + depends on I2C && TYPEC > select REGMAP_I2C > help > Say Y here to enable support for USB Type C cable detection extcon > diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c > index aced4bbb455dc..edb8c3f997c91 100644 > --- a/drivers/extcon/extcon-usbc-tusb320.c > +++ b/drivers/extcon/extcon-usbc-tusb320.c > @@ -6,6 +6,7 @@ > * Author: Michael Auchter <michael.auchter@ni.com> > */ > > +#include <linux/bitfield.h> > #include <linux/extcon-provider.h> > #include <linux/i2c.h> > #include <linux/init.h> > @@ -13,6 +14,24 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/regmap.h> > +#include <linux/usb/typec.h> > + > +#define TUSB320_REG8 0x8 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE GENMASK(7, 6) > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB 0x0 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A 0x1 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A 0x2 > +#define TUSB320_REG8_CURRENT_MODE_DETECT GENMASK(5, 4) > +#define TUSB320_REG8_CURRENT_MODE_DETECT_DEF 0x0 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_MED 0x1 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_ACC 0x2 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_HI 0x3 > +#define TUSB320_REG8_ACCESSORY_CONNECTED GENMASK(3, 2) > +#define TUSB320_REG8_ACCESSORY_CONNECTED_NONE 0x0 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_AUDIO 0x4 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_ACC 0x5 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_DEBUG 0x6 > +#define TUSB320_REG8_ACTIVE_CABLE_DETECTION BIT(0) > > #define TUSB320_REG9 0x9 > #define TUSB320_REG9_ATTACHED_STATE_SHIFT 6 > @@ -55,6 +74,10 @@ struct tusb320_priv { > struct extcon_dev *edev; > struct tusb320_ops *ops; > enum tusb320_attached_state state; > + struct typec_port *port; > + struct typec_capability cap; > + enum typec_port_type port_type; > + enum typec_pwr_opmode pwr_opmode; > }; > > static const char * const tusb_attached_states[] = { > @@ -184,6 +207,44 @@ static struct tusb320_ops tusb320l_ops = { > .get_revision = tusb320l_get_revision, > }; > > +static int tusb320_set_adv_pwr_mode(struct tusb320_priv *priv) > +{ > + u8 mode; > + > + if (priv->pwr_opmode == TYPEC_PWR_MODE_USB) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB; > + else if (priv->pwr_opmode == TYPEC_PWR_MODE_1_5A) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A; > + else if (priv->pwr_opmode == TYPEC_PWR_MODE_3_0A) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A; > + else /* No other mode is supported. */ > + return -EINVAL; > + > + return regmap_write_bits(priv->regmap, TUSB320_REG8, > + TUSB320_REG8_CURRENT_MODE_ADVERTISE, > + FIELD_PREP(TUSB320_REG8_CURRENT_MODE_ADVERTISE, > + mode)); > +} > + > +static int tusb320_port_type_set(struct typec_port *port, > + enum typec_port_type type) > +{ > + struct tusb320_priv *priv = typec_get_drvdata(port); > + > + if (type == TYPEC_PORT_SRC) > + return priv->ops->set_mode(priv, TUSB320_MODE_DFP); > + else if (type == TYPEC_PORT_SNK) > + return priv->ops->set_mode(priv, TUSB320_MODE_UFP); > + else if (type == TYPEC_PORT_DRP) > + return priv->ops->set_mode(priv, TUSB320_MODE_DRP); > + else > + return priv->ops->set_mode(priv, TUSB320_MODE_PORT); > +} > + > +static const struct typec_operations tusb320_typec_ops = { > + .port_type_set = tusb320_port_type_set, > +}; > + > static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) > { > int state, polarity; > @@ -211,6 +272,47 @@ static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) > priv->state = state; > } > > +static void tusb320_typec_irq_handler(struct tusb320_priv *priv, u8 reg9) > +{ > + struct typec_port *port = priv->port; > + struct device *dev = priv->dev; > + u8 mode, role, state; > + int ret, reg8; > + bool ori; > + > + ori = reg9 & TUSB320_REG9_CABLE_DIRECTION; > + typec_set_orientation(port, ori ? TYPEC_ORIENTATION_REVERSE : > + TYPEC_ORIENTATION_NORMAL); > + > + state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) & > + TUSB320_REG9_ATTACHED_STATE_MASK; > + if (state == TUSB320_ATTACHED_STATE_DFP) > + role = TYPEC_SOURCE; > + else > + role = TYPEC_SINK; > + > + typec_set_vconn_role(port, role); > + typec_set_pwr_role(port, role); > + typec_set_data_role(port, role == TYPEC_SOURCE ? > + TYPEC_HOST : TYPEC_DEVICE); > + > + ret = regmap_read(priv->regmap, TUSB320_REG8, ®8); > + if (ret) { > + dev_err(dev, "error during reg8 i2c read, ret=%d!\n", ret); > + return; > + } > + > + mode = FIELD_GET(TUSB320_REG8_CURRENT_MODE_DETECT, reg8); > + if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_DEF) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); > + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_MED) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_1_5A); > + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_HI) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_3_0A); > + else /* Charge through accessory */ > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); > +} > + > static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > { > struct tusb320_priv *priv = dev_id; > @@ -225,6 +327,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > return IRQ_NONE; > > tusb320_extcon_irq_handler(priv, reg); > + tusb320_typec_irq_handler(priv, reg); > > regmap_write(priv->regmap, TUSB320_REG9, reg); > > @@ -260,6 +363,58 @@ static int tusb320_extcon_probe(struct tusb320_priv *priv) > return 0; > } > > +static int tusb320_typec_probe(struct i2c_client *client, > + struct tusb320_priv *priv) > +{ > + struct fwnode_handle *connector; > + const char *cap_str; > + int ret; > + > + /* The Type-C connector is optional, for backward compatibility. */ > + connector = device_get_named_child_node(&client->dev, "connector"); > + if (!connector) > + return 0; > + > + /* Type-C connector found. */ > + ret = typec_get_fw_cap(&priv->cap, connector); > + if (ret) > + return ret; > + > + priv->port_type = priv->cap.type; > + > + /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */ > + ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str); > + if (ret) > + return ret; > + > + ret = typec_find_pwr_opmode(cap_str); > + if (ret < 0) > + return ret; > + if (ret == TYPEC_PWR_MODE_PD) > + return -EINVAL; > + > + priv->pwr_opmode = ret; > + > + /* Initialize the hardware with the devicetree settings. */ > + ret = tusb320_set_adv_pwr_mode(priv); > + if (ret) > + return ret; > + > + priv->cap.revision = USB_TYPEC_REV_1_1; > + priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO; > + priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG; > + priv->cap.orientation_aware = true; > + priv->cap.driver_data = priv; > + priv->cap.ops = &tusb320_typec_ops; > + priv->cap.fwnode = connector; > + > + priv->port = typec_register_port(&client->dev, &priv->cap); > + if (IS_ERR(priv->port)) > + return PTR_ERR(priv->port); > + > + return 0; > +} > + > static int tusb320_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -300,6 +455,10 @@ static int tusb320_probe(struct i2c_client *client, > if (ret) > return ret; > > + ret = tusb320_typec_probe(client, priv); > + if (ret) > + return ret; > + > /* update initial state */ > tusb320_irq_handler(client->irq, priv); > > -- > 2.35.1
Hi Marek, On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: > The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, > which can expose details collected by the TUSB320 in a far more > precise way than extcon. Since there are existing users in the > kernel and in DT which depend on the extcon interface, keep it > for now. > > Add TYPE-C interface and expose the supported supply current, > direction and connector polarity via the TYPE-C interface. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Yassine Oudjana <y.oudjana@protonmail.com> > To: linux-usb@vger.kernel.org > --- > drivers/extcon/Kconfig | 2 +- > drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+), 1 deletion(-) Happy to see I'm not the only one that observed this. I wonder if you saw also my previous stab at this: https://lore.kernel.org/linux-usb/20220301132010.115258-1-alvin@pqrs.dk/ I had some issues with the dt-bindings which I could not reconcile, but the basic problem was how to describe a typec accessory mode mux connected to the TUSB320. Perhaps you have a better intuition for how this should look? One thing that is missing from your implementation that we are using on our end is the USB role switch. I set this from the typec driver via usb_role_switch_set_role(). Kind regards, Alvin
On 8/23/22 11:49, Alvin Šipraga wrote: > Hi Marek, Hi, > On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: >> The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, >> which can expose details collected by the TUSB320 in a far more >> precise way than extcon. Since there are existing users in the >> kernel and in DT which depend on the extcon interface, keep it >> for now. >> >> Add TYPE-C interface and expose the supported supply current, >> direction and connector polarity via the TYPE-C interface. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Chanwoo Choi <cw00.choi@samsung.com> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> Cc: Yassine Oudjana <y.oudjana@protonmail.com> >> To: linux-usb@vger.kernel.org >> --- >> drivers/extcon/Kconfig | 2 +- >> drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ >> 2 files changed, 160 insertions(+), 1 deletion(-) > > Happy to see I'm not the only one that observed this. I wonder if you > saw also my previous stab at this: > > https://lore.kernel.org/linux-usb/20220301132010.115258-1-alvin@pqrs.dk/ I have not. > I had some issues with the dt-bindings which I could not reconcile, but > the basic problem was how to describe a typec accessory mode mux > connected to the TUSB320. Perhaps you have a better intuition for how > this should look? > > One thing that is missing from your implementation that we are using on > our end is the USB role switch. I set this from the typec driver via > usb_role_switch_set_role(). I only use this chip to detect charger type (and cable polarity), the device where this is integrated is always peripheral and cannot charge other devices or become host. But I think those aforementioned requirements could be extended on top of this patch, can they not ? I recall I looked at least at the direction detection and that could be added easily. I have no way of testing any of that functionality, so I didn't add them as part of the patch.
On 8/2/22 09:40, Heikki Krogerus wrote: > On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: >> The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, >> which can expose details collected by the TUSB320 in a far more >> precise way than extcon. Since there are existing users in the >> kernel and in DT which depend on the extcon interface, keep it >> for now. >> >> Add TYPE-C interface and expose the supported supply current, >> direction and connector polarity via the TYPE-C interface. >> >> Signed-off-by: Marek Vasut <marex@denx.de> > > Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> I wonder, can this now be applied ? The LKP robot is picking up on macro name, i.e. the report seems bogus ?
On Tue, Aug 23, 2022 at 12:39:28PM +0200, Marek Vasut wrote: > On 8/23/22 11:49, Alvin Šipraga wrote: > > Hi Marek, > > Hi, > > > On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: > > > The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, > > > which can expose details collected by the TUSB320 in a far more > > > precise way than extcon. Since there are existing users in the > > > kernel and in DT which depend on the extcon interface, keep it > > > for now. > > > > > > Add TYPE-C interface and expose the supported supply current, > > > direction and connector polarity via the TYPE-C interface. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > --- > > > Cc: Chanwoo Choi <cw00.choi@samsung.com> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Cc: Yassine Oudjana <y.oudjana@protonmail.com> > > > To: linux-usb@vger.kernel.org > > > --- > > > drivers/extcon/Kconfig | 2 +- > > > drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ > > > 2 files changed, 160 insertions(+), 1 deletion(-) > > > > Happy to see I'm not the only one that observed this. I wonder if you > > saw also my previous stab at this: > > > > https://lore.kernel.org/linux-usb/20220301132010.115258-1-alvin@pqrs.dk/ > > I have not. > > > I had some issues with the dt-bindings which I could not reconcile, but > > the basic problem was how to describe a typec accessory mode mux > > connected to the TUSB320. Perhaps you have a better intuition for how > > this should look? > > > > One thing that is missing from your implementation that we are using on > > our end is the USB role switch. I set this from the typec driver via > > usb_role_switch_set_role(). > > I only use this chip to detect charger type (and cable polarity), the device > where this is integrated is always peripheral and cannot charge other > devices or become host. > > But I think those aforementioned requirements could be extended on top of > this patch, can they not ? I recall I looked at least at the direction > detection and that could be added easily. I have no way of testing any of > that functionality, so I didn't add them as part of the patch. Sure - if your patch gets merged then I'll just extend it. Fair enough that you cannot test on your board. To that end, you can add: Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Thanks! Kind regards, Alvin
On 8/23/22 14:57, Alvin Šipraga wrote: > On Tue, Aug 23, 2022 at 12:39:28PM +0200, Marek Vasut wrote: >> On 8/23/22 11:49, Alvin Šipraga wrote: >>> Hi Marek, >> >> Hi, >> >>> On Sat, Jul 30, 2022 at 08:05:00PM +0200, Marek Vasut wrote: >>>> The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, >>>> which can expose details collected by the TUSB320 in a far more >>>> precise way than extcon. Since there are existing users in the >>>> kernel and in DT which depend on the extcon interface, keep it >>>> for now. >>>> >>>> Add TYPE-C interface and expose the supported supply current, >>>> direction and connector polarity via the TYPE-C interface. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> --- >>>> Cc: Chanwoo Choi <cw00.choi@samsung.com> >>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> >>>> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> >>>> Cc: Yassine Oudjana <y.oudjana@protonmail.com> >>>> To: linux-usb@vger.kernel.org >>>> --- >>>> drivers/extcon/Kconfig | 2 +- >>>> drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ >>>> 2 files changed, 160 insertions(+), 1 deletion(-) >>> >>> Happy to see I'm not the only one that observed this. I wonder if you >>> saw also my previous stab at this: >>> >>> https://lore.kernel.org/linux-usb/20220301132010.115258-1-alvin@pqrs.dk/ >> >> I have not. >> >>> I had some issues with the dt-bindings which I could not reconcile, but >>> the basic problem was how to describe a typec accessory mode mux >>> connected to the TUSB320. Perhaps you have a better intuition for how >>> this should look? >>> >>> One thing that is missing from your implementation that we are using on >>> our end is the USB role switch. I set this from the typec driver via >>> usb_role_switch_set_role(). >> >> I only use this chip to detect charger type (and cable polarity), the device >> where this is integrated is always peripheral and cannot charge other >> devices or become host. >> >> But I think those aforementioned requirements could be extended on top of >> this patch, can they not ? I recall I looked at least at the direction >> detection and that could be added easily. I have no way of testing any of >> that functionality, so I didn't add them as part of the patch. > > Sure - if your patch gets merged then I'll just extend it. Fair enough > that you cannot test on your board. > > To that end, you can add: > > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> Thanks. If you plan to submit anything on top, I might be able to test at least the charger detect and plug orientation still works ... but that's probably something you can also test on your own, that's the easy part of the USB-C.
On 22. 7. 31. 03:05, Marek Vasut wrote: > The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, > which can expose details collected by the TUSB320 in a far more > precise way than extcon. Since there are existing users in the > kernel and in DT which depend on the extcon interface, keep it > for now. > > Add TYPE-C interface and expose the supported supply current, > direction and connector polarity via the TYPE-C interface. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Yassine Oudjana <y.oudjana@protonmail.com> > To: linux-usb@vger.kernel.org > --- > drivers/extcon/Kconfig | 2 +- > drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ > 2 files changed, 160 insertions(+), 1 deletion(-) > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index c69d40ae5619a..7684b3afa6304 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -180,7 +180,7 @@ config EXTCON_USBC_CROS_EC > > config EXTCON_USBC_TUSB320 > tristate "TI TUSB320 USB-C extcon support" > - depends on I2C > + depends on I2C && TYPEC > select REGMAP_I2C > help > Say Y here to enable support for USB Type C cable detection extcon > diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c > index aced4bbb455dc..edb8c3f997c91 100644 > --- a/drivers/extcon/extcon-usbc-tusb320.c > +++ b/drivers/extcon/extcon-usbc-tusb320.c > @@ -6,6 +6,7 @@ > * Author: Michael Auchter <michael.auchter@ni.com> > */ > > +#include <linux/bitfield.h> > #include <linux/extcon-provider.h> > #include <linux/i2c.h> > #include <linux/init.h> > @@ -13,6 +14,24 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/regmap.h> > +#include <linux/usb/typec.h> > + > +#define TUSB320_REG8 0x8 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE GENMASK(7, 6) > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB 0x0 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A 0x1 > +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A 0x2 > +#define TUSB320_REG8_CURRENT_MODE_DETECT GENMASK(5, 4) > +#define TUSB320_REG8_CURRENT_MODE_DETECT_DEF 0x0 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_MED 0x1 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_ACC 0x2 > +#define TUSB320_REG8_CURRENT_MODE_DETECT_HI 0x3 > +#define TUSB320_REG8_ACCESSORY_CONNECTED GENMASK(3, 2) > +#define TUSB320_REG8_ACCESSORY_CONNECTED_NONE 0x0 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_AUDIO 0x4 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_ACC 0x5 > +#define TUSB320_REG8_ACCESSORY_CONNECTED_DEBUG 0x6 > +#define TUSB320_REG8_ACTIVE_CABLE_DETECTION BIT(0) > > #define TUSB320_REG9 0x9 > #define TUSB320_REG9_ATTACHED_STATE_SHIFT 6 > @@ -55,6 +74,10 @@ struct tusb320_priv { > struct extcon_dev *edev; > struct tusb320_ops *ops; > enum tusb320_attached_state state; > + struct typec_port *port; > + struct typec_capability cap; > + enum typec_port_type port_type; > + enum typec_pwr_opmode pwr_opmode; > }; > > static const char * const tusb_attached_states[] = { > @@ -184,6 +207,44 @@ static struct tusb320_ops tusb320l_ops = { > .get_revision = tusb320l_get_revision, > }; > > +static int tusb320_set_adv_pwr_mode(struct tusb320_priv *priv) > +{ > + u8 mode; > + > + if (priv->pwr_opmode == TYPEC_PWR_MODE_USB) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB; > + else if (priv->pwr_opmode == TYPEC_PWR_MODE_1_5A) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A; > + else if (priv->pwr_opmode == TYPEC_PWR_MODE_3_0A) > + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A; > + else /* No other mode is supported. */ > + return -EINVAL; > + > + return regmap_write_bits(priv->regmap, TUSB320_REG8, > + TUSB320_REG8_CURRENT_MODE_ADVERTISE, > + FIELD_PREP(TUSB320_REG8_CURRENT_MODE_ADVERTISE, > + mode)); > +} > + > +static int tusb320_port_type_set(struct typec_port *port, > + enum typec_port_type type) > +{ > + struct tusb320_priv *priv = typec_get_drvdata(port); > + > + if (type == TYPEC_PORT_SRC) > + return priv->ops->set_mode(priv, TUSB320_MODE_DFP); > + else if (type == TYPEC_PORT_SNK) > + return priv->ops->set_mode(priv, TUSB320_MODE_UFP); > + else if (type == TYPEC_PORT_DRP) > + return priv->ops->set_mode(priv, TUSB320_MODE_DRP); > + else > + return priv->ops->set_mode(priv, TUSB320_MODE_PORT); > +} > + > +static const struct typec_operations tusb320_typec_ops = { > + .port_type_set = tusb320_port_type_set, > +}; > + > static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) > { > int state, polarity; > @@ -211,6 +272,47 @@ static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) > priv->state = state; > } > > +static void tusb320_typec_irq_handler(struct tusb320_priv *priv, u8 reg9) > +{ > + struct typec_port *port = priv->port; > + struct device *dev = priv->dev; > + u8 mode, role, state; > + int ret, reg8; > + bool ori; > + > + ori = reg9 & TUSB320_REG9_CABLE_DIRECTION; > + typec_set_orientation(port, ori ? TYPEC_ORIENTATION_REVERSE : > + TYPEC_ORIENTATION_NORMAL); > + > + state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) & > + TUSB320_REG9_ATTACHED_STATE_MASK; > + if (state == TUSB320_ATTACHED_STATE_DFP) > + role = TYPEC_SOURCE; > + else > + role = TYPEC_SINK; > + > + typec_set_vconn_role(port, role); > + typec_set_pwr_role(port, role); > + typec_set_data_role(port, role == TYPEC_SOURCE ? > + TYPEC_HOST : TYPEC_DEVICE); > + > + ret = regmap_read(priv->regmap, TUSB320_REG8, ®8); > + if (ret) { > + dev_err(dev, "error during reg8 i2c read, ret=%d!\n", ret); > + return; > + } > + > + mode = FIELD_GET(TUSB320_REG8_CURRENT_MODE_DETECT, reg8); > + if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_DEF) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); > + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_MED) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_1_5A); > + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_HI) > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_3_0A); > + else /* Charge through accessory */ > + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); > +} > + > static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > { > struct tusb320_priv *priv = dev_id; > @@ -225,6 +327,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > return IRQ_NONE; > > tusb320_extcon_irq_handler(priv, reg); > + tusb320_typec_irq_handler(priv, reg); > > regmap_write(priv->regmap, TUSB320_REG9, reg); > > @@ -260,6 +363,58 @@ static int tusb320_extcon_probe(struct tusb320_priv *priv) > return 0; > } > > +static int tusb320_typec_probe(struct i2c_client *client, > + struct tusb320_priv *priv) > +{ > + struct fwnode_handle *connector; > + const char *cap_str; > + int ret; > + > + /* The Type-C connector is optional, for backward compatibility. */ > + connector = device_get_named_child_node(&client->dev, "connector"); > + if (!connector) > + return 0; > + > + /* Type-C connector found. */ > + ret = typec_get_fw_cap(&priv->cap, connector); > + if (ret) > + return ret; > + > + priv->port_type = priv->cap.type; > + > + /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */ > + ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str); > + if (ret) > + return ret; > + > + ret = typec_find_pwr_opmode(cap_str); > + if (ret < 0) > + return ret; > + if (ret == TYPEC_PWR_MODE_PD) > + return -EINVAL; > + > + priv->pwr_opmode = ret; > + > + /* Initialize the hardware with the devicetree settings. */ > + ret = tusb320_set_adv_pwr_mode(priv); > + if (ret) > + return ret; > + > + priv->cap.revision = USB_TYPEC_REV_1_1; > + priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO; > + priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG; > + priv->cap.orientation_aware = true; > + priv->cap.driver_data = priv; > + priv->cap.ops = &tusb320_typec_ops; > + priv->cap.fwnode = connector; > + > + priv->port = typec_register_port(&client->dev, &priv->cap); > + if (IS_ERR(priv->port)) > + return PTR_ERR(priv->port); > + > + return 0; > +} > + > static int tusb320_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -300,6 +455,10 @@ static int tusb320_probe(struct i2c_client *client, > if (ret) > return ret; > > + ret = tusb320_typec_probe(client, priv); > + if (ret) > + return ret; > + > /* update initial state */ > tusb320_irq_handler(client->irq, priv); > Applied it. Thanks.
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index c69d40ae5619a..7684b3afa6304 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -180,7 +180,7 @@ config EXTCON_USBC_CROS_EC config EXTCON_USBC_TUSB320 tristate "TI TUSB320 USB-C extcon support" - depends on I2C + depends on I2C && TYPEC select REGMAP_I2C help Say Y here to enable support for USB Type C cable detection extcon diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c index aced4bbb455dc..edb8c3f997c91 100644 --- a/drivers/extcon/extcon-usbc-tusb320.c +++ b/drivers/extcon/extcon-usbc-tusb320.c @@ -6,6 +6,7 @@ * Author: Michael Auchter <michael.auchter@ni.com> */ +#include <linux/bitfield.h> #include <linux/extcon-provider.h> #include <linux/i2c.h> #include <linux/init.h> @@ -13,6 +14,24 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/regmap.h> +#include <linux/usb/typec.h> + +#define TUSB320_REG8 0x8 +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE GENMASK(7, 6) +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB 0x0 +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A 0x1 +#define TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A 0x2 +#define TUSB320_REG8_CURRENT_MODE_DETECT GENMASK(5, 4) +#define TUSB320_REG8_CURRENT_MODE_DETECT_DEF 0x0 +#define TUSB320_REG8_CURRENT_MODE_DETECT_MED 0x1 +#define TUSB320_REG8_CURRENT_MODE_DETECT_ACC 0x2 +#define TUSB320_REG8_CURRENT_MODE_DETECT_HI 0x3 +#define TUSB320_REG8_ACCESSORY_CONNECTED GENMASK(3, 2) +#define TUSB320_REG8_ACCESSORY_CONNECTED_NONE 0x0 +#define TUSB320_REG8_ACCESSORY_CONNECTED_AUDIO 0x4 +#define TUSB320_REG8_ACCESSORY_CONNECTED_ACC 0x5 +#define TUSB320_REG8_ACCESSORY_CONNECTED_DEBUG 0x6 +#define TUSB320_REG8_ACTIVE_CABLE_DETECTION BIT(0) #define TUSB320_REG9 0x9 #define TUSB320_REG9_ATTACHED_STATE_SHIFT 6 @@ -55,6 +74,10 @@ struct tusb320_priv { struct extcon_dev *edev; struct tusb320_ops *ops; enum tusb320_attached_state state; + struct typec_port *port; + struct typec_capability cap; + enum typec_port_type port_type; + enum typec_pwr_opmode pwr_opmode; }; static const char * const tusb_attached_states[] = { @@ -184,6 +207,44 @@ static struct tusb320_ops tusb320l_ops = { .get_revision = tusb320l_get_revision, }; +static int tusb320_set_adv_pwr_mode(struct tusb320_priv *priv) +{ + u8 mode; + + if (priv->pwr_opmode == TYPEC_PWR_MODE_USB) + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_USB; + else if (priv->pwr_opmode == TYPEC_PWR_MODE_1_5A) + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_15A; + else if (priv->pwr_opmode == TYPEC_PWR_MODE_3_0A) + mode = TUSB320_REG8_CURRENT_MODE_ADVERTISE_30A; + else /* No other mode is supported. */ + return -EINVAL; + + return regmap_write_bits(priv->regmap, TUSB320_REG8, + TUSB320_REG8_CURRENT_MODE_ADVERTISE, + FIELD_PREP(TUSB320_REG8_CURRENT_MODE_ADVERTISE, + mode)); +} + +static int tusb320_port_type_set(struct typec_port *port, + enum typec_port_type type) +{ + struct tusb320_priv *priv = typec_get_drvdata(port); + + if (type == TYPEC_PORT_SRC) + return priv->ops->set_mode(priv, TUSB320_MODE_DFP); + else if (type == TYPEC_PORT_SNK) + return priv->ops->set_mode(priv, TUSB320_MODE_UFP); + else if (type == TYPEC_PORT_DRP) + return priv->ops->set_mode(priv, TUSB320_MODE_DRP); + else + return priv->ops->set_mode(priv, TUSB320_MODE_PORT); +} + +static const struct typec_operations tusb320_typec_ops = { + .port_type_set = tusb320_port_type_set, +}; + static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) { int state, polarity; @@ -211,6 +272,47 @@ static void tusb320_extcon_irq_handler(struct tusb320_priv *priv, u8 reg) priv->state = state; } +static void tusb320_typec_irq_handler(struct tusb320_priv *priv, u8 reg9) +{ + struct typec_port *port = priv->port; + struct device *dev = priv->dev; + u8 mode, role, state; + int ret, reg8; + bool ori; + + ori = reg9 & TUSB320_REG9_CABLE_DIRECTION; + typec_set_orientation(port, ori ? TYPEC_ORIENTATION_REVERSE : + TYPEC_ORIENTATION_NORMAL); + + state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) & + TUSB320_REG9_ATTACHED_STATE_MASK; + if (state == TUSB320_ATTACHED_STATE_DFP) + role = TYPEC_SOURCE; + else + role = TYPEC_SINK; + + typec_set_vconn_role(port, role); + typec_set_pwr_role(port, role); + typec_set_data_role(port, role == TYPEC_SOURCE ? + TYPEC_HOST : TYPEC_DEVICE); + + ret = regmap_read(priv->regmap, TUSB320_REG8, ®8); + if (ret) { + dev_err(dev, "error during reg8 i2c read, ret=%d!\n", ret); + return; + } + + mode = FIELD_GET(TUSB320_REG8_CURRENT_MODE_DETECT, reg8); + if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_DEF) + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_MED) + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_1_5A); + else if (mode == TUSB320_REG8_CURRENT_MODE_DETECT_HI) + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_3_0A); + else /* Charge through accessory */ + typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); +} + static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) { struct tusb320_priv *priv = dev_id; @@ -225,6 +327,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) return IRQ_NONE; tusb320_extcon_irq_handler(priv, reg); + tusb320_typec_irq_handler(priv, reg); regmap_write(priv->regmap, TUSB320_REG9, reg); @@ -260,6 +363,58 @@ static int tusb320_extcon_probe(struct tusb320_priv *priv) return 0; } +static int tusb320_typec_probe(struct i2c_client *client, + struct tusb320_priv *priv) +{ + struct fwnode_handle *connector; + const char *cap_str; + int ret; + + /* The Type-C connector is optional, for backward compatibility. */ + connector = device_get_named_child_node(&client->dev, "connector"); + if (!connector) + return 0; + + /* Type-C connector found. */ + ret = typec_get_fw_cap(&priv->cap, connector); + if (ret) + return ret; + + priv->port_type = priv->cap.type; + + /* This goes into register 0x8 field CURRENT_MODE_ADVERTISE */ + ret = fwnode_property_read_string(connector, "typec-power-opmode", &cap_str); + if (ret) + return ret; + + ret = typec_find_pwr_opmode(cap_str); + if (ret < 0) + return ret; + if (ret == TYPEC_PWR_MODE_PD) + return -EINVAL; + + priv->pwr_opmode = ret; + + /* Initialize the hardware with the devicetree settings. */ + ret = tusb320_set_adv_pwr_mode(priv); + if (ret) + return ret; + + priv->cap.revision = USB_TYPEC_REV_1_1; + priv->cap.accessory[0] = TYPEC_ACCESSORY_AUDIO; + priv->cap.accessory[1] = TYPEC_ACCESSORY_DEBUG; + priv->cap.orientation_aware = true; + priv->cap.driver_data = priv; + priv->cap.ops = &tusb320_typec_ops; + priv->cap.fwnode = connector; + + priv->port = typec_register_port(&client->dev, &priv->cap); + if (IS_ERR(priv->port)) + return PTR_ERR(priv->port); + + return 0; +} + static int tusb320_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -300,6 +455,10 @@ static int tusb320_probe(struct i2c_client *client, if (ret) return ret; + ret = tusb320_typec_probe(client, priv); + if (ret) + return ret; + /* update initial state */ tusb320_irq_handler(client->irq, priv);
The TI TUSB320 seems like a better fit for USB TYPE-C subsystem, which can expose details collected by the TUSB320 in a far more precise way than extcon. Since there are existing users in the kernel and in DT which depend on the extcon interface, keep it for now. Add TYPE-C interface and expose the supported supply current, direction and connector polarity via the TYPE-C interface. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Chanwoo Choi <cw00.choi@samsung.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: Yassine Oudjana <y.oudjana@protonmail.com> To: linux-usb@vger.kernel.org --- drivers/extcon/Kconfig | 2 +- drivers/extcon/extcon-usbc-tusb320.c | 159 +++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 1 deletion(-)