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

[Greg KH]

On Sun, Feb 16, 2020 at 09:52:19PM +0530, Dwivedi, Avaneesh Kumar (avani) wrote:
> 
> On 2/7/2020 3:34 PM, Greg KH wrote:
> > On Fri, Jan 31, 2020 at 10:13:31AM +0530, Avaneesh Kumar Dwivedi wrote:
> > > Add support for control peripheral of EUD (Embedded USB Debugger) to
> > > listen to events such as USB attach/detach, charger enable/disable, pet
> > > EUD to indicate software is functional. Reusing the platform device kobj,
> > > sysfs entry 'enable' is created to enable or disable EUD.
> > > 
> > > Signed-off-by: Satya Durga Srinivasu Prabhala <satyap@xxxxxxxxxxxxxx>
> > > Signed-off-by: Prakruthi Deepak Heragu <pheragu@xxxxxxxxxxxxxx>
> > > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> > > ---
> > >   Documentation/ABI/stable/sysfs-driver-msm-eud |   5 +
> > >   drivers/soc/qcom/Kconfig                      |  12 +
> > >   drivers/soc/qcom/Makefile                     |   1 +
> > >   drivers/soc/qcom/eud.c                        | 329 ++++++++++++++++++++++++++
> > >   4 files changed, 347 insertions(+)
> > >   create mode 100644 Documentation/ABI/stable/sysfs-driver-msm-eud
> > >   create mode 100644 drivers/soc/qcom/eud.c
> > > 
> > > diff --git a/Documentation/ABI/stable/sysfs-driver-msm-eud b/Documentation/ABI/stable/sysfs-driver-msm-eud
> > > new file mode 100644
> > > index 0000000..d96ae05
> > > --- /dev/null
> > > +++ b/Documentation/ABI/stable/sysfs-driver-msm-eud
> > > @@ -0,0 +1,5 @@
> > > +What:           /sys/bus/platform/drivers/msm-eud/enable
> > > +Date:           Jan 2020
> > > +Contact:        Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> > > +Description:    Enable/Disable use of eud device.
> > What are valid values to be used here?
> it should be bool variable relying on 0 or 1.

Then document it.

> > 
> > > +Users:          User space debug application which intend to use EUD h/w block.
> > > 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
> > > +       tristate "QTI Embedded USB Debugger (EUD)"
> > > +       depends on ARCH_QCOM
> > Why not let everyone test build this?
> EUD is Qualcomm IP, shall not it be associated with ARCH_QCOM?

No, why can't everyone buid it for testing?  What about when I want to
build a generic arm64 kernel to run on multiple SoCs?

Do not put dependancies in here that you really do not have.  There's no
reason for this to be limited to that one chip, right?  And if you allow
others to build the code, you will get proper bug reports when things
break, and others will fix them, which is what you want.

I think the ARCH_RANDOM_SOC_NAME is totally broken and needs to be, at
most, just an arch-specific thing, if even that.

Look at almost all other kernel drivers, they do not have those types of
dependancies.

> > > +	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
> > > +	if (!chip)
> > > +		return -ENOMEM;
> > > +
> > > +	chip->dev = &pdev->dev;
> > No reference counting???
> you mean get/put_device?

yes.

thanks,

greg k-h