Re: [PATCH v3 4/8] reset: Add Sunplus SP7021 reset driver

On Mon, 2021-11-01 at 13:01 +0800, Qin Jian wrote:

Add reset driver for Sunplus SP7021 SoC.

You don't mention Q645 here, it appears this driver supports both SoCs?

[...]

Please add these entries in alphabetical order.

+	bool "Sunplus SoCs Reset Driver"

Can this be made:

	depends SOC_SP7021 || SOC_Q645 || COMPILE_TEST

?

+	help
+	  This enables the reset driver support for Sunplus SP7021 SoC family.
+	  Say Y if you want to control reset signals by the reset controller.
+	  Otherwise, say N.
+	  This driver is selected automatically by platform config.

Which platform config?


[...]

Drop this boilerplate, this is not required with the SPDX identifier
above.

+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/reboot.h>
+
+#if defined(CONFIG_SOC_SP7021)
+#include <dt-bindings/reset/sp-sp7021.h>
+#elif defined(CONFIG_SOC_Q645)
+#include <dt-bindings/reset/sp-q645.h>
+#endif

I'd prefer if you added namespace prefixes to the defines and included
both headers unconditionally.
These are just required for RST_MAX, correct?

+
+#define BITASSERT(id, val)          ((1 << (16 + id)) | (val << id))
+
+
+struct sp_reset_data {
+	struct reset_controller_dev	rcdev;
+	void __iomem			*membase;
+} sp_reset;

Please allocate this with devm_kzalloc in the probe function instead.

+
+
+static inline struct sp_reset_data *
+to_sp_reset_data(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct sp_reset_data, rcdev);
+}
+
+static int sp_reset_update(struct reset_controller_dev *rcdev,
+			      unsigned long id, bool assert)
+{
+	struct sp_reset_data *data = to_sp_reset_data(rcdev);
+	int reg_width = sizeof(u32)/2;
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	void __iomem *addr;
+
+	addr = data->membase + (bank * 4);
+
+	if (assert)
+		writel(BITASSERT(offset, 1), addr);
+	else
+		writel(BITASSERT(offset, 0), addr);

Could be

	writel(BITASSERT(offset, assert), addr);

+
+	return 0;
+}
+
+static int sp_reset_assert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	return sp_reset_update(rcdev, id, true);
+}
+
+
+static int sp_reset_deassert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	return sp_reset_update(rcdev, id, false);
+}
+
+static int sp_reset_status(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct sp_reset_data *data = to_sp_reset_data(rcdev);
+	int reg_width = sizeof(u32)/2;
+	int bank = id / (reg_width * BITS_PER_BYTE);
+	int offset = id % (reg_width * BITS_PER_BYTE);
+	u32 reg;
+
+	reg = readl(data->membase + (bank * 4));
+
+	return !!(reg & BIT(offset));
+}
+
+static int sp_restart(struct notifier_block *this, unsigned long mode,
+				void *cmd)
+{
+	sp_reset_assert(&sp_reset.rcdev, RST_SYSTEM);
+	sp_reset_deassert(&sp_reset.rcdev, RST_SYSTEM);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block sp_restart_nb = {
+	.notifier_call = sp_restart,
+	.priority = 192,
+};
+
+static const struct reset_control_ops sp_reset_ops = {
+	.assert		= sp_reset_assert,
+	.deassert	= sp_reset_deassert,
+	.status		= sp_reset_status,
+};
+
+static const struct of_device_id sp_reset_dt_ids[] = {
+	{ .compatible = "sunplus,sp7021-reset", },
+	{ .compatible = "sunplus,q645-reset", },
+	{ /* sentinel */ },
+};
+
+static int sp_reset_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sp_reset_data *data = &sp_reset;
+	void __iomem *membase;
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	membase = devm_ioremap(dev, res->start, resource_size(res));
+	if (IS_ERR(membase))
+		return PTR_ERR(membase);
+
+	data->membase = membase;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = RST_MAX;

Use of_device_get_match_data() to determine the correct number of
resets.

regards
Philipp