Hi Greg, On Wed, 17 May 2023 at 10:21, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > > 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@xxxxxxxxxx> > > --- > > 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? Oops, I will do that in the next version. > > 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. Ok, I will double check. > > 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... Sure. > > #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 :( Sure, will spin a separate patch for cleanups. > > #define EUD_ENABLE BIT(0) > > -#define EUD_INT_PET_EUD BIT(0) > > +#define EUD_INT_PET_EUD BIT(0) > > Again, why this change? Ack. Will send a v6 shortly. Thanks, Bhupesh
