Re: [PATCH v8 15/24] coresight: allow replicator driver to be built as module

From: Tingwei Zhang <tingweiz@codeaurora•org>
To: Mathieu Poirier <mathieu.poirier@linaro•org>
Cc: tsoni@codeaurora•org,
	Sai Prakash Ranjan <saiprakash.ranjan@codeaurora•org>,
	Kim Phillips <kim.phillips@arm•com>,
	Mao Jinlong <jinlmao@codeaurora•org>,
	Suzuki K Poulose <suzuki.poulose@arm•com>,
	Alexander Shishkin <alexander.shishkin@linux•intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation•org>,
	coresight@lists•linaro.org, Randy Dunlap <rdunlap@infradead•org>,
	Mian Yousaf Kaukab <ykaukab@suse•de>,
	Russell King <linux@armlinux•org.uk>,
	Tingwei Zhang <tingwei@codeaurora•org>,
	Leo Yan <leo.yan@linaro•org>,
	linux-arm-kernel@lists•infradead.org,
	Mike Leach <mike.leach@linaro•org>
Subject: Re: [PATCH v8 15/24] coresight: allow replicator driver to be built as module
Date: Fri, 14 Aug 2020 08:08:06 +0800	[thread overview]
Message-ID: <20200814000806.GC23288@codeaurora.org> (raw)
In-Reply-To: <20200813203511.GL3393195@xps15>

On Fri, Aug 14, 2020 at 04:35:11AM +0800, Mathieu Poirier wrote:
> On Fri, Aug 07, 2020 at 07:11:44PM +0800, Tingwei Zhang wrote:
> > From: Kim Phillips <kim.phillips@arm•com>
> > 
> > Allow to build coresight-replicator as modules, for ease of development.
> > 
> > - Kconfig becomes a tristate, to allow =m
> > - combine static and dynamic replicator init into single
> >   module_init/exit call
> > - add replicator_remove functions, for module unload
> > - add a MODULE_DEVICE_TABLE for autoloading on boot
> > 
> > Cc: Mathieu Poirier <mathieu.poirier@linaro•org>
> > Cc: Leo Yan <leo.yan@linaro•org>
> > Cc: Alexander Shishkin <alexander.shishkin@linux•intel.com>
> > Cc: Randy Dunlap <rdunlap@infradead•org>
> > Cc: Suzuki K Poulose <Suzuki.Poulose@arm•com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation•org>
> > Cc: Russell King <linux@armlinux•org.uk>
> > Co-developed-by: Mian Yousaf Kaukab <ykaukab@suse•de>
> > Signed-off-by: Mian Yousaf Kaukab <ykaukab@suse•de>
> > Signed-off-by: Kim Phillips <kim.phillips@arm•com>
> > Signed-off-by: Tingwei Zhang <tingwei@codeaurora•org>
> > Tested-by: Mike Leach <mike.leach@linaro•org>
> > ---
> >  drivers/hwtracing/coresight/Kconfig           |  5 +-
> >  .../coresight/coresight-replicator.c          | 63 ++++++++++++++++++-
> >  2 files changed, 65 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwtracing/coresight/Kconfig
> b/drivers/hwtracing/coresight/Kconfig
> > index fc48ae086746..f31778dd0b5d 100644
> > --- a/drivers/hwtracing/coresight/Kconfig
> > +++ b/drivers/hwtracing/coresight/Kconfig
> > @@ -17,13 +17,16 @@ menuconfig CORESIGHT
> >  
> >  if CORESIGHT
> >  config CORESIGHT_LINKS_AND_SINKS
> > -	bool "CoreSight Link and Sink drivers"
> > +	tristate "CoreSight Link and Sink drivers"
> >  	help
> >  	  This enables support for CoreSight link and sink drivers that
> are
> >  	  responsible for transporting and collecting the trace data
> >  	  respectively.  Link and sinks are dynamically aggregated with a
> trace
> >  	  entity at run time to form a complete trace path.
> 
> With the passage of time and the refinement of the supported components,
> the
> CONFIG_ and description are no longer valid.  I'll leave it up to you to
> decide
> if you want to fix it as part of this set, the choice is entirely up to
> you.
> 

I'll leave that to another patch and focus this patch set to make driver
as module.

> >  
> > +	  To compile these drivers as modules, choose M here: the
> > +	  modules will be called coresight-funnel and
> coresight-replicator.
> > +
> >  config CORESIGHT_LINK_AND_SINK_TMC
> >  	tristate "Coresight generic TMC driver"
> >  
> > diff --git a/drivers/hwtracing/coresight/coresight-replicator.c
> b/drivers/hwtracing/coresight/coresight-replicator.c
> > index 78acf29c49ca..05db434c79d0 100644
> > --- a/drivers/hwtracing/coresight/coresight-replicator.c
> > +++ b/drivers/hwtracing/coresight/coresight-replicator.c
> > @@ -291,6 +291,14 @@ static int replicator_probe(struct device *dev,
> struct resource *res)
> >  	return ret;
> >  }
> >  
> > +static int __exit replicator_remove(struct device *dev)
> > +{
> > +	struct replicator_drvdata *drvdata = dev_get_drvdata(dev);
> > +
> > +	coresight_unregister(drvdata->csdev);
> > +	return 0;
> > +}
> > +
> >  static int static_replicator_probe(struct platform_device *pdev)
> >  {
> >  	int ret;
> > @@ -310,6 +318,13 @@ static int static_replicator_probe(struct
> platform_device *pdev)
> >  	return ret;
> >  }
> >  
> > +static int __exit static_replicator_remove(struct platform_device
> *pdev)
> > +{
> > +	replicator_remove(&pdev->dev);
> > +	pm_runtime_disable(&pdev->dev);
> > +	return 0;
> > +}
> > +
> >  #ifdef CONFIG_PM
> >  static int replicator_runtime_suspend(struct device *dev)
> >  {
> > @@ -342,25 +357,28 @@ static const struct of_device_id
> static_replicator_match[] = {
> >  	{.compatible = "arm,coresight-static-replicator"},
> >  	{}
> >  };
> > +MODULE_DEVICE_TABLE(of, static_replicator_match);
> 
> Pick a way to do this, a newline or not, but stick with it for _all_ the
> drivers
> in this set.
>

I will add a new line and check all the coresight drivers.

> >  
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id static_replicator_acpi_ids[] = {
> >  	{"ARMHC985", 0}, /* ARM CoreSight Static Replicator */
> >  	{}
> >  };
> > +MODULE_DEVICE_TABLE(acpi, static_replicator_acpi_ids);
> 
> Same as above
> 

I will add a new line.

> >  #endif
> >  
> >  static struct platform_driver static_replicator_driver = {
> >  	.probe          = static_replicator_probe,
> > +	.remove         = static_replicator_remove,
> >  	.driver         = {
> >  		.name   = "coresight-static-replicator",
> > +		.owner	= THIS_MODULE,
> >  		.of_match_table = of_match_ptr(static_replicator_match),
> >  		.acpi_match_table = ACPI_PTR(static_replicator_acpi_ids),
> >  		.pm	= &replicator_dev_pm_ops,
> >  		.suppress_bind_attrs = true,
> >  	},
> >  };
> > -builtin_platform_driver(static_replicator_driver);
> >  
> >  static int dynamic_replicator_probe(struct amba_device *adev,
> >  				    const struct amba_id *id)
> > @@ -368,19 +386,60 @@ static int dynamic_replicator_probe(struct
> amba_device *adev,
> >  	return replicator_probe(&adev->dev, &adev->res);
> >  }
> >  
> > +static int __exit dynamic_replicator_remove(struct amba_device *adev)
> > +{
> > +	return replicator_remove(&adev->dev);
> > +}
> > +
> >  static const struct amba_id dynamic_replicator_ids[] = {
> >  	CS_AMBA_ID(0x000bb909),
> >  	CS_AMBA_ID(0x000bb9ec),		/* Coresight SoC-600 */
> >  	{},
> >  };
> >  
> > +MODULE_DEVICE_TABLE(amba, dynamic_replicator_ids);
> > +
> >  static struct amba_driver dynamic_replicator_driver = {
> >  	.drv = {
> >  		.name	= "coresight-dynamic-replicator",
> >  		.pm	= &replicator_dev_pm_ops,
> > +		.owner	= THIS_MODULE,
> >  		.suppress_bind_attrs = true,
> >  	},
> >  	.probe		= dynamic_replicator_probe,
> > +	.remove         = dynamic_replicator_remove,
> >  	.id_table	= dynamic_replicator_ids,
> >  };
> > -builtin_amba_driver(dynamic_replicator_driver);
> > +
> > +static int __init replicator_init(void)
> > +{
> > +	int ret;
> > +
> > +	ret = platform_driver_register(&static_replicator_driver);
> > +	if (ret) {
> > +		pr_info("Error registering platform driver\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = amba_driver_register(&dynamic_replicator_driver);
> > +	if (ret) {
> > +		pr_info("Error registering amba driver\n");
> > +		platform_driver_unregister(&static_replicator_driver);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void __exit replicator_exit(void)
> > +{
> > +	platform_driver_unregister(&static_replicator_driver);
> > +	amba_driver_unregister(&dynamic_replicator_driver);
> > +}
> > +
> 
> Same here, choose to add a newline or not but stay consistent for all the
> drives.
>

Sure. 

> > +module_init(replicator_init);
> > +module_exit(replicator_exit);
> > +
> > +MODULE_AUTHOR("Pratik Patel <pratikp@codeaurora•org>");
> 
> Pratik should also be mentioned in the funnel driver.
>

I'll add Pratik in v9.

> > +MODULE_AUTHOR("Mathieu Poirier <mathieu.poirier@linaro•org>");
> > +MODULE_DESCRIPTION("Arm CoreSight Replicator Driver");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> Forum,
> > a Linux Foundation Collaborative Project
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel