[RFC,3/3] ucm: Execute sequence of component devices
diff mbox

Message ID b610787835a8530f0b3105321e8f15661fc55b0b.1479195801.git.mengdong.lin@linux.intel.com
State New
Headers show

Commit Message

mengdong.lin@linux.intel.com Nov. 15, 2016, 8:02 a.m. UTC
From: Mengdong Lin <mengdong.lin@linux.intel.com>

A machine device's sequence can enable or disable a component device. So
when executing a machine device's sequence, the enable or disable sequence
of its component devices will also be excecuted.

Add a parameter 'cdev_in' to function execute_sequence(). This function
is used to execute a sequence of either machine or component devices.
Since the sequence of a component device does not define the card device,
when executing its sequence, this parameter can pass the cdev defined by
the sequence of its parent, the machine device. When executing sequence of
machine devices, this parameter should be set to NULL.

Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>

Comments

Liam Girdwood Nov. 15, 2016, 8:49 a.m. UTC | #1
On Tue, 2016-11-15 at 16:02 +0800, mengdong.lin@linux.intel.com wrote:
> From: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> A machine device's sequence can enable or disable a component device. So
> when executing a machine device's sequence, the enable or disable sequence
> of its component devices will also be excecuted.
> 
> Add a parameter 'cdev_in' to function execute_sequence(). This function
> is used to execute a sequence of either machine or component devices.
> Since the sequence of a component device does not define the card device,
> when executing its sequence, this parameter can pass the cdev defined by
> the sequence of its parent, the machine device. When executing sequence of
> machine devices, this parameter should be set to NULL.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@linux.intel.com>
> 
> diff --git a/src/ucm/main.c b/src/ucm/main.c
> index 8cc9208..a852cf4 100644
> --- a/src/ucm/main.c
> +++ b/src/ucm/main.c
> @@ -48,6 +48,13 @@ static int get_value3(char **value,
>  		      struct list_head *value_list2,
>  		      struct list_head *value_list3);
>  
> +static int execute_component_seq(snd_use_case_mgr_t *uc_mgr,
> +				 struct component_sequence *cmpt_seq,
> +				 struct list_head *value_list1,
> +				 struct list_head *value_list2,
> +				 struct list_head *value_list3,
> +				 char *cdev_in);
> +
>  static int check_identifier(const char *identifier, const char *prefix)
>  {
>  	int len;
> @@ -341,13 +348,22 @@ static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
>   * \brief Execute the sequence
>   * \param uc_mgr Use case manager
>   * \param seq Sequence
> + * \param cdev_in input cdev,  parenet's cdev for a component device,
> + *				or NULL for a machine device.
>   * \return zero on success, otherwise a negative error code
> + *
> + * A machine device's sequence can enable or disable a component device.
> + * But the enable/disable sequence of a component device does not define
> + * cdev, the card device. So when executing a component device's sequence,
> + * we need to pass the cdev defined by the sequence of its parent, the
> + * machine device. And for machine device, cdev should be set to NULL.
>   */
>  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  			    struct list_head *seq,
>  			    struct list_head *value_list1,
>  			    struct list_head *value_list2,
> -			    struct list_head *value_list3)
> +			    struct list_head *value_list3,
> +			    char *cdev_in)

Could the current cdev be embedded in uc_mgr ? 

>  {
>  	struct list_head *pos;
>  	struct sequence_element *s;
> @@ -366,7 +382,16 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  		case SEQUENCE_ELEMENT_TYPE_CSET:
>  		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
>  		case SEQUENCE_ELEMENT_TYPE_CSET_TLV:
> -			if (cdev == NULL) {
> +			if (cdev == NULL && cdev_in) {
> +				/* Sequence of a component device, should use
> +				 * the input cdev defined by sequence of its
> +				 * parent, the machine device.
> +				 */
> +				cdev = strdup(cdev_in);
> +				if (cdev == NULL)
> +					goto __fail_nomem;
> +
> +			} else if (cdev == NULL) {
>  				char *playback_ctl = NULL;
>  				char *capture_ctl = NULL;
>  
> @@ -405,7 +430,9 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  					free(capture_ctl);
>  				} else
>  					cdev = capture_ctl;
> +
>  			}
> +

Looks like this formatting change was added by mistake ?

>  			if (ctl == NULL) {
>  				err = open_ctl(uc_mgr, &ctl, cdev);
>  				if (err < 0) {
> @@ -427,6 +454,19 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  			if (err < 0)
>  				goto __fail;
>  			break;
> +		case SEQUENCE_ELEMENT_TYPE_CMPT_SEQ:
> +			/* Execute enable or disable sequence of a component
> +			 * device. Pass the cdev defined by the machine device.
> +			 */
> +			err = execute_component_seq(uc_mgr,
> +							&s->data.cmpt_seq,
> +							value_list1,
> +							value_list2,
> +							value_list3,
> +							cdev);
> +			if (err < 0)
> +				goto __fail;
> +			break;
>  		default:
>  			uc_error("unknown sequence command %i", s->type);
>  			break;
> @@ -442,6 +482,39 @@ static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
>  
>  }
>  
> +/* Execute enable or disable sequence of a component device.
> + *
> + * For a component device (a codec or embedded DSP), its sequence doesn't
> + * specify the sound card device 'cdev', because a component can be reused
> + * by different sound cards (machines). So when executing its sequence, a
> + * parameter 'cdev_in' is used to pass cdev defined by the sequence of its
> + * parent, the machine device.
> + */
> +static int execute_component_seq(snd_use_case_mgr_t *uc_mgr,
> +				 struct component_sequence *cmpt_seq,
> +				 struct list_head *value_list1,
> +				 struct list_head *value_list2,
> +				 struct list_head *value_list3,
> +				 char *cdev_in)
> +{
> +	struct use_case_device *device = cmpt_seq->device;
> +	struct list_head *seq;
> +	int err;
> +
> +
> +	if (cmpt_seq->enable)
> +		seq = &device->enable_list;
> +	else
> +		seq = &device->disable_list;

What happens here if there is only an enable sequence and no disable
sequence ? i.e. will seq be NULL ?

> +
> +	err = execute_sequence(uc_mgr, seq,
> +			       &device->value_list,
> +			       &uc_mgr->active_verb->value_list,
> +			       &uc_mgr->value_list,
> +			       cdev_in);
> +	return err;
> +}
> +
>  /**
>   * \brief Import master config and execute the default sequence
>   * \param uc_mgr Use case manager
> @@ -455,7 +528,7 @@ static int import_master_config(snd_use_case_mgr_t *uc_mgr)
>  	if (err < 0)
>  		return err;
>  	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
> -			       &uc_mgr->value_list, NULL, NULL);
> +			       &uc_mgr->value_list, NULL, NULL, NULL);
>  	if (err < 0)
>  		uc_error("Unable to execute default sequence");
>  	return err;
> @@ -761,7 +834,7 @@ static int set_verb(snd_use_case_mgr_t *uc_mgr,
>  	err = execute_sequence(uc_mgr, seq,
>  			       &verb->value_list,
>  			       &uc_mgr->value_list,
> -			       NULL);
> +			       NULL, NULL);
>  	if (enable && err >= 0)
>  		uc_mgr->active_verb = verb;
>  	return err;
> @@ -792,7 +865,8 @@ static int set_modifier(snd_use_case_mgr_t *uc_mgr,
>  	err = execute_sequence(uc_mgr, seq,
>  			       &modifier->value_list,
>  			       &uc_mgr->active_verb->value_list,
> -			       &uc_mgr->value_list);
> +			       &uc_mgr->value_list,
> +			       NULL);
>  	if (enable && err >= 0) {
>  		list_add_tail(&modifier->active_list, &uc_mgr->active_modifiers);
>  	} else if (!enable) {
> @@ -826,7 +900,8 @@ static int set_device(snd_use_case_mgr_t *uc_mgr,
>  	err = execute_sequence(uc_mgr, seq,
>  			       &device->value_list,
>  			       &uc_mgr->active_verb->value_list,
> -			       &uc_mgr->value_list);
> +			       &uc_mgr->value_list,
> +			       NULL);
>  	if (enable && err >= 0) {
>  		list_add_tail(&device->active_list, &uc_mgr->active_devices);
>  	} else if (!enable) {
> @@ -953,7 +1028,7 @@ static int dismantle_use_case(snd_use_case_mgr_t *uc_mgr)
>  	uc_mgr->active_verb = NULL;
>  
>  	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
> -			       &uc_mgr->value_list, NULL, NULL);
> +			       &uc_mgr->value_list, NULL, NULL, NULL);
>  	
>  	return err;
>  }
> @@ -969,7 +1044,7 @@ int snd_use_case_mgr_reset(snd_use_case_mgr_t *uc_mgr)
>  
>  	pthread_mutex_lock(&uc_mgr->mutex);
>  	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
> -			       &uc_mgr->value_list, NULL, NULL);
> +			       &uc_mgr->value_list, NULL, NULL, NULL);
>  	INIT_LIST_HEAD(&uc_mgr->active_modifiers);
>  	INIT_LIST_HEAD(&uc_mgr->active_devices);
>  	uc_mgr->active_verb = NULL;
> @@ -1578,6 +1653,7 @@ static int handle_transition_verb(snd_use_case_mgr_t *uc_mgr,
>                          err = execute_sequence(uc_mgr, &trans->transition_list,
>  					       &uc_mgr->active_verb->value_list,
>  					       &uc_mgr->value_list,
> +					       NULL,
>  					       NULL);
>                          if (err >= 0)
>                                  return 1;
> @@ -1689,7 +1765,8 @@ static int switch_device(snd_use_case_mgr_t *uc_mgr,
>                          err = execute_sequence(uc_mgr, &trans->transition_list,
>  					       &xold->value_list,
>  					       &uc_mgr->active_verb->value_list,
> -					       &uc_mgr->value_list);
> +					       &uc_mgr->value_list,
> +					       NULL);
>                          if (err >= 0) {
>                                  list_del(&xold->active_list);
>                                  list_add_tail(&xnew->active_list, &uc_mgr->active_devices);
> @@ -1741,7 +1818,8 @@ static int switch_modifier(snd_use_case_mgr_t *uc_mgr,
>                          err = execute_sequence(uc_mgr, &trans->transition_list,
>  					       &xold->value_list,
>  					       &uc_mgr->active_verb->value_list,
> -					       &uc_mgr->value_list);
> +					       &uc_mgr->value_list,
> +					       NULL);
>                          if (err >= 0) {
>                                  list_del(&xold->active_list);
>                                  list_add_tail(&xnew->active_list, &uc_mgr->active_modifiers);
Lin, Mengdong Nov. 16, 2016, 7:36 a.m. UTC | #2
> -----Original Message-----
> From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com]
> Sent: Tuesday, November 15, 2016 4:50 PM

> > @@ -341,13 +348,22 @@ static int execute_cset(snd_ctl_t *ctl, const char
> *cset, unsigned int type)
> >   * \brief Execute the sequence
> >   * \param uc_mgr Use case manager
> >   * \param seq Sequence
> > + * \param cdev_in input cdev,  parenet's cdev for a component device,
> > + *				or NULL for a machine device.
> >   * \return zero on success, otherwise a negative error code
> > + *
> > + * A machine device's sequence can enable or disable a component device.
> > + * But the enable/disable sequence of a component device does not
> > + define
> > + * cdev, the card device. So when executing a component device's
> > + sequence,
> > + * we need to pass the cdev defined by the sequence of its parent,
> > + the
> > + * machine device. And for machine device, cdev should be set to NULL.
> >   */
> >  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
> >  			    struct list_head *seq,
> >  			    struct list_head *value_list1,
> >  			    struct list_head *value_list2,
> > -			    struct list_head *value_list3)
> > +			    struct list_head *value_list3,
> > +			    char *cdev_in)
> 
> Could the current cdev be embedded in uc_mgr ?

Yes, it could and there will be less code change. 
Actually I found the machine devices always set the same cdev name in their sequences.

> > @@ -366,7 +382,16 @@ static int execute_sequence(snd_use_case_mgr_t
> *uc_mgr,
> >  		case SEQUENCE_ELEMENT_TYPE_CSET:
> >  		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
> >  		case SEQUENCE_ELEMENT_TYPE_CSET_TLV:
> > -			if (cdev == NULL) {
> > +			if (cdev == NULL && cdev_in) {
> > +				/* Sequence of a component device, should
> use
> > +				 * the input cdev defined by sequence of its
> > +				 * parent, the machine device.
> > +				 */
> > +				cdev = strdup(cdev_in);
> > +				if (cdev == NULL)
> > +					goto __fail_nomem;
> > +
> > +			} else if (cdev == NULL) {
> >  				char *playback_ctl = NULL;
> >  				char *capture_ctl = NULL;
> >
> > @@ -405,7 +430,9 @@ static int execute_sequence(snd_use_case_mgr_t
> *uc_mgr,
> >  					free(capture_ctl);
> >  				} else
> >  					cdev = capture_ctl;
> > +
> >  			}
> > +
> 
> Looks like this formatting change was added by mistake ?

Yes, it was added by mistake. I'll remove it.

> > +/* Execute enable or disable sequence of a component device.
> > + *
> > + * For a component device (a codec or embedded DSP), its sequence
> > +doesn't
> > + * specify the sound card device 'cdev', because a component can be
> > +reused
> > + * by different sound cards (machines). So when executing its
> > +sequence, a
> > + * parameter 'cdev_in' is used to pass cdev defined by the sequence
> > +of its
> > + * parent, the machine device.
> > + */
> > +static int execute_component_seq(snd_use_case_mgr_t *uc_mgr,
> > +				 struct component_sequence *cmpt_seq,
> > +				 struct list_head *value_list1,
> > +				 struct list_head *value_list2,
> > +				 struct list_head *value_list3,
> > +				 char *cdev_in)
> > +{
> > +	struct use_case_device *device = cmpt_seq->device;
> > +	struct list_head *seq;
> > +	int err;
> > +
> > +
> > +	if (cmpt_seq->enable)
> > +		seq = &device->enable_list;
> > +	else
> > +		seq = &device->disable_list;
> 
> What happens here if there is only an enable sequence and no disable
> sequence ? i.e. will seq be NULL ?

The device's enable_list or disable_list will be empty, assured by the UCM parser.
And so 'seq' will be an empty list and the following execute_sequence() will exit without doing anything.
> 
> > +
> > +	err = execute_sequence(uc_mgr, seq,
> > +			       &device->value_list,
> > +			       &uc_mgr->active_verb->value_list,
> > +			       &uc_mgr->value_list,
> > +			       cdev_in);
> > +	return err;
> > +}
> > +

Thanks
Mengdong

Patch
diff mbox

diff --git a/src/ucm/main.c b/src/ucm/main.c
index 8cc9208..a852cf4 100644
--- a/src/ucm/main.c
+++ b/src/ucm/main.c
@@ -48,6 +48,13 @@  static int get_value3(char **value,
 		      struct list_head *value_list2,
 		      struct list_head *value_list3);
 
+static int execute_component_seq(snd_use_case_mgr_t *uc_mgr,
+				 struct component_sequence *cmpt_seq,
+				 struct list_head *value_list1,
+				 struct list_head *value_list2,
+				 struct list_head *value_list3,
+				 char *cdev_in);
+
 static int check_identifier(const char *identifier, const char *prefix)
 {
 	int len;
@@ -341,13 +348,22 @@  static int execute_cset(snd_ctl_t *ctl, const char *cset, unsigned int type)
  * \brief Execute the sequence
  * \param uc_mgr Use case manager
  * \param seq Sequence
+ * \param cdev_in input cdev,  parenet's cdev for a component device,
+ *				or NULL for a machine device.
  * \return zero on success, otherwise a negative error code
+ *
+ * A machine device's sequence can enable or disable a component device.
+ * But the enable/disable sequence of a component device does not define
+ * cdev, the card device. So when executing a component device's sequence,
+ * we need to pass the cdev defined by the sequence of its parent, the
+ * machine device. And for machine device, cdev should be set to NULL.
  */
 static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 			    struct list_head *seq,
 			    struct list_head *value_list1,
 			    struct list_head *value_list2,
-			    struct list_head *value_list3)
+			    struct list_head *value_list3,
+			    char *cdev_in)
 {
 	struct list_head *pos;
 	struct sequence_element *s;
@@ -366,7 +382,16 @@  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 		case SEQUENCE_ELEMENT_TYPE_CSET:
 		case SEQUENCE_ELEMENT_TYPE_CSET_BIN_FILE:
 		case SEQUENCE_ELEMENT_TYPE_CSET_TLV:
-			if (cdev == NULL) {
+			if (cdev == NULL && cdev_in) {
+				/* Sequence of a component device, should use
+				 * the input cdev defined by sequence of its
+				 * parent, the machine device.
+				 */
+				cdev = strdup(cdev_in);
+				if (cdev == NULL)
+					goto __fail_nomem;
+
+			} else if (cdev == NULL) {
 				char *playback_ctl = NULL;
 				char *capture_ctl = NULL;
 
@@ -405,7 +430,9 @@  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 					free(capture_ctl);
 				} else
 					cdev = capture_ctl;
+
 			}
+
 			if (ctl == NULL) {
 				err = open_ctl(uc_mgr, &ctl, cdev);
 				if (err < 0) {
@@ -427,6 +454,19 @@  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 			if (err < 0)
 				goto __fail;
 			break;
+		case SEQUENCE_ELEMENT_TYPE_CMPT_SEQ:
+			/* Execute enable or disable sequence of a component
+			 * device. Pass the cdev defined by the machine device.
+			 */
+			err = execute_component_seq(uc_mgr,
+							&s->data.cmpt_seq,
+							value_list1,
+							value_list2,
+							value_list3,
+							cdev);
+			if (err < 0)
+				goto __fail;
+			break;
 		default:
 			uc_error("unknown sequence command %i", s->type);
 			break;
@@ -442,6 +482,39 @@  static int execute_sequence(snd_use_case_mgr_t *uc_mgr,
 
 }
 
+/* Execute enable or disable sequence of a component device.
+ *
+ * For a component device (a codec or embedded DSP), its sequence doesn't
+ * specify the sound card device 'cdev', because a component can be reused
+ * by different sound cards (machines). So when executing its sequence, a
+ * parameter 'cdev_in' is used to pass cdev defined by the sequence of its
+ * parent, the machine device.
+ */
+static int execute_component_seq(snd_use_case_mgr_t *uc_mgr,
+				 struct component_sequence *cmpt_seq,
+				 struct list_head *value_list1,
+				 struct list_head *value_list2,
+				 struct list_head *value_list3,
+				 char *cdev_in)
+{
+	struct use_case_device *device = cmpt_seq->device;
+	struct list_head *seq;
+	int err;
+
+
+	if (cmpt_seq->enable)
+		seq = &device->enable_list;
+	else
+		seq = &device->disable_list;
+
+	err = execute_sequence(uc_mgr, seq,
+			       &device->value_list,
+			       &uc_mgr->active_verb->value_list,
+			       &uc_mgr->value_list,
+			       cdev_in);
+	return err;
+}
+
 /**
  * \brief Import master config and execute the default sequence
  * \param uc_mgr Use case manager
@@ -455,7 +528,7 @@  static int import_master_config(snd_use_case_mgr_t *uc_mgr)
 	if (err < 0)
 		return err;
 	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
-			       &uc_mgr->value_list, NULL, NULL);
+			       &uc_mgr->value_list, NULL, NULL, NULL);
 	if (err < 0)
 		uc_error("Unable to execute default sequence");
 	return err;
@@ -761,7 +834,7 @@  static int set_verb(snd_use_case_mgr_t *uc_mgr,
 	err = execute_sequence(uc_mgr, seq,
 			       &verb->value_list,
 			       &uc_mgr->value_list,
-			       NULL);
+			       NULL, NULL);
 	if (enable && err >= 0)
 		uc_mgr->active_verb = verb;
 	return err;
@@ -792,7 +865,8 @@  static int set_modifier(snd_use_case_mgr_t *uc_mgr,
 	err = execute_sequence(uc_mgr, seq,
 			       &modifier->value_list,
 			       &uc_mgr->active_verb->value_list,
-			       &uc_mgr->value_list);
+			       &uc_mgr->value_list,
+			       NULL);
 	if (enable && err >= 0) {
 		list_add_tail(&modifier->active_list, &uc_mgr->active_modifiers);
 	} else if (!enable) {
@@ -826,7 +900,8 @@  static int set_device(snd_use_case_mgr_t *uc_mgr,
 	err = execute_sequence(uc_mgr, seq,
 			       &device->value_list,
 			       &uc_mgr->active_verb->value_list,
-			       &uc_mgr->value_list);
+			       &uc_mgr->value_list,
+			       NULL);
 	if (enable && err >= 0) {
 		list_add_tail(&device->active_list, &uc_mgr->active_devices);
 	} else if (!enable) {
@@ -953,7 +1028,7 @@  static int dismantle_use_case(snd_use_case_mgr_t *uc_mgr)
 	uc_mgr->active_verb = NULL;
 
 	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
-			       &uc_mgr->value_list, NULL, NULL);
+			       &uc_mgr->value_list, NULL, NULL, NULL);
 	
 	return err;
 }
@@ -969,7 +1044,7 @@  int snd_use_case_mgr_reset(snd_use_case_mgr_t *uc_mgr)
 
 	pthread_mutex_lock(&uc_mgr->mutex);
 	err = execute_sequence(uc_mgr, &uc_mgr->default_list,
-			       &uc_mgr->value_list, NULL, NULL);
+			       &uc_mgr->value_list, NULL, NULL, NULL);
 	INIT_LIST_HEAD(&uc_mgr->active_modifiers);
 	INIT_LIST_HEAD(&uc_mgr->active_devices);
 	uc_mgr->active_verb = NULL;
@@ -1578,6 +1653,7 @@  static int handle_transition_verb(snd_use_case_mgr_t *uc_mgr,
                         err = execute_sequence(uc_mgr, &trans->transition_list,
 					       &uc_mgr->active_verb->value_list,
 					       &uc_mgr->value_list,
+					       NULL,
 					       NULL);
                         if (err >= 0)
                                 return 1;
@@ -1689,7 +1765,8 @@  static int switch_device(snd_use_case_mgr_t *uc_mgr,
                         err = execute_sequence(uc_mgr, &trans->transition_list,
 					       &xold->value_list,
 					       &uc_mgr->active_verb->value_list,
-					       &uc_mgr->value_list);
+					       &uc_mgr->value_list,
+					       NULL);
                         if (err >= 0) {
                                 list_del(&xold->active_list);
                                 list_add_tail(&xnew->active_list, &uc_mgr->active_devices);
@@ -1741,7 +1818,8 @@  static int switch_modifier(snd_use_case_mgr_t *uc_mgr,
                         err = execute_sequence(uc_mgr, &trans->transition_list,
 					       &xold->value_list,
 					       &uc_mgr->active_verb->value_list,
-					       &uc_mgr->value_list);
+					       &uc_mgr->value_list,
+					       NULL);
                         if (err >= 0) {
                                 list_del(&xold->active_list);
                                 list_add_tail(&xnew->active_list, &uc_mgr->active_modifiers);