[PATCH v2 0/2] extcon: ptn5150: Add usb-typec support for Intel LGM SoC

[PATCH v2 0/2] extcon: ptn5150: Add usb-typec support for Intel LGM SoC

[Ramuthevar,Vadivel MuruganX]

Add usb-typec detection support for the Intel LGM SoC based boards.

Original driver is not supporting usb detection on Intel LGM SoC based boards
then we debugged and fixed the issue, but before sending our patches Mr.Krzyszto
has sent the same kind of patches, so I have rebased over his latest patches
which is present in maintainer tree.

Built and tested it's working fine, overthat created the new patch.

Thanks for Krzyszto and Chanwoo Choi
---
v2:
  - Krzyszto review comments update
  - squash my previous patches 1 to 5 as single patch
  - add extcon_set_property_capability for EXTCON_USB and EXTCON_PROP_USB_TYPEC_POLARITY 

Reference to mail discussion:
 https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2281723.html

Ramuthevar Vadivel Murugan (2):
  extcon: ptn5150: Switch to GENMASK() and BIT() macros
  extcon: ptn5150: Set the VBUS and POLARITY property state

 drivers/extcon/extcon-ptn5150.c | 49 ++++++++++++++---------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

-- 
2.11.0

[PATCH v2 1/2] extcon: ptn5150: Switch to GENMASK() and BIT() macros

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Switch to GENMASK() and BIT() macros.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/extcon/extcon-ptn5150.c | 43 +++++++++++------------------------------
 1 file changed, 11 insertions(+), 32 deletions(-)

diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
index 8ba706fad887..8b930050a3f1 100644
--- a/drivers/extcon/extcon-ptn5150.c
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -7,6 +7,7 @@
 // Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
 // Copyright (c) 2020 Krzysztof Kozlowski <krzk@kernel.org>
 
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -35,29 +36,13 @@ enum ptn5150_reg {
 #define PTN5150_UFP_ATTACHED			0x2
 
 /* Define PTN5150 MASK/SHIFT constant */
-#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
-#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	\
-	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
+#define PTN5150_REG_DEVICE_ID_VERSION		GENMASK(7, 3)
+#define PTN5150_REG_DEVICE_ID_VENDOR		GENMASK(2, 0)
 
-#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
-#define PTN5150_REG_DEVICE_ID_VERSION_MASK	\
-	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
-
-#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
-#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	\
-	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
-
-#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
-#define PTN5150_REG_CC_VBUS_DETECTION_MASK	\
-	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
-
-#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
-#define PTN5150_REG_INT_CABLE_ATTACH_MASK	\
-	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
-
-#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
-#define PTN5150_REG_INT_CABLE_DETACH_MASK	\
-	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
+#define PTN5150_REG_CC_PORT_ATTACHMENT		GENMASK(4, 2)
+#define PTN5150_REG_CC_VBUS_DETECTION		BIT(7)
+#define PTN5150_REG_INT_CABLE_ATTACH_MASK	BIT(0)
+#define PTN5150_REG_INT_CABLE_DETACH_MASK	BIT(1)
 
 struct ptn5150_info {
 	struct device *dev;
@@ -95,9 +80,7 @@ static void ptn5150_check_state(struct ptn5150_info *info)
 		return;
 	}
 
-	port_status = ((reg_data &
-			PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
-			PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
+	port_status = FIELD_GET(PTN5150_REG_CC_PORT_ATTACHMENT, reg_data);
 
 	switch (port_status) {
 	case PTN5150_DFP_ATTACHED:
@@ -107,8 +90,7 @@ static void ptn5150_check_state(struct ptn5150_info *info)
 		break;
 	case PTN5150_UFP_ATTACHED:
 		extcon_set_state_sync(info->edev, EXTCON_USB, false);
-		vbus = ((reg_data & PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
-			PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
+		vbus = FIELD_GET(PTN5150_REG_CC_VBUS_DETECTION, reg_data);
 		if (vbus)
 			gpiod_set_value_cansleep(info->vbus_gpiod, 0);
 		else
@@ -191,11 +173,8 @@ static int ptn5150_init_dev_type(struct ptn5150_info *info)
 		return -EINVAL;
 	}
 
-	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
-				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
-	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
-				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
-
+	vendor_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VENDOR, reg_data);
+	version_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VERSION, reg_data);
 	dev_dbg(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
 		version_id, vendor_id);
 
-- 
2.11.0

[PATCH v2 2/2] extcon: ptn5150: Set the VBUS and POLARITY property state

[Ramuthevar,Vadivel MuruganX]

From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>

Set the VBUS and POLARITY property state.

Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
---
 drivers/extcon/extcon-ptn5150.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
index 8b930050a3f1..b5217a61615c 100644
--- a/drivers/extcon/extcon-ptn5150.c
+++ b/drivers/extcon/extcon-ptn5150.c
@@ -279,6 +279,12 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c)
 		return ret;
 	}
 
+	extcon_set_property_capability(info->edev, EXTCON_USB,
+				       EXTCON_PROP_USB_VBUS);
+	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
+				       EXTCON_PROP_USB_VBUS);
+	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
+				       EXTCON_PROP_USB_TYPEC_POLARITY);
 	/* Initialize PTN5150 device and print vendor id and version id */
 	ret = ptn5150_init_dev_type(info);
 	if (ret)
-- 
2.11.0

Re: [PATCH v2 2/2] extcon: ptn5150: Set the VBUS and POLARITY property state

[Chanwoo Choi]

Hi,

You better to change the 'state' word to 'capability'.
Actually, this patch doesn't change the value of property.
It set the capability value of property.

"Set the VBUS and POLARITY property capability"

On 8/27/20 12:56 PM, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Set the VBUS and POLARITY property state.

ditto. Need to change the work from 'state' and 'capability'.

> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> ---
>  drivers/extcon/extcon-ptn5150.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
> index 8b930050a3f1..b5217a61615c 100644
> --- a/drivers/extcon/extcon-ptn5150.c
> +++ b/drivers/extcon/extcon-ptn5150.c
> @@ -279,6 +279,12 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c)
>  		return ret;
>  	}
>  
> +	extcon_set_property_capability(info->edev, EXTCON_USB,
> +				       EXTCON_PROP_USB_VBUS);
> +	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
> +				       EXTCON_PROP_USB_VBUS);
> +	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
> +				       EXTCON_PROP_USB_TYPEC_POLARITY);

Need to add blank line.

I understood that you set the property capability
because of get_flipped() function of your patch[1].

But, I think that you need to change the value of EXTCON_PROP_USB_TYPEC_POLARITY
when changing the state of EXTCON_USB_HOST. The polarity property value is always
zero regardless of EXTCON_USB_HOST state as following: The get_flipped()[1] returns
always the same *flipped value.

	EXTCON_USB_HOST is 1, EXTCON_PROP_USB_TYPEC_POLARITY is 0
	EXTCON_USB_HOST is 0, EXTCON_PROP_USB_TYPEC_POLARITY is 0

If EXTCON_PROP_USB_TYPEC_POLARITY value is not related to any behavior,
you don't need to get the property value from extcon consumer driver
like drivers/phy/phy-lgm-usb.c.

Actually, I don't understand why you don't handle the value
of EXTCON_PROP_USB_TYPEC_POLARITY. 

Or, are there any case of what drivers/phy/phy-lgm-usb.c
uses the different extcon device with EXTCON_PROP_USB_TYPEC_POLARITY property
in the future?

So, do you set the EXTCON_PROP_USB_TYPEC_POLARITY capability 
for the extensibility in order to use other extcon device on later?


[1] https://www.spinics.net/lists/devicetree/msg371828.html
+static int get_flipped(struct tca_apb *ta, bool *flipped)
+{
+	union extcon_property_value property;
+	int ret;
+
+	ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
+				  EXTCON_PROP_USB_TYPEC_POLARITY, &property);
+	if (ret) {
+		dev_err(ta->phy.dev, "no polarity property from extcon\n");
+		return ret;
+	}
+
+	*flipped = property.intval;
+
+	return ret;
+}


>  	/* Initialize PTN5150 device and print vendor id and version id */
>  	ret = ptn5150_init_dev_type(info);
>  	if (ret)
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] extcon: ptn5150: Set the VBUS and POLARITY property state

[Ramuthevar, Vadivel MuruganX]

Hi,

On 27/8/2020 12:51 pm, Chanwoo Choi wrote:
> Hi,
> 
> You better to change the 'state' word to 'capability'.
> Actually, this patch doesn't change the value of property.
> It set the capability value of property.
> 
> "Set the VBUS and POLARITY property capability"
Thank you for the review comments, sure will update.
> 
> On 8/27/20 12:56 PM, Ramuthevar,Vadivel MuruganX wrote:
>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>
>> Set the VBUS and POLARITY property state.
> 
> ditto. Need to change the work from 'state' and 'capability'.
Noted.
> 
>>
>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>> ---
>>   drivers/extcon/extcon-ptn5150.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
>> index 8b930050a3f1..b5217a61615c 100644
>> --- a/drivers/extcon/extcon-ptn5150.c
>> +++ b/drivers/extcon/extcon-ptn5150.c
>> @@ -279,6 +279,12 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c)
>>   		return ret;
>>   	}
>>   
>> +	extcon_set_property_capability(info->edev, EXTCON_USB,
>> +				       EXTCON_PROP_USB_VBUS);
>> +	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>> +				       EXTCON_PROP_USB_VBUS);
>> +	extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>> +				       EXTCON_PROP_USB_TYPEC_POLARITY);
> 
> Need to add blank line.
Noted.
> 
> I understood that you set the property capability
> because of get_flipped() function of your patch[1].
> 
> But, I think that you need to change the value of EXTCON_PROP_USB_TYPEC_POLARITY
> when changing the state of EXTCON_USB_HOST. The polarity property value is always
> zero regardless of EXTCON_USB_HOST state as following: The get_flipped()[1] returns
> always the same *flipped value.
> 
> 	EXTCON_USB_HOST is 1, EXTCON_PROP_USB_TYPEC_POLARITY is 0
> 	EXTCON_USB_HOST is 0, EXTCON_PROP_USB_TYPEC_POLARITY is 0
by default EXTCON_PROP_USB_TYPEC_POLARITY is 1
> 
> If EXTCON_PROP_USB_TYPEC_POLARITY value is not related to any behavior,
> you don't need to get the property value from extcon consumer driver
> like drivers/phy/phy-lgm-usb.c.
> 
> Actually, I don't understand why you don't handle the value
> of EXTCON_PROP_USB_TYPEC_POLARITY.
> 
> Or, are there any case of what drivers/phy/phy-lgm-usb.c
> uses the different extcon device with EXTCON_PROP_USB_TYPEC_POLARITY property
> in the future?
Yes, you're right, user connect the different USB cable then we check 
polarity, accordingly driver proceeds, thanks!
> 
> So, do you set the EXTCON_PROP_USB_TYPEC_POLARITY capability
> for the extensibility in order to use other extcon device on later?
yes, that might be the case as well.
> 
> 
> [1] https://www.spinics.net/lists/devicetree/msg371828.html
> +static int get_flipped(struct tca_apb *ta, bool *flipped)
> +{
> +	union extcon_property_value property;
> +	int ret;
> +
> +	ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
> +				  EXTCON_PROP_USB_TYPEC_POLARITY, &property);
> +	if (ret) {
> +		dev_err(ta->phy.dev, "no polarity property from extcon\n");
> +		return ret;
> +	}
> +
> +	*flipped = property.intval;
> +
> +	return ret;
> +}
Thank you for the gone through my usb-phy patch as well

Regards
Vadivel
> 
> 
>>   	/* Initialize PTN5150 device and print vendor id and version id */
>>   	ret = ptn5150_init_dev_type(info);
>>   	if (ret)
>>
> 
> 

Re: [PATCH v2 1/2] extcon: ptn5150: Switch to GENMASK() and BIT() macros

[Ramuthevar, Vadivel MuruganX]

Hi,

On 27/8/2020 1:26 pm, Chanwoo Choi wrote:
>> -
>> +	vendor_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VENDOR, reg_data);
>> +	version_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VERSION, reg_data);
>>   	dev_dbg(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
>>   		version_id, vendor_id);
>>   
>>
> Applied it. Thanks.
Thank you!

Regards
Vadivel

Re: [PATCH v2 1/2] extcon: ptn5150: Switch to GENMASK() and BIT() macros

[Chanwoo Choi]

On 8/27/20 12:56 PM, Ramuthevar,Vadivel MuruganX wrote:
> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> 
> Switch to GENMASK() and BIT() macros.
> 
> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  drivers/extcon/extcon-ptn5150.c | 43 +++++++++++------------------------------
>  1 file changed, 11 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
> index 8ba706fad887..8b930050a3f1 100644
> --- a/drivers/extcon/extcon-ptn5150.c
> +++ b/drivers/extcon/extcon-ptn5150.c
> @@ -7,6 +7,7 @@
>  // Author: Vijai Kumar K <vijaikumar.kanagarajan@gmail.com>
>  // Copyright (c) 2020 Krzysztof Kozlowski <krzk@kernel.org>
>  
> +#include <linux/bitfield.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> @@ -35,29 +36,13 @@ enum ptn5150_reg {
>  #define PTN5150_UFP_ATTACHED			0x2
>  
>  /* Define PTN5150 MASK/SHIFT constant */
> -#define PTN5150_REG_DEVICE_ID_VENDOR_SHIFT	0
> -#define PTN5150_REG_DEVICE_ID_VENDOR_MASK	\
> -	(0x3 << PTN5150_REG_DEVICE_ID_VENDOR_SHIFT)
> +#define PTN5150_REG_DEVICE_ID_VERSION		GENMASK(7, 3)
> +#define PTN5150_REG_DEVICE_ID_VENDOR		GENMASK(2, 0)
>  
> -#define PTN5150_REG_DEVICE_ID_VERSION_SHIFT	3
> -#define PTN5150_REG_DEVICE_ID_VERSION_MASK	\
> -	(0x1f << PTN5150_REG_DEVICE_ID_VERSION_SHIFT)
> -
> -#define PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT	2
> -#define PTN5150_REG_CC_PORT_ATTACHMENT_MASK	\
> -	(0x7 << PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT)
> -
> -#define PTN5150_REG_CC_VBUS_DETECTION_SHIFT	7
> -#define PTN5150_REG_CC_VBUS_DETECTION_MASK	\
> -	(0x1 << PTN5150_REG_CC_VBUS_DETECTION_SHIFT)
> -
> -#define PTN5150_REG_INT_CABLE_ATTACH_SHIFT	0
> -#define PTN5150_REG_INT_CABLE_ATTACH_MASK	\
> -	(0x1 << PTN5150_REG_INT_CABLE_ATTACH_SHIFT)
> -
> -#define PTN5150_REG_INT_CABLE_DETACH_SHIFT	1
> -#define PTN5150_REG_INT_CABLE_DETACH_MASK	\
> -	(0x1 << PTN5150_REG_CC_CABLE_DETACH_SHIFT)
> +#define PTN5150_REG_CC_PORT_ATTACHMENT		GENMASK(4, 2)
> +#define PTN5150_REG_CC_VBUS_DETECTION		BIT(7)
> +#define PTN5150_REG_INT_CABLE_ATTACH_MASK	BIT(0)
> +#define PTN5150_REG_INT_CABLE_DETACH_MASK	BIT(1)
>  
>  struct ptn5150_info {
>  	struct device *dev;
> @@ -95,9 +80,7 @@ static void ptn5150_check_state(struct ptn5150_info *info)
>  		return;
>  	}
>  
> -	port_status = ((reg_data &
> -			PTN5150_REG_CC_PORT_ATTACHMENT_MASK) >>
> -			PTN5150_REG_CC_PORT_ATTACHMENT_SHIFT);
> +	port_status = FIELD_GET(PTN5150_REG_CC_PORT_ATTACHMENT, reg_data);
>  
>  	switch (port_status) {
>  	case PTN5150_DFP_ATTACHED:
> @@ -107,8 +90,7 @@ static void ptn5150_check_state(struct ptn5150_info *info)
>  		break;
>  	case PTN5150_UFP_ATTACHED:
>  		extcon_set_state_sync(info->edev, EXTCON_USB, false);
> -		vbus = ((reg_data & PTN5150_REG_CC_VBUS_DETECTION_MASK) >>
> -			PTN5150_REG_CC_VBUS_DETECTION_SHIFT);
> +		vbus = FIELD_GET(PTN5150_REG_CC_VBUS_DETECTION, reg_data);
>  		if (vbus)
>  			gpiod_set_value_cansleep(info->vbus_gpiod, 0);
>  		else
> @@ -191,11 +173,8 @@ static int ptn5150_init_dev_type(struct ptn5150_info *info)
>  		return -EINVAL;
>  	}
>  
> -	vendor_id = ((reg_data & PTN5150_REG_DEVICE_ID_VENDOR_MASK) >>
> -				PTN5150_REG_DEVICE_ID_VENDOR_SHIFT);
> -	version_id = ((reg_data & PTN5150_REG_DEVICE_ID_VERSION_MASK) >>
> -				PTN5150_REG_DEVICE_ID_VERSION_SHIFT);
> -
> +	vendor_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VENDOR, reg_data);
> +	version_id = FIELD_GET(PTN5150_REG_DEVICE_ID_VERSION, reg_data);
>  	dev_dbg(info->dev, "Device type: version: 0x%x, vendor: 0x%x\n",
>  		version_id, vendor_id);
>  
> 

Applied it. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] extcon: ptn5150: Set the VBUS and POLARITY property state

[Chanwoo Choi]

Hi,

On 8/27/20 2:17 PM, Ramuthevar, Vadivel MuruganX wrote:
> Hi,
> 
> On 27/8/2020 12:51 pm, Chanwoo Choi wrote:
>> Hi,
>>
>> You better to change the 'state' word to 'capability'.
>> Actually, this patch doesn't change the value of property.
>> It set the capability value of property.
>>
>> "Set the VBUS and POLARITY property capability"
> Thank you for the review comments, sure will update.
>>
>> On 8/27/20 12:56 PM, Ramuthevar,Vadivel MuruganX wrote:
>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>
>>> Set the VBUS and POLARITY property state.
>>
>> ditto. Need to change the work from 'state' and 'capability'.
> Noted.
>>
>>>
>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>> ---
>>>   drivers/extcon/extcon-ptn5150.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
>>> index 8b930050a3f1..b5217a61615c 100644
>>> --- a/drivers/extcon/extcon-ptn5150.c
>>> +++ b/drivers/extcon/extcon-ptn5150.c
>>> @@ -279,6 +279,12 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c)
>>>           return ret;
>>>       }
>>>   +    extcon_set_property_capability(info->edev, EXTCON_USB,
>>> +                       EXTCON_PROP_USB_VBUS);
>>> +    extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>>> +                       EXTCON_PROP_USB_VBUS);
>>> +    extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>>> +                       EXTCON_PROP_USB_TYPEC_POLARITY);
>>
>> Need to add blank line.
> Noted.
>>
>> I understood that you set the property capability
>> because of get_flipped() function of your patch[1].
>>
>> But, I think that you need to change the value of EXTCON_PROP_USB_TYPEC_POLARITY
>> when changing the state of EXTCON_USB_HOST. The polarity property value is always
>> zero regardless of EXTCON_USB_HOST state as following: The get_flipped()[1] returns
>> always the same *flipped value.
>>
>>     EXTCON_USB_HOST is 1, EXTCON_PROP_USB_TYPEC_POLARITY is 0
>>     EXTCON_USB_HOST is 0, EXTCON_PROP_USB_TYPEC_POLARITY is 0
> by default EXTCON_PROP_USB_TYPEC_POLARITY is 1

If you don't touch the value of EXTCON_PROP_USB_TYPEC_POLARITY property,
EXTCON_PROP_USB_TYPEC_POLARITY has default value (0).

>>
>> If EXTCON_PROP_USB_TYPEC_POLARITY value is not related to any behavior,
>> you don't need to get the property value from extcon consumer driver
>> like drivers/phy/phy-lgm-usb.c.
>>
>> Actually, I don't understand why you don't handle the value
>> of EXTCON_PROP_USB_TYPEC_POLARITY.
>>
>> Or, are there any case of what drivers/phy/phy-lgm-usb.c
>> uses the different extcon device with EXTCON_PROP_USB_TYPEC_POLARITY property
>> in the future?
> Yes, you're right, user connect the different USB cable then we check polarity, accordingly driver proceeds, thanks!

OK.

>>
>> So, do you set the EXTCON_PROP_USB_TYPEC_POLARITY capability
>> for the extensibility in order to use other extcon device on later?
> yes, that might be the case as well.

OK.

>>
>>
>> [1] https://protect2.fireeye.com/v1/url?k=1fb29698-422c0d72-1fb31dd7-0cc47a6cba04-3009aa7184024984&q=1&e=566e4565-e7db-4a90-b036-fc28dbdb742f&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdevicetree%2Fmsg371828.html
>> +static int get_flipped(struct tca_apb *ta, bool *flipped)
>> +{
>> +    union extcon_property_value property;
>> +    int ret;
>> +
>> +    ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
>> +                  EXTCON_PROP_USB_TYPEC_POLARITY, &property);
>> +    if (ret) {
>> +        dev_err(ta->phy.dev, "no polarity property from extcon\n");
>> +        return ret;
>> +    }
>> +
>> +    *flipped = property.intval;
>> +
>> +    return ret;
>> +}
> Thank you for the gone through my usb-phy patch as well
> 
> Regards
> Vadivel
>>
>>
>>>       /* Initialize PTN5150 device and print vendor id and version id */
>>>       ret = ptn5150_init_dev_type(info);
>>>       if (ret)
>>>
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 2/2] extcon: ptn5150: Set the VBUS and POLARITY property state

[Ramuthevar, Vadivel MuruganX]

Hi,

On 27/8/2020 1:35 pm, Chanwoo Choi wrote:
> Hi,
> 
> On 8/27/20 2:17 PM, Ramuthevar, Vadivel MuruganX wrote:
>> Hi,
>>
>> On 27/8/2020 12:51 pm, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> You better to change the 'state' word to 'capability'.
>>> Actually, this patch doesn't change the value of property.
>>> It set the capability value of property.
>>>
>>> "Set the VBUS and POLARITY property capability"
>> Thank you for the review comments, sure will update.
>>>
>>> On 8/27/20 12:56 PM, Ramuthevar,Vadivel MuruganX wrote:
>>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>>
>>>> Set the VBUS and POLARITY property state.
>>>
>>> ditto. Need to change the work from 'state' and 'capability'.
>> Noted.
>>>
>>>>
>>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com>
>>>> ---
>>>>    drivers/extcon/extcon-ptn5150.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/extcon/extcon-ptn5150.c b/drivers/extcon/extcon-ptn5150.c
>>>> index 8b930050a3f1..b5217a61615c 100644
>>>> --- a/drivers/extcon/extcon-ptn5150.c
>>>> +++ b/drivers/extcon/extcon-ptn5150.c
>>>> @@ -279,6 +279,12 @@ static int ptn5150_i2c_probe(struct i2c_client *i2c)
>>>>            return ret;
>>>>        }
>>>>    +    extcon_set_property_capability(info->edev, EXTCON_USB,
>>>> +                       EXTCON_PROP_USB_VBUS);
>>>> +    extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>>>> +                       EXTCON_PROP_USB_VBUS);
>>>> +    extcon_set_property_capability(info->edev, EXTCON_USB_HOST,
>>>> +                       EXTCON_PROP_USB_TYPEC_POLARITY);
>>>
>>> Need to add blank line.
>> Noted.
>>>
>>> I understood that you set the property capability
>>> because of get_flipped() function of your patch[1].
>>>
>>> But, I think that you need to change the value of EXTCON_PROP_USB_TYPEC_POLARITY
>>> when changing the state of EXTCON_USB_HOST. The polarity property value is always
>>> zero regardless of EXTCON_USB_HOST state as following: The get_flipped()[1] returns
>>> always the same *flipped value.
>>>
>>>      EXTCON_USB_HOST is 1, EXTCON_PROP_USB_TYPEC_POLARITY is 0
>>>      EXTCON_USB_HOST is 0, EXTCON_PROP_USB_TYPEC_POLARITY is 0
>> by default EXTCON_PROP_USB_TYPEC_POLARITY is 1
> 
> If you don't touch the value of EXTCON_PROP_USB_TYPEC_POLARITY property,
> EXTCON_PROP_USB_TYPEC_POLARITY has default value (0).
Ok, thanks!
will update the patch, send it.

Regards
Vadivel
> 
>>>
>>> If EXTCON_PROP_USB_TYPEC_POLARITY value is not related to any behavior,
>>> you don't need to get the property value from extcon consumer driver
>>> like drivers/phy/phy-lgm-usb.c.
>>>
>>> Actually, I don't understand why you don't handle the value
>>> of EXTCON_PROP_USB_TYPEC_POLARITY.
>>>
>>> Or, are there any case of what drivers/phy/phy-lgm-usb.c
>>> uses the different extcon device with EXTCON_PROP_USB_TYPEC_POLARITY property
>>> in the future?
>> Yes, you're right, user connect the different USB cable then we check polarity, accordingly driver proceeds, thanks!
> 
> OK.
> 
>>>
>>> So, do you set the EXTCON_PROP_USB_TYPEC_POLARITY capability
>>> for the extensibility in order to use other extcon device on later?
>> yes, that might be the case as well.
> 
> OK.
> 
>>>
>>>
>>> [1] https://protect2.fireeye.com/v1/url?k=1fb29698-422c0d72-1fb31dd7-0cc47a6cba04-3009aa7184024984&q=1&e=566e4565-e7db-4a90-b036-fc28dbdb742f&u=https%3A%2F%2Fwww.spinics.net%2Flists%2Fdevicetree%2Fmsg371828.html
>>> +static int get_flipped(struct tca_apb *ta, bool *flipped)
>>> +{
>>> +    union extcon_property_value property;
>>> +    int ret;
>>> +
>>> +    ret = extcon_get_property(ta->phy.edev, EXTCON_USB_HOST,
>>> +                  EXTCON_PROP_USB_TYPEC_POLARITY, &property);
>>> +    if (ret) {
>>> +        dev_err(ta->phy.dev, "no polarity property from extcon\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    *flipped = property.intval;
>>> +
>>> +    return ret;
>>> +}
>> Thank you for the gone through my usb-phy patch as well
>>
>> Regards
>> Vadivel
>>>
>>>
>>>>        /* Initialize PTN5150 device and print vendor id and version id */
>>>>        ret = ptn5150_init_dev_type(info);
>>>>        if (ret)
>>>>
>>>
>>>
>>
>>
> 
>