diff mbox series

[v3] usb: gadget: f_uac2: uevent changes for uac2

Message ID 20230829092132.1940-1-quic_akakum@quicinc.com
State New
Headers show
Series [v3] usb: gadget: f_uac2: uevent changes for uac2 | expand

Commit Message

AKASH KUMAR Aug. 29, 2023, 9:21 a.m. UTC 
Adding uevent from usb audio gadget driver for uac2 playback/capture
events, which userspace reads and later reads sysfs entry to know if
playback or capture has stopped or started by host application.

/config/usb_gadget/g1/functions/uac2.0 # cat c_status
1  --> capture started
0  --> capture stopped
/config/usb_gadget/g1/functions/uac2.0 # cat p_status
1 --> playback started
0 --> playback stopped

Signed-off-by: Akash Kumar <quic_akakum@quicinc.com>
---
 Documentation/usb/gadget-testing.rst |  6 ++++
 drivers/usb/gadget/function/f_uac2.c | 47 ++++++++++++++++++++++++++++
 drivers/usb/gadget/function/u_uac2.h |  5 +++
 3 files changed, 58 insertions(+)

Comments

AKASH KUMAR Sept. 15, 2023, 6:32 a.m. UTC | #1 
Gentle Reminder!!!

On 8/29/2023 2:51 PM, Akash Kumar wrote:
> Adding uevent from usb audio gadget driver for uac2 playback/capture
> events, which userspace reads and later reads sysfs entry to know if
> playback or capture has stopped or started by host application.
>
> /config/usb_gadget/g1/functions/uac2.0 # cat c_status
> 1  --> capture started
> 0  --> capture stopped
> /config/usb_gadget/g1/functions/uac2.0 # cat p_status
> 1 --> playback started
> 0 --> playback stopped
>
> Signed-off-by: Akash Kumar <quic_akakum@quicinc.com>
> ---
>   Documentation/usb/gadget-testing.rst |  6 ++++
>   drivers/usb/gadget/function/f_uac2.c | 47 ++++++++++++++++++++++++++++
>   drivers/usb/gadget/function/u_uac2.h |  5 +++
>   3 files changed, 58 insertions(+)
>
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index 2eeb3e9299e4..b2fded232ced 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -733,6 +733,12 @@ The uac2 function provides these attributes in its function directory:
>   	p_ssize		playback sample size (bytes)
>   	req_number	the number of pre-allocated request for both capture
>   			and playback
> +        c_status        audio capture state
> +                        (0: capture stopped, 1: capture started)
> +        p_status        audio playback state
> +                        (0: playback stopped, 1: playback started)
> +        c_status        audio capture state
> +        p_status        audio playback state
>   	=============== ====================================================
>   
>   The attributes have sane default values.
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index db2d4980cb35..f1f7631e9380 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -739,6 +739,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>   	struct usb_gadget *gadget = cdev->gadget;
>   	struct device *dev = &gadget->dev;
>   	int ret = 0;
> +	struct f_uac2_opts *audio_opts =
> +		container_of(fn->fi, struct f_uac2_opts, func_inst);
>   
>   	/* No i/f has more than 2 alt settings */
>   	if (alt > 1) {
> @@ -762,6 +764,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>   			ret = u_audio_start_capture(&uac2->g_audio);
>   		else
>   			u_audio_stop_capture(&uac2->g_audio);
> +		audio_opts->c_status = alt;
> +		schedule_work(&audio_opts->work);
>   	} else if (intf == uac2->as_in_intf) {
>   		uac2->as_in_alt = alt;
>   
> @@ -769,6 +773,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>   			ret = u_audio_start_playback(&uac2->g_audio);
>   		else
>   			u_audio_stop_playback(&uac2->g_audio);
> +		audio_opts->p_status = alt;
> +		schedule_work(&audio_opts->work);
>   	} else {
>   		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
>   		return -EINVAL;
> @@ -801,11 +807,16 @@ static void
>   afunc_disable(struct usb_function *fn)
>   {
>   	struct f_uac2 *uac2 = func_to_uac2(fn);
> +	struct f_uac2_opts *audio_opts =
> +		container_of(fn->fi, struct f_uac2_opts, func_inst);
>   
>   	uac2->as_in_alt = 0;
>   	uac2->as_out_alt = 0;
> +	audio_opts->p_status = 0;//alt;
> +	audio_opts->c_status = 0; //alt;
>   	u_audio_stop_capture(&uac2->g_audio);
>   	u_audio_stop_playback(&uac2->g_audio);
> +	schedule_work(&audio_opts->work);
>   }
>   
>   static int
> @@ -1036,6 +1047,25 @@ UAC2_ATTRIBUTE(c_srate);
>   UAC2_ATTRIBUTE(c_ssize);
>   UAC2_ATTRIBUTE(req_number);
>   
> +#define UAC2_ATTRIBUTE_RO(name)                                         \
> +	static ssize_t f_uac2_opts_##name##_show(                       \
> +			struct config_item *item,                       \
> +			char *page)                                     \
> +{                                                                       \
> +	struct f_uac2_opts *opts = to_f_uac2_opts(item);                \
> +	int result;                                                     \
> +									\
> +	mutex_lock(&opts->lock);                                        \
> +	result = scnprintf(page, PAGE_SIZE, "%u\n", opts->name);        \
> +	mutex_unlock(&opts->lock);                                      \
> +									\
> +	return result;                                                  \
> +}                                                                       \
> +CONFIGFS_ATTR_RO(f_uac2_opts_, name)
> +
> +UAC2_ATTRIBUTE_RO(c_status);
> +UAC2_ATTRIBUTE_RO(p_status);
> +
>   static struct configfs_attribute *f_uac2_attrs[] = {
>   	&f_uac2_opts_attr_p_chmask,
>   	&f_uac2_opts_attr_p_srate,
> @@ -1044,6 +1074,8 @@ static struct configfs_attribute *f_uac2_attrs[] = {
>   	&f_uac2_opts_attr_c_srate,
>   	&f_uac2_opts_attr_c_ssize,
>   	&f_uac2_opts_attr_req_number,
> +	&f_uac2_opts_attr_c_status,
> +	&f_uac2_opts_attr_p_status,
>   	NULL,
>   };
>   
> @@ -1053,11 +1085,23 @@ static const struct config_item_type f_uac2_func_type = {
>   	.ct_owner	= THIS_MODULE,
>   };
>   
> +static void f_uac2_audio_status_change_work(struct work_struct *data)
> +{
> +	struct f_uac2_opts *audio_opts =
> +		container_of(data, struct f_uac2_opts, work);
> +	char *envp[2] = { "UAC2_STATE=Changed", NULL };
> +
> +	kobject_uevent_env(&audio_opts->device->kobj,
> +			KOBJ_CHANGE, envp);
> +}
> +
>   static void afunc_free_inst(struct usb_function_instance *f)
>   {
>   	struct f_uac2_opts *opts;
>   
>   	opts = container_of(f, struct f_uac2_opts, func_inst);
> +	device_destroy(opts->device->class, opts->device->devt);
> +	cancel_work_sync(&opts->work);
>   	kfree(opts);
>   }
>   
> @@ -1082,6 +1126,9 @@ static struct usb_function_instance *afunc_alloc_inst(void)
>   	opts->c_srate = UAC2_DEF_CSRATE;
>   	opts->c_ssize = UAC2_DEF_CSSIZE;
>   	opts->req_number = UAC2_DEF_REQ_NUM;
> +	INIT_WORK(&opts->work, f_uac2_audio_status_change_work);
> +	opts->device = create_function_device("f_uac2");
> +
>   	return &opts->func_inst;
>   }
>   
> diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
> index b5035711172d..3ccf2eb002f1 100644
> --- a/drivers/usb/gadget/function/u_uac2.h
> +++ b/drivers/usb/gadget/function/u_uac2.h
> @@ -36,6 +36,11 @@ struct f_uac2_opts {
>   
>   	struct mutex			lock;
>   	int				refcnt;
> +	int				c_status;
> +	int				p_status;
> +	struct device			*device;
> +	struct work_struct		work;
>   };
>   
> +extern struct device *create_function_device(char *name);
>   #endif
Greg KH Sept. 17, 2023, 10:17 a.m. UTC | #2 
On Tue, Aug 29, 2023 at 02:51:32PM +0530, Akash Kumar wrote:
> Adding uevent from usb audio gadget driver for uac2 playback/capture
> events, which userspace reads and later reads sysfs entry to know if
> playback or capture has stopped or started by host application.
> 
> /config/usb_gadget/g1/functions/uac2.0 # cat c_status
> 1  --> capture started
> 0  --> capture stopped
> /config/usb_gadget/g1/functions/uac2.0 # cat p_status
> 1 --> playback started
> 0 --> playback stopped
> 
> Signed-off-by: Akash Kumar <quic_akakum@quicinc.com>
> ---
>  Documentation/usb/gadget-testing.rst |  6 ++++
>  drivers/usb/gadget/function/f_uac2.c | 47 ++++++++++++++++++++++++++++
>  drivers/usb/gadget/function/u_uac2.h |  5 +++
>  3 files changed, 58 insertions(+)
> 
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index 2eeb3e9299e4..b2fded232ced 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -733,6 +733,12 @@ The uac2 function provides these attributes in its function directory:
>  	p_ssize		playback sample size (bytes)
>  	req_number	the number of pre-allocated request for both capture
>  			and playback
> +        c_status        audio capture state
> +                        (0: capture stopped, 1: capture started)
> +        p_status        audio playback state
> +                        (0: playback stopped, 1: playback started)
> +        c_status        audio capture state
> +        p_status        audio playback state

spaces not tabs?

And shouldn't this also be documented in Documentation/ABI/ as well with
the other attributes?

>  	=============== ====================================================
>  
>  The attributes have sane default values.
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index db2d4980cb35..f1f7631e9380 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -739,6 +739,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>  	struct usb_gadget *gadget = cdev->gadget;
>  	struct device *dev = &gadget->dev;
>  	int ret = 0;
> +	struct f_uac2_opts *audio_opts =
> +		container_of(fn->fi, struct f_uac2_opts, func_inst);

Why assign it here and not below?


>  
>  	/* No i/f has more than 2 alt settings */
>  	if (alt > 1) {
> @@ -762,6 +764,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>  			ret = u_audio_start_capture(&uac2->g_audio);
>  		else
>  			u_audio_stop_capture(&uac2->g_audio);
> +		audio_opts->c_status = alt;
> +		schedule_work(&audio_opts->work);

You are changing functionality here, not just adding statistics, why is
work needed here?

>  	} else if (intf == uac2->as_in_intf) {
>  		uac2->as_in_alt = alt;
>  
> @@ -769,6 +773,8 @@ afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
>  			ret = u_audio_start_playback(&uac2->g_audio);
>  		else
>  			u_audio_stop_playback(&uac2->g_audio);
> +		audio_opts->p_status = alt;
> +		schedule_work(&audio_opts->work);

Same here?

>  	} else {
>  		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
>  		return -EINVAL;
> @@ -801,11 +807,16 @@ static void
>  afunc_disable(struct usb_function *fn)
>  {
>  	struct f_uac2 *uac2 = func_to_uac2(fn);
> +	struct f_uac2_opts *audio_opts =
> +		container_of(fn->fi, struct f_uac2_opts, func_inst);

Normally the bigger line goes at top.

>  
>  	uac2->as_in_alt = 0;
>  	uac2->as_out_alt = 0;
> +	audio_opts->p_status = 0;//alt;
> +	audio_opts->c_status = 0; //alt;

You didn't actually mean to do this, right?

Please get review from someone else in your group before sending this
out for us to find obvious problems.

>  	u_audio_stop_capture(&uac2->g_audio);
>  	u_audio_stop_playback(&uac2->g_audio);
> +	schedule_work(&audio_opts->work);
>  }
>  
>  static int
> @@ -1036,6 +1047,25 @@ UAC2_ATTRIBUTE(c_srate);
>  UAC2_ATTRIBUTE(c_ssize);
>  UAC2_ATTRIBUTE(req_number);
>  
> +#define UAC2_ATTRIBUTE_RO(name)                                         \
> +	static ssize_t f_uac2_opts_##name##_show(                       \
> +			struct config_item *item,                       \
> +			char *page)                                     \
> +{                                                                       \
> +	struct f_uac2_opts *opts = to_f_uac2_opts(item);                \
> +	int result;                                                     \
> +									\
> +	mutex_lock(&opts->lock);                                        \
> +	result = scnprintf(page, PAGE_SIZE, "%u\n", opts->name);        \
> +	mutex_unlock(&opts->lock);                                      \
> +									\
> +	return result;                                                  \
> +}                                                                       \
> +CONFIGFS_ATTR_RO(f_uac2_opts_, name)
> +
> +UAC2_ATTRIBUTE_RO(c_status);
> +UAC2_ATTRIBUTE_RO(p_status);
> +
>  static struct configfs_attribute *f_uac2_attrs[] = {
>  	&f_uac2_opts_attr_p_chmask,
>  	&f_uac2_opts_attr_p_srate,
> @@ -1044,6 +1074,8 @@ static struct configfs_attribute *f_uac2_attrs[] = {
>  	&f_uac2_opts_attr_c_srate,
>  	&f_uac2_opts_attr_c_ssize,
>  	&f_uac2_opts_attr_req_number,
> +	&f_uac2_opts_attr_c_status,
> +	&f_uac2_opts_attr_p_status,
>  	NULL,
>  };
>  
> @@ -1053,11 +1085,23 @@ static const struct config_item_type f_uac2_func_type = {
>  	.ct_owner	= THIS_MODULE,
>  };
>  
> +static void f_uac2_audio_status_change_work(struct work_struct *data)
> +{
> +	struct f_uac2_opts *audio_opts =
> +		container_of(data, struct f_uac2_opts, work);
> +	char *envp[2] = { "UAC2_STATE=Changed", NULL };
> +
> +	kobject_uevent_env(&audio_opts->device->kobj,
> +			KOBJ_CHANGE, envp);
> +}
> +
>  static void afunc_free_inst(struct usb_function_instance *f)
>  {
>  	struct f_uac2_opts *opts;
>  
>  	opts = container_of(f, struct f_uac2_opts, func_inst);
> +	device_destroy(opts->device->class, opts->device->devt);

Wait, why?

> +	cancel_work_sync(&opts->work);
>  	kfree(opts);
>  }
>  
> @@ -1082,6 +1126,9 @@ static struct usb_function_instance *afunc_alloc_inst(void)
>  	opts->c_srate = UAC2_DEF_CSRATE;
>  	opts->c_ssize = UAC2_DEF_CSSIZE;
>  	opts->req_number = UAC2_DEF_REQ_NUM;
> +	INIT_WORK(&opts->work, f_uac2_audio_status_change_work);
> +	opts->device = create_function_device("f_uac2");

A whole new device?  Why?  There should already be a device for this, if
not, you now need to document your new device as this is a whole new
thing for sysfs to show, right?

> +
>  	return &opts->func_inst;
>  }
>  
> diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
> index b5035711172d..3ccf2eb002f1 100644
> --- a/drivers/usb/gadget/function/u_uac2.h
> +++ b/drivers/usb/gadget/function/u_uac2.h
> @@ -36,6 +36,11 @@ struct f_uac2_opts {
>  
>  	struct mutex			lock;
>  	int				refcnt;
> +	int				c_status;
> +	int				p_status;
> +	struct device			*device;
> +	struct work_struct		work;

Some documentation please for what these new fields are for.

thanks,

greg k-h
Greg KH April 4, 2024, 1:03 p.m. UTC | #3 
On Thu, Apr 04, 2024 at 06:03:03PM +0530, AKASH KUMAR wrote:

<something in html removed>

Please fix your email client, this was rejected by the mailing lists.

And get review from your internal team first, and get their
signed-off-by on the commit, before sending a new version.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
index 2eeb3e9299e4..b2fded232ced 100644
--- a/Documentation/usb/gadget-testing.rst
+++ b/Documentation/usb/gadget-testing.rst
@@ -733,6 +733,12 @@  The uac2 function provides these attributes in its function directory:
 	p_ssize		playback sample size (bytes)
 	req_number	the number of pre-allocated request for both capture
 			and playback
+        c_status        audio capture state
+                        (0: capture stopped, 1: capture started)
+        p_status        audio playback state
+                        (0: playback stopped, 1: playback started)
+        c_status        audio capture state
+        p_status        audio playback state
 	=============== ====================================================
 
 The attributes have sane default values.
diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
index db2d4980cb35..f1f7631e9380 100644
--- a/drivers/usb/gadget/function/f_uac2.c
+++ b/drivers/usb/gadget/function/f_uac2.c
@@ -739,6 +739,8 @@  afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
 	struct usb_gadget *gadget = cdev->gadget;
 	struct device *dev = &gadget->dev;
 	int ret = 0;
+	struct f_uac2_opts *audio_opts =
+		container_of(fn->fi, struct f_uac2_opts, func_inst);
 
 	/* No i/f has more than 2 alt settings */
 	if (alt > 1) {
@@ -762,6 +764,8 @@  afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
 			ret = u_audio_start_capture(&uac2->g_audio);
 		else
 			u_audio_stop_capture(&uac2->g_audio);
+		audio_opts->c_status = alt;
+		schedule_work(&audio_opts->work);
 	} else if (intf == uac2->as_in_intf) {
 		uac2->as_in_alt = alt;
 
@@ -769,6 +773,8 @@  afunc_set_alt(struct usb_function *fn, unsigned intf, unsigned alt)
 			ret = u_audio_start_playback(&uac2->g_audio);
 		else
 			u_audio_stop_playback(&uac2->g_audio);
+		audio_opts->p_status = alt;
+		schedule_work(&audio_opts->work);
 	} else {
 		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
 		return -EINVAL;
@@ -801,11 +807,16 @@  static void
 afunc_disable(struct usb_function *fn)
 {
 	struct f_uac2 *uac2 = func_to_uac2(fn);
+	struct f_uac2_opts *audio_opts =
+		container_of(fn->fi, struct f_uac2_opts, func_inst);
 
 	uac2->as_in_alt = 0;
 	uac2->as_out_alt = 0;
+	audio_opts->p_status = 0;//alt;
+	audio_opts->c_status = 0; //alt;
 	u_audio_stop_capture(&uac2->g_audio);
 	u_audio_stop_playback(&uac2->g_audio);
+	schedule_work(&audio_opts->work);
 }
 
 static int
@@ -1036,6 +1047,25 @@  UAC2_ATTRIBUTE(c_srate);
 UAC2_ATTRIBUTE(c_ssize);
 UAC2_ATTRIBUTE(req_number);
 
+#define UAC2_ATTRIBUTE_RO(name)                                         \
+	static ssize_t f_uac2_opts_##name##_show(                       \
+			struct config_item *item,                       \
+			char *page)                                     \
+{                                                                       \
+	struct f_uac2_opts *opts = to_f_uac2_opts(item);                \
+	int result;                                                     \
+									\
+	mutex_lock(&opts->lock);                                        \
+	result = scnprintf(page, PAGE_SIZE, "%u\n", opts->name);        \
+	mutex_unlock(&opts->lock);                                      \
+									\
+	return result;                                                  \
+}                                                                       \
+CONFIGFS_ATTR_RO(f_uac2_opts_, name)
+
+UAC2_ATTRIBUTE_RO(c_status);
+UAC2_ATTRIBUTE_RO(p_status);
+
 static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_p_chmask,
 	&f_uac2_opts_attr_p_srate,
@@ -1044,6 +1074,8 @@  static struct configfs_attribute *f_uac2_attrs[] = {
 	&f_uac2_opts_attr_c_srate,
 	&f_uac2_opts_attr_c_ssize,
 	&f_uac2_opts_attr_req_number,
+	&f_uac2_opts_attr_c_status,
+	&f_uac2_opts_attr_p_status,
 	NULL,
 };
 
@@ -1053,11 +1085,23 @@  static const struct config_item_type f_uac2_func_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static void f_uac2_audio_status_change_work(struct work_struct *data)
+{
+	struct f_uac2_opts *audio_opts =
+		container_of(data, struct f_uac2_opts, work);
+	char *envp[2] = { "UAC2_STATE=Changed", NULL };
+
+	kobject_uevent_env(&audio_opts->device->kobj,
+			KOBJ_CHANGE, envp);
+}
+
 static void afunc_free_inst(struct usb_function_instance *f)
 {
 	struct f_uac2_opts *opts;
 
 	opts = container_of(f, struct f_uac2_opts, func_inst);
+	device_destroy(opts->device->class, opts->device->devt);
+	cancel_work_sync(&opts->work);
 	kfree(opts);
 }
 
@@ -1082,6 +1126,9 @@  static struct usb_function_instance *afunc_alloc_inst(void)
 	opts->c_srate = UAC2_DEF_CSRATE;
 	opts->c_ssize = UAC2_DEF_CSSIZE;
 	opts->req_number = UAC2_DEF_REQ_NUM;
+	INIT_WORK(&opts->work, f_uac2_audio_status_change_work);
+	opts->device = create_function_device("f_uac2");
+
 	return &opts->func_inst;
 }
 
diff --git a/drivers/usb/gadget/function/u_uac2.h b/drivers/usb/gadget/function/u_uac2.h
index b5035711172d..3ccf2eb002f1 100644
--- a/drivers/usb/gadget/function/u_uac2.h
+++ b/drivers/usb/gadget/function/u_uac2.h
@@ -36,6 +36,11 @@  struct f_uac2_opts {
 
 	struct mutex			lock;
 	int				refcnt;
+	int				c_status;
+	int				p_status;
+	struct device			*device;
+	struct work_struct		work;
 };
 
+extern struct device *create_function_device(char *name);
 #endif