lkml.org 
[lkml]   [2022]   [Apr]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH v5 2/5] fpga: fix for coding style issues
    Date
    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
    >
    > 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>
    >
    > Sorry, the checkpatch maintainer Joe gives comments after my Acked-by, so
    > maybe more changes should be made, see my comments inline.
    >
    > 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.
    >
    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(...)' API
    > > as suggested by Joe.
    > > - To align Include declaration and definition of APIs updated
    > > the FPGA-region.h file as suggested by joe.
    >
    > The of_fpga_region_parse_ov() changes are also new to us.
    >
    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 = FPGA_MGR_STATE_WRITE_INIT;
    > > + count = min(mgr->mops->initial_header_size, count);
    >
    > Move the line in 'else' block, cause the count is not used if(!mgr->mops-
    > >initial_header_size).
    >
    Will fix in the next version.
    > > if (!mgr->mops->initial_header_size)
    > > ret = fpga_mgr_write_init(mgr, info, NULL, 0);
    > > else
    > > - ret = fpga_mgr_write_init(
    > > - mgr, info, buf, min(mgr->mops->initial_header_size,
    > count));
    > > + ret = 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 *))
    >
    > According to Joe's suggestion, it had better be:
    >
    > struct fpga_region *
    > fpga_region_class_find(struct device *start, const void *data,
    > int (*match)(struct device *, const void *))
    >
    >
    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)
    >
    > Don't put the '*' at the beginning, it should be:
    >
    Will fix in the next version.

    > static struct fpga_image_info *
    > of_fpga_region_parse_ov(struct fpga_region *region,
    > struct device_node *overlay)
    >
    > > {
    > > struct device *dev = &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 *));
    >
    > Please also follow Joe's suggestion:
    >
    > struct fpga_region *
    > fpga_region_class_find(struct device *start, const void *data,
    > int (*match)(struct device *, const void *));
    >
    >
    Will fix in the next version.

    Regards,
    Navakishore.

    \
     
     \ /
      Last update: 2022-04-22 05:39    [W:9.761 / U:0.088 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site