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

[Chanwoo Choi <cw00.choi@samsung.com>]

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.

> +
> +	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