Re: [PATCH v5 3/5] usb: misc: eud: Add driver support for SM6115 / SM4250

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, May 17, 2023 at 03:03:06AM +0530, Bhupesh Sharma wrote:
> Add SM6115 / SM4250 SoC EUD support in qcom_eud driver.

Why is the subject line duplicated here?

> On some SoCs (like the SM6115 / SM4250 SoC), the mode manager
> needs to be accessed only via the secure world (through 'scm'
> calls).
> 
> Also, the enable bit inside 'tcsr_check_reg' needs to be set
> first to set the eud in 'enable' mode on these SoCs.
> 
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> ---
>  drivers/usb/misc/Kconfig    |  1 +
>  drivers/usb/misc/qcom_eud.c | 69 +++++++++++++++++++++++++++++++++----

Given that you didn't cc the usb maintainer, I'm guessing you don't want
this patch applied?

>  2 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 99b15b77dfd5..fe1b5fec1dfc 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -147,6 +147,7 @@ config USB_APPLEDISPLAY
>  config USB_QCOM_EUD
>  	tristate "QCOM Embedded USB Debugger(EUD) Driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
> +	select QCOM_SCM

How well is that going to work on building on non-QCOM systems?  Can
QCOM_SCM build if COMPILE_TEST is enabled?  select is rough to get
right, are you sure it's correct here?  If so, some documentation in the
changelog would be appreciated.

>  	select USB_ROLE_SWITCH
>  	help
>  	  This module enables support for Qualcomm Technologies, Inc.
> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index b7f13df00764..10d194604d4c 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -5,12 +5,14 @@
>  
>  #include <linux/bitops.h>
>  #include <linux/err.h>
> +#include <linux/firmware/qcom/qcom_scm.h>

There's no rule to keep these sorted, but it's your choice...

>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/iopoll.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sysfs.h>
> @@ -22,23 +24,33 @@
>  #define EUD_REG_VBUS_INT_CLR	0x0080
>  #define EUD_REG_CSR_EUD_EN	0x1014
>  #define EUD_REG_SW_ATTACH_DET	0x1018
> -#define EUD_REG_EUD_EN2        0x0000
> +#define EUD_REG_EUD_EN2		0x0000

Why the coding style cleanup in the same patch?  Remember, changes only
do one thing, and you have already listed 2 things in your commit
message :(

>  
>  #define EUD_ENABLE		BIT(0)
> -#define EUD_INT_PET_EUD	BIT(0)
> +#define EUD_INT_PET_EUD		BIT(0)

Again, why this change?

thanks,

greg k-h