| Message ID | 20240710-max33359-toggling-v1-1-f6dc123f3a0a@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: tcpm/tcpci_maxim: fix non-contaminant CC handling | expand |
Hi, On Wed, 2024-07-10 at 07:28 +0100, André Draszik wrote: > tcpci_maxim currently never triggers the TCPM state machine when CC > status has not changed due to a contaminant but due to a real > connection event, i.e. a genuine plug event, meaning the system will > stay idle and not notify any subscribers. > > The reason is that the initial state of the port is 'toggling', which > causes _max_tcpci_irq() to only drive the contamination part of the > TCPM state machine (via tcpm_port_clean()). > > What should happen instead is that if no contamination was detected, > the TCPM should be notified of the CC change in this case. > > To fix this, we update ...is_contaminant() to also allow its caller to > determine if more CC processing is required and then call into the TCPM > as required. > > While at it, add a kernel-doc for max_contaminant_is_contaminant(). > > Note: the code has an issue where I2C errors during contaminant > detection also cause the TCPM state machine to not be updated. This > commit doesn't change this behaviour and should be addressed by > follow-up commit(s). > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/usb/typec/tcpm/maxim_contaminant.c | 7 +++++-- > drivers/usb/typec/tcpm/tcpci_maxim.h | 15 ++++++++++++++- > drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 ++++++++---- > 3 files changed, 27 insertions(+), 7 deletions(-) Any comments on this patch? Cheers, Andre'
On Fri, Jul 26, 2024 at 07:01:13AM +0100, André Draszik wrote: > Hi, > > On Wed, 2024-07-10 at 07:28 +0100, André Draszik wrote: > > tcpci_maxim currently never triggers the TCPM state machine when CC > > status has not changed due to a contaminant but due to a real > > connection event, i.e. a genuine plug event, meaning the system will > > stay idle and not notify any subscribers. > > > > The reason is that the initial state of the port is 'toggling', which > > causes _max_tcpci_irq() to only drive the contamination part of the > > TCPM state machine (via tcpm_port_clean()). > > > > What should happen instead is that if no contamination was detected, > > the TCPM should be notified of the CC change in this case. > > > > To fix this, we update ...is_contaminant() to also allow its caller to > > determine if more CC processing is required and then call into the TCPM > > as required. > > > > While at it, add a kernel-doc for max_contaminant_is_contaminant(). > > > > Note: the code has an issue where I2C errors during contaminant > > detection also cause the TCPM state machine to not be updated. This > > commit doesn't change this behaviour and should be addressed by > > follow-up commit(s). > > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > drivers/usb/typec/tcpm/maxim_contaminant.c | 7 +++++-- > > drivers/usb/typec/tcpm/tcpci_maxim.h | 15 ++++++++++++++- > > drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 ++++++++---- > > 3 files changed, 27 insertions(+), 7 deletions(-) > > Any comments on this patch? It's the middle of the merge window, nothing we can do until -rc1 is out... thanks, greg k-h
Hi André, On Wed, Jul 10, 2024 at 07:28:32AM +0100, André Draszik wrote: > tcpci_maxim currently never triggers the TCPM state machine when CC > status has not changed due to a contaminant but due to a real > connection event, i.e. a genuine plug event, meaning the system will > stay idle and not notify any subscribers. > > The reason is that the initial state of the port is 'toggling', which > causes _max_tcpci_irq() to only drive the contamination part of the > TCPM state machine (via tcpm_port_clean()). > > What should happen instead is that if no contamination was detected, > the TCPM should be notified of the CC change in this case. > > To fix this, we update ...is_contaminant() to also allow its caller to > determine if more CC processing is required and then call into the TCPM > as required. > > While at it, add a kernel-doc for max_contaminant_is_contaminant(). > > Note: the code has an issue where I2C errors during contaminant > detection also cause the TCPM state machine to not be updated. This > commit doesn't change this behaviour and should be addressed by > follow-up commit(s). This looks okay to me, but just in case, let's wait for comments from Badhri, or maybe RD can take a look at this. One nitpick below. > Signed-off-by: André Draszik <andre.draszik@linaro.org> > --- > drivers/usb/typec/tcpm/maxim_contaminant.c | 7 +++++-- > drivers/usb/typec/tcpm/tcpci_maxim.h | 15 ++++++++++++++- > drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 ++++++++---- > 3 files changed, 27 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c > index f8504a90da26..e7fa3e36f8ae 100644 > --- a/drivers/usb/typec/tcpm/maxim_contaminant.c > +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c > @@ -322,11 +322,14 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip) > return 0; > } > > -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce) > +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, > + bool *cc_handled) > { > u8 cc_status, pwr_cntl; > int ret; > > + *cc_handled = true; > + > ret = max_tcpci_read8(chip, TCPC_CC_STATUS, &cc_status); > if (ret < 0) > return false; > @@ -368,7 +371,6 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect > return true; > } > } > - return false; > } else if (chip->contaminant_state == DETECTED) { > if (STATUS_CHECK(cc_status, TCPC_CC_STATUS_TOGGLING, 0)) { > chip->contaminant_state = max_contaminant_detect_contaminant(chip); > @@ -379,6 +381,7 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect > } > } > > + *cc_handled = false; > return false; > } > > diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h > index 78ff3b73ee7e..9c7f714d2c21 100644 > --- a/drivers/usb/typec/tcpm/tcpci_maxim.h > +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h > @@ -85,6 +85,19 @@ static inline int max_tcpci_write8(struct max_tcpci_chip *chip, unsigned int reg > return regmap_raw_write(chip->data.regmap, reg, &val, sizeof(u8)); > } > > -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce); > +/** > + * max_contaminant_is_contaminant - Test if CC was toggled due to contaminant > + * > + * @chip: Handle to a struct max_tcpci_chip > + * @disconnect_while_debounce: Whether or not to sleep as debouncing measure > + * @cc_handled: Returns whether or not CC toggling was handled here > + * > + * Determine if a contaminant was detected. > + * > + * Returns: true if a contaminant was detected, false otherwise. cc_handled > + * is updated to reflect whether or not further CC handling is required. > + */ > +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, > + bool *cc_handled); > > #endif // TCPCI_MAXIM_H_ > diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c > index eec3bcec119c..55d931672ecd 100644 > --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c > +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c > @@ -357,12 +357,15 @@ static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status) > tcpm_vbus_change(chip->port); > > if (status & TCPC_ALERT_CC_STATUS) { > + bool cc_handled = false; /* CC toggle handled by contaminant detection */ > + > if (chip->contaminant_state == DETECTED || tcpm_port_is_toggling(chip->port)) { > - if (!max_contaminant_is_contaminant(chip, false)) > + if (!max_contaminant_is_contaminant(chip, false, > + &cc_handled)) One line is enough for that. > tcpm_port_clean(chip->port); > - } else { > - tcpm_cc_change(chip->port); > } > + if (!cc_handled) > + tcpm_cc_change(chip->port); > } > > if (status & TCPC_ALERT_POWER_STATUS) > @@ -455,8 +458,9 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data) > static void max_tcpci_check_contaminant(struct tcpci *tcpci, struct tcpci_data *tdata) > { > struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata); > + bool cc_handled; > > - if (!max_contaminant_is_contaminant(chip, true)) > + if (!max_contaminant_is_contaminant(chip, true, &cc_handled)) > tcpm_port_clean(chip->port); > } thanks,
On Thu, Aug 1, 2024 at 1:35 AM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi André, > > On Wed, Jul 10, 2024 at 07:28:32AM +0100, André Draszik wrote: > > tcpci_maxim currently never triggers the TCPM state machine when CC > > status has not changed due to a contaminant but due to a real > > connection event, i.e. a genuine plug event, meaning the system will > > stay idle and not notify any subscribers. > > > > The reason is that the initial state of the port is 'toggling', which > > causes _max_tcpci_irq() to only drive the contamination part of the > > TCPM state machine (via tcpm_port_clean()). > > > > What should happen instead is that if no contamination was detected, > > the TCPM should be notified of the CC change in this case. > > > > To fix this, we update ...is_contaminant() to also allow its caller to > > determine if more CC processing is required and then call into the TCPM > > as required. > > > > While at it, add a kernel-doc for max_contaminant_is_contaminant(). > > > > Note: the code has an issue where I2C errors during contaminant > > detection also cause the TCPM state machine to not be updated. This > > commit doesn't change this behaviour and should be addressed by > > follow-up commit(s). > > This looks okay to me, but just in case, let's wait for comments from > Badhri, or maybe RD can take a look at this. One nitpick below. > > > Signed-off-by: André Draszik <andre.draszik@linaro.org> > > --- > > drivers/usb/typec/tcpm/maxim_contaminant.c | 7 +++++-- > > drivers/usb/typec/tcpm/tcpci_maxim.h | 15 ++++++++++++++- > > drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 ++++++++---- > > 3 files changed, 27 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c > > index f8504a90da26..e7fa3e36f8ae 100644 > > --- a/drivers/usb/typec/tcpm/maxim_contaminant.c > > +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c > > @@ -322,11 +322,14 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip) > > return 0; > > } > > > > -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce) > > +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, > > + bool *cc_handled) > > { > > u8 cc_status, pwr_cntl; > > int ret; > > > > + *cc_handled = true; > > + > > ret = max_tcpci_read8(chip, TCPC_CC_STATUS, &cc_status); > > if (ret < 0) > > return false; > > @@ -368,7 +371,6 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect > > return true; > > } > > } > > - return false; > > } else if (chip->contaminant_state == DETECTED) { > > if (STATUS_CHECK(cc_status, TCPC_CC_STATUS_TOGGLING, 0)) { > > chip->contaminant_state = max_contaminant_detect_contaminant(chip); > > @@ -379,6 +381,7 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect > > } > > } > > > > + *cc_handled = false; > > return false; > > } > > > > diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h > > index 78ff3b73ee7e..9c7f714d2c21 100644 > > --- a/drivers/usb/typec/tcpm/tcpci_maxim.h > > +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h > > @@ -85,6 +85,19 @@ static inline int max_tcpci_write8(struct max_tcpci_chip *chip, unsigned int reg > > return regmap_raw_write(chip->data.regmap, reg, &val, sizeof(u8)); > > } > > > > -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce); > > +/** > > + * max_contaminant_is_contaminant - Test if CC was toggled due to contaminant > > + * > > + * @chip: Handle to a struct max_tcpci_chip > > + * @disconnect_while_debounce: Whether or not to sleep as debouncing measure The description should rather be "Whether the disconnect was detected when CC pins were debouncing". > > + * @cc_handled: Returns whether or not CC toggling was handled here Can we name it cc_update_handled and update the description to "Returns whether or not update to CC status was handled here" as it more accurately describes what you are doing here. > > + * > > + * Determine if a contaminant was detected. > > + * > > + * Returns: true if a contaminant was detected, false otherwise. cc_handled > > + * is updated to reflect whether or not further CC handling is required. > > + */ > > +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, > > + bool *cc_handled); > > > > #endif // TCPCI_MAXIM_H_ > > diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c > > index eec3bcec119c..55d931672ecd 100644 > > --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c > > +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c > > @@ -357,12 +357,15 @@ static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status) > > tcpm_vbus_change(chip->port); > > > > if (status & TCPC_ALERT_CC_STATUS) { > > + bool cc_handled = false; /* CC toggle handled by contaminant detection */ > > + > > if (chip->contaminant_state == DETECTED || tcpm_port_is_toggling(chip->port)) { > > - if (!max_contaminant_is_contaminant(chip, false)) > > + if (!max_contaminant_is_contaminant(chip, false, > > + &cc_handled)) > > One line is enough for that. > > > tcpm_port_clean(chip->port); > > - } else { > > - tcpm_cc_change(chip->port); > > } > > + if (!cc_handled) > > + tcpm_cc_change(chip->port); > > } > > > > if (status & TCPC_ALERT_POWER_STATUS) > > @@ -455,8 +458,9 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data) > > static void max_tcpci_check_contaminant(struct tcpci *tcpci, struct tcpci_data *tdata) > > { > > struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata); > > + bool cc_handled; > > > > - if (!max_contaminant_is_contaminant(chip, true)) > > + if (!max_contaminant_is_contaminant(chip, true, &cc_handled)) > > tcpm_port_clean(chip->port); > > } > > thanks, > > -- > heikki Thanks, Badhri
diff --git a/drivers/usb/typec/tcpm/maxim_contaminant.c b/drivers/usb/typec/tcpm/maxim_contaminant.c index f8504a90da26..e7fa3e36f8ae 100644 --- a/drivers/usb/typec/tcpm/maxim_contaminant.c +++ b/drivers/usb/typec/tcpm/maxim_contaminant.c @@ -322,11 +322,14 @@ static int max_contaminant_enable_dry_detection(struct max_tcpci_chip *chip) return 0; } -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce) +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, + bool *cc_handled) { u8 cc_status, pwr_cntl; int ret; + *cc_handled = true; + ret = max_tcpci_read8(chip, TCPC_CC_STATUS, &cc_status); if (ret < 0) return false; @@ -368,7 +371,6 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect return true; } } - return false; } else if (chip->contaminant_state == DETECTED) { if (STATUS_CHECK(cc_status, TCPC_CC_STATUS_TOGGLING, 0)) { chip->contaminant_state = max_contaminant_detect_contaminant(chip); @@ -379,6 +381,7 @@ bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect } } + *cc_handled = false; return false; } diff --git a/drivers/usb/typec/tcpm/tcpci_maxim.h b/drivers/usb/typec/tcpm/tcpci_maxim.h index 78ff3b73ee7e..9c7f714d2c21 100644 --- a/drivers/usb/typec/tcpm/tcpci_maxim.h +++ b/drivers/usb/typec/tcpm/tcpci_maxim.h @@ -85,6 +85,19 @@ static inline int max_tcpci_write8(struct max_tcpci_chip *chip, unsigned int reg return regmap_raw_write(chip->data.regmap, reg, &val, sizeof(u8)); } -bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce); +/** + * max_contaminant_is_contaminant - Test if CC was toggled due to contaminant + * + * @chip: Handle to a struct max_tcpci_chip + * @disconnect_while_debounce: Whether or not to sleep as debouncing measure + * @cc_handled: Returns whether or not CC toggling was handled here + * + * Determine if a contaminant was detected. + * + * Returns: true if a contaminant was detected, false otherwise. cc_handled + * is updated to reflect whether or not further CC handling is required. + */ +bool max_contaminant_is_contaminant(struct max_tcpci_chip *chip, bool disconnect_while_debounce, + bool *cc_handled); #endif // TCPCI_MAXIM_H_ diff --git a/drivers/usb/typec/tcpm/tcpci_maxim_core.c b/drivers/usb/typec/tcpm/tcpci_maxim_core.c index eec3bcec119c..55d931672ecd 100644 --- a/drivers/usb/typec/tcpm/tcpci_maxim_core.c +++ b/drivers/usb/typec/tcpm/tcpci_maxim_core.c @@ -357,12 +357,15 @@ static irqreturn_t _max_tcpci_irq(struct max_tcpci_chip *chip, u16 status) tcpm_vbus_change(chip->port); if (status & TCPC_ALERT_CC_STATUS) { + bool cc_handled = false; /* CC toggle handled by contaminant detection */ + if (chip->contaminant_state == DETECTED || tcpm_port_is_toggling(chip->port)) { - if (!max_contaminant_is_contaminant(chip, false)) + if (!max_contaminant_is_contaminant(chip, false, + &cc_handled)) tcpm_port_clean(chip->port); - } else { - tcpm_cc_change(chip->port); } + if (!cc_handled) + tcpm_cc_change(chip->port); } if (status & TCPC_ALERT_POWER_STATUS) @@ -455,8 +458,9 @@ static int tcpci_init(struct tcpci *tcpci, struct tcpci_data *data) static void max_tcpci_check_contaminant(struct tcpci *tcpci, struct tcpci_data *tdata) { struct max_tcpci_chip *chip = tdata_to_max_tcpci(tdata); + bool cc_handled; - if (!max_contaminant_is_contaminant(chip, true)) + if (!max_contaminant_is_contaminant(chip, true, &cc_handled)) tcpm_port_clean(chip->port); }
tcpci_maxim currently never triggers the TCPM state machine when CC status has not changed due to a contaminant but due to a real connection event, i.e. a genuine plug event, meaning the system will stay idle and not notify any subscribers. The reason is that the initial state of the port is 'toggling', which causes _max_tcpci_irq() to only drive the contamination part of the TCPM state machine (via tcpm_port_clean()). What should happen instead is that if no contamination was detected, the TCPM should be notified of the CC change in this case. To fix this, we update ...is_contaminant() to also allow its caller to determine if more CC processing is required and then call into the TCPM as required. While at it, add a kernel-doc for max_contaminant_is_contaminant(). Note: the code has an issue where I2C errors during contaminant detection also cause the TCPM state machine to not be updated. This commit doesn't change this behaviour and should be addressed by follow-up commit(s). Signed-off-by: André Draszik <andre.draszik@linaro.org> --- drivers/usb/typec/tcpm/maxim_contaminant.c | 7 +++++-- drivers/usb/typec/tcpm/tcpci_maxim.h | 15 ++++++++++++++- drivers/usb/typec/tcpm/tcpci_maxim_core.c | 12 ++++++++---- 3 files changed, 27 insertions(+), 7 deletions(-) --- base-commit: 82d01fe6ee52086035b201cfa1410a3b04384257 change-id: 20240710-max33359-toggling-cf7f7e5e1443 Best regards,