On 2/4/2020 8:40 AM, Bryan O'Donoghue wrote:
On 03/02/2020 19:35, Bjorn Andersson wrote:
On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
Hi Avaneesh.
Hello Bryan, Thank you very much for your review comments.
Will be replying to your comments and will be posting new patchset soon
as per review comments.
Please aim for keeping the sort order in this file (ignore QCOM_APR
which obviously is in the wrong place)
+ tristate "QTI Embedded USB Debugger (EUD)"
+ depends on ARCH_QCOM
If we persist with the model of EXTCON you should "select EXTCON" here.
I have asked this query with Bjorn Also against his review comments,
whether we need to persist with extcon or need to switch to usb role
switch framework, as we are notifying not only to usb controller but
also to pmic charger so in case we adopt usb role switch then how we
will notify to pmic charger to enable charging battery ? Also as i
mentioned there my dilema is it does not look very apt to model EUD hw
IP as c type connector, so please let me know your views.
+ help
+ The Embedded USB Debugger (EUD) driver is a driver for the
+ control peripheral which waits on events like USB
attach/detach
+ and charger enable/disable. The control peripheral further
helps
+ support the USB-based debug and trace capabilities.
+ This module enables support for Qualcomm Technologies, Inc.
+ Embedded USB Debugger (EUD).
Suggest.
This module enables support for Qualcomm Technologies, Inc.
Embedded USB Debugger (EUD).
The EUD is a control peripheral which reports VBUS attach/detach,
charger enable/disable and USB-based debug and trace capabilities.
OK.
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
2020
OK
+
+static int enable_eud(struct eud_chip *priv)
+{
+ int ret;
+
+ /* write into CSR to enable EUD */
+ writel_relaxed(BIT(0), priv->eud_reg_base + EUD_REG_CSR_EUD_EN);
+ /* Enable vbus, chgr & safe mode warning interrupts */
+ writel_relaxed(EUD_INT_VBUS | EUD_INT_CHGR | EUD_INT_SAFE_MODE,
+ priv->eud_reg_base + EUD_REG_INT1_EN_MASK);
+
+ /* Ensure Register Writes Complete */
So... You are writing a register in an on-chip PMIC. The PMIC is
responsible for detecting USB ID and supplying VBUS as appropriate.
You then get an interrupt to inform you of the state ?
I am writing to EUD control port so that when EUD is enable, EUD hw IP
can intercept VBUS and d+/d- signal and can reroute to PMIC or USB as
per host application command in debug mode.
so for example in debug mode VBUS signal although asserted on connecting
phone with host PC, but EUD based on debug application command can
notify USB to detach(which otherwise would have detected and attached)
+static ssize_t enable_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct eud_chip *chip = dev_get_drvdata(dev);
+ int enable = 0;
You shouldn't need to initialize this as you're checking the return
value of sscanf().
OK
+ int ret = 0;
+
+ if (sscanf(buf, "%du", &enable) != 1)
+ return -EINVAL;
+
+ if (enable == EUD_ENABLE_CMD)
+ ret = enable_eud(chip);
If ret is !0 you should probably return that, rather than count...
OK
+ else if (enable == EUD_DISABLE_CMD)
+ disable_eud(chip);
+ if (!ret)
...and then you don't need this check, or initialize ret to 0 above.
+ chip->enable = enable;
So if I write 42 to "enable" nothing will change in the hardware, but
chip->enable will be 42...
+ return count;
+}
I was just going to comment on usb_connector but, does the above code
need a synchronization primitive to serialize with the worker and
interrupt handler ?
Will evaluate and take corrective action if needed.
+static int msm_eud_probe(struct platform_device *pdev)
+{
+ struct eud_chip *chip;
+ struct resource *res;
+ int ret;
+
+ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ chip->dev = &pdev->dev;
+ platform_set_drvdata(pdev, chip);
+
+ chip->extcon = devm_extcon_dev_allocate(&pdev->dev,
eud_extcon_cable);
Aren't we moving away from extcon in favor of the usb role switching
thing?
Yes.
For the VBUS notification you could use
usb-role-switch and model the USB connector as a child-node of the
dual-role controller.
I am not sure if EUD interface is true USB connector or should be
modeled so, as EUD can trick usb controller about absence of VBUS/d+/d-
signal. To illustrate, when in debug mode even if phone is connected
with PC, EUD can notify USB controller to stop USB s/w stack, by
notifying USB controller about usb detach event, even when d+\d- signals
are valid. moreover in debug mode USB controller will always configure
in device mode, so let me know if EUD qualifies to be modeled as child
of controller node?
See:
https://patchwork.kernel.org/cover/11346247/
https://patchwork.kernel.org/patch/11346295/
https://patchwork.kernel.org/patch/11346263/
Avaneesh do you have any kernel code that cares about the charger state ?
charger state is to be notified to charger driver to start charging
battery so if i switch to usb role switch framework, how will i notify
to pmic charger? so if i have to adopt usb role switch framework then
also i will have to keep extcon framework, let me know your comment.
What we are suggesting here is dropping extcon and using
role-switching but, if you have some other code that cares about
EXTCON_CHG_USB_SDP you'd have to do additional work.
But, if I understood the implication of the code above where you write
to the PMIC and let it handle VBUS/CHARGER on/off and you are just
notified of the state change, you should be fine with usb-role-switching.
as i mentioned usb-role-switch will only cater need to notify to usb
controller so please let me know your views.
---
bod
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
