| Message ID | 20240402204743.1069624-1-peter.colberg@intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2] fpga: dfl: fme: revise kernel-doc comments for some functions | expand |
On Tue, Apr 02, 2024 at 04:47:43PM -0400, Peter Colberg wrote: > From: Xu Yilun <yilun.xu@intel.com> > > 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'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. Do not just passively waiting for reviewers to find out the issue. Maybe you should again read the Documentation/process/*.rst 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, Yilun > Reported-by: kernel test robot <lkp@intel.com> > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > Signed-off-by: Peter Colberg <peter.colberg@intel.com> > --- > 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 > >
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@intel.com> > > > > 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@intel.com> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > > Signed-off-by: Peter Colberg <peter.colberg@intel.com> > > --- > > 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 > > > >
On Tue, Apr 09, 2024 at 06:30:45PM +0000, Colberg, Peter wrote: > 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@intel.com> > > > > > > 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 The out-of-date initial motivation, the commit 782d8e61b5d6, the listed logs are not related to your change. It shouldn't appear in this patch. Remember the commit message goes into git if the patch is merged. People get confused about these information. > addressed already; your patch merely polishes the the doc strings. When you decide to submit a patch public, it is *YOUR* patch. You should not list all the history and expect the original author decides what to do. > > > 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. This is not about the v1 patch, every new comer makes mistake. I just don't like that you sent patches too quickly but didn't address the previous concern. Thanks, Yilun
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)