Re: [PATCH v2] fpga: dfl: fme: revise kernel-doc comments for some functions

[Colberg, Peter]

On Tue, 2024-04-09 at 11:39 +0800, Xu Yilun wrote:
> On Tue, Apr 02, 2024 at 04:47:43PM -0400, Peter Colberg wrote:
> > From: Xu Yilun <yilun.xu@xxxxxxxxx>
> > 
> > This amends commit 782d8e61b5d6 ("fpga: dfl: kernel-doc corrections"),
> > which separately addressed the kernel-doc warnings below. Add a more
> > precise description of the feature argument to dfl_fme_create_mgr(),
> > and also use plural in the description of dfl_fme_destroy_bridges().
> > 
> > lkp reported 2 build warnings:
> > 
> >    drivers/fpga/dfl/dfl-fme-pr.c:175: warning: Function parameter or member 'feature' not described in 'dfl_fme_create_mgr'
> > 
> > > > drivers/fpga/dfl/dfl-fme-pr.c:280: warning: expecting prototype for
> > > > dfl_fme_destroy_bridge(). Prototype was for dfl_fme_destroy_bridges()
> > > > instead
> 
> Why still list the 2 warnings here? Do they still exsit even with commit
> 782d8e61b5d6 ("fpga: dfl: kernel-doc corrections") ?
> 
> > 
> > Fixes: 29de76240e86 ("fpga: dfl: fme: add partial reconfiguration sub feature support")
> 
> You are still trying to fix this commit?

I included the commit message from your original patch in full to show
the initial motivation for the patch. As described, the issue has been
addressed already; your patch merely polishes the the doc strings.

> I'm sorry, but please do check and test your patches before submit.
> Re-submitting quickly but full of errors makes people doubt if you are
> really serious about your patches. At least, I do have doubt if you did
> tests for all your patches, or if your test could sufficiently prove the
> issue exists or fixed.

Apologies for sending the v1 patch, which had been rebased incorrectly.
The v2 patch is correct but can be dropped as you stated.

> 
> Do not just passively waiting for reviewers to find out the issue. Maybe
> you should again read the Documentation/process/*.rst

Apologies again for sending the v1 patch. I was not intending for
kernel reviewers to find any issues with the patch.

> 
> 
> Back to this patch, I think you can just drop it. Because:
> 1. The previous fix works fine, the doc doesn't tell anything wrong.
> 2. The 2 functions are internal, no outside users. Little value for the
> kernel doc.
> 
> So no need a dedicated fix patch. The preferred practice is you point
> out the nits when the previous patch is not yet merged. Or you by the
> way include these fixes in some new patches which relates to these
> functions.

Thanks for the review, I will drop the patch in the downstream tree.

Thanks,
Peter

> 
> Thanks,
> Yilun
> 
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx>
> > Signed-off-by: Peter Colberg <peter.colberg@xxxxxxxxx>
> > ---
> > v2:
> > - Correctly rebase patch onto commit 782d8e61b5d6.
> > ---
> >  drivers/fpga/dfl-fme-pr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/fpga/dfl-fme-pr.c b/drivers/fpga/dfl-fme-pr.c
> > index cdcf6dea4cc9..b66f2c1e10a9 100644
> > --- a/drivers/fpga/dfl-fme-pr.c
> > +++ b/drivers/fpga/dfl-fme-pr.c
> > @@ -164,7 +164,7 @@ static int fme_pr(struct platform_device *pdev, unsigned long arg)
> >  
> >  /**
> >   * dfl_fme_create_mgr - create fpga mgr platform device as child device
> > - * @feature: sub feature info
> > + * @feature: the dfl fme PR sub feature
> >   * @pdata: fme platform_device's pdata
> >   *
> >   * Return: mgr platform device if successful, and error code otherwise.
> > @@ -273,7 +273,7 @@ static void dfl_fme_destroy_bridge(struct dfl_fme_bridge *fme_br)
> >  }
> >  
> >  /**
> > - * dfl_fme_destroy_bridges - destroy all fpga bridge platform device
> > + * dfl_fme_destroy_bridges - destroy all fpga bridge platform devices
> >   * @pdata: fme platform device's pdata
> >   */
> >  static void dfl_fme_destroy_bridges(struct dfl_feature_platform_data *pdata)
> > -- 
> > 2.44.0
> > 
> >