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
signature.asc
Description: PGP signature
