On Thu, 2019-06-13 at 15:05 -0700, Hyun Kwon wrote:

On Tue, 2019-06-04 at 06:55:56 -0700, Vishal Sagar wrote:

trivia:

quoted

diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c b/drivers/media/platform/xilinx/xilinx-sdirxss.c

[]

quoted

+static int xsdirx_get_stream_properties(struct xsdirxss_state *state)
+{

[]

quoted

+	if (valid & XSDIRX_ST352_VALID_DS1_MASK) {
+		payload = xsdirxss_read(core, XSDIRX_ST352_DS1_REG);
+		byte1 = (payload >> XST352_PAYLOAD_BYTE1_SHIFT) &
+				XST352_PAYLOAD_BYTE_MASK;

Is XST352_PAYLOAD_BYTE_MASK correct ?
Should it be XST352_PAYLOAD_BYTE1_MASK ?

quoted

+		active_luma = (payload & XST352_BYTE3_ACT_LUMA_COUNT_MASK) >>
+				XST352_BYTE3_ACT_LUMA_COUNT_OFFSET;
+		pic_type = (payload & XST352_BYTE2_PIC_TYPE_MASK) >>
+				XST352_BYTE2_PIC_TYPE_OFFSET;
+		framerate = (payload >> XST352_BYTE2_FPS_SHIFT) &
+				XST352_BYTE2_FPS_MASK;
+		tscan = (payload & XST352_BYTE2_TS_TYPE_MASK) >>
+				XST352_BYTE2_TS_TYPE_OFFSET;

Please align consistently throughout the patch. I believe the checkpatch
--strict warns on these.

I believe not.

Another possibility would be to use a macro like:

#define mask_and_shift(val, type)	\
	((val) & (XST352_ ## type ## _MASK)) >> (XST352_ ## type ## _OFFSET))

quoted

+		sampling = (payload & XST352_BYTE3_COLOR_FORMAT_MASK) >>
+			   XST352_BYTE3_COLOR_FORMAT_OFFSET;

So these could be something like:

		sampling = mask_and_shift(payload, BYTE3_COLOR_FORMAT);