On 03/02/2020 19:35, Bjorn Andersson wrote:
On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote:
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.
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.
+ÂÂÂÂÂÂÂÂ 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).
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.
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+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 ?
+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().
+ÂÂÂ 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...
Will evaluate and take corrective action if needed.
+ÂÂÂ else if (enable == EUD_DISABLE_CMD)
+ÂÂÂ 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 ?
+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
For the VBUS notification you could use
usb-role-switch and model the USB connector as a child-node of the dual-role controller.
Avaneesh do you have any kernel code that cares about the charger state ?
as i mentioned usb-role-switch will only cater need to notify to usb controller so please let me know your views.
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.