linux-kernel - Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <39e6c0f7d7529da9906a17450a8bcdf416297520.camel@perches.com>
Date:   Thu, 13 Jun 2019 15:31:44 -0700
From:   Joe Perches <joe@...ches.com>
To:     Hyun Kwon <hyun.kwon@...inx.com>,
        Vishal Sagar <vishal.sagar@...inx.com>
Cc:     Hyun Kwon <hyunk@...inx.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Michal Simek <michals@...inx.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Dinesh Kumar <dineshk@...inx.com>,
        Sandip Kothari <sandipk@...inx.com>
Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
 driver

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 ?

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