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

[Krzysztof Kozlowski <krzk@kernel.org>]

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@quicinc.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