Re: [PATCH] Input: touchscreen - Add new Novatek NVT-ts driver

[Jeff LaBundy <jeff@labundy.com>]

Hi Hans,

[...]

> >> +	if (width > NVT_TS_MAX_SIZE || height >= NVT_TS_MAX_SIZE ||
> >> +	    data->max_touches > NVT_TS_MAX_TOUCHES ||
> >> +	    irq_type >= ARRAY_SIZE(nvt_ts_irq_type) ||
> >> +	    data->buf[NVT_TS_PARAMS_WAKE_TYPE] != NVT_TS_SUPPORTED_WAKE_TYPE ||
> >> +	    data->buf[NVT_TS_PARAMS_CHIP_ID] != NVT_TS_SUPPORTED_CHIP_ID) {
> >> +		dev_err(dev, "Unsupported touchscreen parameters: %*ph\n",
> >> +			NVT_TS_PARAMS_SIZE, data->buf);
> >> +		return -EIO;
> > 
> > Nit: because there was no I/O error here necessarily, but rather invalid or
> > unacceptable values, I think -EINVAL is better here.
> 
> AFAIK -EINVAL is reserved for invalid function parameters, typically
> on a syscall / ioctl. Where as here we are receiving invalid data,
> which is more like a a checksum/crc error which is an IO error.
> 
> With that said I have no strong preference either way, so let me know
> if you want me to switch to EINVAL for v2 or not.

Based on this additional information, I agree that -EIO is a better choice. In
theory, only an I/O error could make the device return nonsensical values. The
controller FW is unlikely to store values it cannot support.

[...]

> >> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, nvt_ts_irq,
> >> +					IRQF_ONESHOT | IRQF_NO_AUTOEN | nvt_ts_irq_type[irq_type],
> >> +					client->name, data);
> > 
> > Interesting, it seems interrupt polarity is configurable?
> 
> On the controller-side, yes. The goodix touchscreens have much the same
> thing.
> 
> > For my own
> > understanding, what if there is an inverter on the board?
> 
> Then things break I guess since we program the GPIO input IRQs polarity
> to match the controller polarity when then will be wrong.
> 
> Luckily this has never happened so far AFAIK (mostly talking goodix
> here, since I know only 1 device with this new touchscreen).

ACK.

> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "requesting irq\n");
> > 
> > dev_err_probe() tends not to be accepted in input, the argument being
> > that the callers who can return EPROBE_DEFER be responsible for setting
> > the reason as opposed to every driver calling a separate function that
> > does so.
> 
> To me dev_err_probe() is not so much about setting the probe-defer
> reason, it is is very useful because:
> 
> 1) It deals with not logging afalse-postivive error msg on -EPROBE_DEFER and
> you can return its return value, leading to much more compact code and
> thus IMOH more readable code
> 
> 2) It leads to a consistent format for the printed errors
> 
> To illustrate 1. without dev_err_probe() the reset_gpio request error
> handling turns from this:
> 
> if (IS_ERR(data->reset_gpio))
> 	return dev_err_probe(dev, PTR_ERR(data->reset_gpio), "requesting reset GPIO\n");
> 
> into:
> 
> if (IS_ERR(data->reset_gpio)) {
> 	error = PTR_ERR(data->reset_gpio);
> 	if (error == -EPROBE_DEFER)
> 		return error;
> 	dev_err(dev, "Error %d requesting reset GPIO\n", error);
> 	return error;
> }
> 
> Which is 7 lines vs 2 lines when using dev_err_probe() and more
> importantly IMHO the error handling code using using dev_err_probe()
> is just much more readable and thus more maintainable IMHO.
> 
> Which is why IMHO using dev_err_probe() for errors getting resources
> is just much better.
> 
> So unless you feel really strongly about not using this I would
> prefer to keep using dev_err_probe().

I do not personally feel strongly about this and I think your reasoning is
sound. A quick grep through drivers/ shows it is immensely popular. However
the same grep through drivers/input shows it has yet to be accepted there.

That is the only reason I mention it; I however have no issue with it being
left as-is for v2.

> Once more thank you for the review. If you can clarify what
> you want for the EINVAL vs EIO and for the dev_err_probe()
> review remarks then I'll prepare a v2.

Thank you for the productive discussion as always :)

> 
> Regards,
> 
> Hans
> 

Kind regards,
Jeff LaBundy