Re: [PATCH v9 11/39] drm: helper functions for hdcp2 seq_num to from u32

From: Daniel Vetter <daniel@ffwll.ch>
To: Ramalingam C <ramalingam.c@intel.com>
Cc: daniel.vetter@ffwll.ch, intel-gfx@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, uma.shankar@intel.com,
	seanpaul@chromium.org, tomas.winkler@intel.com
Subject: Re: [PATCH v9 11/39] drm: helper functions for hdcp2 seq_num to from u32
Date: Wed, 19 Dec 2018 15:38:50 +0100	[thread overview]
Message-ID: <20181219143850.GG21184@phenom.ffwll.local> (raw)
In-Reply-To: <1544673701-6353-12-git-send-email-ramalingam.c@intel.com>

On Thu, Dec 13, 2018 at 09:31:13AM +0530, Ramalingam C wrote:
> Library functions for endianness are aligned for 16/32/64 bits.
> But hdcp sequence numbers are 24bits(big endian).
> So for their conversion to and from u32 helper functions are developed.
> 
> Signed-off-by: Ramalingam C <ramalingam.c@intel.com>
> ---
>  include/drm/drm_hdcp.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h
> index a6de09c5e47f..d8093ecf3ddc 100644
> --- a/include/drm/drm_hdcp.h
> +++ b/include/drm/drm_hdcp.h
> @@ -250,4 +250,22 @@ struct hdcp2_dp_errata_stream_type {
>  #define HDCP_2_2_HDMI_RXSTATUS_READY(x)		((x) & BIT(2))
>  #define HDCP_2_2_HDMI_RXSTATUS_REAUTH_REQ(x)	((x) & BIT(3))
>  
> +/*
> + * Library functions for endianness are aligned for 16/32/64 bits.
> + * But hdcp sequence numbers are 24bits(big endian). So for their conversion
> + * from and to u32 below functions are developed.

Comment is a bit confusing, I'd do a simple:

/*
 * Helper functions to convert 24bit big endian hdcp sequence number to
 * host format and back
 */

Explaining why we need them exactly isn't terribly interesting.

> + */
> +static inline void
> +drm_hdcp2_seq_num_to_u32(u32 *val, u8 seq_num[HDCP_2_2_SEQ_NUM_LEN])

make u32 val the return value of this function, should be cleaner.

> +{
> +	*val = seq_num[2] | seq_num[1] << 8 | seq_num[0] << 16;
> +}
> +
> +static inline void drm_hdcp2_u32_to_seq_num(u8 *seq_num, u32 val)

u8 seq_num[HDCP_2_2_SEQ_NUM_LEN]. Not any different in the actual function
type, just a bit more informative.

> +{
> +	seq_num[0] = (val & (0xff << 16)) >> 16;
> +	seq_num[1] = (val & (0xff << 8)) >> 8;

Shift alone should be enough, since seq_num[x] is of type u8 the & 0xff is
implied.

With the above comments addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +	seq_num[2] = val & 0xff;
> +}
> +
>  #endif
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel