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:   Sat, 22 Oct 2022 13:32:56 +0200
From:   Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To:     Dan Vacura <w36195@motorola.com>
Cc:     linux-usb@vger.kernel.org,
        Daniel Scally <dan.scally@ideasonboard.com>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Jeff Vanhoof <qjv001@motorola.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Felipe Balbi <balbi@kernel.org>,
        Paul Elder <paul.elder@ideasonboard.com>,
        Michael Grzeschik <m.grzeschik@pengutronix.de>,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH v4 5/6] usb: gadget: uvc: make interrupt skip logic
 configurable
Message-ID: <Y1PU6K4Izr9n5tcs@kroah.com>
References: <20221018215044.765044-1-w36195@motorola.com>
 <20221018215044.765044-6-w36195@motorola.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221018215044.765044-6-w36195@motorola.com>
Precedence: bulk

On Tue, Oct 18, 2022 at 04:50:41PM -0500, Dan Vacura wrote:
> Some UDC hw may not support skipping interrupts, but still support the
> request. Allow the interrupt frequency to be configurable to the user.
> 
> Signed-off-by: Dan Vacura <w36195@motorola.com>
> ---
> V1 -> V2:
> - no change, new patch in series
> V2 -> V3:
> - default to baseline value of 4, fix storing the initial value
> V3 -> V4:
> - no change
> 
>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 1 +
>  Documentation/usb/gadget-testing.rst              | 2 ++

Adding more tunable knobs always just adds complexity and fragility as
things do not get tested.

Can't we just make this dynamic and work properly on all systems?  What
UDC hardware types do not allow skipping interrupts?  Why can't we just
detect that and only do that if this is the case or not?

>  drivers/usb/gadget/function/f_uvc.c               | 3 +++
>  drivers/usb/gadget/function/u_uvc.h               | 1 +
>  drivers/usb/gadget/function/uvc.h                 | 2 ++
>  drivers/usb/gadget/function/uvc_configfs.c        | 2 ++
>  drivers/usb/gadget/function/uvc_queue.c           | 6 ++++++
>  drivers/usb/gadget/function/uvc_video.c           | 3 ++-
>  8 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 611b23e6488d..5dfaa3f7f6a4 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -8,6 +8,7 @@ Description:	UVC function directory
>  		streaming_maxpacket	1..1023 (fs), 1..3072 (hs/ss)
>  		streaming_interval	1..16
>  		function_name		string [32]
> +		req_int_skip_div	unsigned int
>  		===================	=============================
>  
>  What:		/config/usb-gadget/gadget/functions/uvc.name/control
> diff --git a/Documentation/usb/gadget-testing.rst b/Documentation/usb/gadget-testing.rst
> index 2278c9ffb74a..f9b5a09be1f4 100644
> --- a/Documentation/usb/gadget-testing.rst
> +++ b/Documentation/usb/gadget-testing.rst
> @@ -794,6 +794,8 @@ The uvc function provides these attributes in its function directory:
>  			    sending or receiving when this configuration is
>  			    selected
>  	function_name       name of the interface
> +	req_int_skip_div    divisor of total requests to aid in calculating
> +			    interrupt frequency, 0 indicates all interrupt

This provides no information that would help to determine how to
configure this at all.  I wouldn't know what to do with this, so that's
a huge sign that this is not a good interface :(

thanks,

greg k-h