linux-kernel - Re: [PATCH v1 2/7] coresight: Add coresight TGU driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <db2bc170-052e-4c9b-a191-4d916bbe0993@kernel.org>
Date: Fri, 30 Aug 2024 12:24:34 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: songchai <quic_songchai@...cinc.com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>, James Clark <james.clark@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Andy Gross <agross@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: linux-kernel@...r.kernel.org, coresight@...ts.linaro.org,
 linux-arm-kernel@...ts.infradead.org, linux-arm-msm@...r.kernel.org,
 devicetree@...r.kernel.org
Subject: Re: [PATCH v1 2/7] coresight: Add coresight TGU driver

On 30/08/2024 11:23, songchai wrote:
> Add driver to support Coresight device TGU (Trigger Generation Unit).
> TGU is a Data Engine which can be utilized to sense a plurality of
> signals and create a trigger into the CTI or generate interrupts to
> processors. Add probe/enable/disable functions for tgu.
> 
> Signed-off-by: songchai <quic_songchai@...cinc.com>

...

> +
> +static struct attribute *tgu_common_attrs[] = {
> +	&dev_attr_enable_tgu.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group tgu_common_grp = {

Not const?

> +	.attrs = tgu_common_attrs,
> +	NULL,
> +};
> +
> +static const struct attribute_group *tgu_attr_groups[] = {
> +	&tgu_common_grp,
> +	NULL,
> +};
> +
> +static int tgu_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> +	int ret = 0;
> +	struct device *dev = &adev->dev;
> +	struct coresight_platform_data *pdata;
> +	struct tgu_drvdata *drvdata;
> +	struct coresight_desc desc = { 0 };

Code is quite mixed here... Bring some order - declarations with and
without assignments.

> +
> +	desc.name = coresight_alloc_device_name(&tgu_devs, dev);
> +	if (!desc.name)
> +		return -ENOMEM;
> +
> +	pdata = coresight_get_platform_data(dev);
> +	if (IS_ERR(pdata))
> +		return PTR_ERR(pdata);
> +
> +	adev->dev.platform_data = pdata;
> +
> +	drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +	if (!drvdata)
> +		return -ENOMEM;
> +
> +	drvdata->dev = &adev->dev;
> +	dev_set_drvdata(dev, drvdata);
> +
> +	drvdata->base = devm_ioremap_resource(dev, &adev->res);
> +	if (!drvdata->base)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&drvdata->spinlock);
> +
> +	drvdata->enable = false;
> +	desc.type = CORESIGHT_DEV_TYPE_HELPER;
> +	desc.pdata = adev->dev.platform_data;
> +	desc.dev = &adev->dev;
> +	desc.ops = &tgu_ops;
> +	desc.groups = tgu_attr_groups;
> +
> +	drvdata->csdev = coresight_register(&desc);
> +	if (IS_ERR(drvdata->csdev)) {
> +		ret = PTR_ERR(drvdata->csdev);
> +		goto err;
> +	}
> +
> +	pm_runtime_put(&adev->dev);
> +	dev_dbg(dev, "TGU initialized\n");

Drop, useless. Kernel provides you already ways to know probe status.

> +	return 0;
> +err:
> +	pm_runtime_put(&adev->dev);
> +	return ret;
> +}
> +
> +static struct amba_id tgu_ids[] = {

Not const?

> +	{
> +		.id = 0x0003b999,
> +		.mask = 0x0003ffff,
> +		.data = "TGU",
> +	},
> +	{ 0, 0 },
> +};

No module device table?

> +
> +static struct amba_driver tgu_driver = {
> +	.drv = {
> +			.name = "coresight-tgu",
> +			.owner = THIS_MODULE,

Please drop. Also one-less indentation.

> +			.suppress_bind_attrs = true,
> +		},
> +	.probe = tgu_probe,
> +	.id_table = tgu_ids,
> +};
> +
> +module_amba_driver(tgu_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("CoreSight TGU driver");
Best regards,
Krzysztof