Hi Joe,

Thanks for reviewing. 

-----Original Message-----
From: linux-media-owner@vger.kernel.org [mailto:linux-media-
owner@vger.kernel.org] On Behalf Of Joe Perches
Sent: Friday, June 14, 2019 4:02 AM
To: Hyun Kwon <redacted>; Vishal Sagar <redacted>
Cc: Hyun Kwon <redacted>; Laurent Pinchart
[off-list ref]; Mauro Carvalho Chehab
[off-list ref]; Michal Simek [off-list ref]; Rob Herring
[off-list ref]; Mark Rutland [off-list ref]; linux-
kernel@vger.kernel.org; linux-media@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; devicetree@vger.kernel.org; Dinesh Kumar
[off-list ref]; Sandip Kothari [off-list ref]
Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
driver

EXTERNAL EMAIL

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

quoted

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

trivia:

quoted

quoted

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

b/drivers/media/platform/xilinx/xilinx-sdirxss.c
[]

quoted

quoted

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

[]

quoted

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 ?

I had thought of it to be a generic mask to extract a byte out of 4 bytes in a ST352 packet.
Hence named it as XST352_PAYLOAD_BYTE_MASK

quoted

quoted

+           active_luma = (payload &

XST352_BYTE3_ACT_LUMA_COUNT_MASK) >>

quoted

quoted

+                           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

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);

This looks like a good way. I will modify this in v2. 
I will also modify the XST352_PAYLOAD_BYTE_MASK to 
XST352_PAYLOAD_BYTE1_MASK so that this aligns with the macro.

Regards
Vishal Sagar


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel