Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sat, 31 Jul 2021 11:24:57 +0000
To:     Michael Auchter <michael.auchter@ni.com>
From:   Yassine Oudjana <y.oudjana@protonmail.com>
Cc:     Yassine Oudjana <y.oudjana@protonmail.com>,
        MyungJoo Ham <myungjoo.ham@samsung.com>,
        Chanwoo Choi <cw00.choi@samsung.com>,
        Rob Herring <robh+dt@kernel.org>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Reply-To: Yassine Oudjana <y.oudjana@protonmail.com>
Subject: Re: [PATCH v2 1/3] extcon: usbc-tusb320: Add support for mode setting and reset
Message-ID: <27V3XQ.2D0PFTDOHG4Q3@protonmail.com>
In-Reply-To: <YQF0/P6ngk7pjSCH@xaphan>
References: <Fz2Ehjftp2eVDCxfgd4j3TYcrmSf5ForizOdT7rngo@cp7-web-042.plabs.ch> <YQF0/P6ngk7pjSCH@xaphan>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jul 28 2021 at 19:17:16 +0400, Michael Auchter=20
<michael.auchter@ni.com> wrote:
> Hello Yassine,
>=20
> 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=20
>> PORT pin)
>>  during probe to make sure it comes up in the default state.
>>=20
>>  Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com>
>>  ---
>>   drivers/extcon/extcon-usbc-tusb320.c | 111=20
>> +++++++++++++++++++++++----
>>   1 file changed, 97 insertions(+), 14 deletions(-)
>>=20
>>  diff --git a/drivers/extcon/extcon-usbc-tusb320.c=20
>> 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=090x3
>>   #define TUSB320_REG9_CABLE_DIRECTION=09=09BIT(5)
>>   #define TUSB320_REG9_INTERRUPT_STATUS=09=09BIT(4)
>>  -#define TUSB320_ATTACHED_STATE_NONE=09=090x0
>>  -#define TUSB320_ATTACHED_STATE_DFP=09=090x1
>>  -#define TUSB320_ATTACHED_STATE_UFP=09=090x2
>>  -#define TUSB320_ATTACHED_STATE_ACC=09=090x3
>>  +
>>  +#define TUSB320_REGA=09=09=09=090xa
>>  +#define TUSB320_REGA_I2C_SOFT_RESET=09=09BIT(3)
>>  +#define TUSB320_REGA_MODE_SELECT_SHIFT=09=094
>>  +#define TUSB320_REGA_MODE_SELECT_MASK=09=090x3
>>  +
>>  +enum tusb320_attached_state {
>>  +=09TUSB320_ATTACHED_STATE_NONE,
>>  +=09TUSB320_ATTACHED_STATE_DFP,
>>  +=09TUSB320_ATTACHED_STATE_UFP,
>>  +=09TUSB320_ATTACHED_STATE_ACC,
>>  +};
>>  +
>>  +enum tusb320_mode {
>>  +=09TUSB320_MODE_PORT,
>>  +=09TUSB320_MODE_UFP,
>>  +=09TUSB320_MODE_DFP,
>>  +=09TUSB320_MODE_DRP,
>>  +};
>>=20
>>   struct tusb320_priv {
>>   =09struct device *dev;
>>   =09struct regmap *regmap;
>>   =09struct extcon_dev *edev;
>>  +
>>  +=09enum tusb320_attached_state state;
>>   };
>>=20
>>   static const char * const tusb_attached_states[] =3D {
>>  @@ -37,6 +54,13 @@ static const char * const tusb_attached_states[]=20
>> =3D {
>>   =09[TUSB320_ATTACHED_STATE_ACC]  =3D "accessory",
>>   };
>>=20
>>  +static const char * const tusb_modes[] =3D {
>>  +=09[TUSB320_MODE_PORT] =3D "maintain mode set by PORT pin",
>>  +=09[TUSB320_MODE_UFP]  =3D "upstream facing port",
>>  +=09[TUSB320_MODE_DFP]  =3D "downstream facing port",
>>  +=09[TUSB320_MODE_DRP]  =3D "dual role port",
>>  +};
>>  +
>>   static const unsigned int tusb320_extcon_cable[] =3D {
>>   =09EXTCON_USB,
>>   =09EXTCON_USB_HOST,
>>  @@ -62,26 +86,77 @@ static int tusb320_check_signature(struct=20
>> tusb320_priv *priv)
>>   =09return 0;
>>   }
>>=20
>>  +static int tusb320_set_mode(struct tusb320_priv *priv, enum=20
>> tusb320_mode mode)
>>  +{
>>  +=09int ret;
>>  +
>>  +=09/* Mode cannot be changed while cable is attached */
>>  +=09if(priv->state !=3D TUSB320_ATTACHED_STATE_NONE)
>>  +=09=09return -EBUSY;
>=20
> 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=20
the state first.

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

Noted.

>>  +
>>  +=09/* Write mode */
>>  +=09ret =3D regmap_write_bits(priv->regmap, TUSB320_REGA,
>>  +=09=09TUSB320_REGA_MODE_SELECT_MASK << TUSB320_REGA_MODE_SELECT_SHIFT,
>>  +=09=09mode << TUSB320_REGA_MODE_SELECT_SHIFT);
>>  +=09if(ret) {
>>  +=09=09dev_err(priv->dev, "failed to write mode: %d\n", ret);
>>  +=09=09return ret;
>>  +=09}
>>  +
>>  +=09return 0;
>>  +}
>>  +
>>  +static int tusb320_reset(struct tusb320_priv *priv)
>>  +{
>>  +=09int ret;
>>  +
>>  +=09/* Set mode to default (follow PORT pin) */
>>  +=09ret =3D tusb320_set_mode(priv, TUSB320_MODE_PORT);
>>  +=09if(ret && ret !=3D -EBUSY) {
>>  +=09=09dev_err(priv->dev,
>>  +=09=09=09"failed to set mode to PORT: %d\n", ret);
>>  +=09=09return ret;
>>  +=09}
>>  +
>>  +=09/* Perform soft reset */
>>  +=09ret =3D regmap_write_bits(priv->regmap, TUSB320_REGA,
>>  +=09=09=09TUSB320_REGA_I2C_SOFT_RESET, 1);
>>  +=09if(ret) {
>>  +=09=09dev_err(priv->dev,
>>  +=09=09=09"failed to write soft reset bit: %d\n", ret);
>>  +=09=09return ret;
>>  +=09}
>>  +
>>  +=09return 0;
>>  +}
>>  +
>>   static irqreturn_t tusb320_irq_handler(int irq, void *dev_id)
>>   {
>>   =09struct tusb320_priv *priv =3D dev_id;
>>  -=09int state, polarity;
>>  -=09unsigned reg;
>>  +=09int state, polarity, mode;
>>  +=09unsigned reg9, rega;
>>  +
>>  +=09if (regmap_read(priv->regmap, TUSB320_REG9, &reg9)) {
>>  +=09=09dev_err(priv->dev, "error during register 0x9 i2c read!\n");
>>  +=09=09return IRQ_NONE;
>>  +=09}
>>=20
>>  -=09if (regmap_read(priv->regmap, TUSB320_REG9, &reg)) {
>>  -=09=09dev_err(priv->dev, "error during i2c read!\n");
>>  +=09if (regmap_read(priv->regmap, TUSB320_REGA, &rega)) {
>>  +=09=09dev_err(priv->dev, "error during register 0xa i2c read!\n");
>=20
> Why is this register being read in the interrupt handler? The
> datasheet's documentation for the INTERRUPT_STATUS register says that=20
> an
> interrupt will be generated "whenever a CSR with RU in [the] Access=20
> field
> changes" (i.e., whenever hardware has autonomously updated a value).=20
> 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=20
datasheet where
it's stated that the default is to follow the PORT pin, so I thought it=20
was being
changed on its own somehow, so I read the register here to debug. But=20
thinking
about it now, it's probably being set to UFP by the bootloader since as=20
you said
the chip doesn't change its mode on its own. I'll just remove this=20
change.

>=20
>>   =09=09return IRQ_NONE;
>>   =09}
>>=20
>>  -=09if (!(reg & TUSB320_REG9_INTERRUPT_STATUS))
>>  +=09if (!(reg9 & TUSB320_REG9_INTERRUPT_STATUS))
>>   =09=09return IRQ_NONE;
>>=20
>>  -=09state =3D (reg >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>>  +=09state =3D (reg9 >> TUSB320_REG9_ATTACHED_STATE_SHIFT) &
>>   =09=09TUSB320_REG9_ATTACHED_STATE_MASK;
>>  -=09polarity =3D !!(reg & TUSB320_REG9_CABLE_DIRECTION);
>>  +=09polarity =3D !!(reg9 & TUSB320_REG9_CABLE_DIRECTION);
>>  +=09mode =3D (rega >> TUSB320_REGA_MODE_SELECT_SHIFT) &
>>  +=09=09TUSB320_REGA_MODE_SELECT_MASK;
>>=20
>>  -=09dev_dbg(priv->dev, "attached state: %s, polarity: %d\n",
>>  -=09=09tusb_attached_states[state], polarity);
>>  +=09dev_dbg(priv->dev, "mode: %s, attached state: %s, polarity: %d\n",
>>  +=09=09tusb_modes[mode], tusb_attached_states[state], polarity);
>=20
> 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=20
> set
> to, correct?

Same answer as question above.

>=20
>>   =09extcon_set_state(priv->edev, EXTCON_USB,
>>   =09=09=09 state =3D=3D TUSB320_ATTACHED_STATE_UFP);
>>  @@ -96,7 +171,10 @@ static irqreturn_t tusb320_irq_handler(int irq,=20
>> void *dev_id)
>>   =09extcon_sync(priv->edev, EXTCON_USB);
>>   =09extcon_sync(priv->edev, EXTCON_USB_HOST);
>>=20
>>  -=09regmap_write(priv->regmap, TUSB320_REG9, reg);
>>  +=09priv->state =3D state;
>>  +
>>  +=09regmap_write(priv->regmap, TUSB320_REG9, reg9);
>>  +=09regmap_write(priv->regmap, TUSB320_REGA, rega);
>=20
> 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=20
confused. I'll remove
this along with reading REGA and tracing the mode.

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

Regards,
Yassine