Re: [PATCH 1/3] hwspinlock: sunxi: Implement support for Allwinner's A64 SoC

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Mon 10 Feb 09:01 PST 2020, Nikolay Borisov wrote:
[..]
> diff --git a/drivers/hwspinlock/sunxi_hwspinlock.c b/drivers/hwspinlock/sunxi_hwspinlock.c
> new file mode 100644
> index 000000000000..8e5685357fbf
> --- /dev/null
> +++ b/drivers/hwspinlock/sunxi_hwspinlock.c
> @@ -0,0 +1,181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Author: Nikolay Borisov <nborisov@suse.com> */
> +
> +#include <linux/clk.h>
> +#include <linux/reset.h>
> +#include <linux/hwspinlock.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>

Please sort these.

> +
> +#include "hwspinlock_internal.h"
> +
> +/* Spinlock register offsets */
> +#define LOCK_BASE_OFFSET		0x0100
> +
> +#define SPINLOCK_NUMLOCKS_BIT_OFFSET	(28)
> +/* Possible values of SPINLOCK_LOCK_REG */
> +#define SPINLOCK_NOTTAKEN		(0)	/* free */
> +#define SPINLOCK_TAKEN			(1)	/* locked */
> +
> +struct sunxi_hwspinlock {
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	struct hwspinlock_device bank;
> +};
> +
> +static int sunxi_hwspinlock_trylock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* attempt to acquire the lock by reading its value */
> +	return (SPINLOCK_NOTTAKEN == readl(lock_addr));

Please drop the parenthesis and flip the expression around, i.e.
variable == constant.

> +}
> +
> +static void sunxi_hwspinlock_unlock(struct hwspinlock *lock)
> +{
> +	void __iomem *lock_addr = lock->priv;
> +
> +	/* release the lock by writing 0 to it */
> +	writel(SPINLOCK_NOTTAKEN, lock_addr);
> +}
> +
> +static const struct hwspinlock_ops sunxi_hwspinlock_ops = {
> +	.trylock	= sunxi_hwspinlock_trylock,
> +	.unlock		= sunxi_hwspinlock_unlock,
> +};
> +
> +static int sunxi_get_num_locks(void __iomem *io_base)
> +{
> +	int i = readl(io_base);
> +	i >>= SPINLOCK_NUMLOCKS_BIT_OFFSET;

Please make i u32.

> +
> +	switch (i) {
> +	case 0x1: return 32;
> +	case 0x2: return 64;
> +	case 0x3: return 128;
> +	case 0x4: return 256;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sunxi_hwspinlock_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw;
> +	void __iomem *io_base;
> +	struct resource *res;
> +	struct clk *clk;
> +	struct reset_control *reset;
> +	int i, ret, num_locks;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	io_base = devm_ioremap_resource(&pdev->dev, res);

Please use devm_platform_ioremap_resource()

> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	/*
> +	 * make sure the module is enabled and clocked before reading
> +	 * the module SYSSTATUS register
> +	 */
> +	clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot enable clock\n");
> +		return ret;
> +	}
> +
> +	/* Disable soft reset */
> +        reset= devm_reset_control_get_exclusive(&pdev->dev, NULL);
> +        if (IS_ERR(reset)) {
> +                ret = PTR_ERR(reset);
> +                goto out_declock;
> +        }
> +        reset_control_deassert(reset);

Indentation of this chunk looks off.

> +
> +	num_locks = sunxi_get_num_locks(io_base);
> +	if (!num_locks) {
> +		dev_err(&pdev->dev, "Unrecognised sunxi hwspinlock device\n");
> +		ret = -EINVAL;
> +		goto out_reset;
> +	}
> +
> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw) +
> +			  num_locks * sizeof(struct hwspinlock), GFP_KERNEL);

Please use struct_size() to calculate the size here.

> +	if (!hw) {
> +		ret = -ENOMEM;
> +		goto out_reset;
> +	}
> +
> +	hw->clk = clk;
> +	hw->reset = reset;
> +
> +	io_base += LOCK_BASE_OFFSET;
> +	for (i = 0; i < num_locks; i++)
> +		hw->bank.lock[i].priv = io_base + i * sizeof(u32);
> +
> +	platform_set_drvdata(pdev, hw);
> +
> +	ret = hwspin_lock_register(&hw->bank, &pdev->dev, &sunxi_hwspinlock_ops,
> +				   0, num_locks);

People will likely send patches to replace this with
devm_hwspin_lock_register(), but that will create an invalid ordering
between the clock disable, reset assert and the hwspinlock
unregistration.

You could deal with this using devm_add_action() and
devm_add_action_or_reset() for the clock and reset above. That will save
us future churn, would clean up your error handling and you could drop
the remove function completely.

> +
> +	if (!ret)
> +		return ret;
> +out_reset:
> +	reset_control_assert(reset);
> +out_declock:
> +	clk_disable_unprepare(clk);
> +	return ret;
> +}
> +
> +static int sunxi_hwspinlock_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_hwspinlock *hw = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = hwspin_lock_unregister(&hw->bank);
> +	if (ret)
> +		dev_err(&pdev->dev, "%s failed: %d\n", __func__, ret);
> +
> +	reset_control_assert(hw->reset);
> +	clk_disable_unprepare(hw->clk);
> +
> +	return 0;
> +}
> +

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel