Re: [PATCH] ALSA: emu10k1: Fix coding style issues and warnings

[Abhishek Tamboli <abhishektamboli9@xxxxxxxxx>]

On Tue, Aug 06, 2024 at 06:29:17PM +0200, Takashi Iwai wrote:
> On Mon, 05 Aug 2024 17:52:06 +0200,
> Abhishek Tamboli wrote:
> > 
> > Fix coding style errors and warnings in sound/pci/emu10k1/memory.c
> > reported by checkpatch.pl. Changes includes:
> > 
> > - Add spaces after commas in macro definitions.
> > - Insert blank lines after variable declarations.
> > - Consistent spacing around operators.
> > - Remove trailing whitespace.
> > - Move EXPORT_SYMBOL declarations to immediately follow their
> >   respective functions.
> > - Correct spacing in conditional statements and type casts.
> > 
> > Fix the following errors and warnings:
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define __set_ptb_entry(emu,page,addr) \
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define __set_ptb_entry(emu,page,addr) \
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_ptb_entry(emu,page,addr)	__set_ptb_entry(emu,page,addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_ptb_entry(emu,page,addr)	__set_ptb_entry(emu,page,addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_ptb_entry(emu,page,addr)	__set_ptb_entry(emu,page,addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_ptb_entry(emu,page,addr)	__set_ptb_entry(emu,page,addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_silent_ptb(emu,page)
> > 	__set_ptb_entry(emu,page,emu->silent_page.addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_silent_ptb(emu,page)
> > 	__set_ptb_entry(emu,page,emu->silent_page.addr)
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define set_silent_ptb(emu,page)
> > 	__set_ptb_entry(emu,page,emu->silent_page.addr)
> > 
> > WARNING: Missing a blank line after declarations
> > +	int i;
> > +	page *= UNIT_PAGES;
> > 
> > WARNING: Missing a blank line after declarations
> > +	int i;
> > +	page *= UNIT_PAGES;
> > 
> > ERROR: space required after that ',' (ctx:VxV)
> > +#define get_emu10k1_memblk(l,member)
> > 	list_entry(l, struct snd_emu10k1_memblk, member)
> > 
> > WARNING: space prohibited between function name and open parenthesis '('
> > +	list_for_each (pos, &emu->mapped_link_head) {
> > 
> > WARNING: Missing a blank line after declarations
> > +	struct snd_emu10k1_memblk *blk =
> > 		get_emu10k1_memblk(pos, mapped_link);
> > +	if (blk->mapped_page < 0)
> > 
> > ERROR: else should follow close brace '}'
> > +		}
> > +		else if (size > max_size) {
> > 
> > ERROR: need consistent spacing around '-' (ctx:WxV)
> > +	psize = get_aligned_page(size + PAGE_SIZE -1);
> >  	                                          ^
> > 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > +EXPORT_SYMBOL(snd_emu10k1_memblk_map);
> > 
> > WARNING: Missing a blank line after declarations
> > +		dma_addr_t addr;
> > +		if (ofs >= runtime->dma_bytes)
> > 
> > ERROR: space prohibited after that '!' (ctx:BxW)
> > +		if (! is_valid_page(emu, addr)) {
> >  		    ^
> > 
> > ERROR: trailing whitespace
> > +^Istruct snd_util_memhdr *hdr = hw->memhdr; $
> > 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > +EXPORT_SYMBOL(snd_emu10k1_synth_alloc);
> > 
> > ERROR: trailing whitespace
> > +^Istruct snd_util_memhdr *hdr = emu->memhdr; $
> > 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > +EXPORT_SYMBOL(snd_emu10k1_synth_free);
> > 
> > WARNING: Missing a blank line after declarations
> > +	int first_page, last_page;
> > +	first_page = blk->first_page;
> > 
> > WARNING: Missing a blank line after declarations
> > +	char *ptr;
> > +	if (snd_BUG_ON(page < 0 || page >= emu->max_cache_pages))
> > 
> > ERROR: space prohibited after that '!' (ctx:BxW)
> > +	if (! ptr) {
> >  	    ^
> > 
> > ERROR: "(foo*)" should be "(foo *)"
> > +	return (void*)ptr;
> > 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > +EXPORT_SYMBOL(snd_emu10k1_synth_memset);
> > 
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> > +EXPORT_SYMBOL(snd_emu10k1_synth_copy_from_user);
> > 
> > Signed-off-by: Abhishek Tamboli <abhishektamboli9@xxxxxxxxx>
> 
> Thanks for the patch.  But in general, I take this kind of
> white-space-only patches as a preliminary only when coupled with some
> other real fixes or improvement changes -- especially for old driver
> code.  Otherwise it'd just scratch the git history.
> 
> So, if you have other actual fixes for this driver based on this clean
> up, let me know.

Hi Takashi,

Thank you for the feedback. I understand your point regarding
white-space-only patches and I'll keep this in mind for future
contributions. Unfortunately, I don't have any additional
fixes or improvements for this driver at the moment. 

Regards,
Abhishek