Hi,

On Tue, 2013-10-29 at 11:26 -0500, Josh Cartwright wrote:

On Tue, Oct 29, 2013 at 05:02:03PM +0200, Ivan T. Ivanov wrote:

quoted

On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote: 

quoted

From: Kenneth Heitke <redacted>

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 <redacted>
Signed-off-by: Michael Bohan <redacted>
Signed-off-by: Josh Cartwright <redacted>

[..]

quoted

quoted

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

quoted

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

Oh, I have missed this.

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

Please ignore my comment.

quoted

quoted

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

There is no need from this 'continue' here.

quoted

quoted

+		}
+	}
+
+	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;
+
+	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? 

This was related to my previous comment, which is not valid.

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

Sound reasonable to me and will be the same as i2c bus.

Regards,
Ivan