Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <4ad9e733-7d8b-a73b-c59e-d9b6d5e58498@linaro.org>
Date:   Wed, 6 Apr 2022 12:15:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH] extcon: ptn5150: add usb role class support
Content-Language: en-US
To:     Li Jun <jun.li@nxp.com>, myungjoo.ham@samsung.com,
        cw00.choi@samsung.com
Cc:     linux-kernel@vger.kernel.org, frank.li@nxp.com, xu.yang_2@nxp.com
References: <1649238142-27564-1-git-send-email-jun.li@nxp.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <1649238142-27564-1-git-send-email-jun.li@nxp.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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