* [PATCH RFC 0/3] Add support for the System Power Management Interface (SPMI) @ 2013-08-15 19:50 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-15 19:50 UTC (permalink / raw) To: Grant Likely, Rob Herring, Greg Kroah-Hartman Cc: linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan 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]. Introduced in this patchset are the core SPMI bus components, as well an implementation of an SPMI controller, the "PMIC arbiter", which is used on several Qualcomm SoCs to communicate with off-chip Power Management ICs (PMICs). Notably missing from this patchset is an implementation of a spmi device/slave driver, which is still forthcoming. With this patchset in it's current state, it is possible to perform register accesses to a slave via debugfs. [1]: http://www.mipi.org/specifications/system-power-management-interface Josh Cartwright (1): spmi: document the PMIC arbiter SPMI bindings Kenneth Heitke (2): spmi: Linux driver framework for SPMI spmi: Add MSM PMIC Arbiter SPMI controller .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_spmi.c | 74 +++ drivers/spmi/Kconfig | 24 + drivers/spmi/Makefile | 9 + drivers/spmi/spmi-dbgfs.c | 591 +++++++++++++++++++++ drivers/spmi/spmi-dbgfs.h | 37 ++ drivers/spmi/spmi-pmic-arb.c | 499 +++++++++++++++++ drivers/spmi/spmi.c | 449 ++++++++++++++++ include/linux/mod_devicetable.h | 8 + include/linux/of_spmi.h | 37 ++ include/linux/spmi.h | 337 ++++++++++++ 15 files changed, 2101 insertions(+) create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt create mode 100644 drivers/of/of_spmi.c create mode 100644 drivers/spmi/Kconfig create mode 100644 drivers/spmi/Makefile create mode 100644 drivers/spmi/spmi-dbgfs.c create mode 100644 drivers/spmi/spmi-dbgfs.h create mode 100644 drivers/spmi/spmi-pmic-arb.c create mode 100644 drivers/spmi/spmi.c create mode 100644 include/linux/of_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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 0/3] Add support for the System Power Management Interface (SPMI) @ 2013-08-15 19:50 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-15 19:50 UTC (permalink / raw) To: linux-arm-kernel 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]. Introduced in this patchset are the core SPMI bus components, as well an implementation of an SPMI controller, the "PMIC arbiter", which is used on several Qualcomm SoCs to communicate with off-chip Power Management ICs (PMICs). Notably missing from this patchset is an implementation of a spmi device/slave driver, which is still forthcoming. With this patchset in it's current state, it is possible to perform register accesses to a slave via debugfs. [1]: http://www.mipi.org/specifications/system-power-management-interface Josh Cartwright (1): spmi: document the PMIC arbiter SPMI bindings Kenneth Heitke (2): spmi: Linux driver framework for SPMI spmi: Add MSM PMIC Arbiter SPMI controller .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 + drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_spmi.c | 74 +++ drivers/spmi/Kconfig | 24 + drivers/spmi/Makefile | 9 + drivers/spmi/spmi-dbgfs.c | 591 +++++++++++++++++++++ drivers/spmi/spmi-dbgfs.h | 37 ++ drivers/spmi/spmi-pmic-arb.c | 499 +++++++++++++++++ drivers/spmi/spmi.c | 449 ++++++++++++++++ include/linux/mod_devicetable.h | 8 + include/linux/of_spmi.h | 37 ++ include/linux/spmi.h | 337 ++++++++++++ 15 files changed, 2101 insertions(+) create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt create mode 100644 drivers/of/of_spmi.c create mode 100644 drivers/spmi/Kconfig create mode 100644 drivers/spmi/Makefile create mode 100644 drivers/spmi/spmi-dbgfs.c create mode 100644 drivers/spmi/spmi-dbgfs.h create mode 100644 drivers/spmi/spmi-pmic-arb.c create mode 100644 drivers/spmi/spmi.c create mode 100644 include/linux/of_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 ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 2/3] spmi: Add MSM PMIC Arbiter SPMI controller 2013-08-15 19:50 ` Josh Cartwright @ 2013-08-09 20:37 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-09 20:37 UTC (permalink / raw) To: Grant Likely, Rob Herring, Greg Kroah-Hartman Cc: linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan From: Kenneth Heitke <kheitke@codeaurora.org> 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 <kheitke@codeaurora.org> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- drivers/spmi/Kconfig | 15 ++ drivers/spmi/Makefile | 2 + drivers/spmi/spmi-pmic-arb.c | 499 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 516 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 ef96afd..f83ea87 100644 --- a/drivers/spmi/Makefile +++ b/drivers/spmi/Makefile @@ -5,3 +5,5 @@ obj-$(CONFIG_SPMI) += spmi-core.o spmi-core-y += spmi.o spmi-core-$(CONFIG_DEBUG_FS) += spmi-dbgfs.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..ebc0038 --- /dev/null +++ b/drivers/spmi/spmi-pmic-arb.c @@ -0,0 +1,499 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/spmi.h> +#include <linux/of.h> +#include <linux/interrupt.h> +#include <linux/of_spmi.h> +#include <linux/module.h> +#include <linux/seq_file.h> + +#include "spmi-dbgfs.h" + +#define SPMI_PMIC_ARB_NAME "spmi_pmic_arb" + +/* 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) + +/** + * base - base address of the PMIC Arbiter core registers. + * intr - base address of the SPMI interrupt control registers + */ +struct spmi_pmic_arb_dev { + struct spmi_controller controller; + struct device *dev; + struct device *slave; + void __iomem *base; + void __iomem *intr; + void __iomem *cnfg; + bool allow_wakeup; + spinlock_t lock; + u8 channel; + u8 min_apid; + u8 max_apid; + u16 periph_id_map[PMIC_ARB_MAX_PERIPHS]; + u32 mapping_table[SPMI_MAPPING_TABLE_LEN]; +}; + +static u32 pmic_arb_read(struct spmi_pmic_arb_dev *dev, u32 offset) +{ + u32 val = readl_relaxed(dev->base + offset); + pr_debug("address 0x%p, val 0x%x\n", dev->base + offset, val); + return val; +} + +static void pmic_arb_write(struct spmi_pmic_arb_dev *dev, u32 offset, u32 val) +{ + pr_debug("address 0x%p, val 0x%x\n", dev->base + offset, val); + writel_relaxed(val, dev->base + offset); +} + +static int pmic_arb_wait_for_done(struct spmi_pmic_arb_dev *dev) +{ + u32 status = 0; + u32 timeout = PMIC_ARB_TIMEOUT_US; + u32 offset = PMIC_ARB_STATUS(dev->channel); + + while (timeout--) { + status = pmic_arb_read(dev, offset); + + if (status & PMIC_ARB_STATUS_DONE) { + if (status & PMIC_ARB_STATUS_DENIED) { + dev_err(dev->dev, + "%s: transaction denied (0x%x)\n", + __func__, status); + return -EPERM; + } + + if (status & PMIC_ARB_STATUS_FAILURE) { + dev_err(dev->dev, + "%s: transaction failed (0x%x)\n", + __func__, status); + return -EIO; + } + + if (status & PMIC_ARB_STATUS_DROPPED) { + dev_err(dev->dev, + "%s: transaction dropped (0x%x)\n", + __func__, status); + return -EIO; + } + + return 0; + } + udelay(1); + } + + dev_err(dev->dev, "%s: timeout, status 0x%x\n", __func__, status); + return -ETIMEDOUT; +} + +/** + * 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_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, u8 *buf, u32 reg, u8 bc) +{ + u32 data = 0; + memcpy(&data, buf, (bc & 3) + 1); + pmic_arb_write(dev, reg, data); +} + +/* Non-data command */ +static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid) +{ + struct spmi_pmic_arb_dev *pmic_arb = spmi_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + 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_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) { + dev_err(pmic_arb->dev + , "pmic-arb supports 1..%d bytes per trans, but:%d requested" + , PMIC_ARB_MAX_TRANS_BYTES, bc+1); + return -EINVAL; + } + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + 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, u8 *buf) +{ + struct spmi_pmic_arb_dev *pmic_arb = spmi_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) { + dev_err(pmic_arb->dev + , "pmic-arb supports 1..%d bytes per trans, but:%d requested" + , PMIC_ARB_MAX_TRANS_BYTES, bc+1); + return -EINVAL; + } + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + spin_unlock_irqrestore(&pmic_arb->lock, flags); + + return rc; +} + +/* APID to PPID */ +static inline u16 get_peripheral_id(struct spmi_pmic_arb_dev *pmic_arb, u8 apid) +{ + return pmic_arb->periph_id_map[apid] & PMIC_ARB_PPID_MASK; +} + +/* APID to PPID, returns valid flag */ +static inline int is_apid_valid(struct spmi_pmic_arb_dev *pmic_arb, u8 apid) +{ + return pmic_arb->periph_id_map[apid] & PMIC_ARB_PERIPH_ID_VALID; +} + +static int pmic_arb_mapping_data_show(struct seq_file *file, void *unused) +{ + struct spmi_pmic_arb_dev *pmic_arb = file->private; + int first = pmic_arb->min_apid; + int last = pmic_arb->max_apid; + int i; + + for (i = first; i <= last; ++i) { + if (!is_apid_valid(pmic_arb, i)) + continue; + + seq_printf(file, "APID 0x%.2x = PPID 0x%.3x. Enabled:%d\n", + i, get_peripheral_id(pmic_arb, i), + readl_relaxed(pmic_arb->intr + SPMI_PIC_ACC_ENABLE(i))); + } + + return 0; +} + +static int pmic_arb_mapping_data_open(struct inode *inode, struct file *file) +{ + return single_open(file, pmic_arb_mapping_data_show, inode->i_private); +} + +static const struct file_operations pmic_arb_dfs_fops = { + .open = pmic_arb_mapping_data_open, + .read = seq_read, + .llseek = seq_lseek, + .release = seq_release, +}; + +static int +spmi_pmic_arb_get_property(struct platform_device *pdev, char *pname, u32 *prop) +{ + int ret = of_property_read_u32(pdev->dev.of_node, pname, prop); + + if (ret) + dev_err(&pdev->dev, "missing property: %s\n", pname); + else + pr_debug("%s = 0x%x\n", pname, *prop); + + return ret; +} + +static int spmi_pmic_arb_probe(struct platform_device *pdev) +{ + struct spmi_pmic_arb_dev *pmic_arb; + struct resource *mem_res; + int ret = 0; + u32 prop; + int i; + + pr_debug("SPMI PMIC Arbiter\n"); + + pmic_arb = devm_kzalloc(&pdev->dev, + sizeof(struct spmi_pmic_arb_dev), GFP_KERNEL); + if (!pmic_arb) { + dev_err(&pdev->dev, "can not allocate pmic_arb data\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); + if (!mem_res) { + dev_err(&pdev->dev, "missing base memory resource\n"); + return -ENODEV; + } + + pmic_arb->base = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->base) { + dev_err(&pdev->dev, "ioremap of 'base' failed\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr"); + if (!mem_res) { + dev_err(&pdev->dev, "missing mem resource (interrupts)\n"); + return -ENODEV; + } + + pmic_arb->intr = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->intr) { + dev_err(&pdev->dev, "ioremap of 'intr' failed\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg"); + if (!mem_res) { + dev_err(&pdev->dev, "missing mem resource (configuration)\n"); + return -ENODEV; + } + + pmic_arb->cnfg = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->cnfg) { + dev_err(&pdev->dev, "ioremap of 'cnfg' failed\n"); + return -ENOMEM; + } + + for (i = 0; i < ARRAY_SIZE(pmic_arb->mapping_table); ++i) + pmic_arb->mapping_table[i] = readl_relaxed( + pmic_arb->cnfg + SPMI_MAPPING_TABLE_REG(i)); + + ret = spmi_pmic_arb_get_property(pdev, "qcom,pmic-arb-channel", &prop); + if (ret) + return -ENODEV; + pmic_arb->channel = (u8)prop; + + pmic_arb->max_apid = 0; + pmic_arb->min_apid = PMIC_ARB_MAX_PERIPHS - 1; + + pmic_arb->dev = &pdev->dev; + platform_set_drvdata(pdev, pmic_arb); + spmi_set_ctrldata(&pmic_arb->controller, pmic_arb); + + spin_lock_init(&pmic_arb->lock); + + pmic_arb->controller.dev.parent = pdev->dev.parent; + pmic_arb->controller.dev.of_node = of_node_get(pdev->dev.of_node); + + /* Callbacks */ + pmic_arb->controller.cmd = pmic_arb_cmd; + pmic_arb->controller.read_cmd = pmic_arb_read_cmd; + pmic_arb->controller.write_cmd = pmic_arb_write_cmd; + + ret = spmi_add_controller(&pmic_arb->controller); + if (ret) + goto err_add_controller; + + /* Register device(s) from the device tree */ + of_spmi_register_devices(&pmic_arb->controller); + + pr_debug("PMIC Arb Version 0x%x\n", + pmic_arb_read(pmic_arb, PMIC_ARB_VERSION)); + + return 0; + +err_add_controller: + platform_set_drvdata(pdev, NULL); + return ret; +} + +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev) +{ + struct spmi_pmic_arb_dev *pmic_arb = platform_get_drvdata(pdev); + spmi_del_controller(&pmic_arb->controller); + 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_NAME, + .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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH RFC 2/3] spmi: Add MSM PMIC Arbiter SPMI controller @ 2013-08-09 20:37 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-09 20:37 UTC (permalink / raw) To: linux-arm-kernel From: Kenneth Heitke <kheitke@codeaurora.org> 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 <kheitke@codeaurora.org> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- drivers/spmi/Kconfig | 15 ++ drivers/spmi/Makefile | 2 + drivers/spmi/spmi-pmic-arb.c | 499 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 516 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 ef96afd..f83ea87 100644 --- a/drivers/spmi/Makefile +++ b/drivers/spmi/Makefile @@ -5,3 +5,5 @@ obj-$(CONFIG_SPMI) += spmi-core.o spmi-core-y += spmi.o spmi-core-$(CONFIG_DEBUG_FS) += spmi-dbgfs.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..ebc0038 --- /dev/null +++ b/drivers/spmi/spmi-pmic-arb.c @@ -0,0 +1,499 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/spmi.h> +#include <linux/of.h> +#include <linux/interrupt.h> +#include <linux/of_spmi.h> +#include <linux/module.h> +#include <linux/seq_file.h> + +#include "spmi-dbgfs.h" + +#define SPMI_PMIC_ARB_NAME "spmi_pmic_arb" + +/* 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) + +/** + * base - base address of the PMIC Arbiter core registers. + * intr - base address of the SPMI interrupt control registers + */ +struct spmi_pmic_arb_dev { + struct spmi_controller controller; + struct device *dev; + struct device *slave; + void __iomem *base; + void __iomem *intr; + void __iomem *cnfg; + bool allow_wakeup; + spinlock_t lock; + u8 channel; + u8 min_apid; + u8 max_apid; + u16 periph_id_map[PMIC_ARB_MAX_PERIPHS]; + u32 mapping_table[SPMI_MAPPING_TABLE_LEN]; +}; + +static u32 pmic_arb_read(struct spmi_pmic_arb_dev *dev, u32 offset) +{ + u32 val = readl_relaxed(dev->base + offset); + pr_debug("address 0x%p, val 0x%x\n", dev->base + offset, val); + return val; +} + +static void pmic_arb_write(struct spmi_pmic_arb_dev *dev, u32 offset, u32 val) +{ + pr_debug("address 0x%p, val 0x%x\n", dev->base + offset, val); + writel_relaxed(val, dev->base + offset); +} + +static int pmic_arb_wait_for_done(struct spmi_pmic_arb_dev *dev) +{ + u32 status = 0; + u32 timeout = PMIC_ARB_TIMEOUT_US; + u32 offset = PMIC_ARB_STATUS(dev->channel); + + while (timeout--) { + status = pmic_arb_read(dev, offset); + + if (status & PMIC_ARB_STATUS_DONE) { + if (status & PMIC_ARB_STATUS_DENIED) { + dev_err(dev->dev, + "%s: transaction denied (0x%x)\n", + __func__, status); + return -EPERM; + } + + if (status & PMIC_ARB_STATUS_FAILURE) { + dev_err(dev->dev, + "%s: transaction failed (0x%x)\n", + __func__, status); + return -EIO; + } + + if (status & PMIC_ARB_STATUS_DROPPED) { + dev_err(dev->dev, + "%s: transaction dropped (0x%x)\n", + __func__, status); + return -EIO; + } + + return 0; + } + udelay(1); + } + + dev_err(dev->dev, "%s: timeout, status 0x%x\n", __func__, status); + return -ETIMEDOUT; +} + +/** + * 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_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, u8 *buf, u32 reg, u8 bc) +{ + u32 data = 0; + memcpy(&data, buf, (bc & 3) + 1); + pmic_arb_write(dev, reg, data); +} + +/* Non-data command */ +static int pmic_arb_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid) +{ + struct spmi_pmic_arb_dev *pmic_arb = spmi_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + 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_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) { + dev_err(pmic_arb->dev + , "pmic-arb supports 1..%d bytes per trans, but:%d requested" + , PMIC_ARB_MAX_TRANS_BYTES, bc+1); + return -EINVAL; + } + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + 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, u8 *buf) +{ + struct spmi_pmic_arb_dev *pmic_arb = spmi_get_ctrldata(ctrl); + unsigned long flags; + u32 cmd; + int rc; + + if (bc >= PMIC_ARB_MAX_TRANS_BYTES) { + dev_err(pmic_arb->dev + , "pmic-arb supports 1..%d bytes per trans, but:%d requested" + , PMIC_ARB_MAX_TRANS_BYTES, bc+1); + return -EINVAL; + } + pr_debug("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_write(pmic_arb, PMIC_ARB_CMD(pmic_arb->channel), cmd); + rc = pmic_arb_wait_for_done(pmic_arb); + spin_unlock_irqrestore(&pmic_arb->lock, flags); + + return rc; +} + +/* APID to PPID */ +static inline u16 get_peripheral_id(struct spmi_pmic_arb_dev *pmic_arb, u8 apid) +{ + return pmic_arb->periph_id_map[apid] & PMIC_ARB_PPID_MASK; +} + +/* APID to PPID, returns valid flag */ +static inline int is_apid_valid(struct spmi_pmic_arb_dev *pmic_arb, u8 apid) +{ + return pmic_arb->periph_id_map[apid] & PMIC_ARB_PERIPH_ID_VALID; +} + +static int pmic_arb_mapping_data_show(struct seq_file *file, void *unused) +{ + struct spmi_pmic_arb_dev *pmic_arb = file->private; + int first = pmic_arb->min_apid; + int last = pmic_arb->max_apid; + int i; + + for (i = first; i <= last; ++i) { + if (!is_apid_valid(pmic_arb, i)) + continue; + + seq_printf(file, "APID 0x%.2x = PPID 0x%.3x. Enabled:%d\n", + i, get_peripheral_id(pmic_arb, i), + readl_relaxed(pmic_arb->intr + SPMI_PIC_ACC_ENABLE(i))); + } + + return 0; +} + +static int pmic_arb_mapping_data_open(struct inode *inode, struct file *file) +{ + return single_open(file, pmic_arb_mapping_data_show, inode->i_private); +} + +static const struct file_operations pmic_arb_dfs_fops = { + .open = pmic_arb_mapping_data_open, + .read = seq_read, + .llseek = seq_lseek, + .release = seq_release, +}; + +static int +spmi_pmic_arb_get_property(struct platform_device *pdev, char *pname, u32 *prop) +{ + int ret = of_property_read_u32(pdev->dev.of_node, pname, prop); + + if (ret) + dev_err(&pdev->dev, "missing property: %s\n", pname); + else + pr_debug("%s = 0x%x\n", pname, *prop); + + return ret; +} + +static int spmi_pmic_arb_probe(struct platform_device *pdev) +{ + struct spmi_pmic_arb_dev *pmic_arb; + struct resource *mem_res; + int ret = 0; + u32 prop; + int i; + + pr_debug("SPMI PMIC Arbiter\n"); + + pmic_arb = devm_kzalloc(&pdev->dev, + sizeof(struct spmi_pmic_arb_dev), GFP_KERNEL); + if (!pmic_arb) { + dev_err(&pdev->dev, "can not allocate pmic_arb data\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); + if (!mem_res) { + dev_err(&pdev->dev, "missing base memory resource\n"); + return -ENODEV; + } + + pmic_arb->base = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->base) { + dev_err(&pdev->dev, "ioremap of 'base' failed\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr"); + if (!mem_res) { + dev_err(&pdev->dev, "missing mem resource (interrupts)\n"); + return -ENODEV; + } + + pmic_arb->intr = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->intr) { + dev_err(&pdev->dev, "ioremap of 'intr' failed\n"); + return -ENOMEM; + } + + mem_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg"); + if (!mem_res) { + dev_err(&pdev->dev, "missing mem resource (configuration)\n"); + return -ENODEV; + } + + pmic_arb->cnfg = devm_ioremap(&pdev->dev, + mem_res->start, resource_size(mem_res)); + if (!pmic_arb->cnfg) { + dev_err(&pdev->dev, "ioremap of 'cnfg' failed\n"); + return -ENOMEM; + } + + for (i = 0; i < ARRAY_SIZE(pmic_arb->mapping_table); ++i) + pmic_arb->mapping_table[i] = readl_relaxed( + pmic_arb->cnfg + SPMI_MAPPING_TABLE_REG(i)); + + ret = spmi_pmic_arb_get_property(pdev, "qcom,pmic-arb-channel", &prop); + if (ret) + return -ENODEV; + pmic_arb->channel = (u8)prop; + + pmic_arb->max_apid = 0; + pmic_arb->min_apid = PMIC_ARB_MAX_PERIPHS - 1; + + pmic_arb->dev = &pdev->dev; + platform_set_drvdata(pdev, pmic_arb); + spmi_set_ctrldata(&pmic_arb->controller, pmic_arb); + + spin_lock_init(&pmic_arb->lock); + + pmic_arb->controller.dev.parent = pdev->dev.parent; + pmic_arb->controller.dev.of_node = of_node_get(pdev->dev.of_node); + + /* Callbacks */ + pmic_arb->controller.cmd = pmic_arb_cmd; + pmic_arb->controller.read_cmd = pmic_arb_read_cmd; + pmic_arb->controller.write_cmd = pmic_arb_write_cmd; + + ret = spmi_add_controller(&pmic_arb->controller); + if (ret) + goto err_add_controller; + + /* Register device(s) from the device tree */ + of_spmi_register_devices(&pmic_arb->controller); + + pr_debug("PMIC Arb Version 0x%x\n", + pmic_arb_read(pmic_arb, PMIC_ARB_VERSION)); + + return 0; + +err_add_controller: + platform_set_drvdata(pdev, NULL); + return ret; +} + +static int __exit spmi_pmic_arb_remove(struct platform_device *pdev) +{ + struct spmi_pmic_arb_dev *pmic_arb = platform_get_drvdata(pdev); + spmi_del_controller(&pmic_arb->controller); + 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_NAME, + .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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 2/3] spmi: Add MSM PMIC Arbiter SPMI controller 2013-08-09 20:37 ` Josh Cartwright @ 2013-08-16 18:52 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:52 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 09, 2013 at 01:37:07PM -0700, Josh Cartwright wrote: > +struct spmi_pmic_arb_dev { > + struct spmi_controller controller; > + struct device *dev; > + struct device *slave; This is problematic. Why do you have the driver "own" the controller? What is dev for, there's already a struct device within the controller. Same for slave, what is it? You have 3 struct devices here, which one controls the lifecycle of the object (hint, I know the answer, but I think it's wrong...) thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 2/3] spmi: Add MSM PMIC Arbiter SPMI controller @ 2013-08-16 18:52 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:52 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 09, 2013 at 01:37:07PM -0700, Josh Cartwright wrote: > +struct spmi_pmic_arb_dev { > + struct spmi_controller controller; > + struct device *dev; > + struct device *slave; This is problematic. Why do you have the driver "own" the controller? What is dev for, there's already a struct device within the controller. Same for slave, what is it? You have 3 struct devices here, which one controls the lifecycle of the object (hint, I know the answer, but I think it's wrong...) thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-15 19:50 ` Josh Cartwright @ 2013-08-09 20:37 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-09 20:37 UTC (permalink / raw) To: Grant Likely, Rob Herring, Greg Kroah-Hartman Cc: linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan From: Kenneth Heitke <kheitke@codeaurora.org> 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 <kheitke@codeaurora.org> Signed-off-by: Michael Bohan <mbohan@codeaurora.org> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_spmi.c | 74 +++++ drivers/spmi/Kconfig | 9 + drivers/spmi/Makefile | 7 + drivers/spmi/spmi-dbgfs.c | 591 ++++++++++++++++++++++++++++++++++++++++ drivers/spmi/spmi-dbgfs.h | 37 +++ drivers/spmi/spmi.c | 449 ++++++++++++++++++++++++++++++ include/linux/mod_devicetable.h | 8 + include/linux/of_spmi.h | 37 +++ include/linux/spmi.h | 337 +++++++++++++++++++++++ 13 files changed, 1559 insertions(+) create mode 100644 drivers/of/of_spmi.c create mode 100644 drivers/spmi/Kconfig create mode 100644 drivers/spmi/Makefile create mode 100644 drivers/spmi/spmi-dbgfs.c create mode 100644 drivers/spmi/spmi-dbgfs.h create mode 100644 drivers/spmi/spmi.c create mode 100644 include/linux/of_spmi.h create mode 100644 include/linux/spmi.h 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/of/Kconfig b/drivers/of/Kconfig index 80e5c13..916b0c1 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -76,6 +76,12 @@ config OF_PCI_IRQ help OpenFirmware PCI IRQ routing helpers +config OF_SPMI + def_tristate SPMI + depends on SPMI + help + OpenFirmware SPMI bus accessors + config OF_MTD depends on MTD def_bool y diff --git a/drivers/of/Makefile b/drivers/of/Makefile index 1f9c0c4..5486614 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -9,4 +9,5 @@ obj-$(CONFIG_OF_SELFTEST) += selftest.o obj-$(CONFIG_OF_MDIO) += of_mdio.o obj-$(CONFIG_OF_PCI) += of_pci.o obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o +obj-$(CONFIG_OF_SPMI) += of_spmi.o obj-$(CONFIG_OF_MTD) += of_mtd.o diff --git a/drivers/of/of_spmi.c b/drivers/of/of_spmi.c new file mode 100644 index 0000000..fca19c5 --- /dev/null +++ b/drivers/of/of_spmi.c @@ -0,0 +1,74 @@ +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/spmi.h> +#include <linux/of_spmi.h> + +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 usid; + + dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name); + + err = of_property_read_u32(node, "reg", &usid); + if (err) { + dev_err(&ctrl->dev, + "node %s does not have 'reg' property\n", + node->full_name); + continue; + } + + if (usid > 0xFF) { + dev_err(&ctrl->dev, + "invalid usid on node %s\n", + node->full_name); + continue; + } + + dev_dbg(&ctrl->dev, "read usid %02x\n", usid); + + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); + if (!sdev) + continue; + + sdev->dev.of_node = node; + sdev->usid = (u8) usid; + spmi_device_initialize(sdev); + err = spmi_controller_add_device(ctrl, sdev); + if (err) { + dev_err(&sdev->dev, + "failure adding device. status %d\n", err); + continue; + } + } + + return 0; +} +EXPORT_SYMBOL(of_spmi_register_devices); + +MODULE_LICENSE("GPL v2"); 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..ef96afd --- /dev/null +++ b/drivers/spmi/Makefile @@ -0,0 +1,7 @@ +# +# Makefile for kernel SPMI framework. +# +obj-$(CONFIG_SPMI) += spmi-core.o + +spmi-core-y += spmi.o +spmi-core-$(CONFIG_DEBUG_FS) += spmi-dbgfs.o diff --git a/drivers/spmi/spmi-dbgfs.c b/drivers/spmi/spmi-dbgfs.c new file mode 100644 index 0000000..5ca0a44 --- /dev/null +++ b/drivers/spmi/spmi-dbgfs.c @@ -0,0 +1,591 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/sizes.h> +#include <linux/slab.h> +#include <linux/uaccess.h> +#include <linux/spinlock.h> +#include <linux/string.h> +#include <linux/debugfs.h> +#include <linux/spmi.h> +#include <linux/ctype.h> +#include "spmi-dbgfs.h" + +#define ADDR_LEN 6 /* 5 byte address + 1 space character */ +#define CHARS_PER_ITEM 3 /* Format is 'XX ' */ +#define ITEMS_PER_LINE 16 /* 16 data items per line */ +#define MAX_LINE_LENGTH (ADDR_LEN + (ITEMS_PER_LINE * CHARS_PER_ITEM) + 1) + +#define MAX_REG_PER_TRANSACTION (8) + +static const mode_t DFS_MODE = S_IRUSR | S_IWUSR; + +/* Log buffer */ +struct spmi_log_buffer { + u32 rpos; /* Current 'read' position in buffer */ + u32 wpos; /* Current 'write' position in buffer */ + u32 len; /* Length of the buffer */ + char data[0]; /* Log buffer */ +}; + +/* SPMI transaction parameters */ +struct spmi_trans { + u32 cnt; /* Number of bytes to read */ + u32 addr; /* 20-bit address: SID + PID + Register offset */ + u32 offset; /* Offset of last read data */ + bool raw_data; /* Set to true for raw data dump */ + struct spmi_device *sdev; + struct spmi_log_buffer *log; /* log buffer */ +}; + +static char dbgfs_help[] = + "SPMI Debug-FS support\n" + "\n" + "Hierarchy schema:\n" + "/sys/kernel/debug/spmi\n" + " /help -- Static help text\n" + " /spmi-0 -- Directory for SPMI bus 0\n" + " /spmi-0/0-1 -- Directory for SPMI device '0-1'\n" + " /spmi-0/0-1/address -- Starting register for reads or writes\n" + " /spmi-0/0-1/count -- Number of registers to read (only used for reads)\n" + " /spmi-0/0-1/data -- Initiates the SPMI read (formatted output)\n" + " /spmi-0/0-1/data_raw -- Initiates the SPMI raw read or write\n" + " /spmi-n -- Directory for SPMI bus n\n" + "\n" + "To perform SPMI read or write transactions, you need to first write the\n" + "address of the slave device register to the 'address' file. For read\n" + "transactions, the number of bytes to be read needs to be written to the\n" + "'count' file.\n" + "\n" + "The 'address' file specifies the 20-bit address of a slave device register.\n" + "The upper 4 bits 'address[19..16]' specify the slave identifier (SID) for\n" + "the slave device. The lower 16 bits specify the slave register address.\n" + "\n" + "Reading from the 'data' file will initiate a SPMI read transaction starting\n" + "from slave register 'address' for 'count' number of bytes.\n" + "\n" + "Writing to the 'data' file will initiate a SPMI write transaction starting\n" + "from slave register 'address'. The number of registers written to will\n" + "match the number of bytes written to the 'data' file.\n" + "\n" + "Example: Read 4 bytes starting at register address 0x1234 for SID 2\n" + "\n" + "echo 0x21234 > address\n" + "echo 4 > count\n" + "cat data\n" + "\n" + "Example: Write 3 bytes starting at register address 0x1008 for SID 1\n" + "\n" + "echo 0x11008 > address\n" + "echo 0x01 0x02 0x03 > data\n" + "\n" + "Note that the count file is not used for writes. Since 3 bytes are\n" + "written to the 'data' file, then 3 bytes will be written across the\n" + "SPMI bus.\n\n"; + +static struct debugfs_blob_wrapper spmi_debug_help = { + .data = dbgfs_help, + .size = sizeof(dbgfs_help), +}; + +static struct dentry *spmi_debug_root; + +static int spmi_device_dfs_open(struct spmi_device *sdev, struct file *file) +{ + struct spmi_log_buffer *log; + struct spmi_trans *trans; + + size_t logbufsize = SZ_4K; + + /* Per file "transaction" data */ + trans = kzalloc(sizeof(*trans), GFP_KERNEL); + if (!trans) + return -ENOMEM; + + log = kzalloc(logbufsize, GFP_KERNEL); + if (!log) { + kfree(trans); + return -ENOMEM; + } + + log->rpos = 0; + log->wpos = 0; + log->len = logbufsize - sizeof(*log); + + trans->log = log; + trans->cnt = sdev->dfs_cnt; + trans->addr = sdev->dfs_addr; + trans->sdev = sdev; + trans->offset = trans->addr; + + file->private_data = trans; + return 0; +} + +static int spmi_device_dfs_data_open(struct inode *inode, struct file *file) +{ + struct spmi_device *sdev = inode->i_private; + return spmi_device_dfs_open(sdev, file); +} + +static int spmi_device_dfs_raw_data_open(struct inode *inode, struct file *file) +{ + struct spmi_device *sdev = inode->i_private; + struct spmi_trans *trans; + int rc; + + rc = spmi_device_dfs_open(sdev, file); + trans = file->private_data; + trans->raw_data = true; + return rc; +} + +static int spmi_device_dfs_close(struct inode *inode, struct file *file) +{ + struct spmi_trans *trans = file->private_data; + kfree(trans->log); + kfree(trans); + return 0; +} + +/** + * spmi_read_data: reads data across the SPMI bus + * @ctrl: The SPMI controller + * @buf: buffer to store the data read. + * @offset: SPMI address offset to start reading from. + * @cnt: The number of bytes to read. + * + * Returns 0 on success, otherwise returns error code from SPMI driver. + */ +static int +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) +{ + int ret = 0; + int len; + uint16_t addr; + + while (cnt > 0) { + addr = offset & 0xFFFF; + len = min(cnt, MAX_REG_PER_TRANSACTION); + + ret = spmi_ext_register_readl(sdev, addr, buf, len); + if (ret < 0) { + pr_err("SPMI read failed, err = %d\n", ret); + goto done; + } + + cnt -= len; + buf += len; + offset += len; + } + +done: + return ret; +} + +/** + * spmi_write_data: writes data across the SPMI bus + * @ctrl: The SPMI controller + * @buf: data to be written. + * @offset: SPMI address offset to start writing to. + * @cnt: The number of bytes to write. + * + * Returns 0 on success, otherwise returns error code from SPMI driver. + */ +static int +spmi_write_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) +{ + int ret = 0; + int len; + uint16_t addr; + + while (cnt > 0) { + addr = offset & 0xFFFF; + len = min(cnt, MAX_REG_PER_TRANSACTION); + + ret = spmi_ext_register_writel(sdev, addr, buf, len); + if (ret < 0) { + pr_err("SPMI write failed, err = %d\n", ret); + goto done; + } + + cnt -= len; + buf += len; + offset += len; + } + +done: + return ret; +} + +/** + * print_to_log: format a string and place into the log buffer + * @log: The log buffer to place the result into. + * @fmt: The format string to use. + * @...: The arguments for the format string. + * + * The return value is the number of characters written to @log buffer + * not including the trailing '\0'. + */ +static int print_to_log(struct spmi_log_buffer *log, const char *fmt, ...) +{ + va_list args; + int cnt; + char *buf = &log->data[log->wpos]; + size_t size = log->len - log->wpos; + + va_start(args, fmt); + cnt = vscnprintf(buf, size, fmt, args); + va_end(args); + + log->wpos += cnt; + return cnt; +} + +/** + * write_next_line_to_log: Writes a single "line" of data into the log buffer + * @trans: Pointer to SPMI transaction data. + * @offset: SPMI address offset to start reading from. + * @pcnt: Pointer to 'cnt' variable. Indicates the number of bytes to read. + * + * The 'offset' is a 20-bits SPMI address which includes a 4-bit slave id (SID), + * an 8-bit peripheral id (PID), and an 8-bit peripheral register address. + * + * On a successful read, the pcnt is decremented by the number of data + * bytes read across the SPMI bus. When the cnt reaches 0, all requested + * bytes have been read. + */ +static int +write_next_line_to_log(struct spmi_trans *trans, int offset, size_t *pcnt) +{ + int i, j; + u8 data[ITEMS_PER_LINE]; + struct spmi_log_buffer *log = trans->log; + + int cnt = 0; + int padding = offset % ITEMS_PER_LINE; + int items_to_read = min(ARRAY_SIZE(data) - padding, *pcnt); + int items_to_log = min(ITEMS_PER_LINE, padding + items_to_read); + + /* Buffer needs enough space for an entire line */ + if ((log->len - log->wpos) < MAX_LINE_LENGTH) + goto done; + + /* Read the desired number of "items" */ + if (spmi_read_data(trans->sdev, data, offset, items_to_read)) + goto done; + + *pcnt -= items_to_read; + + /* Each line starts with the aligned offset (20-bit address) */ + cnt = print_to_log(log, "%5.5X ", offset & 0xffff0); + if (cnt == 0) + goto done; + + /* If the offset is unaligned, add padding to right justify items */ + for (i = 0; i < padding; ++i) { + cnt = print_to_log(log, "-- "); + if (cnt == 0) + goto done; + } + + /* Log the data items */ + for (j = 0; i < items_to_log; ++i, ++j) { + cnt = print_to_log(log, "%2.2X ", data[j]); + if (cnt == 0) + goto done; + } + + /* If the last character was a space, then replace it with a newline */ + if (log->wpos > 0 && log->data[log->wpos - 1] == ' ') + log->data[log->wpos - 1] = '\n'; + +done: + return cnt; +} + +/** + * write_raw_data_to_log: Writes a single "line" of data into the log buffer + * @trans: Pointer to SPMI transaction data. + * @offset: SPMI address offset to start reading from. + * @pcnt: Pointer to 'cnt' variable. Indicates the number of bytes to read. + * + * The 'offset' is a 20-bits SPMI address which includes a 4-bit slave id (SID), + * an 8-bit peripheral id (PID), and an 8-bit peripheral register address. + * + * On a successful read, the pcnt is decremented by the number of data + * bytes read across the SPMI bus. When the cnt reaches 0, all requested + * bytes have been read. + */ +static int +write_raw_data_to_log(struct spmi_trans *trans, int offset, size_t *pcnt) +{ + u8 data[16]; + struct spmi_log_buffer *log = trans->log; + + int i; + int cnt = 0; + int items_to_read = min(ARRAY_SIZE(data), *pcnt); + + /* Buffer needs enough space for an entire line */ + if ((log->len - log->wpos) < 80) + goto done; + + /* Read the desired number of "items" */ + if (spmi_read_data(trans->sdev, data, offset, items_to_read)) + goto done; + + *pcnt -= items_to_read; + + /* Log the data items */ + for (i = 0; i < items_to_read; ++i) { + cnt = print_to_log(log, "0x%2.2X ", data[i]); + if (cnt == 0) + goto done; + } + + /* If the last character was a space, then replace it with a newline */ + if (log->wpos > 0 && log->data[log->wpos - 1] == ' ') + log->data[log->wpos - 1] = '\n'; + +done: + return cnt; +} + +/** + * get_log_data - reads data across the SPMI bus and saves to the log buffer + * @trans: Pointer to SPMI transaction data. + * + * Returns the number of "items" read or SPMI error code for read failures. + */ +static int get_log_data(struct spmi_trans *trans) +{ + int cnt; + int last_cnt; + int items_read; + int total_items_read = 0; + u32 offset = trans->offset; + size_t item_cnt = trans->cnt; + struct spmi_log_buffer *log = trans->log; + int (*write_to_log)(struct spmi_trans *, int, size_t *); + + if (item_cnt == 0) + return 0; + + if (trans->raw_data) + write_to_log = write_raw_data_to_log; + else + write_to_log = write_next_line_to_log; + + /* Reset the log buffer 'pointers' */ + log->wpos = log->rpos = 0; + + /* Keep reading data until the log is full */ + do { + last_cnt = item_cnt; + cnt = write_to_log(trans, offset, &item_cnt); + items_read = last_cnt - item_cnt; + offset += items_read; + total_items_read += items_read; + } while (cnt && item_cnt > 0); + + /* Adjust the transaction offset and count */ + trans->cnt = item_cnt; + trans->offset += total_items_read; + + return total_items_read; +} + +static ssize_t spmi_device_dfs_reg_write(struct file *file, + const char __user *buf, + size_t count, loff_t *ppos) +{ + int bytes_read; + int data; + int pos = 0; + int cnt = 0; + u8 *values; + size_t ret = 0; + + struct spmi_trans *trans = file->private_data; + u32 offset = trans->offset; + + /* Make a copy of the user data */ + char *kbuf = kmalloc(count + 1, GFP_KERNEL); + if (!kbuf) + return -ENOMEM; + + ret = copy_from_user(kbuf, buf, count); + if (ret == count) { + pr_err("failed to copy data from user\n"); + ret = -EFAULT; + goto free_buf; + } + + count -= ret; + *ppos += count; + kbuf[count] = '\0'; + + /* Override the text buffer with the raw data */ + values = kbuf; + + /* Parse the data in the buffer. It should be a string of numbers */ + while (sscanf(kbuf + pos, "%i%n", &data, &bytes_read) == 1) { + pos += bytes_read; + values[cnt++] = data & 0xff; + } + + if (!cnt) + goto free_buf; + + /* Perform the SPMI write(s) */ + ret = spmi_write_data(trans->sdev, values, offset, cnt); + + if (ret) { + pr_err("SPMI write failed, err = %zu\n", ret); + } else { + ret = count; + trans->offset += cnt; + } + +free_buf: + kfree(kbuf); + return ret; +} + +static ssize_t spmi_device_dfs_reg_read(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct spmi_trans *trans = file->private_data; + struct spmi_log_buffer *log = trans->log; + size_t ret; + size_t len; + + /* Is the the log buffer empty */ + if (log->rpos >= log->wpos) { + if (get_log_data(trans) <= 0) + return 0; + } + + len = min(count, (size_t) log->wpos - log->rpos); + + ret = copy_to_user(buf, &log->data[log->rpos], len); + if (ret == len) { + pr_err("error copy SPMI register values to user\n"); + return -EFAULT; + } + + /* 'ret' is the number of bytes not copied */ + len -= ret; + + *ppos += len; + log->rpos += len; + return len; +} + +static const struct file_operations spmi_dfs_reg_fops = { + .open = spmi_device_dfs_data_open, + .release = spmi_device_dfs_close, + .read = spmi_device_dfs_reg_read, + .write = spmi_device_dfs_reg_write, +}; + +static const struct file_operations spmi_dfs_raw_data_fops = { + .open = spmi_device_dfs_raw_data_open, + .release = spmi_device_dfs_close, + .read = spmi_device_dfs_reg_read, + .write = spmi_device_dfs_reg_write, +}; + +void spmi_dfs_add_controller(struct spmi_controller *ctrl) +{ + ctrl->dfs_dir = debugfs_create_dir(dev_name(&ctrl->dev), + spmi_debug_root); + WARN_ON(!ctrl->dfs_dir); + + dev_dbg(&ctrl->dev, "adding debug entries for spmi controller\n"); +} + +void spmi_dfs_del_controller(struct spmi_controller *ctrl) +{ + debugfs_remove_recursive(ctrl->dfs_dir); +} + +void spmi_dfs_add_device(struct spmi_device *sdev) +{ + struct dentry *file; + + dev_dbg(&sdev->dev, "adding debugfs entries for spmi device\n"); + + sdev->dfs_dir = debugfs_create_dir(dev_name(&sdev->dev), + sdev->ctrl->dfs_dir); + if (WARN_ON(!sdev->dfs_dir)) + return; + + sdev->dfs_cnt = 1; + + file = debugfs_create_u32("count", DFS_MODE, sdev->dfs_dir, + &sdev->dfs_cnt); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_x32("address", DFS_MODE, sdev->dfs_dir, + &sdev->dfs_addr); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_file("data", DFS_MODE, sdev->dfs_dir, sdev, + &spmi_dfs_reg_fops); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_file("data_raw", DFS_MODE, sdev->dfs_dir, + sdev, &spmi_dfs_raw_data_fops); + if (WARN_ON(!file)) + goto err_remove_fs; + + return; + +err_remove_fs: + debugfs_remove_recursive(sdev->dfs_dir); +} + +void spmi_dfs_del_device(struct spmi_device *sdev) +{ + dev_dbg(&sdev->dev, "Deleting device\n"); + debugfs_remove_recursive(sdev->dfs_dir); +} + +void __exit spmi_dfs_exit(void) +{ + pr_debug("de-initializing spmi debugfs ..."); + debugfs_remove_recursive(spmi_debug_root); +} + +void __init spmi_dfs_init(void) +{ + struct dentry *help; + + pr_debug("creating SPMI debugfs file-system\n"); + + spmi_debug_root = debugfs_create_dir("spmi", NULL); + + help = debugfs_create_blob("help", S_IRUGO, spmi_debug_root, + &spmi_debug_help); + + WARN_ON(!spmi_debug_root || !help); +} + +MODULE_LICENSE("GPL v2"); diff --git a/drivers/spmi/spmi-dbgfs.h b/drivers/spmi/spmi-dbgfs.h new file mode 100644 index 0000000..896bb5a --- /dev/null +++ b/drivers/spmi/spmi-dbgfs.h @@ -0,0 +1,37 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef _SPMI_DBGFS_H +#define _SPMI_DBGFS_H + +#include <linux/spmi.h> +#include <linux/debugfs.h> + +#ifdef CONFIG_DEBUG_FS + +extern void __init spmi_dfs_init(void); +extern void __exit spmi_dfs_exit(void); +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); +extern void spmi_dfs_add_device(struct spmi_device *sdev); +extern void spmi_dfs_del_device(struct spmi_device *sdev); + +#else + +static inline void __init spmi_dfs_init(void) { } +static inline void __exit spmi_dfs_exit(void) { } +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } +#endif + +#endif /* _SPMI_DBGFS_H */ diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c new file mode 100644 index 0000000..3bbcc63 --- /dev/null +++ b/drivers/spmi/spmi.c @@ -0,0 +1,449 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/idr.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/spmi.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> + +#include "spmi-dbgfs.h" + +static DEFINE_MUTEX(board_lock); +static DEFINE_IDR(ctrl_idr); + +static void spmi_dev_release(struct device *dev) +{ + struct spmi_device *spmidev = to_spmi_device(dev); + kfree(spmidev); +} + +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); + complete(&ctrl->dev_released); +} + +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; +} + +/* 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); + struct spmi_controller *ctrl = data; + + if (dev->type == &spmi_dev_type && spmidev->ctrl == ctrl) + spmi_remove_device(spmidev); + + return 0; +} + +int spmi_controller_add_device(struct spmi_controller *ctrl, + struct spmi_device *sdev) +{ + int err; + + sdev->ctrl = ctrl; + sdev->dev.parent = &ctrl->dev; + + 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; + } + + spmi_dfs_add_device(sdev); + + dev_dbg(&sdev->dev, "device %s registered\n", dev_name(&sdev->dev)); + +err_device_add: + return err; +} +EXPORT_SYMBOL_GPL(spmi_controller_add_device); + +void spmi_remove_device(struct spmi_device *sdev) +{ + device_unregister(&sdev->dev); +} +EXPORT_SYMBOL_GPL(spmi_remove_device); + +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, 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, 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, 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, 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, +}; + +/** + * spmi_device_initialize: initialize a new SPMI device. + * @sdev: device which is to be initialized + * + * Caller is responsible to call spmi_controller_add_device() on the returned + * spmi_device. + */ +void spmi_device_initialize(struct spmi_device *sdev) +{ + sdev->dev.bus = &spmi_bus_type; + sdev->dev.type = &spmi_dev_type; + device_initialize(&sdev->dev); +} +EXPORT_SYMBOL_GPL(spmi_device_initialize); + +static int spmi_register_controller(struct spmi_controller *ctrl) +{ + int ret; + + /* Can't register until after driver model init */ + if (WARN_ON(!spmi_bus_type.p)) { + ret = -EAGAIN; + goto exit; + } + + dev_set_name(&ctrl->dev, "spmi-%d", ctrl->nr); + ctrl->dev.bus = &spmi_bus_type; + ctrl->dev.type = &spmi_ctrl_type; + ret = device_register(&ctrl->dev); + if (ret) + goto exit; + + spmi_dfs_add_controller(ctrl); + + dev_dbg(&ctrl->dev, "Bus spmi-%d registered: dev:%p\n", + ctrl->nr, &ctrl->dev); + + return 0; + +exit: + mutex_lock(&board_lock); + idr_remove(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + return ret; +} + +int spmi_add_controller(struct spmi_controller *ctrl) +{ + int id; + + pr_debug("adding controller for bus %d (0x%p)\n", ctrl->nr, ctrl); + + if (((int)ctrl->nr) < 0) { + pr_err("invalid bus identifier %d\n", ctrl->nr); + return -EINVAL; + } + + idr_preload(GFP_KERNEL); + mutex_lock(&board_lock); + id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, ctrl->nr + 1, GFP_NOWAIT); + mutex_unlock(&board_lock); + idr_preload_end(); + + if (id < 0) { + pr_err( + "idr_alloc(start:%d end:%d):%d at spmi_add_controller()\n", + ctrl->nr, ctrl->nr, id); + return id; + } + + return spmi_register_controller(ctrl); +}; +EXPORT_SYMBOL_GPL(spmi_add_controller); + +/** + * spmi_del_controller: Controller tear-down. + * @ctrl: controller to be removed. + * + * Controller added with the above API is torn down using this API. + */ +int spmi_del_controller(struct spmi_controller *ctrl) +{ + struct spmi_controller *found; + + if (!ctrl) + return -EINVAL; + + /* Check that the ctrl has been added */ + mutex_lock(&board_lock); + found = idr_find(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + + if (found != ctrl) + return -EINVAL; + + spmi_dfs_del_controller(ctrl); + + /* Remove all the clients associated with this controller */ + mutex_lock(&board_lock); + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); + idr_remove(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + + init_completion(&ctrl->dev_released); + device_unregister(&ctrl->dev); + wait_for_completion(&ctrl->dev_released); + + return 0; +} +EXPORT_SYMBOL_GPL(spmi_del_controller); + +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); + spmi_dfs_exit(); +} +module_exit(spmi_exit); + +static int __init spmi_init(void) +{ + spmi_dfs_init(); + 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/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/of_spmi.h b/include/linux/of_spmi.h new file mode 100644 index 0000000..6965d7d --- /dev/null +++ b/include/linux/of_spmi.h @@ -0,0 +1,37 @@ +/* Copyright (c) 2012, 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_OF_SPMI +#define _LINUX_OF_SPMI + +struct spmi_controller; + +#ifdef CONFIG_OF_SPMI +/** + * of_spmi_register_devices() - Register devices in the SPMI Device Tree + * @ctrl: spmi_controller which devices should be registered to. + * + * This routine scans the SPMI Device Tree, allocating resources and + * creating spmi_devices according to the SPMI bus Device Tree + * hierarchy. Details of this hierarchy can be found in + * Documentation/devicetree/bindings/spmi. This routine is normally + * called from the probe routine of the driver registering as a + * spmi_controller. + */ +int of_spmi_register_devices(struct spmi_controller *ctrl); +#else +static int of_spmi_register_devices(struct spmi_controller *ctrl) +{ + return -ENXIO; +} +#endif /* CONFIG_OF_SPMI */ + +#endif diff --git a/include/linux/spmi.h b/include/linux/spmi.h new file mode 100644 index 0000000..091d82bf --- /dev/null +++ b/include/linux/spmi.h @@ -0,0 +1,337 @@ +/* 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. + * @dfs_dir: dentry for this device in debugfs + * @dfs_cnt: number of bytes to access for a debugfs operation + * @dfs_addr: address to access for a debugfs operation + */ +struct spmi_device { + struct device dev; + struct spmi_controller *ctrl; + u8 usid; + +#if CONFIG_DEBUG_FS + struct dentry *dfs_dir; + u32 dfs_cnt; + u32 dfs_addr; +#endif +}; +#define to_spmi_device(d) container_of(d, struct spmi_device, dev) + +static inline void *spmi_device_getdata(const struct spmi_device *sdev) +{ + return dev_get_drvdata(&sdev->dev); +} + +static inline void spmi_device_setdata(struct spmi_device *sdev, void *data) +{ + dev_set_drvdata(&sdev->dev, data); +} + +static inline void spmi_device_put(struct spmi_device *sdev) +{ + if (sdev) + put_device(&sdev->dev); +} + +/** + * 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 + * @dev_released: completion used during controller teardown + * @dfs_dir: dentry for controller directory in debugfs + * @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; + struct completion dev_released; + struct dentry *dfs_dir; + 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, u8 *buf); +}; +#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev) + +static inline void *spmi_get_ctrldata(const struct spmi_controller *ctrl) +{ + return dev_get_drvdata(&ctrl->dev); +} + +static inline void spmi_set_ctrldata(struct spmi_controller *ctrl, void *data) +{ + dev_set_drvdata(&ctrl->dev, data); +} + +/** + * 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. + */ +extern 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_add_controller: Controller bring-up. + * @ctrl: controller to be registered. + * + * A controller is registered with the framework using this API. ctrl->nr is the + * desired number with which SPMI framework registers the controller. + * Function will return -EBUSY if the number is in use. + */ +extern int spmi_add_controller(struct spmi_controller *ctrl); + +/** + * spmi_del_controller: Controller tear-down. + * Controller added with the above API is torn down using this API. + */ +extern int spmi_del_controller(struct spmi_controller *ctrl); + +/** + * spmi_device_initialize - initialize spmi_device + * @sdev: spmi_device to be initialized + * + * Caller is responsible for calling spmi_controller_add_device() + */ +extern void spmi_device_initialize(struct spmi_device *sdev); + +/** + * spmi_controller_add_device: Add spmi_device attached to the given controller + * @ctrl: controlling master device + * @sdev: spmi_device to be added (registered). + */ +extern int spmi_controller_add_device(struct spmi_controller *ctrl, + struct spmi_device *sdev); + +/** + * spmi_remove_device: remove a device + * @sdev: spmi_device to be removed + */ +extern void spmi_remove_device(struct spmi_device *sdev); + +/** + * 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. + */ +extern 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. + */ +extern 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. + */ +extern 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. + */ +extern int spmi_register_write(struct spmi_device *sdev, u8 addr, 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. + */ +extern 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. + */ +extern int spmi_ext_register_write(struct spmi_device *sdev, u8 addr, 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. + */ +extern int spmi_ext_register_writel(struct spmi_device *sdev, u16 addr, + 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. + */ +extern 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. + */ +extern 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. + */ +extern 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. + */ +extern 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-09 20:37 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-09 20:37 UTC (permalink / raw) To: linux-arm-kernel From: Kenneth Heitke <kheitke@codeaurora.org> 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 <kheitke@codeaurora.org> Signed-off-by: Michael Bohan <mbohan@codeaurora.org> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> --- drivers/Kconfig | 2 + drivers/Makefile | 1 + drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/of_spmi.c | 74 +++++ drivers/spmi/Kconfig | 9 + drivers/spmi/Makefile | 7 + drivers/spmi/spmi-dbgfs.c | 591 ++++++++++++++++++++++++++++++++++++++++ drivers/spmi/spmi-dbgfs.h | 37 +++ drivers/spmi/spmi.c | 449 ++++++++++++++++++++++++++++++ include/linux/mod_devicetable.h | 8 + include/linux/of_spmi.h | 37 +++ include/linux/spmi.h | 337 +++++++++++++++++++++++ 13 files changed, 1559 insertions(+) create mode 100644 drivers/of/of_spmi.c create mode 100644 drivers/spmi/Kconfig create mode 100644 drivers/spmi/Makefile create mode 100644 drivers/spmi/spmi-dbgfs.c create mode 100644 drivers/spmi/spmi-dbgfs.h create mode 100644 drivers/spmi/spmi.c create mode 100644 include/linux/of_spmi.h create mode 100644 include/linux/spmi.h 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/of/Kconfig b/drivers/of/Kconfig index 80e5c13..916b0c1 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -76,6 +76,12 @@ config OF_PCI_IRQ help OpenFirmware PCI IRQ routing helpers +config OF_SPMI + def_tristate SPMI + depends on SPMI + help + OpenFirmware SPMI bus accessors + config OF_MTD depends on MTD def_bool y diff --git a/drivers/of/Makefile b/drivers/of/Makefile index 1f9c0c4..5486614 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -9,4 +9,5 @@ obj-$(CONFIG_OF_SELFTEST) += selftest.o obj-$(CONFIG_OF_MDIO) += of_mdio.o obj-$(CONFIG_OF_PCI) += of_pci.o obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o +obj-$(CONFIG_OF_SPMI) += of_spmi.o obj-$(CONFIG_OF_MTD) += of_mtd.o diff --git a/drivers/of/of_spmi.c b/drivers/of/of_spmi.c new file mode 100644 index 0000000..fca19c5 --- /dev/null +++ b/drivers/of/of_spmi.c @@ -0,0 +1,74 @@ +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/slab.h> +#include <linux/spmi.h> +#include <linux/of_spmi.h> + +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 usid; + + dev_dbg(&ctrl->dev, "adding child %s\n", node->full_name); + + err = of_property_read_u32(node, "reg", &usid); + if (err) { + dev_err(&ctrl->dev, + "node %s does not have 'reg' property\n", + node->full_name); + continue; + } + + if (usid > 0xFF) { + dev_err(&ctrl->dev, + "invalid usid on node %s\n", + node->full_name); + continue; + } + + dev_dbg(&ctrl->dev, "read usid %02x\n", usid); + + sdev = kzalloc(sizeof(*sdev), GFP_KERNEL); + if (!sdev) + continue; + + sdev->dev.of_node = node; + sdev->usid = (u8) usid; + spmi_device_initialize(sdev); + err = spmi_controller_add_device(ctrl, sdev); + if (err) { + dev_err(&sdev->dev, + "failure adding device. status %d\n", err); + continue; + } + } + + return 0; +} +EXPORT_SYMBOL(of_spmi_register_devices); + +MODULE_LICENSE("GPL v2"); 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..ef96afd --- /dev/null +++ b/drivers/spmi/Makefile @@ -0,0 +1,7 @@ +# +# Makefile for kernel SPMI framework. +# +obj-$(CONFIG_SPMI) += spmi-core.o + +spmi-core-y += spmi.o +spmi-core-$(CONFIG_DEBUG_FS) += spmi-dbgfs.o diff --git a/drivers/spmi/spmi-dbgfs.c b/drivers/spmi/spmi-dbgfs.c new file mode 100644 index 0000000..5ca0a44 --- /dev/null +++ b/drivers/spmi/spmi-dbgfs.c @@ -0,0 +1,591 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/sizes.h> +#include <linux/slab.h> +#include <linux/uaccess.h> +#include <linux/spinlock.h> +#include <linux/string.h> +#include <linux/debugfs.h> +#include <linux/spmi.h> +#include <linux/ctype.h> +#include "spmi-dbgfs.h" + +#define ADDR_LEN 6 /* 5 byte address + 1 space character */ +#define CHARS_PER_ITEM 3 /* Format is 'XX ' */ +#define ITEMS_PER_LINE 16 /* 16 data items per line */ +#define MAX_LINE_LENGTH (ADDR_LEN + (ITEMS_PER_LINE * CHARS_PER_ITEM) + 1) + +#define MAX_REG_PER_TRANSACTION (8) + +static const mode_t DFS_MODE = S_IRUSR | S_IWUSR; + +/* Log buffer */ +struct spmi_log_buffer { + u32 rpos; /* Current 'read' position in buffer */ + u32 wpos; /* Current 'write' position in buffer */ + u32 len; /* Length of the buffer */ + char data[0]; /* Log buffer */ +}; + +/* SPMI transaction parameters */ +struct spmi_trans { + u32 cnt; /* Number of bytes to read */ + u32 addr; /* 20-bit address: SID + PID + Register offset */ + u32 offset; /* Offset of last read data */ + bool raw_data; /* Set to true for raw data dump */ + struct spmi_device *sdev; + struct spmi_log_buffer *log; /* log buffer */ +}; + +static char dbgfs_help[] = + "SPMI Debug-FS support\n" + "\n" + "Hierarchy schema:\n" + "/sys/kernel/debug/spmi\n" + " /help -- Static help text\n" + " /spmi-0 -- Directory for SPMI bus 0\n" + " /spmi-0/0-1 -- Directory for SPMI device '0-1'\n" + " /spmi-0/0-1/address -- Starting register for reads or writes\n" + " /spmi-0/0-1/count -- Number of registers to read (only used for reads)\n" + " /spmi-0/0-1/data -- Initiates the SPMI read (formatted output)\n" + " /spmi-0/0-1/data_raw -- Initiates the SPMI raw read or write\n" + " /spmi-n -- Directory for SPMI bus n\n" + "\n" + "To perform SPMI read or write transactions, you need to first write the\n" + "address of the slave device register to the 'address' file. For read\n" + "transactions, the number of bytes to be read needs to be written to the\n" + "'count' file.\n" + "\n" + "The 'address' file specifies the 20-bit address of a slave device register.\n" + "The upper 4 bits 'address[19..16]' specify the slave identifier (SID) for\n" + "the slave device. The lower 16 bits specify the slave register address.\n" + "\n" + "Reading from the 'data' file will initiate a SPMI read transaction starting\n" + "from slave register 'address' for 'count' number of bytes.\n" + "\n" + "Writing to the 'data' file will initiate a SPMI write transaction starting\n" + "from slave register 'address'. The number of registers written to will\n" + "match the number of bytes written to the 'data' file.\n" + "\n" + "Example: Read 4 bytes starting at register address 0x1234 for SID 2\n" + "\n" + "echo 0x21234 > address\n" + "echo 4 > count\n" + "cat data\n" + "\n" + "Example: Write 3 bytes starting at register address 0x1008 for SID 1\n" + "\n" + "echo 0x11008 > address\n" + "echo 0x01 0x02 0x03 > data\n" + "\n" + "Note that the count file is not used for writes. Since 3 bytes are\n" + "written to the 'data' file, then 3 bytes will be written across the\n" + "SPMI bus.\n\n"; + +static struct debugfs_blob_wrapper spmi_debug_help = { + .data = dbgfs_help, + .size = sizeof(dbgfs_help), +}; + +static struct dentry *spmi_debug_root; + +static int spmi_device_dfs_open(struct spmi_device *sdev, struct file *file) +{ + struct spmi_log_buffer *log; + struct spmi_trans *trans; + + size_t logbufsize = SZ_4K; + + /* Per file "transaction" data */ + trans = kzalloc(sizeof(*trans), GFP_KERNEL); + if (!trans) + return -ENOMEM; + + log = kzalloc(logbufsize, GFP_KERNEL); + if (!log) { + kfree(trans); + return -ENOMEM; + } + + log->rpos = 0; + log->wpos = 0; + log->len = logbufsize - sizeof(*log); + + trans->log = log; + trans->cnt = sdev->dfs_cnt; + trans->addr = sdev->dfs_addr; + trans->sdev = sdev; + trans->offset = trans->addr; + + file->private_data = trans; + return 0; +} + +static int spmi_device_dfs_data_open(struct inode *inode, struct file *file) +{ + struct spmi_device *sdev = inode->i_private; + return spmi_device_dfs_open(sdev, file); +} + +static int spmi_device_dfs_raw_data_open(struct inode *inode, struct file *file) +{ + struct spmi_device *sdev = inode->i_private; + struct spmi_trans *trans; + int rc; + + rc = spmi_device_dfs_open(sdev, file); + trans = file->private_data; + trans->raw_data = true; + return rc; +} + +static int spmi_device_dfs_close(struct inode *inode, struct file *file) +{ + struct spmi_trans *trans = file->private_data; + kfree(trans->log); + kfree(trans); + return 0; +} + +/** + * spmi_read_data: reads data across the SPMI bus + * @ctrl: The SPMI controller + * @buf: buffer to store the data read. + * @offset: SPMI address offset to start reading from. + * @cnt: The number of bytes to read. + * + * Returns 0 on success, otherwise returns error code from SPMI driver. + */ +static int +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) +{ + int ret = 0; + int len; + uint16_t addr; + + while (cnt > 0) { + addr = offset & 0xFFFF; + len = min(cnt, MAX_REG_PER_TRANSACTION); + + ret = spmi_ext_register_readl(sdev, addr, buf, len); + if (ret < 0) { + pr_err("SPMI read failed, err = %d\n", ret); + goto done; + } + + cnt -= len; + buf += len; + offset += len; + } + +done: + return ret; +} + +/** + * spmi_write_data: writes data across the SPMI bus + * @ctrl: The SPMI controller + * @buf: data to be written. + * @offset: SPMI address offset to start writing to. + * @cnt: The number of bytes to write. + * + * Returns 0 on success, otherwise returns error code from SPMI driver. + */ +static int +spmi_write_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) +{ + int ret = 0; + int len; + uint16_t addr; + + while (cnt > 0) { + addr = offset & 0xFFFF; + len = min(cnt, MAX_REG_PER_TRANSACTION); + + ret = spmi_ext_register_writel(sdev, addr, buf, len); + if (ret < 0) { + pr_err("SPMI write failed, err = %d\n", ret); + goto done; + } + + cnt -= len; + buf += len; + offset += len; + } + +done: + return ret; +} + +/** + * print_to_log: format a string and place into the log buffer + * @log: The log buffer to place the result into. + * @fmt: The format string to use. + * @...: The arguments for the format string. + * + * The return value is the number of characters written to @log buffer + * not including the trailing '\0'. + */ +static int print_to_log(struct spmi_log_buffer *log, const char *fmt, ...) +{ + va_list args; + int cnt; + char *buf = &log->data[log->wpos]; + size_t size = log->len - log->wpos; + + va_start(args, fmt); + cnt = vscnprintf(buf, size, fmt, args); + va_end(args); + + log->wpos += cnt; + return cnt; +} + +/** + * write_next_line_to_log: Writes a single "line" of data into the log buffer + * @trans: Pointer to SPMI transaction data. + * @offset: SPMI address offset to start reading from. + * @pcnt: Pointer to 'cnt' variable. Indicates the number of bytes to read. + * + * The 'offset' is a 20-bits SPMI address which includes a 4-bit slave id (SID), + * an 8-bit peripheral id (PID), and an 8-bit peripheral register address. + * + * On a successful read, the pcnt is decremented by the number of data + * bytes read across the SPMI bus. When the cnt reaches 0, all requested + * bytes have been read. + */ +static int +write_next_line_to_log(struct spmi_trans *trans, int offset, size_t *pcnt) +{ + int i, j; + u8 data[ITEMS_PER_LINE]; + struct spmi_log_buffer *log = trans->log; + + int cnt = 0; + int padding = offset % ITEMS_PER_LINE; + int items_to_read = min(ARRAY_SIZE(data) - padding, *pcnt); + int items_to_log = min(ITEMS_PER_LINE, padding + items_to_read); + + /* Buffer needs enough space for an entire line */ + if ((log->len - log->wpos) < MAX_LINE_LENGTH) + goto done; + + /* Read the desired number of "items" */ + if (spmi_read_data(trans->sdev, data, offset, items_to_read)) + goto done; + + *pcnt -= items_to_read; + + /* Each line starts with the aligned offset (20-bit address) */ + cnt = print_to_log(log, "%5.5X ", offset & 0xffff0); + if (cnt == 0) + goto done; + + /* If the offset is unaligned, add padding to right justify items */ + for (i = 0; i < padding; ++i) { + cnt = print_to_log(log, "-- "); + if (cnt == 0) + goto done; + } + + /* Log the data items */ + for (j = 0; i < items_to_log; ++i, ++j) { + cnt = print_to_log(log, "%2.2X ", data[j]); + if (cnt == 0) + goto done; + } + + /* If the last character was a space, then replace it with a newline */ + if (log->wpos > 0 && log->data[log->wpos - 1] == ' ') + log->data[log->wpos - 1] = '\n'; + +done: + return cnt; +} + +/** + * write_raw_data_to_log: Writes a single "line" of data into the log buffer + * @trans: Pointer to SPMI transaction data. + * @offset: SPMI address offset to start reading from. + * @pcnt: Pointer to 'cnt' variable. Indicates the number of bytes to read. + * + * The 'offset' is a 20-bits SPMI address which includes a 4-bit slave id (SID), + * an 8-bit peripheral id (PID), and an 8-bit peripheral register address. + * + * On a successful read, the pcnt is decremented by the number of data + * bytes read across the SPMI bus. When the cnt reaches 0, all requested + * bytes have been read. + */ +static int +write_raw_data_to_log(struct spmi_trans *trans, int offset, size_t *pcnt) +{ + u8 data[16]; + struct spmi_log_buffer *log = trans->log; + + int i; + int cnt = 0; + int items_to_read = min(ARRAY_SIZE(data), *pcnt); + + /* Buffer needs enough space for an entire line */ + if ((log->len - log->wpos) < 80) + goto done; + + /* Read the desired number of "items" */ + if (spmi_read_data(trans->sdev, data, offset, items_to_read)) + goto done; + + *pcnt -= items_to_read; + + /* Log the data items */ + for (i = 0; i < items_to_read; ++i) { + cnt = print_to_log(log, "0x%2.2X ", data[i]); + if (cnt == 0) + goto done; + } + + /* If the last character was a space, then replace it with a newline */ + if (log->wpos > 0 && log->data[log->wpos - 1] == ' ') + log->data[log->wpos - 1] = '\n'; + +done: + return cnt; +} + +/** + * get_log_data - reads data across the SPMI bus and saves to the log buffer + * @trans: Pointer to SPMI transaction data. + * + * Returns the number of "items" read or SPMI error code for read failures. + */ +static int get_log_data(struct spmi_trans *trans) +{ + int cnt; + int last_cnt; + int items_read; + int total_items_read = 0; + u32 offset = trans->offset; + size_t item_cnt = trans->cnt; + struct spmi_log_buffer *log = trans->log; + int (*write_to_log)(struct spmi_trans *, int, size_t *); + + if (item_cnt == 0) + return 0; + + if (trans->raw_data) + write_to_log = write_raw_data_to_log; + else + write_to_log = write_next_line_to_log; + + /* Reset the log buffer 'pointers' */ + log->wpos = log->rpos = 0; + + /* Keep reading data until the log is full */ + do { + last_cnt = item_cnt; + cnt = write_to_log(trans, offset, &item_cnt); + items_read = last_cnt - item_cnt; + offset += items_read; + total_items_read += items_read; + } while (cnt && item_cnt > 0); + + /* Adjust the transaction offset and count */ + trans->cnt = item_cnt; + trans->offset += total_items_read; + + return total_items_read; +} + +static ssize_t spmi_device_dfs_reg_write(struct file *file, + const char __user *buf, + size_t count, loff_t *ppos) +{ + int bytes_read; + int data; + int pos = 0; + int cnt = 0; + u8 *values; + size_t ret = 0; + + struct spmi_trans *trans = file->private_data; + u32 offset = trans->offset; + + /* Make a copy of the user data */ + char *kbuf = kmalloc(count + 1, GFP_KERNEL); + if (!kbuf) + return -ENOMEM; + + ret = copy_from_user(kbuf, buf, count); + if (ret == count) { + pr_err("failed to copy data from user\n"); + ret = -EFAULT; + goto free_buf; + } + + count -= ret; + *ppos += count; + kbuf[count] = '\0'; + + /* Override the text buffer with the raw data */ + values = kbuf; + + /* Parse the data in the buffer. It should be a string of numbers */ + while (sscanf(kbuf + pos, "%i%n", &data, &bytes_read) == 1) { + pos += bytes_read; + values[cnt++] = data & 0xff; + } + + if (!cnt) + goto free_buf; + + /* Perform the SPMI write(s) */ + ret = spmi_write_data(trans->sdev, values, offset, cnt); + + if (ret) { + pr_err("SPMI write failed, err = %zu\n", ret); + } else { + ret = count; + trans->offset += cnt; + } + +free_buf: + kfree(kbuf); + return ret; +} + +static ssize_t spmi_device_dfs_reg_read(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct spmi_trans *trans = file->private_data; + struct spmi_log_buffer *log = trans->log; + size_t ret; + size_t len; + + /* Is the the log buffer empty */ + if (log->rpos >= log->wpos) { + if (get_log_data(trans) <= 0) + return 0; + } + + len = min(count, (size_t) log->wpos - log->rpos); + + ret = copy_to_user(buf, &log->data[log->rpos], len); + if (ret == len) { + pr_err("error copy SPMI register values to user\n"); + return -EFAULT; + } + + /* 'ret' is the number of bytes not copied */ + len -= ret; + + *ppos += len; + log->rpos += len; + return len; +} + +static const struct file_operations spmi_dfs_reg_fops = { + .open = spmi_device_dfs_data_open, + .release = spmi_device_dfs_close, + .read = spmi_device_dfs_reg_read, + .write = spmi_device_dfs_reg_write, +}; + +static const struct file_operations spmi_dfs_raw_data_fops = { + .open = spmi_device_dfs_raw_data_open, + .release = spmi_device_dfs_close, + .read = spmi_device_dfs_reg_read, + .write = spmi_device_dfs_reg_write, +}; + +void spmi_dfs_add_controller(struct spmi_controller *ctrl) +{ + ctrl->dfs_dir = debugfs_create_dir(dev_name(&ctrl->dev), + spmi_debug_root); + WARN_ON(!ctrl->dfs_dir); + + dev_dbg(&ctrl->dev, "adding debug entries for spmi controller\n"); +} + +void spmi_dfs_del_controller(struct spmi_controller *ctrl) +{ + debugfs_remove_recursive(ctrl->dfs_dir); +} + +void spmi_dfs_add_device(struct spmi_device *sdev) +{ + struct dentry *file; + + dev_dbg(&sdev->dev, "adding debugfs entries for spmi device\n"); + + sdev->dfs_dir = debugfs_create_dir(dev_name(&sdev->dev), + sdev->ctrl->dfs_dir); + if (WARN_ON(!sdev->dfs_dir)) + return; + + sdev->dfs_cnt = 1; + + file = debugfs_create_u32("count", DFS_MODE, sdev->dfs_dir, + &sdev->dfs_cnt); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_x32("address", DFS_MODE, sdev->dfs_dir, + &sdev->dfs_addr); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_file("data", DFS_MODE, sdev->dfs_dir, sdev, + &spmi_dfs_reg_fops); + if (WARN_ON(!file)) + goto err_remove_fs; + + file = debugfs_create_file("data_raw", DFS_MODE, sdev->dfs_dir, + sdev, &spmi_dfs_raw_data_fops); + if (WARN_ON(!file)) + goto err_remove_fs; + + return; + +err_remove_fs: + debugfs_remove_recursive(sdev->dfs_dir); +} + +void spmi_dfs_del_device(struct spmi_device *sdev) +{ + dev_dbg(&sdev->dev, "Deleting device\n"); + debugfs_remove_recursive(sdev->dfs_dir); +} + +void __exit spmi_dfs_exit(void) +{ + pr_debug("de-initializing spmi debugfs ..."); + debugfs_remove_recursive(spmi_debug_root); +} + +void __init spmi_dfs_init(void) +{ + struct dentry *help; + + pr_debug("creating SPMI debugfs file-system\n"); + + spmi_debug_root = debugfs_create_dir("spmi", NULL); + + help = debugfs_create_blob("help", S_IRUGO, spmi_debug_root, + &spmi_debug_help); + + WARN_ON(!spmi_debug_root || !help); +} + +MODULE_LICENSE("GPL v2"); diff --git a/drivers/spmi/spmi-dbgfs.h b/drivers/spmi/spmi-dbgfs.h new file mode 100644 index 0000000..896bb5a --- /dev/null +++ b/drivers/spmi/spmi-dbgfs.h @@ -0,0 +1,37 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef _SPMI_DBGFS_H +#define _SPMI_DBGFS_H + +#include <linux/spmi.h> +#include <linux/debugfs.h> + +#ifdef CONFIG_DEBUG_FS + +extern void __init spmi_dfs_init(void); +extern void __exit spmi_dfs_exit(void); +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); +extern void spmi_dfs_add_device(struct spmi_device *sdev); +extern void spmi_dfs_del_device(struct spmi_device *sdev); + +#else + +static inline void __init spmi_dfs_init(void) { } +static inline void __exit spmi_dfs_exit(void) { } +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } +#endif + +#endif /* _SPMI_DBGFS_H */ diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c new file mode 100644 index 0000000..3bbcc63 --- /dev/null +++ b/drivers/spmi/spmi.c @@ -0,0 +1,449 @@ +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#define pr_fmt(fmt) "%s: " fmt, __func__ + +#include <linux/kernel.h> +#include <linux/errno.h> +#include <linux/idr.h> +#include <linux/slab.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/spmi.h> +#include <linux/module.h> +#include <linux/pm_runtime.h> + +#include "spmi-dbgfs.h" + +static DEFINE_MUTEX(board_lock); +static DEFINE_IDR(ctrl_idr); + +static void spmi_dev_release(struct device *dev) +{ + struct spmi_device *spmidev = to_spmi_device(dev); + kfree(spmidev); +} + +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); + complete(&ctrl->dev_released); +} + +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; +} + +/* 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); + struct spmi_controller *ctrl = data; + + if (dev->type == &spmi_dev_type && spmidev->ctrl == ctrl) + spmi_remove_device(spmidev); + + return 0; +} + +int spmi_controller_add_device(struct spmi_controller *ctrl, + struct spmi_device *sdev) +{ + int err; + + sdev->ctrl = ctrl; + sdev->dev.parent = &ctrl->dev; + + 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; + } + + spmi_dfs_add_device(sdev); + + dev_dbg(&sdev->dev, "device %s registered\n", dev_name(&sdev->dev)); + +err_device_add: + return err; +} +EXPORT_SYMBOL_GPL(spmi_controller_add_device); + +void spmi_remove_device(struct spmi_device *sdev) +{ + device_unregister(&sdev->dev); +} +EXPORT_SYMBOL_GPL(spmi_remove_device); + +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, 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, 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, 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, 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, +}; + +/** + * spmi_device_initialize: initialize a new SPMI device. + * @sdev: device which is to be initialized + * + * Caller is responsible to call spmi_controller_add_device() on the returned + * spmi_device. + */ +void spmi_device_initialize(struct spmi_device *sdev) +{ + sdev->dev.bus = &spmi_bus_type; + sdev->dev.type = &spmi_dev_type; + device_initialize(&sdev->dev); +} +EXPORT_SYMBOL_GPL(spmi_device_initialize); + +static int spmi_register_controller(struct spmi_controller *ctrl) +{ + int ret; + + /* Can't register until after driver model init */ + if (WARN_ON(!spmi_bus_type.p)) { + ret = -EAGAIN; + goto exit; + } + + dev_set_name(&ctrl->dev, "spmi-%d", ctrl->nr); + ctrl->dev.bus = &spmi_bus_type; + ctrl->dev.type = &spmi_ctrl_type; + ret = device_register(&ctrl->dev); + if (ret) + goto exit; + + spmi_dfs_add_controller(ctrl); + + dev_dbg(&ctrl->dev, "Bus spmi-%d registered: dev:%p\n", + ctrl->nr, &ctrl->dev); + + return 0; + +exit: + mutex_lock(&board_lock); + idr_remove(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + return ret; +} + +int spmi_add_controller(struct spmi_controller *ctrl) +{ + int id; + + pr_debug("adding controller for bus %d (0x%p)\n", ctrl->nr, ctrl); + + if (((int)ctrl->nr) < 0) { + pr_err("invalid bus identifier %d\n", ctrl->nr); + return -EINVAL; + } + + idr_preload(GFP_KERNEL); + mutex_lock(&board_lock); + id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, ctrl->nr + 1, GFP_NOWAIT); + mutex_unlock(&board_lock); + idr_preload_end(); + + if (id < 0) { + pr_err( + "idr_alloc(start:%d end:%d):%d@spmi_add_controller()\n", + ctrl->nr, ctrl->nr, id); + return id; + } + + return spmi_register_controller(ctrl); +}; +EXPORT_SYMBOL_GPL(spmi_add_controller); + +/** + * spmi_del_controller: Controller tear-down. + * @ctrl: controller to be removed. + * + * Controller added with the above API is torn down using this API. + */ +int spmi_del_controller(struct spmi_controller *ctrl) +{ + struct spmi_controller *found; + + if (!ctrl) + return -EINVAL; + + /* Check that the ctrl has been added */ + mutex_lock(&board_lock); + found = idr_find(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + + if (found != ctrl) + return -EINVAL; + + spmi_dfs_del_controller(ctrl); + + /* Remove all the clients associated with this controller */ + mutex_lock(&board_lock); + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); + idr_remove(&ctrl_idr, ctrl->nr); + mutex_unlock(&board_lock); + + init_completion(&ctrl->dev_released); + device_unregister(&ctrl->dev); + wait_for_completion(&ctrl->dev_released); + + return 0; +} +EXPORT_SYMBOL_GPL(spmi_del_controller); + +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); + spmi_dfs_exit(); +} +module_exit(spmi_exit); + +static int __init spmi_init(void) +{ + spmi_dfs_init(); + 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/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/of_spmi.h b/include/linux/of_spmi.h new file mode 100644 index 0000000..6965d7d --- /dev/null +++ b/include/linux/of_spmi.h @@ -0,0 +1,37 @@ +/* Copyright (c) 2012, 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_OF_SPMI +#define _LINUX_OF_SPMI + +struct spmi_controller; + +#ifdef CONFIG_OF_SPMI +/** + * of_spmi_register_devices() - Register devices in the SPMI Device Tree + * @ctrl: spmi_controller which devices should be registered to. + * + * This routine scans the SPMI Device Tree, allocating resources and + * creating spmi_devices according to the SPMI bus Device Tree + * hierarchy. Details of this hierarchy can be found in + * Documentation/devicetree/bindings/spmi. This routine is normally + * called from the probe routine of the driver registering as a + * spmi_controller. + */ +int of_spmi_register_devices(struct spmi_controller *ctrl); +#else +static int of_spmi_register_devices(struct spmi_controller *ctrl) +{ + return -ENXIO; +} +#endif /* CONFIG_OF_SPMI */ + +#endif diff --git a/include/linux/spmi.h b/include/linux/spmi.h new file mode 100644 index 0000000..091d82bf --- /dev/null +++ b/include/linux/spmi.h @@ -0,0 +1,337 @@ +/* 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. + * @dfs_dir: dentry for this device in debugfs + * @dfs_cnt: number of bytes to access for a debugfs operation + * @dfs_addr: address to access for a debugfs operation + */ +struct spmi_device { + struct device dev; + struct spmi_controller *ctrl; + u8 usid; + +#if CONFIG_DEBUG_FS + struct dentry *dfs_dir; + u32 dfs_cnt; + u32 dfs_addr; +#endif +}; +#define to_spmi_device(d) container_of(d, struct spmi_device, dev) + +static inline void *spmi_device_getdata(const struct spmi_device *sdev) +{ + return dev_get_drvdata(&sdev->dev); +} + +static inline void spmi_device_setdata(struct spmi_device *sdev, void *data) +{ + dev_set_drvdata(&sdev->dev, data); +} + +static inline void spmi_device_put(struct spmi_device *sdev) +{ + if (sdev) + put_device(&sdev->dev); +} + +/** + * 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 + * @dev_released: completion used during controller teardown + * @dfs_dir: dentry for controller directory in debugfs + * @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; + struct completion dev_released; + struct dentry *dfs_dir; + 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, u8 *buf); +}; +#define to_spmi_controller(d) container_of(d, struct spmi_controller, dev) + +static inline void *spmi_get_ctrldata(const struct spmi_controller *ctrl) +{ + return dev_get_drvdata(&ctrl->dev); +} + +static inline void spmi_set_ctrldata(struct spmi_controller *ctrl, void *data) +{ + dev_set_drvdata(&ctrl->dev, data); +} + +/** + * 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. + */ +extern 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_add_controller: Controller bring-up. + * @ctrl: controller to be registered. + * + * A controller is registered with the framework using this API. ctrl->nr is the + * desired number with which SPMI framework registers the controller. + * Function will return -EBUSY if the number is in use. + */ +extern int spmi_add_controller(struct spmi_controller *ctrl); + +/** + * spmi_del_controller: Controller tear-down. + * Controller added with the above API is torn down using this API. + */ +extern int spmi_del_controller(struct spmi_controller *ctrl); + +/** + * spmi_device_initialize - initialize spmi_device + * @sdev: spmi_device to be initialized + * + * Caller is responsible for calling spmi_controller_add_device() + */ +extern void spmi_device_initialize(struct spmi_device *sdev); + +/** + * spmi_controller_add_device: Add spmi_device attached to the given controller + * @ctrl: controlling master device + * @sdev: spmi_device to be added (registered). + */ +extern int spmi_controller_add_device(struct spmi_controller *ctrl, + struct spmi_device *sdev); + +/** + * spmi_remove_device: remove a device + * @sdev: spmi_device to be removed + */ +extern void spmi_remove_device(struct spmi_device *sdev); + +/** + * 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. + */ +extern 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. + */ +extern 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. + */ +extern 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. + */ +extern int spmi_register_write(struct spmi_device *sdev, u8 addr, 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. + */ +extern 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. + */ +extern int spmi_ext_register_write(struct spmi_device *sdev, u8 addr, 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. + */ +extern int spmi_ext_register_writel(struct spmi_device *sdev, u16 addr, + 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. + */ +extern 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. + */ +extern 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. + */ +extern 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. + */ +extern 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 ^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-09 20:37 ` Josh Cartwright @ 2013-08-16 18:46 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:46 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > --- /dev/null > +++ b/drivers/of/of_spmi.c > @@ -0,0 +1,74 @@ > +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ As you never make a pr_* call in this file, this line isn't needed. > +++ b/drivers/spmi/spmi-dbgfs.c > @@ -0,0 +1,591 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__ __func__ and __LINE__? Is that really needed, what will it help with? Seems like extra noise, especially as you only have a few pr_* calls in this file: > +static int > +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > +{ > + int ret = 0; > + int len; > + uint16_t addr; > + > + while (cnt > 0) { > + addr = offset & 0xFFFF; > + len = min(cnt, MAX_REG_PER_TRANSACTION); > + > + ret = spmi_ext_register_readl(sdev, addr, buf, len); > + if (ret < 0) { > + pr_err("SPMI read failed, err = %d\n", ret); Should be using dev_err() instead. > +/** > + * spmi_write_data: writes data across the SPMI bus > + * @ctrl: The SPMI controller > + * @buf: data to be written. > + * @offset: SPMI address offset to start writing to. > + * @cnt: The number of bytes to write. > + * > + * Returns 0 on success, otherwise returns error code from SPMI driver. > + */ > +static int > +spmi_write_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > +{ > + int ret = 0; > + int len; > + uint16_t addr; > + > + while (cnt > 0) { > + addr = offset & 0xFFFF; > + len = min(cnt, MAX_REG_PER_TRANSACTION); > + > + ret = spmi_ext_register_writel(sdev, addr, buf, len); > + if (ret < 0) { > + pr_err("SPMI write failed, err = %d\n", ret); Same, dev_err() please. > +static ssize_t spmi_device_dfs_reg_write(struct file *file, > + const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + int bytes_read; > + int data; > + int pos = 0; > + int cnt = 0; > + u8 *values; > + size_t ret = 0; > + > + struct spmi_trans *trans = file->private_data; > + u32 offset = trans->offset; > + > + /* Make a copy of the user data */ > + char *kbuf = kmalloc(count + 1, GFP_KERNEL); > + if (!kbuf) > + return -ENOMEM; > + > + ret = copy_from_user(kbuf, buf, count); > + if (ret == count) { > + pr_err("failed to copy data from user\n"); No need for a message here at all, you will get a message in the function if something happened wrong. Also, shouldn't it just be a simple: if (copy_from_user()) { test? > + ret = -EFAULT; > + goto free_buf; > + } > + > + count -= ret; > + *ppos += count; > + kbuf[count] = '\0'; > + > + /* Override the text buffer with the raw data */ > + values = kbuf; > + > + /* Parse the data in the buffer. It should be a string of numbers */ > + while (sscanf(kbuf + pos, "%i%n", &data, &bytes_read) == 1) { > + pos += bytes_read; > + values[cnt++] = data & 0xff; > + } > + > + if (!cnt) > + goto free_buf; > + > + /* Perform the SPMI write(s) */ > + ret = spmi_write_data(trans->sdev, values, offset, cnt); > + > + if (ret) { > + pr_err("SPMI write failed, err = %zu\n", ret); dev_err() please. > +static ssize_t spmi_device_dfs_reg_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct spmi_trans *trans = file->private_data; > + struct spmi_log_buffer *log = trans->log; > + size_t ret; > + size_t len; > + > + /* Is the the log buffer empty */ > + if (log->rpos >= log->wpos) { > + if (get_log_data(trans) <= 0) > + return 0; > + } > + > + len = min(count, (size_t) log->wpos - log->rpos); > + > + ret = copy_to_user(buf, &log->data[log->rpos], len); > + if (ret == len) { > + pr_err("error copy SPMI register values to user\n"); Same comments as above. > +void __exit spmi_dfs_exit(void) > +{ > + pr_debug("de-initializing spmi debugfs ..."); Not needed, use the in-kernel trace functionality if you really want to know this. > + debugfs_remove_recursive(spmi_debug_root); > +} > + > +void __init spmi_dfs_init(void) > +{ > + struct dentry *help; > + > + pr_debug("creating SPMI debugfs file-system\n"); Same as above, not needed. So, with those changes, pr_fmt() isn't needed at all :) > --- /dev/null > +++ b/drivers/spmi/spmi-dbgfs.h > @@ -0,0 +1,37 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef _SPMI_DBGFS_H > +#define _SPMI_DBGFS_H > + > +#include <linux/spmi.h> > +#include <linux/debugfs.h> > + > +#ifdef CONFIG_DEBUG_FS Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right? > + > +extern void __init spmi_dfs_init(void); > +extern void __exit spmi_dfs_exit(void); > +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); > +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); > +extern void spmi_dfs_add_device(struct spmi_device *sdev); > +extern void spmi_dfs_del_device(struct spmi_device *sdev); > + > +#else > + > +static inline void __init spmi_dfs_init(void) { } > +static inline void __exit spmi_dfs_exit(void) { } > +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } > +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } > +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } > +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } > +#endif > + > +#endif /* _SPMI_DBGFS_H */ > diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c > new file mode 100644 > index 0000000..3bbcc63 > --- /dev/null > +++ b/drivers/spmi/spmi.c > @@ -0,0 +1,449 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ Again, not needed for this file, because: > +int spmi_add_controller(struct spmi_controller *ctrl) > +{ > + int id; > + > + pr_debug("adding controller for bus %d (0x%p)\n", ctrl->nr, ctrl); dev_dbg() if you really need/want it. > + > + if (((int)ctrl->nr) < 0) { > + pr_err("invalid bus identifier %d\n", ctrl->nr); dev_err() please. > + return -EINVAL; > + } > + > + idr_preload(GFP_KERNEL); > + mutex_lock(&board_lock); > + id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, ctrl->nr + 1, GFP_NOWAIT); > + mutex_unlock(&board_lock); > + idr_preload_end(); > + > + if (id < 0) { > + pr_err( > + "idr_alloc(start:%d end:%d):%d at spmi_add_controller()\n", > + ctrl->nr, ctrl->nr, id); dev_err() please. And then the pr_fmt() line can be removed. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 18:46 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:46 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > --- /dev/null > +++ b/drivers/of/of_spmi.c > @@ -0,0 +1,74 @@ > +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ As you never make a pr_* call in this file, this line isn't needed. > +++ b/drivers/spmi/spmi-dbgfs.c > @@ -0,0 +1,591 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s:%d: " fmt, __func__, __LINE__ __func__ and __LINE__? Is that really needed, what will it help with? Seems like extra noise, especially as you only have a few pr_* calls in this file: > +static int > +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > +{ > + int ret = 0; > + int len; > + uint16_t addr; > + > + while (cnt > 0) { > + addr = offset & 0xFFFF; > + len = min(cnt, MAX_REG_PER_TRANSACTION); > + > + ret = spmi_ext_register_readl(sdev, addr, buf, len); > + if (ret < 0) { > + pr_err("SPMI read failed, err = %d\n", ret); Should be using dev_err() instead. > +/** > + * spmi_write_data: writes data across the SPMI bus > + * @ctrl: The SPMI controller > + * @buf: data to be written. > + * @offset: SPMI address offset to start writing to. > + * @cnt: The number of bytes to write. > + * > + * Returns 0 on success, otherwise returns error code from SPMI driver. > + */ > +static int > +spmi_write_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > +{ > + int ret = 0; > + int len; > + uint16_t addr; > + > + while (cnt > 0) { > + addr = offset & 0xFFFF; > + len = min(cnt, MAX_REG_PER_TRANSACTION); > + > + ret = spmi_ext_register_writel(sdev, addr, buf, len); > + if (ret < 0) { > + pr_err("SPMI write failed, err = %d\n", ret); Same, dev_err() please. > +static ssize_t spmi_device_dfs_reg_write(struct file *file, > + const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + int bytes_read; > + int data; > + int pos = 0; > + int cnt = 0; > + u8 *values; > + size_t ret = 0; > + > + struct spmi_trans *trans = file->private_data; > + u32 offset = trans->offset; > + > + /* Make a copy of the user data */ > + char *kbuf = kmalloc(count + 1, GFP_KERNEL); > + if (!kbuf) > + return -ENOMEM; > + > + ret = copy_from_user(kbuf, buf, count); > + if (ret == count) { > + pr_err("failed to copy data from user\n"); No need for a message here at all, you will get a message in the function if something happened wrong. Also, shouldn't it just be a simple: if (copy_from_user()) { test? > + ret = -EFAULT; > + goto free_buf; > + } > + > + count -= ret; > + *ppos += count; > + kbuf[count] = '\0'; > + > + /* Override the text buffer with the raw data */ > + values = kbuf; > + > + /* Parse the data in the buffer. It should be a string of numbers */ > + while (sscanf(kbuf + pos, "%i%n", &data, &bytes_read) == 1) { > + pos += bytes_read; > + values[cnt++] = data & 0xff; > + } > + > + if (!cnt) > + goto free_buf; > + > + /* Perform the SPMI write(s) */ > + ret = spmi_write_data(trans->sdev, values, offset, cnt); > + > + if (ret) { > + pr_err("SPMI write failed, err = %zu\n", ret); dev_err() please. > +static ssize_t spmi_device_dfs_reg_read(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct spmi_trans *trans = file->private_data; > + struct spmi_log_buffer *log = trans->log; > + size_t ret; > + size_t len; > + > + /* Is the the log buffer empty */ > + if (log->rpos >= log->wpos) { > + if (get_log_data(trans) <= 0) > + return 0; > + } > + > + len = min(count, (size_t) log->wpos - log->rpos); > + > + ret = copy_to_user(buf, &log->data[log->rpos], len); > + if (ret == len) { > + pr_err("error copy SPMI register values to user\n"); Same comments as above. > +void __exit spmi_dfs_exit(void) > +{ > + pr_debug("de-initializing spmi debugfs ..."); Not needed, use the in-kernel trace functionality if you really want to know this. > + debugfs_remove_recursive(spmi_debug_root); > +} > + > +void __init spmi_dfs_init(void) > +{ > + struct dentry *help; > + > + pr_debug("creating SPMI debugfs file-system\n"); Same as above, not needed. So, with those changes, pr_fmt() isn't needed at all :) > --- /dev/null > +++ b/drivers/spmi/spmi-dbgfs.h > @@ -0,0 +1,37 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef _SPMI_DBGFS_H > +#define _SPMI_DBGFS_H > + > +#include <linux/spmi.h> > +#include <linux/debugfs.h> > + > +#ifdef CONFIG_DEBUG_FS Why? If debugfs isn't enabled, the functions should just compile away with the debugfs_() calls, so no need to do this type of thing here, right? > + > +extern void __init spmi_dfs_init(void); > +extern void __exit spmi_dfs_exit(void); > +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); > +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); > +extern void spmi_dfs_add_device(struct spmi_device *sdev); > +extern void spmi_dfs_del_device(struct spmi_device *sdev); > + > +#else > + > +static inline void __init spmi_dfs_init(void) { } > +static inline void __exit spmi_dfs_exit(void) { } > +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } > +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } > +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } > +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } > +#endif > + > +#endif /* _SPMI_DBGFS_H */ > diff --git a/drivers/spmi/spmi.c b/drivers/spmi/spmi.c > new file mode 100644 > index 0000000..3bbcc63 > --- /dev/null > +++ b/drivers/spmi/spmi.c > @@ -0,0 +1,449 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ Again, not needed for this file, because: > +int spmi_add_controller(struct spmi_controller *ctrl) > +{ > + int id; > + > + pr_debug("adding controller for bus %d (0x%p)\n", ctrl->nr, ctrl); dev_dbg() if you really need/want it. > + > + if (((int)ctrl->nr) < 0) { > + pr_err("invalid bus identifier %d\n", ctrl->nr); dev_err() please. > + return -EINVAL; > + } > + > + idr_preload(GFP_KERNEL); > + mutex_lock(&board_lock); > + id = idr_alloc(&ctrl_idr, ctrl, ctrl->nr, ctrl->nr + 1, GFP_NOWAIT); > + mutex_unlock(&board_lock); > + idr_preload_end(); > + > + if (id < 0) { > + pr_err( > + "idr_alloc(start:%d end:%d):%d at spmi_add_controller()\n", > + ctrl->nr, ctrl->nr, id); dev_err() please. And then the pr_fmt() line can be removed. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 18:46 ` Greg Kroah-Hartman (?) @ 2013-08-16 19:47 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:47 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel Hey Greg- Thanks for the comments. On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > --- /dev/null > > +++ b/drivers/of/of_spmi.c > > @@ -0,0 +1,74 @@ > > +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#define pr_fmt(fmt) "%s: " fmt, __func__ > > As you never make a pr_* call in this file, this line isn't needed. I'll clean these up for v2. > > +static int > > +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > > +{ > > + int ret = 0; > > + int len; > > + uint16_t addr; > > + > > + while (cnt > 0) { > > + addr = offset & 0xFFFF; > > + len = min(cnt, MAX_REG_PER_TRANSACTION); > > + > > + ret = spmi_ext_register_readl(sdev, addr, buf, len); > > + if (ret < 0) { > > + pr_err("SPMI read failed, err = %d\n", ret); > > Should be using dev_err() instead. These too. [..] > > + > > + /* Make a copy of the user data */ > > + char *kbuf = kmalloc(count + 1, GFP_KERNEL); > > + if (!kbuf) > > + return -ENOMEM; > > + > > + ret = copy_from_user(kbuf, buf, count); > > + if (ret == count) { > > + pr_err("failed to copy data from user\n"); > > No need for a message here at all, you will get a message in the > function if something happened wrong. > > Also, shouldn't it just be a simple: > if (copy_from_user()) { > test? Indeed, thanks. [..] > > +void __exit spmi_dfs_exit(void) > > +{ > > + pr_debug("de-initializing spmi debugfs ..."); > > Not needed, use the in-kernel trace functionality if you really want to > know this. Will kill these. > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? Not sure I follow you, but it may be because this is a bit misleading. Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have. The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile: spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > + > > +extern void __init spmi_dfs_init(void); > > +extern void __exit spmi_dfs_exit(void); > > +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_add_device(struct spmi_device *sdev); > > +extern void spmi_dfs_del_device(struct spmi_device *sdev); > > + > > +#else > > + > > +static inline void __init spmi_dfs_init(void) { } > > +static inline void __exit spmi_dfs_exit(void) { } > > +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } > > +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } > > +#endif > > + > > +#endif /* _SPMI_DBGFS_H */ Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:47 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:47 UTC (permalink / raw) To: linux-arm-kernel Hey Greg- Thanks for the comments. On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > --- /dev/null > > +++ b/drivers/of/of_spmi.c > > @@ -0,0 +1,74 @@ > > +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#define pr_fmt(fmt) "%s: " fmt, __func__ > > As you never make a pr_* call in this file, this line isn't needed. I'll clean these up for v2. > > +static int > > +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > > +{ > > + int ret = 0; > > + int len; > > + uint16_t addr; > > + > > + while (cnt > 0) { > > + addr = offset & 0xFFFF; > > + len = min(cnt, MAX_REG_PER_TRANSACTION); > > + > > + ret = spmi_ext_register_readl(sdev, addr, buf, len); > > + if (ret < 0) { > > + pr_err("SPMI read failed, err = %d\n", ret); > > Should be using dev_err() instead. These too. [..] > > + > > + /* Make a copy of the user data */ > > + char *kbuf = kmalloc(count + 1, GFP_KERNEL); > > + if (!kbuf) > > + return -ENOMEM; > > + > > + ret = copy_from_user(kbuf, buf, count); > > + if (ret == count) { > > + pr_err("failed to copy data from user\n"); > > No need for a message here at all, you will get a message in the > function if something happened wrong. > > Also, shouldn't it just be a simple: > if (copy_from_user()) { > test? Indeed, thanks. [..] > > +void __exit spmi_dfs_exit(void) > > +{ > > + pr_debug("de-initializing spmi debugfs ..."); > > Not needed, use the in-kernel trace functionality if you really want to > know this. Will kill these. > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? Not sure I follow you, but it may be because this is a bit misleading. Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have. The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile: spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > + > > +extern void __init spmi_dfs_init(void); > > +extern void __exit spmi_dfs_exit(void); > > +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_add_device(struct spmi_device *sdev); > > +extern void spmi_dfs_del_device(struct spmi_device *sdev); > > + > > +#else > > + > > +static inline void __init spmi_dfs_init(void) { } > > +static inline void __exit spmi_dfs_exit(void) { } > > +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } > > +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } > > +#endif > > + > > +#endif /* _SPMI_DBGFS_H */ Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:47 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:47 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan Hey Greg- Thanks for the comments. On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > --- /dev/null > > +++ b/drivers/of/of_spmi.c > > @@ -0,0 +1,74 @@ > > +/* Copyright (c) 2012,2013 The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#define pr_fmt(fmt) "%s: " fmt, __func__ > > As you never make a pr_* call in this file, this line isn't needed. I'll clean these up for v2. > > +static int > > +spmi_read_data(struct spmi_device *sdev, uint8_t *buf, int offset, int cnt) > > +{ > > + int ret = 0; > > + int len; > > + uint16_t addr; > > + > > + while (cnt > 0) { > > + addr = offset & 0xFFFF; > > + len = min(cnt, MAX_REG_PER_TRANSACTION); > > + > > + ret = spmi_ext_register_readl(sdev, addr, buf, len); > > + if (ret < 0) { > > + pr_err("SPMI read failed, err = %d\n", ret); > > Should be using dev_err() instead. These too. [..] > > + > > + /* Make a copy of the user data */ > > + char *kbuf = kmalloc(count + 1, GFP_KERNEL); > > + if (!kbuf) > > + return -ENOMEM; > > + > > + ret = copy_from_user(kbuf, buf, count); > > + if (ret == count) { > > + pr_err("failed to copy data from user\n"); > > No need for a message here at all, you will get a message in the > function if something happened wrong. > > Also, shouldn't it just be a simple: > if (copy_from_user()) { > test? Indeed, thanks. [..] > > +void __exit spmi_dfs_exit(void) > > +{ > > + pr_debug("de-initializing spmi debugfs ..."); > > Not needed, use the in-kernel trace functionality if you really want to > know this. Will kill these. > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? Not sure I follow you, but it may be because this is a bit misleading. Currently CONFIG_DEBUG_FS is being extended to also mean "do you want the SPMI core to create device entries?". It would probably make more sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as other busses have. The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in the Makefile: spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > + > > +extern void __init spmi_dfs_init(void); > > +extern void __exit spmi_dfs_exit(void); > > +extern void spmi_dfs_add_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_del_controller(struct spmi_controller *ctrl); > > +extern void spmi_dfs_add_device(struct spmi_device *sdev); > > +extern void spmi_dfs_del_device(struct spmi_device *sdev); > > + > > +#else > > + > > +static inline void __init spmi_dfs_init(void) { } > > +static inline void __exit spmi_dfs_exit(void) { } > > +static inline void spmi_dfs_add_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_del_controller(struct spmi_controller *ctrl) { } > > +static inline void spmi_dfs_add_device(struct spmi_device *sdev) { } > > +static inline void spmi_dfs_del_device(struct spmi_device *sdev) { } > > +#endif > > + > > +#endif /* _SPMI_DBGFS_H */ Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 19:47 ` Josh Cartwright (?) @ 2013-08-16 19:50 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 02:47:14PM -0500, Josh Cartwright wrote: > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more debugfs -^ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:50 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:50 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 02:47:14PM -0500, Josh Cartwright wrote: > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more debugfs -^ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:50 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:50 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 02:47:14PM -0500, Josh Cartwright wrote: > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more debugfs -^ -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 19:47 ` Josh Cartwright (?) @ 2013-08-16 19:58 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 19:58 UTC (permalink / raw) To: Josh Cartwright Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > other busses have. > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > the Makefile: > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o If debugfs is enabled why wouldn't you want debugfs entries for your devices? Don't assume a user is going to be able to rebuild their kernel just for debugging stuff (hint, they usually aren't), so having these present, if they don't cause any performance issues, is usually best to always have around. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:58 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 19:58 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > other busses have. > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > the Makefile: > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o If debugfs is enabled why wouldn't you want debugfs entries for your devices? Don't assume a user is going to be able to rebuild their kernel just for debugging stuff (hint, they usually aren't), so having these present, if they don't cause any performance issues, is usually best to always have around. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:58 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 19:58 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > Not sure I follow you, but it may be because this is a bit misleading. > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > the SPMI core to create device entries?". It would probably make more > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > other busses have. > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > the Makefile: > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o If debugfs is enabled why wouldn't you want debugfs entries for your devices? Don't assume a user is going to be able to rebuild their kernel just for debugging stuff (hint, they usually aren't), so having these present, if they don't cause any performance issues, is usually best to always have around. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 19:58 ` Greg Kroah-Hartman (?) @ 2013-08-16 20:40 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:40 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > with the debugfs_() calls, so no need to do this type of thing here, > > > right? > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > the SPMI core to create device entries?". It would probably make more > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > other busses have. > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > the Makefile: > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > If debugfs is enabled why wouldn't you want debugfs entries for your > devices? Don't assume a user is going to be able to rebuild their > kernel just for debugging stuff (hint, they usually aren't), so having > these present, if they don't cause any performance issues, is usually > best to always have around. Okay, that makes sense. So, backing up a step, you're original comment was regarding the CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? The reason why this is done is because the spmi debugfs support code is is only built-in when CONFIG_DEBUG_FS is set. Would you rather it always be built-in (well, whenever SPMI support is included), and rely on the debugfs_* shims to handle the !CONFIG_DEBUG_FS case? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:40 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:40 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > with the debugfs_() calls, so no need to do this type of thing here, > > > right? > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > the SPMI core to create device entries?". It would probably make more > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > other busses have. > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > the Makefile: > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > If debugfs is enabled why wouldn't you want debugfs entries for your > devices? Don't assume a user is going to be able to rebuild their > kernel just for debugging stuff (hint, they usually aren't), so having > these present, if they don't cause any performance issues, is usually > best to always have around. Okay, that makes sense. So, backing up a step, you're original comment was regarding the CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? The reason why this is done is because the spmi debugfs support code is is only built-in when CONFIG_DEBUG_FS is set. Would you rather it always be built-in (well, whenever SPMI support is included), and rely on the debugfs_* shims to handle the !CONFIG_DEBUG_FS case? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:40 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:40 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > with the debugfs_() calls, so no need to do this type of thing here, > > > right? > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > the SPMI core to create device entries?". It would probably make more > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > other busses have. > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > the Makefile: > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > If debugfs is enabled why wouldn't you want debugfs entries for your > devices? Don't assume a user is going to be able to rebuild their > kernel just for debugging stuff (hint, they usually aren't), so having > these present, if they don't cause any performance issues, is usually > best to always have around. Okay, that makes sense. So, backing up a step, you're original comment was regarding the CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > --- /dev/null > > +++ b/drivers/spmi/spmi-dbgfs.h > > @@ -0,0 +1,37 @@ > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License version 2 and > > + * only version 2 as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + */ > > +#ifndef _SPMI_DBGFS_H > > +#define _SPMI_DBGFS_H > > + > > +#include <linux/spmi.h> > > +#include <linux/debugfs.h> > > + > > +#ifdef CONFIG_DEBUG_FS > > Why? If debugfs isn't enabled, the functions should just compile away > with the debugfs_() calls, so no need to do this type of thing here, > right? The reason why this is done is because the spmi debugfs support code is is only built-in when CONFIG_DEBUG_FS is set. Would you rather it always be built-in (well, whenever SPMI support is included), and rely on the debugfs_* shims to handle the !CONFIG_DEBUG_FS case? -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 20:40 ` Josh Cartwright (?) @ 2013-08-16 20:50 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:50 UTC (permalink / raw) To: Josh Cartwright Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 03:40:55PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > > with the debugfs_() calls, so no need to do this type of thing here, > > > > right? > > > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > > the SPMI core to create device entries?". It would probably make more > > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > > other busses have. > > > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > > the Makefile: > > > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > > > If debugfs is enabled why wouldn't you want debugfs entries for your > > devices? Don't assume a user is going to be able to rebuild their > > kernel just for debugging stuff (hint, they usually aren't), so having > > these present, if they don't cause any performance issues, is usually > > best to always have around. > > Okay, that makes sense. > > So, backing up a step, you're original comment was regarding the > CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: > > On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > > --- /dev/null > > > +++ b/drivers/spmi/spmi-dbgfs.h > > > @@ -0,0 +1,37 @@ > > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 and > > > + * only version 2 as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > +#ifndef _SPMI_DBGFS_H > > > +#define _SPMI_DBGFS_H > > > + > > > +#include <linux/spmi.h> > > > +#include <linux/debugfs.h> > > > + > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > The reason why this is done is because the spmi debugfs support code is > is only built-in when CONFIG_DEBUG_FS is set. > > Would you rather it always be built-in (well, whenever SPMI support is > included), and rely on the debugfs_* shims to handle the > !CONFIG_DEBUG_FS case? Yes, that makes the logic in your driver simpler, and easier to review. The compiler will just compile away almost all of the logic if debugfs isn't present, so there shouldn't be any big size difference. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:50 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:50 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 03:40:55PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > > with the debugfs_() calls, so no need to do this type of thing here, > > > > right? > > > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > > the SPMI core to create device entries?". It would probably make more > > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > > other busses have. > > > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > > the Makefile: > > > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > > > If debugfs is enabled why wouldn't you want debugfs entries for your > > devices? Don't assume a user is going to be able to rebuild their > > kernel just for debugging stuff (hint, they usually aren't), so having > > these present, if they don't cause any performance issues, is usually > > best to always have around. > > Okay, that makes sense. > > So, backing up a step, you're original comment was regarding the > CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: > > On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > > --- /dev/null > > > +++ b/drivers/spmi/spmi-dbgfs.h > > > @@ -0,0 +1,37 @@ > > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 and > > > + * only version 2 as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > +#ifndef _SPMI_DBGFS_H > > > +#define _SPMI_DBGFS_H > > > + > > > +#include <linux/spmi.h> > > > +#include <linux/debugfs.h> > > > + > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > The reason why this is done is because the spmi debugfs support code is > is only built-in when CONFIG_DEBUG_FS is set. > > Would you rather it always be built-in (well, whenever SPMI support is > included), and rely on the debugfs_* shims to handle the > !CONFIG_DEBUG_FS case? Yes, that makes the logic in your driver simpler, and easier to review. The compiler will just compile away almost all of the logic if debugfs isn't present, so there shouldn't be any big size difference. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:50 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:50 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 03:40:55PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 12:58:49PM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 16, 2013 at 02:47:15PM -0500, Josh Cartwright wrote: > > > > > +#ifdef CONFIG_DEBUG_FS > > > > > > > > Why? If debugfs isn't enabled, the functions should just compile away > > > > with the debugfs_() calls, so no need to do this type of thing here, > > > > right? > > > > > > Not sure I follow you, but it may be because this is a bit misleading. > > > > > > Currently CONFIG_DEBUG_FS is being extended to also mean "do you want > > > the SPMI core to create device entries?". It would probably make more > > > sense to have a CONFIG_SPMI_DEBUG option which is def_bool DEBUG_FS, as > > > other busses have. > > > > > > The #ifdef here would then be #ifdef CONFIG_SPMI_DEBUG, as well as in > > > the Makefile: > > > > > > spmi-core-$(CONFIG_SPMI_DEBUG) += spmi-dbgfs.o > > > > If debugfs is enabled why wouldn't you want debugfs entries for your > > devices? Don't assume a user is going to be able to rebuild their > > kernel just for debugging stuff (hint, they usually aren't), so having > > these present, if they don't cause any performance issues, is usually > > best to always have around. > > Okay, that makes sense. > > So, backing up a step, you're original comment was regarding the > CONFIG_DEBUG_FS conditional in spmi-dbgfs.h: > > On Fri, Aug 16, 2013 at 11:46:14AM -0700, Greg Kroah-Hartman wrote: > > > --- /dev/null > > > +++ b/drivers/spmi/spmi-dbgfs.h > > > @@ -0,0 +1,37 @@ > > > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > > > + * > > > + * This program is free software; you can redistribute it and/or modify > > > + * it under the terms of the GNU General Public License version 2 and > > > + * only version 2 as published by the Free Software Foundation. > > > + * > > > + * This program is distributed in the hope that it will be useful, > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > > + * GNU General Public License for more details. > > > + */ > > > +#ifndef _SPMI_DBGFS_H > > > +#define _SPMI_DBGFS_H > > > + > > > +#include <linux/spmi.h> > > > +#include <linux/debugfs.h> > > > + > > > +#ifdef CONFIG_DEBUG_FS > > > > Why? If debugfs isn't enabled, the functions should just compile away > > with the debugfs_() calls, so no need to do this type of thing here, > > right? > > The reason why this is done is because the spmi debugfs support code is > is only built-in when CONFIG_DEBUG_FS is set. > > Would you rather it always be built-in (well, whenever SPMI support is > included), and rely on the debugfs_* shims to handle the > !CONFIG_DEBUG_FS case? Yes, that makes the logic in your driver simpler, and easier to review. The compiler will just compile away almost all of the logic if debugfs isn't present, so there shouldn't be any big size difference. thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-09 20:37 ` Josh Cartwright @ 2013-08-16 18:49 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:49 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > +++ b/drivers/spmi/spmi.c > @@ -0,0 +1,449 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ > + > +#include <linux/kernel.h> > +#include <linux/errno.h> > +#include <linux/idr.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/spmi.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > + > +#include "spmi-dbgfs.h" > + > +static DEFINE_MUTEX(board_lock); > +static DEFINE_IDR(ctrl_idr); > + > +static void spmi_dev_release(struct device *dev) > +{ > + struct spmi_device *spmidev = to_spmi_device(dev); > + kfree(spmidev); > +} > + > +static struct device_type spmi_dev_type = { > + .release = spmi_dev_release, > +}; I give a lot of people crap when they get things wrong with the driver model, so it's only fair to call out when it's done correctly. Nice job here, thanks for getting this right. > +static void spmi_ctrl_release(struct device *dev) > +{ > + struct spmi_controller *ctrl = to_spmi_controller(dev); > + complete(&ctrl->dev_released); When is this memory going to be freed? Ah, you think it will be when you remove the device later on: > +int spmi_del_controller(struct spmi_controller *ctrl) > +{ > + struct spmi_controller *found; > + > + if (!ctrl) > + return -EINVAL; > + > + /* Check that the ctrl has been added */ > + mutex_lock(&board_lock); > + found = idr_find(&ctrl_idr, ctrl->nr); > + mutex_unlock(&board_lock); > + > + if (found != ctrl) > + return -EINVAL; > + > + spmi_dfs_del_controller(ctrl); > + > + /* Remove all the clients associated with this controller */ > + mutex_lock(&board_lock); > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > + idr_remove(&ctrl_idr, ctrl->nr); > + mutex_unlock(&board_lock); > + > + init_completion(&ctrl->dev_released); > + device_unregister(&ctrl->dev); > + wait_for_completion(&ctrl->dev_released); But you just leaked memory, right? You should never have to wait for this to happen, why did you need to add this? Why not just a simple call to kfree() in the release function? thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 18:49 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 18:49 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > +++ b/drivers/spmi/spmi.c > @@ -0,0 +1,449 @@ > +/* Copyright (c) 2012-2013, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#define pr_fmt(fmt) "%s: " fmt, __func__ > + > +#include <linux/kernel.h> > +#include <linux/errno.h> > +#include <linux/idr.h> > +#include <linux/slab.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/platform_device.h> > +#include <linux/spmi.h> > +#include <linux/module.h> > +#include <linux/pm_runtime.h> > + > +#include "spmi-dbgfs.h" > + > +static DEFINE_MUTEX(board_lock); > +static DEFINE_IDR(ctrl_idr); > + > +static void spmi_dev_release(struct device *dev) > +{ > + struct spmi_device *spmidev = to_spmi_device(dev); > + kfree(spmidev); > +} > + > +static struct device_type spmi_dev_type = { > + .release = spmi_dev_release, > +}; I give a lot of people crap when they get things wrong with the driver model, so it's only fair to call out when it's done correctly. Nice job here, thanks for getting this right. > +static void spmi_ctrl_release(struct device *dev) > +{ > + struct spmi_controller *ctrl = to_spmi_controller(dev); > + complete(&ctrl->dev_released); When is this memory going to be freed? Ah, you think it will be when you remove the device later on: > +int spmi_del_controller(struct spmi_controller *ctrl) > +{ > + struct spmi_controller *found; > + > + if (!ctrl) > + return -EINVAL; > + > + /* Check that the ctrl has been added */ > + mutex_lock(&board_lock); > + found = idr_find(&ctrl_idr, ctrl->nr); > + mutex_unlock(&board_lock); > + > + if (found != ctrl) > + return -EINVAL; > + > + spmi_dfs_del_controller(ctrl); > + > + /* Remove all the clients associated with this controller */ > + mutex_lock(&board_lock); > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > + idr_remove(&ctrl_idr, ctrl->nr); > + mutex_unlock(&board_lock); > + > + init_completion(&ctrl->dev_released); > + device_unregister(&ctrl->dev); > + wait_for_completion(&ctrl->dev_released); But you just leaked memory, right? You should never have to wait for this to happen, why did you need to add this? Why not just a simple call to kfree() in the release function? thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 18:49 ` Greg Kroah-Hartman (?) @ 2013-08-16 20:21 ` Josh Cartwright -1 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:21 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > +++ b/drivers/spmi/spmi.c > > @@ -0,0 +1,449 @@ [..] > > +static void spmi_ctrl_release(struct device *dev) > > +{ > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > + complete(&ctrl->dev_released); > > When is this memory going to be freed? > > Ah, you think it will be when you remove the device later on: > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > +{ > > + struct spmi_controller *found; > > + > > + if (!ctrl) > > + return -EINVAL; > > + > > + /* Check that the ctrl has been added */ > > + mutex_lock(&board_lock); > > + found = idr_find(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + if (found != ctrl) > > + return -EINVAL; > > + > > + spmi_dfs_del_controller(ctrl); > > + > > + /* Remove all the clients associated with this controller */ > > + mutex_lock(&board_lock); > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > + idr_remove(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + init_completion(&ctrl->dev_released); > > + device_unregister(&ctrl->dev); > > + wait_for_completion(&ctrl->dev_released); > > But you just leaked memory, right? > > You should never have to wait for this to happen, why did you need to > add this? Why not just a simple call to kfree() in the release > function? Unfortunately, the reason why this was necessary may be lost to history. :( I'll do some testing with the completion removed and a simple kfree() in the release and see if there is any fallout. Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:21 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:21 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > +++ b/drivers/spmi/spmi.c > > @@ -0,0 +1,449 @@ [..] > > +static void spmi_ctrl_release(struct device *dev) > > +{ > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > + complete(&ctrl->dev_released); > > When is this memory going to be freed? > > Ah, you think it will be when you remove the device later on: > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > +{ > > + struct spmi_controller *found; > > + > > + if (!ctrl) > > + return -EINVAL; > > + > > + /* Check that the ctrl has been added */ > > + mutex_lock(&board_lock); > > + found = idr_find(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + if (found != ctrl) > > + return -EINVAL; > > + > > + spmi_dfs_del_controller(ctrl); > > + > > + /* Remove all the clients associated with this controller */ > > + mutex_lock(&board_lock); > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > + idr_remove(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + init_completion(&ctrl->dev_released); > > + device_unregister(&ctrl->dev); > > + wait_for_completion(&ctrl->dev_released); > > But you just leaked memory, right? > > You should never have to wait for this to happen, why did you need to > add this? Why not just a simple call to kfree() in the release > function? Unfortunately, the reason why this was necessary may be lost to history. :( I'll do some testing with the completion removed and a simple kfree() in the release and see if there is any fallout. Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:21 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 20:21 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > +++ b/drivers/spmi/spmi.c > > @@ -0,0 +1,449 @@ [..] > > +static void spmi_ctrl_release(struct device *dev) > > +{ > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > + complete(&ctrl->dev_released); > > When is this memory going to be freed? > > Ah, you think it will be when you remove the device later on: > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > +{ > > + struct spmi_controller *found; > > + > > + if (!ctrl) > > + return -EINVAL; > > + > > + /* Check that the ctrl has been added */ > > + mutex_lock(&board_lock); > > + found = idr_find(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + if (found != ctrl) > > + return -EINVAL; > > + > > + spmi_dfs_del_controller(ctrl); > > + > > + /* Remove all the clients associated with this controller */ > > + mutex_lock(&board_lock); > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > + idr_remove(&ctrl_idr, ctrl->nr); > > + mutex_unlock(&board_lock); > > + > > + init_completion(&ctrl->dev_released); > > + device_unregister(&ctrl->dev); > > + wait_for_completion(&ctrl->dev_released); > > But you just leaked memory, right? > > You should never have to wait for this to happen, why did you need to > add this? Why not just a simple call to kfree() in the release > function? Unfortunately, the reason why this was necessary may be lost to history. :( I'll do some testing with the completion removed and a simple kfree() in the release and see if there is any fallout. Thanks, Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-16 20:21 ` Josh Cartwright (?) @ 2013-08-16 20:28 ` Greg Kroah-Hartman -1 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:28 UTC (permalink / raw) To: Josh Cartwright Cc: linux-arm-msm, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, Grant Likely, Sagar Dharia, linux-arm-kernel On Fri, Aug 16, 2013 at 03:21:10PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > > +++ b/drivers/spmi/spmi.c > > > @@ -0,0 +1,449 @@ > [..] > > > +static void spmi_ctrl_release(struct device *dev) > > > +{ > > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > > + complete(&ctrl->dev_released); > > > > When is this memory going to be freed? > > > > Ah, you think it will be when you remove the device later on: > > > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > > +{ > > > + struct spmi_controller *found; > > > + > > > + if (!ctrl) > > > + return -EINVAL; > > > + > > > + /* Check that the ctrl has been added */ > > > + mutex_lock(&board_lock); > > > + found = idr_find(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + if (found != ctrl) > > > + return -EINVAL; > > > + > > > + spmi_dfs_del_controller(ctrl); > > > + > > > + /* Remove all the clients associated with this controller */ > > > + mutex_lock(&board_lock); > > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > > + idr_remove(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + init_completion(&ctrl->dev_released); > > > + device_unregister(&ctrl->dev); > > > + wait_for_completion(&ctrl->dev_released); > > > > But you just leaked memory, right? > > > > You should never have to wait for this to happen, why did you need to > > add this? Why not just a simple call to kfree() in the release > > function? > > Unfortunately, the reason why this was necessary may be lost to history. :( > > I'll do some testing with the completion removed and a simple kfree() in > the release and see if there is any fallout. There will be, you need to fix up the calling logic and the driver to prevent memory that it thinks it still has access to, from going away (that was my hint for the other patch you sent.) thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:28 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:28 UTC (permalink / raw) To: linux-arm-kernel On Fri, Aug 16, 2013 at 03:21:10PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > > +++ b/drivers/spmi/spmi.c > > > @@ -0,0 +1,449 @@ > [..] > > > +static void spmi_ctrl_release(struct device *dev) > > > +{ > > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > > + complete(&ctrl->dev_released); > > > > When is this memory going to be freed? > > > > Ah, you think it will be when you remove the device later on: > > > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > > +{ > > > + struct spmi_controller *found; > > > + > > > + if (!ctrl) > > > + return -EINVAL; > > > + > > > + /* Check that the ctrl has been added */ > > > + mutex_lock(&board_lock); > > > + found = idr_find(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + if (found != ctrl) > > > + return -EINVAL; > > > + > > > + spmi_dfs_del_controller(ctrl); > > > + > > > + /* Remove all the clients associated with this controller */ > > > + mutex_lock(&board_lock); > > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > > + idr_remove(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + init_completion(&ctrl->dev_released); > > > + device_unregister(&ctrl->dev); > > > + wait_for_completion(&ctrl->dev_released); > > > > But you just leaked memory, right? > > > > You should never have to wait for this to happen, why did you need to > > add this? Why not just a simple call to kfree() in the release > > function? > > Unfortunately, the reason why this was necessary may be lost to history. :( > > I'll do some testing with the completion removed and a simple kfree() in > the release and see if there is any fallout. There will be, you need to fix up the calling logic and the driver to prevent memory that it thinks it still has access to, from going away (that was my hint for the other patch you sent.) thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 20:28 ` Greg Kroah-Hartman 0 siblings, 0 replies; 45+ messages in thread From: Greg Kroah-Hartman @ 2013-08-16 20:28 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan On Fri, Aug 16, 2013 at 03:21:10PM -0500, Josh Cartwright wrote: > On Fri, Aug 16, 2013 at 11:49:21AM -0700, Greg Kroah-Hartman wrote: > > On Fri, Aug 09, 2013 at 01:37:09PM -0700, Josh Cartwright wrote: > > > +++ b/drivers/spmi/spmi.c > > > @@ -0,0 +1,449 @@ > [..] > > > +static void spmi_ctrl_release(struct device *dev) > > > +{ > > > + struct spmi_controller *ctrl = to_spmi_controller(dev); > > > + complete(&ctrl->dev_released); > > > > When is this memory going to be freed? > > > > Ah, you think it will be when you remove the device later on: > > > > > +int spmi_del_controller(struct spmi_controller *ctrl) > > > +{ > > > + struct spmi_controller *found; > > > + > > > + if (!ctrl) > > > + return -EINVAL; > > > + > > > + /* Check that the ctrl has been added */ > > > + mutex_lock(&board_lock); > > > + found = idr_find(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + if (found != ctrl) > > > + return -EINVAL; > > > + > > > + spmi_dfs_del_controller(ctrl); > > > + > > > + /* Remove all the clients associated with this controller */ > > > + mutex_lock(&board_lock); > > > + bus_for_each_dev(&spmi_bus_type, NULL, ctrl, spmi_ctrl_remove_device); > > > + idr_remove(&ctrl_idr, ctrl->nr); > > > + mutex_unlock(&board_lock); > > > + > > > + init_completion(&ctrl->dev_released); > > > + device_unregister(&ctrl->dev); > > > + wait_for_completion(&ctrl->dev_released); > > > > But you just leaked memory, right? > > > > You should never have to wait for this to happen, why did you need to > > add this? Why not just a simple call to kfree() in the release > > function? > > Unfortunately, the reason why this was necessary may be lost to history. :( > > I'll do some testing with the completion removed and a simple kfree() in > the release and see if there is any fallout. There will be, you need to fix up the calling logic and the driver to prevent memory that it thinks it still has access to, from going away (that was my hint for the other patch you sent.) thanks, greg k-h ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI 2013-08-09 20:37 ` Josh Cartwright (?) @ 2013-08-16 19:04 ` Kumar Gala -1 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:04 UTC (permalink / raw) To: Josh Cartwright Cc: devicetree, Greg Kroah-Hartman, Gilad Avidov, linux-kernel@vger.kernel.org list, Rob Herring, Michael Bohan, linux-arm-msm, Grant Likely, Sagar Dharia, linux-arm-kernel On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > From: Kenneth Heitke <kheitke@codeaurora.org> > > 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 <kheitke@codeaurora.org> > Signed-off-by: Michael Bohan <mbohan@codeaurora.org> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > --- > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/of/Kconfig | 6 + > drivers/of/Makefile | 1 + > drivers/of/of_spmi.c | 74 +++++ > drivers/spmi/Kconfig | 9 + > drivers/spmi/Makefile | 7 + > drivers/spmi/spmi-dbgfs.c | 591 ++++++++++++++++++++++++++++++++++++++++ > drivers/spmi/spmi-dbgfs.h | 37 +++ > drivers/spmi/spmi.c | 449 ++++++++++++++++++++++++++++++ > include/linux/mod_devicetable.h | 8 + > include/linux/of_spmi.h | 37 +++ > include/linux/spmi.h | 337 +++++++++++++++++++++++ > 13 files changed, 1559 insertions(+) > create mode 100644 drivers/of/of_spmi.c > create mode 100644 drivers/spmi/Kconfig > create mode 100644 drivers/spmi/Makefile > create mode 100644 drivers/spmi/spmi-dbgfs.c > create mode 100644 drivers/spmi/spmi-dbgfs.h > create mode 100644 drivers/spmi/spmi.c > create mode 100644 include/linux/of_spmi.h > create mode 100644 include/linux/spmi.h Looks like you are missing a patch for the general OF binding for SPMI - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:04 ` Kumar Gala 0 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:04 UTC (permalink / raw) To: linux-arm-kernel On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > From: Kenneth Heitke <kheitke@codeaurora.org> > > 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 <kheitke@codeaurora.org> > Signed-off-by: Michael Bohan <mbohan@codeaurora.org> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > --- > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/of/Kconfig | 6 + > drivers/of/Makefile | 1 + > drivers/of/of_spmi.c | 74 +++++ > drivers/spmi/Kconfig | 9 + > drivers/spmi/Makefile | 7 + > drivers/spmi/spmi-dbgfs.c | 591 ++++++++++++++++++++++++++++++++++++++++ > drivers/spmi/spmi-dbgfs.h | 37 +++ > drivers/spmi/spmi.c | 449 ++++++++++++++++++++++++++++++ > include/linux/mod_devicetable.h | 8 + > include/linux/of_spmi.h | 37 +++ > include/linux/spmi.h | 337 +++++++++++++++++++++++ > 13 files changed, 1559 insertions(+) > create mode 100644 drivers/of/of_spmi.c > create mode 100644 drivers/spmi/Kconfig > create mode 100644 drivers/spmi/Makefile > create mode 100644 drivers/spmi/spmi-dbgfs.c > create mode 100644 drivers/spmi/spmi-dbgfs.h > create mode 100644 drivers/spmi/spmi.c > create mode 100644 include/linux/of_spmi.h > create mode 100644 include/linux/spmi.h Looks like you are missing a patch for the general OF binding for SPMI - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 1/3] spmi: Linux driver framework for SPMI @ 2013-08-16 19:04 ` Kumar Gala 0 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:04 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, Greg Kroah-Hartman, linux-arm-msm, Gilad Avidov, linux-kernel@vger.kernel.org list, Michael Bohan, Sagar Dharia, linux-arm-kernel, devicetree On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > From: Kenneth Heitke <kheitke@codeaurora.org> > > 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 <kheitke@codeaurora.org> > Signed-off-by: Michael Bohan <mbohan@codeaurora.org> > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > --- > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/of/Kconfig | 6 + > drivers/of/Makefile | 1 + > drivers/of/of_spmi.c | 74 +++++ > drivers/spmi/Kconfig | 9 + > drivers/spmi/Makefile | 7 + > drivers/spmi/spmi-dbgfs.c | 591 ++++++++++++++++++++++++++++++++++++++++ > drivers/spmi/spmi-dbgfs.h | 37 +++ > drivers/spmi/spmi.c | 449 ++++++++++++++++++++++++++++++ > include/linux/mod_devicetable.h | 8 + > include/linux/of_spmi.h | 37 +++ > include/linux/spmi.h | 337 +++++++++++++++++++++++ > 13 files changed, 1559 insertions(+) > create mode 100644 drivers/of/of_spmi.c > create mode 100644 drivers/spmi/Kconfig > create mode 100644 drivers/spmi/Makefile > create mode 100644 drivers/spmi/spmi-dbgfs.c > create mode 100644 drivers/spmi/spmi-dbgfs.h > create mode 100644 drivers/spmi/spmi.c > create mode 100644 include/linux/of_spmi.h > create mode 100644 include/linux/spmi.h Looks like you are missing a patch for the general OF binding for SPMI - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <b639088d50df93caaef8fe7e09c12953b1153ce8.1376596224.git.joshc@codeaurora.org>]
[parent not found: <D1534646-7CB5-4EE7-8C1E-1C607BE22396@codeaurora.org>]
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings [not found] ` <D1534646-7CB5-4EE7-8C1E-1C607BE22396@codeaurora.org> 2013-08-16 19:25 ` Josh Cartwright @ 2013-08-16 19:25 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:25 UTC (permalink / raw) To: Kumar Gala Cc: Mark Rutland, devicetree, Ian Campbell, Pawel Moll, Stephen Warren, Greg Kroah-Hartman, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, linux-arm-msm, Grant Likely, Sagar Dharia, linux-arm-kernel Hey Kumar- Thanks for the review. On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: > On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > > --- > > .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > diff --git a/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > new file mode 100644 > > index 0000000..1c14bf4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > @@ -0,0 +1,26 @@ > > +Qualcomm SPMI Controller (PMIC Arbiter) > > We should get in the habit of trying to have at least one sentence about what the device is or does. Certainly. > > + > > +Required properties: > > +- compatible : should be "qcom,spmi-pmic-arb". > > +- reg-names : should be "core", "intr", "cnfg" > > +- reg : offset and length of the PMIC Arbiter Core register map. > > +- reg : offset and length of the PMIC Arbiter Interrupt controller register map. > > +- reg : offset and length of the PMIC Arbiter Configuration register map. > > +- #address-cells : must be set to 1 > > +- #size-cells : must be set to 0 > > Hmm, you aren't describe anything about child nodes. Indeed. I'll put together a generic SPMI binding document as you had suggested before and reference it in this binding. Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 19:25 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:25 UTC (permalink / raw) To: linux-arm-kernel Hey Kumar- Thanks for the review. On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: > On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > > --- > > .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > diff --git a/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > new file mode 100644 > > index 0000000..1c14bf4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > @@ -0,0 +1,26 @@ > > +Qualcomm SPMI Controller (PMIC Arbiter) > > We should get in the habit of trying to have at least one sentence about what the device is or does. Certainly. > > + > > +Required properties: > > +- compatible : should be "qcom,spmi-pmic-arb". > > +- reg-names : should be "core", "intr", "cnfg" > > +- reg : offset and length of the PMIC Arbiter Core register map. > > +- reg : offset and length of the PMIC Arbiter Interrupt controller register map. > > +- reg : offset and length of the PMIC Arbiter Configuration register map. > > +- #address-cells : must be set to 1 > > +- #size-cells : must be set to 0 > > Hmm, you aren't describe anything about child nodes. Indeed. I'll put together a generic SPMI binding document as you had suggested before and reference it in this binding. Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 19:25 ` Josh Cartwright 0 siblings, 0 replies; 45+ messages in thread From: Josh Cartwright @ 2013-08-16 19:25 UTC (permalink / raw) To: Kumar Gala Cc: Grant Likely, Rob Herring, Greg Kroah-Hartman, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan, devicetree Hey Kumar- Thanks for the review. On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: > On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: > > > Signed-off-by: Josh Cartwright <joshc@codeaurora.org> > > --- > > .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > diff --git a/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > new file mode 100644 > > index 0000000..1c14bf4 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt > > @@ -0,0 +1,26 @@ > > +Qualcomm SPMI Controller (PMIC Arbiter) > > We should get in the habit of trying to have at least one sentence about what the device is or does. Certainly. > > + > > +Required properties: > > +- compatible : should be "qcom,spmi-pmic-arb". > > +- reg-names : should be "core", "intr", "cnfg" > > +- reg : offset and length of the PMIC Arbiter Core register map. > > +- reg : offset and length of the PMIC Arbiter Interrupt controller register map. > > +- reg : offset and length of the PMIC Arbiter Configuration register map. > > +- #address-cells : must be set to 1 > > +- #size-cells : must be set to 0 > > Hmm, you aren't describe anything about child nodes. Indeed. I'll put together a generic SPMI binding document as you had suggested before and reference it in this binding. Josh -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings 2013-08-16 19:25 ` Josh Cartwright (?) @ 2013-08-16 19:48 ` Kumar Gala -1 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:48 UTC (permalink / raw) To: Josh Cartwright Cc: Mark Rutland, devicetree, Ian Campbell, Pawel Moll, Stephen Warren, Greg Kroah-Hartman, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, linux-arm-msm, Grant Likely, Sagar Dharia, linux-arm-kernel On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > Hey Kumar- > > Thanks for the review. > > On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >> >>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>> --- >>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >> >> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. > > Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? Sounds good, as I see we have other examples of having a comma in file names for bindings. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 19:48 ` Kumar Gala 0 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:48 UTC (permalink / raw) To: linux-arm-kernel On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > Hey Kumar- > > Thanks for the review. > > On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >> >>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>> --- >>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >> >> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. > > Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? Sounds good, as I see we have other examples of having a comma in file names for bindings. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 19:48 ` Kumar Gala 0 siblings, 0 replies; 45+ messages in thread From: Kumar Gala @ 2013-08-16 19:48 UTC (permalink / raw) To: Josh Cartwright Cc: Grant Likely, Rob Herring, Greg Kroah-Hartman, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan, devicetree On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > Hey Kumar- > > Thanks for the review. > > On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >> >>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>> --- >>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>> 1 file changed, 26 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >> >> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. > > Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? Sounds good, as I see we have other examples of having a comma in file names for bindings. - k -- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings 2013-08-16 19:48 ` Kumar Gala (?) @ 2013-08-16 23:17 ` Stephen Warren -1 siblings, 0 replies; 45+ messages in thread From: Stephen Warren @ 2013-08-16 23:17 UTC (permalink / raw) To: Kumar Gala Cc: Mark Rutland, devicetree, Ian Campbell, Pawel Moll, Greg Kroah-Hartman, Josh Cartwright, Gilad Avidov, linux-kernel, Rob Herring, Michael Bohan, linux-arm-msm, Grant Likely, Sagar Dharia, linux-arm-kernel On 08/16/2013 01:48 PM, Kumar Gala wrote: > > On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > >> Hey Kumar- >> >> Thanks for the review. >> >> On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >>> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >>> >>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>>> --- >>>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >>> >>> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. >> >> Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > Sounds good, as I see we have other examples of having a comma in file names for bindings. The rule I've applied for bindings I created is use the complete compatible value for the file name. Where multiple are supported (e.g. SoC versions), just use the first one. Then, the filename is pretty unambiguous, both from a point of view of `ls` and from choosing a name. ^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 23:17 ` Stephen Warren 0 siblings, 0 replies; 45+ messages in thread From: Stephen Warren @ 2013-08-16 23:17 UTC (permalink / raw) To: linux-arm-kernel On 08/16/2013 01:48 PM, Kumar Gala wrote: > > On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > >> Hey Kumar- >> >> Thanks for the review. >> >> On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >>> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >>> >>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>>> --- >>>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >>> >>> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. >> >> Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > Sounds good, as I see we have other examples of having a comma in file names for bindings. The rule I've applied for bindings I created is use the complete compatible value for the file name. Where multiple are supported (e.g. SoC versions), just use the first one. Then, the filename is pretty unambiguous, both from a point of view of `ls` and from choosing a name. ^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings @ 2013-08-16 23:17 ` Stephen Warren 0 siblings, 0 replies; 45+ messages in thread From: Stephen Warren @ 2013-08-16 23:17 UTC (permalink / raw) To: Kumar Gala Cc: Josh Cartwright, Grant Likely, Rob Herring, Greg Kroah-Hartman, Pawel Moll, Mark Rutland, Ian Campbell, linux-kernel, linux-arm-kernel, linux-arm-msm, Sagar Dharia, Gilad Avidov, Michael Bohan, devicetree On 08/16/2013 01:48 PM, Kumar Gala wrote: > > On Aug 16, 2013, at 2:25 PM, Josh Cartwright wrote: > >> Hey Kumar- >> >> Thanks for the review. >> >> On Fri, Aug 16, 2013 at 01:53:27PM -0500, Kumar Gala wrote: >>> On Aug 9, 2013, at 3:37 PM, Josh Cartwright wrote: >>> >>>> Signed-off-by: Josh Cartwright <joshc@codeaurora.org> >>>> --- >>>> .../devicetree/bindings/spmi/spmi-pmic-arb.txt | 26 ++++++++++++++++++++++ >>>> 1 file changed, 26 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/spmi/spmi-pmic-arb.txt >>> >>> As this is a qcom specific binding, I think the file name should be msm-spmi-pmic-arb.txt or something like that. >> >> Agreed. It might be nice to use a vendor prefix in the name too. How's qcom,msm-pmic-arb.txt sound? > > Sounds good, as I see we have other examples of having a comma in file names for bindings. The rule I've applied for bindings I created is use the complete compatible value for the file name. Where multiple are supported (e.g. SoC versions), just use the first one. Then, the filename is pretty unambiguous, both from a point of view of `ls` and from choosing a name. ^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2013-08-17 0:33 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-15 19:50 [PATCH RFC 0/3] Add support for the System Power Management Interface (SPMI) Josh Cartwright
2013-08-15 19:50 ` Josh Cartwright
2013-08-09 20:37 ` [PATCH RFC 2/3] spmi: Add MSM PMIC Arbiter SPMI controller Josh Cartwright
2013-08-09 20:37 ` Josh Cartwright
2013-08-16 18:52 ` Greg Kroah-Hartman
2013-08-16 18:52 ` Greg Kroah-Hartman
2013-08-09 20:37 ` [PATCH RFC 1/3] spmi: Linux driver framework for SPMI Josh Cartwright
2013-08-09 20:37 ` Josh Cartwright
2013-08-16 18:46 ` Greg Kroah-Hartman
2013-08-16 18:46 ` Greg Kroah-Hartman
2013-08-16 19:47 ` Josh Cartwright
2013-08-16 19:47 ` Josh Cartwright
2013-08-16 19:47 ` Josh Cartwright
2013-08-16 19:50 ` Josh Cartwright
2013-08-16 19:50 ` Josh Cartwright
2013-08-16 19:50 ` Josh Cartwright
2013-08-16 19:58 ` Greg Kroah-Hartman
2013-08-16 19:58 ` Greg Kroah-Hartman
2013-08-16 19:58 ` Greg Kroah-Hartman
2013-08-16 20:40 ` Josh Cartwright
2013-08-16 20:40 ` Josh Cartwright
2013-08-16 20:40 ` Josh Cartwright
2013-08-16 20:50 ` Greg Kroah-Hartman
2013-08-16 20:50 ` Greg Kroah-Hartman
2013-08-16 20:50 ` Greg Kroah-Hartman
2013-08-16 18:49 ` Greg Kroah-Hartman
2013-08-16 18:49 ` Greg Kroah-Hartman
2013-08-16 20:21 ` Josh Cartwright
2013-08-16 20:21 ` Josh Cartwright
2013-08-16 20:21 ` Josh Cartwright
2013-08-16 20:28 ` Greg Kroah-Hartman
2013-08-16 20:28 ` Greg Kroah-Hartman
2013-08-16 20:28 ` Greg Kroah-Hartman
2013-08-16 19:04 ` Kumar Gala
2013-08-16 19:04 ` Kumar Gala
2013-08-16 19:04 ` Kumar Gala
[not found] ` <b639088d50df93caaef8fe7e09c12953b1153ce8.1376596224.git.joshc@codeaurora.org>
[not found] ` <D1534646-7CB5-4EE7-8C1E-1C607BE22396@codeaurora.org>
2013-08-16 19:25 ` [PATCH RFC 3/3] spmi: document the PMIC arbiter SPMI bindings Josh Cartwright
2013-08-16 19:25 ` Josh Cartwright
2013-08-16 19:25 ` Josh Cartwright
2013-08-16 19:48 ` Kumar Gala
2013-08-16 19:48 ` Kumar Gala
2013-08-16 19:48 ` Kumar Gala
2013-08-16 23:17 ` Stephen Warren
2013-08-16 23:17 ` Stephen Warren
2013-08-16 23:17 ` Stephen Warren
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.