At Fri, 16 Jun 2006 13:00:57 +0200 (CEST),
Jaroslav Kysela wrote:
>
> On Fri, 16 Jun 2006, Takashi Iwai wrote:
>
> > At Wed, 14 Jun 2006 16:14:17 +0200,
> > I wrote:
> > >
> > > > What I am trying to say, is that your "simplification" of the API, has
> > > > resulted in this sort of functionality being excluded, so I would need
> > > > to implement yet another IOCTL to support it! My whole point of using
> > > > the full request/response TLV api, was to not restrict us. I was also
> > > > intending to maybe use the TLVs to set values, as well as just get
> > > > values. For example, another way to implement the "continuous gain"
> > > > feature would be to use the TLV api to set a flag, so that the 32bit
> > > > values are returned via the current snd_ca0106_volume_get(), but only
> > > > for this particular mixer open/close cycle, for backward compatibility
> > > > with the current api.
> > > >
> > > > Do you prefer separate new IOCTLs for all these features?
> > >
> > > We can modify the implementation. For example, the patch like one
> > > below. tlv_rw callback is completely free for own implementation.
> > > Each callback is in change of reading/writing the given user-space
> > > pointer arbitrarily.
> >
> > After reconsideration, I found it's better to split the ioctl.
> > It's not nice that the caller has no idea whether the driver actually
> > reads the value or not.
>
> Yes, r/w ops should be splited (it's the reason why I used TLV_READ for
> ioctl).
>
> > Still I feel that the implementation of the callback is OK -- keep the
> > core stuff as small and simple as possible, and let callbacks parse
> > the user-pointer.
>
> I have some cleanup in my tree to reduce memory usage. See bellow. Also,
> while implementing, I realized that it might be worth to add
> SNDRV_CTL_ELEM_ACCESS_TLV_READ and SNDRV_CTL_ELEM_ACCESS_TLV_WRITE flags
> for control elements. Comments?
Looks almost OK but white space problems.
Anyway, let's wait for the consensus.
Takashi
>
> Jaroslav
>
> diff -r 08577d0b45ef core/control.c
> --- a/core/control.c Tue Jun 13 12:01:14 2006 +0200
> +++ b/core/control.c Fri Jun 16 12:57:54 2006 +0200
> @@ -241,7 +241,7 @@ struct snd_kcontrol *snd_ctl_new1(const
> kctl.info = ncontrol->info;
> kctl.get = ncontrol->get;
> kctl.put = ncontrol->put;
> - kctl.tlv = ncontrol->tlv;
> + kctl.tlv.p = ncontrol->tlv.p;
> kctl.private_value = ncontrol->private_value;
> kctl.private_data = private_data;
> return snd_ctl_new(&kctl, access);
> @@ -1068,8 +1068,9 @@ static int snd_ctl_subscribe_events(stru
> return 0;
> }
>
> -static int snd_ctl_tlv_read(struct snd_card *card,
> - struct snd_ctl_tlv __user *_tlv)
> +static int snd_ctl_tlv_ioctl(struct snd_card *card,
> + struct snd_ctl_tlv __user *_tlv,
> + int write_flag)
> {
> struct snd_ctl_tlv tlv;
> struct snd_kcontrol *kctl;
> @@ -1086,17 +1087,26 @@ static int snd_ctl_tlv_read(struct snd_c
> err = -ENOENT;
> goto __kctl_end;
> }
> - if (kctl->tlv == NULL) {
> - err = -ENXIO;
> - goto __kctl_end;
> - }
> - len = kctl->tlv[1] + 2 * sizeof(unsigned int);
> - if (tlv.length < len) {
> - err = -ENOMEM;
> - goto __kctl_end;
> - }
> - if (copy_to_user(_tlv->tlv, kctl->tlv, len))
> - err = -EFAULT;
> + if (kctl->tlv.p == NULL) {
> + err = -ENXIO;
> + goto __kctl_end;
> + }
> + if (kctl->vd[tlv.numid - kctl->id.numid].access &
> + SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> + err = kctl->tlv.c(kctl, write_flag, tlv.length, _tlv->tlv);
> + } else {
> + if (write_flag) {
> + err = -ENXIO;
> + goto __kctl_end;
> + }
> + len = kctl->tlv.p[1] + 2 * sizeof(unsigned int);
> + if (tlv.length < len) {
> + err = -ENOMEM;
> + goto __kctl_end;
> + }
> + if (copy_to_user(_tlv->tlv, kctl->tlv.p, len))
> + err = -EFAULT;
> + }
> __kctl_end:
> up_read(&card->controls_rwsem);
> return err;
> @@ -1141,7 +1151,9 @@ static long snd_ctl_ioctl(struct file *f
> case SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS:
> return snd_ctl_subscribe_events(ctl, ip);
> case SNDRV_CTL_IOCTL_TLV_READ:
> - return snd_ctl_tlv_read(card, argp);
> + return snd_ctl_tlv_ioctl(card, argp, 0);
> + case SNDRV_CTL_IOCTL_TLV_WRITE:
> + return snd_ctl_tlv_ioctl(card, argp, 1);
> case SNDRV_CTL_IOCTL_POWER:
> return -ENOPROTOOPT;
> case SNDRV_CTL_IOCTL_POWER_STATE:
> diff -r 08577d0b45ef include/asound.h
> --- a/include/asound.h Tue Jun 13 12:01:14 2006 +0200
> +++ b/include/asound.h Fri Jun 16 12:57:54 2006 +0200
> @@ -731,6 +731,7 @@ typedef int __bitwise snd_ctl_elem_iface
> #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 */
> +#define SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK (1<<28) /* kernel use a TLV callback */
> #define SNDRV_CTL_ELEM_ACCESS_USER (1<<29) /* user space element */
> #define SNDRV_CTL_ELEM_ACCESS_DINDIRECT (1<<30) /* indirect access for matrix dimensions in the info structure */
> #define SNDRV_CTL_ELEM_ACCESS_INDIRECT (1<<31) /* indirect access for element value in the value structure */
> @@ -838,6 +839,7 @@ enum {
> SNDRV_CTL_IOCTL_ELEM_REPLACE = _IOWR('U', 0x18, struct snd_ctl_elem_info),
> SNDRV_CTL_IOCTL_ELEM_REMOVE = _IOWR('U', 0x19, struct snd_ctl_elem_id),
> SNDRV_CTL_IOCTL_TLV_READ = _IOWR('U', 0x1a, struct snd_ctl_tlv),
> + SNDRV_CTL_IOCTL_TLV_WRITE = _IOWR('U', 0x1b, struct snd_ctl_tlv),
> SNDRV_CTL_IOCTL_HWDEP_NEXT_DEVICE = _IOWR('U', 0x20, int),
> SNDRV_CTL_IOCTL_HWDEP_INFO = _IOR('U', 0x21, struct snd_hwdep_info),
> SNDRV_CTL_IOCTL_PCM_NEXT_DEVICE = _IOR('U', 0x30, int),
> diff -r 08577d0b45ef include/control.h
> --- a/include/control.h Tue Jun 13 12:01:14 2006 +0200
> +++ b/include/control.h Fri Jun 16 12:57:54 2006 +0200
> @@ -30,6 +30,11 @@ typedef int (snd_kcontrol_info_t) (struc
> typedef int (snd_kcontrol_info_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_info * uinfo);
> typedef int (snd_kcontrol_get_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
> typedef int (snd_kcontrol_put_t) (struct snd_kcontrol * kcontrol, struct snd_ctl_elem_value * ucontrol);
> +typedef int (snd_kcontrol_tlv_rw_t)(struct snd_kcontrol *kcontrol,
> + int write_flag,
> + unsigned int size,
> + unsigned int __user *tlv);
> +
>
> struct snd_kcontrol_new {
> snd_ctl_elem_iface_t iface; /* interface identifier */
> @@ -42,7 +47,10 @@ struct snd_kcontrol_new {
> snd_kcontrol_info_t *info;
> snd_kcontrol_get_t *get;
> snd_kcontrol_put_t *put;
> - unsigned int *tlv;
> + union {
> + snd_kcontrol_tlv_rw_t *c;
> + unsigned int *p;
> + } tlv;
> unsigned long private_value;
> };
>
> @@ -59,7 +67,11 @@ struct snd_kcontrol {
> snd_kcontrol_info_t *info;
> snd_kcontrol_get_t *get;
> snd_kcontrol_put_t *put;
> - unsigned int *tlv;
> + snd_kcontrol_tlv_rw_t *tlv_rw;
> + union {
> + snd_kcontrol_tlv_rw_t *c;
> + unsigned int *p;
> + } tlv;
> unsigned long private_value;
> void *private_data;
> void (*private_free)(struct snd_kcontrol *kcontrol);
>
> -----
> Jaroslav Kysela <perex@xxxxxxx>
> Linux Kernel Sound Maintainer
> ALSA Project, SUSE Labs
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.sourceforge.net/lists/listinfo/alsa-devel
