RE: [PATCH] extcon: ptn5150: add usb role class support

[Jun Li]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Wednesday, April 6, 2022 6:16 PM
> To: Jun Li <jun.li@xxxxxxx>; myungjoo.ham@xxxxxxxxxxx;
> cw00.choi@xxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; Frank Li <frank.li@xxxxxxx>; Xu Yang
> <xu.yang_2@xxxxxxx>
> 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@xxxxxxx>
> > ---
> >  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