Add support for the System Power Management Interface (SPMI)

From: Sylwester Nawrocki <[email protected]>

Add this empty macro definition so users can be compiled without excluding this macro call with preprocessor directives when CONFIG_OF is disabled.

Signed-off-by Sylwester Nawrocki <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

From: Kenneth Heitke <[email protected]>

System Power Management Interface (SPMI) is a specification developed by the MIPI (Mobile Industry Process Interface) Alliance optimized for the real time control of Power Management ICs (PMIC).

SPMI is a two-wire serial interface that supports up to 4 master devices and up to 16 logical slaves.

The framework supports message APIs, multiple busses (1 controller per bus) and multiple clients/slave devices per controller.

Signed-off-by Kenneth Heitke <[email protected]>

Signed-off-by Michael Bohan <[email protected]>

Signed-off-by Josh Cartwright <[email protected]>

---
 drivers/Kconfig                 |   2 +
 drivers/Makefile                |   1 +
 drivers/spmi/Kconfig            |   9 +
 drivers/spmi/Makefile           |   4 +
 drivers/spmi/spmi.c             | 491 ++++++++++++++++++++++++++++++++++++++++
 include/dt-bindings/spmi/spmi.h |  18 ++
 include/linux/mod_devicetable.h |   8 +
 include/linux/spmi.h            | 342 ++++++++++++++++++++++++++++
 8 files changed, 875 insertions(+)
 create mode 100644 drivers/spmi/Kconfig
 create mode 100644 drivers/spmi/Makefile
 create mode 100644 drivers/spmi/spmi.c
 create mode 100644 include/dt-bindings/spmi/spmi.h
 create mode 100644 include/linux/spmi.h

drivers/spmi/spmi.c new file +31 -0

+
+	if (pm)
+		return pm_generic_suspend(dev);
+	else
+		return 0;
+}
+
+static int spmi_pm_resume(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (pm)
+		return pm_generic_resume(dev);
+	else
+		return 0;
+}
+#else
+#define spmi_pm_suspend		NULL
+#define spmi_pm_resume		NULL
+#endif
+
+static const struct dev_pm_ops spmi_pm_ops = {
+	.suspend	= spmi_pm_suspend,
+	.resume		= spmi_pm_resume,
+	SET_RUNTIME_PM_OPS(
+		pm_generic_suspend,
+		pm_generic_resume,
+		pm_generic_runtime_idle
+	)
+};
+

drivers/spmi/spmi.c new file +18 -0

+
+static int spmi_drv_probe(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->probe)
+		err = sdrv->probe(to_spmi_device(dev));
+
+	return err;
+}
+
+static int spmi_drv_remove(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->remove)

drivers/spmi/spmi.c new file +12 -0

+		sdrv->shutdown(to_spmi_device(dev));
+}
+
+static struct bus_type spmi_bus_type = {
+	.name		= "spmi",
+	.match		= spmi_device_match,
+	.pm		= &spmi_pm_ops,
+	.probe		= spmi_drv_probe,
+	.remove		= spmi_drv_remove,
+	.shutdown	= spmi_drv_shutdown,
+};
+

drivers/spmi/spmi.c new file +15 -0

+
+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+	struct device_node *node;
+	int err;
+
+	dev_dbg(&ctrl->dev, "of_spmi_register_devices\n");
+
+	if (!ctrl->dev.of_node)
+		return -ENODEV;
+
+	dev_dbg(&ctrl->dev, "looping through children\n");
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		struct spmi_device *sdev;

drivers/spmi/spmi.c new file +41 -0

+		}
+
+		if (reg[1] != SPMI_USID) {
+			dev_err(&ctrl->dev,
+				"node %s contains unsupported 'reg' entry\n",
+				node->full_name);
+			continue;
+		}
+
+		if (reg[0] > 0xF) {
+			dev_err(&ctrl->dev,
+				"invalid usid on node %s\n",
+				node->full_name);
+			continue;
+		}
+
+		dev_dbg(&ctrl->dev, "read usid %02x\n", reg[0]);
+
+		sdev = spmi_device_alloc(ctrl);
+		if (!sdev)
+			continue;
+
+		sdev->dev.of_node = node;
+		sdev->usid = (u8) reg[0];
+
+		err = spmi_device_add(sdev);
+		if (err) {
+			dev_err(&sdev->dev,
+				"failure adding device. status %d\n", err);
+			spmi_device_put(sdev);
+			continue;
+		}
+	}
+
+	return 0;
+}
+
+int spmi_controller_add(struct spmi_controller *ctrl)
+{
+	int ret;
+

Quoted 15 lines · Josh Cartwright

From: Kenneth Heitke <[email protected]>

System Power Management Interface (SPMI) is a specification developed by the MIPI (Mobile Industry Process Interface) Alliance optimized for the real time control of Power Management ICs (PMIC).

SPMI is a two-wire serial interface that supports up to 4 master devices and up to 16 logical slaves.

The framework supports message APIs, multiple busses (1 controller per bus) and multiple clients/slave devices per controller.

Signed-off-by Kenneth Heitke <[email protected]>

Signed-off-by Michael Bohan <[email protected]>

Signed-off-by Josh Cartwright <[email protected]>

Quoted 22 lines

drivers/spmi/spmi.c new file +22 -0

+static int spmi_drv_probe(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->probe)
+		err = sdrv->probe(to_spmi_device(dev));
+
+	return err;
+}
+
+static int spmi_drv_remove(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->remove)
+		err = sdrv->remove(to_spmi_device(dev));
+
+	return err;
+}
+

drivers/spmi/spmi.c line 287

Quoted 5 lines

drivers/spmi/spmi.c new file +5 -0

+static void spmi_drv_shutdown(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+
+	if (sdrv->shutdown)

If driver for device is not loaded this will cause kernel NULL pointer dereference.

Quoted 18 lines

drivers/spmi/spmi.c new file +18 -0

+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+	struct device_node *node;
+	int err;
+
+	dev_dbg(&ctrl->dev, "of_spmi_register_devices\n");
+
+	if (!ctrl->dev.of_node)
+		return -ENODEV;
+
+	dev_dbg(&ctrl->dev, "looping through children\n");
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		struct spmi_device *sdev;
+		u32 reg[2];
+
+		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
+

drivers/spmi/spmi.c line 381

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+		err = of_property_read_u32_array(node, "reg", reg, 2);
+		if (err) {
+			dev_err(&ctrl->dev,
+				"node %s does not have 'reg' property\n",
+				node->full_name);
+			continue;

Shouldn't this be a fatal error?

Quoted 45 lines

drivers/spmi/spmi.c new file +45 -0

+		}
+
+		if (reg[1] != SPMI_USID) {
+			dev_err(&ctrl->dev,
+				"node %s contains unsupported 'reg' entry\n",
+				node->full_name);
+			continue;
+		}
+
+		if (reg[0] > 0xF) {
+			dev_err(&ctrl->dev,
+				"invalid usid on node %s\n",
+				node->full_name);
+			continue;
+		}
+
+		dev_dbg(&ctrl->dev, "read usid %02x\n", reg[0]);
+
+		sdev = spmi_device_alloc(ctrl);
+		if (!sdev)
+			continue;
+
+		sdev->dev.of_node = node;
+		sdev->usid = (u8) reg[0];
+
+		err = spmi_device_add(sdev);
+		if (err) {
+			dev_err(&sdev->dev,
+				"failure adding device. status %d\n", err);
+			spmi_device_put(sdev);
+			continue;
+		}
+	}
+
+	return 0;
+}
+
+int spmi_controller_add(struct spmi_controller *ctrl)
+{
+	int ret;
+
+	/* Can't register until after driver model init */
+	if (WARN_ON(!spmi_bus_type.p))
+		return -EAGAIN;
+

drivers/spmi/spmi.c line 432

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+	ret = device_add(&ctrl->dev);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_OF))
+		of_spmi_register_devices(ctrl);

Check for error code here?

Quoted 16 lines · Ivan T. Ivanov

Quoted 15 lines · Josh Cartwright

From: Kenneth Heitke <[email protected]>

System Power Management Interface (SPMI) is a specification developed by the MIPI (Mobile Industry Process Interface) Alliance optimized for the real time control of Power Management ICs (PMIC).

SPMI is a two-wire serial interface that supports up to 4 master devices and up to 16 logical slaves.

The framework supports message APIs, multiple busses (1 controller per bus) and multiple clients/slave devices per controller.

Signed-off-by Kenneth Heitke <[email protected]>

Signed-off-by Michael Bohan <[email protected]>

Signed-off-by Josh Cartwright <[email protected]>

[..]

Quoted 22 lines

Quoted 22 lines

drivers/spmi/spmi.c new file +22 -0

+static int spmi_drv_probe(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->probe)
+		err = sdrv->probe(to_spmi_device(dev));
+
+	return err;
+}
+
+static int spmi_drv_remove(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+	int err = 0;
+
+	if (sdrv->remove)
+		err = sdrv->remove(to_spmi_device(dev));
+
+	return err;
+}
+

Quoted 8 lines

drivers/spmi/spmi.c line 287

Quoted 5 lines

drivers/spmi/spmi.c new file +5 -0

+static void spmi_drv_shutdown(struct device *dev)
+{
+	const struct spmi_driver *sdrv = to_spmi_driver(dev->driver);
+
+	if (sdrv->shutdown)

If driver for device is not loaded this will cause kernel NULL pointer dereference.

Indeed. I'll fix this.

Quoted 18 lines

Quoted 18 lines

drivers/spmi/spmi.c new file +18 -0

+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+	struct device_node *node;
+	int err;
+
+	dev_dbg(&ctrl->dev, "of_spmi_register_devices\n");
+
+	if (!ctrl->dev.of_node)
+		return -ENODEV;
+
+	dev_dbg(&ctrl->dev, "looping through children\n");
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		struct spmi_device *sdev;
+		u32 reg[2];
+
+		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
+

Quoted 8 lines

drivers/spmi/spmi.c line 381

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+		err = of_property_read_u32_array(node, "reg", reg, 2);
+		if (err) {
+			dev_err(&ctrl->dev,
+				"node %s does not have 'reg' property\n",
+				node->full_name);
+			continue;

Shouldn't this be a fatal error?

Fatal in what way? It is fatal in the sense that this particular child node is skipped, but other children can still be enumerated.

Are you
suggesting that we bail completely when we hit a wrongly-described child?

Quoted 24 lines

Quoted 24 lines

drivers/spmi/spmi.c new file +23 -0

+		}
+
+		if (reg[1] != SPMI_USID) {
+			dev_err(&ctrl->dev,
+				"node %s contains unsupported 'reg' entry\n",
+				node->full_name);
+			continue;
+		}
+
+		if (reg[0] > 0xF) {
+			dev_err(&ctrl->dev,
+				"invalid usid on node %s\n",
+				node->full_name);
+			continue;
+		}
+
+		dev_dbg(&ctrl->dev, "read usid %02x\n", reg[0]);
+
+		sdev = spmi_device_alloc(ctrl);
+		if (!sdev)
+			continue;
+
+		sdev->dev.of_node = node;

Quoted 8 lines

drivers/spmi/spmi.c line 412

Quoted 8 lines

drivers/spmi/spmi.c new file +8 -0

+		sdev->usid = (u8) reg[0];
+
+		err = spmi_device_add(sdev);
+		if (err) {
+			dev_err(&sdev->dev,
+				"failure adding device. status %d\n", err);
+			spmi_device_put(sdev);
+			continue;

Quoted 14 lines

Quoted 14 lines

drivers/spmi/spmi.c new file +14 -0

+		}
+	}
+
+	return 0;
+}
+
+int spmi_controller_add(struct spmi_controller *ctrl)
+{
+	int ret;
+
+	/* Can't register until after driver model init */
+	if (WARN_ON(!spmi_bus_type.p))
+		return -EAGAIN;
+

Quoted 8 lines

drivers/spmi/spmi.c line 432

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+	ret = device_add(&ctrl->dev);
+	if (ret)
+		return ret;
+
+	if (IS_ENABLED(CONFIG_OF))
+		of_spmi_register_devices(ctrl);

Check for error code here?

And do what with it?

Maybe instead, I should make
of_spmi_register_devices() return void.

drivers/spmi/spmi.c new file +7 -0

+	else
+		return 0;
+}
+#else
+#define spmi_pm_suspend		NULL
+#define spmi_pm_resume		NULL
+#endif

drivers/spmi/spmi.c new file +4 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+

include/linux/spmi.h new file +4 -0

+{
+	if (sdev)
+		put_device(&sdev->dev);
+}

+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)

+static inline void spmi_controller_put(struct spmi_controller *ctrl)

include/linux/spmi.h new file +5 -0

+{
+	if (ctrl)
+		put_device(&ctrl->dev);
+}
+

Couple of high-level comments on the in-kernel API.

drivers/spmi/spmi.c line 58

Quoted 7 lines · Josh Cartwright

drivers/spmi/spmi.c new file +7 -0

+#ifdef CONFIG_PM_SLEEP
+static int spmi_pm_suspend(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (pm)
+		return pm_generic_suspend(dev);

pm_generic_suspend() checks both dev->driver and dev->driver->pm and returns 0 if either of them is NULL, so there should be no need to wrap the function.

drivers/spmi/spmi.c line 68

Quoted 10 lines

drivers/spmi/spmi.c new file +10 -0

+	else
+		return 0;
+}
+
+static int spmi_pm_resume(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+
+	if (pm)
+		return pm_generic_resume(dev);

Same here

drivers/spmi/spmi.c line 456

Quoted 7 lines

drivers/spmi/spmi.c new file +7 -0

+/**
+ * spmi_controller_remove: Controller tear-down.
+ * @ctrl: controller to be removed.
+ *
+ * Controller added with the above API is torn down using this API.
+ */
+int spmi_controller_remove(struct spmi_controller *ctrl)

The return type should be void. The function can't fail and nobody is going to check the return value anyway.

Quoted 5 lines

drivers/spmi/spmi.c new file +5 -0

+{
+	int dummy;
+
+	if (!ctrl)
+		return -EINVAL;

drivers/spmi/spmi.c line 465

Quoted 4 lines

drivers/spmi/spmi.c new file +4 -0

+
+	dummy = device_for_each_child(&ctrl->dev, NULL,
+				      spmi_ctrl_remove_device);
+	device_unregister(&ctrl->dev);

Should be device_del(). device_unregister() will do both device_del() and put_device(). But usually you'd want to do something in between like release resources used by the controller.

Quoted 4 lines

drivers/spmi/spmi.c new file +4 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+

[...]

include/linux/spmi.h line 76

Quoted 10 lines

include/linux/spmi.h new file +10 -0

+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)

For symmetry reasons it might make sense to call this spmi_device_free().

Quoted 4 lines

include/linux/spmi.h new file +4 -0

+{
+	if (sdev)
+		put_device(&sdev->dev);
+}

[...]

Quoted 1 line

+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)

Should be a inline function for better type safety.

[...]

Quoted 1 line

+static inline void spmi_controller_put(struct spmi_controller *ctrl)

For symmetry reasons it might make sense to call this spmi_controller_free().

Quoted 5 lines

include/linux/spmi.h new file +5 -0

+{
+	if (ctrl)
+		put_device(&ctrl->dev);
+}
+

[....]

include/linux/spmi.h line 171

Quoted 4 lines

include/linux/spmi.h new file +4 -0

+struct spmi_driver {
+	struct device_driver driver;
+	int	(*probe)(struct spmi_device *sdev);
+	int	(*remove)(struct spmi_device *sdev);

The type of the remove function should be found. The Linux device model doesn't really allow for device removal to fail.

include/linux/spmi.h line 174

Quoted 3 lines

include/linux/spmi.h new file +3 -0

+	void	(*shutdown)(struct spmi_device *sdev);
+	int	(*suspend)(struct spmi_device *sdev, pm_message_t pmesg);
+	int	(*resume)(struct spmi_device *sdev);

The framework seems to support dev_pm_ops just fine, there should be no need for legacy suspend/resume callbacks.

Quoted 3 lines

Quoted 2 lines

+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)

Should be a inline function for better type safety.

Sounds good. Will change the to_spmi_*() macros.

Quoted 4 lines · Josh Cartwright

Quoted 3 lines

Quoted 2 lines

+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)

Should be a inline function for better type safety.

Sounds good. Will change the to_spmi_*() macros.

I was under the impression that container_of() already does type checking. At least it will ensure that typeof(d) == typeof(dev) in the above example which is about as good as it can get.

Quoted 5 lines

Quoted 5 lines

drivers/spmi/spmi.c new file +5 -0

+{
+	int dummy;
+
+	if (!ctrl)
+		return -EINVAL;

Quoted 8 lines

drivers/spmi/spmi.c line 465

Quoted 4 lines

drivers/spmi/spmi.c new file +4 -0

+
+	dummy = device_for_each_child(&ctrl->dev, NULL,
+				      spmi_ctrl_remove_device);
+	device_unregister(&ctrl->dev);

Should be device_del(). device_unregister() will do both device_del() and put_device(). But usually you'd want to do something in between like release resources used by the controller.

I'm not sure I understand your suggestion here. If put_device() isn't called here, wouldn't we be leaking the controller? What resources would I want to be releasing here that aren't released as part of the controller's release() function?

Quoted 5 lines

Quoted 4 lines

drivers/spmi/spmi.c new file +4 -0

+	return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+

[...]

Quoted 12 lines

include/linux/spmi.h line 76

Quoted 10 lines

include/linux/spmi.h new file +10 -0

+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)

For symmetry reasons it might make sense to call this spmi_device_free().

Except that it doesn't necessarily free() the underlying device, so I find that more confusing.

drivers/spmi/Kconfig new file +4 -0

+	help
+	  SPMI (System Power Management Interface) is a two-wire
+	  serial interface between baseband and application processors
+	  and Power Management Integrated Circuits (PMIC).

drivers/spmi/spmi.c new file +26 -0

new file mode 100644
index 0000000..0780bd4
@@ -0,0 +1,491 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/idr.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/spmi.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+
+#include <dt-bindings/spmi/spmi.h>
+
+static DEFINE_MUTEX(ctrl_idr_lock);

drivers/spmi/spmi.c new file +3 -0

+	.release	= spmi_ctrl_release,
+};
+

drivers/spmi/spmi.c new file +7 -0

+		return -EINVAL;
+
+	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr, 0,
+			     buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_read);
+

drivers/spmi/spmi.c new file +15 -0

+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+					      size_t size)
+{
+	struct spmi_controller *ctrl;
+	int id;
+
+	if (WARN_ON(!parent))
+		return NULL;
+
+	ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	device_initialize(&ctrl->dev);
+	ctrl->dev.type = &spmi_ctrl_type;

drivers/spmi/spmi.c new file +12 -0

+
+	if (id < 0) {
+		dev_err(parent,
+			"unable to allocate SPMI controller identifier.\n");
+		spmi_controller_put(ctrl);
+		return NULL;
+	}
+
+	ctrl->nr = id;
+	dev_set_name(&ctrl->dev, "spmi-%d", id);
+
+	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);

drivers/spmi/spmi.c new file +18 -0

+
+	if (!ctrl->dev.of_node)
+		return -ENODEV;
+
+	dev_dbg(&ctrl->dev, "looping through children\n");
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		struct spmi_device *sdev;
+		u32 reg[2];
+
+		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
+
+		err = of_property_read_u32_array(node, "reg", reg, 2);
+		if (err) {
+			dev_err(&ctrl->dev,
+				"node %s does not have 'reg' property\n",
+				node->full_name);
+				continue;

drivers/spmi/spmi.c new file +6 -0

+			dev_err(&ctrl->dev,
+				"invalid usid on node %s\n",
+				node->full_name);
+			continue;
+		}
+

include/linux/spmi.h new file +32 -0

new file mode 100644
index 0000000..29cf0c9
@@ -0,0 +1,342 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef _LINUX_SPMI_H
+#define _LINUX_SPMI_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+
+/* Maximum slave identifier */
+#define SPMI_MAX_SLAVE_ID		16
+
+/* SPMI Commands */
+#define SPMI_CMD_EXT_WRITE		0x00
+#define SPMI_CMD_RESET			0x10
+#define SPMI_CMD_SLEEP			0x11
+#define SPMI_CMD_SHUTDOWN		0x12
+#define SPMI_CMD_WAKEUP			0x13
+#define SPMI_CMD_AUTHENTICATE		0x14
+#define SPMI_CMD_MSTR_READ		0x15
+#define SPMI_CMD_MSTR_WRITE		0x16
+#define SPMI_CMD_TRANSFER_BUS_OWNERSHIP	0x1A
+#define SPMI_CMD_DDB_MASTER_READ	0x1B

include/linux/spmi.h new file +16 -0

+ *  This is the client/device handle returned when a SPMI device
+ *  is registered with a controller.
+ *  Pointer to this structure is used by client-driver as a handle.
+ *  @dev: Driver model representation of the device.
+ *  @ctrl: SPMI controller managing the bus hosting this device.
+ *  @usid: Unique Slave IDentifier.
+ */
+struct spmi_device {
+	struct device		dev;
+	struct spmi_controller	*ctrl;
+	u8			usid;
+};
+#define to_spmi_device(d) container_of(d, struct spmi_device, dev)
+
+static inline void *spmi_device_get_drvdata(const struct spmi_device *sdev)
+{

include/linux/spmi.h new file +13 -0

+ * @ctrl: associated controller
+ *
+ * Caller is responsible for either calling spmi_device_add() to add the
+ * newly allocated controller, or calling spmi_device_put() to discard it.
+ */
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl);
+
+static inline void spmi_device_put(struct spmi_device *sdev)
+{
+	if (sdev)
+		put_device(&sdev->dev);
+}
+

include/linux/spmi.h new file +7 -0

+
+/**
+ * spmi_controller_alloc: Allocate a new SPMI controller
+ * @parent: parent device
+ * @size: size of private data
+ *
+ * Caller is responsible for either calling spmi_controller_add() to add the

include/linux/spmi.h new file +9 -0

+
+/**
+ * spmi_controller_add: Controller bring-up.
+ * @ctrl: controller to be registered.
+ *
+ * Register a controller previously allocated via spmi_controller_alloc() with
+ * the SPMI core
+ */
+int spmi_controller_add(struct spmi_controller *ctrl);

include/linux/spmi.h new file +8 -0

+
+/**
+ * spmi_driver_unregister - reverse effect of spmi_driver_register
+ * @sdrv: the driver to unregister
+ * Context: can sleep
+ */
+static inline void spmi_driver_unregister(struct spmi_driver *sdrv)
+{

+ * @sdev: SPMI device
+ * @addr: slave register address (5-bit address).
+ * @buf: buffer to be populated with data from the Slave.
+ *
+ * Reads 1 byte of data from a Slave device register.
+ */

include/linux/spmi.h new file +2 -0

+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf);
+

drivers/spmi/Kconfig line 5

Quoted 11 lines · Josh Cartwright

drivers/spmi/Kconfig new file +5 -0

new file mode 100644
index 0000000..a03835f
@@ -0,0 +1,9 @@
+#
+# SPMI driver configuration
+#
+menuconfig SPMI
+	bool "SPMI support"

Can we do tristate?

Quoted 36 lines

drivers/spmi/Kconfig new file +4 -0

+	help
+	  SPMI (System Power Management Interface) is a two-wire
+	  serial interface between baseband and application processors
+	  and Power Management Integrated Circuits (PMIC).

drivers/spmi/spmi.c new file +26 -0

new file mode 100644
index 0000000..0780bd4
@@ -0,0 +1,491 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/idr.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/spmi.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+
+#include <dt-bindings/spmi/spmi.h>
+
+static DEFINE_MUTEX(ctrl_idr_lock);

drivers/spmi/spmi.c line 36

Quoted 10 lines

drivers/spmi/spmi.c new file +10 -0

+static DEFINE_IDR(ctrl_idr);
+
+static void spmi_dev_release(struct device *dev)
+{
+	struct spmi_device *sdev = to_spmi_device(dev);
+	kfree(sdev);
+}
+
+static struct device_type spmi_dev_type = {
+	.release	= spmi_dev_release,

const?

Quoted 1 line

+static struct device_type spmi_ctrl_type = {

const?

drivers/spmi/spmi.c line 163

Quoted 4 lines

drivers/spmi/spmi.c new file +4 -0

+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+	/* 5-bit register address */
+	if (addr > 0x1F)

Perhaps 0x1f should be a #define.

Quoted 7 lines

drivers/spmi/spmi.c new file +7 -0

+		return -EINVAL;
+
+	return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr, 0,
+			     buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_read);
+

[...]

Quoted 15 lines

drivers/spmi/spmi.c new file +15 -0

+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+					      size_t size)
+{
+	struct spmi_controller *ctrl;
+	int id;
+
+	if (WARN_ON(!parent))
+		return NULL;
+
+	ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	device_initialize(&ctrl->dev);
+	ctrl->dev.type = &spmi_ctrl_type;

drivers/spmi/spmi.c line 341

Quoted 10 lines

drivers/spmi/spmi.c new file +10 -0

+	ctrl->dev.bus = &spmi_bus_type;
+	ctrl->dev.parent = parent;
+	ctrl->dev.of_node = parent->of_node;
+	spmi_controller_set_drvdata(ctrl, &ctrl[1]);
+
+	idr_preload(GFP_KERNEL);
+	mutex_lock(&ctrl_idr_lock);
+	id = idr_alloc(&ctrl_idr, ctrl, 0, 0, GFP_NOWAIT);
+	mutex_unlock(&ctrl_idr_lock);
+	idr_preload_end();

This can just be:

    mutex_lock(&ctrl_idr_lock);
    id = idr_alloc(&ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
    mutex_unlock(&ctrl_idr_lock);

Quoted 12 lines

drivers/spmi/spmi.c new file +12 -0

+
+	if (id < 0) {
+		dev_err(parent,
+			"unable to allocate SPMI controller identifier.\n");
+		spmi_controller_put(ctrl);
+		return NULL;
+	}
+
+	ctrl->nr = id;
+	dev_set_name(&ctrl->dev, "spmi-%d", id);
+
+	dev_dbg(&ctrl->dev, "allocated controller 0x%p id %d\n", ctrl, id);

drivers/spmi/spmi.c line 363

Quoted 10 lines

drivers/spmi/spmi.c new file +10 -0

+	return ctrl;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_alloc);
+
+static int of_spmi_register_devices(struct spmi_controller *ctrl)
+{
+	struct device_node *node;
+	int err;
+
+	dev_dbg(&ctrl->dev, "of_spmi_register_devices\n");

Could be

    dev_dbg(&ctrl->dev, "%s", __func__);

so that function renaming is transparent.

Quoted 22 lines

drivers/spmi/spmi.c new file +22 -0

+
+	if (!ctrl->dev.of_node)
+		return -ENODEV;
+
+	dev_dbg(&ctrl->dev, "looping through children\n");
+
+	for_each_available_child_of_node(ctrl->dev.of_node, node) {
+		struct spmi_device *sdev;
+		u32 reg[2];
+
+		dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name);
+
+		err = of_property_read_u32_array(node, "reg", reg, 2);
+		if (err) {
+			dev_err(&ctrl->dev,
+				"node %s does not have 'reg' property\n",
+				node->full_name);
+				continue;
+		}
+
+		if (reg[1] != SPMI_USID) {
+			dev_err(&ctrl->dev,

drivers/spmi/spmi.c line 391

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+				"node %s contains unsupported 'reg' entry\n",
+				node->full_name);
+			continue;
+		}
+
+		if (reg[0] > 0xF) {

Perhaps call this MAX_USID?

Quoted 6 lines

drivers/spmi/spmi.c new file +6 -0

+			dev_err(&ctrl->dev,
+				"invalid usid on node %s\n",
+				node->full_name);
+			continue;
+		}
+

[...]

Quoted 38 lines

include/linux/spmi.h new file +32 -0

new file mode 100644
index 0000000..29cf0c9
@@ -0,0 +1,342 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#ifndef _LINUX_SPMI_H
+#define _LINUX_SPMI_H
+
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/mod_devicetable.h>
+
+/* Maximum slave identifier */
+#define SPMI_MAX_SLAVE_ID		16
+
+/* SPMI Commands */
+#define SPMI_CMD_EXT_WRITE		0x00
+#define SPMI_CMD_RESET			0x10
+#define SPMI_CMD_SLEEP			0x11
+#define SPMI_CMD_SHUTDOWN		0x12
+#define SPMI_CMD_WAKEUP			0x13
+#define SPMI_CMD_AUTHENTICATE		0x14
+#define SPMI_CMD_MSTR_READ		0x15
+#define SPMI_CMD_MSTR_WRITE		0x16
+#define SPMI_CMD_TRANSFER_BUS_OWNERSHIP	0x1A
+#define SPMI_CMD_DDB_MASTER_READ	0x1B

include/linux/spmi.h line 42

Quoted 10 lines

include/linux/spmi.h new file +10 -0

+#define SPMI_CMD_DDB_SLAVE_READ		0x1C
+#define SPMI_CMD_EXT_READ		0x20
+#define SPMI_CMD_EXT_WRITEL		0x30
+#define SPMI_CMD_EXT_READL		0x38
+#define SPMI_CMD_WRITE			0x40
+#define SPMI_CMD_READ			0x60
+#define SPMI_CMD_ZERO_WRITE		0x80
+
+/**
+ * Client/device handle (struct spmi_device):

This isn't kernel-doc format.

Quoted 1 line

+ * spmi_controller_alloc: Allocate a new SPMI controller

There should be a dash here instead of colon.

include/linux/spmi.h line 140

Quoted 5 lines

include/linux/spmi.h new file +5 -0

+static inline void spmi_controller_put(struct spmi_controller *ctrl)
+{
+	if (ctrl)
+		put_device(&ctrl->dev);
+}

This function is missing documentation.

Quoted 2 lines

+/**
+ * spmi_register_read() - register read

This is right but then it's not consistent. These functions have () after them but higher up we don't have that.

Usually the prototypes aren't documented because people use tags and such to go to the definition of the function. I would prefer we put the documentation near the implementation so that 1) this file gives a high level overview of the API and 2) I don't have to double jump with tags to figure out what to pass to these functions.

Quoted 6 lines · Stephen Boyd

drivers/spmi/spmi.c line 163

Quoted 4 lines · Josh Cartwright

drivers/spmi/spmi.c new file +4 -0

+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+	/* 5-bit register address */
+	if (addr > 0x1F)

Perhaps 0x1f should be a #define.

Is 0x1F with the comment above it not clear enough?

Quoted 27 lines

Quoted 15 lines

drivers/spmi/spmi.c new file +15 -0

+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+					      size_t size)
+{
+	struct spmi_controller *ctrl;
+	int id;
+
+	if (WARN_ON(!parent))
+		return NULL;
+
+	ctrl = kzalloc(sizeof(*ctrl) + size, GFP_KERNEL);
+	if (!ctrl)
+		return NULL;
+
+	device_initialize(&ctrl->dev);
+	ctrl->dev.type = &spmi_ctrl_type;

drivers/spmi/spmi.c line 341

Quoted 10 lines

drivers/spmi/spmi.c new file +10 -0

+	ctrl->dev.bus = &spmi_bus_type;
+	ctrl->dev.parent = parent;
+	ctrl->dev.of_node = parent->of_node;
+	spmi_controller_set_drvdata(ctrl, &ctrl[1]);
+
+	idr_preload(GFP_KERNEL);
+	mutex_lock(&ctrl_idr_lock);
+	id = idr_alloc(&ctrl_idr, ctrl, 0, 0, GFP_NOWAIT);
+	mutex_unlock(&ctrl_idr_lock);
+	idr_preload_end();

This can just be:

Quoted 3 lines

    mutex_lock(&ctrl_idr_lock);
    id = idr_alloc(&ctrl_idr, ctrl, 0, 0, GFP_KERNEL);
    mutex_unlock(&ctrl_idr_lock);

Actually, I'm wondering if it's just easier to leverage the simpler ida_simple_* APIs instead.

From: Kenneth Heitke <[email protected]>

System Power Management Interface (SPMI) is a specification developed by the MIPI (Mobile Industry Process Interface) Alliance optimized for the real time control of Power Management ICs (PMIC).

SPMI is a two-wire serial interface that supports up to 4 master devices and up to 16 logical slaves.

The framework supports message APIs, multiple busses (1 controller per bus) and multiple clients/slave devices per controller.

+ */

drivers/spmi/spmi-pmic-arb.c new file +8 -0

+static void
+pa_write_data(struct spmi_pmic_arb_dev *dev, const u8 *buf, u32 reg, u8 bc)
+{
+	u32 data = 0;
+	memcpy(&data, buf, (bc & 3) + 1);
+	pmic_arb_base_write(dev, reg, data);
+}
+

drivers/spmi/spmi-pmic-arb.c new file +16 -0

+		return  -EINVAL;
+	}
+	dev_dbg(&ctrl->dev,
+		"op:0x%x sid:%d bc:%d addr:0x%x\n", opc, sid, bc, addr);
+
+	/* Check the opcode */
+	if (opc >= 0x60 && opc <= 0x7F)
+		opc = PMIC_ARB_OP_READ;
+	else if (opc >= 0x20 && opc <= 0x2F)
+		opc = PMIC_ARB_OP_EXT_READ;
+	else if (opc >= 0x38 && opc <= 0x3F)
+		opc = PMIC_ARB_OP_EXT_READL;
+	else
+		return -EINVAL;
+
+	cmd = (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);

drivers/spmi/spmi-pmic-arb.c new file +10 -0

+
+	if (bc > 3)
+		pa_read_data(pmic_arb, buf + 4,
+				PMIC_ARB_RDATA1(pmic_arb->channel), bc - 4);
+
+done:
+	spin_unlock_irqrestore(&pmic_arb->lock, flags);
+	return rc;
+}
+

drivers/spmi/spmi-pmic-arb.c new file +34 -0

+static int spmi_pmic_arb_probe(struct platform_device *pdev)
+{
+	struct spmi_pmic_arb_dev *pa;
+	struct spmi_controller *ctrl;
+	struct resource *res;
+	int err, i;
+
+	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
+	if (!ctrl)
+		return -ENOMEM;
+
+	pa = spmi_controller_get_drvdata(ctrl);
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
+	pa->base = devm_ioremap_resource(&ctrl->dev, res);
+	if (IS_ERR(pa->base)) {
+		err = PTR_ERR(pa->base);
+		goto err_put_ctrl;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
+	pa->intr = devm_ioremap_resource(&ctrl->dev, res);
+	if (IS_ERR(pa->intr)) {
+		err = PTR_ERR(pa->intr);
+		goto err_put_ctrl;
+	}
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg");
+	pa->cnfg = devm_ioremap_resource(&ctrl->dev, res);
+	if (IS_ERR(pa->cnfg)) {
+		err = PTR_ERR(pa->cnfg);
+		goto err_put_ctrl;
+	}
+

drivers/spmi/spmi-pmic-arb.c new file +10 -0

+
+	ctrl->cmd = pmic_arb_cmd;
+	ctrl->read_cmd = pmic_arb_read_cmd;
+	ctrl->write_cmd = pmic_arb_write_cmd;
+
+	err = spmi_controller_add(ctrl);
+	if (err)
+		goto err_put_ctrl;
+
+	dev_dbg(&ctrl->dev, "PMIC Arb Version 0x%x\n",

drivers/spmi/spmi-pmic-arb.c new file +5 -0

+{
+	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+	spmi_controller_remove(ctrl);
+	return 0;
+}

drivers/spmi/spmi-pmic-arb.c line 146

Quoted 6 lines · Josh Cartwright

drivers/spmi/spmi-pmic-arb.c new file +6 -0

+
+/**
+ * pa_write_data: write 1..4 bytes from buf to pmic-arb's register
+ * @bc byte-count -1. range: 0..3
+ * @reg register's address
+ * @buf buffer to write. length must be bc+1

Missing colon between variable and description.

drivers/spmi/spmi-pmic-arb.c line 233

Quoted 3 lines

drivers/spmi/spmi-pmic-arb.c new file +3 -0

+	if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
+		dev_err(&ctrl->dev,
+			"pmic-arb supports 1..%d bytes per trans, but:%d requested",

Nitpick: Please replace the colon between but and %d with a space.

Review

Quoted 1 line

(quoted diff) +1 -0

+			PMIC_ARB_MAX_TRANS_BYTES, bc+1);

Space around that '+' please.

Quoted 20 lines

drivers/spmi/spmi-pmic-arb.c new file +20 -0

+		return  -EINVAL;
+	}
+	dev_dbg(&ctrl->dev,
+		"op:0x%x sid:%d bc:%d addr:0x%x\n", opc, sid, bc, addr);
+
+	/* Check the opcode */
+	if (opc >= 0x60 && opc <= 0x7F)
+		opc = PMIC_ARB_OP_READ;
+	else if (opc >= 0x20 && opc <= 0x2F)
+		opc = PMIC_ARB_OP_EXT_READ;
+	else if (opc >= 0x38 && opc <= 0x3F)
+		opc = PMIC_ARB_OP_EXT_READL;
+	else
+		return -EINVAL;
+
+	cmd = (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
+
+	spin_lock_irqsave(&pmic_arb->lock, flags);
+	pmic_arb_base_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd);
+	rc = pmic_arb_wait_for_done(ctrl);

drivers/spmi/spmi-pmic-arb.c line 260

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c new file +6 -0

+	if (rc)
+		goto done;
+
+	/* Read from FIFO, note 'bc' is actually number of bytes minus 1 */
+	pa_read_data(pmic_arb, buf, PMIC_ARB_RDATA0(pmic_arb->channel)
+							, min_t(u8, bc, 3));

Nitpick: Weird comma starting a line here.

Quoted 1 line

+static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)

__exit shouldn't be here. We want this function in modules.

Quoted 5 lines

drivers/spmi/spmi-pmic-arb.c new file +5 -0

+{
+	struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+	spmi_controller_remove(ctrl);
+	return 0;
+}

drivers/spmi/spmi-pmic-arb.c line 396

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c new file +10 -0

+
+static struct of_device_id spmi_pmic_arb_match_table[] = {
+	{	.compatible = "qcom,spmi-pmic-arb", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, spmi_pmic_arb_match_table);
+
+static struct platform_driver spmi_pmic_arb_driver = {
+	.probe		= spmi_pmic_arb_probe,
+	.remove		= __exit_p(spmi_pmic_arb_remove),

Please drop this __exit_p() usage as well.

drivers/spmi/spmi-pmic-arb.c line 403

Quoted 7 lines

drivers/spmi/spmi-pmic-arb.c new file +7 -0

+	.driver		= {
+		.name	= "spmi_pmic_arb",
+		.owner	= THIS_MODULE,
+		.of_match_table = spmi_pmic_arb_match_table,
+	},
+};
+module_platform_driver(spmi_pmic_arb_driver);

MODULE_LICENSE()
MODULE_ALIAS()

drivers/spmi/spmi-pmic-arb.c +3 -0

+	struct irq_domain	*domain;
+	struct spmi_controller	*spmic;
+	u16			apid_to_ppid[256];
 };
 
 static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset)

drivers/spmi/spmi-pmic-arb.c +16 -0

+	u32 status;
+
+	chained_irq_enter(chip, desc);
+
+	for (i = first; i <= last; ++i) {
+		status = readl_relaxed(intr + SPMI_PIC_OWNER_ACC_STATUS(ee, i));
+		while (status) {
+			id = ffs(status) - 1;
+			status &= ~(1 << id);
+			periph_interrupt(pa, id + i * 32);
+		}
+	}
+
+	chained_irq_exit(chip, desc);
+}
+

drivers/spmi/spmi-pmic-arb.c +28 -0

+static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
+					   struct device_node *controller,
+					   const u32 *intspec,
+					   unsigned int intsize,
+					   unsigned long *out_hwirq,
+					   unsigned int *out_type)
+{
+	struct spmi_pmic_arb_dev *pa = d->host_data;
+	struct spmi_pmic_arb_irq_spec spec;
+	int err;
+	u8 apid;
+
+	dev_dbg(&pa->spmic->dev,
+		"intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
+		intspec[0], intspec[1], intspec[2]);
+
+	if (d->of_node != controller)
+		return -EINVAL;
+	if (intsize != 4)
+		return -EINVAL;
+	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
+		return -EINVAL;
+
+	spec.slave = intspec[0];
+	spec.per   = intspec[1];
+	spec.irq   = intspec[2];
+
+	err = search_mapping_table(pa, &spec, &apid);

drivers/spmi/spmi-pmic-arb.c +12 -0

+
+	*out_hwirq = spec.slave << 24
+		   | spec.per   << 16
+		   | spec.irq   << 8
+		   | apid;
+	*out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
+
+	dev_dbg(&pa->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
+
+	return 0;
+}
+

static int spmi_pmic_arb_probe(struct platform_device *pdev) {

drivers/spmi/spmi-pmic-arb.c +8 -1

 	struct spmi_pmic_arb_dev *pa;
 	struct spmi_controller *ctrl;
 	struct resource *res;
-	int err, i;
+	int err = 0, i;
 
 	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
 	if (!ctrl)
 		return -ENOMEM;
 
 	pa = spmi_controller_get_drvdata(ctrl);
+	pa->spmic = ctrl;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
 	pa->base = devm_ioremap_resource(&ctrl->dev, res);

@@ -349,6 +679,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
+	pa->irq = platform_get_irq(pdev, 0);
+	if (pa->irq < 0) {
+		err = pa->irq;
+		goto err_put_ctrl;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
 		pa->mapping_table[i] = readl_relaxed(
 				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));

drivers/spmi/spmi-pmic-arb.c +10 -1

+	}
+
+	irq_set_handler_data(pa->irq, pa);
+	irq_set_chained_handler(pa->irq, pmic_arb_chained_irq);
+
 	err = spmi_controller_add(ctrl);
 	if (err)
-		goto err_put_ctrl;
+		goto err_domain_remove;
 
 	dev_dbg(&ctrl->dev, "PMIC Arb Version 0x%x\n",
 		pmic_arb_base_read(pa, PMIC_ARB_VERSION));
 
 	return 0;
 
+err_domain_remove:
+	irq_set_chained_handler(pa->irq, NULL);
+	irq_set_handler_data(pa->irq, NULL);
+	irq_domain_remove(pa->domain);
 err_put_ctrl:
 	spmi_controller_put(ctrl);
 	return err;

drivers/spmi/spmi-pmic-arb.c struct spmi_pmic_arb_dev { · line 121

Quoted 12 lines · Josh Cartwright

drivers/spmi/spmi-pmic-arb.c +2 -0

@@ -108,12 +111,17 @@ struct spmi_pmic_arb_dev {
 	void __iomem		*base;
 	void __iomem		*intr;
 	void __iomem		*cnfg;
+	unsigned int		irq;
 	bool			allow_wakeup;
 	spinlock_t		lock;
 	u8			channel;
 	u8			min_apid;
 	u8			max_apid;
 	u32			mapping_table[SPMI_MAPPING_TABLE_LEN];
+	int			bus_nr;

This looks unused.

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +3 -0

+	struct irq_domain	*domain;
+	struct spmi_controller	*spmic;
+	u16			apid_to_ppid[256];
 };
 
 static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset)

[...]

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_write_cmd(struct spmi_controller *ctrl, · line 397

Quoted 10 lines

drivers/spmi/spmi-pmic-arb.c +10 -0

+
+static void pmic_arb_chained_irq(unsigned int irq, struct irq_desc *desc)
+{
+	struct spmi_pmic_arb_dev *pa = irq_get_handler_data(irq);
+	struct irq_chip *chip = irq_get_chip(irq);
+	void __iomem *intr = pa->intr;
+	int first = pa->min_apid >> 5;
+	int last = pa->max_apid >> 5;
+	int i, id;
+	u8 ee = 0; /*pa->owner;*/

TODO?

Quoted 34 lines

drivers/spmi/spmi-pmic-arb.c +34 -0

+static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
+					   struct device_node *controller,
+					   const u32 *intspec,
+					   unsigned int intsize,
+					   unsigned long *out_hwirq,
+					   unsigned int *out_type)
+{
+	struct spmi_pmic_arb_dev *pa = d->host_data;
+	struct spmi_pmic_arb_irq_spec spec;
+	int err;
+	u8 apid;
+
+	dev_dbg(&pa->spmic->dev,
+		"intspec[0] 0x%1x intspec[1] 0x%02x intspec[2] 0x%02x\n",
+		intspec[0], intspec[1], intspec[2]);
+
+	if (d->of_node != controller)
+		return -EINVAL;
+	if (intsize != 4)
+		return -EINVAL;
+	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
+		return -EINVAL;
+
+	spec.slave = intspec[0];
+	spec.per   = intspec[1];
+	spec.irq   = intspec[2];
+
+	err = search_mapping_table(pa, &spec, &apid);
+	if (err)
+		return err;
+
+	pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
+
+	/* Keep track of {max,min}_apid for bounding search during interrupt */

drivers/spmi/spmi-pmic-arb.c static int pmic_arb_write_cmd(struct spmi_controller *ctrl, · line 611

Quoted 4 lines

drivers/spmi/spmi-pmic-arb.c +4 -0

+	if (apid > pa->max_apid)
+		pa->max_apid = apid;
+	if (apid < pa->min_apid)
+		pa->min_apid = apid;

Ah makes sense now why we set this to the opposite values in probe. Please put a comment in patch 4 and maybe move that setup in patch 4 to this patch.

Quoted 12 lines

drivers/spmi/spmi-pmic-arb.c +12 -0

+
+	*out_hwirq = spec.slave << 24
+		   | spec.per   << 16
+		   | spec.irq   << 8
+		   | apid;
+	*out_type  = intspec[3] & IRQ_TYPE_SENSE_MASK;
+
+	dev_dbg(&pa->spmic->dev, "out_hwirq = %lu\n", *out_hwirq);
+
+	return 0;
+}
+

[...]

Quoted 34 lines

static int spmi_pmic_arb_probe(struct platform_device *pdev) {

drivers/spmi/spmi-pmic-arb.c +8 -1

 	struct spmi_pmic_arb_dev *pa;
 	struct spmi_controller *ctrl;
 	struct resource *res;
-	int err, i;
+	int err = 0, i;
 
 	ctrl = spmi_controller_alloc(&pdev->dev, sizeof(*pa));
 	if (!ctrl)
 		return -ENOMEM;
 
 	pa = spmi_controller_get_drvdata(ctrl);
+	pa->spmic = ctrl;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core");
 	pa->base = devm_ioremap_resource(&ctrl->dev, res);

@@ -349,6 +679,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 		goto err_put_ctrl;
 	}
 
+	pa->irq = platform_get_irq(pdev, 0);
+	if (pa->irq < 0) {
+		err = pa->irq;
+		goto err_put_ctrl;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
 		pa->mapping_table[i] = readl_relaxed(
 				pa->cnfg + SPMI_MAPPING_TABLE_REG(i));

@@ -364,15 +700,30 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
 	ctrl->read_cmd = pmic_arb_read_cmd;
 	ctrl->write_cmd = pmic_arb_write_cmd;
 

drivers/spmi/spmi-pmic-arb.c static int spmi_pmic_arb_probe(struct platform_device *pdev) · line 708

Quoted 6 lines

drivers/spmi/spmi-pmic-arb.c +6 -0

+	dev_dbg(&pdev->dev, "adding irq domain\n");
+	pa->domain = irq_domain_add_tree(pdev->dev.of_node,
+					 &pmic_arb_irq_domain_ops, pa);
+	if (!pa->domain) {
+		dev_err(&pdev->dev, "unable to create irq_domain\n");
+		goto err_put_ctrl;

Why do we silently ignore the error here? Is the irqchip functionality optional? Setting err here should allow us to skip intializing err to 0 up at the top of this function.

Signed-off-by Josh Cartwright <[email protected]>

---
 .../bindings/spmi/qcom,spmi-pmic-arb.txt           | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt new file +9 -0

new file mode 100644
index 0000000..68949aa
@@ -0,0 +1,42 @@
+Qualcomm SPMI Controller (PMIC Arbiter)
+
+The SPMI PMIC Arbiter is found on the Snapdragon 800 Series.  It is an SPMI
+controller with wrapping arbitration logic to allow for multiple on-chip
+devices to control a single SPMI master.
+
+The PMIC Arbiter can also act as an interrupt controller, providing interrupts
+to slave devices.
+

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt new file +23 -0

+- #size-cells : must be set to 0
+- interrupt-controller : indicates the PMIC arbiter is an interrupt controller
+- #interrupt-cells = <4>:  interrupts are specified as a 4-tuple:
+    cell 1: slave ID for the requested interrupt (0-15)
+    cell 2: peripheral ID for requested interrupt (0-255)
+    cell 3: the requested peripheral interrupt (0-7)
+    cell 4: interrupt flags indicating level-sense information, as defined in
+            dt-bindings/interrupt-controller/irq.h
+
+Example:
+
+	qcom,spmi at fc4c0000 {
+		compatible = "qcom,spmi-pmic-arb";
+		reg-names = "core", "intr", "cnfg";
+		reg = <0xfc4cf000 0x1000>,
+		      <0Xfc4cb000 0x1000>,
+		      <0Xfc4ca000 0x1000>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		interrupt-controller;
+		#interrupt-cells = <4>;
+	};

Quoted 26 lines · Josh Cartwright

Signed-off-by Josh Cartwright <[email protected]>

---
 .../bindings/spmi/qcom,spmi-pmic-arb.txt           | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt new file +14 -0

new file mode 100644
index 0000000..68949aa
@@ -0,0 +1,42 @@
+Qualcomm SPMI Controller (PMIC Arbiter)
+
+The SPMI PMIC Arbiter is found on the Snapdragon 800 Series.  It is an SPMI
+controller with wrapping arbitration logic to allow for multiple on-chip
+devices to control a single SPMI master.
+
+The PMIC Arbiter can also act as an interrupt controller, providing interrupts
+to slave devices.
+
+See spmi.txt for the generic SPMI controller binding requirements for child
+nodes.
+
+Required properties:
+- compatible : should be "qcom,spmi-pmic-arb".

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt line 19

Quoted 5 lines

Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt new file +5 -0

+- reg-names  : should be "core", "intr", "cnfg"
+- reg : offset and length of the PMIC Arbiter Core register map.
+- reg : offset and length of the PMIC Arbiter Interrupt controller register map.
+- reg : offset and length of the PMIC Arbiter Configuration register map.
+- #address-cells : must be set to 1

This doesn't seem to follow generic set of bindings for the SPMI controllers. #address-cells : must be set to 2.