Message-ID: <Y0i9yEiZn0kjYCU6@nixie71>
Date:   Thu, 13 Oct 2022 20:39:20 -0500
From:   Jeff LaBundy <jeff@...undy.com>
To:     Job Noorman <job@...rman.info>
Cc:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Henrik Rydberg <rydberg@...math.org>,
        Luca Weiss <luca@...tu.xyz>, linux-kernel@...r.kernel.org,
        linux-input@...r.kernel.org
Subject: Re: [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen
 devices

Hi Job,

On Thu, Oct 13, 2022 at 02:57:55PM +0000, Job Noorman wrote:
> Hi Jeff,
> 
> Thanks for the extensive review!
> 
> I've addressed most of your comments. I will reply below to the ones I
> have not yet addressed, or have questions about. You can consider the
> comments I don't reply to as addressed in the next version.
> 
> On Thu Oct 13, 2022 at 4:34 AM CEST, Jeff LaBundy wrote:
> > That being said, use of dev_err_probe() is generally frowned upon in
> > the input subsystem and I tend to agree. The argument against it is
> > that resources that may not be ready in time should be responsible for
> > the housekeeping done in dev_err_probe() rather than every possible
> > consumer doing so through every possible error path.
> >
> > I only mention this because you will likely be asked to change even
> > the "valid" calls to dev_err_probe().
> 
> I have removed all uses of dev_err_probe(). I followed the approach I
> saw in other drivers of only calling dev_err() when the error is not
> -EPROBE_DEFER, and calling it unconditionally when this is not a
> possible error. However, while I _think_ I managed to figure out for
> which functions this can never be returned (based mostly on the Goodix
> driver), I wouldn't mind if somebody could double-check this :)
> 
> > > +static bool himax_event_point_is_valid(const struct himax_event_point *point)
> > > +{
> > > +	return himax_event_point_get_x(point) != 0xffff &&
> > > +	       himax_event_point_get_y(point) != 0xffff;
> > > +}
> >
> > How about U16_MAX?
> 
> This feels strange to me because conceptually, I don't want to know if
> these values are equal to the maximum u16 value, I want to know if they
> are invalid coordinates. It's incidental that invalid values correspond
> to U16_MAX.
> 
> I created a #define HIMAX_INVALID_COORD for this. Would you agree with
> this approach?

That sounds fine too.

> 
> > > +static irqreturn_t himax_irq_handler(int irq, void *dev_id)
> > > +{
> > > +	struct himax_ts_data *ts = dev_id;
> > > +
> > > +	himax_handle_input(ts);
> >
> > Is it accurate to assume that the act of reading the event status
> > register(s) is what acknowledges the interrupt and de-asserts the
> > interrupt pin?
> 
> I assume so but without datasheets this is impossible to know for sure
> unfortunately. However, since this is the only interaction with the
> device during an IRQ, I cannot imagine what else could cause the
> de-assert.
> 
> > If so, I think it is safer to define himax_handle_input() with an
> > integer return type, then return IRQ_NONE upon failure. If the I2C
> > adapter goes south such that reads are never initiated and the pin
> > is stuck low causing an interrupt storm, the handler would get cut
> > off quickly.
> 
> Done! Two questions:
> - Do you think it's necessary to return IRQ_NONE on a checksum failure?

I personally do not think so. In this case the problem is not that the
interrupt went unhandled, but rather the driver handled the interrupt
but decided not to trust the data and post an input event.

> - If I understand correctly, this means that once an error occurs, the
>   driver becomes unusable. Would it make sense to try to reset the
>   device after an error?

That's up to you; it's not terribly uncommon to wrap low-level register
access in some kind of retry loop if there is reason to believe the
device may be unresponsive for some reason. One could issue a hardware
reset after some number of failed attempts.

Personally I think that is a bit overkill for an initial submission if
the device operates perfectly fine on your platform.

> 
> > Just for my own understanding, _when_ does the pin get de-asserted?
> > Is it early in the I2C read, or after the stop condition? In case
> > of the latter, consider a delay to prevent the interrupt from being
> > immediately triggered once more after the handler has returned, but
> > the pin hasn't quite returned to a logic-high level.
> 
> I don't know the answer to this question unfortunately. The downstream
> driver doesn't seem to have any delay after reading the registers
> though.
> 
> > > +	error = himax_setup_gpio(ts);
> > > +	if (error)
> > > +		return error;
> > > +
> > > +	himax_reset(ts);
> >
> > It looks like we're expecting an I2C read to occur directly after reset
> > is de-asserted. Understanding that no datasheet is available to specify
> > how much time the device takes to exit reset, does the downstream driver
> > at least include some delay?
> 
> The downstream driver has a 20ms delay between asserting and
> de-asserting, but not _after_ de-asserting. I have copied this behavior.
> 
> Here's the funny thing: the I2C read you're talking about (to read the
> product id) happens _before_ the reset in the downstream driver and that
> also seems to work...

Interesting; I'm guessing that's because the reset GPIO may be pulled up
such that the device has been out of reset for quite some time before the
vendor driver reads the product ID.

> 
> So I'm not sure what to do here. It does feel safer to add an additional
> delay so shall we just do that?

Since reset is asserted for a whole 20 ms, it seems relatively harmless
to insert something like usleep_range(1000, 1100) after reset is released.
It adds some margin and doesn't noticeably increase the probe time.

At the very least, it would be very obvious where in the code one would
increase the delay if required in the future. Consider adding a comment
that the delay is a guess to prevent future developers from believing it
came from a datasheet.

> 
> Kind regards,
> Job
> 

Kind regards,
Jeff LaBundy

Powered by blists - more mailing lists