Messages in this thread | | | Subject | Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver | From | "Dwivedi, Avaneesh Kumar (avani)" <> | Date | Tue, 18 Feb 2020 18:30:26 +0530 |
| |
On 2/18/2020 9:05 AM, Bjorn Andersson wrote: > On Sun 16 Feb 06:14 PST 2020, Dwivedi, Avaneesh Kumar (avani) wrote: > >> Thank you very much Bjorn for your comments, will address them and post >> latest patchset soon. >> >> On 2/4/2020 1:05 AM, Bjorn Andersson wrote: >>> On Thu 30 Jan 20:43 PST 2020, Avaneesh Kumar Dwivedi wrote: > [..] >>>> 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 >>> Please aim for keeping the sort order in this file (ignore QCOM_APR >>> which obviously is in the wrong place) >> Please help to elaborate more, do you mean adding configs in alphabetical >> order? > Yes, we want to maintain alphabetical sort order of the config options > in the Kconfig file. Unfortunately I must have missed this as I picked > up QCOM_APR - hence my ask to add you entry further up, even if the > order isn't perfect... Ok > >>>> + tristate "QTI Embedded USB Debugger (EUD)" >>>> + depends on ARCH_QCOM >>>> + 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). >>>> + If unsure, say N. >>>> endmenu > [..] >>>> +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. >> ok >>>> + chip->enable = enable; >>> So if I write 42 to "enable" nothing will change in the hardware, but >>> chip->enable will be 42... >> will change enable struct member to bool? > The problem I meant was hat if buf is "42", then you will hit the > following code path: > > int ret = 0; > sscanf(buf, "%du", &enable); > chip->enable = 42; > > As enable isn't 1 or 0, neither conditional path is taken, but you still > store the value in chip->enable. > > Changing enable to bool won't change this problem, adding an else and > returning -EINVAL; would. ok. >>>> + return count; >>>> +} >>>> + >>>> +static DEVICE_ATTR_RW(enable); > [..] >>>> +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? >> i could see that usb role switch has been implemented for c type connector >> and that connector is modeled as child of usb controller, but EUD is not a >> true connector, it intercepts PHY signals and reroute it to USB controller >> as per EUD Command issued by debug appliaction >> >> i am not sure if i need to implement EUD DT node as child of usb controller, >> if i do so, as per my understanding EUD driver need to set USB controller >> mode(host or device mode) by calling usb role switch API's, please let me >> know if my understanding is correct? >> > I don't know how to properly represent this, but I would like the USB > guys to chime in before merging something.
I will check with USB folks if they can give their opinion.
based on that will proceed.
> >>>> + if (IS_ERR(chip->extcon)) >>>> + return PTR_ERR(chip->extcon); >>>> + > [..] >>>> +static const struct of_device_id msm_eud_dt_match[] = { >>>> + {.compatible = "qcom,msm-eud"}, >>> Is this the one and only, past and future, version of the EUD hardware >>> block? Or do we need this compatible to be more specific? >> EUD h/w IP is Qualcomm IP, As of now this is only hw IP available, if >> future version of EUD IP comes, we can modify and add support then? > You can add additional compatibles, but you can't change this one as > existing devicetree files must continue to function. > > If you have a number of platforms that works with this very same > implementation then you could make the binding require a specific > platform and qcom,msm-eud (although qcom,eud should be enough?) and then > keep the implementation as is. > > I.e. dt would say: > compatible = "qcom,sc7180-eud", "qcom,eud"; > > And driver would match on the latter only, for now. Ok. > > Regards, > Bjorn
-- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
| |