| Message ID | 1423651052-19593-9-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: [...] > -/** > - * snd_ctl_new - create a control instance from the template > - * @control: the control template > - * @access: the default control access > - * > - * Allocates a new struct snd_kcontrol instance and copies the given template > - * to the new instance. It does not copy volatile data (access). > - * > - * Return: The pointer of the new instance, or %NULL on failure. > - */ > -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, > - unsigned int access) > +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, I'd prefer to keep both the documentation as well as the 'snd_' prefix. Maybe just replace the '/**' with '/*' to prevent it from appearing in the public API documentation. - Lars
At Wed, 11 Feb 2015 13:49:29 +0100, Lars-Peter Clausen wrote: > > On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: > [...] > > -/** > > - * snd_ctl_new - create a control instance from the template > > - * @control: the control template > > - * @access: the default control access > > - * > > - * Allocates a new struct snd_kcontrol instance and copies the given template > > - * to the new instance. It does not copy volatile data (access). > > - * > > - * Return: The pointer of the new instance, or %NULL on failure. > > - */ > > -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, > > - unsigned int access) > > +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, > > I'd prefer to keep both the documentation as well as the 'snd_' prefix. > Maybe just replace the '/**' with '/*' to prevent it from appearing in the > public API documentation. Yes, agreed. There is no strict rule about snd_ prefix for non-exported objects. We prefer not having snd_ prefix for local stuff in general just for readability. But in this case, it doesn't improve so much. Takashi
On Feb 11 2015 22:04, Takashi Iwai wrote: > At Wed, 11 Feb 2015 13:49:29 +0100, > Lars-Peter Clausen wrote: >> >> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: >> [...] >>> -/** >>> - * snd_ctl_new - create a control instance from the template >>> - * @control: the control template >>> - * @access: the default control access >>> - * >>> - * Allocates a new struct snd_kcontrol instance and copies the given template >>> - * to the new instance. It does not copy volatile data (access). >>> - * >>> - * Return: The pointer of the new instance, or %NULL on failure. >>> - */ >>> -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, >>> - unsigned int access) >>> +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, >> >> I'd prefer to keep both the documentation as well as the 'snd_' prefix. >> Maybe just replace the '/**' with '/*' to prevent it from appearing in the >> public API documentation. > > Yes, agreed. There is no strict rule about snd_ prefix for > non-exported objects. We prefer not having snd_ prefix for local > stuff in general just for readability. But in this case, it doesn't > improve so much. Hm. I don't disagree with remaining these comments. ...But I have another issue. In my final patch in another series, I change this function drastically. Then, should I spend a bit time to revise them even if this is just a local function? http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/087796.html Thanks Takashi Sakamoto
At Thu, 12 Feb 2015 21:45:24 +0900, Takashi Sakamoto wrote: > > On Feb 11 2015 22:04, Takashi Iwai wrote: > > At Wed, 11 Feb 2015 13:49:29 +0100, > > Lars-Peter Clausen wrote: > >> > >> On 02/11/2015 11:37 AM, Takashi Sakamoto wrote: > >> [...] > >>> -/** > >>> - * snd_ctl_new - create a control instance from the template > >>> - * @control: the control template > >>> - * @access: the default control access > >>> - * > >>> - * Allocates a new struct snd_kcontrol instance and copies the given template > >>> - * to the new instance. It does not copy volatile data (access). > >>> - * > >>> - * Return: The pointer of the new instance, or %NULL on failure. > >>> - */ > >>> -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, > >>> - unsigned int access) > >>> +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, > >> > >> I'd prefer to keep both the documentation as well as the 'snd_' prefix. > >> Maybe just replace the '/**' with '/*' to prevent it from appearing in the > >> public API documentation. > > > > Yes, agreed. There is no strict rule about snd_ prefix for > > non-exported objects. We prefer not having snd_ prefix for local > > stuff in general just for readability. But in this case, it doesn't > > improve so much. > > Hm. I don't disagree with remaining these comments. > > ...But I have another issue. In my final patch in another series, I > change this function drastically. Then, should I spend a bit time to > revise them even if this is just a local function? Yes, please. Takashi
diff --git a/sound/core/control.c b/sound/core/control.c index 04534f3..af95783 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -193,18 +193,8 @@ void snd_ctl_notify(struct snd_card *card, unsigned int mask, } EXPORT_SYMBOL(snd_ctl_notify); -/** - * snd_ctl_new - create a control instance from the template - * @control: the control template - * @access: the default control access - * - * Allocates a new struct snd_kcontrol instance and copies the given template - * to the new instance. It does not copy volatile data (access). - * - * Return: The pointer of the new instance, or %NULL on failure. - */ -static struct snd_kcontrol *snd_ctl_new(struct snd_kcontrol *control, - unsigned int access) +static struct snd_kcontrol *ctl_new(struct snd_kcontrol *control, + unsigned int access) { struct snd_kcontrol *kctl; unsigned int size; @@ -273,7 +263,7 @@ struct snd_kcontrol *snd_ctl_new1(const struct snd_kcontrol_new *ncontrol, kctl.tlv.p = ncontrol->tlv.p; kctl.private_value = ncontrol->private_value; kctl.private_data = private_data; - return snd_ctl_new(&kctl, access); + return ctl_new(&kctl, access); } EXPORT_SYMBOL(snd_ctl_new1); @@ -281,9 +271,8 @@ EXPORT_SYMBOL(snd_ctl_new1); * snd_ctl_free_one - release the control instance * @kcontrol: the control instance * - * Releases the control instance created via snd_ctl_new() - * or snd_ctl_new1(). - * Don't call this after the control was added to the card. + * Releases the control instance created via ctl_new() or snd_ctl_new1(). Don't + * call this after the control was added to the card. */ void snd_ctl_free_one(struct snd_kcontrol *kcontrol) { @@ -334,9 +323,8 @@ static int snd_ctl_find_hole(struct snd_card *card, unsigned int count) * @card: the card instance * @kcontrol: the control instance to add * - * Adds the control instance created via snd_ctl_new() or - * snd_ctl_new1() to the given card. Assigns also an unique - * numid used for fast search. + * Adds the control instance created via ctl_new() or snd_ctl_new1() to the + * given card. Assigns also an unique numid used for fast search. * * It frees automatically the control which cannot be added. * @@ -1254,7 +1242,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, } } kctl.private_free = snd_ctl_elem_user_free; - _kctl = snd_ctl_new(&kctl, access); + _kctl = ctl_new(&kctl, access); if (_kctl == NULL) { kfree(ue->priv_data); kfree(ue);
The snd_ctl_new1() was added in 2001. I guess that snd_ctl_new() becames a local function in this timing. This commit arranges the function as a local helper function, remove 'snd_' prefix and comments, change comments to refer to the function. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/core/control.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-)