Messages in this thread | | | From | Jun Li <> | Subject | RE: [PATCH] extcon: ptn5150: add usb role class support | Date | Wed, 6 Apr 2022 11:48:31 +0000 |
| |
> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > Sent: Wednesday, April 6, 2022 6:16 PM > To: Jun Li <jun.li@nxp.com>; myungjoo.ham@samsung.com; > cw00.choi@samsung.com > 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 > > On 06/04/2022 11:42, Li Jun wrote: > > Add support of usb role class consumer to do role switch. > > Please mention also why you are doing this.
Will improve in v2.
> > > > > 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 > > You do not need to select it. Driver will work without role switch, > won't it? If it works, then probably it should be just imply.
Okay, usb role class provider should enable this for me, will drop it.
> > > 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); > > + > > > Instead of having usb_role_switch_set_role() in two places, how about: > 1. setting local variable to USB_ROLE_HOST/USB_ROLE_NONE > 2. return on default > 3. one call to usb_role_switch_set_role() outside of the case? > > Should create less code.
Yes, this will be better, I will update in v2.
> > > 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); > > This looks independent and should be executed always - even if getting > role_sw in probe does not succeed.
Agreed, I will add one patch to make this action taken in all cases.
Thanks Li Jun
> > > + usb_role_switch_put(info->role_sw); > > +} > > + > > > > Best regards, > Krzysztof
| |