Hi Neil,

Am Donnerstag, den 03.03.2016, 12:39 +0100 schrieb Neil Armstrong:

I'd prefer not to select MFD_SYSCON here, but rather let ARCH_OXNAS do
that.

Is there any need to include linux/io.h ?

+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/types.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+/* Regmap offsets */
+#define RST_SET_REGOFFSET	0x34
+#define RST_CLR_REGOFFSET	0x38
+
+struct oxnas_reset {
+	struct regmap *regmap;
+	struct reset_controller_dev rcdev;
+};
+
+static int oxnas_reset_reset(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct oxnas_reset *data =
+		container_of(rcdev, struct oxnas_reset, rcdev);
+
+	regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
+	msleep(50);

Is this the right delay for all of the resets in this register?
If not, I'd drop the .reset callback.

+	regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
+
+	return 0;
+}
+
+static int oxnas_reset_assert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct oxnas_reset *data =
+		container_of(rcdev, struct oxnas_reset, rcdev);
+
+	regmap_write(data->regmap, RST_SET_REGOFFSET, BIT(id));
+
+	return 0;
+}
+
+static int oxnas_reset_deassert(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct oxnas_reset *data =
+		container_of(rcdev, struct oxnas_reset, rcdev);
+
+	regmap_write(data->regmap, RST_CLR_REGOFFSET, BIT(id));
+
+	return 0;
+}
+
+static struct reset_control_ops oxnas_reset_ops = {

const

+	.reset		= oxnas_reset_reset,
+	.assert		= oxnas_reset_assert,
+	.deassert	= oxnas_reset_deassert,
+};
+
+static const struct of_device_id oxnas_reset_dt_ids[] = {
+	 { .compatible = "plxtech,nas782x-reset", },
+	 { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, oxnas_reset_dt_ids);
+
+static int oxnas_reset_probe(struct platform_device *pdev)
+{
+	struct oxnas_reset *data;
+	struct device *parent;
+
+	parent = pdev->dev.parent;
+	if (!parent) {
+		dev_err(&pdev->dev, "no parent\n");

Can this even happen?

+		return -ENODEV;
+	}
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = syscon_node_to_regmap(parent->of_node);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&pdev->dev, "failed to get parent regmap\n");
+		return -ENODEV;

Better print the error code and return it.

+	}
+
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.nr_resets = 32;
+	data->rcdev.ops = &oxnas_reset_ops;
+	data->rcdev.of_node = pdev->dev.of_node;
+	reset_controller_register(&data->rcdev);

Move this down a bit:

+
+	platform_set_drvdata(pdev, data);
+
+	return 0;

and
	return reset_controller_register(&data->rcdev);
here.

+}
+
+static int oxnas_reset_remove(struct platform_device *pdev)
+{
+	struct oxnas_reset *data = platform_get_drvdata(pdev);
+
+	reset_controller_unregister(&data->rcdev);
+
+	return 0;
+}
+
+static struct platform_driver oxnas_reset_driver = {
+	.probe	= oxnas_reset_probe,
+	.remove	= oxnas_reset_remove,
+	.driver = {
+		.name		= "oxnas-reset",
+		.owner		= THIS_MODULE,

The .owner field is overwritten by __platform_driver_register() anyway,
just drop it.

+		.of_match_table	= oxnas_reset_dt_ids,
+	},
+};
+
+static int __init oxnas_reset_init(void)
+{
+	return platform_driver_probe(&oxnas_reset_driver,
+				     oxnas_reset_probe);
+}
+
+/*
+ * Reset controller does not support probe deferral, so it has to be
+ * initialized before any user, in particular, PCIE uses subsys_initcall.
+ */
+arch_initcall(oxnas_reset_init);

That doesn't sound right. (of_)reset_control_get return -EPROBE_DEFER if
the rcdev isn't found in the list. Could you elaborate on this?

regards
Philipp