Re: [PATCH v13 3/3] Input: Add TouchNetix axiom i2c touchscreen driver

[Dmitry Torokhov]

Hi Gregory,

On Fri, Jun 07, 2024 at 09:23:36AM +0200, Gregory CLEMENT wrote:
> Hello Dmitry,
> 
> > On Wed, Jun 05, 2024 at 03:48:20PM +0200, Kamel BOUHARA wrote:
> >> [...]
> >> 
> >> > > > +
> >> > > > +	error = devm_request_threaded_irq(dev, client->irq, NULL,
> >> > > > +					  axiom_irq, IRQF_ONESHOT, dev_name(dev), ts);
> >> > > > +	if (error) {
> >> > > > +		dev_info(dev, "Request irq failed, falling back to polling mode");
> >> > > 
> >> > > I do not think you should fall back to polling mode if you fail to get
> >> > > interrupt. If it was not specified (client->irq) then I can see that
> >> > > we
> >> > > might want to fall back, but if the system configured for using
> >> > > interrupt and you can not get it you should bail out.
> >> > > 
> >> > 
> >> > Yes, clear, the polling mode can be decorrelated to the irq not provided
> >> > case.
> >> 
> >> Just to make sure I understood, is this what you propose ?
> >> 
> >> if (client->irq) {
> >>         error = devm_request_threaded_irq(...)
> >>         if (error) {
> >> 		dev_warn(dev, "failed to request IRQ\n");
> >> 		client->irq = 0;
> >>          }
> >> }
> >> 
> >> if(!client->irq) {
> >>     // setup polling stuff
> >>     ...
> >> }
> >
> > No, if you fail to acquire interrupt that was described in ACPI/DT it
> > should be treated as an error, like this:
> >
> > 	if (client->irq) {
> > 		error = devm_request_threaded_irq(...)
> > 		if (error)
> > 			return dev_err_probe(...);
> > 	} else {
> > 		.. set up polling ..
> > 	}
> >
> > This also makes sure that if interrupt controller is not ready and
> > requests probe deferral we will not end up with device in polling
> > mode.
> 
> In the case of probe deferral, I see the benefit of treating it as an
> error. However, in the other case, I find it better to fall back to
> polling mode with a big error message than just exiting in error. As a
> user, I think we prefer having a degraded feature over not having the
> feature at all.

No, this is not how the drivers work, we do not simply ignore errors and
hope for the best. If resources are described in platform definition (be
it ACPI or device tree) they need to be there and they need to work. It
is true for regulators and reset gpios (you do not ignore errors if you
fail to obtain a them in the hope that the device is operable), and you
should not ignore errors when trying to set up interrupt.

Thanks.

-- 
Dmitry