Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver

From: Dwivedi, Avaneesh Kumar (avani)
Date: Mon Feb 17 2020 - 02:22:11 EST

On 2/16/2020 10:05 PM, Greg KH wrote:
On Sun, Feb 16, 2020 at 09:52:19PM +0530, Dwivedi, Avaneesh Kumar (avani) wrote:
On 2/7/2020 3:34 PM, Greg KH wrote:
On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
Add support for control peripheral of EUD (Embedded USB Debugger) to
listen to events such as USB attach/detach, charger enable/disable, pet
EUD to indicate software is functional. Reusing the platform device kobj,
sysfs entry 'enable' is created to enable or disable EUD.

Signed-off-by: Satya Durga Srinivasu Prabhala <satyap@xxxxxxxxxxxxxx>
Signed-off-by: Prakruthi Deepak Heragu <pheragu@xxxxxxxxxxxxxx>
Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
Documentation/ABI/stable/sysfs-driver-msm-eud | 5 +
drivers/soc/qcom/Kconfig | 12 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/eud.c | 329 ++++++++++++++++++++++++++
4 files changed, 347 insertions(+)
create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
create mode 100644 drivers/soc/qcom/eud.c

diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
new file mode 100644
index 0000000..d96ae05
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
@@ -0,0 +1,5 @@
+What: /sys/bus/platform/drivers/msm-eud/enable
+Date: Jan 2020
+Contact: Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
+Description: Enable/Disable use of eud device.
What are valid values to be used here?
it should be bool variable relying on 0 or 1.
Then document it.

+Users: User space debug application which intend to use EUD h/w block.
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index d0a73e7..6b7c9d0 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -202,4 +202,16 @@ config QCOM_APR
application processor and QDSP6. APR is
used by audio driver to configure QDSP6
ASM, ADM and AFE modules.
+config QCOM_EUD
+ tristate "QTI Embedded USB Debugger (EUD)"
+ depends on ARCH_QCOM
Why not let everyone test build this?
EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?
No, why can't everyone buid it for testing? What about when I want to
build a generic arm64 kernel to run on multiple SoCs?

Do not put dependancies in here that you really do not have. There's no
reason for this to be limited to that one chip, right? And if you allow
others to build the code, you will get proper bug reports when things
break, and others will fix them, which is what you want.

I think the ARCH_RANDOM_SOC_NAME is totally broken and needs to be, at
most, just an arch-specific thing, if even that.

Look at almost all other kernel drivers, they do not have those types of
Will check and address concerns.

+ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+ chip->dev = &pdev->dev;
No reference counting???
you mean get/put_device?


greg k-h
Thank you very much Greg for your time to review.

Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.