Re: Re: [PATCH v2 1/2] extcon: add driver for TI TUSB320
On Wed, Oct 14, 2020 at 12:51:26PM +0900, Chanwoo Choi wrote: > Hi, > > Looks good to me. I add some comment. Please check them. Thanks for the prompt review! I'll address these and send out a v3. Cheers, Michael > On 10/12/20 11:47 PM, Michael Auchter wrote: > > This patch adds an extcon driver for the TI TUSB320 USB Type-C device. > > This can be used to detect whether the port is configured as a > > downstream or upstream facing port. > > > > Signed-off-by: Michael Auchter > > --- > > Changes since v1: > > - Drop license text that's redundant with SPDX tag > > - Cleanup, sort list of includes > > - Add additional register defines > > - Switch to use regmap API > > - Fix Kconfig to depend on I2C, not GPIOLIB > > > > drivers/extcon/Kconfig | 8 ++ > > drivers/extcon/Makefile | 1 + > > drivers/extcon/extcon-usbc-tusb320.c | 191 +++ > > 3 files changed, 200 insertions(+) > > create mode 100644 drivers/extcon/extcon-usbc-tusb320.c > > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > > index aac507bff135..af58ebca2bf6 100644 > > --- a/drivers/extcon/Kconfig > > +++ b/drivers/extcon/Kconfig > > @@ -186,4 +186,12 @@ config EXTCON_USBC_CROS_EC > > Say Y here to enable USB Type C cable detection extcon support when > > using Chrome OS EC based USB Type-C ports. > > > > +config EXTCON_USBC_TUSB320 > > + tristate "TI TUSB320 USB-C extcon support" > > + depends on I2C > > + select REGMAP_I2C > > + help > > + Say Y here to enable support for USB Type C cable detection extcon > > + support using a TUSB320. > > + > > endif > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > > index 52096fd8a216..fe10a1b7d18b 100644 > > --- a/drivers/extcon/Makefile > > +++ b/drivers/extcon/Makefile > > @@ -25,3 +25,4 @@ obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o > > obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o > > obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o > > obj-$(CONFIG_EXTCON_USBC_CROS_EC) += extcon-usbc-cros-ec.o > > +obj-$(CONFIG_EXTCON_USBC_TUSB320) += extcon-usbc-tusb320.o > > diff --git a/drivers/extcon/extcon-usbc-tusb320.c > > b/drivers/extcon/extcon-usbc-tusb320.c > > new file mode 100644 > > index ..93f1843ca89b > > --- /dev/null > > +++ b/drivers/extcon/extcon-usbc-tusb320.c > > @@ -0,0 +1,191 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/** > > + * drivers/extcon/extcon-tusb320.c - TUSB320 extcon driver > > + * > > + * Copyright (C) 2020 National Instruments Corporation > > + * Author: Michael Auchter > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > irq.h doesn't be needed. Better to remove irq.h. > > > +#include > > +#include > > +#include > > + > > +#define TUSB320_REG9 0x9 > > +#define TUSB320_REG9_ATTACHED_STATE_SHIFT 6 > > +#define TUSB320_REG9_ATTACHED_STATE_MASK 0x3 > > +#define TUSB320_REG9_CABLE_DIRECTION BIT(5) > > +#define TUSB320_REG9_INTERRUPT_STATUS BIT(4) > > +#define TUSB320_ATTACHED_STATE_NONE0x0 > > +#define TUSB320_ATTACHED_STATE_DFP 0x1 > > +#define TUSB320_ATTACHED_STATE_UFP 0x2 > > +#define TUSB320_ATTACHED_STATE_ACC 0x3 > > Above definition contain the 'space' for indentation. > Please edit it as following: > > > -#define TUSB320_ATTACHED_STATE_DFP 0x1 > -#define TUSB320_ATTACHED_STATE_UFP 0x2 > -#define TUSB320_ATTACHED_STATE_ACC 0x3 > +#define TUSB320_ATTACHED_STATE_DFP 0x1 > +#define TUSB320_ATTACHED_STATE_UFP 0x2 > +#define TUSB320_ATTACHED_STATE_ACC 0x3 > > > > + > > +struct tusb320_priv { > > + struct device *dev; > > + struct regmap *regmap; > > + struct extcon_dev *edev; > > +}; > > + > > +static const char * const tusb_attached_states[] = { > > + [TUSB320_ATTACHED_STATE_NONE] = "not attached", > > + [TUSB320_ATTACHED_STATE_DFP] = "downstream facing port", > > + [TUSB320_ATTACHED_STATE_UFP] = "upstream facing port", > > + [TUSB320_ATTACHED_STATE_ACC] = "accessory", > > +}; > > + > > +static const unsigned int tusb320_extcon_cable[] = { > > + EXTCON_USB, > > + EXTCON_USB_HOST, > > + EXTCON_NONE, > > +}; > > + > > +static int tusb320_check_signature(struct tusb320_priv *priv) > > +{ > > + static const char sig[] = { '\0', 'T', 'U', 'S', 'B', '3', '2', '0' }; > > + unsigned val; > > + int i, ret; > > + > > + for (i = 0; i < sizeof(sig); i++) { > > + ret = regmap_read(priv->regmap, sizeof(sig) - 1 - i, ); > > + if (ret < 0) > > + return ret; > > + if (val != sig[i]) { > > + dev_err(priv->dev, "signature mismatch!\n"); > > + return -ENODEV; > > + } > > + } > > + > > + return 0; > > +} > > + > > +static irqreturn_t tusb320_irq_handler(int
Re: [PATCH v2 1/2] extcon: add driver for TI TUSB320
Hi, Looks good to me. I add some comment. Please check them. On 10/12/20 11:47 PM, Michael Auchter wrote: > This patch adds an extcon driver for the TI TUSB320 USB Type-C device. > This can be used to detect whether the port is configured as a > downstream or upstream facing port. > > Signed-off-by: Michael Auchter > --- > Changes since v1: > - Drop license text that's redundant with SPDX tag > - Cleanup, sort list of includes > - Add additional register defines > - Switch to use regmap API > - Fix Kconfig to depend on I2C, not GPIOLIB > > drivers/extcon/Kconfig | 8 ++ > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-usbc-tusb320.c | 191 +++ > 3 files changed, 200 insertions(+) > create mode 100644 drivers/extcon/extcon-usbc-tusb320.c > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index aac507bff135..af58ebca2bf6 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -186,4 +186,12 @@ config EXTCON_USBC_CROS_EC > Say Y here to enable USB Type C cable detection extcon support when > using Chrome OS EC based USB Type-C ports. > > +config EXTCON_USBC_TUSB320 > + tristate "TI TUSB320 USB-C extcon support" > + depends on I2C > + select REGMAP_I2C > + help > + Say Y here to enable support for USB Type C cable detection extcon > + support using a TUSB320. > + > endif > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index 52096fd8a216..fe10a1b7d18b 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -25,3 +25,4 @@ obj-$(CONFIG_EXTCON_RT8973A)+= extcon-rt8973a.o > obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o > obj-$(CONFIG_EXTCON_USB_GPIO)+= extcon-usb-gpio.o > obj-$(CONFIG_EXTCON_USBC_CROS_EC) += extcon-usbc-cros-ec.o > +obj-$(CONFIG_EXTCON_USBC_TUSB320) += extcon-usbc-tusb320.o > diff --git a/drivers/extcon/extcon-usbc-tusb320.c > b/drivers/extcon/extcon-usbc-tusb320.c > new file mode 100644 > index ..93f1843ca89b > --- /dev/null > +++ b/drivers/extcon/extcon-usbc-tusb320.c > @@ -0,0 +1,191 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/** > + * drivers/extcon/extcon-tusb320.c - TUSB320 extcon driver > + * > + * Copyright (C) 2020 National Instruments Corporation > + * Author: Michael Auchter > + */ > + > +#include > +#include > +#include > +#include > +#include irq.h doesn't be needed. Better to remove irq.h. > +#include > +#include > +#include > + > +#define TUSB320_REG9 0x9 > +#define TUSB320_REG9_ATTACHED_STATE_SHIFT6 > +#define TUSB320_REG9_ATTACHED_STATE_MASK 0x3 > +#define TUSB320_REG9_CABLE_DIRECTION BIT(5) > +#define TUSB320_REG9_INTERRUPT_STATUSBIT(4) > +#define TUSB320_ATTACHED_STATE_NONE 0x0 > +#define TUSB320_ATTACHED_STATE_DFP 0x1 > +#define TUSB320_ATTACHED_STATE_UFP 0x2 > +#define TUSB320_ATTACHED_STATE_ACC 0x3 Above definition contain the 'space' for indentation. Please edit it as following: -#define TUSB320_ATTACHED_STATE_DFP 0x1 -#define TUSB320_ATTACHED_STATE_UFP 0x2 -#define TUSB320_ATTACHED_STATE_ACC 0x3 +#define TUSB320_ATTACHED_STATE_DFP 0x1 +#define TUSB320_ATTACHED_STATE_UFP 0x2 +#define TUSB320_ATTACHED_STATE_ACC 0x3 > + > +struct tusb320_priv { > + struct device *dev; > + struct regmap *regmap; > + struct extcon_dev *edev; > +}; > + > +static const char * const tusb_attached_states[] = { > + [TUSB320_ATTACHED_STATE_NONE] = "not attached", > + [TUSB320_ATTACHED_STATE_DFP] = "downstream facing port", > + [TUSB320_ATTACHED_STATE_UFP] = "upstream facing port", > + [TUSB320_ATTACHED_STATE_ACC] = "accessory", > +}; > + > +static const unsigned int tusb320_extcon_cable[] = { > + EXTCON_USB, > + EXTCON_USB_HOST, > + EXTCON_NONE, > +}; > + > +static int tusb320_check_signature(struct tusb320_priv *priv) > +{ > + static const char sig[] = { '\0', 'T', 'U', 'S', 'B', '3', '2', '0' }; > + unsigned val; > + int i, ret; > + > + for (i = 0; i < sizeof(sig); i++) { > + ret = regmap_read(priv->regmap, sizeof(sig) - 1 - i, ); > + if (ret < 0) > + return ret; > + if (val != sig[i]) { > + dev_err(priv->dev, "signature mismatch!\n"); > + return -ENODEV; > + } > + } > + > + return 0; > +} > + > +static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > +{ > + struct tusb320_priv *priv = dev_id; > + int state, polarity; > + unsigned reg; > + > + if (regmap_read(priv->regmap, TUSB320_REG9, )) { > + dev_err(priv->dev, "error during i2c read!\n"); > + return IRQ_NONE; > + } > + > + if (!(reg & TUSB320_REG9_INTERRUPT_STATUS)) > + return IRQ_NONE; > + > + state
Re: [PATCH v2 1/2] extcon: add driver for TI TUSB320
Hi Michael, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on chanwoo-extcon/extcon-next] [also build test WARNING on v5.9 next-20201012] [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] url: https://github.com/0day-ci/linux/commits/Michael-Auchter/extcon-add-driver-for-TI-TUSB320/20201012-232733 base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next config: s390-randconfig-r022-20201013 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a324d8f964bf421fa7d8b882b0f64ead28c4149c) 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 # install s390 cross compiling tool for clang build # apt-get install binutils-s390x-linux-gnu # https://github.com/0day-ci/linux/commit/5065d684df89cf11f57743f3c1440e6bd09054ea git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Michael-Auchter/extcon-add-driver-for-TI-TUSB320/20201012-232733 git checkout 5065d684df89cf11f57743f3c1440e6bd09054ea # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:19:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x00ffUL) << 24) |\ ^ In file included from drivers/extcon/extcon-usbc-tusb320.c:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:20:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0xff00UL) << 8) |\ ^ In file included from drivers/extcon/extcon-usbc-tusb320.c:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^ include/uapi/linux/swab.h:119:21: note: expanded from macro '__swab32' ___constant_swab32(x) : \ ^ include/uapi/linux/swab.h:21:12: note: expanded from macro '___constant_swab32' (((__u32)(x) & (__u32)0x00ffUL) >> 8) |\ ^ In file included from drivers/extcon/extcon-usbc-tusb320.c:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/s390/include/asm/io.h:72: include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr)); ~~ ^ include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x)) ^
[PATCH v2 1/2] extcon: add driver for TI TUSB320
This patch adds an extcon driver for the TI TUSB320 USB Type-C device. This can be used to detect whether the port is configured as a downstream or upstream facing port. Signed-off-by: Michael Auchter --- Changes since v1: - Drop license text that's redundant with SPDX tag - Cleanup, sort list of includes - Add additional register defines - Switch to use regmap API - Fix Kconfig to depend on I2C, not GPIOLIB drivers/extcon/Kconfig | 8 ++ drivers/extcon/Makefile | 1 + drivers/extcon/extcon-usbc-tusb320.c | 191 +++ 3 files changed, 200 insertions(+) create mode 100644 drivers/extcon/extcon-usbc-tusb320.c diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index aac507bff135..af58ebca2bf6 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -186,4 +186,12 @@ config EXTCON_USBC_CROS_EC Say Y here to enable USB Type C cable detection extcon support when using Chrome OS EC based USB Type-C ports. +config EXTCON_USBC_TUSB320 + tristate "TI TUSB320 USB-C extcon support" + depends on I2C + select REGMAP_I2C + help + Say Y here to enable support for USB Type C cable detection extcon + support using a TUSB320. + endif diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index 52096fd8a216..fe10a1b7d18b 100644 --- a/drivers/extcon/Makefile +++ b/drivers/extcon/Makefile @@ -25,3 +25,4 @@ obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o obj-$(CONFIG_EXTCON_SM5502)+= extcon-sm5502.o obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o obj-$(CONFIG_EXTCON_USBC_CROS_EC) += extcon-usbc-cros-ec.o +obj-$(CONFIG_EXTCON_USBC_TUSB320) += extcon-usbc-tusb320.o diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c new file mode 100644 index ..93f1843ca89b --- /dev/null +++ b/drivers/extcon/extcon-usbc-tusb320.c @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-2.0 +/** + * drivers/extcon/extcon-tusb320.c - TUSB320 extcon driver + * + * Copyright (C) 2020 National Instruments Corporation + * Author: Michael Auchter + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#define TUSB320_REG9 0x9 +#define TUSB320_REG9_ATTACHED_STATE_SHIFT 6 +#define TUSB320_REG9_ATTACHED_STATE_MASK 0x3 +#define TUSB320_REG9_CABLE_DIRECTION BIT(5) +#define TUSB320_REG9_INTERRUPT_STATUS BIT(4) +#define TUSB320_ATTACHED_STATE_NONE0x0 +#define TUSB320_ATTACHED_STATE_DFP 0x1 +#define TUSB320_ATTACHED_STATE_UFP 0x2 +#define TUSB320_ATTACHED_STATE_ACC 0x3 + +struct tusb320_priv { + struct device *dev; + struct regmap *regmap; + struct extcon_dev *edev; +}; + +static const char * const tusb_attached_states[] = { + [TUSB320_ATTACHED_STATE_NONE] = "not attached", + [TUSB320_ATTACHED_STATE_DFP] = "downstream facing port", + [TUSB320_ATTACHED_STATE_UFP] = "upstream facing port", + [TUSB320_ATTACHED_STATE_ACC] = "accessory", +}; + +static const unsigned int tusb320_extcon_cable[] = { + EXTCON_USB, + EXTCON_USB_HOST, + EXTCON_NONE, +}; + +static int tusb320_check_signature(struct tusb320_priv *priv) +{ + static const char sig[] = { '\0', 'T', 'U', 'S', 'B', '3', '2', '0' }; + unsigned val; + int i, ret; + + for (i = 0; i < sizeof(sig); i++) { + ret = regmap_read(priv->regmap, sizeof(sig) - 1 - i, ); + if (ret < 0) + return ret; + if (val != sig[i]) { + dev_err(priv->dev, "signature mismatch!\n"); + return -ENODEV; + } + } + + return 0; +} + +static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) +{ + struct tusb320_priv *priv = dev_id; + int state, polarity; + unsigned reg; + + if (regmap_read(priv->regmap, TUSB320_REG9, )) { + dev_err(priv->dev, "error during i2c read!\n"); + return IRQ_NONE; + } + + if (!(reg & TUSB320_REG9_INTERRUPT_STATUS)) + return IRQ_NONE; + + state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) & + TUSB320_REG9_ATTACHED_STATE_MASK; + polarity = !!(reg & TUSB320_REG9_CABLE_DIRECTION); + + dev_dbg(priv->dev, "attached state: %s, polarity: %d\n", + tusb_attached_states[state], polarity); + + extcon_set_state(priv->edev, EXTCON_USB, +state == TUSB320_ATTACHED_STATE_UFP); + extcon_set_state(priv->edev, EXTCON_USB_HOST, +state == TUSB320_ATTACHED_STATE_DFP); + extcon_set_property(priv->edev, EXTCON_USB, + EXTCON_PROP_USB_TYPEC_POLARITY, + (union extcon_property_value)polarity); + extcon_set_property(priv->edev,