Revert "ASoC: core: mark SND_SOC_BYTES_EXT as deprecated"
diff mbox

Message ID 1473947149-6663-1-git-send-email-o-takashi@sakamocchi.jp
State New
Headers show

Commit Message

Takashi Sakamoto Sept. 15, 2016, 1:45 p.m. UTC
SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
to a protocol of ctl and tlv operation. At least, this macro and related
kernel APIs include two misunderstandings:
 - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
   packet paylaod, snd_soc_bytes_info_ext() performs in this way.
 - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
   packet structure, snd_soc_bytes_tlv_callback() performs in this way.

In a policy of kernel land development, it's quite worse to break protocols
for applications. Therefore, developers are discouraged to use these kernel
APIs.

In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
developers who need to add control elements which transfer data larger than
the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
bytes. However, as long as the size is less than the size; e.g. 512 bytes,
SND_SOC_BYTES_EXT is still available. Although there is actually the
limitation of maximum data size, it's better to use this API for stable
application interface till better alternative ways are implemented in
future.

For these reasons, this commit reverts the previous commit which lead
developers to the worse behaviour.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 include/sound/soc.h | 3 ---
 1 file changed, 3 deletions(-)

Comments

Charles Keepax Sept. 15, 2016, 2:21 p.m. UTC | #1
On Thu, Sep 15, 2016 at 10:45:49PM +0900, Takashi Sakamoto wrote:
> SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
> to a protocol of ctl and tlv operation. At least, this macro and related
> kernel APIs include two misunderstandings:
>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.
> 
> In a policy of kernel land development, it's quite worse to break protocols
> for applications. Therefore, developers are discouraged to use these kernel
> APIs.
> 
> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
> developers who need to add control elements which transfer data larger than
> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
> bytes. However, as long as the size is less than the size; e.g. 512 bytes,
> SND_SOC_BYTES_EXT is still available. Although there is actually the
> limitation of maximum data size, it's better to use this API for stable
> application interface till better alternative ways are implemented in
> future.
> 
> For these reasons, this commit reverts the previous commit which lead
> developers to the worse behaviour.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>

Yeah would be better to use normal controls for <512 bytes, so
this looks good to me.

Although you might want to resend using Mark's kernel.org address
as I imagine he would be the one to apply the change and that is
the address he normally uses for patches.

Thanks,
Charles
Takashi Sakamoto Sept. 15, 2016, 2:41 p.m. UTC | #2
On Sep 15 2016 23:21, Charles Keepax wrote:
> On Thu, Sep 15, 2016 at 10:45:49PM +0900, Takashi Sakamoto wrote:
>> SND_SOC_BYTES_TLV brings confusions to user land because it doesn't follow
>> to a protocol of ctl and tlv operation. At least, this macro and related
>> kernel APIs include two misunderstandings:
>>  - 'struct snd_ctl_elem_info.count' can also represent the length of TLV
>>    packet paylaod, snd_soc_bytes_info_ext() performs in this way.
>>  - 'struct snd_ctl_tlv.tlv' can include arbitrary data regardless of TLV
>>    packet structure, snd_soc_bytes_tlv_callback() performs in this way.
>>
>> In a policy of kernel land development, it's quite worse to break protocols
>> for applications. Therefore, developers are discouraged to use these kernel
>> APIs.
>>
>> In the first place, SND_SOC_BYTES_TLV was added to satisfy a request of
>> developers who need to add control elements which transfer data larger than
>> the size which 'struct snd_ctl_elem_value' can represent; e.g. over 512
>> bytes. However, as long as the size is less than the size; e.g. 512 bytes,
>> SND_SOC_BYTES_EXT is still available. Although there is actually the
>> limitation of maximum data size, it's better to use this API for stable
>> application interface till better alternative ways are implemented in
>> future.
>>
>> For these reasons, this commit reverts the previous commit which lead
>> developers to the worse behaviour.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> ---
> 
> Acked-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> 
> Yeah would be better to use normal controls for <512 bytes, so
> this looks good to me.
> 
> Although you might want to resend using Mark's kernel.org address
> as I imagine he would be the one to apply the change and that is
> the address he normally uses for patches.

Oh, he uses the domain instead of linaro.org. I missed it.

Now I add him to CC list.


Thanks

Takashi Sakamoto
Mark Brown Sept. 15, 2016, 2:54 p.m. UTC | #3
On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
> On Sep 15 2016 23:21, Charles Keepax wrote:

> > Although you might want to resend using Mark's kernel.org address
> > as I imagine he would be the one to apply the change and that is
> > the address he normally uses for patches.

> Oh, he uses the domain instead of linaro.org. I missed it.

> Now I add him to CC list.

As documented in SubmittingPatches please send patches to the 
maintainers for the code you would like to change.  The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.
Takashi Sakamoto Sept. 15, 2016, 10:29 p.m. UTC | #4
On Sep 15 2016 23:54, Mark Brown wrote:
> On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
>> On Sep 15 2016 23:21, Charles Keepax wrote:
> 
>>> Although you might want to resend using Mark's kernel.org address
>>> as I imagine he would be the one to apply the change and that is
>>> the address he normally uses for patches.
> 
>> Oh, he uses the domain instead of linaro.org. I missed it.
> 
>> Now I add him to CC list.
> 
> As documented in SubmittingPatches please send patches to the 
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.

I see. I'll post this to below addresses.

$ ./scripts/get_maintainer.pl
/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch
fatal:
/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch:
'/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch'
is outside repository
Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
DYNAMIC AUDIO POWER MANAGEM...)
Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
alsa-devel@alsa-project.org (moderated list:SOUND - SOC LAYER / DYNAMIC
AUDIO POWER MANAGEM...)
linux-kernel@vger.kernel.org (open list)


Takashi Sakamoto
Takashi Sakamoto Sept. 15, 2016, 11:26 p.m. UTC | #5
On Sep 16 2016 07:29, Takashi Sakamoto wrote:
> On Sep 15 2016 23:54, Mark Brown wrote:
>> On Thu, Sep 15, 2016 at 11:41:43PM +0900, Takashi Sakamoto wrote:
>>> On Sep 15 2016 23:21, Charles Keepax wrote:
>>
>>>> Although you might want to resend using Mark's kernel.org address
>>>> as I imagine he would be the one to apply the change and that is
>>>> the address he normally uses for patches.
>>
>>> Oh, he uses the domain instead of linaro.org. I missed it.
>>
>>> Now I add him to CC list.
>>
>> As documented in SubmittingPatches please send patches to the 
>> maintainers for the code you would like to change.  The normal kernel
>> workflow is that people apply patches from their inboxes, if they aren't
>> copied they are likely to not see the patch at all and it is much more
>> difficult to apply patches.
> 
> I see. I'll post this to below addresses.
> 
> $ ./scripts/get_maintainer.pl
> /tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch
> fatal:
> /tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch:
> '/tmp/patch/0001-Revert-ASoC-core-mark-SND_SOC_BYTES_EXT-as-deprecate.patch'
> is outside repository
> Liam Girdwood <lgirdwood@gmail.com> (supporter:SOUND - SOC LAYER /
> DYNAMIC AUDIO POWER MANAGEM...)
> Mark Brown <broonie@kernel.org> (supporter:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> Jaroslav Kysela <perex@perex.cz> (maintainer:SOUND)
> Takashi Iwai <tiwai@suse.com> (maintainer:SOUND)
> alsa-devel@alsa-project.org (moderated list:SOUND - SOC LAYER / DYNAMIC
> AUDIO POWER MANAGEM...)
> linux-kernel@vger.kernel.org (open list)

I changed my mind. I just added you to CC list of new patch, then sent
it. Please apply it your tree after reviewing. I don't like to be
involved to the yeast, and dull work.


Regards

Takashi Sakamoto

Patch
diff mbox

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6144882..18f7d21 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -311,9 +311,6 @@ 
 		{.base = xbase, .num_regs = xregs,	      \
 		 .mask = xmask }) }
 
-/*
- * SND_SOC_BYTES_EXT is deprecated, please USE SND_SOC_BYTES_TLV instead
- */
 #define SND_SOC_BYTES_EXT(xname, xcount, xhandler_get, xhandler_put) \
 {	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
 	.info = snd_soc_bytes_info_ext, \