| Message ID | 20221120141509.81012-1-marex@denx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 581c848b610dbf3fe1ed4d85fd53d0743c61faba |
| Headers | show |
| Series | extcon: usbc-tusb320: Update state on probe even if no IRQ pending | expand |
Hi Marek, On Sun, Nov 20, 2022 at 03:15:09PM +0100, Marek Vasut wrote: > Currently this driver triggers extcon and typec state update in its > probe function, to read out current state reported by the chip and > report the correct state to upper layers. This synchronization is > performed correctly, but only in case the chip indicates a pending > interrupt in reg09 register. > > This fails to cover the situation where all interrupts reported by > the chip were already handled by Linux before reboot, then the system > rebooted, and then Linux starts again. In this case, the TUSB320 no > longer reports any interrupts in reg09, and the state update does not > perform any update as it depends on that interrupt indication. > > Fix this by turning tusb320_irq_handler() into a thin wrapper around > tusb320_state_update_handler(), where the later now contains the bulk > of the code of tusb320_irq_handler(), but adds new function parameter > "force_update". The "force_update" parameter can be used by the probe > function to assure that the state synchronization is always performed, > independent of the interrupt indicated in reg09. The interrupt handler > tusb320_irq_handler() callback uses force_update=false to avoid state > updates on potential spurious interrupts and retain current behavior. > > Fixes: 06bc4ca115cdd ("extcon: Add driver for TI TUSB320") > Signed-off-by: Marek Vasut <marex@denx.de> Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> snip > @@ -466,7 +473,7 @@ static int tusb320_probe(struct i2c_client *client, > return ret; > > /* update initial state */ > - tusb320_irq_handler(client->irq, priv); > + tusb320_state_update_handler(priv, true); I wonder, is this function call even necessary? > > /* Reset chip to its default state */ > ret = tusb320_reset(priv); > @@ -477,7 +484,7 @@ static int tusb320_probe(struct i2c_client *client, > * State and polarity might change after a reset, so update > * them again and make sure the interrupt status bit is cleared. > */ > - tusb320_irq_handler(client->irq, priv); > + tusb320_state_update_handler(priv, true); > > ret = devm_request_threaded_irq(priv->dev, client->irq, NULL, > tusb320_irq_handler, > -- > 2.35.1 >
On 11/21/22 00:52, Alvin Šipraga wrote: > Hi Marek, Hi, > On Sun, Nov 20, 2022 at 03:15:09PM +0100, Marek Vasut wrote: >> Currently this driver triggers extcon and typec state update in its >> probe function, to read out current state reported by the chip and >> report the correct state to upper layers. This synchronization is >> performed correctly, but only in case the chip indicates a pending >> interrupt in reg09 register. >> >> This fails to cover the situation where all interrupts reported by >> the chip were already handled by Linux before reboot, then the system >> rebooted, and then Linux starts again. In this case, the TUSB320 no >> longer reports any interrupts in reg09, and the state update does not >> perform any update as it depends on that interrupt indication. >> >> Fix this by turning tusb320_irq_handler() into a thin wrapper around >> tusb320_state_update_handler(), where the later now contains the bulk >> of the code of tusb320_irq_handler(), but adds new function parameter >> "force_update". The "force_update" parameter can be used by the probe >> function to assure that the state synchronization is always performed, >> independent of the interrupt indicated in reg09. The interrupt handler >> tusb320_irq_handler() callback uses force_update=false to avoid state >> updates on potential spurious interrupts and retain current behavior. >> >> Fixes: 06bc4ca115cdd ("extcon: Add driver for TI TUSB320") >> Signed-off-by: Marek Vasut <marex@denx.de> > > Reviewed-by: Alvin Šipraga <alsi@bang-olufsen.dk> > > snip > >> @@ -466,7 +473,7 @@ static int tusb320_probe(struct i2c_client *client, >> return ret; >> >> /* update initial state */ >> - tusb320_irq_handler(client->irq, priv); >> + tusb320_state_update_handler(priv, true); > > I wonder, is this function call even necessary? Yes, it is, even though this is not immediately obvious why. The tusb320_reset() rcalled right after the aforementioned call triggers priv->ops->set_mode(), which is implemented e.g. by tusb320_set_mode(), which contains this conditional: if (priv->state != TUSB320_ATTACHED_STATE_NONE) and the priv->state is assigned in tusb320_state_update_handler(), or rather in function it calls, the tusb320_extcon_irq_handler().
On Sun, Nov 20, 2022 at 03:15:09PM +0100, Marek Vasut wrote: > Currently this driver triggers extcon and typec state update in its > probe function, to read out current state reported by the chip and > report the correct state to upper layers. This synchronization is > performed correctly, but only in case the chip indicates a pending > interrupt in reg09 register. > > This fails to cover the situation where all interrupts reported by > the chip were already handled by Linux before reboot, then the system > rebooted, and then Linux starts again. In this case, the TUSB320 no > longer reports any interrupts in reg09, and the state update does not > perform any update as it depends on that interrupt indication. > > Fix this by turning tusb320_irq_handler() into a thin wrapper around > tusb320_state_update_handler(), where the later now contains the bulk > of the code of tusb320_irq_handler(), but adds new function parameter > "force_update". The "force_update" parameter can be used by the probe > function to assure that the state synchronization is always performed, > independent of the interrupt indicated in reg09. The interrupt handler > tusb320_irq_handler() callback uses force_update=false to avoid state > updates on potential spurious interrupts and retain current behavior. > > Fixes: 06bc4ca115cdd ("extcon: Add driver for TI TUSB320") > Signed-off-by: Marek Vasut <marex@denx.de> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > Cc: Alvin Šipraga <alsi@bang-olufsen.dk> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: MyungJoo Ham <myungjoo.ham@samsung.com> > Cc: Yassine Oudjana <y.oudjana@protonmail.com> > --- > drivers/extcon/extcon-usbc-tusb320.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c > index 2a120d8d3c272..9dfa545427ca1 100644 > --- a/drivers/extcon/extcon-usbc-tusb320.c > +++ b/drivers/extcon/extcon-usbc-tusb320.c > @@ -313,9 +313,9 @@ static void tusb320_typec_irq_handler(struct tusb320_priv *priv, u8 reg9) > typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); > } > > -static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > +static irqreturn_t tusb320_state_update_handler(struct tusb320_priv *priv, > + bool force_update) > { > - struct tusb320_priv *priv = dev_id; > unsigned int reg; > > if (regmap_read(priv->regmap, TUSB320_REG9, ®)) { > @@ -323,7 +323,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > return IRQ_NONE; > } > > - if (!(reg & TUSB320_REG9_INTERRUPT_STATUS)) > + if (!force_update && !(reg & TUSB320_REG9_INTERRUPT_STATUS)) > return IRQ_NONE; > > tusb320_extcon_irq_handler(priv, reg); > @@ -340,6 +340,13 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > return IRQ_HANDLED; > } > > +static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) > +{ > + struct tusb320_priv *priv = dev_id; > + > + return tusb320_state_update_handler(priv, false); > +} > + > static const struct regmap_config tusb320_regmap_config = { > .reg_bits = 8, > .val_bits = 8, > @@ -466,7 +473,7 @@ static int tusb320_probe(struct i2c_client *client, > return ret; > > /* update initial state */ > - tusb320_irq_handler(client->irq, priv); > + tusb320_state_update_handler(priv, true); > > /* Reset chip to its default state */ > ret = tusb320_reset(priv); > @@ -477,7 +484,7 @@ static int tusb320_probe(struct i2c_client *client, > * State and polarity might change after a reset, so update > * them again and make sure the interrupt status bit is cleared. > */ > - tusb320_irq_handler(client->irq, priv); > + tusb320_state_update_handler(priv, true); > > ret = devm_request_threaded_irq(priv->dev, client->irq, NULL, > tusb320_irq_handler, thanks,
diff --git a/drivers/extcon/extcon-usbc-tusb320.c b/drivers/extcon/extcon-usbc-tusb320.c index 2a120d8d3c272..9dfa545427ca1 100644 --- a/drivers/extcon/extcon-usbc-tusb320.c +++ b/drivers/extcon/extcon-usbc-tusb320.c @@ -313,9 +313,9 @@ static void tusb320_typec_irq_handler(struct tusb320_priv *priv, u8 reg9) typec_set_pwr_opmode(port, TYPEC_PWR_MODE_USB); } -static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) +static irqreturn_t tusb320_state_update_handler(struct tusb320_priv *priv, + bool force_update) { - struct tusb320_priv *priv = dev_id; unsigned int reg; if (regmap_read(priv->regmap, TUSB320_REG9, ®)) { @@ -323,7 +323,7 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) return IRQ_NONE; } - if (!(reg & TUSB320_REG9_INTERRUPT_STATUS)) + if (!force_update && !(reg & TUSB320_REG9_INTERRUPT_STATUS)) return IRQ_NONE; tusb320_extcon_irq_handler(priv, reg); @@ -340,6 +340,13 @@ static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) return IRQ_HANDLED; } +static irqreturn_t tusb320_irq_handler(int irq, void *dev_id) +{ + struct tusb320_priv *priv = dev_id; + + return tusb320_state_update_handler(priv, false); +} + static const struct regmap_config tusb320_regmap_config = { .reg_bits = 8, .val_bits = 8, @@ -466,7 +473,7 @@ static int tusb320_probe(struct i2c_client *client, return ret; /* update initial state */ - tusb320_irq_handler(client->irq, priv); + tusb320_state_update_handler(priv, true); /* Reset chip to its default state */ ret = tusb320_reset(priv); @@ -477,7 +484,7 @@ static int tusb320_probe(struct i2c_client *client, * State and polarity might change after a reset, so update * them again and make sure the interrupt status bit is cleared. */ - tusb320_irq_handler(client->irq, priv); + tusb320_state_update_handler(priv, true); ret = devm_request_threaded_irq(priv->dev, client->irq, NULL, tusb320_irq_handler,
Currently this driver triggers extcon and typec state update in its probe function, to read out current state reported by the chip and report the correct state to upper layers. This synchronization is performed correctly, but only in case the chip indicates a pending interrupt in reg09 register. This fails to cover the situation where all interrupts reported by the chip were already handled by Linux before reboot, then the system rebooted, and then Linux starts again. In this case, the TUSB320 no longer reports any interrupts in reg09, and the state update does not perform any update as it depends on that interrupt indication. Fix this by turning tusb320_irq_handler() into a thin wrapper around tusb320_state_update_handler(), where the later now contains the bulk of the code of tusb320_irq_handler(), but adds new function parameter "force_update". The "force_update" parameter can be used by the probe function to assure that the state synchronization is always performed, independent of the interrupt indicated in reg09. The interrupt handler tusb320_irq_handler() callback uses force_update=false to avoid state updates on potential spurious interrupts and retain current behavior. Fixes: 06bc4ca115cdd ("extcon: Add driver for TI TUSB320") Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Alvin Šipraga <alsi@bang-olufsen.dk> Cc: Chanwoo Choi <cw00.choi@samsung.com> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Cc: MyungJoo Ham <myungjoo.ham@samsung.com> Cc: Yassine Oudjana <y.oudjana@protonmail.com> --- drivers/extcon/extcon-usbc-tusb320.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)