linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset
@ 2021-07-27  9:56 Yassine Oudjana
  2021-07-28 15:17 ` Michael Auchter
  0 siblings, 1 reply; 3+ messages in thread
From: Yassine Oudjana @ 2021-07-27  9:56 UTC (permalink / raw)
  To: Michael Auchter
  Cc: Yassine Oudjana, MyungJoo Ham, Chanwoo Choi, Rob Herring,
	linux-kernel, devicetree

Reset the chip and set its mode to default (maintain mode set by PORT pin)
during probe to make sure it comes up in the default state.

Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
---
 drivers/extcon/extcon-usbc-tusb320.c | 111 +++++++++++++++++++++++----
 1 file changed, 97 insertions(+), 14 deletions(-)

diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
index 805af73b4152..c8d931abbf9f 100644
--- a/drivers/extcon/extcon-usbc-tusb320.c
+++ b/drivers/extcon/extcon-usbc-tusb320.c
@@ -19,15 +19,32 @@
 #define TUSB320_REG9_ATTACHED_STATE_MASK	0x3
 #define TUSB320_REG9_CABLE_DIRECTION		BIT(5)
 #define TUSB320_REG9_INTERRUPT_STATUS		BIT(4)
-#define TUSB320_ATTACHED_STATE_NONE		0x0
-#define TUSB320_ATTACHED_STATE_DFP		0x1
-#define TUSB320_ATTACHED_STATE_UFP		0x2
-#define TUSB320_ATTACHED_STATE_ACC		0x3
+
+#define TUSB320_REGA				0xa
+#define TUSB320_REGA_I2C_SOFT_RESET		BIT(3)
+#define TUSB320_REGA_MODE_SELECT_SHIFT		4
+#define TUSB320_REGA_MODE_SELECT_MASK		0x3
+
+enum tusb320_attached_state {
+	TUSB320_ATTACHED_STATE_NONE,
+	TUSB320_ATTACHED_STATE_DFP,
+	TUSB320_ATTACHED_STATE_UFP,
+	TUSB320_ATTACHED_STATE_ACC,
+};
+
+enum tusb320_mode {
+	TUSB320_MODE_PORT,
+	TUSB320_MODE_UFP,
+	TUSB320_MODE_DFP,
+	TUSB320_MODE_DRP,
+};
 
 struct tusb320_priv {
 	struct device *dev;
 	struct regmap *regmap;
 	struct extcon_dev *edev;
+
+	enum tusb320_attached_state state;
 };
 
 static const char * const tusb_attached_states[] = {
@@ -37,6 +54,13 @@ static const char * const tusb_attached_states[] = {
 	[TUSB320_ATTACHED_STATE_ACC]  = "accessory",
 };
 
+static const char * const tusb_modes[] = {
+	[TUSB320_MODE_PORT] = "maintain mode set by PORT pin",
+	[TUSB320_MODE_UFP]  = "upstream facing port",
+	[TUSB320_MODE_DFP]  = "downstream facing port",
+	[TUSB320_MODE_DRP]  = "dual role port",
+};
+
 static const unsigned int tusb320_extcon_cable[] = {
 	EXTCON_USB,
 	EXTCON_USB_HOST,
@@ -62,26 +86,77 @@ static int tusb320_check_signature(struct tusb320_priv *priv)
 	return 0;
 }
 
+static int tusb320_set_mode(struct tusb320_priv *priv, enum tusb320_mode mode)
+{
+	int ret;
+
+	/* Mode cannot be changed while cable is attached */
+	if(priv->state != TUSB320_ATTACHED_STATE_NONE)
+		return -EBUSY;
+
+	/* Write mode */
+	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
+		TUSB320_REGA_MODE_SELECT_MASK << TUSB320_REGA_MODE_SELECT_SHIFT,
+		mode << TUSB320_REGA_MODE_SELECT_SHIFT);
+	if(ret) {
+		dev_err(priv->dev, "failed to write mode: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tusb320_reset(struct tusb320_priv *priv)
+{
+	int ret;
+
+	/* Set mode to default (follow PORT pin) */
+	ret = tusb320_set_mode(priv, TUSB320_MODE_PORT);
+	if(ret && ret != -EBUSY) {
+		dev_err(priv->dev,
+			"failed to set mode to PORT: %d\n", ret);
+		return ret;
+	}
+
+	/* Perform soft reset */
+	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
+			TUSB320_REGA_I2C_SOFT_RESET, 1);
+	if(ret) {
+		dev_err(priv->dev,
+			"failed to write soft reset bit: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
 {
 	struct tusb320_priv *priv = dev_id;
-	int state, polarity;
-	unsigned reg;
+	int state, polarity, mode;
+	unsigned reg9, rega;
+
+	if (regmap_read(priv->regmap, TUSB320_REG9, &reg9)) {
+		dev_err(priv->dev, "error during register 0x9 i2c read!\n");
+		return IRQ_NONE;
+	}
 
-	if (regmap_read(priv->regmap, TUSB320_REG9, &reg)) {
-		dev_err(priv->dev, "error during i2c read!\n");
+	if (regmap_read(priv->regmap, TUSB320_REGA, &rega)) {
+		dev_err(priv->dev, "error during register 0xa i2c read!\n");
 		return IRQ_NONE;
 	}
 
-	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
+	if (!(reg9 & TUSB320_REG9_INTERRUPT_STATUS))
 		return IRQ_NONE;
 
-	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
+	state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
 		TUSB320_REG9_ATTACHED_STATE_MASK;
-	polarity = !!(reg & TUSB320_REG9_CABLE_DIRECTION);
+	polarity = !!(reg9 & TUSB320_REG9_CABLE_DIRECTION);
+	mode = (rega >> TUSB320_REGA_MODE_SELECT_SHIFT) &
+		TUSB320_REGA_MODE_SELECT_MASK;
 
-	dev_dbg(priv->dev, "attached state: %s, polarity: %d\n",
-		tusb_attached_states[state], polarity);
+	dev_dbg(priv->dev, "mode: %s, attached state: %s, polarity: %d\n",
+		tusb_modes[mode], tusb_attached_states[state], polarity);
 
 	extcon_set_state(priv->edev, EXTCON_USB,
 			 state == TUSB320_ATTACHED_STATE_UFP);
@@ -96,7 +171,10 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
 	extcon_sync(priv->edev, EXTCON_USB);
 	extcon_sync(priv->edev, EXTCON_USB_HOST);
 
-	regmap_write(priv->regmap, TUSB320_REG9, reg);
+	priv->state = state;
+
+	regmap_write(priv->regmap, TUSB320_REG9, reg9);
+	regmap_write(priv->regmap, TUSB320_REGA, rega);
 
 	return IRQ_HANDLED;
 }
@@ -137,6 +215,11 @@ static int tusb320_extcon_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* Reset chip to its default state */
+	ret = tusb320_reset(priv);
+	if(ret)
+		dev_warn(priv->dev, "failed to reset chip: %d\n", ret);
+
 	extcon_set_property_capability(priv->edev, EXTCON_USB,
 				       EXTCON_PROP_USB_TYPEC_POLARITY);
 	extcon_set_property_capability(priv->edev, EXTCON_USB_HOST,
-- 
2.32.0



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset
  2021-07-27  9:56 [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset Yassine Oudjana
@ 2021-07-28 15:17 ` Michael Auchter
  2021-07-31 11:24   ` Yassine Oudjana
  0 siblings, 1 reply; 3+ messages in thread
From: Michael Auchter @ 2021-07-28 15:17 UTC (permalink / raw)
  To: Yassine Oudjana
  Cc: MyungJoo Ham, Chanwoo Choi, Rob Herring, linux-kernel, devicetree

Hello Yassine,

On Tue, Jul 27, 2021 at 09:56:41AM +0000, Yassine Oudjana wrote:
> Reset the chip and set its mode to default (maintain mode set by PORT pin)
> during probe to make sure it comes up in the default state.
> 
> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
> ---
>  drivers/extcon/extcon-usbc-tusb320.c | 111 +++++++++++++++++++++++----
>  1 file changed, 97 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c
> index 805af73b4152..c8d931abbf9f 100644
> --- a/drivers/extcon/extcon-usbc-tusb320.c
> +++ b/drivers/extcon/extcon-usbc-tusb320.c
> @@ -19,15 +19,32 @@
>  #define TUSB320_REG9_ATTACHED_STATE_MASK	0x3
>  #define TUSB320_REG9_CABLE_DIRECTION		BIT(5)
>  #define TUSB320_REG9_INTERRUPT_STATUS		BIT(4)
> -#define TUSB320_ATTACHED_STATE_NONE		0x0
> -#define TUSB320_ATTACHED_STATE_DFP		0x1
> -#define TUSB320_ATTACHED_STATE_UFP		0x2
> -#define TUSB320_ATTACHED_STATE_ACC		0x3
> +
> +#define TUSB320_REGA				0xa
> +#define TUSB320_REGA_I2C_SOFT_RESET		BIT(3)
> +#define TUSB320_REGA_MODE_SELECT_SHIFT		4
> +#define TUSB320_REGA_MODE_SELECT_MASK		0x3
> +
> +enum tusb320_attached_state {
> +	TUSB320_ATTACHED_STATE_NONE,
> +	TUSB320_ATTACHED_STATE_DFP,
> +	TUSB320_ATTACHED_STATE_UFP,
> +	TUSB320_ATTACHED_STATE_ACC,
> +};
> +
> +enum tusb320_mode {
> +	TUSB320_MODE_PORT,
> +	TUSB320_MODE_UFP,
> +	TUSB320_MODE_DFP,
> +	TUSB320_MODE_DRP,
> +};
>  
>  struct tusb320_priv {
>  	struct device *dev;
>  	struct regmap *regmap;
>  	struct extcon_dev *edev;
> +
> +	enum tusb320_attached_state state;
>  };
>  
>  static const char * const tusb_attached_states[] = {
> @@ -37,6 +54,13 @@ static const char * const tusb_attached_states[] = {
>  	[TUSB320_ATTACHED_STATE_ACC]  = "accessory",
>  };
>  
> +static const char * const tusb_modes[] = {
> +	[TUSB320_MODE_PORT] = "maintain mode set by PORT pin",
> +	[TUSB320_MODE_UFP]  = "upstream facing port",
> +	[TUSB320_MODE_DFP]  = "downstream facing port",
> +	[TUSB320_MODE_DRP]  = "dual role port",
> +};
> +
>  static const unsigned int tusb320_extcon_cable[] = {
>  	EXTCON_USB,
>  	EXTCON_USB_HOST,
> @@ -62,26 +86,77 @@ static int tusb320_check_signature(struct tusb320_priv *priv)
>  	return 0;
>  }
>  
> +static int tusb320_set_mode(struct tusb320_priv *priv, enum tusb320_mode mode)
> +{
> +	int ret;
> +
> +	/* Mode cannot be changed while cable is attached */
> +	if(priv->state != TUSB320_ATTACHED_STATE_NONE)
> +		return -EBUSY;

When tusb320_set_mode() is called from probe() via tusb320_reset(),
priv->state will be always be 0 as it hasn't been read from the chip
yet.

Also, per CodingStyle, please ensure there's a space between the "if"
and the opening paren (here and elsewhere in this patchset)

> +
> +	/* Write mode */
> +	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
> +		TUSB320_REGA_MODE_SELECT_MASK << TUSB320_REGA_MODE_SELECT_SHIFT,
> +		mode << TUSB320_REGA_MODE_SELECT_SHIFT);
> +	if(ret) {
> +		dev_err(priv->dev, "failed to write mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tusb320_reset(struct tusb320_priv *priv)
> +{
> +	int ret;
> +
> +	/* Set mode to default (follow PORT pin) */
> +	ret = tusb320_set_mode(priv, TUSB320_MODE_PORT);
> +	if(ret && ret != -EBUSY) {
> +		dev_err(priv->dev,
> +			"failed to set mode to PORT: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Perform soft reset */
> +	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
> +			TUSB320_REGA_I2C_SOFT_RESET, 1);
> +	if(ret) {
> +		dev_err(priv->dev,
> +			"failed to write soft reset bit: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>  {
>  	struct tusb320_priv *priv = dev_id;
> -	int state, polarity;
> -	unsigned reg;
> +	int state, polarity, mode;
> +	unsigned reg9, rega;
> +
> +	if (regmap_read(priv->regmap, TUSB320_REG9, &reg9)) {
> +		dev_err(priv->dev, "error during register 0x9 i2c read!\n");
> +		return IRQ_NONE;
> +	}
>  
> -	if (regmap_read(priv->regmap, TUSB320_REG9, &reg)) {
> -		dev_err(priv->dev, "error during i2c read!\n");
> +	if (regmap_read(priv->regmap, TUSB320_REGA, &rega)) {
> +		dev_err(priv->dev, "error during register 0xa i2c read!\n");

Why is this register being read in the interrupt handler? The
datasheet's documentation for the INTERRUPT_STATUS register says that an
interrupt will be generated "whenever a CSR with RU in [the] Access field
changes" (i.e., whenever hardware has autonomously updated a value). As
far as I can see, there are no RU registers here.

>  		return IRQ_NONE;
>  	}
>  
> -	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
> +	if (!(reg9 & TUSB320_REG9_INTERRUPT_STATUS))
>  		return IRQ_NONE;
>  
> -	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
> +	state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>  		TUSB320_REG9_ATTACHED_STATE_MASK;
> -	polarity = !!(reg & TUSB320_REG9_CABLE_DIRECTION);
> +	polarity = !!(reg9 & TUSB320_REG9_CABLE_DIRECTION);
> +	mode = (rega >> TUSB320_REGA_MODE_SELECT_SHIFT) &
> +		TUSB320_REGA_MODE_SELECT_MASK;
>  
> -	dev_dbg(priv->dev, "attached state: %s, polarity: %d\n",
> -		tusb_attached_states[state], polarity);
> +	dev_dbg(priv->dev, "mode: %s, attached state: %s, polarity: %d\n",
> +		tusb_modes[mode], tusb_attached_states[state], polarity);

What's the purpose of tracing the mode here? Since the chip does not
change the mode on its own, it should always be whatever it was last set
to, correct?

>  	extcon_set_state(priv->edev, EXTCON_USB,
>  			 state == TUSB320_ATTACHED_STATE_UFP);
> @@ -96,7 +171,10 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>  	extcon_sync(priv->edev, EXTCON_USB);
>  	extcon_sync(priv->edev, EXTCON_USB_HOST);
>  
> -	regmap_write(priv->regmap, TUSB320_REG9, reg);
> +	priv->state = state;
> +
> +	regmap_write(priv->regmap, TUSB320_REG9, reg9);
> +	regmap_write(priv->regmap, TUSB320_REGA, rega);

The write to REG9 is required in order to clear the INTERRUPT_STATUS
bit, but I do not see a need to write back to REGA...

>  
>  	return IRQ_HANDLED;
>  }
> @@ -137,6 +215,11 @@ static int tusb320_extcon_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	/* Reset chip to its default state */
> +	ret = tusb320_reset(priv);
> +	if(ret)
> +		dev_warn(priv->dev, "failed to reset chip: %d\n", ret);
> +

As mentioned above, the tusb320_reset() should be probably be done after
the call to tusb320_irq_handler(), which will read and update the
attached state.

>  	extcon_set_property_capability(priv->edev, EXTCON_USB,
>  				       EXTCON_PROP_USB_TYPEC_POLARITY);
>  	extcon_set_property_capability(priv->edev, EXTCON_USB_HOST,
> -- 
> 2.32.0
> 
> 

Thanks,
 Michael

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset
  2021-07-28 15:17 ` Michael Auchter
@ 2021-07-31 11:24   ` Yassine Oudjana
  0 siblings, 0 replies; 3+ messages in thread
From: Yassine Oudjana @ 2021-07-31 11:24 UTC (permalink / raw)
  To: Michael Auchter
  Cc: Yassine Oudjana, MyungJoo Ham, Chanwoo Choi, Rob Herring,
	linux-kernel, devicetree

On Wed, Jul 28 2021 at 19:17:16 +0400, Michael Auchter 
<michael.auchter@ni.com> wrote:
> Hello Yassine,
> 
> On Tue, Jul 27, 2021 at 09:56:41AM +0000, Yassine Oudjana wrote:
>>  Reset the chip and set its mode to default (maintain mode set by 
>> PORT pin)
>>  during probe to make sure it comes up in the default state.
>> 
>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>  ---
>>   drivers/extcon/extcon-usbc-tusb320.c | 111 
>> +++++++++++++++++++++++----
>>   1 file changed, 97 insertions(+), 14 deletions(-)
>> 
>>  diff --git a/drivers/extcon/extcon-usbc-tusb320.c 
>> b/drivers/extcon/extcon-usbc-tusb320.c
>>  index 805af73b4152..c8d931abbf9f 100644
>>  --- a/drivers/extcon/extcon-usbc-tusb320.c
>>  +++ b/drivers/extcon/extcon-usbc-tusb320.c
>>  @@ -19,15 +19,32 @@
>>   #define TUSB320_REG9_ATTACHED_STATE_MASK	0x3
>>   #define TUSB320_REG9_CABLE_DIRECTION		BIT(5)
>>   #define TUSB320_REG9_INTERRUPT_STATUS		BIT(4)
>>  -#define TUSB320_ATTACHED_STATE_NONE		0x0
>>  -#define TUSB320_ATTACHED_STATE_DFP		0x1
>>  -#define TUSB320_ATTACHED_STATE_UFP		0x2
>>  -#define TUSB320_ATTACHED_STATE_ACC		0x3
>>  +
>>  +#define TUSB320_REGA				0xa
>>  +#define TUSB320_REGA_I2C_SOFT_RESET		BIT(3)
>>  +#define TUSB320_REGA_MODE_SELECT_SHIFT		4
>>  +#define TUSB320_REGA_MODE_SELECT_MASK		0x3
>>  +
>>  +enum tusb320_attached_state {
>>  +	TUSB320_ATTACHED_STATE_NONE,
>>  +	TUSB320_ATTACHED_STATE_DFP,
>>  +	TUSB320_ATTACHED_STATE_UFP,
>>  +	TUSB320_ATTACHED_STATE_ACC,
>>  +};
>>  +
>>  +enum tusb320_mode {
>>  +	TUSB320_MODE_PORT,
>>  +	TUSB320_MODE_UFP,
>>  +	TUSB320_MODE_DFP,
>>  +	TUSB320_MODE_DRP,
>>  +};
>> 
>>   struct tusb320_priv {
>>   	struct device *dev;
>>   	struct regmap *regmap;
>>   	struct extcon_dev *edev;
>>  +
>>  +	enum tusb320_attached_state state;
>>   };
>> 
>>   static const char * const tusb_attached_states[] = {
>>  @@ -37,6 +54,13 @@ static const char * const tusb_attached_states[] 
>> = {
>>   	[TUSB320_ATTACHED_STATE_ACC]  = "accessory",
>>   };
>> 
>>  +static const char * const tusb_modes[] = {
>>  +	[TUSB320_MODE_PORT] = "maintain mode set by PORT pin",
>>  +	[TUSB320_MODE_UFP]  = "upstream facing port",
>>  +	[TUSB320_MODE_DFP]  = "downstream facing port",
>>  +	[TUSB320_MODE_DRP]  = "dual role port",
>>  +};
>>  +
>>   static const unsigned int tusb320_extcon_cable[] = {
>>   	EXTCON_USB,
>>   	EXTCON_USB_HOST,
>>  @@ -62,26 +86,77 @@ static int tusb320_check_signature(struct 
>> tusb320_priv *priv)
>>   	return 0;
>>   }
>> 
>>  +static int tusb320_set_mode(struct tusb320_priv *priv, enum 
>> tusb320_mode mode)
>>  +{
>>  +	int ret;
>>  +
>>  +	/* Mode cannot be changed while cable is attached */
>>  +	if(priv->state != TUSB320_ATTACHED_STATE_NONE)
>>  +		return -EBUSY;
> 
> When tusb320_set_mode() is called from probe() via tusb320_reset(),
> priv->state will be always be 0 as it hasn't been read from the chip
> yet.

I'll move the call after the first tusb320_irq_handler() call to read 
the state first.

> 
> Also, per CodingStyle, please ensure there's a space between the "if"
> and the opening paren (here and elsewhere in this patchset)
> 

Noted.

>>  +
>>  +	/* Write mode */
>>  +	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
>>  +		TUSB320_REGA_MODE_SELECT_MASK << TUSB320_REGA_MODE_SELECT_SHIFT,
>>  +		mode << TUSB320_REGA_MODE_SELECT_SHIFT);
>>  +	if(ret) {
>>  +		dev_err(priv->dev, "failed to write mode: %d\n", ret);
>>  +		return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int tusb320_reset(struct tusb320_priv *priv)
>>  +{
>>  +	int ret;
>>  +
>>  +	/* Set mode to default (follow PORT pin) */
>>  +	ret = tusb320_set_mode(priv, TUSB320_MODE_PORT);
>>  +	if(ret && ret != -EBUSY) {
>>  +		dev_err(priv->dev,
>>  +			"failed to set mode to PORT: %d\n", ret);
>>  +		return ret;
>>  +	}
>>  +
>>  +	/* Perform soft reset */
>>  +	ret = regmap_write_bits(priv->regmap, TUSB320_REGA,
>>  +			TUSB320_REGA_I2C_SOFT_RESET, 1);
>>  +	if(ret) {
>>  +		dev_err(priv->dev,
>>  +			"failed to write soft reset bit: %d\n", ret);
>>  +		return ret;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>   static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>>   {
>>   	struct tusb320_priv *priv = dev_id;
>>  -	int state, polarity;
>>  -	unsigned reg;
>>  +	int state, polarity, mode;
>>  +	unsigned reg9, rega;
>>  +
>>  +	if (regmap_read(priv->regmap, TUSB320_REG9, &reg9)) {
>>  +		dev_err(priv->dev, "error during register 0x9 i2c read!\n");
>>  +		return IRQ_NONE;
>>  +	}
>> 
>>  -	if (regmap_read(priv->regmap, TUSB320_REG9, &reg)) {
>>  -		dev_err(priv->dev, "error during i2c read!\n");
>>  +	if (regmap_read(priv->regmap, TUSB320_REGA, &rega)) {
>>  +		dev_err(priv->dev, "error during register 0xa i2c read!\n");
> 
> Why is this register being read in the interrupt handler? The
> datasheet's documentation for the INTERRUPT_STATUS register says that 
> an
> interrupt will be generated "whenever a CSR with RU in [the] Access 
> field
> changes" (i.e., whenever hardware has autonomously updated a value). 
> As
> far as I can see, there are no RU registers here.

My chip had its mode set to UFP by default which contradicted the 
datasheet where
it's stated that the default is to follow the PORT pin, so I thought it 
was being
changed on its own somehow, so I read the register here to debug. But 
thinking
about it now, it's probably being set to UFP by the bootloader since as 
you said
the chip doesn't change its mode on its own. I'll just remove this 
change.

> 
>>   		return IRQ_NONE;
>>   	}
>> 
>>  -	if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
>>  +	if (!(reg9 & TUSB320_REG9_INTERRUPT_STATUS))
>>   		return IRQ_NONE;
>> 
>>  -	state = (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>>  +	state = (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>>   		TUSB320_REG9_ATTACHED_STATE_MASK;
>>  -	polarity = !!(reg & TUSB320_REG9_CABLE_DIRECTION);
>>  +	polarity = !!(reg9 & TUSB320_REG9_CABLE_DIRECTION);
>>  +	mode = (rega >> TUSB320_REGA_MODE_SELECT_SHIFT) &
>>  +		TUSB320_REGA_MODE_SELECT_MASK;
>> 
>>  -	dev_dbg(priv->dev, "attached state: %s, polarity: %d\n",
>>  -		tusb_attached_states[state], polarity);
>>  +	dev_dbg(priv->dev, "mode: %s, attached state: %s, polarity: %d\n",
>>  +		tusb_modes[mode], tusb_attached_states[state], polarity);
> 
> What's the purpose of tracing the mode here? Since the chip does not
> change the mode on its own, it should always be whatever it was last 
> set
> to, correct?

Same answer as question above.

> 
>>   	extcon_set_state(priv->edev, EXTCON_USB,
>>   			 state == TUSB320_ATTACHED_STATE_UFP);
>>  @@ -96,7 +171,10 @@ static irqreturn_t tusb320_irq_handler(int irq, 
>> void *dev_id)
>>   	extcon_sync(priv->edev, EXTCON_USB);
>>   	extcon_sync(priv->edev, EXTCON_USB_HOST);
>> 
>>  -	regmap_write(priv->regmap, TUSB320_REG9, reg);
>>  +	priv->state = state;
>>  +
>>  +	regmap_write(priv->regmap, TUSB320_REG9, reg9);
>>  +	regmap_write(priv->regmap, TUSB320_REGA, rega);
> 
> The write to REG9 is required in order to clear the INTERRUPT_STATUS
> bit, but I do not see a need to write back to REGA...

I didn't figure out the reason for this write actually so I got 
confused. I'll remove
this along with reading REGA and tracing the mode.

> 
>> 
>>   	return IRQ_HANDLED;
>>   }
>>  @@ -137,6 +215,11 @@ static int tusb320_extcon_probe(struct 
>> i2c_client *client,
>>   		return ret;
>>   	}
>> 
>>  +	/* Reset chip to its default state */
>>  +	ret = tusb320_reset(priv);
>>  +	if(ret)
>>  +		dev_warn(priv->dev, "failed to reset chip: %d\n", ret);
>>  +
> 
> As mentioned above, the tusb320_reset() should be probably be done 
> after
> the call to tusb320_irq_handler(), which will read and update the
> attached state.
> 
>>   	extcon_set_property_capability(priv->edev, EXTCON_USB,
>>   				       EXTCON_PROP_USB_TYPEC_POLARITY);
>>   	extcon_set_property_capability(priv->edev, EXTCON_USB_HOST,
>>  --
>>  2.32.0
>> 
>> 
> 
> Thanks,
>  Michael

Regards,
Yassine



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-07-31 11:25 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27  9:56 [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset Yassine Oudjana
2021-07-28 15:17 ` Michael Auchter
2021-07-31 11:24   ` Yassine Oudjana

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).