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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > ---
> >   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)