Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Nava kishore Manne <navam@xilinx.com>
To:     Xu Yilun <yilun.xu@intel.com>
CC:     "mdf@kernel.org" <mdf@kernel.org>,
        "hao.wu@intel.com" <hao.wu@intel.com>,
        "trix@redhat.com" <trix@redhat.com>,
        Michal Simek <michals@xilinx.com>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>, git <git@xilinx.com>
Subject: RE: [PATCH v5 2/5] fpga: fix for coding style issues
Thread-Topic: [PATCH v5 2/5] fpga: fix for coding style issues
Thread-Index: AQHYVTr68HLIPy4mREOhtkhcmTvWhaz6gAiAgADJcmA=
Date:   Fri, 22 Apr 2022 03:38:41 +0000
Message-ID: <SN6PR02MB45768D3AE78598F410CF7AF1C2F79@SN6PR02MB4576.namprd02.prod.outlook.com>
References: <20220421044744.3777983-1-nava.manne@xilinx.com>
 <20220421044744.3777983-3-nava.manne@xilinx.com>
 <20220421153411.GA347460@yilunxu-OptiPlex-7050>
In-Reply-To: <20220421153411.GA347460@yilunxu-OptiPlex-7050>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?eWSixQ4pAYVSZHELFUkKxUpaAzwfN1gs5tk/3MyKySMpGGF3ji6I7quuAyEv?=
 =?us-ascii?Q?QmsqHOQa9/V1cou0kR7F3qSqgwEnDZgdjZcEpfY2m4oQbQp4eIQ+7fMQe7f/?=
 =?us-ascii?Q?r+ywHwdEynuJFMv8v80h0qpZxu7+nPLMJ1yjs9HWt7l9tUCpjM2B2nk6+981?=
 =?us-ascii?Q?hDZBqzBPRfv0BXGqzjRWg550gT7ZTmS62BmLkvc3v6V5JikMm85KnR+KqShk?=
 =?us-ascii?Q?BZrMcYtpOKLBdj1SjPgQDwNuJdNpag4dUe9vv9idWfsjS0BG47PxubJmt4W4?=
 =?us-ascii?Q?1kTtZzBpBRr04hu/CLbA0gqU6lG5RB1PaJn88nRANBEhuY/gTHWAHNIrC0Jh?=
 =?us-ascii?Q?fTxuqreC4uGAK6HrelglPqA4ObMJvkFX/nz/KNP5RdLj4/ujSG1rvRDHgyb9?=
 =?us-ascii?Q?bK+VE4EfZcLLLYwMfuqnN3tIYj48d8Grsv/4jOO3l1WNU1ueONvovI+LrWHA?=
 =?us-ascii?Q?5cvJswLAH4sm+MhjiOc4t5WNU/JcnzjLSWzxLB43FdQkasUOb0eJUdJUys+A?=
 =?us-ascii?Q?AgQqgXIzJKVeAnEusvXiPSyv8fzU8SXnWoC1ZWqKXr3PU5KydN+ECo6yHTPg?=
 =?us-ascii?Q?ESMu2ZeIayYetelhh0cy57XxNxlitlazkmWlRY/g7l/Uvy6YHgg0XUjkeWae?=
 =?us-ascii?Q?9uWUG2yCbhoLY4Z9n0pzXN2i7WtnSqt5Y19iD5Fw9NJ32rokD6++xjnsmilE?=
 =?us-ascii?Q?9jqcS9FnXjzC3Nd3pTUAOFLVpz8PkPoOGqGff82duB8PUmrjU7S7aP6ZVqYx?=
 =?us-ascii?Q?8uvXFQXZI1Jxg7YibFeVJFcgtbcXWmRIAVfVigPkp9LOA/ACVfAasJybaop7?=
 =?us-ascii?Q?S4mAE6qyWycvOjhtzAlM2vvLOd2jdLnPKs34Ab3gDLqE1fU7YsWFWhKyX7/E?=
 =?us-ascii?Q?SElberKDE77zbXChySyyQkuUSrwrA++eRJGr0fv6NBqckSsr/1e6tnjJNU6h?=
 =?us-ascii?Q?fIxoF4dm8jV2uZh6sJ5jHsMV2HTMBUDBTpFmUnACGtinOpfxSX2u7JBlBcAU?=
 =?us-ascii?Q?85y1ihl8+em0/JIqzFeAh0KVqIu9pjNLyPdlHLiiU9YDY+A20IX0dBXRykYk?=
 =?us-ascii?Q?c3vz/0fowQrHk1Y3vatubm6QIxqc/YGNhxALOMpAbP8tSVc0MuhQn8na3JD8?=
 =?us-ascii?Q?E0CqEsEbLy6OOUFKBoOhfGQI/X2NrTp5k+eS7SzicYSRP+5R/GaRWsT53W3G?=
 =?us-ascii?Q?qWnbcLe5HPr9SSwSrRzAUi/gn9okQfXbjVIzR6Z/W/1R8QHVLi/Se+7Ntu9a?=
 =?us-ascii?Q?l60xEhQvKukVgXnXHg1Ecl2Mer3OobtS6o5RB9uiWGEo9lMejanQBiCkuytp?=
 =?us-ascii?Q?OYfaGZQWuDEpD5xnN1vOdb1HCHmevF9jWPruOYZjjaAhSqevOLKpwuUhm6Ls?=
 =?us-ascii?Q?SHFf09oQplY41+yuT8qM7hdtu+gWI0501ltA4XQTIQXzVl2ri3r9CxUCBB2C?=
 =?us-ascii?Q?p5LgOydm6N/AxPzE4IgkjsQDFiKP7zVz+umoPd41yQ4uSpR/83DBM8OJjAaU?=
 =?us-ascii?Q?eCnb1Qh80kb7uaGPziwJZZ0S6zvq5aA9hBgmXyKCtXp+DUar1EtYO0WsVMIJ?=
 =?us-ascii?Q?IeOZHnn1vD+bt6/579JKCJ8sTW0l5V/gxE9OXR+5Oc+iBbLO7mzgDAYBVj8A?=
 =?us-ascii?Q?8YTGuMO2uAgg7mHjEomyIJ4k1f4TGfDBMebYUo9YJVXPSQi5wSnQ/M0XyRo7?=
 =?us-ascii?Q?mhyRmQOXb3w/tXhIGzUAf2dX+F+JmjSVYYKikKldSQiTSVSt4OXm31kMLQsq?=
 =?us-ascii?Q?yaZpGJ1ANQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: xilinx.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR02MB4576.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 648b961d-95fc-4a50-5859-08da2411983a
X-MS-Exchange-CrossTenant-originalarrivaltime: 22 Apr 2022 03:38:41.2726
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Kqlm2TT65sMvSOzfs/4zqZEH3JTThL8yffJJXHDmWAB9YBHyYImlq9l4ej30wI5fDbMHgu+xkAz4QNByde55gQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB4517
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Yilun,

Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@intel.com>
> Sent: Thursday, April 21, 2022 9:04 PM
> To: Nava kishore Manne <navam@xilinx.com>
> Cc: mdf@kernel.org; hao.wu@intel.com; trix@redhat.com; Michal Simek
> <michals@xilinx.com>; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org; git
> <git@xilinx.com>
> Subject: Re: [PATCH v5 2/5] fpga: fix for coding style issues
>=20
> On Thu, Apr 21, 2022 at 10:17:41AM +0530, Nava kishore Manne wrote:
> > fixes the below checks reported by checkpatch.pl:
> > - Lines should not end with a '('
> > - Alignment should match open parenthesis
> >
> > Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com>
> > Acked-by: Xu Yilun <yilun.xu@intel.com>
>=20
> Sorry, the checkpatch maintainer Joe gives comments after my Acked-by, so
> maybe more changes should be made, see my comments inline.
>=20
> And if you have more changes than expectation, it is better to remove the
> Acked-by/Reviewed-by tags, otherwise people thought the reviewers are
> good to new changes which are actually not checked at all.
>=20
Agree, Will remove in the next version.
> > ---
> > Changes for v2:
> >                 -None.
> > Changes for v3:
> >                -Fixed similar issue exists in "drivers/fpga/*".
> > Changes for v4:
> >                -None.
> > Changes for v5:
> >               - Reduced the length of the 'fpga_mgr_write_init(...)' AP=
I
> >                 as suggested by Joe.
> >               - To align Include declaration and definition of APIs upd=
ated
> >                 the FPGA-region.h file as suggested by joe.
>=20
> The of_fpga_region_parse_ov() changes are also new to us.
>=20
Will fix in the next version.
> >
> >  drivers/fpga/fpga-mgr.c          | 4 ++--
> >  drivers/fpga/fpga-region.c       | 7 ++++---
> >  drivers/fpga/of-fpga-region.c    | 6 +++---
> >  include/linux/fpga/fpga-region.h | 7 ++++---
> >  4 files changed, 13 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c index
> > d49a9ce34568..d9a2aad7b35f 100644
> > --- a/drivers/fpga/fpga-mgr.c
> > +++ b/drivers/fpga/fpga-mgr.c
> > @@ -148,11 +148,11 @@ static int fpga_mgr_write_init_buf(struct
> fpga_manager *mgr,
> >  	int ret;
> >
> >  	mgr->state =3D FPGA_MGR_STATE_WRITE_INIT;
> > +	count =3D min(mgr->mops->initial_header_size, count);
>=20
> Move the line in 'else' block, cause the count is not used if(!mgr->mops-
> >initial_header_size).
>=20
Will fix in the next version.
> >  	if (!mgr->mops->initial_header_size)
> >  		ret =3D fpga_mgr_write_init(mgr, info, NULL, 0);
> >  	else
> > -		ret =3D fpga_mgr_write_init(
> > -		    mgr, info, buf, min(mgr->mops->initial_header_size,
> count));
> > +		ret =3D fpga_mgr_write_init(mgr, info, buf, count);
> >
> >  	if (ret) {
> >  		dev_err(&mgr->dev, "Error preparing FPGA for writing\n");
> diff
> > --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c index
> > b0ac18de4885..3864bf4f8920 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -18,9 +18,10 @@
> >  static DEFINE_IDA(fpga_region_ida);
> >  static struct class *fpga_region_class;
> >
> > -struct fpga_region *fpga_region_class_find(
> > -	struct device *start, const void *data,
> > -	int (*match)(struct device *, const void *))
> > +struct fpga_region *fpga_region_class_find(struct device *start,
> > +					   const void *data,
> > +					   int (*match)(struct device *,
> > +							const void *))
>=20
> According to Joe's suggestion, it had better be:
>=20
> struct fpga_region *
> fpga_region_class_find(struct device *start, const void *data,
> 		       int (*match)(struct device *, const void *))
>=20
>=20
Will fix in the next version.

> >  {
> >  	struct device *dev;
> >
> > diff --git a/drivers/fpga/of-fpga-region.c
> > b/drivers/fpga/of-fpga-region.c index 50b83057c048..55209737075f
> > 100644
> > --- a/drivers/fpga/of-fpga-region.c
> > +++ b/drivers/fpga/of-fpga-region.c
> > @@ -189,9 +189,9 @@ static int child_regions_with_firmware(struct
> device_node *overlay)
> >   *   fpga_image_info struct if there is an image to program.
> >   *   error code for invalid overlay.
> >   */
> > -static struct fpga_image_info *of_fpga_region_parse_ov(
> > -						struct fpga_region *region,
> > -						struct device_node *overlay)
> > +static struct fpga_image_info
> > +*of_fpga_region_parse_ov(struct fpga_region *region,
> > +			 struct device_node *overlay)
>=20
> Don't put the '*' at the beginning, it should be:
>=20
Will fix in the next version.

> static struct fpga_image_info *
> of_fpga_region_parse_ov(struct fpga_region *region,
> 			struct device_node *overlay)
>=20
> >  {
> >  	struct device *dev =3D &region->dev;
> >  	struct fpga_image_info *info;
> > diff --git a/include/linux/fpga/fpga-region.h
> > b/include/linux/fpga/fpga-region.h
> > index 3b87f232425c..7ebf743b8f8a 100644
> > --- a/include/linux/fpga/fpga-region.h
> > +++ b/include/linux/fpga/fpga-region.h
> > @@ -52,9 +52,10 @@ struct fpga_region {
> >
> >  #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
> >
> > -struct fpga_region *fpga_region_class_find(
> > -	struct device *start, const void *data,
> > -	int (*match)(struct device *, const void *));
> > +struct fpga_region *fpga_region_class_find(struct device *start,
> > +					   const void *data,
> > +					   int (*match)(struct device *,
> > +							const void *));
>=20
> Please also follow Joe's suggestion:
>=20
> struct fpga_region *
> fpga_region_class_find(struct device *start, const void *data,
> 		       int (*match)(struct device *, const void *));
>=20
>
Will fix in the next version.

Regards,
Navakishore.