Re: [PATCH v2 2/3] Input: add driver for Himax hx83112b touchscreen devices

[Job Noorman]

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?

> > +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?
- 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?

> 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...

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

Kind regards,
Job