Re: [PATCH 1/2] ASoC: Add support for Conexant CX2072X CODEC

[Takashi Iwai <tiwai@suse.de>]

On Fri, 03 May 2019 08:47:29 +0200,
Mark Brown wrote:
> 
> > > > +#define cx2072x_plbk_eq_en_info		snd_ctl_boolean_mono_info
> 
> > > Why not just use the function directly rather than hiding it?
> 
> > Just a standard idiom.  Can be replaced if preferred.
> 
> Please.

BTW, a good reason for the style above is that it makes code more
undrestandable.  For defining a ctl element, typically we define three
callback functions, info, get and put, in that order:

static int foo_info()
{
	....
}

static int foo_get()
{
	....
}

static int foo_put()
{
	...
};

and often the actual snd_kcontrol_new table containing these callbacks
appears much later, where you'd need to scroll down a few screens to
read that point.

In the above, especially defining the info callback at the beginning
is important.  By reading foo_init() at first, readers can know which
type (int, boolean, enum, etc) and the number of elements to be used
in get/put callbacks beforehand.  It's a commonly seen mistake, for
example, that a wrong type (e.g. integer) is passed to info callback
while enum type is used in the get/put.

The #define above keeps this foo_info() definition while optimizing
with the standard helper.  If we drop this and set
snd_ctl_boolean_mono_info directly in the snd_kcontrol_new entry,
you'll have to go and back the screen just to take a look at the info
callback.

That's why I called it a standard idiom.  It's not strictly necessary,
but often help reading / reviewing the code.  Though, it's just
another bikeshed theme, so I'm not sticking with this style.


thanks,

Takashi