On Mon, Dec 16, 2024 at 07:55:40PM +0100, Francesco Dolcini wrote:
> On Thu, Dec 12, 2024 at 08:24:09PM +0800, Xu Yang wrote:
> > With edge irq support, the ALERT event may be missed currently. The reason
> > is that ALERT_MASK register is written before devm_request_threaded_irq().
> > If ALERT event happens in this time gap, it will be missed and ALERT line
> > will not recover to high level. However, we can't meet this issue with
> > level irq. To avoid the issue, this will add a flag set_alert_mask. So
> > ALERT_MASK can be written after devm_request_threaded_irq() is called. The
> > behavior of tcpm_init() keeps unchanged.
> >
> > Fixes: 77e85107a771 ("usb: typec: tcpci: support edge irq")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
>
> I wonder if this should be squashed together with the first commit,
> given you re-introduce an issue with the previous commit.
No. One patch normally should do one thing. To support edge irq, commit
77e85107a771 cause NULL ponter issue so path 1 fix it, it also didn't
handle irq or alert_mask correctly, then patch 2 is needed.
>
>
> >
> > ---
> > Changes in v2:
> > - new patch
> > ---
> > drivers/usb/typec/tcpm/tcpci.c | 22 +++++++++++++++++-----
> > 1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpci.c b/drivers/usb/typec/tcpm/tcpci.c
> > index db42f4bf3632..836f6d54d267 100644
> > --- a/drivers/usb/typec/tcpm/tcpci.c
> > +++ b/drivers/usb/typec/tcpm/tcpci.c
> > @@ -37,6 +37,7 @@ struct tcpci {
> > unsigned int alert_mask;
> >
> > bool controls_vbus;
> > + bool set_alert_mask;
> >
> > struct tcpc_dev tcpc;
> > struct tcpci_data *data;
> > @@ -700,7 +701,10 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> >
> > tcpci->alert_mask = reg;
> >
> > - return tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > + if (tcpci->set_alert_mask)
> > + ret = tcpci_write16(tcpci, TCPC_ALERT_MASK, reg);
> > +
> > + return ret;
> > }
> >
> > irqreturn_t tcpci_irq(struct tcpci *tcpci)
> > @@ -931,12 +935,20 @@ static int tcpci_probe(struct i2c_client *client)
> > _tcpci_irq,
> > IRQF_SHARED | IRQF_ONESHOT,
> > dev_name(&client->dev), chip);
> > - if (err < 0) {
> > - tcpci_unregister_port(chip->tcpci);
> > - return err;
> > - }
> > + if (err < 0)
> > + goto unregister_port;
> >
> > + /* Enable the interrupt on chip at last */
> > + err = tcpci_write16(chip->tcpci, TCPC_ALERT_MASK, chip->tcpci->alert_mask);
> what's the content of alert_mask here?
tcpci_register_port()
tcpm_register_port()
tcpm_init()
tcpci_init()
tcpci->alert_mask = reg;
After tcpci_register_port() completed, alert_mask is assigned a specific value.
For example:
reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
>
> I am not fully understanding why this flag is needed, can't we just ensure
> that within probe the alert mask is set to 0, after probe return with
> success we have the interrupt handler enabled and we can just write the
> alert mask unconditionally.
I wrongly thought ALERT_MASK is firstly reset and set again in tcpci_init().
Actually it's not reset, so the flag is not needed. I'll change it later.
Thanks,
Xu Yang
