RE: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C DRP port controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Heikki,

Thanks for the feedback.

> Subject: Re: [PATCH V4 3/7] usb: typec: driver for TI HD3SS3220 USB Type-C
> DRP port controller
> 
> On Tue, Apr 16, 2019 at 10:38:03AM +0100, 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>
> 
> It's too bad they call it "port controller" even though it's not Port Controller
> Interface compliant. I put a few minor nitpicks below.
> Otherwise this looks okay to me, so if there are no other comments:
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> 
> > ---
> >  Note: This patch has compilation dependency on
> > https://patchwork.kernel.org/patch/10882555/
> >
> >  V3-->V4
> >    * Incorporated Chunfeng Yun's review comment
> >    * Used fwnode API's to get usb role switch handle.
> >
> >  V2-->V3
> >    * Used the new api "usb_role_switch by node" for getting
> >      remote endpoint associated with Type-C USB DRP port
> >      controller devices.
> >  V1-->V2
> >    * Driver uses usb role class instead of extcon for dual role switch
> >      and also handles connect/disconnect events.
> > ---
> >  drivers/usb/typec/Kconfig     |  10 ++
> >  drivers/usb/typec/Makefile    |   1 +
> >  drivers/usb/typec/hd3ss3220.c | 263
> > ++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 274 insertions(+)
> >  create mode 100644 drivers/usb/typec/hd3ss3220.c
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 89d9193..92a3717 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -50,6 +50,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..e98a38f
> > --- /dev/null
> > +++ b/drivers/usb/typec/hd3ss3220.c
> > @@ -0,0 +1,263 @@
> > +// 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/usb/role.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.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 usb_role_switch	*role_sw;
> > +	struct typec_port *port;
> > +	struct typec_capability typec_cap;
> > +};
> > +
> > +static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int
> > +src_pref) {
> > +	return regmap_update_bits(hd3ss3220->regmap,
> HD3SS3220_REG_GEN_CTRL,
> > +
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK,
> > +				  src_pref);
> > +}
> > +
> > +static enum usb_role hd3ss3220_get_attached_state(struct hd3ss3220
> > +*hd3ss3220) {
> > +	unsigned int reg_val;
> > +	enum usb_role attached_state;
> > +	int ret;
> > +
> > +	ret = regmap_read(hd3ss3220->regmap,
> HD3SS3220_REG_CN_STAT_CTRL,
> > +			  &reg_val);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (reg_val &
> HD3SS3220_REG_CN_STAT_CTRL_ATTACHED_STATE_MASK) {
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_DFP:
> > +		attached_state = USB_ROLE_HOST;
> > +	break;
> > +	case HD3SS3220_REG_CN_STAT_CTRL_AS_UFP:
> > +		attached_state = USB_ROLE_DEVICE;
> > +	break;
> 
> It looks like your line indentation has collapsed here. I pretty sure
> scripts/checkpatch.pl would have found these.

OK. Will fix this in V5. I have ran Check patch before sending the patch and it didn't complain this.

> > +	default:
> > +		attached_state = USB_ROLE_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);
> > +	enum usb_role role_val;
> > +	int pref, ret = 0;
> > +
> > +	if (role == TYPEC_HOST) {
> > +		role_val = USB_ROLE_HOST;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC;
> > +	} else {
> > +		role_val = USB_ROLE_DEVICE;
> > +		pref =
> HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK;
> > +	}
> > +
> > +	ret = hd3ss3220_set_source_pref(hd3ss3220, pref);
> > +	usleep_range(10, 100);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_val);
> > +	typec_set_data_role(hd3ss3220->port, role);
> > +
> > +	return ret;
> > +}
> > +
> > +static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) {
> > +	enum usb_role role_state =
> hd3ss3220_get_attached_state(hd3ss3220);
> > +
> > +	usb_role_switch_set_role(hd3ss3220->role_sw, role_state);
> > +	if (role_state == USB_ROLE_NONE)
> > +		hd3ss3220_set_source_pref(hd3ss3220,
> > +
> 	HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT);
> > +
> > +	switch (role_state) {
> > +	case USB_ROLE_HOST:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_HOST);
> > +		break;
> > +	case USB_ROLE_DEVICE:
> > +		typec_set_data_role(hd3ss3220->port, TYPEC_DEVICE);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> > +irqreturn_t hd3ss3220_irq(struct hd3ss3220 *hd3ss3220) {
> > +	int err;
> > +
> > +	hd3ss3220_set_role(hd3ss3220);
> > +	err = regmap_update_bits_base(hd3ss3220->regmap,
> > +				      HD3SS3220_REG_CN_STAT_CTRL,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +
> HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS,
> > +				      NULL, false, true);
> > +	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 const struct regmap_config config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = 0x0A,
> > +};
> > +
> > +static int hd3ss3220_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct hd3ss3220 *hd3ss3220;
> > +	struct fwnode_handle *parent, *child;
> > +	int ret;
> > +	unsigned int data;
> > +
> > +	hd3ss3220 = devm_kzalloc(&client->dev, sizeof(struct hd3ss3220),
> > +				 GFP_KERNEL);
> > +	if (!hd3ss3220)
> > +		return -ENOMEM;
> > +
> > +	i2c_set_clientdata(client, hd3ss3220);
> > +
> > +	hd3ss3220->dev = &client->dev;
> > +	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);
> > +	child = fwnode_graph_get_next_endpoint(dev_fwnode(hd3ss3220-
> >dev),
> > +					       NULL);
> > +	parent = fwnode_graph_get_remote_port_parent(child);
> > +	hd3ss3220->role_sw = fwnode_usb_role_switch_get(parent);
> > +	if (IS_ERR_OR_NULL(hd3ss3220->role_sw)) {
> > +		fwnode_handle_put(child);
> > +		fwnode_handle_put(parent);
> > +		return PTR_ERR(hd3ss3220->role_sw);
> > +	}
> > +
> > +	fwnode_handle_put(child);
> > +	fwnode_handle_put(parent);
> > +
> > +	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_role(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;
> > +		}
> > +		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;
> > +	}
> 
> I would move the above if (client->irq > 0) case to it's own function.

OK. Will fix this in V5.

Regards,
Biju

> > +	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);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	return ret;
> > +}
> > +
> > +static int hd3ss3220_remove(struct i2c_client *client) {
> > +	struct hd3ss3220 *hd3ss3220 = i2c_get_clientdata(client);
> > +
> > +	typec_unregister_port(hd3ss3220->port);
> > +	usb_role_switch_put(hd3ss3220->role_sw);
> > +
> > +	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",
> > +		.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");
> > --
> > 2.7.4
> 
> thanks,
> 
> --
> heikki




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux