| Message ID | 20240313114126.1418453-1-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] staging: bcm2835-audio: add terminating new line to Kconfig | expand |
On Wed, Mar 13, 2024 at 05:11:26PM +0530, Prasad Pandit wrote: > From: Prasad Pandit <pjp@fedoraproject.org> > > Add terminating new line to the Kconfig file. It helps > Kconfig parsers to read file without error. > > Signed-off-by: Prasad Pandit <pjp@fedoraproject.org> > --- > drivers/staging/vc04_services/bcm2835-audio/Kconfig | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > v3: fix typo and note about parsing errors. > -> https://lore.kernel.org/lkml/CAE8KmOzcD+__7xdC7tegbHO9HEP48s7=reA4j-tvqVDwzHr+8Q@mail.gmail.com/T/#t Normally this would go about the diffstat but no stress... Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org> regards, dan carpenter
On Wed, Mar 13, 2024 at 05:11:26PM +0530, Prasad Pandit wrote: > From: Prasad Pandit <pjp@fedoraproject.org> > > Add terminating new line to the Kconfig file. It helps > Kconfig parsers to read file without error. What in-tree parser has a problem with this file as-is? If it's an out-of-tree parser, that's different, and the tool should be fixed, no need to touch the kernel files for no good reason. thanks, greg k-h
On Mon, Mar 25, 2024 at 07:04:15PM +0100, Greg KH wrote: > On Wed, Mar 13, 2024 at 05:11:26PM +0530, Prasad Pandit wrote: > > From: Prasad Pandit <pjp@fedoraproject.org> > > > > Add terminating new line to the Kconfig file. It helps > > Kconfig parsers to read file without error. > > What in-tree parser has a problem with this file as-is? If it's an > out-of-tree parser, that's different, and the tool should be fixed, no > need to touch the kernel files for no good reason. It's annoying to cat a file when it doesn't have a newline on the end... dcarpenter@moroto:~/progs/kernel/trees$ cat -n drivers/staging/vc04_services/bcm2835-audio/Kconfig 1 # SPDX-License-Identifier: GPL-2.0 2 config SND_BCM2835 3 tristate "BCM2835 Audio" 4 depends on (ARCH_BCM2835 || COMPILE_TEST) && SND 5 select SND_PCM 6 select BCM2835_VCHIQ if HAS_DMA 7 help 8 Say Y or M if you want to support BCM2835 built in audio. 9 This driver handles both 3.5mm and HDMI audio, by leveraging 10 the VCHIQ messaging interface between the kernel and the firmware 11 running on VideoCore.dcarpenter@moroto:~/progs/kernel/trees$ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ So you could resend with that as a justification. But, yeah, it's a good idea to fix the tool as well. regards, dan carpenter
Hello Greg, Dan On Tue, 26 Mar 2024 at 01:12, Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Mon, Mar 25, 2024 at 07:04:15PM +0100, Greg KH wrote: >> If it's an out-of-tree parser, that's different, and the tool should be fixed, no >> need to touch the kernel files for no good reason. > It's annoying to cat a file when it doesn't have a newline on the end... > > dcarpenter@moroto:~/progs/kernel/trees$ cat -n drivers/staging/vc04_services/bcm2835-audio/Kconfig > 11 running on VideoCore.dcarpenter@moroto:~/progs/kernel/trees$ > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > So you could resend with that as a justification. But, yeah, it's a > good idea to fix the tool as well. * I'm trying to fix errors reported by the config-kernel[1] tool. It is an out-of-tree parser. * IMHO, this patch should be evaluated on whether not having the terminating new-line character is right or wrong? Saying that patch is not required because we don't know if it breaks an in-tree parser OR that patch is acceptable for the annoyance while using cat(1) command does not seem right. * As for fixing the parser, I did try to do that in the past but did not find a good fix for it, I'll try again. Meanwhile, I tried to find out how many such files are there? === $ tf=0 df=0; for f in $(find . -path "./[a-zA-Z0-9]*" -name Kconfig*); do tf=$(($tf+1)); eof=$(tail -c1 $f); if [ -n "$eof" -a "$eof" != "\n" ]; then echo $f; df=$(($df+1)); fi done 2> /dev/null; echo "No-NL-Files/Total-Files: $df/$tf"; ./drivers/staging/vc04_services/bcm2835-audio/Kconfig ./drivers/media/dvb-frontends/cxd2880/Kconfig No-NL-Files/Total-Files: 2/1698 $ tf=0 df=0; for f in $(find . -path "./[a-zA-Z0-9]*" -type f); do tf=$(($tf+1)); eof=$(tail -c1 $f); if [ -n "$eof" -a "$eof" != "\n" ]; then echo $f; df=$(($df+1)); fi done 2> /dev/null; echo "No-NL-Files/Total-Files: $df/$tf"; ./drivers/gpu/drm/i915/gt/shaders/README ./drivers/gpu/drm/xe/display/xe_fb_pin.c ./drivers/gpu/drm/sprd/Makefile ... No-NL-Files/Total-Files: 65/84319 === * There are 2 Kconfig files which don't terminate with a new-line character. I have sent fix patches for both of them. Rest of all ~99.90+% regular files follow the norm of terminating with a new-line character. Clearly files not terminating with a new-line are erroneous and should be fixed. Suggesting to fix the parsers to handle erroneous input files, which can be easily fixed in the source tree itself, is not reasonable. Maybe as Dan suggested, it'll help to add a check to 'checkpatch.pl' script to flag patches which try to add files without terminating new-lines. I tried to look into checkpatch.pl script, but it's quite big to understand in a day or two. * I'm okay to resend the patch saying annoyance while using cat(1) command as justification if that works. If in-tree parser is the only criteria, what would be the process to make config-kernel[1] tool an in-tree parser? I can try to follow that process. Thank you. --- - Prasad [1] https://github.com/pjps/config-kernel
diff --git a/drivers/staging/vc04_services/bcm2835-audio/Kconfig b/drivers/staging/vc04_services/bcm2835-audio/Kconfig index 7f22f6c85067..7fbb29d3c34d 100644 --- a/drivers/staging/vc04_services/bcm2835-audio/Kconfig +++ b/drivers/staging/vc04_services/bcm2835-audio/Kconfig @@ -8,4 +8,4 @@ config SND_BCM2835 Say Y or M if you want to support BCM2835 built in audio. This driver handles both 3.5mm and HDMI audio, by leveraging the VCHIQ messaging interface between the kernel and the firmware - running on VideoCore. \ No newline at end of file + running on VideoCore.