On Fri, 02 Sep 2016 13:18:04 +0200,
Takashi Sakamoto wrote:
>
> Clemens,
>
> On Aug 31 2016 20:54, Clemens Ladisch wrote:
> > Takashi Iwai wrote:
> >> TLV has never been and (will never be) an API to handle a
> >> generic binary stream.
> >
> > It would be possible to define something like SNDRV_CTL_TLVT_HWDEP_BLOB
> > or _COEFFICIENTS, or to reserve a range of TLVT values for driver-
> > defined types. (But we cannot change soc-wm-adsp to support only proper
> > TLV data, because this would introduce a regression.)
>
> For our information, I wrote a patch including your idea.
>
>
> Thanks for your bright comment ;)
>
> Takashi Sakamoto
>
> ---- 8< ----
>
> >From d1a0eabf1b33fae5de7cedf7aee23778f5dcf46c Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@xxxxxxxxxxxxx>
> Date: Fri, 2 Sep 2016 00:12:20 +0900
> Subject: [PATCH] ALSA: control: coefficient support
>
> ---
> include/uapi/sound/asound.h | 5 +++++
> include/uapi/sound/tlv.h | 1 +
> sound/soc/codecs/wm_adsp.c | 3 ++-
> sound/soc/soc-ops.c | 37 ++++++++++++++++++++++++++++++++++---
> sound/soc/soc-topology.c | 3 ++-
> 5 files changed, 44 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
> index 609cadb..69c0585 100644
> --- a/include/uapi/sound/asound.h
> +++ b/include/uapi/sound/asound.h
> @@ -848,6 +848,11 @@ typedef int __bitwise snd_ctl_elem_iface_t;
> #define SNDRV_CTL_ELEM_ACCESS_TLV_WRITE (1<<5) /* TLV write is possible */
> #define SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> (SNDRV_CTL_ELEM_ACCESS_TLV_READ|SNDRV_CTL_ELEM_ACCESS_TLV_WRITE)
> #define SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND (1<<6) /* TLV command is
> possible */
> +/*
> + * Arbitrary data is accepted for coefficients, instead of pure
> threshold level
> + * information.
> + */
> +#define SNDRV_CTL_ELEM_ACCESS_TLV_COEFF (1<<7)
> #define SNDRV_CTL_ELEM_ACCESS_INACTIVE (1<<8) /* control does actually
> nothing, but may be updated */
> #define SNDRV_CTL_ELEM_ACCESS_LOCK (1<<9) /* write lock */
> #define SNDRV_CTL_ELEM_ACCESS_OWNER (1<<10) /* write lock owner */
The introduction of a new flag is a good idea (although it's better to
be named not specifically to coef). Then we can skip such elements to
be accessed from alsactl or amixer.
However...
> @@ -773,19 +774,49 @@ int snd_soc_bytes_tlv_callback(struct snd_kcontrol
> *kcontrol, int op_flag,
> unsigned int size, unsigned int __user *tlv)
> {
> struct soc_bytes_ext *params = (void *)kcontrol->private_value;
> - unsigned int count = size < params->max ? size : params->max;
> + unsigned int count;
> + unsigned int type;
> int ret = -ENXIO;
>
> + /*
> + * The TLV packet can transfer numerical ID for one control element.
> + * But ALSA control core don't tell it to each implementation of
> + * TLV callback. Here, instead, use the first volatile data to
> + * check access information.
> + */
> + if (!(kcontrol->vd[0].access & SNDRV_CTL_ELEM_ACCESS_TLV_COEFF))
> + return -ENXIO;
> +
> + /* The data should be constructed according to TLV protocol. */
> + if (copy_from_user(&type, tlv, sizeof(unsigned int)))
> + return -EFAULT;
> +
> + /* The type should be an arbitrary data. */
> + if (type != SNDRV_CTL_TLVT_COEFF)
> + return -ENXIO;
This breaks the already existing application and is a regression, as
Clemens mentioned. This should have been forced from the beginning,
but it's too late to put now. So, it's likely no-go.
But, the addition of the flag is helpful alone. Could you concentrate
on that and resubmit?
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
