Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 8 May 2023 14:14:18 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     AKASH KUMAR <quic_akakum@quicinc.com>
Cc:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Jing Leng <jleng@ambarella.com>,
        Pratham =?iso-8859-1?Q?Pratap=A0?= <quic_ppratap@quicinc.com>,
        Jack Pham <quic_jackp@quicinc.com>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: gadget: f_uac2: uevent changes for uac2
Message-ID: <2023050801-handshake-refusing-0367@gregkh>
References: <20230508120535.31472-1-quic_akakum@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230508120535.31472-1-quic_akakum@quicinc.com>
Precedence: bulk

On Mon, May 08, 2023 at 05:35:35PM +0530, AKASH KUMAR wrote:
> Adding uvent from usb audio gadget driver for

What is a "uvent"?

> 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.

Please use your full 72 columns please.

> 
> /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

You need to document these new files in Documentation/ABI/ right?

> 
> Signed-off-by: AKASH KUMAR <quic_akakum@quicinc.com>

No need to UPPER CASE your name :)

> ---
>  drivers/usb/gadget/function/f_uac2.c | 57 ++++++++++++++++++++++++++++
>  drivers/usb/gadget/function/u_uac2.h |  7 ++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/f_uac2.c b/drivers/usb/gadget/function/f_uac2.c
> index 0219cd79493a..d0a5fa6b49b8 100644
> --- a/drivers/usb/gadget/function/f_uac2.c
> +++ b/drivers/usb/gadget/function/f_uac2.c
> @@ -1423,6 +1423,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) {
> @@ -1454,6 +1456,7 @@ 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;
>  	} else if (intf == uac2->as_in_intf) {
>  		uac2->as_in_alt = alt;
>  
> @@ -1461,10 +1464,14 @@ 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;
>  	} else {
>  		dev_err(dev, "%s:%d Error!\n", __func__, __LINE__);
>  		return -EINVAL;
>  	}
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +	schedule_work(&audio_opts->work);
> +#endif

Why the #ifdef?  Please never do that in .c files if at all possible.

>  
>  	return ret;
>  }
> @@ -1493,9 +1500,17 @@ 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;
> +
> +#ifdef CONFIG_USB_CONFIGFS_UEVENT
> +	schedule_work(&audio_opts->work);
> +#endif

Same here and elsewhere in this patch.

thanks,

greg k-h