Hi Nicolas,
On Wed, 20 May 2026 at 14:36, Nicolas Frattaroli
<[email protected]> wrote:
> - for (i = 0; i < ARRAY_SIZE(buf) - 1; i += 2) {
> + for (i = 0; i <= ERR_DET_OFF(SCDC_ERR_DET_2_H); i += 2) {
> if (buf[i + 1] & SCDC_CHANNEL_VALID)
> counter[i / 2] = buf[i] | (buf[i + 1] &
> ~SCDC_CHANNEL_VALID) << 8;
> else
> @@ -355,9 +433,15 @@ int drm_scdc_read_error_counters(struct drm_connector
> *connector, u16 counter[3]
> buf[i] = 0;
> buf[i + 1] = 0;
> }
> - buf[ARRAY_SIZE(buf) - 1] = 0;
> + buf[ERR_DET_OFF(SCDC_ERR_DET_CHECKSUM)] = 0;
> +
> + if (num_lanes == 4)
> + counter[3] = buf[ERR_DET_OFF(SCDC_ERR_DET_3_L)] |
> + (buf[ERR_DET_OFF(SCDC_ERR_DET_3_H)] &
> ~SCDC_CHANNEL_VALID) << 8;
> + else
> + counter[3] = 0;
I get that this is separate from the loop above because it's
discontiguous, but is it also missing the validity check? I mean,
lane_count == 3 means 'fsvo 3 which may be 1 or 2' so have to check
SCDC_CHANNEL_VALID there, but if we have 4 lanes explicitly specified
then we are we missing the check for the valid bit, or is it just
always valid / we shouldn't check?
tbh having it separately is a bit messy. I half-wonder if unrolling
the loop wouldn't be cleaner, e.g.:
#define GET_SCDC_ERR_CNT(c) { \
bool valid = (buf[(c) * 2 + 1] & SCDC_CHANNEL_VALID; \
if (valid) { \
counter[c] = buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _L)]; \
counter[c] |= (buf[ERR_DET_OFF(SCDC_ERR_DET_ ##x _H)]
& ~SCDC_CHANNEL_VALID) << 8; \
} else {
counter[c] = 0;
} \
}
GET_SCDC_ERR_CNT(0);
GET_SCDC_ERR_CNT(1);
GET_SCDC_ERR_CNT(2);
if (num_lanes == 4)
GET_SCDC_ERR_CNT(3);
else
counter[3] = 0;
Up to you though. It's a messy format, with no objectively great solution.
With or without that suggestion, series is:
Reviewed-by: Daniel Stone <[email protected]>
Thanks for pulling all this together!
Cheers,
Daniel
