Hi Heikki,
Thanks for the feedback.
> Subject: Re: [PATCH 3/9] usb: typec: driver for TI HD3SS3220 USB Type-C DRP
> port controller
>
> On Wed, Mar 06, 2019 at 09:07:20AM +0000, Biju Das wrote:
> > Driver for TI HD3SS3220 USB Type-C DRP port controller.
> >
> > The driver currently registers the port and supports data role swapping.
> >
> > Signed-off-by: Biju Das <biju.das@xxxxxxxxxxxxxx>
> > ---
> > drivers/usb/typec/Kconfig | 10 ++
> > drivers/usb/typec/Makefile | 1 +
> > drivers/usb/typec/hd3ss3220.c | 284
> > ++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 295 insertions(+)
> > create mode 100644 drivers/usb/typec/hd3ss3220.c
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 30a847c..91696d2 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -49,6 +49,16 @@ source "drivers/usb/typec/tcpm/Kconfig"
> >
> > source "drivers/usb/typec/ucsi/Kconfig"
> >
> > +config TYPEC_HD3SS3220
> > + tristate "TI HD3SS3220 Type-C DRP Port controller driver"
> > + depends on I2C
> > + help
> > + Say Y or M here if your system has TI HD3SS3220 Type-C DRP Port
> > + controller driver.
> > +
> > + If you choose to build this driver as a dynamically linked module, the
> > + module will be called hd3ss3220.ko.
> > +
> > config TYPEC_TPS6598X
> > tristate "TI TPS6598x USB Power Delivery controller driver"
> > depends on I2C
> > diff --git a/drivers/usb/typec/Makefile b/drivers/usb/typec/Makefile
> > index 6696b72..7753a5c3 100644
> > --- a/drivers/usb/typec/Makefile
> > +++ b/drivers/usb/typec/Makefile
> > @@ -4,5 +4,6 @@ typec-y := class.o mux.o bus.o
> > obj-$(CONFIG_TYPEC) += altmodes/
> > obj-$(CONFIG_TYPEC_TCPM) += tcpm/
> > obj-$(CONFIG_TYPEC_UCSI) += ucsi/
> > +obj-$(CONFIG_TYPEC_HD3SS3220) += hd3ss3220.o
> > obj-$(CONFIG_TYPEC_TPS6598X) += tps6598x.o
> > obj-$(CONFIG_TYPEC) += mux/
> > diff --git a/drivers/usb/typec/hd3ss3220.c
> > b/drivers/usb/typec/hd3ss3220.c new file mode 100644 index
> > 0000000..bd3e1ec
> > --- /dev/null
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -0,0 +1,284 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * TI HD3SS3220 Type-C DRP Port Controller Driver
> > + *
> > + * Copyright (C) 2019 Renesas Electronics Corp.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/extcon-provider.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +#include <linux/usb/typec.h>
> > +#include <linux/delay.h>
> > +
> > +#define HD3SS3220_REG_CN_STAT_CTRL 0x09
> > +#define HD3SS3220_REG_GEN_CTRL 0x0A
> > +#define HD3SS3220_REG_DEV_REV 0xA0
> > +
> > +/* Register HD3SS3220_REG_CN_STAT_CTRL*/
> > +#define HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK
> (BIT(7) | BIT(6))
> > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_DFP BIT(6)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_AS_UFP BIT(7)
> > +#define HD3SS3220_REG_CN_STAT_CTRL_TO_ACCESSORY
> (BIT(7) | BIT(6))
> > +#define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS BIT(4)
> > +
> > +/* Register HD3SS3220_REG_GEN_CTRL*/
> > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK
> (BIT(2) | BIT(1))
> > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00
> > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1)
> > +#define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC
> (BIT(2) | BIT(1))
> > +
> > +struct hd3ss3220 {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct extcon_dev *extcon;
> > + struct typec_port *port;
> > + struct typec_capability typec_cap;
> > +};
> > +
> > +static const unsigned int hd3ss3220_cable[] = {
> > + EXTCON_USB,
> > + EXTCON_USB_HOST,
> > + EXTCON_NONE
> > +};
>
> You need to use the USB role class instead of extcon. Check
> drivers/usb/roles/class/ and include/linux/usb/role.h
>
> You may also want to check the updates Yu Chen is introducing to that
> class:
> https://lkml.org/lkml/2019/3/2/42
Ok. Will use USB role class. Basically this driver need to get role_sw handle from
usb3 driver. Based on the cable attach status/role switch it need to call
"usb_role_switch_set_role" function and driver will get notified after
setting the role.
> > +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> > +src_pref) {
> > + unsigned int reg_val;
> > + int ret;
> > +
> > + ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_GEN_CTRL, ®_val);
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg_val &= ~HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK;
> > + reg_val |= src_pref;
> > +
> > + return regmap_write(hd3ss3220->regmap,
> > + HD3SS3220_REG_GEN_CTRL,
> reg_val);
>
> Please fix the alignment:
OK. Will fix it.
> return regmap_write(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> reg_val);
>
> > +}
> > +
> > +static int hd3ss3220_attached_state_detect(struct hd3ss3220
> > +*hd3ss3220) {
> > + unsigned int reg_val;
> > + int ret, attached_state;
> > +
> > + ret = regmap_read(hd3ss3220->regmap,
> > + HD3SS3220_REG_CN_STAT_CTRL, ®_val);
>
> ditto:
OK. Will fix it.
> ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> ®_val);
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + reg_val &=
> HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK;
> > + switch (reg_val) {
>
> switch (reg_val &
> HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK)
>
> > + case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> > + attached_state = EXTCON_USB_HOST;
> > + break;
> > + case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> > + attached_state = EXTCON_USB;
> > + break;
> > + default:
> > + attached_state = EXTCON_NONE;
> > + }
> > +
> > + return attached_state;
> > +}
> > +
> > +static int hd3ss3220_dr_set(const struct typec_capability *cap,
> > + enum typec_data_role role)
> > +{
> > + struct hd3ss3220 *hd3ss3220 =
> > + container_of(cap, struct hd3ss3220,
> typec_cap);
>
> I think scripts/checkpatch.pl would find all these alignment issues:
I have ran check patch and it didn't complained about any alignment issues.
Any way will fix all the alignment issues.
> struct hd3ss3220 *hd3ss3220 = container_of(cap, struct hd3ss3220,
> typec_cap);
>
> There are a lot more of those in this patch. Please fix all of them.
OK. Will fix.
> > + int ret = 0;
> > +
> > + if (role == TYPEC_HOST) {
> > + extcon_set_state_sync(hd3ss3220->extcon, EXTCON_USB,
> false);
> > + ret = hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC);
> > + mdelay(100);
>
> Whoa! That's a long delay. A bit too long. You should sleep if you really need
> to wait that long, but 100 ms?
OK. Will check this again and put appropriate delay/sleep.
> > + extcon_set_state_sync(hd3ss3220->extcon,
> EXTCON_USB_HOST, true);
> > + } else {
> > + extcon_set_state_sync(hd3ss3220->extcon,
> > + EXTCON_USB_HOST, false);
> > + ret = hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK);
> > + mdelay(100);
> > + extcon_set_state_sync(hd3ss3220->extcon, EXTCON_USB,
> true);
> > + }
>
> I think you could just call hd3ss3220_set_source_pref() ones here.
> Just store the value to a variable in those conditions:
OK. Will do.
> if (role == TYPEC_HOST)
> pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> else
> pref = HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
>
> ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> ...
>
> > + typec_set_data_role(hd3ss3220->port, role);
> > +
> > + return ret;
> > +}
> > +
> > +static void hd3ss3220_set_extcon_state(struct hd3ss3220 *hd3ss3220) {
> > + int attached_state = hd3ss3220_attached_state_detect(hd3ss3220);
> > +
> > + if (attached_state == EXTCON_USB_HOST) {
> > + extcon_set_state_sync(hd3ss3220->extcon, EXTCON_USB,
> false);
> > + extcon_set_state_sync(hd3ss3220->extcon,
> EXTCON_USB_HOST, true);
> > + typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> > + } else if (attached_state == EXTCON_USB) {
> > + extcon_set_state_sync(hd3ss3220->extcon,
> > + EXTCON_USB_HOST, false);
> > + extcon_set_state_sync(hd3ss3220->extcon, EXTCON_USB,
> true);
> > + typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> > + } else {
> > + hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > + extcon_set_state_sync(hd3ss3220->extcon,
> > + EXTCON_USB_HOST, false);
> > + extcon_set_state_sync(hd3ss3220->extcon, EXTCON_USB,
> false);
> > + }
> > +}
> > +
> > +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220) {
> > + int err;
> > + unsigned int data;
> > +
> > + hd3ss3220_set_extcon_state(hd3ss3220);
> > +
> > + err = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL, &data);
> > + if (err < 0)
> > + return IRQ_NONE;
> > +
> > + err = regmap_write(hd3ss3220->regmap,
> > + HD3SS3220_REG_CN_STAT_CTRL,
> > + data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > + if (err < 0)
> > + return IRQ_NONE;
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t hd3ss3220_irq_handler(int irq, void *data) {
> > + struct i2c_client *client = to_i2c_client(data);
> > + struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > + return hd3ss3220_irq(hd3ss3220);
> > +}
> > +
> > +static int hd3ss3220_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct hd3ss3220 *hd3ss3220;
> > + int ret;
> > + unsigned int data;
> > + static const struct regmap_config config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
> > + .max_register = 0x0A,
> > + };
>
> Move that outside of the function.
OK. Will do.
> > + hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> > + GFP_KERNEL);
> > + if (!hd3ss3220)
> > + return -ENOMEM;
> > +
> > + i2c_set_clientdata(client, hd3ss3220);
> > +
> > + hd3ss3220->regmap = devm_regmap_init_i2c(client, &config);
> > + if (IS_ERR(hd3ss3220->regmap))
> > + return PTR_ERR(hd3ss3220->regmap);
> > +
> > + hd3ss3220_set_source_pref(hd3ss3220,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +
> > + hd3ss3220->extcon = devm_extcon_dev_allocate(&client->dev,
> > + hd3ss3220_cable);
> > + if (IS_ERR(hd3ss3220->extcon)) {
> > + dev_err(&client->dev, "failed to allocate extcon device\n");
> > + return PTR_ERR(hd3ss3220->extcon);
> > + }
> > +
> > + ret = devm_extcon_dev_register(&client->dev, hd3ss3220->extcon);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "failed to register extcon device\n");
> > + return ret;
> > + }
> > +
> > + hd3ss3220->dev = &client->dev;
> > + hd3ss3220->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
> > + hd3ss3220->typec_cap.dr_set = hd3ss3220_dr_set;
> > + hd3ss3220->typec_cap.type = TYPEC_PORT_DRP;
> > + hd3ss3220->typec_cap.data = TYPEC_PORT_DRD;
> > +
> > + hd3ss3220->port = typec_register_port(&client->dev,
> > + &hd3ss3220->typec_cap);
> > + if (IS_ERR(hd3ss3220->port))
> > + return PTR_ERR(hd3ss3220->port);
> > +
> > + hd3ss3220_set_extcon_state(hd3ss3220);
> > + if (client->irq > 0) {
> > + ret = regmap_read(hd3ss3220->regmap,
> > + HD3SS3220_REG_CN_STAT_CTRL, &data);
> > + if (ret < 0)
> > + goto error;
> > +
> > + if (data & HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS) {
> > + ret = regmap_write(hd3ss3220->regmap,
> > + HD3SS3220_REG_CN_STAT_CTRL,
> > + data |
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS);
> > + if (ret < 0)
> > + goto error;
> > + }
> > +
> > + ret = devm_request_threaded_irq(&client->dev, client->irq,
> > + NULL, hd3ss3220_irq_handler,
> > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> > + "hd3ss3220", &client->dev);
> > + if (ret)
> > + goto error;
> > + }
> > +
> > + ret = i2c_smbus_read_byte_data(client, HD3SS3220_REG_DEV_REV);
> > + if (ret < 0)
> > + goto error;
> > +
> > + dev_info(&client->dev, "probed revision=0x%x\n", ret);
> > +
> > + return 0;
> > +error:
> > + typec_unregister_port(hd3ss3220->port);
> > +
> > + return ret;
> > +}
> > +
> > +static int hd3ss3220_remove(struct i2c_client *client) {
> > + struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > + typec_unregister_port(hd3ss3220->port);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id dev_ids[] = {
> > + { .compatible = "ti,hd3ss3220"},
> > + {}
> > +};
> > +MODULE_DEVICE_TABLE(of, dev_ids);
> > +
> > +static struct i2c_driver hd3ss3220_driver = {
> > + .driver = {
> > + .name = "hd3ss3220",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(dev_ids),
> > + },
> > + .probe = hd3ss3220_probe,
> > + .remove = hd3ss3220_remove,
> > +};
> > +
> > +module_i2c_driver(hd3ss3220_driver);
> > +
> > +MODULE_AUTHOR("Biju Das <biju.das@xxxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("TI HD3SS3220 DRP Port Controller Driver");
> > +MODULE_LICENSE("GPL");
>
> You will need to use the USB role class instead of extcon. Otherwise this
> looks OK to me, assuming you fix the style issues ;-).
Regards,
Biju
