Re: [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 30 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> How does this fit in with Thierry's dp aux helper improvements? I prefer
> if we start to converge on that sooner than later ...

Patch 1/4 actually fixes a potential bug in intel_dp_sink_crc(), which
now detects failure only if the read returns 0, but plunges on with any
negative error values. So we may want to queue that for fixes.

Patch 2/4 could be dropped.

I'd like to do 3/4 as a prep patch anyway, as it potentially changes
behaviour, and makes conversion to the helpers more straightforward.

Patch 4/4 could be dropped.

To see how this would pan out, I'm almost done with the native aux
conversion (did not look at the i2c-over-aux part yet).

If we use those interfaces directly all over the place, the only
annoyance is the potential for the same confusion that I've tried to
avoid in this series.

The only correct check for success is comparing the transfer function
return value to the number of bytes that was to be transferred:

	if (drm_dp_dpcd_read(&intel_dp->dp_aux, offset, buffer, len) != len)

but len may in fact be a fairly long #define like
DP_DEVICE_SERVICE_IRQ_VECTOR.

I do notice Thierry checks for ret < 0 in his code, although, IIUC, it's
possible the transfer ends with fewer bytes transferred than requested.

I'm beginning to feel like the functions should return and guarantee an
error code < 0 if not all bytes were transferred, just for the ease of
use. I mean, it's a nice feature to be able to make the distinction, but
I can't fathom a practical situation where that would be necessary.


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center