Message-ID: <20200218033542.GY955802@ripper>
Date:   Mon, 17 Feb 2020 19:35:42 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     "Dwivedi, Avaneesh Kumar (avani)" <akdwived@...eaurora.org>
Cc:     linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        ckadabi@...eaurora.org, tsoni@...eaurora.org,
        bryanh@...eaurora.org, psodagud@...eaurora.org,
        rnayak@...eaurora.org, satyap@...eaurora.org,
        pheragu@...eaurora.org
Subject: Re: [PATCH v4 2/2] Embedded USB Debugger (EUD) driver

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...

> > 
> > > +       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.

> > > +	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.

> > 
> > > +	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.

Regards,
Bjorn

Powered by blists - more mailing lists