Messages in this thread | | | Date | Wed, 6 Apr 2022 12:15:48 +0200 | Subject | Re: [PATCH] extcon: ptn5150: add usb role class support | From | Krzysztof Kozlowski <> |
| |
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.
> > 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.
> 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.
> 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.
> + usb_role_switch_put(info->role_sw); > +} > +
Best regards, Krzysztof
| |