Hi Joe, Thanks for reviewing. > -----Original Message----- > From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- > owner@xxxxxxxxxxxxxxx] On Behalf Of Joe Perches > Sent: Friday, June 14, 2019 4:02 AM > To: Hyun Kwon <hyunk@xxxxxxxxxx>; Vishal Sagar <vishal.sagar@xxxxxxxxxx> > Cc: Hyun Kwon <hyunk@xxxxxxxxxx>; Laurent Pinchart > <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab > <mchehab@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; linux- > kernel@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Dinesh Kumar > <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx> > 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: > > On Tue, 2019-06-04 at 06:55:56 -0700, Vishal Sagar wrote: > > trivia: > > > > diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c > b/drivers/media/platform/xilinx/xilinx-sdirxss.c > [] > > > +static int xsdirx_get_stream_properties(struct xsdirxss_state *state) > > > +{ > [] > > > + 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 > > > + 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)) > > > > + 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
