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

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

On Mon 10 Feb 11:06 PST 2020, Nikolay Borisov wrote:

> 
> 
> On 10.02.20 ??. 20:57 ??., Bjorn Andersson wrote:
> > 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.
> 
> alphabetically or reverse christmas tree?
> 

Alphabetically please.

> > 
> >> +
> 
> <snip>
> >> +	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.
> 
> This is my first rodeo in device driver land so I'm learning. It looks
> like what you want here is similar to what there is in
> sprd_hwspinlock.c. Will rework it.
> 

Exactly like that, forgot that we had an example of this in the sprd
driver. That will ensure that we rely solely on devres to unroll the
resources in a suitable order.

Regards,
Bjorn

> > 
> >> +
> >> +	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