| Message ID | 20190307163607.24016-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: fusb302: Various fixes | expand |
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 <hdegoede@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 <linux@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 >
On Thu, Mar 07, 2019 at 10:12:59AM -0800, Guenter Roeck wrote: > 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 <hdegoede@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 <linux@roeck-us.net> I'll test this series with my GPD win board today. thanks,
Hi, On 08-03-19 08:33, Heikki Krogerus wrote: > On Thu, Mar 07, 2019 at 10:12:59AM -0800, Guenter Roeck wrote: >> 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 <hdegoede@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 <linux@roeck-us.net> > > I'll test this series with my GPD win board today. Note I've already tested this series on both a GPD win and a GPD pocket. Also Guenter's reviewed has shown that a small error has crept up in patch 5, patch 5 adds 2: usleep_range(5000, 10000); lines which should be: usleep_range(50, 100); So if you test please make that change, or wait for v3 of the series. Regards, Hans
On Fri, Mar 08, 2019 at 11:58:32AM +0100, Hans de Goede wrote: > Hi, > > On 08-03-19 08:33, Heikki Krogerus wrote: > > On Thu, Mar 07, 2019 at 10:12:59AM -0800, Guenter Roeck wrote: > > > 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 <hdegoede@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 <linux@roeck-us.net> > > > > I'll test this series with my GPD win board today. > > Note I've already tested this series on both a GPD win and a GPD pocket. > > Also Guenter's reviewed has shown that a small error has crept up > in patch 5, patch 5 adds 2: > > usleep_range(5000, 10000); > > lines which should be: > > usleep_range(50, 100); > > So if you test please make that change, or wait for v3 of the series. OK. thanks,
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);
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 <hdegoede@redhat.com> --- drivers/usb/typec/tcpm/fusb302.c | 85 ++++++-------------------------- 1 file changed, 15 insertions(+), 70 deletions(-)