ASoC: topology: Add support for TLV bytes control

Commit Message

Lin, Mengdong Aug. 12, 2015, 2:10 a.m. UTC

From: Mengdong Lin <[e-mail]>

Allow vendor drivers to define bespoke bytes ext handlers and IDs for
TLV bytes controls. And the topology core will bind these handlers by
matching IDs defined by the vendor driver and user space topology
data file.

Signed-off-by: Mengdong Lin <[e-mail]>

Comments

Lin, Mengdong Aug. 13, 2015, 1:41 a.m. UTC | #1

Hi Mark,

If convenient, would you please review this patch before my another series:
"[PATCH v2 00/10] ASoC: support adding PCM dynamically from topology"?

This patch is more like a fix for TLV bytes control in topology kernel.
The user space part has been merged in alsa-lib now. We want to avoid mismatch between the kernel and user space.

That v2 series for dynamic PCM support in topology is actually still in RFC phase. We hope to get your feedback to adjust the direction and go ahead.

Thanks
Mengdong  

> -----Original Message-----
> From: Lin, Mengdong
> Sent: Wednesday, August 12, 2015 10:11 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail]; Girdwood, Liam R; Lin, Mengdong
> Subject: [PATCH] ASoC: topology: Add support for TLV bytes control
> 
> From: Mengdong Lin <[e-mail]>
> 
> Allow vendor drivers to define bespoke bytes ext handlers and IDs for TLV
> bytes controls. And the topology core will bind these handlers by matching
> IDs defined by the vendor driver and user space topology data file.
> 
> Signed-off-by: Mengdong Lin <[e-mail]>
> 
> diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
> index 865a141..3e412c6 100644
> --- a/include/sound/soc-topology.h
> +++ b/include/sound/soc-topology.h
> @@ -89,6 +89,13 @@ struct snd_soc_tplg_kcontrol_ops {
>  		struct snd_ctl_elem_info *uinfo);
>  };
> 
> +/* Bytes ext operations, for TLV byte controls */ struct
> +snd_soc_tplg_bytes_ext_ops {
> +	u32 id;
> +	int (*get)(unsigned int __user *bytes, unsigned int size);
> +	int (*put)(const unsigned int __user *bytes, unsigned int size); };
> +
>  /*
>   * DAPM widget event handlers - used to map handlers onto widgets.
>   */
> @@ -139,6 +146,10 @@ struct snd_soc_tplg_ops {
>  	/* bespoke kcontrol handlers available for binding */
>  	const struct snd_soc_tplg_kcontrol_ops *io_ops;
>  	int io_ops_count;
> +
> +	/* bespoke bytes ext handlers available for binding */
> +	const struct snd_soc_tplg_bytes_ext_ops *bytes_ext_ops;
> +	int bytes_ext_ops_count;
>  };
> 
>  /* gets a pointer to data from the firmware block header */ diff --git
> a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index
> 607f98b..b5a2868 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -70,6 +70,10 @@ struct soc_tplg {
>  	const struct snd_soc_tplg_kcontrol_ops *io_ops;
>  	int io_ops_count;
> 
> +	/* bespoke bytes ext handlers, for TLV bytes controls */
> +	const struct snd_soc_tplg_bytes_ext_ops *bytes_ext_ops;
> +	int bytes_ext_ops_count;
> +
>  	/* optional fw loading callbacks to component drivers */
>  	struct snd_soc_tplg_ops *ops;
>  };
> @@ -513,6 +517,15 @@ static int soc_tplg_kcontrol_bind_io(struct
> snd_soc_tplg_ctl_hdr *hdr,  {
>  	int i;
> 
> +	/* TLV bytes controls don't need get/put ops */
> +	if (k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
> +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK
> +		&& hdr->ops.info == SND_SOC_TPLG_CTL_BYTES) {
> +			k->info = snd_soc_bytes_info_ext;
> +			return 0;
> +	}
> +
>  	/* try and map standard kcontrols handler first */
>  	for (i = 0; i < num_ops; i++) {
> 
> @@ -547,6 +560,32 @@ static int soc_tplg_kcontrol_bind_io(struct
> snd_soc_tplg_ctl_hdr *hdr,
>  	return -EINVAL;
>  }
> 
> +/* bind bytes ext get/put handlers for TLV bytes controls */ static int
> +soc_tplg_bytes_ext_bind_io(struct soc_tplg *tplg,
> +	struct snd_soc_tplg_io_ops *ops_id,
> +	struct soc_bytes_ext *sbe)
> +{
> +	const struct snd_soc_tplg_bytes_ext_ops *ops = tplg->bytes_ext_ops;
> +	int num_ops = tplg->bytes_ext_ops_count;
> +	int i;
> +
> +	/* try bespoke handlers */
> +	for (i = 0; i < num_ops; i++) {
> +
> +		if (sbe->put == NULL && ops[i].id == ops_id->put)
> +			sbe->put = ops[i].put;
> +		if (sbe->get == NULL && ops[i].id == ops_id->get)
> +			sbe->get = ops[i].get;
> +	}
> +
> +	/* bespoke handlers found ? */
> +	if (sbe->put && sbe->get)
> +		return 0;
> +
> +	/* nothing to bind */
> +	return -EINVAL;
> +}
> +
>  /* bind a widgets to it's evnt handlers */  int
> snd_soc_tplg_widget_bind_event(struct snd_soc_dapm_widget *w,
>  		const struct snd_soc_tplg_widget_events *events, @@ -690,6
> +729,14 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, unsigned
> int count,
>  			continue;
>  		}
> 
> +		/* create any TLV data */
> +		soc_tplg_create_tlv(tplg, &kc, &be->hdr);
> +		/* map bytes ext io handlers for TLV bytes controls */
> +		if (kc.access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> +			soc_tplg_bytes_ext_bind_io(tplg,
> +					&be->ext_ops, sbe);
> +		}
> +
>  		/* pass control to driver for optional further init */
>  		err = soc_tplg_init_kcontrol(tplg, &kc,
>  			(struct snd_soc_tplg_ctl_hdr *)be);
> @@ -1315,6 +1362,14 @@ static struct snd_kcontrol_new
> *soc_tplg_dapm_widget_dbytes_create(
>  			continue;
>  		}
> 
> +		/* create any TLV data */
> +		soc_tplg_create_tlv(tplg, &kc[i], &be->hdr);
> +		/* map bytes ext io handlers for TLV bytes controls */
> +		if (kc[i].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
> +			soc_tplg_bytes_ext_bind_io(tplg,
> +					&be->ext_ops, sbe);
> +		}
> +
>  		/* pass control to driver for optional further init */
>  		err = soc_tplg_init_kcontrol(tplg, &kc[i],
>  			(struct snd_soc_tplg_ctl_hdr *)be);
> @@ -1736,6 +1791,8 @@ int snd_soc_tplg_component_load(struct
> snd_soc_component *comp,
>  	tplg.req_index = id;
>  	tplg.io_ops = ops->io_ops;
>  	tplg.io_ops_count = ops->io_ops_count;
> +	tplg.bytes_ext_ops = ops->bytes_ext_ops;
> +	tplg.bytes_ext_ops_count = ops->bytes_ext_ops_count;
> 
>  	return soc_tplg_load(&tplg);
>  }
> --
> 1.9.1

Mark Brown Aug. 14, 2015, 7:17 p.m. UTC | #2

On Wed, Aug 12, 2015 at 10:10:51AM +0800, [e-mail] wrote:

> Allow vendor drivers to define bespoke bytes ext handlers and IDs for
> TLV bytes controls. And the topology core will bind these handlers by
> matching IDs defined by the vendor driver and user space topology
> data file.

> @@ -513,6 +517,15 @@ static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
>  {
>  	int i;
>  
> +	/* TLV bytes controls don't need get/put ops */

AFAICT it's actually the case that we're binding them later rather than
that the ops aren't needed.  Isn't that what soc_tplg_bytes_ext_bind_io()
does?

> +	if (k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
> +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK
> +		&& hdr->ops.info == SND_SOC_TPLG_CTL_BYTES) {
> +			k->info = snd_soc_bytes_info_ext;
> +			return 0;
> +	}
> +

I'm confused.  What's special about TLV controls compared to other
controls, why are the existing bespoke handlers which get bound in this
function not sufficient and why don't we bind the TLV bespoke handlers
here?

Having two different classes of bespoke handlers like this both called
bespoke handlers seems like it's going to lead to problems - either
bringing the interfaces closer together or naming them in a way that
clarifies their meaning seems better.  Instead of just calling them
"bespoke handlers" we should probably just call them handlers for
whatever kind of control they're for and probably bind them first rather
than second so we're doing more specific to less specific handling.
That way drivers can also have specific handling for things the topology
data didn't anticipate they'd need it for which seems like it might come
in handy.

Lin, Mengdong Aug. 15, 2015, 3:04 p.m. UTC | #3

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Saturday, August 15, 2015 3:18 AM
> To: Lin, Mengdong
> Cc: [e-mail]; [e-mail]; Girdwood, Liam R
> Subject: Re: [PATCH] ASoC: topology: Add support for TLV bytes control
> 
> On Wed, Aug 12, 2015 at 10:10:51AM +0800, [e-mail] wrote:
> 
> > Allow vendor drivers to define bespoke bytes ext handlers and IDs for
> > TLV bytes controls. And the topology core will bind these handlers by
> > matching IDs defined by the vendor driver and user space topology data
> > file.
> 
> > @@ -513,6 +517,15 @@ static int soc_tplg_kcontrol_bind_io(struct
> > snd_soc_tplg_ctl_hdr *hdr,  {
> >  	int i;
> >
> > +	/* TLV bytes controls don't need get/put ops */
> 
> AFAICT it's actually the case that we're binding them later rather than that
> the ops aren't needed.  Isn't that what soc_tplg_bytes_ext_bind_io() does?
> 
> > +	if (k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
> > +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
> > +		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK
> > +		&& hdr->ops.info == SND_SOC_TPLG_CTL_BYTES) {
> > +			k->info = snd_soc_bytes_info_ext;
> > +			return 0;
> > +	}
> > +
> 
> I'm confused.  What's special about TLV controls compared to other controls,
> why are the existing bespoke handlers which get bound in this function not
> sufficient and why don't we bind the TLV bespoke handlers here?

Okay. I'll bind the TLV bespoke handlers here, and remove soc_tplg_bytes_ext_bind_io().
So all ops binding will be handled in a single function soc_tplg_kcontrol_bind_io().

> Having two different classes of bespoke handlers like this both called bespoke
> handlers seems like it's going to lead to problems - either bringing the
> interfaces closer together or naming them in a way that clarifies their
> meaning seems better.  Instead of just calling them "bespoke handlers" we
> should probably just call them handlers for whatever kind of control they're
> for and probably bind them first rather than second so we're doing more
> specific to less specific handling.
> That way drivers can also have specific handling for things the topology data
> didn't anticipate they'd need it for which seems like it might come in handy.

Are you suggesting that we bind bespoke handlers at first and then the standard handlers?
Since the bespoke handlers means more vendor-specific handling that need to be taken care at first.

We add comments to explain these two different classes of bespoke handlers.
struct snd_soc_tplg_ops {
...
	/* bespoke kcontrol handlers available for binding */
	const struct snd_soc_tplg_kcontrol_ops *io_ops;
	int io_ops_count;

	/* bespoke bytes ext handlers available for binding */
	const struct snd_soc_tplg_bytes_ext_ops *bytes_ext_ops;
	int bytes_ext_ops_count;
};

Thanks
Mengdong

Mark Brown Aug. 15, 2015, 4:10 p.m. UTC | #4

On Sat, Aug 15, 2015 at 03:04:53PM +0000, Lin, Mengdong wrote:
> > -----Original Message-----
> > From: Mark Brown [mailto:[e-mail]]

Please fix your mail client to word wrap within paragraphs, your
messages are quite hard to read.  It also seems to be rewriting quoted
material :/

> > Having two different classes of bespoke handlers like this both called bespoke
> > handlers seems like it's going to lead to problems - either bringing the
> > interfaces closer together or naming them in a way that clarifies their
> > meaning seems better.  Instead of just calling them "bespoke handlers" we
> > should probably just call them handlers for whatever kind of control they're
> > for and probably bind them first rather than second so we're doing more
> > specific to less specific handling.
> > That way drivers can also have specific handling for things the topology data
> > didn't anticipate they'd need it for which seems like it might come in handy.

> Are you suggesting that we bind bespoke handlers at first and then the
> standard handlers?  Since the bespoke handlers means more
> vendor-specific handling that need to be taken care at first.

Yes, that's what I'm suggesting.  Why would this mean any additional
work?  Either things have custom handlers or they don't...

Lin, Mengdong Aug. 17, 2015, 1:33 a.m. UTC | #5

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Sunday, August 16, 2015 12:10 AM

> On Sat, Aug 15, 2015 at 03:04:53PM +0000, Lin, Mengdong wrote:
> > > -----Original Message-----
> > > From: Mark Brown [mailto:[e-mail]]
> 
> Please fix your mail client to word wrap within paragraphs, your messages are
> quite hard to read.  It also seems to be rewriting quoted material :/

Okay. Sorry for the inconvenience.

> > > Having two different classes of bespoke handlers like this both
> > > called bespoke handlers seems like it's going to lead to problems -
> > > either bringing the interfaces closer together or naming them in a
> > > way that clarifies their meaning seems better.  Instead of just
> > > calling them "bespoke handlers" we should probably just call them
> > > handlers for whatever kind of control they're for and probably bind
> > > them first rather than second so we're doing more specific to less specific
> handling.
> > > That way drivers can also have specific handling for things the
> > > topology data didn't anticipate they'd need it for which seems like it
> might come in handy.
> 
> > Are you suggesting that we bind bespoke handlers at first and then the
> > standard handlers?  Since the bespoke handlers means more
> > vendor-specific handling that need to be taken care at first.
> 
> Yes, that's what I'm suggesting.  Why would this mean any additional work?
> Either things have custom handlers or they don't...

Got it. Not additional work. I'll revise the patch.

Thanks
Mengdong

Patch

diff --git a/include/sound/soc-topology.h b/include/sound/soc-topology.h
index 865a141..3e412c6 100644
--- a/include/sound/soc-topology.h
+++ b/include/sound/soc-topology.h
@@ -89,6 +89,13 @@  struct snd_soc_tplg_kcontrol_ops {
 		struct snd_ctl_elem_info *uinfo);
 };
 
+/* Bytes ext operations, for TLV byte controls */
+struct snd_soc_tplg_bytes_ext_ops {
+	u32 id;
+	int (*get)(unsigned int __user *bytes, unsigned int size);
+	int (*put)(const unsigned int __user *bytes, unsigned int size);
+};
+
 /*
  * DAPM widget event handlers - used to map handlers onto widgets.
  */
@@ -139,6 +146,10 @@  struct snd_soc_tplg_ops {
 	/* bespoke kcontrol handlers available for binding */
 	const struct snd_soc_tplg_kcontrol_ops *io_ops;
 	int io_ops_count;
+
+	/* bespoke bytes ext handlers available for binding */
+	const struct snd_soc_tplg_bytes_ext_ops *bytes_ext_ops;
+	int bytes_ext_ops_count;
 };
 
 /* gets a pointer to data from the firmware block header */
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 607f98b..b5a2868 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -70,6 +70,10 @@  struct soc_tplg {
 	const struct snd_soc_tplg_kcontrol_ops *io_ops;
 	int io_ops_count;
 
+	/* bespoke bytes ext handlers, for TLV bytes controls */
+	const struct snd_soc_tplg_bytes_ext_ops *bytes_ext_ops;
+	int bytes_ext_ops_count;
+
 	/* optional fw loading callbacks to component drivers */
 	struct snd_soc_tplg_ops *ops;
 };
@@ -513,6 +517,15 @@  static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
 {
 	int i;
 
+	/* TLV bytes controls don't need get/put ops */
+	if (k->iface & SNDRV_CTL_ELEM_IFACE_MIXER
+		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_READWRITE
+		&& k->access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK
+		&& hdr->ops.info == SND_SOC_TPLG_CTL_BYTES) {
+			k->info = snd_soc_bytes_info_ext;
+			return 0;
+	}
+
 	/* try and map standard kcontrols handler first */
 	for (i = 0; i < num_ops; i++) {
 
@@ -547,6 +560,32 @@  static int soc_tplg_kcontrol_bind_io(struct snd_soc_tplg_ctl_hdr *hdr,
 	return -EINVAL;
 }
 
+/* bind bytes ext get/put handlers for TLV bytes controls */
+static int soc_tplg_bytes_ext_bind_io(struct soc_tplg *tplg,
+	struct snd_soc_tplg_io_ops *ops_id,
+	struct soc_bytes_ext *sbe)
+{
+	const struct snd_soc_tplg_bytes_ext_ops *ops = tplg->bytes_ext_ops;
+	int num_ops = tplg->bytes_ext_ops_count;
+	int i;
+
+	/* try bespoke handlers */
+	for (i = 0; i < num_ops; i++) {
+
+		if (sbe->put == NULL && ops[i].id == ops_id->put)
+			sbe->put = ops[i].put;
+		if (sbe->get == NULL && ops[i].id == ops_id->get)
+			sbe->get = ops[i].get;
+	}
+
+	/* bespoke handlers found ? */
+	if (sbe->put && sbe->get)
+		return 0;
+
+	/* nothing to bind */
+	return -EINVAL;
+}
+
 /* bind a widgets to it's evnt handlers */
 int snd_soc_tplg_widget_bind_event(struct snd_soc_dapm_widget *w,
 		const struct snd_soc_tplg_widget_events *events,
@@ -690,6 +729,14 @@  static int soc_tplg_dbytes_create(struct soc_tplg *tplg, unsigned int count,
 			continue;
 		}
 
+		/* create any TLV data */
+		soc_tplg_create_tlv(tplg, &kc, &be->hdr);
+		/* map bytes ext io handlers for TLV bytes controls */
+		if (kc.access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+			soc_tplg_bytes_ext_bind_io(tplg,
+					&be->ext_ops, sbe);
+		}
+
 		/* pass control to driver for optional further init */
 		err = soc_tplg_init_kcontrol(tplg, &kc,
 			(struct snd_soc_tplg_ctl_hdr *)be);
@@ -1315,6 +1362,14 @@  static struct snd_kcontrol_new *soc_tplg_dapm_widget_dbytes_create(
 			continue;
 		}
 
+		/* create any TLV data */
+		soc_tplg_create_tlv(tplg, &kc[i], &be->hdr);
+		/* map bytes ext io handlers for TLV bytes controls */
+		if (kc[i].access & SNDRV_CTL_ELEM_ACCESS_TLV_CALLBACK) {
+			soc_tplg_bytes_ext_bind_io(tplg,
+					&be->ext_ops, sbe);
+		}
+
 		/* pass control to driver for optional further init */
 		err = soc_tplg_init_kcontrol(tplg, &kc[i],
 			(struct snd_soc_tplg_ctl_hdr *)be);
@@ -1736,6 +1791,8 @@  int snd_soc_tplg_component_load(struct snd_soc_component *comp,
 	tplg.req_index = id;
 	tplg.io_ops = ops->io_ops;
 	tplg.io_ops_count = ops->io_ops_count;
+	tplg.bytes_ext_ops = ops->bytes_ext_ops;
+	tplg.bytes_ext_ops_count = ops->bytes_ext_ops_count;
 
 	return soc_tplg_load(&tplg);
 }