RE: [PATCH v3 07/12] staging: typec: tcpci: register port before request irq

[Jun Li <jun.li@xxxxxxx>]

Hi
> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter
> Roeck
> Sent: 2018年3月15日 12:51
> To: Jun Li <jun.li@xxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx;
> gregkh@xxxxxxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx;
> a.hajda@xxxxxxxxxxx; yueyao@xxxxxxxxxx; shufan_lee@xxxxxxxxxxx;
> o_leveque@xxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: Re: [PATCH v3 07/12] staging: typec: tcpci: register port before
> request irq
> 
> On Tue, Mar 13, 2018 at 05:34:33PM +0800, Li Jun wrote:
> > With that we can clear any pending events and the port is registered
> > so driver can be ready to handle typec events once we request irq.
> >
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxx>
> > Signed-off-by: Li Jun <jun.li@xxxxxxx>
> > ---
> >  drivers/staging/typec/tcpci.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index ba4627f..f5a3bf5 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -600,25 +600,26 @@ static int tcpci_probe(struct i2c_client *client,
> >  	if (IS_ERR(chip->data.regmap))
> >  		return PTR_ERR(chip->data.regmap);
> >
> > +	i2c_set_clientdata(client, chip);
> > +
> >  	/* Disable chip interrupts before requesting irq */
> >  	err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, &val,
> >  			       sizeof(u16));
> >  	if (err < 0)
> >  		return err;
> >
> > +	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> > +	if (PTR_ERR_OR_ZERO(chip->tcpci))
> > +		return PTR_ERR(chip->tcpci);
> > +
> 
> I am concerned that the regitration above might already trigger commands,
> and that we might miss interrupts if that is the case.
> 
> Would it make sense to check for chip->tcpci in the interrupt handler instead ?

Tcpci_irq()
{
	if(!chip->tcpci)
		return IRQ_HANDLED;

	/* Read alert and clear events */
	...
}

With this way, is it possible we repeat enter interrupt handler without chance to
clear it?

Thanks
Jun
> 
> >  	err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> >  					_tcpci_irq,
> >  					IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> >  					dev_name(&client->dev), chip);
> >  	if (err < 0)
> > -		return err;
> > +		tcpci_unregister_port(chip->tcpci);
> >
> > -	chip->tcpci = tcpci_register_port(&client->dev, &chip->data);
> > -	if (PTR_ERR_OR_ZERO(chip->tcpci))
> > -		return PTR_ERR(chip->tcpci);
> > -
> > -	i2c_set_clientdata(client, chip);
> > -	return 0;
> > +	return err;
> >  }
> >
> >  static int tcpci_remove(struct i2c_client *client)
> > --
> > 2.7.4
> >
?韬{.n?????%??檩??w?{.n???{炳???骅w*jg????????G??⒏⒎?:+v????????????＂??????