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

["Dwivedi, Avaneesh Kumar (avani)" <akdwived@codeaurora.org>]

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.