| Message ID | 67eca38a.d40a0220.22c3d5.88f3@mx.google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | staging: media: Fix indentation to use tabs instead of spaces | expand |
On Wed, Apr 2, 2025 at 5:40 AM <gshahrouzi@gmail.com> wrote: Is it your first patch to the Linux kernel? See my comments below. > >From d6a08153171ac52b438b6ddc1da50ebdd3550951 Mon Sep 17 00:00:00 2001 > From: Gabriel Shahrouzi <gshahrouzi@gmail.com> > Date: Tue, 1 Apr 2025 22:04:25 -0400 > Subject: [PATCH] staging: media: Fix indentation to use tabs instead of spaces First of all, your patch is mangled. You want to use proper tools, perhaps. One of which is `git format-patch ...` and another one is `git send-email ...` > Replace spaces with tab to comply with kernel coding style. Change 'tab' to 'TAB'. ... Change itself is okay, but is this the only one case in the entire driver (which is something like 100k LoCs long)? Even though, and while for the training purposes this is fine, you can also think about checking the common style of other functions, which may be shifted with TABs, but still having not enough spaces or so.
On Wed, Apr 2, 2025 at 3:12 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 2, 2025 at 5:40 AM <gshahrouzi@gmail.com> wrote: > > Is it your first patch to the Linux kernel? See my comments below. It's among the first patches I've submitted. > > > >From d6a08153171ac52b438b6ddc1da50ebdd3550951 Mon Sep 17 00:00:00 2001 > > From: Gabriel Shahrouzi <gshahrouzi@gmail.com> > > Date: Tue, 1 Apr 2025 22:04:25 -0400 > > Subject: [PATCH] staging: media: Fix indentation to use tabs instead of spaces > > First of all, your patch is mangled. You want to use proper tools, > perhaps. One of which is `git format-patch ...` and another one is > `git send-email ...` I was using git format-patch but not git send-email which seems to have been the issue. The meta-data from the patch was getting appended to the top. > > > Replace spaces with tab to comply with kernel coding style. > > Change 'tab' to 'TAB'. Got it. > > ... > > Change itself is okay, but is this the only one case in the entire > driver (which is something like 100k LoCs long)? Even though, and > while for the training purposes this is fine, you can also think about > checking the common style of other functions, which may be shifted > with TABs, but still having not enough spaces or so. Good point. Regarding formatting, it probably makes the most sense to address these issues comprehensively in a single cleanup pass (similar to https://lore.kernel.org/linux-staging/cover.1743524096.git.karanja99erick@gmail.com/T/#t). This particular instance caught my attention because I initially thought the author might have accidentally used spaces instead of tabs. The line in question used 2 tabs + 8 spaces, while subsequent similarly-aligned lines used 3 tabs. However, after examining different files in the driver, I noticed that while the formatting appears inconsistent, it likely exists for specific reasons. It's probably better to avoid changing a single detail without considering the broader formatting approach, and to treat checkpatch.pl more as a guide than the final authority. > > > -- > With Best Regards, > Andy Shevchenko
On Wed, Apr 2, 2025 at 3:28 PM Gabriel <gshahrouzi@gmail.com> wrote: > On Wed, Apr 2, 2025 at 3:12 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Wed, Apr 2, 2025 at 5:40 AM <gshahrouzi@gmail.com> wrote: > > > > Is it your first patch to the Linux kernel? See my comments below. > It's among the first patches I've submitted. Good start! ... > > > >From d6a08153171ac52b438b6ddc1da50ebdd3550951 Mon Sep 17 00:00:00 2001 > > > From: Gabriel Shahrouzi <gshahrouzi@gmail.com> > > > Date: Tue, 1 Apr 2025 22:04:25 -0400 > > > Subject: [PATCH] staging: media: Fix indentation to use tabs instead of spaces > > > > First of all, your patch is mangled. You want to use proper tools, > > perhaps. One of which is `git format-patch ...` and another one is > > `git send-email ...` > I was using git format-patch but not git send-email which seems to > have been the issue. The meta-data from the patch was getting appended to > the top. These are just normal headers for an email. They may be used by tools such as `git send-email ...` or `mutt -H ...`. ... > > Change itself is okay, but is this the only one case in the entire > > driver (which is something like 100k LoCs long)? Even though, and > > while for the training purposes this is fine, you can also think about > > checking the common style of other functions, which may be shifted > > with TABs, but still having not enough spaces or so. > Good point. Regarding formatting, it probably makes the most sense to > address these issues comprehensively in a single cleanup pass (similar > to https://lore.kernel.org/linux-staging/cover.1743524096.git.karanja99erick@gmail.com/T/#t). > This particular instance caught my attention because I initially > thought the author might have accidentally used spaces instead of > tabs. The line in question used 2 tabs + 8 spaces, while subsequent > similarly-aligned lines used 3 tabs. However, after examining > different files in the driver, I noticed that while the formatting > appears inconsistent, it likely exists for specific reasons. It's > probably better to avoid changing a single detail without considering > the broader formatting approach, and to treat checkpatch.pl more as a > guide than the final authority. Okay! In any case this patch is fine to me in case you want to send a v2.
diff --git a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c index ece5e3da34ee..127f12ba2214 100644 --- a/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c +++ b/drivers/staging/media/atomisp/pci/isp/kernels/vf/vf_1.0/ia_css_vf.host.c @@ -114,7 +114,7 @@ configure_dma( } int ia_css_vf_configure(const struct ia_css_binary *binary, - const struct ia_css_frame_info *out_info, + const struct ia_css_frame_info *out_info, struct ia_css_frame_info *vf_info, unsigned int *downscale_log2) {