Add support for the System Power Management Interface (SPMI)

drivers/of/of_spmi.c new file +2 -0

@@ -0,0 +1,74 @@
+/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved.
+ *

drivers/of/of_spmi.c new file +3 -0

+++ b/drivers/spmi/spmi-dbgfs.c

@@ -0,0 +1,591 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *

drivers/of/of_spmi.c new file +4 -0

+static int
+spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt)
+{
+	int ret = 0;

drivers/of/of_spmi.c new file +13 -0

+/**
+ * spmi_write_data: writes data across the SPMI bus
+ * @ctrl: The SPMI controller
+ * @buf: data to be written.
+ * @offset: SPMI address offset to start writing to.
+ * @cnt: The number of bytes to write.
+ *
+ * Returns 0 on success, otherwise returns error code from SPMI driver.
+ */
+static int
+spmi_write_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt)
+{
+	int ret = 0;

drivers/of/of_spmi.c new file +12 -0

+static ssize_t spmi_device_dfs_reg_write(struct file *file,
+					 const char __user *buf,
+					 size_t count, loff_t *ppos)
+{
+	int bytes_read;
+	int data;
+	int pos = 0;
+	int cnt = 0;
+	u8  *values;
+	size_t ret = 0;
+
+	struct spmi_trans *trans = file->private_data;

drivers/of/of_spmi.c new file +15 -0

+		ret = -EFAULT;
+		goto free_buf;
+	}
+
+	count -= ret;
+	*ppos += count;
+	kbuf[count] = '\0';
+
+	/* Override the text buffer with the raw data */
+	values = kbuf;
+
+	/* Parse the data in the buffer.  It should be a string of numbers */
+	while (sscanf(kbuf + pos, "%i%n", &data, &bytes_read) == 1) {
+		pos += bytes_read;
+		values[cnt++] = data & 0xff;

drivers/of/of_spmi.c new file +9 -0

+static ssize_t spmi_device_dfs_reg_read(struct file *file, char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	struct spmi_trans *trans = file->private_data;
+	struct spmi_log_buffer *log = trans->log;
+	size_t ret;
+	size_t len;
+
+	/* Is the the log buffer empty */

drivers/spmi/spmi-dbgfs.h new file +8 -0

@@ -0,0 +1,37 @@
+/* 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

drivers/spmi/spmi-dbgfs.h new file +19 -0

+
+extern void __init spmi_dfs_init(void);
+extern void __exit spmi_dfs_exit(void);
+extern void spmi_dfs_add_controller(struct spmi_controller *ctrl);
+extern void spmi_dfs_del_controller(struct spmi_controller *ctrl);
+extern void spmi_dfs_add_device(struct spmi_device *sdev);
+extern void spmi_dfs_del_device(struct spmi_device *sdev);
+
+#else
+
+static inline void __init spmi_dfs_init(void) { }
+static inline void __exit spmi_dfs_exit(void) { }
+static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { }
+static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { }
+static inline void spmi_dfs_add_device(struct spmi_device *sdev) { }
+static inline void spmi_dfs_del_device(struct spmi_device *sdev) { }
+#endif
+
+#endif /* _SPMI_DBGFS_H */

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

new file mode 100644
index 0000000..3bbcc63
@@ -0,0 +1,449 @@
+/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *

Quoted 9 lines · Josh Cartwright

drivers/of/of_spmi.c new file +6 -0

@@ -0,0 +1,74 @@
+/* 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.
+ *

drivers/of/of_spmi.c line 12

Quoted 6 lines

drivers/of/of_spmi.c new file +6 -0

+ * 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.
+ */
+#define pr_fmt(fmt) "%s: " fmt, __func__

As you never make a pr_* call in this file, this line isn't needed.

Quoted 8 lines

drivers/of/of_spmi.c new file +8 -0

+static int
+spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt)
+{
+	int ret = 0;
+	int len;
+	uint16_t addr;
+
+	while (cnt > 0) {

drivers/of/of_spmi.c line 26

Quoted 6 lines

drivers/of/of_spmi.c new file +6 -0

+		addr = offset & 0xFFFF;
+		len = min(cnt, MAX_REG_PER_TRANSACTION);
+
+		ret = spmi_ext_register_readl(sdev, addr, buf, len);
+		if (ret < 0) {
+			pr_err("SPMI read failed, err = %d\n", ret);

Should be using dev_err() instead.

drivers/of/of_spmi.c line 35

Quoted 9 lines

drivers/of/of_spmi.c new file +9 -0

+
+	/* Make a copy of the user data */
+	char *kbuf = kmalloc(count + 1, GFP_KERNEL);
+	if (!kbuf)
+		return -ENOMEM;
+
+	ret = copy_from_user(kbuf, buf, count);
+	if (ret == count) {
+		pr_err("failed to copy data from user\n");

No need for a message here at all, you will get a message in the function if something happened wrong.

Also, shouldn't it just be a simple:

	if (copy_from_user()) {

test?

drivers/of/of_spmi.c line 38

Quoted 3 lines

drivers/of/of_spmi.c new file +3 -0

+void __exit spmi_dfs_exit(void)
+{
+	pr_debug("de-initializing spmi debugfs ...");

Not needed, use the in-kernel trace functionality if you really want to know this.

Quoted 17 lines

drivers/spmi/spmi-dbgfs.h new file +14 -0

@@ -0,0 +1,37 @@
+/* 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 _SPMI_DBGFS_H
+#define _SPMI_DBGFS_H
+

drivers/spmi/spmi-dbgfs.h line 18

Quoted 4 lines

drivers/spmi/spmi-dbgfs.h new file +4 -0

+#include <linux/spmi.h>
+#include <linux/debugfs.h>
+
+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Quoted 11 lines

drivers/spmi/spmi-dbgfs.h new file +11 -0

+
+extern void __init spmi_dfs_init(void);
+extern void __exit spmi_dfs_exit(void);
+extern void spmi_dfs_add_controller(struct spmi_controller *ctrl);
+extern void spmi_dfs_del_controller(struct spmi_controller *ctrl);
+extern void spmi_dfs_add_device(struct spmi_device *sdev);
+extern void spmi_dfs_del_device(struct spmi_device *sdev);
+
+#else
+
+static inline void __init spmi_dfs_init(void) { }

drivers/spmi/spmi-dbgfs.h line 37

Quoted 8 lines

drivers/spmi/spmi-dbgfs.h new file +8 -0

+static inline void __exit spmi_dfs_exit(void) { }
+static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { }
+static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { }
+static inline void spmi_dfs_add_device(struct spmi_device *sdev) { }
+static inline void spmi_dfs_del_device(struct spmi_device *sdev) { }
+#endif
+
+#endif /* _SPMI_DBGFS_H */

Quoted 4 lines

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Not sure I follow you, but it may be because this is a bit misleading.

Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more

Quoted 5 lines

Quoted 1 line

+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Not sure I follow you, but it may be because this is a bit misleading.

Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have.

The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile:

  spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o

Quoted 16 lines · Josh Cartwright

Quoted 5 lines

Quoted 1 line

+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Not sure I follow you, but it may be because this is a bit misleading.

Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have.

The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile:

Quoted 1 line

  spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o

If debugfs is enabled why wouldn't you want debugfs entries for your devices? Don't assume a user is going to be able to rebuild their kernel just for debugging stuff (hint, they usually aren't), so having these present, if they don't cause any performance issues, is usually best to always have around.

Quoted 17 lines

drivers/spmi/spmi-dbgfs.h new file +14 -0

@@ -0,0 +1,37 @@
+/* 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 _SPMI_DBGFS_H
+#define _SPMI_DBGFS_H
+

drivers/spmi/spmi-dbgfs.h line 18

Quoted 4 lines

drivers/spmi/spmi-dbgfs.h new file +4 -0

+#include <linux/spmi.h>
+#include <linux/debugfs.h>
+
+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Quoted 17 lines · Greg Kroah-Hartman

Quoted 16 lines · Josh Cartwright

Quoted 5 lines

Quoted 1 line

+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

Not sure I follow you, but it may be because this is a bit misleading.

Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have.

The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile:

Quoted 7 lines

Quoted 1 line

  spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o

If debugfs is enabled why wouldn't you want debugfs entries for your devices? Don't assume a user is going to be able to rebuild their kernel just for debugging stuff (hint, they usually aren't), so having these present, if they don't cause any performance issues, is usually best to always have around.

Okay, that makes sense.

So, backing up a step, you're original comment was regarding the CONFIG_DEBUG_FS conditional in spmi-dbgfs.h:

Quoted 17 lines · Greg Kroah-Hartman

Quoted 17 lines

drivers/spmi/spmi-dbgfs.h new file +14 -0

@@ -0,0 +1,37 @@
+/* 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 _SPMI_DBGFS_H
+#define _SPMI_DBGFS_H
+

Quoted 8 lines

drivers/spmi/spmi-dbgfs.h line 18

Quoted 4 lines

drivers/spmi/spmi-dbgfs.h new file +4 -0

+#include <linux/spmi.h>
+#include <linux/debugfs.h>
+
+#ifdef CONFIG_DEBUG_FS

Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right?

The reason why this is done is because the spmi debugfs support code is is only built-in when CONFIG_DEBUG_FS is set.

Would you rather it always be built-in (well, whenever SPMI support is included), and rely on the debugfs_* shims to handle the !CONFIG_DEBUG_FS case?

+++ b/drivers/spmi/spmi.c

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

@@ -0,0 +1,449 @@
+/* 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.
+ */
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/idr.h>
+#include <linux/slab.h>
+#include <linux/module.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 "spmi-dbgfs.h"
+
+static DEFINE_MUTEX(board_lock);
+static DEFINE_IDR(ctrl_idr);

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

+int spmi_del_controller(struct spmi_controller *ctrl)
+{
+	struct spmi_controller *found;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	/* Check that the ctrl has been added */
+	mutex_lock(&board_lock);
+	found = idr_find(&ctrl_idr, ctrl->nr);
+	mutex_unlock(&board_lock);
+
+	if (found != ctrl)
+		return -EINVAL;
+
+	spmi_dfs_del_controller(ctrl);

Quoted 2 lines · Josh Cartwright

+++ b/drivers/spmi/spmi.c

(quoted diff) +0 -0

@@ -0,0 +1,449 @@

drivers/spmi/spmi.c line 43

Quoted 4 lines

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

+static void spmi_ctrl_release(struct device *dev)
+{
+	struct spmi_controller *ctrl = to_spmi_controller(dev);
+	complete(&ctrl->dev_released);

When is this memory going to be freed?

Ah, you think it will be when you remove the device later on:

Quoted 16 lines

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

+int spmi_del_controller(struct spmi_controller *ctrl)
+{
+	struct spmi_controller *found;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	/* Check that the ctrl has been added */
+	mutex_lock(&board_lock);
+	found = idr_find(&ctrl_idr, ctrl->nr);
+	mutex_unlock(&board_lock);
+
+	if (found != ctrl)
+		return -EINVAL;
+
+	spmi_dfs_del_controller(ctrl);

drivers/spmi/spmi.c line 419

Quoted 10 lines

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

+
+	/* Remove all the clients associated with this controller */
+	mutex_lock(&board_lock);
+	bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device);
+	idr_remove(&ctrl_idr, ctrl->nr);
+	mutex_unlock(&board_lock);
+
+	init_completion(&ctrl->dev_released);
+	device_unregister(&ctrl->dev);
+	wait_for_completion(&ctrl->dev_released);

But you just leaked memory, right?

You should never have to wait for this to happen, why did you need to add this? Why not just a simple call to kfree() in the release function?

Quoted 3 lines · Greg Kroah-Hartman

Quoted 2 lines · Josh Cartwright

+++ b/drivers/spmi/spmi.c

(quoted diff) +0 -0

@@ -0,0 +1,449 @@

[..]

Quoted 38 lines

drivers/spmi/spmi.c line 43

Quoted 4 lines

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

+static void spmi_ctrl_release(struct device *dev)
+{
+	struct spmi_controller *ctrl = to_spmi_controller(dev);
+	complete(&ctrl->dev_released);

When is this memory going to be freed?

Ah, you think it will be when you remove the device later on:

Quoted 16 lines

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

+int spmi_del_controller(struct spmi_controller *ctrl)
+{
+	struct spmi_controller *found;
+
+	if (!ctrl)
+		return -EINVAL;
+
+	/* Check that the ctrl has been added */
+	mutex_lock(&board_lock);
+	found = idr_find(&ctrl_idr, ctrl->nr);
+	mutex_unlock(&board_lock);
+
+	if (found != ctrl)
+		return -EINVAL;
+
+	spmi_dfs_del_controller(ctrl);

drivers/spmi/spmi.c line 419

Quoted 10 lines

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

+
+	/* Remove all the clients associated with this controller */
+	mutex_lock(&board_lock);
+	bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device);
+	idr_remove(&ctrl_idr, ctrl->nr);
+	mutex_unlock(&board_lock);
+
+	init_completion(&ctrl->dev_released);
+	device_unregister(&ctrl->dev);
+	wait_for_completion(&ctrl->dev_released);

But you just leaked memory, right?

Quoted 3 lines

You should never have to wait for this to happen, why did you need to add this? Why not just a simple call to kfree() in the release function?

Unfortunately, the reason why this was necessary may be lost to history. :(

I'll do some testing with the completion removed and a simple kfree() in the release and see if there is any fallout.

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/of/Kconfig              |   6 +
drivers/of/Makefile             |   1 +
drivers/of/of_spmi.c            |  74 +++++
drivers/spmi/Kconfig            |   9 +
drivers/spmi/Makefile           |   7 +
drivers/spmi/spmi-dbgfs.c       | 591 ++++++++++++++++++++++++++++++++++++++++
drivers/spmi/spmi-dbgfs.h       |  37 +++
drivers/spmi/spmi.c             | 449 ++++++++++++++++++++++++++++++
include/linux/mod_devicetable.h |   8 +
include/linux/of_spmi.h         |  37 +++
include/linux/spmi.h            | 337 +++++++++++++++++++++++
13 files changed, 1559 insertions(+)

create mode 100644 drivers/of/of_spmi.c
create mode 100644 drivers/spmi/Kconfig
create mode 100644 drivers/spmi/Makefile
create mode 100644 drivers/spmi/spmi-dbgfs.c
create mode 100644 drivers/spmi/spmi-dbgfs.h
create mode 100644 drivers/spmi/spmi.c
create mode 100644 include/linux/of_spmi.h
create mode 100644 include/linux/spmi.h

Quoted 5 lines · Josh Cartwright

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

---
.../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++

1 file changed, 26 insertions(+)

create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt

As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that.

Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt line 1

Quoted 7 lines

Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt new file +1 -0

new file mode 100644
index 0000000..1c14bf4
@@ -0,0 +1,26 @@
+Qualcomm SPMI Controller (PMIC Arbiter)

We should get in the habit of trying to have at least one sentence about what the device is or does.

Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt line 10

Quoted 9 lines

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

+
+Required properties:
+- compatible : should be "qcom,spmi-pmic-arb".
+- 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
+- #size-cells : must be set to 0

Hmm, you aren't describe anything about child nodes.

Hey Kumar-

Thanks for the review.

Quoted 8 lines · Kumar Gala

Quoted 5 lines · Josh Cartwright

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

---
.../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++

1 file changed, 26 insertions(+)

create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt

Quoted 1 line

As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that.

Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound?

Quoted 13 lines · Josh Cartwright

Hey Kumar-

Thanks for the review.

Quoted 8 lines · Kumar Gala

Quoted 5 lines · Josh Cartwright

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

---
.../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++

1 file changed, 26 insertions(+)

create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt

Quoted 3 lines

Quoted 1 line

As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that.

Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound?

Sounds good, as I see we have other examples of having a comma in file names for bindings.