On Thu, Aug 1, 2024 at 1:35 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
