On Thu, Mar 07, 2019 at 05:36:01PM +0100, Hans de Goede wrote:
> After commit ea3b4d5523bc ("usb: typec: fusb302: Resolve fixed power role
> contract setup"), tcpm_set_cc always calls fusb302_set_toggling.
>
> Before this refactor tcpm_set_cc does the following:
>
> 1) fusb302_set_toggling(TOGGLING_MODE_OFF),
> this sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG.
> 2) fusb302_set_cc_pull(...).
> 3) "reset cc status".
> 4) if pull-up fusb302_set_src_current(...).
> 5) if pull-up or pull-down enable bc-lvl resp comp-chng irq.
> 6) fusb302_set_toggling(new-toggling-mode), which again
> sets both FUSB_REG_MASK_BC_LVL and FUSB_REG_MASK_COMP_CHNG disabling
> the just enabled irq. fusb302_set_toggling is skipped when the new
> toggling mode is TOGGLING_MODE_OFF because this is already done in 1,
> note in this case 5) is a no-op.
>
> When we are toggling the bits set by fusb302_set_cc_pull will be ignored
> until we turn toggling off, so we can safely move the fusb302_set_cc_pull
> call to before setting TOGGLING_MODE_OFF.
>
> Either we are not toggling yet, or the src-current has already been set,
> so we can also safely set the src-current earlier, allowing us to do the
> fusb302_set_toggling(TOGGLING_MODE_OFF) call at the same time as we
> set the other toggling modes. Also setting the src-current is a no-op
> when not enabling pull-ups, so we can drop the if.
>
> And since the second fusb302_set_toggling undoes the effects of step 5,
> we can skip step 5, the bc-lvl resp comp-chng irq wil be enabled by
> fusb302_handle_togdone_snk resp. fusb302_handle_togdone_src when toggling
> is done.
>
> Together this allows us to simplify things to:
>
> 1) fusb302_set_cc_pull(...)
> 2) "reset cc status"
> 3) fusb302_set_src_current(...)
> 4) fusb302_set_toggling(new-toggling-mode)
>
> This commit does this, leading to a nice cleanup.
>
> Signed-off-by: Hans de Goede <hdego...@redhat.com>
I think I understand the logic, but I think it would be really beneficial
if this (and the rest of the series) can be tested independently (ie
by someone with hardware).
Reviewed-by: Guenter Roeck <li...@roeck-us.net>
> ---
> drivers/usb/typec/tcpm/fusb302.c | 85 ++++++--------------------------
> 1 file changed, 15 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm/fusb302.c
> b/drivers/usb/typec/tcpm/fusb302.c
> index d2cce67289d4..bcb38e397712 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -676,7 +676,6 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> tcpc_dev);
> int ret = 0;
> bool pull_up, pull_down;
> - u8 rd_mda;
> enum toggling_mode mode;
>
> mutex_lock(&chip->lock);
> @@ -684,16 +683,19 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> case TYPEC_CC_OPEN:
> pull_up = false;
> pull_down = false;
> + mode = TOGGLING_MODE_OFF;
> break;
> case TYPEC_CC_RD:
> pull_up = false;
> pull_down = true;
> + mode = TOGGLING_MODE_SNK;
> break;
> case TYPEC_CC_RP_DEF:
> case TYPEC_CC_RP_1_5:
> case TYPEC_CC_RP_3_0:
> pull_up = true;
> pull_down = false;
> + mode = TOGGLING_MODE_SRC;
> break;
> default:
> fusb302_log(chip, "unsupported cc value %s",
> @@ -701,11 +703,9 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> ret = -EINVAL;
> goto done;
> }
> - ret = fusb302_set_toggling(chip, TOGGLING_MODE_OFF);
> - if (ret < 0) {
> - fusb302_log(chip, "cannot stop toggling, ret=%d", ret);
> - goto done;
> - }
> +
> + fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
> +
> ret = fusb302_set_cc_pull(chip, pull_up, pull_down);
> if (ret < 0) {
> fusb302_log(chip,
> @@ -718,74 +718,19 @@ static int tcpm_set_cc(struct tcpc_dev *dev, enum
> typec_cc_status cc)
> /* reset the cc status */
> chip->cc1 = TYPEC_CC_OPEN;
> chip->cc2 = TYPEC_CC_OPEN;
> +
> /* adjust current for SRC */
> - if (pull_up) {
> - ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> - if (ret < 0) {
> - fusb302_log(chip, "cannot set src current %s, ret=%d",
> - typec_cc_status_name[cc], ret);
> - goto done;
> - }
> - }
> - /* enable/disable interrupts, BC_LVL for SNK and COMP_CHNG for SRC */
> - if (pull_up) {
> - rd_mda = rd_mda_value[cc_src_current[cc]];
> - ret = fusb302_i2c_write(chip, FUSB_REG_MEASURE, rd_mda);
> - if (ret < 0) {
> - fusb302_log(chip,
> - "cannot set SRC measure value, ret=%d",
> - ret);
> - goto done;
> - }
> - ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
> - FUSB_REG_MASK_BC_LVL |
> - FUSB_REG_MASK_COMP_CHNG,
> - FUSB_REG_MASK_COMP_CHNG);
> - if (ret < 0) {
> - fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
> - ret);
> - goto done;
> - }
> - chip->intr_bc_lvl = false;
> - chip->intr_comp_chng = true;
> - }
> - if (pull_down) {
> - ret = fusb302_i2c_mask_write(chip, FUSB_REG_MASK,
> - FUSB_REG_MASK_BC_LVL |
> - FUSB_REG_MASK_COMP_CHNG,
> - FUSB_REG_MASK_BC_LVL);
> - if (ret < 0) {
> - fusb302_log(chip, "cannot set SRC interrupt, ret=%d",
> - ret);
> - goto done;
> - }
> - chip->intr_bc_lvl = true;
> - chip->intr_comp_chng = false;
> + ret = fusb302_set_src_current(chip, cc_src_current[cc]);
> + if (ret < 0) {
> + fusb302_log(chip, "cannot set src current %s, ret=%d",
> + typec_cc_status_name[cc], ret);
> + goto done;
> }
> - fusb302_log(chip, "cc := %s", typec_cc_status_name[cc]);
>
> - /* Enable detection for fixed SNK or SRC only roles */
> - switch (cc) {
> - case TYPEC_CC_RD:
> - mode = TOGGLING_MODE_SNK;
> - break;
> - case TYPEC_CC_RP_DEF:
> - case TYPEC_CC_RP_1_5:
> - case TYPEC_CC_RP_3_0:
> - mode = TOGGLING_MODE_SRC;
> - break;
> - default:
> - mode = TOGGLING_MODE_OFF;
> - break;
> - }
> + ret = fusb302_set_toggling(chip, mode);
> + if (ret < 0)
> + fusb302_log(chip, "cannot set toggling mode, ret=%d", ret);
>
> - if (mode != TOGGLING_MODE_OFF) {
> - ret = fusb302_set_toggling(chip, mode);
> - if (ret < 0)
> - fusb302_log(chip,
> - "cannot set fixed role toggling mode,
> ret=%d",
> - ret);
> - }
> done:
> mutex_unlock(&chip->lock);
>
> --
> 2.20.1
>
