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
