Messages in this thread | | | From | Jun Li <> | Subject | RE: [PATCH] extcon: ptn5150: add usb role class support | Date | Tue, 19 Apr 2022 07:48:51 +0000 |
| |
Hi
> -----Original Message----- > From: Chanwoo Choi <cw00.choi@samsung.com> > Sent: Thursday, April 7, 2022 10:18 AM > To: Jun Li <jun.li@nxp.com>; myungjoo.ham@samsung.com; krzk@kernel.org > Cc: linux-kernel@vger.kernel.org; Frank Li <frank.li@nxp.com>; Xu Yang > <xu.yang_2@nxp.com> > Subject: Re: [PATCH] extcon: ptn5150: add usb role class support > > Hi, > > On 4/6/22 6:42 PM, Li Jun wrote: > > Add support of usb role class consumer to do role switch. > > > > Signed-off-by: Li Jun <jun.li@nxp.com> > > --- > > drivers/extcon/Kconfig | 1 + > > drivers/extcon/extcon-ptn5150.c | 39 > > +++++++++++++++++++++++++++++++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index > > 0d42e49105dd..9828ade787a8 100644 > > --- a/drivers/extcon/Kconfig > > +++ b/drivers/extcon/Kconfig > > @@ -132,6 +132,7 @@ config EXTCON_PTN5150 > > tristate "NXP PTN5150 CC LOGIC USB EXTCON support" > > depends on I2C && (GPIOLIB || COMPILE_TEST) > > select REGMAP_I2C > > + select USB_ROLE_SWITCH > > help > > Say Y here to enable support for USB peripheral and USB host > > detection by NXP PTN5150 CC (Configuration Channel) logic chip. > > diff --git a/drivers/extcon/extcon-ptn5150.c > > b/drivers/extcon/extcon-ptn5150.c index 5b9a3cf8df26..821b0f390308 > > 100644 > > --- a/drivers/extcon/extcon-ptn5150.c > > +++ b/drivers/extcon/extcon-ptn5150.c > > @@ -17,6 +17,7 @@ > > #include <linux/slab.h> > > #include <linux/extcon-provider.h> > > #include <linux/gpio/consumer.h> > > +#include <linux/usb/role.h> > > > > /* PTN5150 registers */ > > #define PTN5150_REG_DEVICE_ID 0x01 > > @@ -52,6 +53,7 @@ struct ptn5150_info { > > int irq; > > struct work_struct irq_work; > > struct mutex mutex; > > + struct usb_role_switch *role_sw; > > }; > > > > /* List of detectable cables */ > > @@ -85,6 +87,12 @@ static void ptn5150_check_state(struct ptn5150_info > *info) > > extcon_set_state_sync(info->edev, EXTCON_USB_HOST, false); > > gpiod_set_value_cansleep(info->vbus_gpiod, 0); > > extcon_set_state_sync(info->edev, EXTCON_USB, true); > > + > > + ret = usb_role_switch_set_role(info->role_sw, USB_ROLE_DEVICE); > > + if (ret) > > + dev_err(info->dev, "failed to set device role: %d\n", > > + ret); > > > + > > break; > > case PTN5150_UFP_ATTACHED: > > extcon_set_state_sync(info->edev, EXTCON_USB, false); @@ -95,6 > > +103,12 @@ static void ptn5150_check_state(struct ptn5150_info *info) > > gpiod_set_value_cansleep(info->vbus_gpiod, 1); > > > > extcon_set_state_sync(info->edev, EXTCON_USB_HOST, true); > > + > > + ret = usb_role_switch_set_role(info->role_sw, USB_ROLE_HOST); > > + if (ret) > > + dev_err(info->dev, "failed to set host role: %d\n", > > + ret); > > + > > break; > > default: > > break; > > @@ -133,6 +147,13 @@ static void ptn5150_irq_work(struct work_struct > *work) > > extcon_set_state_sync(info->edev, > > EXTCON_USB, false); > > gpiod_set_value_cansleep(info->vbus_gpiod, 0); > > + > > + ret = usb_role_switch_set_role(info->role_sw, > > + USB_ROLE_NONE); > > + if (ret) > > + dev_err(info->dev, > > + "failed to set none role: %d\n", > > + ret); > > } > > } > > > > @@ -194,6 +215,14 @@ static int ptn5150_init_dev_type(struct ptn5150_info > *info) > > return 0; > > } > > > > +static void ptn5150_put_role_sw(void *data) { > > + struct ptn5150_info *info = data; > > + > > + cancel_work_sync(&info->irq_work); > > + usb_role_switch_put(info->role_sw); > > +} > > + > > static int ptn5150_i2c_probe(struct i2c_client *i2c) { > > struct device *dev = &i2c->dev; > > @@ -284,6 +313,16 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c) > > if (ret) > > return -EINVAL; > > > > + info->role_sw = usb_role_switch_get(info->dev); > > + if (IS_ERR(info->role_sw)) > > + return PTR_ERR(info->role_sw); > > If usb_role_switch is optional, you don't need to return error when > usb_role_switch_get returns error. Because 'usb_role_switch_set_role' > on this patch doesn't break the sequence. Just print the error log if error > case.
Yes, it's optional, if the usb role switch is not provided, then usb_role_switch_get() will return NULL, then usb role switch will do nothing; if the usb role switch is provided(intended to use) but there is something wrong to get it, we must return error to force a failure in probe(including probe defer).
Li Jun > > > + > > + if (info->role_sw) { > > + ret = devm_add_action_or_reset(dev, ptn5150_put_role_sw, info); > > + if (ret) > > + return ret; > > + } > > + > > /* > > * Update current extcon state if for example OTG connection was there > > * before the probe > > > > > -- > Best Regards, > Chanwoo Choi > Samsung Electronics
| |