On Tue, Oct 29, 2013 at 09:52:15AM -0700, Stephen Boyd wrote:

On 10/28/13 11:12, Josh Cartwright wrote:

quoted

diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
new file mode 100644
index 0000000..a03835f
--- /dev/null
+++ b/drivers/spmi/Kconfig

@@ -0,0 +1,9 @@
+#
+# SPMI driver configuration
+#
+menuconfig SPMI
+	bool "SPMI support"

Can we do tristate?

I don't think there is any reason why we can't do tristate here.  I do
foresee in the future, however, that we'll run into ordering/dependency
problems when we get the regulator drivers in place.

I suppose we'll wait until we get there to deal with those.

quoted

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

diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c
new file mode 100644
index 0000000..0780bd4
--- /dev/null
+++ b/drivers/spmi/spmi.c

@@ -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);
+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

+static struct device_type spmi_ctrl_type = {

const?

Yep.  Thanks.

[..]

quoted

+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

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

[...]

quoted

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

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

quoted

+
+	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);
+	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.

Some of these dev_dbg()'s (like this one) can probably be just be
removed, especially because we have an additional dev_dbg() right below
this one...

quoted

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

Perhaps call this MAX_USID?

Sure.

quoted

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

[...]

quoted

diff --git a/include/linux/spmi.h b/include/linux/spmi.h
new file mode 100644
index 0000000..29cf0c9
--- /dev/null
+++ b/include/linux/spmi.h

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

+ * spmi_controller_alloc: Allocate a new SPMI controller

There should be a dash here instead of colon.

[..]

quoted

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

This function is missing documentation.

[..]

quoted

+/**
+ * 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.

Will move/clean these up, thanks.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation