[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZotaBxRpv29crRHV@jiegan-gv.ap.qualcomm.com>
Date: Mon, 8 Jul 2024 11:16:23 +0800
From: JieGan <quic_jiegan@...cinc.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC: Mathieu Poirier <mathieu.poirier@...aro.org>,
Suzuki K Poulose
<suzuki.poulose@....com>,
Alexander Shishkin
<alexander.shishkin@...ux.intel.com>,
Mike Leach <mike.leach@...aro.org>, "Rob Herring" <robh+dt@...nel.org>,
Krzysztof Kozlowski
<krzysztof.kozlowski+dt@...aro.org>,
James Clark <james.clark@....com>,
Jinlong Mao <quic_jinlmao@...cinc.com>, Leo Yan <leo.yan@...aro.org>,
<coresight@...ts.linaro.org>, <linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
Tingwei Zhang
<quic_tingweiz@...cinc.com>,
Yuanfang Zhang <quic_yuanfang@...cinc.com>,
"Tao
Zhang" <quic_taozha@...cinc.com>,
Trilok Soni <quic_tsoni@...cinc.com>,
"Song
Chai" <quic_songchai@...cinc.com>,
<linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] Coresight: Add Coresight Control Unit driver
On Fri, Jul 05, 2024 at 11:11:19AM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2024 11:00, Jie Gan wrote:
> > The Coresight Control Unit hosts miscellaneous configuration registers
> > which control various features related to TMC ETR sink.
> >
> > Based on the trace ID, which is programmed in the related CCU ATID register
> > of a specific ETR, trace data with that trace ID gets into the ETR buffer,
>
> ....
>
> > +static int ccu_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct coresight_platform_data *pdata;
> > + struct ccu_drvdata *drvdata;
> > + struct coresight_desc desc = { 0 };
> > + struct resource *res;
> > +
> > + desc.name = coresight_alloc_device_name(&ccu_devs, dev);
> > + if (!desc.name)
> > + return -ENOMEM;
> > + pdata = coresight_get_platform_data(dev);
> > + if (IS_ERR(pdata))
> > + return PTR_ERR(pdata);
> > + pdev->dev.platform_data = pdata;
> > +
> > + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> > + if (!drvdata)
> > + return -ENOMEM;
> > + drvdata->dev = &pdev->dev;
>
> Use stored dev variable.
pdev->dev replaced by dev variable.
>
> > + drvdata->atid_offset = 0;
> > + platform_set_drvdata(pdev, drvdata);
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ccu-base");
> > + if (!res)
> > + return -ENODEV;
> > + drvdata->pbase = res->start;
>
> Drop.
Dropped.
>
> > +
> > + drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
>
> Use proper wrapper for this two.
Replaced by:
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
drvdata->base = devm_ioremap_resource(dev, res);
>
> > + if (!drvdata->base)
> > + return -ENOMEM;
> > +
> > + desc.type = CORESIGHT_DEV_TYPE_HELPER;
> > + desc.pdata = pdev->dev.platform_data;
> > + desc.dev = &pdev->dev;
> > + desc.ops = &ccu_ops;
> > +
> > + drvdata->csdev = coresight_register(&desc);
> > + if (IS_ERR(drvdata->csdev))
> > + return PTR_ERR(drvdata->csdev);
> > +
> > + dev_dbg(dev, "CCU initialized: %s\n", desc.name);
>
> Drop.
Dropped.
>
> > + return 0;
> > +}
> > +
> > +static void ccu_remove(struct platform_device *pdev)
> > +{
> > + struct ccu_drvdata *drvdata = platform_get_drvdata(pdev);
> > +
> > + coresight_unregister(drvdata->csdev);
> > +}
> > +
> > +static const struct of_device_id ccu_match[] = {
> > + {.compatible = "qcom,coresight-ccu"},
> > + {}
> > +};
> > +
> > +static struct platform_driver ccu_driver = {
> > + .probe = ccu_probe,
> > + .remove = ccu_remove,
> > + .driver = {
> > + .name = "coresight-ccu",
> > + .of_match_table = ccu_match,
> > + .suppress_bind_attrs = true,
>
> Why?
Sorry, I dont get the point here.
We dont need automatic bind/unbind, so the suppress_bind_attrs sets to true.
We need configure some settings before we register the device.
>
> > + },
> > +};
> > +
> > +static int __init ccu_init(void)
> > +{
> > + return platform_driver_register(&ccu_driver);
> > +}
> > +module_init(ccu_init);
> > +
> > +static void __exit ccu_exit(void)
> > +{
> > + platform_driver_unregister(&ccu_driver);
> > +}
> > +module_exit(ccu_exit);
>
> Why this is not just module platform driver?
Replaced by module_platform_driver(ccu_driver);
>
> Best regards,
> Krzysztof
>
Thanks,
Jie
Powered by blists - more mailing lists