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

Tingwei Zhang
2020-08-14T00:09:54Z

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