Re: [PATCH v3 2/2] hwspinlock: add sun8i hardware spinlock support

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 3615 bytes --]

On Mon, Dec 07, 2020 at 05:05:34PM +0100, Wilken Gottwalt wrote:
> +	io_base = devm_platform_ioremap_resource(pdev, SPINLOCK_BASE_ID);
> +	if (IS_ERR(io_base)) {
> +		err = PTR_ERR(io_base);
> +		dev_err(&pdev->dev, "unable to request MMIO (%d)\n", err);

There's already a message printed by the core if it fails

> +		return err;
> +	}
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	err = devm_add_action_or_reset(&pdev->dev, sun8i_hwspinlock_disable, priv);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to add disable action\n");
> +		return err;
> +	}

If the next call fails, you're going to free some resources that you
haven't taken in the first place.

> +
> +	priv->ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> +	if (IS_ERR(priv->ahb_clk)) {
> +		err = PTR_ERR(priv->ahb_clk);
> +		dev_err(&pdev->dev, "unable to get AHB clock (%d)\n", err);
> +		return err;
> +	}
> +
> +	priv->reset = devm_reset_control_get_optional(&pdev->dev, "ahb");

Your binding has it mandatory, so you don't really need it to be
optional?

> +	if (IS_ERR(priv->reset)) {
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset),
> +				     "unable to get reset control\n");
> +	}

You shouldn't have braces on a single line return

> +
> +	err = reset_control_deassert(priv->reset);
> +	if (err) {
> +		dev_err(&pdev->dev, "deassert reset control failure (%d)\n", err);
> +		return err;
> +	}
> +
> +	err = clk_prepare_enable(priv->ahb_clk);
> +	if (err) {
> +		dev_err(&pdev->dev, "unable to prepare AHB clk (%d)\n", err);
> +		return err;
> +	}
> +
> +	/*
> +	 * bit 28 and 29 hold the amount of spinlock banks, but at the same time the datasheet
> +	 * says, bit 30 and 31 are reserved while the values can be 0 to 4, which is not reachable
> +	 * by two bits alone, so the reserved bits are also taken into account
> +	 */
> +	num_banks = readl(io_base + SPINLOCK_SYSSTATUS_REG) >> 28;
> +	switch (num_banks) {
> +	case 1 ... 4:
> +		/*
> +		 * 32, 64, 128 and 256 spinlocks are supported by the hardware implementation,
> +		 * though most of the SoCs support 32 spinlocks only
> +		 */
> +		priv->nlocks = 1 << (4 + num_banks);
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "unsupported hwspinlock setup (%d)\n", num_banks);
> +		return -EINVAL;
> +	}
> +
> +	priv->bank = devm_kzalloc(&pdev->dev, struct_size(priv->bank, lock, priv->nlocks),
> +				  GFP_KERNEL);
> +	if (!priv->bank)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < priv->nlocks; ++i) {
> +		hwlock = &priv->bank->lock[i];
> +		hwlock->priv = io_base + SPINLOCK_LOCK_REGN + sizeof(u32) * i;
> +	}
> +
> +	sun8i_hwspinlock_debugfs_init(priv);
> +	platform_set_drvdata(pdev, priv);
> +
> +	return devm_hwspin_lock_register(&pdev->dev, priv->bank, &sun8i_hwspinlock_ops,
> +					 SPINLOCK_BASE_ID, priv->nlocks);
> +}
> +
> +static const struct of_device_id sun8i_hwspinlock_ids[] = {
> +	{ .compatible = "allwinner,sun8i-hwspinlock", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_hwspinlock_ids);
> +
> +static struct platform_driver sun8i_hwspinlock_driver = {
> +	.probe	= sun8i_hwspinlock_probe,
> +	.driver	= {
> +		.name		= DRIVER_NAME,
> +		.of_match_table	= sun8i_hwspinlock_ids,
> +	},
> +};
> +
> +static int __init sun8i_hwspinlock_init(void)
> +{
> +	return platform_driver_register(&sun8i_hwspinlock_driver);
> +}
> +module_init(sun8i_hwspinlock_init);
> +
> +static void __exit sun8i_hwspinlock_exit(void)
> +{
> +	platform_driver_unregister(&sun8i_hwspinlock_driver);
> +}
> +module_exit(sun8i_hwspinlock_exit);

This can be replaced by module_platform_driver

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]