Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port type

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Oliver,

I'm sorry, I noticed a problem with this...

On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote:
> The TI HD3SS3220 Type-C controller supports configuring the port type
> it will operate as through the MODE_SELECT field of the General
> Control Register.
> 
> Configure the port type based on the fwnode property "power-role"
> during probe, and through the port_type_set typec_operation.
> 
> The MODE_SELECT field can only be changed when the controller is in
> unattached state, so follow the sequence recommended by the datasheet to:
> 1. disable termination on CC pins to disable the controller
> 2. change the mode
> 3. re-enable termination
> 
> This will effectively cause a connected device to disconnect
> for the duration of the mode change.

Changing the type of the port is really problematic, and IMO we should
actually never support that.

Consider for example, if your port is sink only, then the platform
almost certainly can't drive the VBUS. This patch would still allow
the port to be changed to source port.

Sorry for not realising this in v1.

I think what you want here is just a power role swap. Currently power
role swap is only supported when USB PD is supported in the class
code, but since the USB Type-C specification quite clearly states that
power role and data role swap can be optionally supported even when
USB PD is not supported (section 2.3.3) we need to fix that:

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 58f40156de56..ee81909565a4 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device *dev,
                return -EOPNOTSUPP;
        }

-       if (port->pwr_opmode != TYPEC_PWR_MODE_PD) {
-               dev_dbg(dev, "partner unable to swap power role\n");
-               return -EIO;
-       }
-
        ret = sysfs_match_string(typec_roles, buf);
        if (ret < 0)
                return ret;


After that it should be possible to do power role swap also in this
driver when the port is DRP capable.

> Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com>
> ---
>  drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c
> index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644
> --- a/drivers/usb/typec/hd3ss3220.c
> +++ b/drivers/usb/typec/hd3ss3220.c
> @@ -35,10 +35,16 @@
>  #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS		BIT(4)
>  
>  /* Register HD3SS3220_REG_GEN_CTRL*/
> +#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM		BIT(0)
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK		(BIT(2) | BIT(1))
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT	0x00
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK	BIT(1)
>  #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC	(BIT(2) | BIT(1))
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK		(BIT(5) | BIT(4))
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT	0x00
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP		BIT(5)
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP		BIT(4)
> +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP		(BIT(5) | BIT(4))
>  
>  struct hd3ss3220 {
>  	struct device *dev;
> @@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm
>  				  current_mode);
>  }
>  
> +static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type)
> +{
> +	int mode_select, err;
> +
> +	switch (type) {
> +	case TYPEC_PORT_SRC:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP;
> +		break;
> +	case TYPEC_PORT_SNK:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP;
> +		break;
> +	case TYPEC_PORT_DRP:
> +		mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP;
> +		break;
> +	default:
> +		dev_err(hd3ss3220->dev, "bad port type: %d\n", type);
> +		return -EINVAL;
> +	}
> +
> +	/* Disable termination before changing MODE_SELECT as required by datasheet */
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM);
> +	if (err < 0) {
> +		dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err);
> +		return err;
> +	}
> +
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK,
> +				 mode_select);
> +	if (err < 0) {
> +		dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err);
> +		regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				   HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
> +		return err;
> +	}
> +
> +	err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> +				 HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0);
> +	if (err < 0)
> +		dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err);
> +
> +	return err;
> +}
> +
>  static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref)
>  {
>  	return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL,
> @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role)
>  	return ret;
>  }
>  
> +static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type)
> +{
> +	struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port);
> +
> +	return hd3ss3220_set_port_type(hd3ss3220, type);
> +}

This wrapper seems completely useless. You only need one function here
for the callback.

>  static const struct typec_operations hd3ss3220_ops = {
> -	.dr_set = hd3ss3220_dr_set
> +	.dr_set = hd3ss3220_dr_set,
> +	.port_type_set = hd3ss3220_port_type_set,
>  };

So here I think you should implement the pr_set callback instead.

Let me kwno wh

>  static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220)
> @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client)
>  	if (ret != 0 && ret != -EINVAL && ret != -ENXIO)
>  		goto err_put_role;
>  
> +	ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type);
> +	if (ret < 0)
> +		goto err_put_role;
> +
>  	hd3ss3220->port = typec_register_port(&client->dev, &typec_cap);
>  	if (IS_ERR(hd3ss3220->port)) {
>  		ret = PTR_ERR(hd3ss3220->port);
> 

thanks,

-- 
heikki