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 <[email protected]> > > --- > > 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 000000000000..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 <[email protected]> > > + */ > > + > > +#include <linux/extcon-provider.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > irq.h doesn't be needed. Better to remove irq.h. > > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/regmap.h> > > + > > +#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_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, &val); > > + 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, EXTCON_USB_HOST, > > + EXTCON_PROP_USB_TYPEC_POLARITY, > > + (union extcon_property_value)polarity); > > + extcon_sync(priv->edev, EXTCON_USB); > > + extcon_sync(priv->edev, EXTCON_USB_HOST); > > + > > + regmap_write(priv->regmap, TUSB320_REG9, reg); > > + > > + return IRQ_HANDLED; > > +} > > + > > +static const struct regmap_config tusb320_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 8, > > +}; > > + > > +static int tusb320_extcon_probe(struct i2c_client *client, > > + const struct i2c_device_id *id) > > +{ > > + struct tusb320_priv *priv; > > + int ret; > > + > > + priv = devm_kzalloc(&client->dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + priv->dev = &client->dev; > > + > > + priv->regmap = devm_regmap_init_i2c(client, &tusb320_regmap_config); > > + if (IS_ERR(priv->regmap)) > > + return PTR_ERR(priv->regmap); > > + > > + ret = tusb320_check_signature(priv); > > + if (ret) > > + return ret; > > + > > + priv->edev = devm_extcon_dev_allocate(priv->dev, tusb320_extcon_cable); > > + if (IS_ERR(priv->edev)) { > > + dev_err(priv->dev, "failed to allocate extcon device\n"); > > + return -ENOMEM; > > Use 'return PTR_ERR(priv->edev)' instead of '-ENOMEM'. > > > + } > > + > > + ret = devm_extcon_dev_register(priv->dev, priv->edev); > > + if (ret < 0) { > > + dev_err(priv->dev, "failed to register extcon device\n"); > > + return ret; > > + } > > + > > + extcon_set_property_capability(priv->edev, EXTCON_USB, > > + EXTCON_PROP_USB_TYPEC_POLARITY); > > + extcon_set_property_capability(priv->edev, EXTCON_USB_HOST, > > + EXTCON_PROP_USB_TYPEC_POLARITY); > > + > > + /* update initial state */ > > + tusb320_irq_handler(client->irq, priv); > > + > > + ret = devm_request_threaded_irq(priv->dev, client->irq, NULL, > > + tusb320_irq_handler, > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + dev_name(priv->dev), priv); > > + > > + return ret; > > +} > > + > > +static const struct of_device_id tusb320_extcon_dt_match[] = { > > + { .compatible = "ti,tusb320", }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, tusb320_extcon_dt_match); > > + > > +static const struct i2c_device_id tusb320_i2c_id[] = { > > + { "tusb320", 0 }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(i2c, tusb320_i2c_id); > > tusb320_i2c_id is not used on this driver. > > > + > > +static struct i2c_driver tusb320_extcon_driver = { > > + .probe = tusb320_extcon_probe, > > + .driver = { > > + .name = "extcon-tusb320", > > + .of_match_table = tusb320_extcon_dt_match, > > + }, > > +}; > > + > > +static int __init tusb320_init(void) > > +{ > > + return i2c_add_driver(&tusb320_extcon_driver); > > +} > > +subsys_initcall(tusb320_init); > > + > > +static void __exit tusb320_exit(void) > > +{ > > + i2c_del_driver(&tusb320_extcon_driver); > > +} > > +module_exit(tusb320_exit); > > + > > +MODULE_AUTHOR("Michael Auchter <[email protected]>"); > > +MODULE_DESCRIPTION("TI TUSB320 extcon driver"); > > +MODULE_LICENSE("GPL v2"); > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
