From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Konrad Dybcio <konradybcio@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
usb4-upstream@oss.qualcomm.com,
Raghavendra Thoorpu <rthoorpu@qti.qualcomm.com>,
Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>,
Jack Pham <jack.pham@oss.qualcomm.com>
Subject: Re: [PATCH] usb: typec: mux: ps883x: Power the retimer off when not in use
Date: Mon, 27 Apr 2026 12:22:39 +0300 [thread overview]
Message-ID: <ae8q3yLta16oS5At@kuha> (raw)
In-Reply-To: <20260420-topic-ps883x_unused_reset-v1-1-7aabf7004d2a@oss.qualcomm.com>
On Mon, Apr 20, 2026 at 01:40:28PM +0200, Konrad Dybcio wrote:
> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>
> When there's nothing going through the retimer, there's no reason to
> keep it online. Put it in reset when possible to save power.
>
> Also, remove the register cache-compare optimization as it makes little
> sense now that the chip resets during almost all transitions and
> tracking the validity of that cache becomes a headache.
>
> Suggested-by: Jack Pham <jack.pham@oss.qualcomm.com>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Note most of the diff happens to be there because I need to move
> ps883x_(en/dis)able_vregs() around..
> ---
> drivers/usb/typec/mux/ps883x.c | 196 ++++++++++++++++++++++++-----------------
> 1 file changed, 114 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index 1256252eceed..f52443638ee2 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -61,19 +61,110 @@ struct ps883x_retimer {
> struct mutex lock; /* protect non-concurrent retimer & switch */
>
> enum typec_orientation orientation;
> - u8 cfg0;
> - u8 cfg1;
> - u8 cfg2;
> + bool in_reset;
> };
>
> +static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
> +{
> + struct device *dev = &retimer->client->dev;
> + int ret;
> +
> + ret = regulator_enable(retimer->vdd33_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regulator_enable(retimer->vdd33_cap_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
> + goto err_vdd33_disable;
> + }
> +
> + usleep_range(4000, 10000);
> +
> + ret = regulator_enable(retimer->vdd_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
> + goto err_vdd33_cap_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddar_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
> + goto err_vdd_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddat_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
> + goto err_vddar_disable;
> + }
> +
> + ret = regulator_enable(retimer->vddio_supply);
> + if (ret) {
> + dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
> + goto err_vddat_disable;
> + }
> +
> + return 0;
> +
> +err_vddat_disable:
> + regulator_disable(retimer->vddat_supply);
> +err_vddar_disable:
> + regulator_disable(retimer->vddar_supply);
> +err_vdd_disable:
> + regulator_disable(retimer->vdd_supply);
> +err_vdd33_cap_disable:
> + regulator_disable(retimer->vdd33_cap_supply);
> +err_vdd33_disable:
> + regulator_disable(retimer->vdd33_supply);
> +
> + return ret;
> +}
> +
> +static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
> +{
> + regulator_disable(retimer->vddio_supply);
> + regulator_disable(retimer->vddat_supply);
> + regulator_disable(retimer->vddar_supply);
> + regulator_disable(retimer->vdd_supply);
> + regulator_disable(retimer->vdd33_cap_supply);
> + regulator_disable(retimer->vdd33_supply);
> +}
> +
> +static void ps883x_reset(struct ps883x_retimer *retimer)
> +{
> + if (retimer->in_reset)
> + return;
> +
> + gpiod_set_value(retimer->reset_gpio, 1);
> + ps883x_disable_vregs(retimer);
> + retimer->in_reset = true;
> +}
> +
> static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> - int cfg1, int cfg2)
> + int cfg1, int cfg2, bool reset)
> {
> struct device *dev = &retimer->client->dev;
> int ret;
>
> - if (retimer->cfg0 == cfg0 && retimer->cfg1 == cfg1 && retimer->cfg2 == cfg2)
> + if (reset) {
> + ps883x_reset(retimer);
> +
> return 0;
> + } else if (retimer->in_reset) {
> + ret = ps883x_enable_vregs(retimer);
> + if (ret)
> + return ret;
> +
> + gpiod_set_value(retimer->reset_gpio, 0);
> +
> + /* firmware initialization delay */
> + msleep(60);
> +
> + retimer->in_reset = false;
> + }
>
> ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
> if (ret) {
> @@ -93,10 +184,6 @@ static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> return ret;
> }
>
> - retimer->cfg0 = cfg0;
> - retimer->cfg1 = cfg1;
> - retimer->cfg2 = cfg2;
> -
> return 0;
> }
>
> @@ -107,6 +194,7 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> int cfg0 = CONN_STATUS_0_CONNECTION_PRESENT;
> int cfg1 = 0x00;
> int cfg2 = 0x00;
> + bool reset = false;
>
> if (retimer->orientation == TYPEC_ORIENTATION_REVERSE)
> cfg0 |= CONN_STATUS_0_ORIENTATION_REVERSED;
> @@ -148,9 +236,13 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> }
> } else {
> switch (state->mode) {
> + /* SAFE can be transient or point to an actual disconnect */
> case TYPEC_STATE_SAFE:
> + reset = retimer->orientation == TYPEC_ORIENTATION_NONE;
> + break;
> /* USB2 pins don't even go through this chip */
> case TYPEC_MODE_USB2:
> + reset = true;
> break;
> case TYPEC_STATE_USB:
> case TYPEC_MODE_USB3:
> @@ -171,7 +263,7 @@ static int ps883x_set(struct ps883x_retimer *retimer, struct typec_retimer_state
> }
> }
>
> - return ps883x_configure(retimer, cfg0, cfg1, cfg2);
> + return ps883x_configure(retimer, cfg0, cfg1, cfg2, reset);
> }
>
> static int ps883x_sw_set(struct typec_switch_dev *sw,
> @@ -184,11 +276,19 @@ static int ps883x_sw_set(struct typec_switch_dev *sw,
> if (ret)
> return ret;
>
> - mutex_lock(&retimer->lock);
> + guard(mutex)(&retimer->lock);
>
> if (retimer->orientation != orientation) {
> retimer->orientation = orientation;
>
> + /*
> + * Orientation notifications usually come prior to mode switch
> + * events. If the retimer is already in reset, we still want to
> + * cache the new orientation value for the subsequent ps883x_set().
> + */
> + if (retimer->in_reset)
> + return 0;
> +
> ret = regmap_assign_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> CONN_STATUS_0_ORIENTATION_REVERSED,
> orientation == TYPEC_ORIENTATION_REVERSE);
> @@ -196,8 +296,6 @@ static int ps883x_sw_set(struct typec_switch_dev *sw,
> dev_err(&retimer->client->dev, "failed to set orientation: %d\n", ret);
> }
>
> - mutex_unlock(&retimer->lock);
> -
> return ret;
> }
>
> @@ -222,75 +320,6 @@ static int ps883x_retimer_set(struct typec_retimer *rtmr,
> return typec_mux_set(retimer->typec_mux, &mux_state);
> }
>
> -static int ps883x_enable_vregs(struct ps883x_retimer *retimer)
> -{
> - struct device *dev = &retimer->client->dev;
> - int ret;
> -
> - ret = regulator_enable(retimer->vdd33_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD 3.3V regulator: %d\n", ret);
> - return ret;
> - }
> -
> - ret = regulator_enable(retimer->vdd33_cap_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD 3.3V CAP regulator: %d\n", ret);
> - goto err_vdd33_disable;
> - }
> -
> - usleep_range(4000, 10000);
> -
> - ret = regulator_enable(retimer->vdd_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD regulator: %d\n", ret);
> - goto err_vdd33_cap_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddar_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD AR regulator: %d\n", ret);
> - goto err_vdd_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddat_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD AT regulator: %d\n", ret);
> - goto err_vddar_disable;
> - }
> -
> - ret = regulator_enable(retimer->vddio_supply);
> - if (ret) {
> - dev_err(dev, "cannot enable VDD IO regulator: %d\n", ret);
> - goto err_vddat_disable;
> - }
> -
> - return 0;
> -
> -err_vddat_disable:
> - regulator_disable(retimer->vddat_supply);
> -err_vddar_disable:
> - regulator_disable(retimer->vddar_supply);
> -err_vdd_disable:
> - regulator_disable(retimer->vdd_supply);
> -err_vdd33_cap_disable:
> - regulator_disable(retimer->vdd33_cap_supply);
> -err_vdd33_disable:
> - regulator_disable(retimer->vdd33_supply);
> -
> - return ret;
> -}
> -
> -static void ps883x_disable_vregs(struct ps883x_retimer *retimer)
> -{
> - regulator_disable(retimer->vddio_supply);
> - regulator_disable(retimer->vddat_supply);
> - regulator_disable(retimer->vddar_supply);
> - regulator_disable(retimer->vdd_supply);
> - regulator_disable(retimer->vdd33_cap_supply);
> - regulator_disable(retimer->vdd33_supply);
> -}
> -
> static int ps883x_get_vregs(struct ps883x_retimer *retimer)
> {
> struct device *dev = &retimer->client->dev;
> @@ -422,6 +451,9 @@ static int ps883x_retimer_probe(struct i2c_client *client)
> }
> }
>
> + /* Keep the retimer in reset until a Type-C notification comes */
> + ps883x_reset(retimer);
> +
> sw_desc.drvdata = retimer;
> sw_desc.fwnode = dev_fwnode(dev);
> sw_desc.set = ps883x_sw_set;
>
> ---
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> change-id: 20260420-topic-ps883x_unused_reset-9af0909cefac
>
> Best regards,
> --
> Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
--
heikki
prev parent reply other threads:[~2026-04-27 9:22 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-20 11:40 [PATCH] usb: typec: mux: ps883x: Power the retimer off when not in use Konrad Dybcio
2026-04-24 16:31 ` Anthony Ruhier
2026-04-27 9:22 ` Heikki Krogerus [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ae8q3yLta16oS5At@kuha \
--to=heikki.krogerus@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=jack.pham@oss.qualcomm.com \
--cc=konrad.dybcio@oss.qualcomm.com \
--cc=konradybcio@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=rthoorpu@qti.qualcomm.com \
--cc=usb4-upstream@oss.qualcomm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox