The System Power Management Interface (SPMI) is a high-speed,
low-latency, bi-directional, two-wire serial bus suitable for real-time
control of voltage and frequency scaled multi-core application
processors and its power management of auxiliary components. SPMI
obsoletes a number of legacy, custom point-to-point interfaces and
provides a low pin count, high-speed control bus for up to 4 Master and
16 Slave devices.
SPMI is specified by the MIPI (Mobile Industry Process Interface)
Alliance [1].
This patchset is intended both to provide a core implementation of SPMI and
also to provide a more-or-less complete example of it's use.
- Patch 1 provides a stubbed implementation of for_each_available_child_of_node()
to allow SPMI to build with !CONFIG_OF
- Patches 2-3 implement the SPMI core functionality.
- Patches 4-6 provide an implementation of an SPMI controller, the Qualcomm
"PMIC Arbiter", currently used on the 8x74 series SoCs.
- Patch 7 provides an implementation of regmap for SPMI
- Patches 8-9 is an implementation of a client driver for the PM8x41 PMICs paired
with the 800 series SoCs.
- Patch 10 reworks the existing pm8xxx-rtc driver to work with the PM8x41 PMIC.
Changes from v2[2]:
- Dropped RFC.
- Add basic regmap support at Mark Brown's suggestion
- Drop debugfs interface. Debugging SPMI accesses can happen via the regmap
debugfs interface if necessary.
- Add second address-cell in SPMI generic device tree binding, encoding the
address type (suggestion by Stephen Warren)
- Implement interrupt handling functionality within the PMIC Arbiter driver
- Provide basic MFD driver for the PMIC8x41 PMICs, demonstrating SPMI regmap
client use
- Adapt existing pm8xxx-rtc driver to work as a child of the PM8x41 mfd device
Changes from v1[3]:
- Adopted patch (1/5) to #define for_each_available_node() shim
in the !CONFIG_OF case
- Moved device tree logic out of drivers/of and into spmi.c core (this
mirrors what SPI is doing, and what i2c will soon be doing)
- Move of_spmi_add_devices() call into spmi_device_add(), so drivers don't
have to call it explicitly
- Unconditionally build in debugfs code (rely on the underlying
CONFIG_DEBUG_FS switch to throw unused code away)
- Change pr_* print functions to their dev_* equivalents
- Fix copy_{to,from}_user error handling
- Renamed "board_lock" to "ctrl_idr_lock" to better describe it's purpose
- Rework device object lifetime management
- Rename PMIC arb binding document, add description of PMIC arb
- Add generic SPMI device tree bindings
[1]: http://www.mipi.org/specifications/system-power-management-interface
[2]: http://marc.info/?l=linux-arm-kernel&m=137721241427533&w=2
[3]: http://thread.gmane.org/gmane.linux.ports.arm.msm/4886
Josh Cartwright (7):
spmi: add generic SPMI controller binding documentation
spmi: pmic_arb: add support for interrupt handling
spmi: document the PMIC arbiter SPMI bindings
regmap: add SPMI support
mfd: pm8x41: add support for Qualcomm 8x41 PMICs
mfd: pm8x41: document device tree bindings
rtc: pm8xxx: add support for pm8941
Kenneth Heitke (2):
spmi: Linux driver framework for SPMI
spmi: Add MSM PMIC Arbiter SPMI controller
Sylwester Nawrocki (1):
of: Add empty for_each_available_child_of_node() macro definition
Documentation/devicetree/bindings/mfd/pm8x41.txt | 33 +
.../bindings/spmi/qcom,spmi-pmic-arb.txt | 42 ++
Documentation/devicetree/bindings/spmi/spmi.txt | 41 ++
drivers/Kconfig | 2 +
drivers/Makefile | 1 +
drivers/base/regmap/Kconfig | 5 +-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-spmi.c | 90 +++
drivers/mfd/Kconfig | 10 +
drivers/mfd/Makefile | 1 +
drivers/mfd/pm8x41.c | 64 ++
drivers/rtc/Kconfig | 1 -
drivers/rtc/rtc-pm8xxx.c | 229 ++++---
drivers/spmi/Kconfig | 24 +
drivers/spmi/Makefile | 6 +
drivers/spmi/spmi-pmic-arb.c | 758 +++++++++++++++++++++
drivers/spmi/spmi.c | 491 +++++++++++++
include/dt-bindings/spmi/spmi.h | 18 +
include/linux/mod_devicetable.h | 8 +
include/linux/of.h | 3 +
include/linux/regmap.h | 5 +
include/linux/spmi.h | 342 ++++++++++
22 files changed, 2087 insertions(+), 88 deletions(-)
create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
create mode 100644 Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
create mode 100644 Documentation/devicetree/bindings/spmi/spmi.txt
create mode 100644 drivers/base/regmap/regmap-spmi.c
create mode 100644 drivers/mfd/pm8x41.c
create mode 100644 drivers/spmi/Kconfig
create mode 100644 drivers/spmi/Makefile
create mode 100644 drivers/spmi/spmi-pmic-arb.c
create mode 100644 drivers/spmi/spmi.c
create mode 100644 include/dt-bindings/spmi/spmi.h
create mode 100644 include/linux/spmi.h
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Document the bindings used to describe the Qualcomm 8x41 PMICs.
Signed-off-by: Josh Cartwright <[email protected]>
---
Documentation/devicetree/bindings/mfd/pm8x41.txt | 33 ++++++++++++++++++++++++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt
new file mode 100644
index 0000000..6afd4ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt
@@ -0,0 +1,33 @@
+Qualcomm PM8841 and PM8941 PMIC multi-function devices
+
+The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are
+interfaced to the chip via the SPMI (System Power Management Interface) bus.
+Support for multiple independent functions are implemented by splitting the
+16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes
+each. A function can consume one or more of these fixed-size register regions.
+
+Required properties:
+- compatible: Must be one of:
+ "qcom,pm8841"
+ "qcom,pm8941"
+- reg: Specifies the SPMI USID slave address for this device
+- #address-cells = <1>
+- #size-cells = <0>
+
+Each child node represents a function of the PM8x41. Each child 'reg' entry
+describes an offset within the USID slave address where the region starts.
+
+Example:
+
+pm8941@0 {
+ compatible = "qcom,pm8941";
+ reg = <0x0>;
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rtc {
+ compatible = "qcom,pm8941-rtc";
+ reg = <0x6000 0x6100>;
+ };
+}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
800 series SoC family. This driver exists largely as a glue mfd component,
it exists to be an owner of an SPMI regmap for children devices
described in device tree.
Signed-off-by: Josh Cartwright <[email protected]>
---
drivers/mfd/Kconfig | 10 ++++++++
drivers/mfd/Makefile | 1 +
drivers/mfd/pm8x41.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 75 insertions(+)
create mode 100644 drivers/mfd/pm8x41.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 914c3d1..0a288cb 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -476,6 +476,16 @@ config MFD_PM8XXX_IRQ
This is required to use certain other PM 8xxx features, such as GPIO
and MPP.
+config MFD_PM8X41
+ bool "Qualcomm PM8X41 PMIC"
+ depends on ARCH_MSM
+ select REGMAP_SPMI
+ help
+ This enables basic support for the Qualcomm 8941 and 8841 PMICs. These
+ PMICs are currently used with the Snapdragon 800 series of SoCs. Note, that
+ this will only be useful paired with descriptions of the independent functions
+ as children nodes in the device tree.
+
config MFD_RDC321X
tristate "RDC R-321x southbridge"
select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 15b905c..ee4d338 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -149,6 +149,7 @@ obj-$(CONFIG_MFD_CS5535) += cs5535-mfd.o
obj-$(CONFIG_MFD_OMAP_USB_HOST) += omap-usb-host.o omap-usb-tll.o
obj-$(CONFIG_MFD_PM8921_CORE) += pm8921-core.o ssbi.o
obj-$(CONFIG_MFD_PM8XXX_IRQ) += pm8xxx-irq.o
+obj-$(CONFIG_MFD_PM8X41) += pm8x41.o
obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o
obj-$(CONFIG_MFD_TPS65090) += tps65090.o
obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o
diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
new file mode 100644
index 0000000..0a57221
--- /dev/null
+++ b/drivers/mfd/pm8x41.c
@@ -0,0 +1,64 @@
+/* Copyright (c) 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/module.h>
+#include <linux/spmi.h>
+#include <linux/regmap.h>
+#include <linux/of_platform.h>
+
+static const struct regmap_config pm8x41_regmap_config = {
+ .reg_bits = 16,
+ .val_bits = 8,
+ .max_register = 0xFFFF,
+};
+
+static int pm8x41_remove_child(struct device *dev, void *unused)
+{
+ platform_device_unregister(to_platform_device(dev));
+ return 0;
+}
+
+static int pm8x41_remove(struct spmi_device *sdev)
+{
+ device_for_each_child(&sdev->dev, NULL, pm8x41_remove_child);
+ return 0;
+}
+
+static int pm8x41_probe(struct spmi_device *sdev)
+{
+ struct regmap *regmap;
+
+ regmap = devm_regmap_init_spmi(sdev, &pm8x41_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_dbg(&sdev->dev, "regmap creation failed.\n");
+ return PTR_ERR(regmap);
+ }
+
+ return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
+}
+
+static struct of_device_id pm8x41_id_table[] = {
+ { "qcom,pm8841", },
+ { "qcom,pm8941", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, pm8x41_id_table);
+
+static struct spmi_driver pm8x41_driver = {
+ .probe = pm8x41_probe,
+ .remove = pm8x41_remove,
+ .driver = {
+ .name = "qpnp,pm8x41",
+ .of_match_table = pm8x41_id_table,
+ },
+};
+module_spmi_driver(pm8x41_driver);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
From: Kenneth Heitke <[email protected]>
Qualcomm's PMIC Arbiter SPMI controller functions as a bus master and
is used to communication with one or more PMIC (slave) devices on the
SPMI bus. The PMIC Arbiter is actually a hardware wrapper around the
SPMI controller that provides concurrent and autonomous PMIC access
to various entities that need to communicate with the PMIC.
The SPMI controller hardware handles all of the SPMI bus activity (bus
arbitration, sequence start condition, transmission of frames, etc).
This software driver uses the PMIC Arbiter register interface to
initiate command sequences on the SPMI bus. The status register is
read to determine when the command sequence has completed and whether
or not it completed successfully.
Signed-off-by: Kenneth Heitke <[email protected]>
Signed-off-by: Josh Cartwright <[email protected]>
---
drivers/spmi/Kconfig | 15 ++
drivers/spmi/Makefile | 2 +
drivers/spmi/spmi-pmic-arb.c | 403 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 420 insertions(+)
create mode 100644 drivers/spmi/spmi-pmic-arb.c
diff --git a/drivers/spmi/Kconfig b/drivers/spmi/Kconfig
index a03835f..056891d 100644
--- a/drivers/spmi/Kconfig
+++ b/drivers/spmi/Kconfig
@@ -7,3 +7,18 @@ menuconfig SPMI
SPMI (System Power Management Interface) is a two-wire
serial interface between baseband and application processors
and Power Management Integrated Circuits (PMIC).
+
+if SPMI
+
+config SPMI_MSM_PMIC_ARB
+ tristate "Qualcomm MSM SPMI Controller (PMIC Arbiter)"
+ depends on ARCH_MSM
+ help
+ If you say yes to this option, support will be included for the
+ built-in SPMI PMIC Arbiter interface on Qualcomm MSM family
+ processors.
+
+ This is required for communicating with Qualcomm PMICs and
+ other devices that have the SPMI interface.
+
+endif
diff --git a/drivers/spmi/Makefile b/drivers/spmi/Makefile
index 1de1acd..fc75104 100644
--- a/drivers/spmi/Makefile
+++ b/drivers/spmi/Makefile
@@ -2,3 +2,5 @@
# Makefile for kernel SPMI framework.
#
obj-$(CONFIG_SPMI) += spmi.o
+
+obj-$(CONFIG_SPMI_MSM_PMIC_ARB) += spmi-pmic-arb.o
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
new file mode 100644
index 0000000..92e1416
--- /dev/null
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -0,0 +1,403 @@
+/* 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/delay.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/spmi.h>
+
+/* PMIC Arbiter configuration registers */
+#define PMIC_ARB_VERSION 0x0000
+#define PMIC_ARB_INT_EN 0x0004
+
+/* PMIC Arbiter channel registers */
+#define PMIC_ARB_CMD(N) (0x0800 + (0x80 * (N)))
+#define PMIC_ARB_CONFIG(N) (0x0804 + (0x80 * (N)))
+#define PMIC_ARB_STATUS(N) (0x0808 + (0x80 * (N)))
+#define PMIC_ARB_WDATA0(N) (0x0810 + (0x80 * (N)))
+#define PMIC_ARB_WDATA1(N) (0x0814 + (0x80 * (N)))
+#define PMIC_ARB_RDATA0(N) (0x0818 + (0x80 * (N)))
+#define PMIC_ARB_RDATA1(N) (0x081C + (0x80 * (N)))
+
+/* Interrupt Controller */
+#define SPMI_PIC_OWNER_ACC_STATUS(M, N) (0x0000 + ((32 * (M)) + (4 * (N))))
+#define SPMI_PIC_ACC_ENABLE(N) (0x0200 + (4 * (N)))
+#define SPMI_PIC_IRQ_STATUS(N) (0x0600 + (4 * (N)))
+#define SPMI_PIC_IRQ_CLEAR(N) (0x0A00 + (4 * (N)))
+
+/* Mapping Table */
+#define SPMI_MAPPING_TABLE_REG(N) (0x0B00 + (4 * (N)))
+#define SPMI_MAPPING_BIT_INDEX(X) (((X) >> 18) & 0xF)
+#define SPMI_MAPPING_BIT_IS_0_FLAG(X) (((X) >> 17) & 0x1)
+#define SPMI_MAPPING_BIT_IS_0_RESULT(X) (((X) >> 9) & 0xFF)
+#define SPMI_MAPPING_BIT_IS_1_FLAG(X) (((X) >> 8) & 0x1)
+#define SPMI_MAPPING_BIT_IS_1_RESULT(X) (((X) >> 0) & 0xFF)
+
+#define SPMI_MAPPING_TABLE_LEN 255
+#define SPMI_MAPPING_TABLE_TREE_DEPTH 16 /* Maximum of 16-bits */
+
+/* Ownership Table */
+#define SPMI_OWNERSHIP_TABLE_REG(N) (0x0700 + (4 * (N)))
+#define SPMI_OWNERSHIP_PERIPH2OWNER(X) ((X) & 0x7)
+
+/* Channel Status fields */
+enum pmic_arb_chnl_status {
+ PMIC_ARB_STATUS_DONE = (1 << 0),
+ PMIC_ARB_STATUS_FAILURE = (1 << 1),
+ PMIC_ARB_STATUS_DENIED = (1 << 2),
+ PMIC_ARB_STATUS_DROPPED = (1 << 3),
+};
+
+/* Command register fields */
+#define PMIC_ARB_CMD_MAX_BYTE_COUNT 8
+
+/* Command Opcodes */
+enum pmic_arb_cmd_op_code {
+ PMIC_ARB_OP_EXT_WRITEL = 0,
+ PMIC_ARB_OP_EXT_READL = 1,
+ PMIC_ARB_OP_EXT_WRITE = 2,
+ PMIC_ARB_OP_RESET = 3,
+ PMIC_ARB_OP_SLEEP = 4,
+ PMIC_ARB_OP_SHUTDOWN = 5,
+ PMIC_ARB_OP_WAKEUP = 6,
+ PMIC_ARB_OP_AUTHENTICATE = 7,
+ PMIC_ARB_OP_MSTR_READ = 8,
+ PMIC_ARB_OP_MSTR_WRITE = 9,
+ PMIC_ARB_OP_EXT_READ = 13,
+ PMIC_ARB_OP_WRITE = 14,
+ PMIC_ARB_OP_READ = 15,
+ PMIC_ARB_OP_ZERO_WRITE = 16,
+};
+
+/* Maximum number of support PMIC peripherals */
+#define PMIC_ARB_MAX_PERIPHS 256
+#define PMIC_ARB_PERIPH_ID_VALID (1 << 15)
+#define PMIC_ARB_TIMEOUT_US 100
+#define PMIC_ARB_MAX_TRANS_BYTES (8)
+
+#define PMIC_ARB_APID_MASK 0xFF
+#define PMIC_ARB_PPID_MASK 0xFFF
+
+/* interrupt enable bit */
+#define SPMI_PIC_ACC_ENABLE_BIT BIT(0)
+
+/**
+ * spmi_pmic_arb_dev - SPMI PMIC Arbiter object
+ *
+ * @base: address of the PMIC Arbiter core registers
+ * @intr: address of the SPMI interrupt control registers
+ * @cnfg: address of the PMIC Arbiter configuration registers
+ * @mapping_table: in-memory copy of PPID -> APID mapping table
+ */
+struct spmi_pmic_arb_dev {
+ void __iomem *base;
+ void __iomem *intr;
+ void __iomem *cnfg;
+ bool allow_wakeup;
+ spinlock_t lock;
+ u8 channel;
+ u8 min_apid;
+ u8 max_apid;
+ u32 mapping_table[SPMI_MAPPING_TABLE_LEN];
+};
+
+static inline u32 pmic_arb_base_read(struct spmi_pmic_arb_dev *dev, u32 offset)
+{
+ return readl_relaxed(dev->base + offset);
+}
+
+static inline void pmic_arb_base_write(struct spmi_pmic_arb_dev *dev, u32 offset,
+ u32 val)
+{
+ writel_relaxed(val, dev->base + offset);
+}
+
+/**
+ * pa_read_data: reads pmic-arb's register and copy 1..4 bytes to buf
+ * @bc byte count -1. range: 0..3
+ * @reg register's address
+ * @buf output parameter, length must be bc+1
+ */
+static void pa_read_data(struct spmi_pmic_arb_dev *dev, u8 *buf, u32 reg, u8 bc)
+{
+ u32 data = pmic_arb_base_read(dev, reg);
+ memcpy(buf, &data, (bc & 3) + 1);
+}
+
+/**
+ * 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
+ */
+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);
+}
+
+static int pmic_arb_wait_for_done(struct spmi_controller *ctrl)
+{
+ struct spmi_pmic_arb_dev *dev = spmi_controller_get_drvdata(ctrl);
+ u32 status = 0;
+ u32 timeout = PMIC_ARB_TIMEOUT_US;
+ u32 offset = PMIC_ARB_STATUS(dev->channel);
+
+ while (timeout--) {
+ status = pmic_arb_base_read(dev, offset);
+
+ if (status & PMIC_ARB_STATUS_DONE) {
+ if (status & PMIC_ARB_STATUS_DENIED) {
+ dev_err(&ctrl->dev,
+ "%s: transaction denied (0x%x)\n",
+ __func__, status);
+ return -EPERM;
+ }
+
+ if (status & PMIC_ARB_STATUS_FAILURE) {
+ dev_err(&ctrl->dev,
+ "%s: transaction failed (0x%x)\n",
+ __func__, status);
+ return -EIO;
+ }
+
+ if (status & PMIC_ARB_STATUS_DROPPED) {
+ dev_err(&ctrl->dev,
+ "%s: transaction dropped (0x%x)\n",
+ __func__, status);
+ return -EIO;
+ }
+
+ return 0;
+ }
+ udelay(1);
+ }
+
+ dev_err(&ctrl->dev,
+ "%s: timeout, status 0x%x\n",
+ __func__, status);
+ return -ETIMEDOUT;
+}
+
+/* Non-data command */
+static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid)
+{
+ struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+ unsigned long flags;
+ u32 cmd;
+ int rc;
+
+ dev_dbg(&ctrl->dev, "op:0x%x sid:%d\n", opc, sid);
+
+ /* Check for valid non-data command */
+ if (opc < SPMI_CMD_RESET || opc > SPMI_CMD_WAKEUP)
+ return -EINVAL;
+
+ cmd = ((opc | 0x40) << 27) | ((sid & 0xf) << 20);
+
+ 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);
+ spin_unlock_irqrestore(&pmic_arb->lock, flags);
+
+ return rc;
+}
+
+static int pmic_arb_read_cmd(struct spmi_controller *ctrl,
+ u8 opc, u8 sid, u16 addr, u8 bc, u8 *buf)
+{
+ struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+ unsigned long flags;
+ u32 cmd;
+ int rc;
+
+ if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
+ dev_err(&ctrl->dev,
+ "pmic-arb supports 1..%d bytes per trans, but:%d requested",
+ PMIC_ARB_MAX_TRANS_BYTES, bc+1);
+ 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);
+ 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));
+
+ 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;
+}
+
+static int pmic_arb_write_cmd(struct spmi_controller *ctrl,
+ u8 opc, u8 sid, u16 addr, u8 bc, const u8 *buf)
+{
+ struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
+ unsigned long flags;
+ u32 cmd;
+ int rc;
+
+ if (bc >= PMIC_ARB_MAX_TRANS_BYTES) {
+ dev_err(&ctrl->dev,
+ "pmic-arb supports 1..%d bytes per trans, but:%d requested",
+ PMIC_ARB_MAX_TRANS_BYTES, bc+1);
+ 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 >= 0x40 && opc <= 0x5F)
+ opc = PMIC_ARB_OP_WRITE;
+ else if (opc >= 0x00 && opc <= 0x0F)
+ opc = PMIC_ARB_OP_EXT_WRITE;
+ else if (opc >= 0x30 && opc <= 0x37)
+ opc = PMIC_ARB_OP_EXT_WRITEL;
+ else if (opc >= 0x80 && opc <= 0xFF)
+ opc = PMIC_ARB_OP_ZERO_WRITE;
+ else
+ return -EINVAL;
+
+ cmd = (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
+
+ /* Write data to FIFOs */
+ spin_lock_irqsave(&pmic_arb->lock, flags);
+ pa_write_data(pmic_arb, buf, PMIC_ARB_WDATA0(pmic_arb->channel)
+ , min_t(u8, bc, 3));
+ if (bc > 3)
+ pa_write_data(pmic_arb, buf + 4,
+ PMIC_ARB_WDATA1(pmic_arb->channel), bc - 4);
+
+ /* Start the transaction */
+ pmic_arb_base_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd);
+ rc = pmic_arb_wait_for_done(ctrl);
+ spin_unlock_irqrestore(&pmic_arb->lock, flags);
+
+ return rc;
+}
+
+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;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
+ pa->mapping_table[i] = readl_relaxed(
+ pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
+
+ platform_set_drvdata(pdev, ctrl);
+ spin_lock_init(&pa->lock);
+
+ pa->channel = 0;
+ pa->max_apid = 0;
+ pa->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
+
+ 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",
+ pmic_arb_base_read(pa, PMIC_ARB_VERSION));
+
+ return 0;
+
+err_put_ctrl:
+ spmi_controller_put(ctrl);
+ return err;
+}
+
+static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
+{
+ struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+ spmi_controller_remove(ctrl);
+ return 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),
+ .driver = {
+ .name = "spmi_pmic_arb",
+ .owner = THIS_MODULE,
+ .of_match_table = spmi_pmic_arb_match_table,
+ },
+};
+module_platform_driver(spmi_pmic_arb_driver);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
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
diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
new file mode 100644
index 0000000..68949aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
@@ -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".
+- 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
+- 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@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>;
+ };
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
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]>
---
include/linux/of.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index f95aee3..908584e 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -364,6 +364,9 @@ static inline bool of_have_populated_dt(void)
#define for_each_child_of_node(parent, child) \
while (0)
+#define for_each_available_child_of_node(parent, child) \
+ while (0)
+
static inline struct device_node *of_get_child_by_name(
const struct device_node *node,
const char *name)
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Add basic support for the System Power Management Interface (SPMI) bus.
This is a simple implementation which only implements register accesses
via the Extended Register Read/Write Long commands.
Signed-off-by: Josh Cartwright <[email protected]>
---
drivers/base/regmap/Kconfig | 5 ++-
drivers/base/regmap/Makefile | 1 +
drivers/base/regmap/regmap-spmi.c | 90 +++++++++++++++++++++++++++++++++++++++
include/linux/regmap.h | 5 +++
4 files changed, 100 insertions(+), 1 deletion(-)
create mode 100644 drivers/base/regmap/regmap-spmi.c
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index f0d3054..4251570 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -3,7 +3,7 @@
# subsystems should select the appropriate symbols.
config REGMAP
- default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_MMIO || REGMAP_IRQ)
+ default y if (REGMAP_I2C || REGMAP_SPI || REGMAP_SPMI || REGMAP_MMIO || REGMAP_IRQ)
select LZO_COMPRESS
select LZO_DECOMPRESS
select IRQ_DOMAIN if REGMAP_IRQ
@@ -15,6 +15,9 @@ config REGMAP_I2C
config REGMAP_SPI
tristate
+config REGMAP_SPMI
+ tristate
+
config REGMAP_MMIO
tristate
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index cf12998..a7c670b 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -3,5 +3,6 @@ obj-$(CONFIG_REGMAP) += regcache-rbtree.o regcache-lzo.o regcache-flat.o
obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
+obj-$(CONFIG_REGMAP_SPMI) += regmap-spmi.o
obj-$(CONFIG_REGMAP_MMIO) += regmap-mmio.o
obj-$(CONFIG_REGMAP_IRQ) += regmap-irq.o
diff --git a/drivers/base/regmap/regmap-spmi.c b/drivers/base/regmap/regmap-spmi.c
new file mode 100644
index 0000000..ac23910
--- /dev/null
+++ b/drivers/base/regmap/regmap-spmi.c
@@ -0,0 +1,90 @@
+/*
+ * Register map access API - SPMI support
+ *
+ * Copyright (c) 2012-2013, The Linux Foundation. All rights reserved.
+ *
+ * Based on regmap-i2c.c:
+ * Copyright 2011 Wolfson Microelectronics plc
+ * Author: Mark Brown <[email protected]>
+ *
+ * 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/regmap.h>
+#include <linux/spmi.h>
+#include <linux/module.h>
+#include <linux/init.h>
+
+static int regmap_spmi_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ BUG_ON(reg_size != 2);
+ return spmi_ext_register_readl(context, *(u16 *)reg,
+ val, val_size);
+}
+
+static int regmap_spmi_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ BUG_ON(reg_size != 2);
+ return spmi_ext_register_writel(context, *(u16 *)reg, val, val_size);
+}
+
+static int regmap_spmi_write(void *context, const void *data,
+ size_t count)
+{
+ BUG_ON(count < 2);
+ return regmap_spmi_gather_write(context, data, 2, data + 2, count - 2);
+}
+
+static struct regmap_bus regmap_spmi = {
+ .read = regmap_spmi_read,
+ .write = regmap_spmi_write,
+ .gather_write = regmap_spmi_gather_write,
+ .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
+/**
+ * regmap_init_spmi(): Initialize register map
+ *
+ * @sdev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer to
+ * a struct regmap.
+ */
+struct regmap *regmap_init_spmi(struct spmi_device *sdev,
+ const struct regmap_config *config)
+{
+ return regmap_init(&sdev->dev, ®map_spmi, sdev, config);
+}
+EXPORT_SYMBOL_GPL(regmap_init_spmi);
+
+/**
+ * devm_regmap_init_spmi(): Initialise managed register map
+ *
+ * @sdev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap. The regmap will be automatically freed by the
+ * device management code.
+ */
+struct regmap *devm_regmap_init_spmi(struct spmi_device *sdev,
+ const struct regmap_config *config)
+{
+ return devm_regmap_init(&sdev->dev, ®map_spmi, sdev, config);
+}
+EXPORT_SYMBOL_GPL(devm_regmap_init_spmi);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a10380b..3f5abc8 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -23,6 +23,7 @@ struct device;
struct i2c_client;
struct irq_domain;
struct spi_device;
+struct spmi_device;
struct regmap;
struct regmap_range_cfg;
struct regmap_field;
@@ -318,6 +319,8 @@ struct regmap *regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
struct regmap *regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
+struct regmap *regmap_init_spmi(struct spmi_device *dev,
+ const struct regmap_config *config);
struct regmap *regmap_init_mmio_clk(struct device *dev, const char *clk_id,
void __iomem *regs,
const struct regmap_config *config);
@@ -330,6 +333,8 @@ struct regmap *devm_regmap_init_i2c(struct i2c_client *i2c,
const struct regmap_config *config);
struct regmap *devm_regmap_init_spi(struct spi_device *dev,
const struct regmap_config *config);
+struct regmap *devm_regmap_init_spmi(struct spmi_device *dev,
+ const struct regmap_config *config);
struct regmap *devm_regmap_init_mmio_clk(struct device *dev, const char *clk_id,
void __iomem *regs,
const struct regmap_config *config);
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
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
diff --git a/drivers/Kconfig b/drivers/Kconfig
index aa43b91..2b35420 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -52,6 +52,8 @@ source "drivers/i2c/Kconfig"
source "drivers/spi/Kconfig"
+source "drivers/spmi/Kconfig"
+
source "drivers/hsi/Kconfig"
source "drivers/pps/Kconfig"
diff --git a/drivers/Makefile b/drivers/Makefile
index ab93de8..5c250df 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -64,6 +64,7 @@ obj-$(CONFIG_ATA) += ata/
obj-$(CONFIG_TARGET_CORE) += target/
obj-$(CONFIG_MTD) += mtd/
obj-$(CONFIG_SPI) += spi/
+obj-$(CONFIG_SPMI) += spmi/
obj-y += hsi/
obj-y += net/
obj-$(CONFIG_ATM) += atm/
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"
+ 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/Makefile b/drivers/spmi/Makefile
new file mode 100644
index 0000000..1de1acd
--- /dev/null
+++ b/drivers/spmi/Makefile
@@ -0,0 +1,4 @@
+#
+# Makefile for kernel SPMI framework.
+#
+obj-$(CONFIG_SPMI) += spmi.o
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,
+};
+
+static void spmi_ctrl_release(struct device *dev)
+{
+ struct spmi_controller *ctrl = to_spmi_controller(dev);
+ mutex_lock(&ctrl_idr_lock);
+ idr_remove(&ctrl_idr, ctrl->nr);
+ mutex_unlock(&ctrl_idr_lock);
+ kfree(ctrl);
+}
+
+static struct device_type spmi_ctrl_type = {
+ .release = spmi_ctrl_release,
+};
+
+#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);
+ 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
+ )
+};
+
+static int spmi_device_match(struct device *dev, struct device_driver *drv)
+{
+ if (of_driver_match_device(dev, drv))
+ return 1;
+
+ if (drv->name)
+ return strncmp(dev_name(dev), drv->name,
+ SPMI_NAME_SIZE) == 0;
+
+ return 0;
+}
+
+int spmi_device_add(struct spmi_device *sdev)
+{
+ struct spmi_controller *ctrl = sdev->ctrl;
+ int err;
+
+ dev_set_name(&sdev->dev, "%d-%02x", ctrl->nr, sdev->usid);
+
+ err = device_add(&sdev->dev);
+ if (err < 0) {
+ dev_err(&sdev->dev, "Can't add %s, status %d\n",
+ dev_name(&sdev->dev), err);
+ goto err_device_add;
+ }
+
+ dev_dbg(&sdev->dev, "device %s registered\n", dev_name(&sdev->dev));
+
+err_device_add:
+ return err;
+}
+EXPORT_SYMBOL_GPL(spmi_device_add);
+
+void spmi_device_remove(struct spmi_device *sdev)
+{
+ device_unregister(&sdev->dev);
+}
+EXPORT_SYMBOL_GPL(spmi_device_remove);
+
+static inline int
+spmi_cmd(struct spmi_controller *ctrl, u8 opcode, u8 sid)
+{
+ if (!ctrl || !ctrl->cmd || ctrl->dev.type != &spmi_ctrl_type)
+ return -EINVAL;
+
+ return ctrl->cmd(ctrl, opcode, sid);
+}
+
+static inline int spmi_read_cmd(struct spmi_controller *ctrl,
+ u8 opcode, u8 sid, u16 addr, u8 bc, u8 *buf)
+{
+ if (!ctrl || !ctrl->read_cmd || ctrl->dev.type != &spmi_ctrl_type)
+ return -EINVAL;
+
+ return ctrl->read_cmd(ctrl, opcode, sid, addr, bc, buf);
+}
+
+static inline int spmi_write_cmd(struct spmi_controller *ctrl,
+ u8 opcode, u8 sid, u16 addr, u8 bc,
+ const u8 *buf)
+{
+ if (!ctrl || !ctrl->write_cmd || ctrl->dev.type != &spmi_ctrl_type)
+ return -EINVAL;
+
+ return ctrl->write_cmd(ctrl, opcode, sid, addr, bc, buf);
+}
+
+/*
+ * register read/write: 5-bit address, 1 byte of data
+ * extended register read/write: 8-bit address, up to 16 bytes of data
+ * extended register read/write long: 16-bit address, up to 8 bytes of data
+ */
+
+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf)
+{
+ /* 5-bit register address */
+ if (addr > 0x1F)
+ return -EINVAL;
+
+ return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr, 0,
+ buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_read);
+
+int spmi_ext_register_read(struct spmi_device *sdev, u8 addr, u8 *buf,
+ size_t len)
+{
+ /* 8-bit register address, up to 16 bytes */
+ if (len == 0 || len > 16)
+ return -EINVAL;
+
+ return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READ, sdev->usid, addr,
+ len - 1, buf);
+}
+EXPORT_SYMBOL_GPL(spmi_ext_register_read);
+
+int spmi_ext_register_readl(struct spmi_device *sdev, u16 addr, u8 *buf,
+ size_t len)
+{
+ /* 16-bit register address, up to 8 bytes */
+ if (len == 0 || len > 8)
+ return -EINVAL;
+
+ return spmi_read_cmd(sdev->ctrl, SPMI_CMD_EXT_READL, sdev->usid, addr,
+ len - 1, buf);
+}
+EXPORT_SYMBOL_GPL(spmi_ext_register_readl);
+
+int spmi_register_write(struct spmi_device *sdev, u8 addr, const u8 *buf)
+{
+ /* 5-bit register address */
+ if (addr > 0x1F)
+ return -EINVAL;
+
+ return spmi_write_cmd(sdev->ctrl, SPMI_CMD_WRITE, sdev->usid, addr, 0,
+ buf);
+}
+EXPORT_SYMBOL_GPL(spmi_register_write);
+
+int spmi_register_zero_write(struct spmi_device *sdev, u8 data)
+{
+ return spmi_write_cmd(sdev->ctrl, SPMI_CMD_ZERO_WRITE, sdev->usid, 0,
+ 0, &data);
+}
+EXPORT_SYMBOL_GPL(spmi_register_zero_write);
+
+int spmi_ext_register_write(struct spmi_device *sdev, u8 addr, const u8 *buf,
+ size_t len)
+{
+ /* 8-bit register address, up to 16 bytes */
+ if (len == 0 || len > 16)
+ return -EINVAL;
+
+ return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITE, sdev->usid, addr,
+ len - 1, buf);
+}
+EXPORT_SYMBOL_GPL(spmi_ext_register_write);
+
+int spmi_ext_register_writel(struct spmi_device *sdev, u16 addr, const u8 *buf,
+ size_t len)
+{
+ /* 4-bit Slave Identifier, 16-bit register address, up to 8 bytes */
+ if (len == 0 || len > 8)
+ return -EINVAL;
+
+ return spmi_write_cmd(sdev->ctrl, SPMI_CMD_EXT_WRITEL, sdev->usid,
+ addr, len - 1, buf);
+}
+EXPORT_SYMBOL_GPL(spmi_ext_register_writel);
+
+int spmi_command_reset(struct spmi_device *sdev)
+{
+ return spmi_cmd(sdev->ctrl, SPMI_CMD_RESET, sdev->usid);
+}
+EXPORT_SYMBOL_GPL(spmi_command_reset);
+
+int spmi_command_sleep(struct spmi_device *sdev)
+{
+ return spmi_cmd(sdev->ctrl, SPMI_CMD_SLEEP, sdev->usid);
+}
+EXPORT_SYMBOL_GPL(spmi_command_sleep);
+
+int spmi_command_wakeup(struct spmi_device *sdev)
+{
+ return spmi_cmd(sdev->ctrl, SPMI_CMD_WAKEUP, sdev->usid);
+}
+EXPORT_SYMBOL_GPL(spmi_command_wakeup);
+
+int spmi_command_shutdown(struct spmi_device *sdev)
+{
+ return spmi_cmd(sdev->ctrl, SPMI_CMD_SHUTDOWN, sdev->usid);
+}
+EXPORT_SYMBOL_GPL(spmi_command_shutdown);
+
+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)
+ 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,
+};
+
+struct spmi_device *spmi_device_alloc(struct spmi_controller *ctrl)
+{
+ struct spmi_device *sdev;
+
+ sdev = kzalloc(sizeof(*sdev), GFP_KERNEL);
+ if (!sdev)
+ return NULL;
+
+ sdev->ctrl = ctrl;
+ device_initialize(&sdev->dev);
+ sdev->dev.parent = &ctrl->dev;
+ sdev->dev.bus = &spmi_bus_type;
+ sdev->dev.type = &spmi_dev_type;
+ return sdev;
+}
+EXPORT_SYMBOL_GPL(spmi_device_alloc);
+
+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();
+
+ 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");
+
+ 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) {
+ 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;
+
+ ret = device_add(&ctrl->dev);
+ if (ret)
+ return ret;
+
+ if (IS_ENABLED(CONFIG_OF))
+ of_spmi_register_devices(ctrl);
+
+ dev_dbg(&ctrl->dev, "spmi-%d registered: dev:%p\n",
+ ctrl->nr, &ctrl->dev);
+
+ return 0;
+};
+EXPORT_SYMBOL_GPL(spmi_controller_add);
+
+/* Remove a device associated with a controller */
+static int spmi_ctrl_remove_device(struct device *dev, void *data)
+{
+ struct spmi_device *spmidev = to_spmi_device(dev);
+ if (dev->type == &spmi_dev_type)
+ spmi_device_remove(spmidev);
+ return 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)
+{
+ int dummy;
+
+ if (!ctrl)
+ return -EINVAL;
+
+ dummy = device_for_each_child(&ctrl->dev, NULL,
+ spmi_ctrl_remove_device);
+ device_unregister(&ctrl->dev);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(spmi_controller_remove);
+
+int spmi_driver_register(struct spmi_driver *drv)
+{
+ drv->driver.bus = &spmi_bus_type;
+ return driver_register(&drv->driver);
+}
+EXPORT_SYMBOL_GPL(spmi_driver_register);
+
+static void __exit spmi_exit(void)
+{
+ bus_unregister(&spmi_bus_type);
+}
+module_exit(spmi_exit);
+
+static int __init spmi_init(void)
+{
+ return bus_register(&spmi_bus_type);
+}
+postcore_initcall(spmi_init);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("SPMI module");
+MODULE_ALIAS("platform:spmi");
diff --git a/include/dt-bindings/spmi/spmi.h b/include/dt-bindings/spmi/spmi.h
new file mode 100644
index 0000000..d11e1e5
--- /dev/null
+++ b/include/dt-bindings/spmi/spmi.h
@@ -0,0 +1,18 @@
+/* Copyright (c) 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 __DT_BINDINGS_SPMI_H
+#define __DT_BINDINGS_SPMI_H
+
+#define SPMI_USID 0
+#define SPMI_GSID 1
+
+#endif
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 45e9214..677e474 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -432,6 +432,14 @@ struct spi_device_id {
kernel_ulong_t driver_data; /* Data private to the driver */
};
+#define SPMI_NAME_SIZE 32
+#define SPMI_MODULE_PREFIX "spmi:"
+
+struct spmi_device_id {
+ char name[SPMI_NAME_SIZE];
+ kernel_ulong_t driver_data; /* Data private to the driver */
+};
+
/* dmi */
enum dmi_field {
DMI_NONE,
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 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)
+{
+ return dev_get_drvdata(&sdev->dev);
+}
+
+static inline void spmi_device_set_drvdata(struct spmi_device *sdev, void *data)
+{
+ dev_set_drvdata(&sdev->dev, data);
+}
+
+/**
+ * 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)
+{
+ if (sdev)
+ put_device(&sdev->dev);
+}
+
+/**
+ * spmi_device_add: add a device previously constructed via spmi_device_alloc()
+ * @sdev: spmi_device to be added
+ */
+int spmi_device_add(struct spmi_device *sdev);
+
+/**
+ * spmi_device_remove: remove a device
+ * @sdev: spmi_device to be removed
+ */
+void spmi_device_remove(struct spmi_device *sdev);
+
+/**
+ * struct spmi_controller: interface to the SPMI master controller
+ * @dev: Driver model representation of the device.
+ * @nr: board-specific number identifier for this controller/bus
+ * @cmd: sends a non-data command sequence on the SPMI bus.
+ * @read_cmd: sends a register read command sequence on the SPMI bus.
+ * @write_cmd: sends a register write command sequence on the SPMI bus.
+ */
+struct spmi_controller {
+ struct device dev;
+ unsigned int nr;
+ int (*cmd)(struct spmi_controller *ctrl, u8 opcode, u8 sid);
+ int (*read_cmd)(struct spmi_controller *ctrl, u8 opcode,
+ u8 sid, u16 addr, u8 bc, u8 *buf);
+ int (*write_cmd)(struct spmi_controller *ctrl, u8 opcode,
+ u8 sid, u16 addr, u8 bc, const u8 *buf);
+};
+#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)
+
+static inline
+void *spmi_controller_get_drvdata(const struct spmi_controller *ctrl)
+{
+ return dev_get_drvdata(&ctrl->dev);
+}
+
+static inline void spmi_controller_set_drvdata(struct spmi_controller *ctrl,
+ void *data)
+{
+ dev_set_drvdata(&ctrl->dev, data);
+}
+
+/**
+ * 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
+ * newly allocated controller, or calling spmi_controller_put() to discard it.
+ */
+struct spmi_controller *spmi_controller_alloc(struct device *parent,
+ size_t size);
+
+static inline void spmi_controller_put(struct spmi_controller *ctrl)
+{
+ if (ctrl)
+ put_device(&ctrl->dev);
+}
+
+/**
+ * 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);
+
+/**
+ * spmi_controller_remove: Controller tear-down.
+ *
+ * Remove a SPMI controller.
+ */
+int spmi_controller_remove(struct spmi_controller *ctrl);
+
+/**
+ * struct spmi_driver: Manage SPMI generic/slave device driver
+ * @driver: SPMI device drivers should initialize name and owner field of
+ * this structure
+ * @probe: binds this driver to a SPMI device.
+ * @remove: unbinds this driver from the SPMI device.
+ * @shutdown: standard shutdown callback used during powerdown/halt.
+ * @suspend: standard suspend callback used during system suspend
+ * @resume: standard resume callback used during system resume
+ */
+struct spmi_driver {
+ struct device_driver driver;
+ int (*probe)(struct spmi_device *sdev);
+ int (*remove)(struct spmi_device *sdev);
+ void (*shutdown)(struct spmi_device *sdev);
+ int (*suspend)(struct spmi_device *sdev, pm_message_t pmesg);
+ int (*resume)(struct spmi_device *sdev);
+};
+#define to_spmi_driver(d) container_of(d, struct spmi_driver, driver)
+
+/**
+ * spmi_driver_register: Client driver registration with SPMI framework.
+ * @sdrv: client driver to be associated with client-device.
+ *
+ * This API will register the client driver with the SPMI framework.
+ * It is called from the driver's module-init function.
+ */
+int spmi_driver_register(struct spmi_driver *sdrv);
+
+/**
+ * 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)
+{
+ if (sdrv)
+ driver_unregister(&sdrv->driver);
+}
+
+#define module_spmi_driver(__spmi_driver) \
+ module_driver(__spmi_driver, spmi_driver_register, \
+ spmi_driver_unregister)
+
+/**
+ * spmi_register_read() - register read
+ * @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.
+ */
+int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf);
+
+/**
+ * spmi_ext_register_read() - extended register read
+ * @sdev: SPMI device
+ * @addr: slave register address (8-bit address).
+ * @buf: buffer to be populated with data from the Slave.
+ * @len: the request number of bytes to read (up to 16 bytes).
+ *
+ * Reads up to 16 bytes of data from the extended register space on a
+ * Slave device.
+ */
+int spmi_ext_register_read(struct spmi_device *sdev, u8 addr, u8 *buf,
+ size_t len);
+
+/**
+ * spmi_ext_register_readl() - extended register read long
+ * @sdev: SPMI device
+ * @addr: slave register address (16-bit address).
+ * @buf: buffer to be populated with data from the Slave.
+ * @len: the request number of bytes to read (up to 8 bytes).
+ *
+ * Reads up to 8 bytes of data from the extended register space on a
+ * Slave device using 16-bit address.
+ */
+int spmi_ext_register_readl(struct spmi_device *sdev, u16 addr, u8 *buf,
+ size_t len);
+
+/**
+ * spmi_register_write() - register write
+ * @sdev: SPMI device
+ * @addr: slave register address (5-bit address).
+ * @buf: buffer containing the data to be transferred to the Slave.
+ *
+ * Writes 1 byte of data to a Slave device register.
+ */
+int spmi_register_write(struct spmi_device *sdev, u8 addr, const u8 *buf);
+
+/**
+ * spmi_register_zero_write() - register zero write
+ * @sdev: SPMI device
+ * @data: the data to be written to register 0 (7-bits).
+ *
+ * Writes data to register 0 of the Slave device.
+ */
+int spmi_register_zero_write(struct spmi_device *sdev, u8 data);
+
+/**
+ * spmi_ext_register_write() - extended register write
+ * @sdev: SPMI device
+ * @addr: slave register address (8-bit address).
+ * @buf: buffer containing the data to be transferred to the Slave.
+ * @len: the request number of bytes to read (up to 16 bytes).
+ *
+ * Writes up to 16 bytes of data to the extended register space of a
+ * Slave device.
+ */
+int spmi_ext_register_write(struct spmi_device *sdev, u8 addr,
+ const u8 *buf, size_t len);
+
+/**
+ * spmi_ext_register_writel() - extended register write long
+ * @sdev: SPMI device
+ * @addr: slave register address (16-bit address).
+ * @buf: buffer containing the data to be transferred to the Slave.
+ * @len: the request number of bytes to read (up to 8 bytes).
+ *
+ * Writes up to 8 bytes of data to the extended register space of a
+ * Slave device using 16-bit address.
+ */
+int spmi_ext_register_writel(struct spmi_device *sdev, u16 addr,
+ const u8 *buf, size_t len);
+
+/**
+ * spmi_command_reset() - sends RESET command to the specified slave
+ * @sdev: SPMI device
+ *
+ * The Reset command initializes the Slave and forces all registers to
+ * their reset values. The Slave shall enter the STARTUP state after
+ * receiving a Reset command.
+ *
+ * Returns
+ * -EINVAL for invalid slave identifier.
+ * -EPERM if the SPMI transaction is denied due to permission issues.
+ * -EIO if the SPMI transaction fails (parity errors, etc).
+ * -ETIMEDOUT if the SPMI transaction times out.
+ */
+int spmi_command_reset(struct spmi_device *sdev);
+
+/**
+ * spmi_command_sleep() - sends SLEEP command to the specified slave
+ * @sdev: SPMI device
+ *
+ * The Sleep command causes the Slave to enter the user defined SLEEP state.
+ *
+ * Returns
+ * -EINVAL for invalid slave identifier.
+ * -EPERM if the SPMI transaction is denied due to permission issues.
+ * -EIO if the SPMI transaction fails (parity errors, etc).
+ * -ETIMEDOUT if the SPMI transaction times out.
+ */
+int spmi_command_sleep(struct spmi_device *sdev);
+
+/**
+ * spmi_command_wakeup() - sends WAKEUP command to the specified slave
+ * @sdev: SPMI device
+ *
+ * The Wakeup command causes the Slave to move from the SLEEP state to
+ * the ACTIVE state.
+ *
+ * Returns
+ * -EINVAL for invalid slave identifier.
+ * -EPERM if the SPMI transaction is denied due to permission issues.
+ * -EIO if the SPMI transaction fails (parity errors, etc).
+ * -ETIMEDOUT if the SPMI transaction times out.
+ */
+int spmi_command_wakeup(struct spmi_device *sdev);
+
+/**
+ * spmi_command_shutdown() - sends SHUTDOWN command to the specified slave
+ * @sdev: SPMI device
+ *
+ * The Shutdown command causes the Slave to enter the SHUTDOWN state.
+ *
+ * Returns
+ * -EINVAL for invalid slave identifier.
+ * -EPERM if the SPMI transaction is denied due to permission issues.
+ * -EIO if the SPMI transaction fails (parity errors, etc).
+ * -ETIMEDOUT if the SPMI transaction times out.
+ */
+int spmi_command_shutdown(struct spmi_device *sdev);
+
+#endif
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
The Qualcomm PMIC Arbiter, in addition to being a basic SPMI controller,
also implements interrupt handling for slave devices. Note, this is
outside the scope of SPMI, as SPMI leaves interrupt handling completely
unspecified.
Extend the driver to provide a irq_chip implementation and chained irq
handling which allows for these interrupts to be used.
Signed-off-by: Josh Cartwright <[email protected]>
---
drivers/spmi/spmi-pmic-arb.c | 359 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 357 insertions(+), 2 deletions(-)
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 92e1416..6926a6b 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -13,6 +13,9 @@
#include <linux/err.h>
#include <linux/interrupt.h>
#include <linux/io.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -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;
+ 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)
@@ -315,18 +323,340 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl,
return rc;
}
+enum qpnpint_regs {
+ QPNPINT_REG_RT_STS = 0x10,
+ QPNPINT_REG_SET_TYPE = 0x11,
+ QPNPINT_REG_POLARITY_HIGH = 0x12,
+ QPNPINT_REG_POLARITY_LOW = 0x13,
+ QPNPINT_REG_LATCHED_CLR = 0x14,
+ QPNPINT_REG_EN_SET = 0x15,
+ QPNPINT_REG_EN_CLR = 0x16,
+ QPNPINT_REG_LATCHED_STS = 0x18,
+};
+
+struct spmi_pmic_arb_qpnpint_type {
+ u8 type; /* 1 -> edge */
+ u8 polarity_high;
+ u8 polarity_low;
+} __packed;
+
+/* Simplified accessor functions for irqchip callbacks */
+static void qpnpint_spmi_write(struct irq_data *d, u8 reg, void *buf,
+ size_t len)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 sid = d->hwirq >> 24;
+ u8 per = d->hwirq >> 16;
+
+ if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+ (per << 8) + reg, len, buf))
+ dev_err_ratelimited(&pa->spmic->dev,
+ "failed irqchip transaction on %x\n",
+ d->irq);
+}
+
+static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 sid = d->hwirq >> 24;
+ u8 per = d->hwirq >> 16;
+
+ if (pmic_arb_read_cmd(pa->spmic, SPMI_CMD_EXT_READL, sid,
+ (per << 8) + reg, len, buf))
+ dev_err_ratelimited(&pa->spmic->dev,
+ "failed irqchip transaction on %x\n",
+ d->irq);
+}
+
+static void periph_interrupt(struct spmi_pmic_arb_dev *pa, u8 apid)
+{
+ unsigned int irq;
+ u32 status;
+ int id;
+
+ status = readl_relaxed(pa->intr + SPMI_PIC_IRQ_STATUS(apid));
+ while (status) {
+ id = ffs(status) - 1;
+ status &= ~(1 << id);
+ irq = irq_find_mapping(pa->domain,
+ pa->apid_to_ppid[apid] << 16
+ | id << 8
+ | apid);
+ generic_handle_irq(irq);
+ }
+}
+
+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;*/
+ 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);
+}
+
+static void qpnpint_irq_ack(struct irq_data *d)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 irq = d->hwirq >> 8;
+ u8 apid = d->hwirq;
+ unsigned long flags;
+ u8 data;
+
+ dev_dbg(&pa->spmic->dev, "hwirq %lu irq: %d\n", d->hwirq, d->irq);
+
+ spin_lock_irqsave(&pa->lock, flags);
+ writel_relaxed(1 << irq, pa->intr + SPMI_PIC_IRQ_CLEAR(apid));
+ spin_unlock_irqrestore(&pa->lock, flags);
+
+ data = 1 << irq;
+ qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
+}
+
+static void qpnpint_irq_mask(struct irq_data *d)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 irq = d->hwirq >> 8;
+ u8 apid = d->hwirq;
+ unsigned long flags;
+ u32 status;
+ u8 data;
+
+ dev_dbg(&pa->spmic->dev, "hwirq %lu irq: %d\n", d->hwirq, d->irq);
+
+ spin_lock_irqsave(&pa->lock, flags);
+ status = readl_relaxed(pa->intr + SPMI_PIC_ACC_ENABLE(apid));
+ if (status & SPMI_PIC_ACC_ENABLE_BIT) {
+ status = status & ~SPMI_PIC_ACC_ENABLE_BIT;
+ writel_relaxed(status, pa->intr + SPMI_PIC_ACC_ENABLE(apid));
+ }
+ spin_unlock_irqrestore(&pa->lock, flags);
+
+ data = 1 << irq;
+ qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);
+}
+
+static void qpnpint_irq_unmask(struct irq_data *d)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 irq = d->hwirq >> 8;
+ u8 apid = d->hwirq;
+ unsigned long flags;
+ u32 status;
+ u8 data;
+
+ dev_dbg(&pa->spmic->dev, "hwirq %lu irq: %d, apid = %02x\n", d->hwirq,
+ d->irq, apid);
+
+ spin_lock_irqsave(&pa->lock, flags);
+ status = readl_relaxed(pa->intr + SPMI_PIC_ACC_ENABLE(apid));
+ if (!(status & SPMI_PIC_ACC_ENABLE_BIT)) {
+ writel_relaxed(status | SPMI_PIC_ACC_ENABLE_BIT,
+ pa->intr + SPMI_PIC_ACC_ENABLE(apid));
+ }
+ spin_unlock_irqrestore(&pa->lock, flags);
+
+ data = 1 << irq;
+ qpnpint_spmi_write(d, QPNPINT_REG_EN_SET, &data, 1);
+}
+
+static void qpnpint_irq_enable(struct irq_data *d)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ u8 irq = d->hwirq >> 8;
+ u8 data;
+
+ dev_dbg(&pa->spmic->dev, "hwirq %lu irq: %d\n", d->hwirq, d->irq);
+
+ qpnpint_irq_unmask(d);
+
+ data = 1 << irq;
+ qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
+}
+
+static int qpnpint_irq_set_type(struct irq_data *d, unsigned int flow_type)
+{
+ struct spmi_pmic_arb_dev *pa = irq_data_get_irq_chip_data(d);
+ struct spmi_pmic_arb_qpnpint_type type;
+ u8 irq = d->hwirq >> 8;
+
+ dev_dbg(&pa->spmic->dev, "qpnpint_irq_set_type %p, %x\n", d, flow_type);
+
+ qpnpint_spmi_read(d, QPNPINT_REG_SET_TYPE, &type, 3);
+
+ if (flow_type & (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING)) {
+ type.type |= 1 << irq;
+ if (flow_type & IRQF_TRIGGER_RISING)
+ type.polarity_high |= 1 << irq;
+ if (flow_type & IRQF_TRIGGER_FALLING)
+ type.polarity_low |= 1 << irq;
+ } else {
+ if ((flow_type & (IRQF_TRIGGER_HIGH)) &&
+ (flow_type & (IRQF_TRIGGER_LOW)))
+ return -EINVAL;
+
+ type.type &= ~(1 << irq); /* level trig */
+ if (flow_type & IRQF_TRIGGER_HIGH)
+ type.polarity_high |= 1 << irq;
+ else
+ type.polarity_low |= 1 << irq;
+ }
+
+ qpnpint_spmi_write(d, QPNPINT_REG_SET_TYPE, &type, 3);
+ return 0;
+}
+
+static struct irq_chip pmic_arb_irqchip = {
+ .name = "pmic_arb",
+ .irq_enable = qpnpint_irq_enable,
+ .irq_ack = qpnpint_irq_ack,
+ .irq_mask = qpnpint_irq_mask,
+ .irq_unmask = qpnpint_irq_unmask,
+ .irq_set_type = qpnpint_irq_set_type,
+ .flags = IRQCHIP_MASK_ON_SUSPEND
+ | IRQCHIP_SKIP_SET_WAKE,
+};
+
+struct spmi_pmic_arb_irq_spec {
+ unsigned slave:4;
+ unsigned per:8;
+ unsigned irq:3;
+};
+
+static int search_mapping_table(struct spmi_pmic_arb_dev *pa,
+ struct spmi_pmic_arb_irq_spec *spec,
+ u8 *apid)
+{
+ u16 ppid = spec->slave << 8 | spec->per;
+ u32 *mapping_table = pa->mapping_table;
+ int index = 0, i;
+ u32 data;
+
+ for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
+ data = mapping_table[index];
+
+ if (ppid & (1 << SPMI_MAPPING_BIT_INDEX(data))) {
+ if (SPMI_MAPPING_BIT_IS_1_FLAG(data)) {
+ index = SPMI_MAPPING_BIT_IS_1_RESULT(data);
+ } else {
+ *apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
+ return 0;
+ }
+ } else {
+ if (SPMI_MAPPING_BIT_IS_0_FLAG(data)) {
+ index = SPMI_MAPPING_BIT_IS_0_RESULT(data);
+ } else {
+ *apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
+ return 0;
+ }
+ }
+ }
+
+ return -ENODEV;
+}
+
+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 */
+ if (apid > pa->max_apid)
+ pa->max_apid = apid;
+ if (apid < pa->min_apid)
+ pa->min_apid = apid;
+
+ *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 qpnpint_irq_domain_map(struct irq_domain *d,
+ unsigned int virq,
+ irq_hw_number_t hwirq)
+{
+ struct spmi_pmic_arb_dev *pa = d->host_data;
+
+ dev_dbg(&pa->spmic->dev, "virq = %u, hwirq = %lu\n", virq, hwirq);
+
+ irq_set_chip_and_handler(virq, &pmic_arb_irqchip, handle_level_irq);
+ irq_set_chip_data(virq, d->host_data);
+#ifdef CONFIG_ARM
+ set_irq_flags(virq, IRQF_VALID);
+#else
+ irq_set_noprobe(virq);
+#endif
+ return 0;
+}
+
+static const struct irq_domain_ops pmic_arb_irq_domain_ops = {
+ .map = qpnpint_irq_domain_map,
+ .xlate = qpnpint_irq_domain_dt_translate,
+};
+
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;
+ 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;
+ 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;
+ }
+
+ 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;
@@ -381,6 +732,10 @@ err_put_ctrl:
static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
{
struct spmi_controller *ctrl = platform_get_drvdata(pdev);
+ struct spmi_pmic_arb_dev *pa = spmi_controller_get_drvdata(ctrl);
+ irq_set_chained_handler(pa->irq, NULL);
+ irq_set_handler_data(pa->irq, NULL);
+ irq_domain_remove(pa->domain);
spmi_controller_remove(ctrl);
return 0;
}
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Signed-off-by: Josh Cartwright <[email protected]>
---
Documentation/devicetree/bindings/spmi/spmi.txt | 41 +++++++++++++++++++++++++
1 file changed, 41 insertions(+)
create mode 100644 Documentation/devicetree/bindings/spmi/spmi.txt
diff --git a/Documentation/devicetree/bindings/spmi/spmi.txt b/Documentation/devicetree/bindings/spmi/spmi.txt
new file mode 100644
index 0000000..32a180d
--- /dev/null
+++ b/Documentation/devicetree/bindings/spmi/spmi.txt
@@ -0,0 +1,41 @@
+System Power Management Interface (SPMI) Controller
+
+This document defines a generic set of bindings for use by SPMI controllers. A
+controller is modelled in device tree as a node with zero or more child nodes,
+each representing a unique slave on the bus.
+
+Required properties:
+- #address-cells : must be set to 2
+- #size-cells : must be set to 0
+
+Child nodes:
+
+An SPMI controller node can contain zero or more child nodes representing slave
+devices on the bus. Child 'reg' properties are specified as an address, type
+pair. The address must be in the range 0-15 (4 bits). The type must be one of
+SPMI_USID or SPMI_GSID (1) for Unique Slave ID or Group Slave ID respectively.
+These are the identifiers "statically assigned by the system integrator", as
+per the SPMI spec.
+
+Each child node must have one and only one 'reg' entry of type SPMI_USID.
+
+#include <dt-bindings/spmi/spmi.h>
+
+ controller@.. {
+ compatible = "...";
+ reg = <...>;
+
+ #address-cells = <2>;
+ #size-cells <0>;
+
+ child@0 {
+ compatible = "...";
+ reg = <0 SPMI_USID>;
+ };
+
+ child@7 {
+ compatible = "...";
+ reg = <7 SPMI_USID
+ 3 SPMI_GSID>;
+ };
+ };
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Adapt the existing pm8xxx driver to work with the RTC available on
Qualcomm's 8941 PMIC. In order to do this, rework the register access
functions to use a regmap provided by the parent driver. Account for
slight differences in the RTC address space by parameterizing addresses
and providing a chip-specific initialization function.
Signed-off-by: Josh Cartwright <[email protected]>
---
drivers/rtc/Kconfig | 1 -
drivers/rtc/rtc-pm8xxx.c | 229 +++++++++++++++++++++++++++++------------------
2 files changed, 143 insertions(+), 87 deletions(-)
diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 9654aa3..19b89ee 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -1177,7 +1177,6 @@ config RTC_DRV_LPC32XX
config RTC_DRV_PM8XXX
tristate "Qualcomm PMIC8XXX RTC"
- depends on MFD_PM8XXX
help
If you say yes here you get support for the
Qualcomm PMIC8XXX RTC.
diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
index 03f8f75..a9044d4 100644
--- a/drivers/rtc/rtc-pm8xxx.c
+++ b/drivers/rtc/rtc-pm8xxx.c
@@ -1,4 +1,5 @@
/* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
+ * Copyright (c) 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
@@ -13,35 +14,33 @@
#include <linux/module.h>
#include <linux/init.h>
#include <linux/rtc.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
#include <linux/pm.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/mfd/pm8xxx/core.h>
#include <linux/mfd/pm8xxx/rtc.h>
-
-/* RTC Register offsets from RTC CTRL REG */
-#define PM8XXX_ALARM_CTRL_OFFSET 0x01
-#define PM8XXX_RTC_WRITE_OFFSET 0x02
-#define PM8XXX_RTC_READ_OFFSET 0x06
-#define PM8XXX_ALARM_RW_OFFSET 0x0A
-
/* RTC_CTRL register bit fields */
#define PM8xxx_RTC_ENABLE BIT(7)
#define PM8xxx_RTC_ALARM_ENABLE BIT(1)
#define PM8xxx_RTC_ALARM_CLEAR BIT(0)
#define NUM_8_BIT_RTC_REGS 0x4
-
/**
* struct pm8xxx_rtc - rtc driver internal structure
* @rtc: rtc device for this driver.
* @rtc_alarm_irq: rtc alarm irq number.
- * @rtc_base: address of rtc control register.
+ * @rtc_control_reg: address of control register.
* @rtc_read_base: base address of read registers.
* @rtc_write_base: base address of write registers.
* @alarm_rw_base: base address of alarm registers.
+ * @alarm_ctrl1: address of alarm ctrl1 register.
+ * @alarm_ctrl2: address of alarm ctrl2 register (only used on pm8941).
+ * @alarm_clear: RTC-specific callback to clear alarm interrupt.
* @ctrl_reg: rtc control register.
* @rtc_dev: device structure.
* @ctrl_reg_lock: spinlock protecting access to ctrl_reg.
@@ -49,51 +48,34 @@
struct pm8xxx_rtc {
struct rtc_device *rtc;
int rtc_alarm_irq;
- int rtc_base;
+ int rtc_control_reg;
int rtc_read_base;
int rtc_write_base;
int alarm_rw_base;
- u8 ctrl_reg;
+ int alarm_ctrl1;
+ int alarm_ctrl2;
+ int (*alarm_clear)(struct pm8xxx_rtc *);
+ u8 ctrl_reg;
struct device *rtc_dev;
spinlock_t ctrl_reg_lock;
+ struct regmap *regmap;
};
/*
* The RTC registers need to be read/written one byte at a time. This is a
* hardware limitation.
*/
-static int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
- int base, int count)
+static inline int pm8xxx_read_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
+ int base, int count)
{
- int i, rc;
- struct device *parent = rtc_dd->rtc_dev->parent;
-
- for (i = 0; i < count; i++) {
- rc = pm8xxx_readb(parent, base + i, &rtc_val[i]);
- if (rc < 0) {
- dev_err(rtc_dd->rtc_dev, "PMIC read failed\n");
- return rc;
- }
- }
-
- return 0;
+ return regmap_bulk_read(rtc_dd->regmap, base, rtc_val, count);
}
-static int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd, u8 *rtc_val,
- int base, int count)
+static inline int pm8xxx_write_wrapper(struct pm8xxx_rtc *rtc_dd,
+ const u8 *rtc_val,
+ int base, int count)
{
- int i, rc;
- struct device *parent = rtc_dd->rtc_dev->parent;
-
- for (i = 0; i < count; i++) {
- rc = pm8xxx_writeb(parent, base + i, rtc_val[i]);
- if (rc < 0) {
- dev_err(rtc_dd->rtc_dev, "PMIC write failed\n");
- return rc;
- }
- }
-
- return 0;
+ return regmap_bulk_write(rtc_dd->regmap, base, rtc_val, count);
}
/*
@@ -125,8 +107,8 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
if (ctrl_reg & PM8xxx_RTC_ALARM_ENABLE) {
alarm_enabled = 1;
ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
- 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(dev, "Write to RTC control register "
"failed\n");
@@ -161,8 +143,8 @@ static int pm8xxx_rtc_set_time(struct device *dev, struct rtc_time *tm)
if (alarm_enabled) {
ctrl_reg |= PM8xxx_RTC_ALARM_ENABLE;
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
- 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(dev, "Write to RTC control register "
"failed\n");
@@ -255,7 +237,8 @@ static int pm8xxx_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
ctrl_reg = alarm->enabled ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
(ctrl_reg & ~PM8xxx_RTC_ALARM_ENABLE);
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(dev, "Write to RTC control register failed\n");
goto rtc_rw_fail;
@@ -316,7 +299,8 @@ static int pm8xxx_rtc_alarm_irq_enable(struct device *dev, unsigned int enable)
ctrl_reg = (enable) ? (ctrl_reg | PM8xxx_RTC_ALARM_ENABLE) :
(ctrl_reg & ~PM8xxx_RTC_ALARM_ENABLE);
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(dev, "Write to RTC control register failed\n");
goto rtc_rw_fail;
@@ -351,7 +335,8 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
ctrl_reg = rtc_dd->ctrl_reg;
ctrl_reg &= ~PM8xxx_RTC_ALARM_ENABLE;
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
dev_err(rtc_dd->rtc_dev, "Write to RTC control register "
@@ -363,37 +348,105 @@ static irqreturn_t pm8xxx_alarm_trigger(int irq, void *dev_id)
spin_unlock_irqrestore(&rtc_dd->ctrl_reg_lock, irq_flags);
/* Clear RTC alarm register */
- rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
- PM8XXX_ALARM_CTRL_OFFSET, 1);
+ rc = rtc_dd->alarm_clear(rtc_dd);
+ if (rc < 0)
+ dev_err(rtc_dd->rtc_dev, "failed to clear RTC alarm\n");
+
+rtc_alarm_handled:
+ return IRQ_HANDLED;
+}
+
+static int pm8941_alarm_clear(struct pm8xxx_rtc *rtc_dd)
+{
+ u8 ctrl = PM8xxx_RTC_ALARM_CLEAR;
+ return pm8xxx_write_wrapper(rtc_dd, &ctrl, rtc_dd->alarm_ctrl2, 1);
+}
+
+static int pm8941_chip_init(struct platform_device *pdev,
+ struct pm8xxx_rtc *rtc)
+{
+ u32 addr[2];
+ int err;
+
+ err = of_property_read_u32_array(pdev->dev.of_node,
+ "reg", addr, 2);
+ if (err || addr[0] > 0xFFFF || addr[1] > 0xFFFF) {
+ dev_err(&pdev->dev, "RTC IO resources absent or invalid\n");
+ return err;
+ }
+
+ rtc->alarm_clear = pm8941_alarm_clear;
+
+ rtc->rtc_control_reg = addr[0] + 0x46;
+ rtc->rtc_write_base = addr[0] + 0x40;
+ rtc->rtc_read_base = addr[0] + 0x48;
+ rtc->alarm_rw_base = addr[1] + 0x40;
+ rtc->alarm_ctrl1 = addr[1] + 0x46;
+ rtc->alarm_ctrl2 = addr[1] + 0x48;
+ return 0;
+}
+
+static int pm8xxx_alarm_clear(struct pm8xxx_rtc *rtc_dd)
+{
+ u8 ctrl_reg;
+ int rc;
+
+ rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->alarm_ctrl1, 1);
if (rc < 0) {
dev_err(rtc_dd->rtc_dev, "RTC Alarm control register read "
"failed\n");
- goto rtc_alarm_handled;
+ return rc;
}
ctrl_reg &= ~PM8xxx_RTC_ALARM_CLEAR;
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base +
- PM8XXX_ALARM_CTRL_OFFSET, 1);
- if (rc < 0)
- dev_err(rtc_dd->rtc_dev, "Write to RTC Alarm control register"
- " failed\n");
+ return pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->alarm_ctrl1, 1);
+}
-rtc_alarm_handled:
- return IRQ_HANDLED;
+static int pm8xxx_chip_init(struct platform_device *pdev,
+ struct pm8xxx_rtc *rtc)
+{
+ const struct pm8xxx_rtc_platform_data *pdata =
+ dev_get_platdata(&pdev->dev);
+ struct resource *res;
+
+ if (!pdata) {
+ dev_err(&pdev->dev, "Platform data not initialized.\n");
+ return -ENXIO;
+ }
+
+ if (pdata->rtc_write_enable)
+ pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_IO,
+ "pmic_rtc_base");
+ if (!res) {
+ dev_err(&pdev->dev, "RTC IO resource absent\n");
+ return -ENXIO;
+ }
+
+ rtc->rtc_control_reg = res->start;
+ rtc->rtc_write_base = res->start + 0x02;
+ rtc->rtc_read_base = res->start + 0x06;
+ rtc->alarm_rw_base = res->start + 0x0A;
+ rtc->alarm_ctrl1 = res->start + 0x01;
+
+ rtc->alarm_clear = pm8xxx_alarm_clear;
+ return 0;
}
+static const struct of_device_id pm8xxx_rtc_idtable[] = {
+ { .compatible = "qcom,pm8941-rtc", pm8941_chip_init },
+ {},
+};
+MODULE_DEVICE_TABLE(of, pm8xxx_rtc_idtable);
+
static int pm8xxx_rtc_probe(struct platform_device *pdev)
{
- int rc;
- u8 ctrl_reg;
- bool rtc_write_enable = false;
+ int (*chip_init)(struct platform_device *pdev, struct pm8xxx_rtc *rtc);
+ const struct device_node *node = pdev->dev.of_node;
struct pm8xxx_rtc *rtc_dd;
- struct resource *rtc_resource;
- const struct pm8xxx_rtc_platform_data *pdata =
- dev_get_platdata(&pdev->dev);
-
- if (pdata != NULL)
- rtc_write_enable = pdata->rtc_write_enable;
+ u8 ctrl_reg;
+ int rc;
rtc_dd = devm_kzalloc(&pdev->dev, sizeof(*rtc_dd), GFP_KERNEL);
if (rtc_dd == NULL) {
@@ -401,33 +454,40 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
return -ENOMEM;
}
- /* Initialise spinlock to protect RTC control register */
- spin_lock_init(&rtc_dd->ctrl_reg_lock);
+ chip_init = pm8xxx_chip_init;
+ if (node) {
+ const
+ struct of_device_id *id = of_match_node(pm8xxx_rtc_idtable,
+ pdev->dev.of_node);
+ if (id && id->data)
+ chip_init = id->data;
+ }
- rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
- if (rtc_dd->rtc_alarm_irq < 0) {
- dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+ rc = chip_init(pdev, rtc_dd);
+ if (rc) {
+ dev_err(&pdev->dev, "Failed to initialize chip.\n");
return -ENXIO;
}
- rtc_resource = platform_get_resource_byname(pdev, IORESOURCE_IO,
- "pmic_rtc_base");
- if (!(rtc_resource && rtc_resource->start)) {
- dev_err(&pdev->dev, "RTC IO resource absent!\n");
+ rtc_dd->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+ if (!rtc_dd->regmap) {
+ dev_err(&pdev->dev, "Unable to get regmap.\n");
return -ENXIO;
}
- rtc_dd->rtc_base = rtc_resource->start;
+ /* Initialise spinlock to protect RTC control register */
+ spin_lock_init(&rtc_dd->ctrl_reg_lock);
- /* Setup RTC register addresses */
- rtc_dd->rtc_write_base = rtc_dd->rtc_base + PM8XXX_RTC_WRITE_OFFSET;
- rtc_dd->rtc_read_base = rtc_dd->rtc_base + PM8XXX_RTC_READ_OFFSET;
- rtc_dd->alarm_rw_base = rtc_dd->rtc_base + PM8XXX_ALARM_RW_OFFSET;
+ rtc_dd->rtc_alarm_irq = platform_get_irq(pdev, 0);
+ if (rtc_dd->rtc_alarm_irq < 0) {
+ dev_err(&pdev->dev, "Alarm IRQ resource absent!\n");
+ return -ENXIO;
+ }
rtc_dd->rtc_dev = &pdev->dev;
/* Check if the RTC is on, else turn it on */
- rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base, 1);
+ rc = pm8xxx_read_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(&pdev->dev, "RTC control register read failed!\n");
return rc;
@@ -435,8 +495,8 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
if (!(ctrl_reg & PM8xxx_RTC_ENABLE)) {
ctrl_reg |= PM8xxx_RTC_ENABLE;
- rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg, rtc_dd->rtc_base,
- 1);
+ rc = pm8xxx_write_wrapper(rtc_dd, &ctrl_reg,
+ rtc_dd->rtc_control_reg, 1);
if (rc < 0) {
dev_err(&pdev->dev, "Write to RTC control register "
"failed\n");
@@ -444,10 +504,6 @@ static int pm8xxx_rtc_probe(struct platform_device *pdev)
}
}
- rtc_dd->ctrl_reg = ctrl_reg;
- if (rtc_write_enable == true)
- pm8xxx_rtc_ops.set_time = pm8xxx_rtc_set_time;
-
platform_set_drvdata(pdev, rtc_dd);
/* Register the RTC device */
@@ -516,6 +572,7 @@ static struct platform_driver pm8xxx_rtc_driver = {
.name = PM8XXX_RTC_DEV_NAME,
.owner = THIS_MODULE,
.pm = &pm8xxx_rtc_pm_ops,
+ .of_match_table = pm8xxx_rtc_idtable,
},
};
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Mon, Oct 28, 2013 at 01:12:35PM -0500, Josh Cartwright wrote:
> Add basic support for the System Power Management Interface (SPMI) bus.
> This is a simple implementation which only implements register accesses
> via the Extended Register Read/Write Long commands.
This looks good - I've applied it on the assumption that the underlying
interfaces won't change and the suspicion that it'll be after the merge
window before users get merged. However I did provide a tag at:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/spmi
which I'm happy to see pulled into any tree that requires it. I can
drop the patch for now if there would be a problem with getting it in
during the merge window.
On 10/28/13 11:12, Josh Cartwright wrote:
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 914c3d1..0a288cb 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -476,6 +476,16 @@ config MFD_PM8XXX_IRQ
> This is required to use certain other PM 8xxx features, such as GPIO
> and MPP.
>
> +config MFD_PM8X41
> + bool "Qualcomm PM8X41 PMIC"
Can this be tristate?
> + depends on ARCH_MSM
It would be nice if we didn't have to depend on ARCH_MSM so we get some
more build coverage but the driver is so simple it probably doesn't matter.
> + select REGMAP_SPMI
> + help
> + This enables basic support for the Qualcomm 8941 and 8841 PMICs. These
> + PMICs are currently used with the Snapdragon 800 series of SoCs. Note, that
> + this will only be useful paired with descriptions of the independent functions
> + as children nodes in the device tree.
> +
> config MFD_RDC321X
> tristate "RDC R-321x southbridge"
> select MFD_CORE
> diff --git a/drivers/mfd/pm8x41.c b/drivers/mfd/pm8x41.c
> new file mode 100644
> index 0000000..0a57221
> --- /dev/nullalso
> +++ b/drivers/mfd/pm8x41.c
> @@ -0,0 +1,64 @@
[...]
> +
> +static int pm8x41_probe(struct spmi_device *sdev)
> +{
> + struct regmap *regmap;
> +
> + regmap = devm_regmap_init_spmi(sdev, &pm8x41_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_dbg(&sdev->dev, "regmap creation failed.\n");
> + return PTR_ERR(regmap);
> + }
> +
> + return of_platform_populate(sdev->dev.of_node, NULL, NULL, &sdev->dev);
> +}
> +
> +static struct of_device_id pm8x41_id_table[] = {
const?
> + { "qcom,pm8841", },
> + { "qcom,pm8941", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, pm8x41_id_table);
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Mon, Oct 28, 2013 at 1:12 PM, Josh Cartwright <[email protected]> wrote:
> 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]>
I'm assuming the rest of this is not going to make 3.13, so I've
applied this for 3.13.
Rob
Hi Josh,
On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> 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
>
> diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> new file mode 100644
> index 0000000..68949aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> @@ -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".
> +- 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.
Regards,
Ivan
> +- #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@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>;
> + };
Hi Josh,
On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> Document the bindings used to describe the Qualcomm 8x41 PMICs.
>
> Signed-off-by: Josh Cartwright <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/pm8x41.txt | 33 ++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
>
> diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> new file mode 100644
> index 0000000..6afd4ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> @@ -0,0 +1,33 @@
> +Qualcomm PM8841 and PM8941 PMIC multi-function devices
> +
> +The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are
> +interfaced to the chip via the SPMI (System Power Management Interface) bus.
> +Support for multiple independent functions are implemented by splitting the
> +16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes
> +each. A function can consume one or more of these fixed-size register regions.
> +
> +Required properties:
> +- compatible: Must be one of:
> + "qcom,pm8841"
> + "qcom,pm8941"
> +- reg: Specifies the SPMI USID slave address for this device
> +- #address-cells = <1>
> +- #size-cells = <0>
> +
> +Each child node represents a function of the PM8x41. Each child 'reg' entry
> +describes an offset within the USID slave address where the region starts.
> +
> +Example:
> +
> +pm8941@0 {
> + compatible = "qcom,pm8941";
> + reg = <0x0>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + rtc {
> + compatible = "qcom,pm8941-rtc";
> + reg = <0x6000 0x6100>;
This doesn't look right. Probably #size-cells have to be <1>?
Regards,
Ivan
> + };
> +}
Hi Josh,
On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> 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
>
<snip>
> +#ifdef CONFIG_PM_SLEEP
> +static int spmi_pm_suspend(struct device *dev)
> +{
> + const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
This is what pm_generic_suspend() do. Do we really need this wrapper?
> +
> + 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
> + )
> +};
> +
<snip>
> +
> +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.
> + 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,
> +};
> +
<snip>
> +
> +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?
> + }
> +
> + 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;
> +
> + ret = device_add(&ctrl->dev);
> + if (ret)
> + return ret;
> +
> + if (IS_ENABLED(CONFIG_OF))
> + of_spmi_register_devices(ctrl);
Check for error code here?
> +
> + dev_dbg(&ctrl->dev, "spmi-%d registered: dev:%p\n",
> + ctrl->nr, &ctrl->dev);
> +
> + return 0;
> +};
> +EXPORT_SYMBOL_GPL(spmi_controller_add);
> +
Regards,
Ivan
On Tue, Oct 29, 2013 at 04:18:35PM +0200, Ivan T. Ivanov wrote:
> On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> > Document the bindings used to describe the Qualcomm 8x41 PMICs.
> >
> > Signed-off-by: Josh Cartwright <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mfd/pm8x41.txt | 33 ++++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> > new file mode 100644
> > index 0000000..6afd4ce
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> > @@ -0,0 +1,33 @@
> > +Qualcomm PM8841 and PM8941 PMIC multi-function devices
> > +
> > +The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are
> > +interfaced to the chip via the SPMI (System Power Management Interface) bus.
> > +Support for multiple independent functions are implemented by splitting the
> > +16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes
> > +each. A function can consume one or more of these fixed-size register regions.
> > +
> > +Required properties:
> > +- compatible: Must be one of:
> > + "qcom,pm8841"
> > + "qcom,pm8941"
> > +- reg: Specifies the SPMI USID slave address for this device
> > +- #address-cells = <1>
> > +- #size-cells = <0>
> > +
> > +Each child node represents a function of the PM8x41. Each child 'reg' entry
> > +describes an offset within the USID slave address where the region starts.
> > +
> > +Example:
> > +
> > +pm8941@0 {
> > + compatible = "qcom,pm8941";
> > + reg = <0x0>;
> > +
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + rtc {
> > + compatible = "qcom,pm8941-rtc";
> > + reg = <0x6000 0x6100>;
>
> This doesn't look right. Probably #size-cells have to be <1>?
Some functions of the PMIC actually consume more than one fixed-size
region of the slave address space. This example is showing one such
peripheral (consuming the region starting at 0x6000 and another at
0x6100).
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 04:08:29PM +0200, Ivan T. Ivanov wrote:
> On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> > 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
> >
> > diff --git a/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> > new file mode 100644
> > index 0000000..68949aa
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/spmi/qcom,spmi-pmic-arb.txt
> > @@ -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".
> > +- 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.
Indeed, good catch. I'll fix it up.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Couple of high-level comments on the in-kernel API.
On 10/28/2013 07:12 PM, Josh Cartwright wrote:
> +#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.
> + 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
> + else
> + return 0;
> +}
> +#else
> +#define spmi_pm_suspend NULL
> +#define spmi_pm_resume NULL
> +#endif
[...]
> +/**
> + * 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.
> +{
> + int dummy;
> +
> + if (!ctrl)
> + return -EINVAL;
> +
> + 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.
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(spmi_controller_remove);
> +
[...]
> +/**
> + * 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().
> +{
> + if (sdev)
> + put_device(&sdev->dev);
> +}
[...]
> +#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev)
Should be a inline function for better type safety.
[...]
> +static inline void spmi_controller_put(struct spmi_controller *ctrl)
For symmetry reasons it might make sense to call this spmi_controller_free().
> +{
> + if (ctrl)
> + put_device(&ctrl->dev);
> +}
> +
[....]
> +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.
> + 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.
> +};
> +#define to_spmi_driver(d) container_of(d, struct spmi_driver, driver)
Inline function here as well
[...]
Hi,
On Tue, 2013-10-29 at 10:05 -0500, Josh Cartwright wrote:
> On Tue, Oct 29, 2013 at 04:18:35PM +0200, Ivan T. Ivanov wrote:
> > On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> > > Document the bindings used to describe the Qualcomm 8x41 PMICs.
> > >
> > > Signed-off-by: Josh Cartwright <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/mfd/pm8x41.txt | 33 ++++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mfd/pm8x41.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/mfd/pm8x41.txt b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> > > new file mode 100644
> > > index 0000000..6afd4ce
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/pm8x41.txt
> > > @@ -0,0 +1,33 @@
> > > +Qualcomm PM8841 and PM8941 PMIC multi-function devices
> > > +
> > > +The PM8x41 PMICs are used with the Qualcomm Snapdragon 800 series SoCs, and are
> > > +interfaced to the chip via the SPMI (System Power Management Interface) bus.
> > > +Support for multiple independent functions are implemented by splitting the
> > > +16-bit SPMI slave address space into 256 smaller fixed-size regions, 256 bytes
> > > +each. A function can consume one or more of these fixed-size register regions.
> > > +
> > > +Required properties:
> > > +- compatible: Must be one of:
> > > + "qcom,pm8841"
> > > + "qcom,pm8941"
> > > +- reg: Specifies the SPMI USID slave address for this device
> > > +- #address-cells = <1>
> > > +- #size-cells = <0>
> > > +
> > > +Each child node represents a function of the PM8x41. Each child 'reg' entry
> > > +describes an offset within the USID slave address where the region starts.
> > > +
> > > +Example:
> > > +
> > > +pm8941@0 {
> > > + compatible = "qcom,pm8941";
> > > + reg = <0x0>;
> > > +
> > > + #address-cells = <1>;
> > > + #size-cells = <0>;
> > > +
> > > + rtc {
> > > + compatible = "qcom,pm8941-rtc";
> > > + reg = <0x6000 0x6100>;
> >
> > This doesn't look right. Probably #size-cells have to be <1>?
>
> Some functions of the PMIC actually consume more than one fixed-size
> region of the slave address space. This example is showing one such
> peripheral (consuming the region starting at 0x6000 and another at
> 0x6100).
>
Ok. Thanks for clarification.
Regards,
Ivan
On Mon, 28 Oct 2013, Josh Cartwright wrote:
> The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> 800 series SoC family. This driver exists largely as a glue mfd component,
> it exists to be an owner of an SPMI regmap for children devices
> described in device tree.
>
> Signed-off-by: Josh Cartwright <[email protected]>
> ---
> drivers/mfd/Kconfig | 10 ++++++++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/pm8x41.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
Can you CC me on your next submission, or it may not get applied.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Hey Lars-
Thanks for the feedback. CC'ing Ivan, since he had the same feedback
regarding the PM callbacks.
On Tue, Oct 29, 2013 at 04:21:28PM +0100, Lars-Peter Clausen wrote:
> Couple of high-level comments on the in-kernel API.
>
> On 10/28/2013 07:12 PM, Josh Cartwright wrote:
> > +#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.
>
> > + 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
Sounds good. I'll drop these.
> > +/**
> > + * 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.
Alright.
> > +{
> > + int dummy;
> > +
> > + if (!ctrl)
> > + return -EINVAL;
> > +
> > + 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?
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(spmi_controller_remove);
> > +
> [...]
> > +/**
> > + * 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.
> > +{
> > + if (sdev)
> > + put_device(&sdev->dev);
> > +}
> [...]
> > +#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.
> [...]
> > +static inline void spmi_controller_put(struct spmi_controller *ctrl)
>
> For symmetry reasons it might make sense to call this spmi_controller_free().
>
> > +{
> > + if (ctrl)
> > + put_device(&ctrl->dev);
> > +}
> > +
> [....]
> > +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.
>
> > + 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.
Yep. Will drop.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 08:56:05AM -0700, Lee Jones wrote:
> On Mon, 28 Oct 2013, Josh Cartwright wrote:
>
> > The Qualcomm 8941 and 8841 PMICs are components used with the Snapdragon
> > 800 series SoC family. This driver exists largely as a glue mfd component,
> > it exists to be an owner of an SPMI regmap for children devices
> > described in device tree.
> >
> > Signed-off-by: Josh Cartwright <[email protected]>
> > ---
> > drivers/mfd/Kconfig | 10 ++++++++
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/pm8x41.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>
> Can you CC me on your next submission, or it may not get applied.
Certainly. Sorry about that.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 05:02:03PM +0200, Ivan T. Ivanov wrote:
> On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> > 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]>
[..]
> > +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.
> > +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. Are you
suggesting that we bail completely when we hit a wrongly-described
child?
> > + }
> > +
> > + 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;
> > +
> > + 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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/29/13 08:56, Josh Cartwright wrote:
>
>>> +#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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/28/13 11:12, Josh Cartwright wrote:
> 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?
> + 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?
> +};
> +
> +static void spmi_ctrl_release(struct device *dev)
> +{
> + struct spmi_controller *ctrl = to_spmi_controller(dev);
> + mutex_lock(&ctrl_idr_lock);
> + idr_remove(&ctrl_idr, ctrl->nr);
> + mutex_unlock(&ctrl_idr_lock);
> + kfree(ctrl);
> +}
> +
> +static struct device_type spmi_ctrl_type = {
const?
> + .release = spmi_ctrl_release,
> +};
> +
[...]
> +
> +/*
> + * register read/write: 5-bit address, 1 byte of data
> + * extended register read/write: 8-bit address, up to 16 bytes of data
> + * extended register read/write long: 16-bit address, up to 8 bytes of data
> + */
> +
> +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.
> + return -EINVAL;
> +
> + return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr, 0,
> + buf);
> +}
> +EXPORT_SYMBOL_GPL(spmi_register_read);
> +
[...]
> +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);
> +
> + 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.
> +
> + 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?
> + dev_err(&ctrl->dev,
> + "invalid usid on node %s\n",
> + node->full_name);
> + continue;
> + }
> +
[...]
> 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.
> + * 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)
> +{
> + return dev_get_drvdata(&sdev->dev);
> +}
> +
> +static inline void spmi_device_set_drvdata(struct spmi_device *sdev, void *data)
> +{
> + dev_set_drvdata(&sdev->dev, data);
> +}
> +
> +/**
> + * spmi_controller_alloc: Allocate a new SPMI controller
There should be a dash here instead of colon.
> + * @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);
> +}
> +
[...]
> +
> +/**
> + * 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
> + * newly allocated controller, or calling spmi_controller_put() to discard it.
> + */
> +struct spmi_controller *spmi_controller_alloc(struct device *parent,
> + size_t size);
> +
> +static inline void spmi_controller_put(struct spmi_controller *ctrl)
> +{
> + if (ctrl)
> + put_device(&ctrl->dev);
> +}
This function is missing documentation.
> +
> +/**
> + * 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);
[...]
> +
> +/**
> + * 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)
> +{
> + if (sdrv)
> + driver_unregister(&sdrv->driver);
> +}
> +
> +#define module_spmi_driver(__spmi_driver) \
> + module_driver(__spmi_driver, spmi_driver_register, \
> + spmi_driver_unregister)
> +
> +/**
> + * 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.
> + * @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.
> + */
> +int spmi_register_read(struct spmi_device *sdev, u8 addr, u8 *buf);
> +
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
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:
> > On Mon, 2013-10-28 at 13:12 -0500, Josh Cartwright wrote:
> > > 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]>
> [..]
> > > +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.
>
> > > +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.
>
> > > + }
> > > +
> > > + 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.
> > > + }
> > > + }
> > > +
> > > + 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
>
On 10/29/2013 05:30 PM, Stephen Boyd wrote:
> On 10/29/13 08:56, Josh Cartwright wrote:
>>
>>>> +#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.
Well you'll get a warning, but the quality of the warning message is much
better when an inline function is used.
warning: initialization from incompatible pointer type
vs.
warning: Passing argument 1 of to_smpi_controller() from incompatible
pointer type. Expected struct device * got struct driver *
- Lars
On 10/29/2013 04:56 PM, Josh Cartwright wrote:
>>> +{
>>> + int dummy;
>>> +
>>> + if (!ctrl)
>>> + return -EINVAL;
>>> +
>>> + 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?
>
Resources used by the driver implementing the controller. Usually the driver
state struct will be allocated by spmi_controller_alloc() as well. So if you
store resources in that struct, e.g. a clk you first want to unregister the
spmi controller to make sure that the resources are no longer accessed, then
free the resources and finally drop the reference to the controller so the
memory can be freed. E.g.
static int foobar_remove(struct platform_device *pdev)
{
struct spmi_controller *ctrl = platform_get_drvdata(pdev);
struct foobar *foobar = spmi_controller_get_drvdata(ctrl);
spmi_controller_remove(ctrl);
free_irq(foobar->irq)
clk_put(foobar->clk);
...
spmi_controller_put(ctrl);
return 0;
}
>>> + return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(spmi_controller_remove);
>>> +
>> [...]
>>> +/**
>>> + * 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.
Well, for all the driver cares the device has been freed.
On 10/28, Josh Cartwright wrote:
> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> index 03f8f75..a9044d4 100644
> --- a/drivers/rtc/rtc-pm8xxx.c
> +++ b/drivers/rtc/rtc-pm8xxx.c
> @@ -1,4 +1,5 @@
> /* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> *
Please rewrite this as '2010-2011,2013, The Linux Foundation'
> * 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
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 01:09:27PM -0700, Stephen Boyd wrote:
> On 10/28, Josh Cartwright wrote:
> > diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> > index 03f8f75..a9044d4 100644
> > --- a/drivers/rtc/rtc-pm8xxx.c
> > +++ b/drivers/rtc/rtc-pm8xxx.c
> > @@ -1,4 +1,5 @@
> > /* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> > + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> > *
>
> Please rewrite this as '2010-2011,2013, The Linux Foundation'
Has the Linux Foundation really taken on all prior copyrights of this
codebase? CAF wasn't around in 2010 as part of the LF, so be careful
here.
greg k-h
On 10/29/13 13:15, Greg Kroah-Hartman wrote:
> On Tue, Oct 29, 2013 at 01:09:27PM -0700, Stephen Boyd wrote:
>> On 10/28, Josh Cartwright wrote:
>>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
>>> index 03f8f75..a9044d4 100644
>>> --- a/drivers/rtc/rtc-pm8xxx.c
>>> +++ b/drivers/rtc/rtc-pm8xxx.c
>>> @@ -1,4 +1,5 @@
>>> /* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
>>> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
>>> *
>> Please rewrite this as '2010-2011,2013, The Linux Foundation'
> Has the Linux Foundation really taken on all prior copyrights of this
> codebase? CAF wasn't around in 2010 as part of the LF, so be careful
> here.
>
>From what I recall our lawyers told us to do this and there is some
precedence for this upstream as well.
commit e1858b2a21cd84a855945a4747fb2db41b250c22
Author: Richard Kuo <[email protected]>
Date: Wed Sep 19 16:22:02 2012 -0500
Hexagon: Copyright marking changes
Code Aurora Forum (CAF) is becoming a part of Linux Foundation Labs.
Signed-off-by: Richard Kuo <[email protected]>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 01:20:13PM -0700, Stephen Boyd wrote:
> On 10/29/13 13:15, Greg Kroah-Hartman wrote:
> > On Tue, Oct 29, 2013 at 01:09:27PM -0700, Stephen Boyd wrote:
> >> On 10/28, Josh Cartwright wrote:
> >>> diff --git a/drivers/rtc/rtc-pm8xxx.c b/drivers/rtc/rtc-pm8xxx.c
> >>> index 03f8f75..a9044d4 100644
> >>> --- a/drivers/rtc/rtc-pm8xxx.c
> >>> +++ b/drivers/rtc/rtc-pm8xxx.c
> >>> @@ -1,4 +1,5 @@
> >>> /* Copyright (c) 2010-2011, Code Aurora Forum. All rights reserved.
> >>> + * Copyright (c) 2013, The Linux Foundation. All rights reserved.
> >>> *
> >> Please rewrite this as '2010-2011,2013, The Linux Foundation'
> > Has the Linux Foundation really taken on all prior copyrights of this
> > codebase? CAF wasn't around in 2010 as part of the LF, so be careful
> > here.
> >
>
> >From what I recall our lawyers told us to do this and there is some
> precedence for this upstream as well.
>
> commit e1858b2a21cd84a855945a4747fb2db41b250c22
> Author: Richard Kuo <[email protected]>
> Date: Wed Sep 19 16:22:02 2012 -0500
>
> Hexagon: Copyright marking changes
>
> Code Aurora Forum (CAF) is becoming a part of Linux Foundation Labs.
>
> Signed-off-by: Richard Kuo <[email protected]>
Ok, fair enough, I was not aware of this.
sorry for the noise,
greg k-h
On Mon, Oct 28, 2013 at 01:12:35PM -0500, Josh Cartwright wrote:
> 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.
I haven't read this in depth, but... if you want to support runtime PM
for your spmi devices, then I suggest that you also include the fragments
to setup runtime PM in the bus-level probe handler and clean it up in
the bus-level remove handler.
What that means is doing what PCI, AMBA and similar buses do:
pm_runtime_get_noresume(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
ret = driver->probe(dev);
if (ret != 0) {
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
}
and:
pm_runtime_get_sync(dev);
ret = driver->remove(dev);
pm_runtime_put_noidle(dev);
pm_runtime_disable(dev);
pm_runtime_set_suspended(dev);
pm_runtime_put_noidle(dev);
What this means is that your devices get runtime enabled by default,
but they have to do a pm_runtime_put() or similar in their probe
function to benefit from it and a balancing pm_runtime_get() in
their remove method.
The set_active() call above may need to be conditional upon whether
the device really is in a powered up state at that point or not.
Others have made comments on various other issues so I won't repeat
those points here.
On 10/28, Josh Cartwright wrote:
> +
> +/**
> + * 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.
> + */
> +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);
> +}
> +
[...]
> +static int pmic_arb_read_cmd(struct spmi_controller *ctrl,
> + u8 opc, u8 sid, u16 addr, u8 bc, u8 *buf)
> +{
> + struct spmi_pmic_arb_dev *pmic_arb = spmi_controller_get_drvdata(ctrl);
> + unsigned long flags;
> + u32 cmd;
> + int rc;
> +
> + 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.
> + PMIC_ARB_MAX_TRANS_BYTES, bc+1);
Space around that '+' please.
> + 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);
> + 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.
> +
> + 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;
> +}
> +
[...]
> +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;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(pa->mapping_table); ++i)
> + pa->mapping_table[i] = readl_relaxed(
> + pa->cnfg + SPMI_MAPPING_TABLE_REG(i));
> +
> + platform_set_drvdata(pdev, ctrl);
> + spin_lock_init(&pa->lock);
> +
> + pa->channel = 0;
> + pa->max_apid = 0;
> + pa->min_apid = PMIC_ARB_MAX_PERIPHS - 1;
That looks backwards. Is this right?
> +
> + 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",
> + pmic_arb_base_read(pa, PMIC_ARB_VERSION));
> +
> + return 0;
> +
> +err_put_ctrl:
> + spmi_controller_put(ctrl);
> + return err;
> +}
> +
> +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
__exit shouldn't be here. We want this function in modules.
> +{
> + struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> + spmi_controller_remove(ctrl);
> + return 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.
> + .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()
?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On 10/28, Josh Cartwright wrote:
> @@ -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.
> + 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)
[...]
> +
> +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?
> + 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);
> +}
> +
[...]
> +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 */
> + 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.
> +
> + *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)
> {
> 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;
>
> + 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.
> + }
> +
> + 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;
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
Stephen-
Thanks for all the comments.
On Wed, Oct 30, 2013 at 11:17:55AM -0700, Stephen Boyd wrote:
> On 10/28, Josh Cartwright wrote:
> > @@ -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.
Indeed, will remove (I think this is a holdout from the split qpnpint
handling that exists in the vendor tree).
> > + 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)
> [...]
> > +
> > +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?
Indeed, I removed this for some reason awhile back, but this really
should be put back into place, and the EE should be a required property
in the bindings.
[..]
> > +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 */
> > + 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.
Indeed, I'll move it and add a comment at init-time.
> > +
> > + *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)
> > {
> > 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;
> >
> > + 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.
Nice catch, it wasn't my intent to ignore it.
Thanks again,
Josh
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Wed, Oct 30, 2013 at 11:05:36AM -0700, Stephen Boyd wrote:
> On 10/28, Josh Cartwright wrote:
> > +
> > +/**
> > + * 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.
I'll clean the documentation up a bit and push it all to the
implementation (as suggested in a previous review).
[..]
> > + 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.
>
> > + PMIC_ARB_MAX_TRANS_BYTES, bc+1);
>
> Space around that '+' please.
Sure.
> > + 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);
> > + 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.
Okay.
[..]
> > +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev)
>
> __exit shouldn't be here. We want this function in modules.
>
> > +{
> > + struct spmi_controller *ctrl = platform_get_drvdata(pdev);
> > + spmi_controller_remove(ctrl);
> > + return 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.
>
> > + .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()
Yep, will re-add. Not sure why I dropped these...
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
On Tue, Oct 29, 2013 at 09:52:15AM -0700, Stephen Boyd wrote:
> On 10/28/13 11:12, Josh Cartwright wrote:
> > 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.
> > + 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?
>
[..]
> > +static struct device_type spmi_ctrl_type = {
>
>
> const?
Yep. Thanks.
[..]
> > +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?
> > + return -EINVAL;
> > +
> > + return spmi_read_cmd(sdev->ctrl, SPMI_CMD_READ, sdev->usid, addr, 0,
> > + buf);
> > +}
> > +EXPORT_SYMBOL_GPL(spmi_register_read);
> > +
> [...]
> > +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.
> > +
> > + 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...
> > +
> > + 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.
> > + dev_err(&ctrl->dev,
> > + "invalid usid on node %s\n",
> > + node->full_name);
> > + continue;
> > + }
> > +
> [...]
> > 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.
[..]
> > + * spmi_controller_alloc: Allocate a new SPMI controller
>
> There should be a dash here instead of colon.
>
[..]
> > +static inline void spmi_controller_put(struct spmi_controller *ctrl)
> > +{
> > + if (ctrl)
> > + put_device(&ctrl->dev);
> > +}
>
> This function is missing documentation.
>
[..]
> > +/**
> > + * 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
On 10/30/13 12:37, Josh Cartwright wrote:
> On Tue, Oct 29, 2013 at 09:52:15AM -0700, Stephen Boyd wrote:
>> On 10/28/13 11:12, Josh Cartwright wrote:
>>> +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?
It triggered my 'magic number used twice' alarm. I'm ok with it either way.
>>> +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.
Yes that also works.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation