[PATCH 0/4] regmap: Generic I2C and SPI register map library

[PATCH 0/4] regmap: Generic I2C and SPI register map library

[Mark Brown]

This is a version of a series I've been posting for most of this release
cycle which abstracts out a bunch of widely duplicated code for doing
register I/O on devices with control slow buses like I2C and SPI.  As
well as saving us code for the register I/O itself this should also make
it easier to factor out further higher level code for this class of
devices.

Previously the patches had created a new directory under drivers/ but
one of the comments on the last review was that we should move to
drivers/base and another was that we should get this into mainline so
I've done that move and am now sending the patches to you.

The code is in a subdirectory because I'm anticipating it'll grow quite
a bit as we add facilities like fancy cache mechanisms on top of it.
The bus code isn't moved into the buses as I'm not currently entirely
happy with the internal interface to that and want to avoid cross tree
issues for the time being, I am pretty happy with the interfaces offered
to drivers though.

I realise it's a bit late in the cycle but it'd be really good if we
could get this merged in the next merge window, or accepted for about
then so we can create a stable branch we can merge into subsystem trees
in order to get drivers using this merged for 3.2.  As there's only one
user included the series it should be pretty low risk.

A more verbose cover letter:

Many I2C and SPI based devices implement register maps on top of the raw
wire interface.  This is generally done in a very standard fashion by
devices, resulting in a lot of very similar code in drivers.  For some
time now ASoC has factored this code out into the subsystem but that's
only useful for audio devices.  The intention with this series is to
generalise the concept so that it can be used throughout the kernel.

It's not intended that this be suitable for all devices - some devices
have things that are hard to generalise like registers with variable
size and paging which are hard to support genericly.  At the minute the
code is focused on the common cases.  It is likely that the same code
could be used with other buses with similar properties to I2C and SPI.

Currently only physical I/O is handled, the intention is that once this
support has been reviewed and merged the generic register cache code
that ASoC includes will also be factored out too.  For devices with read
heavy workloads (especially those that need a lot of read/modify/write
cycles) or which don't support read at all this can provide a useful
performance improvement and for sparse register maps there's a lot of
benefit in relatively complex cache code.

I'm not entirely happy with the implementation currently but am fairly
happy with the external interfaces.

A bunch more drivers have been converted to use this framework,
including the existing ASoC register I/O code, but due to various -next
dependencies these don't apply easily so I've omitted them from this
posting of the series.

There's a git branch at:

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

I've also created a regmap-asoc branch there which merges current ASoC
code with the regmap code.

The following changes since commit fe0d42203cb5616eeff68b14576a0f7e2dd56625:

  Linux 3.0-rc6 (2011-07-04 15:56:24 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Mark Brown (4):
      regmap: Add generic non-memory mapped register access API
      regmap: Add I2C bus support
      regmap: Add SPI bus support
      regulator: Convert tps65023 to use regmap API

 MAINTAINERS                            |    7 +
 drivers/base/Kconfig                   |    2 +
 drivers/base/Makefile                  |    1 +
 drivers/base/regmap/Kconfig            |    6 +
 drivers/base/regmap/Makefile           |    3 +
 drivers/base/regmap/regmap-i2c.c       |  113 ++++++++
 drivers/base/regmap/regmap-spi.c       |   75 +++++
 drivers/base/regmap/regmap.c           |  481 ++++++++++++++++++++++++++++++++
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   98 ++-----
 include/linux/regmap.h                 |   76 +++++
 11 files changed, 792 insertions(+), 71 deletions(-)
 create mode 100644 drivers/base/regmap/Kconfig
 create mode 100644 drivers/base/regmap/Makefile
 create mode 100644 drivers/base/regmap/regmap-i2c.c
 create mode 100644 drivers/base/regmap/regmap-spi.c
 create mode 100644 drivers/base/regmap/regmap.c
 create mode 100644 include/linux/regmap.h

[PATCH 1/4] regmap: Add generic non-memory mapped register access API

[Mark Brown]

There are many places in the tree where we implement register access for
devices on non-memory mapped buses, especially I2C and SPI. Since hardware
designers seem to have settled on a relatively consistent set of register
interfaces this can be effectively factored out into shared code.  There
are a standard set of formats for marshalling data for exchange with the
device, with the actual I/O mechanisms generally being simple byte
streams.

We create an abstraction for marshaling data into formats which can be
sent on the control interfaces, and create a standard method for
plugging in actual transport underneath that.

This is mostly a refactoring and renaming of the bottom level of the
existing code for sharing register I/O which we have in ASoC. A
subsequent patch in this series converts ASoC to use this.  The main
difference in interface is that reads return values by writing to a
location provided by a pointer rather than in the return value, ensuring
we can use the full range of the type for register data.  We also use
unsigned types rather than ints for the same reason.

As some of the devices can have very large register maps the existing
ASoC code also contains infrastructure for managing register caches.
This cache work will be moved over in a future stage to allow for
separate review, the current patch only deals with the physical I/O.
We also only deal with access to a single register at a time initially
as this is the most common case.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Liam Girdwood <lrg@ti.com>
---
 MAINTAINERS                  |    7 +
 drivers/base/Kconfig         |    2 +
 drivers/base/Makefile        |    1 +
 drivers/base/regmap/Kconfig  |    6 +
 drivers/base/regmap/Makefile |    2 +
 drivers/base/regmap/regmap.c |  481 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h       |   76 +++++++
 7 files changed, 575 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/regmap/Kconfig
 create mode 100644 drivers/base/regmap/Makefile
 create mode 100644 drivers/base/regmap/regmap.c
 create mode 100644 include/linux/regmap.h

diff --git a/MAINTAINERS b/MAINTAINERS
index bf0e348..0cf0f9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5343,6 +5343,13 @@ L:	reiserfs-devel@vger.kernel.org
 S:	Supported
 F:	fs/reiserfs/
 
+REGISTER MAP ABSTRACTION
+M:	Mark Brown <broonie@opensource.wolfsonmicro.com>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
+S:	Supported
+F:	drivers/base/regmap/
+F:	include/linux/regmap.h
+
 RFKILL
 M:	Johannes Berg <johannes@sipsolutions.net>
 L:	linux-wireless@vger.kernel.org
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index fa64fa0..21cf46f 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,4 +172,6 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+source "drivers/base/regmap/Kconfig"
+
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 4c5701c..dd4f9b2 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_REGMAP)	+= regmap/
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
 
diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
new file mode 100644
index 0000000..fc0eb1d
--- /dev/null
+++ b/drivers/base/regmap/Kconfig
@@ -0,0 +1,6 @@
+# Generic register map support.  There are no user servicable options here,
+# this is an API intended to be used by other kernel subsystems.  These
+# subsystems should select the appropriate symbols.
+
+config REGMAP
+	bool
diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
new file mode 100644
index 0000000..1e5037e
--- /dev/null
+++ b/drivers/base/regmap/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_REGMAP) += regmap.o
+
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
new file mode 100644
index 0000000..f6a83e9
--- /dev/null
+++ b/drivers/base/regmap/regmap.c
@@ -0,0 +1,481 @@
+/*
+ * Register map access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+
+#include <linux/regmap.h>
+
+struct regmap;
+
+static DEFINE_MUTEX(regmap_bus_lock);
+static LIST_HEAD(regmap_bus_list);
+
+struct regmap_format {
+	size_t buf_size;
+	size_t reg_bytes;
+	size_t val_bytes;
+	void (*format_write)(struct regmap *map,
+			     unsigned int reg, unsigned int val);
+	void (*format_reg)(void *buf, unsigned int reg);
+	void (*format_val)(void *buf, unsigned int val);
+	unsigned int (*parse_val)(void *buf);
+};
+
+struct regmap {
+	struct mutex lock;
+
+	struct device *dev; /* Device we do I/O on */
+	void *work_buf;     /* Scratch buffer used to format I/O */
+	struct regmap_format format;  /* Buffer format */
+	const struct regmap_bus *bus;
+};
+
+static void regmap_format_4_12_write(struct regmap *map,
+				     unsigned int reg, unsigned int val)
+{
+	__be16 *out = map->work_buf;
+	*out = cpu_to_be16((reg << 12) | val);
+}
+
+static void regmap_format_7_9_write(struct regmap *map,
+				    unsigned int reg, unsigned int val)
+{
+	__be16 *out = map->work_buf;
+	*out = cpu_to_be16((reg << 9) | val);
+}
+
+static void regmap_format_8(void *buf, unsigned int val)
+{
+	u8 *b = buf;
+
+	b[0] = val;
+}
+
+static void regmap_format_16(void *buf, unsigned int val)
+{
+	__be16 *b = buf;
+
+	b[0] = cpu_to_be16(val);
+}
+
+static unsigned int regmap_parse_8(void *buf)
+{
+	u8 *b = buf;
+
+	return b[0];
+}
+
+static unsigned int regmap_parse_16(void *buf)
+{
+	__be16 *b = buf;
+
+	b[0] = be16_to_cpu(b[0]);
+
+	return b[0];
+}
+
+/**
+ * remap_init: Initialise register map
+ *
+ * @dev: Device that will be interacted with
+ * @config: Configuration for register map
+ *
+ * The return value will be an ERR_PTR() on error or a valid pointer
+ * to a struct regmap.
+ */
+struct regmap *regmap_init(struct device *dev,
+			   const struct regmap_config *config)
+{
+	struct regmap *map;
+	int ret = -EINVAL;
+
+	map = kzalloc(sizeof(*map), GFP_KERNEL);
+	if (map == NULL) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	mutex_init(&map->lock);
+	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
+	map->format.reg_bytes = config->reg_bits / 8;
+	map->format.val_bytes = config->val_bits / 8;
+	map->dev = dev;
+
+	switch (config->reg_bits) {
+	case 4:
+		switch (config->val_bits) {
+		case 12:
+			map->format.format_write = regmap_format_4_12_write;
+			break;
+		default:
+			goto err_map;
+		}
+		break;
+
+	case 7:
+		switch (config->val_bits) {
+		case 9:
+			map->format.format_write = regmap_format_7_9_write;
+			break;
+		default:
+			goto err_map;
+		}
+		break;
+
+	case 8:
+		map->format.format_reg = regmap_format_8;
+		break;
+
+	case 16:
+		map->format.format_reg = regmap_format_16;
+		break;
+
+	default:
+		goto err_map;
+	}
+
+	switch (config->val_bits) {
+	case 8:
+		map->format.format_val = regmap_format_8;
+		map->format.parse_val = regmap_parse_8;
+		break;
+	case 16:
+		map->format.format_val = regmap_format_16;
+		map->format.parse_val = regmap_parse_16;
+		break;
+	}
+
+	if (!map->format.format_write &&
+	    !(map->format.format_reg && map->format.format_val))
+		goto err_map;
+
+	/* Figure out which bus to use, and also grab a lock on the
+	 * module supplying it. */
+	mutex_lock(&regmap_bus_lock);
+	list_for_each_entry(map->bus, &regmap_bus_list, list)
+		if (map->bus->type == dev->bus &&
+		    try_module_get(map->bus->owner))
+			break;
+	mutex_unlock(&regmap_bus_lock);
+
+	if (map->bus == NULL) {
+		ret = -EINVAL;
+		goto err_map;
+	}
+
+	map->work_buf = kmalloc(map->format.buf_size, GFP_KERNEL);
+	if (map->work_buf == NULL) {
+		ret = -ENOMEM;
+		goto err_bus;
+	}
+
+	return map;
+
+err_bus:
+	module_put(map->bus->owner);
+err_map:
+	kfree(map);
+err:
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(regmap_init);
+
+/**
+ * regmap_exit: Free a previously allocated register map
+ */
+void regmap_exit(struct regmap *map)
+{
+	kfree(map->work_buf);
+	module_put(map->bus->owner);
+	kfree(map);
+}
+EXPORT_SYMBOL_GPL(regmap_exit);
+
+static int _regmap_raw_write(struct regmap *map, unsigned int reg,
+			     const void *val, size_t val_len)
+{
+	void *buf;
+	int ret = -ENOTSUPP;
+	size_t len;
+
+	map->format.format_reg(map->work_buf, reg);
+
+	/* Try to do a gather write if we can */
+	if (map->bus->gather_write)
+		ret = map->bus->gather_write(map->dev, map->work_buf,
+					     map->format.reg_bytes,
+					     val, val_len);
+
+	/* Otherwise fall back on linearising by hand. */
+	if (ret == -ENOTSUPP) {
+		len = map->format.reg_bytes + val_len;
+		buf = kmalloc(len, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+
+		memcpy(buf, map->work_buf, map->format.reg_bytes);
+		memcpy(buf + map->format.reg_bytes, val, val_len);
+		ret = map->bus->write(map->dev, buf, len);
+
+		kfree(buf);
+	}
+
+	return ret;
+}
+
+static int _regmap_write(struct regmap *map, unsigned int reg,
+			 unsigned int val)
+{
+	BUG_ON(!map->format.format_write && !map->format.format_val);
+
+	if (map->format.format_write) {
+		map->format.format_write(map, reg, val);
+
+		return map->bus->write(map->dev, map->work_buf,
+				       map->format.buf_size);
+	} else {
+		map->format.format_val(map->work_buf + map->format.reg_bytes,
+				       val);
+		return _regmap_raw_write(map, reg,
+					 map->work_buf + map->format.reg_bytes,
+					 map->format.val_bytes);
+	}
+}
+
+/**
+ * regmap_write: Write a value to a single register
+ *
+ * @map: Register map to write to
+ * @reg: Register to write to
+ * @val: Value to be written
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
+{
+	int ret;
+
+	mutex_lock(&map->lock);
+
+	ret = _regmap_write(map, reg, val);
+
+	mutex_unlock(&map->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_write);
+
+/**
+ * regmap_raw_write: Write raw values to one or more registers
+ *
+ * @map: Register map to write to
+ * @reg: Initial register to write to
+ * @val: Block of data to be written, laid out for direct transmission to the
+ *       device
+ * @val_len: Length of data pointed to by val.
+ *
+ * This function is intended to be used for things like firmware
+ * download where a large block of data needs to be transferred to the
+ * device.  No formatting will be done on the data provided.
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_raw_write(struct regmap *map, unsigned int reg,
+		     const void *val, size_t val_len)
+{
+	int ret;
+
+	mutex_lock(&map->lock);
+
+	ret = _regmap_raw_write(map, reg, val, val_len);
+
+	mutex_unlock(&map->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_raw_write);
+
+static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
+			    unsigned int val_len)
+{
+	u8 *u8 = map->work_buf;
+	int ret;
+
+	map->format.format_reg(map->work_buf, reg);
+
+	/*
+	 * Some buses flag reads by setting the high bits in the
+	 * register addresss; since it's always the high bits for all
+	 * current formats we can do this here rather than in
+	 * formatting.  This may break if we get interesting formats.
+	 */
+	if (map->bus->read_flag_mask)
+		u8[0] |= map->bus->read_flag_mask;
+
+	ret = map->bus->read(map->dev, map->work_buf, map->format.reg_bytes,
+			     val, map->format.val_bytes);
+	if (ret != 0)
+		return ret;
+
+	return 0;
+}
+
+static int _regmap_read(struct regmap *map, unsigned int reg,
+			unsigned int *val)
+{
+	int ret;
+
+	if (!map->format.parse_val)
+		return -EINVAL;
+
+	ret = _regmap_raw_read(map, reg, map->work_buf, map->format.val_bytes);
+	if (ret == 0)
+		*val = map->format.parse_val(map->work_buf);
+
+	return ret;
+}
+
+/**
+ * regmap_read: Read a value from a single register
+ *
+ * @map: Register map to write to
+ * @reg: Register to be read from
+ * @val: Pointer to store read value
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
+{
+	int ret;
+
+	mutex_lock(&map->lock);
+
+	ret = _regmap_read(map, reg, val);
+
+	mutex_unlock(&map->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_read);
+
+/**
+ * regmap_raw_read: Read raw data from the device
+ *
+ * @map: Register map to write to
+ * @reg: First register to be read from
+ * @val: Pointer to store read value
+ * @val_len: Size of data to read
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
+		    size_t val_len)
+{
+	int ret;
+
+	mutex_lock(&map->lock);
+
+	ret = _regmap_raw_read(map, reg, val, val_len);
+
+	mutex_unlock(&map->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_raw_read);
+
+/**
+ * regmap_bulk_read: Read multiple registers from the device
+ *
+ * @map: Register map to write to
+ * @reg: First register to be read from
+ * @val: Pointer to store read value, in native register size for device
+ * @val_count: Number of registers to read
+ *
+ * A value of zero will be returned on success, a negative errno will
+ * be returned in error cases.
+ */
+int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
+		     size_t val_count)
+{
+	int ret, i;
+	size_t val_bytes = map->format.val_bytes;
+
+	if (!map->format.parse_val)
+		return -EINVAL;
+
+	ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
+	if (ret != 0)
+		return ret;
+
+	for (i = 0; i < val_count * val_bytes; i += val_bytes)
+		map->format.parse_val(val + i);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(regmap_bulk_read);
+
+/**
+ * remap_update_bits: Perform a read/modify/write cycle on the register map
+ *
+ * @map: Register map to update
+ * @reg: Register to update
+ * @mask: Bitmask to change
+ * @val: New value for bitmask
+ *
+ * Returns zero for success, a negative number on error.
+ */
+int regmap_update_bits(struct regmap *map, unsigned int reg,
+		       unsigned int mask, unsigned int val)
+{
+	int ret;
+	unsigned int tmp;
+
+	mutex_lock(&map->lock);
+
+	ret = _regmap_read(map, reg, &tmp);
+	if (ret != 0)
+		goto out;
+
+	tmp &= ~mask;
+	tmp |= val & mask;
+
+	ret = _regmap_write(map, reg, tmp);
+
+out:
+	mutex_unlock(&map->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(regmap_update_bits);
+
+void regmap_add_bus(struct regmap_bus *bus)
+{
+	mutex_lock(&regmap_bus_lock);
+	list_add(&bus->list, &regmap_bus_list);
+	mutex_unlock(&regmap_bus_lock);
+}
+EXPORT_SYMBOL_GPL(regmap_add_bus);
+
+void regmap_del_bus(struct regmap_bus *bus)
+{
+	mutex_lock(&regmap_bus_lock);
+	list_del(&bus->list);
+	mutex_unlock(&regmap_bus_lock);
+}
+EXPORT_SYMBOL_GPL(regmap_del_bus);
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
new file mode 100644
index 0000000..b033979
--- /dev/null
+++ b/include/linux/regmap.h
@@ -0,0 +1,76 @@
+#ifndef __LINUX_REGMAP_H
+#define __LINUX_REGMAP_H
+
+/*
+ * Register map access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/module.h>
+
+struct regmap_config {
+	int reg_bits;
+	int val_bits;
+};
+
+typedef int (*regmap_hw_write)(struct device *dev, const void *data,
+			       size_t count);
+typedef int (*regmap_hw_gather_write)(struct device *dev,
+				      const void *reg, size_t reg_len,
+				      const void *val, size_t val_len);
+typedef int (*regmap_hw_read)(struct device *dev,
+			      const void *reg_buf, size_t reg_size,
+			      void *val_buf, size_t val_size);
+
+/**
+ * Description of a hardware bus for the register map infrastructure.
+ *
+ * @list: Internal use.
+ * @type: Bus type, used to identify bus to be used for a device.
+ * @write: Write operation.
+ * @gather_write: Write operation with split register/value, return -ENOTSUPP
+ *                if not implemented  on a given device.
+ * @read: Read operation.  Data is returned in the buffer used to transmit
+ *         data.
+ * @owner: Module with the bus implementation, used to pin the implementation
+ *         in memory.
+ * @read_flag_mask: Mask to be set in the top byte of the register when doing
+ *                  a read.
+ */
+struct regmap_bus {
+	struct list_head list;
+	struct bus_type *type;
+	regmap_hw_write write;
+	regmap_hw_gather_write gather_write;
+	regmap_hw_read read;
+	struct module *owner;
+	u8 read_flag_mask;
+};
+
+struct regmap *regmap_init(struct device *dev,
+			   const struct regmap_config *config);
+void regmap_exit(struct regmap *map);
+int regmap_write(struct regmap *map, unsigned int reg, unsigned int val);
+int regmap_raw_write(struct regmap *map, unsigned int reg,
+		     const void *val, size_t val_len);
+int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val);
+int regmap_raw_read(struct regmap *map, unsigned int reg,
+		    void *val, size_t val_len);
+int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
+		     size_t val_count);
+int regmap_update_bits(struct regmap *map, unsigned int reg,
+		       unsigned int mask, unsigned int val);
+
+void regmap_add_bus(struct regmap_bus *bus);
+void regmap_del_bus(struct regmap_bus *bus);
+
+#endif
-- 
1.7.5.4

[PATCH 2/4] regmap: Add I2C bus support

[Mark Brown]

We initialise at postcore_initcall() so that we are available before users
- some users such as PMICs initialise very early. We won't actually try to
use any of the bus until a device initialises a register map.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Liam Girdwood <lrg@ti.com>
---
 drivers/base/regmap/Makefile     |    2 +-
 drivers/base/regmap/regmap-i2c.c |  113 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-i2c.c

diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 1e5037e..641c20a 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -1,2 +1,2 @@
 obj-$(CONFIG_REGMAP) += regmap.o
-
+obj-$(CONFIG_I2C) += regmap-i2c.o
diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
new file mode 100644
index 0000000..19d0739
--- /dev/null
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -0,0 +1,113 @@
+/*
+ * Register map access API - I2C support
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/regmap.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+
+static int regmap_i2c_write(struct device *dev, const void *data, size_t count)
+{
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+
+	ret = i2c_master_send(i2c, data, count);
+	if (ret == count)
+		return 0;
+	else if (ret < 0)
+		return ret;
+	else
+		return -EIO;
+}
+
+static int regmap_i2c_gather_write(struct device *dev,
+				   const void *reg, size_t reg_size,
+				   const void *val, size_t val_size)
+{
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct i2c_msg xfer[2];
+	int ret;
+
+	/* If the I2C controller can't do a gather tell the core, it
+	 * will substitute in a linear write for us.
+	 */
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_PROTOCOL_MANGLING))
+		return -ENOTSUPP;
+
+	xfer[0].addr = i2c->addr;
+	xfer[0].flags = 0;
+	xfer[0].len = reg_size;
+	xfer[0].buf = (void *)reg;
+
+	xfer[1].addr = i2c->addr;
+	xfer[1].flags = I2C_M_NOSTART;
+	xfer[1].len = val_size;
+	xfer[1].buf = (void *)val;
+
+	ret = i2c_transfer(i2c->adapter, xfer, 2);
+	if (ret == 2)
+		return 0;
+	if (ret < 0)
+		return ret;
+	else
+		return -EIO;
+}
+
+static int regmap_i2c_read(struct device *dev,
+			   const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct i2c_msg xfer[2];
+	int ret;
+
+	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_PROTOCOL_MANGLING))
+		return -ENOTSUPP;
+
+	xfer[0].addr = i2c->addr;
+	xfer[0].flags = 0;
+	xfer[0].len = reg_size;
+	xfer[0].buf = (void *)reg;
+
+	xfer[1].addr = i2c->addr;
+	xfer[1].flags = I2C_M_RD;
+	xfer[1].len = val_size;
+	xfer[1].buf = val;
+
+	ret = i2c_transfer(i2c->adapter, xfer, 2);
+	if (ret == 2)
+		return 0;
+	else if (ret < 0)
+		return ret;
+	else
+		return -EIO;
+}
+
+static struct regmap_bus regmap_i2c = {
+	.type = &i2c_bus_type,
+	.write = regmap_i2c_write,
+	.gather_write = regmap_i2c_gather_write,
+	.read = regmap_i2c_read,
+	.owner = THIS_MODULE,
+};
+
+static int __init regmap_i2c_init(void)
+{
+	regmap_add_bus(&regmap_i2c);
+	return 0;
+}
+postcore_initcall(regmap_i2c_init);
+
+static void __exit regmap_i2c_exit(void)
+{
+	regmap_del_bus(&regmap_i2c);
+}
+module_exit(regmap_i2c_exit);
-- 
1.7.5.4

Re: [PATCH 2/4] regmap: Add I2C bus support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 855 bytes --]

Hi Mark,

On Sat, Jul 09, 2011 at 01:50:42PM +0900, Mark Brown wrote:

> +#include <linux/regmap.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

I second Lars-Peter with init.h here (from his first review).

> +static int regmap_i2c_read(struct device *dev,
> +			   const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct i2c_msg xfer[2];
> +	int ret;
> +
> +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_PROTOCOL_MANGLING))
> +		return -ENOTSUPP;

Even more here. MANGLING is not needed for reading (and it will fail for a
number of i2c-masters).

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] regmap: Add I2C bus support

[Mark Brown]

On Sat, Jul 09, 2011 at 01:53:07PM +0200, Wolfram Sang wrote:
> On Sat, Jul 09, 2011 at 01:50:42PM +0900, Mark Brown wrote:

> > +#include <linux/regmap.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>

> I second Lars-Peter with init.h here (from his first review).

Remind me of what you're talking about here?

> > +	if (!i2c_check_functionality(i2c->adapter, I2C_FUNC_PROTOCOL_MANGLING))
> > +		return -ENOTSUPP;

> Even more here. MANGLING is not needed for reading (and it will fail for a
> number of i2c-masters).

Huh, though I'd removed this.

Re: [PATCH 2/4] regmap: Add I2C bus support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 944 bytes --]

On Sat, Jul 09, 2011 at 11:08:36PM +0900, Mark Brown wrote:
> On Sat, Jul 09, 2011 at 01:53:07PM +0200, Wolfram Sang wrote:
> > On Sat, Jul 09, 2011 at 01:50:42PM +0900, Mark Brown wrote:
> 
> > > +#include <linux/regmap.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/module.h>
> 
> > I second Lars-Peter with init.h here (from his first review).
> 
> Remind me of what you're talking about here?

He suggested including init.h, because of the use of postcore_initcall.

That's minor, of course. But you probably need to resend anyway, because
of the mangling-check in the read function.

But in general: great series! I also had played with the idea of
abstracting I2C/SPI-access away. Will surely try to convert a driver
myself.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/4] regmap: Add I2C bus support

[Mark Brown]

On Sat, Jul 09, 2011 at 04:57:33PM +0200, Wolfram Sang wrote:
> On Sat, Jul 09, 2011 at 11:08:36PM +0900, Mark Brown wrote:

> > Remind me of what you're talking about here?

> He suggested including init.h, because of the use of postcore_initcall.

Oh, right.  Thought I'd done that too actually.

> That's minor, of course. But you probably need to resend anyway, because
> of the mangling-check in the read function.

Possibly not, actually - as Greg acked it I'm probably going to try a
pull request during the merge window and see how that goes.

> abstracting I2C/SPI-access away. Will surely try to convert a driver
> myself.

Please send me any patches for that and I'll add them to the series.

Re: [PATCH 2/4] regmap: Add I2C bus support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Hi Mark,

> Possibly not, actually - as Greg acked it I'm probably going to try a
> pull request during the merge window and see how that goes.
> 
> > abstracting I2C/SPI-access away. Will surely try to convert a driver
> > myself.

Just saw you updated your repo with the fixes included. For this version, you
can add my

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

> Please send me any patches for that and I'll add them to the series.

Will do!

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/4] regmap: Add SPI bus support

[Mark Brown]

We initialise at postcore_initcall() so that we are available before users
- some users such as PMICs initialise very early. We won't actually try to
use any of the bus until a device initialises a register map.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Liam Girdwood <lrg@ti.com>
---
 drivers/base/regmap/Makefile     |    1 +
 drivers/base/regmap/regmap-spi.c |   75 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/regmap/regmap-spi.c

diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
index 641c20a..74c6680 100644
--- a/drivers/base/regmap/Makefile
+++ b/drivers/base/regmap/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_REGMAP) += regmap.o
 obj-$(CONFIG_I2C) += regmap-i2c.o
+obj-$(CONFIG_SPI) += regmap-spi.o
diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
new file mode 100644
index 0000000..04328c7
--- /dev/null
+++ b/drivers/base/regmap/regmap-spi.c
@@ -0,0 +1,75 @@
+/*
+ * Register map access API - SPI support
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+static int regmap_spi_write(struct device *dev, const void *data, size_t count)
+{
+	struct spi_device *spi = to_spi_device(dev);
+
+	return spi_write(spi, data, count);
+}
+
+static int regmap_spi_gather_write(struct device *dev,
+				   const void *reg, size_t reg_len,
+				   const void *val, size_t val_len)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct spi_message m;
+	struct spi_transfer t[2];
+
+	spi_message_init(&m);
+
+	memset(&t, 0, sizeof(t));
+
+	t[0].tx_buf = reg;
+	t[0].len = reg_len;
+	spi_message_add_tail(&t[0], &m);
+
+	t[1].tx_buf = val;
+	t[1].len = val_len;
+	spi_message_add_tail(&t[0], &m);
+
+	return spi_sync(spi, &m);
+}
+
+static int regmap_spi_read(struct device *dev,
+			   const void *reg, size_t reg_size,
+			   void *val, size_t val_size)
+{
+	struct spi_device *spi = to_spi_device(dev);
+
+	return spi_write_then_read(spi, reg, reg_size, val, val_size);
+}
+
+static struct regmap_bus regmap_spi = {
+	.type = &spi_bus_type,
+	.write = regmap_spi_write,
+	.gather_write = regmap_spi_gather_write,
+	.read = regmap_spi_read,
+	.owner = THIS_MODULE,
+	.read_flag_mask = 0x80,
+};
+
+static int __init regmap_spi_init(void)
+{
+	regmap_add_bus(&regmap_spi);
+	return 0;
+}
+postcore_initcall(regmap_spi_init);
+
+static void __exit regmap_spi_exit(void)
+{
+	regmap_del_bus(&regmap_spi);
+}
+module_exit(regmap_spi_exit);
-- 
1.7.5.4

Re: [PATCH 3/4] regmap: Add SPI bus support

[Grant Likely]

On Sat, Jul 09, 2011 at 01:50:43PM +0900, Mark Brown wrote:
> We initialise at postcore_initcall() so that we are available before users
> - some users such as PMICs initialise very early. We won't actually try to
> use any of the bus until a device initialises a register map.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Liam Girdwood <lrg@ti.com>
> ---
>  drivers/base/regmap/Makefile     |    1 +
>  drivers/base/regmap/regmap-spi.c |   75 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/regmap/regmap-spi.c
> 
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> index 641c20a..74c6680 100644
> --- a/drivers/base/regmap/Makefile
> +++ b/drivers/base/regmap/Makefile
> @@ -1,2 +1,3 @@
>  obj-$(CONFIG_REGMAP) += regmap.o
>  obj-$(CONFIG_I2C) += regmap-i2c.o
> +obj-$(CONFIG_SPI) += regmap-spi.o

I would think this code should live with drivers/spi.c  And similar
for the i2c implementation.

> diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
> new file mode 100644
> index 0000000..04328c7
> --- /dev/null
> +++ b/drivers/base/regmap/regmap-spi.c
> @@ -0,0 +1,75 @@
> +/*
> + * Register map access API - SPI support
> + *
> + * Copyright 2011 Wolfson Microelectronics plc
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +static int regmap_spi_write(struct device *dev, const void *data, size_t count)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +
> +	return spi_write(spi, data, count);
> +}
> +
> +static int regmap_spi_gather_write(struct device *dev,
> +				   const void *reg, size_t reg_len,
> +				   const void *val, size_t val_len)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct spi_message m;
> +	struct spi_transfer t[2];
> +
> +	spi_message_init(&m);
> +
> +	memset(&t, 0, sizeof(t));

If you do:

	struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
				     {.tx_buf = val, .len = val_len}};

Then the memset() and t[0]/t[1] lines can all be culled.

> +
> +	t[0].tx_buf = reg;
> +	t[0].len = reg_len;
> +	spi_message_add_tail(&t[0], &m);
> +
> +	t[1].tx_buf = val;
> +	t[1].len = val_len;
> +	spi_message_add_tail(&t[0], &m);

t[0]?

> +
> +	return spi_sync(spi, &m);
> +}
> +
> +static int regmap_spi_read(struct device *dev,
> +			   const void *reg, size_t reg_size,
> +			   void *val, size_t val_size)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +
> +	return spi_write_then_read(spi, reg, reg_size, val, val_size);

... of course, after looking at this function, maybe it would be
better to just add a spi_write_then_write() helper.  :-)

> +}
> +
> +static struct regmap_bus regmap_spi = {
> +	.type = &spi_bus_type,
> +	.write = regmap_spi_write,
> +	.gather_write = regmap_spi_gather_write,
> +	.read = regmap_spi_read,
> +	.owner = THIS_MODULE,
> +	.read_flag_mask = 0x80,
> +};
> +
> +static int __init regmap_spi_init(void)
> +{
> +	regmap_add_bus(&regmap_spi);
> +	return 0;
> +}
> +postcore_initcall(regmap_spi_init);
> +
> +static void __exit regmap_spi_exit(void)
> +{
> +	regmap_del_bus(&regmap_spi);
> +}
> +module_exit(regmap_spi_exit);
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] regmap: Add SPI bus support

[Mark Brown]

On Thu, Jul 14, 2011 at 08:53:27PM -0600, Grant Likely wrote:
> On Sat, Jul 09, 2011 at 01:50:43PM +0900, Mark Brown wrote:

> >  obj-$(CONFIG_I2C) += regmap-i2c.o
> > +obj-$(CONFIG_SPI) += regmap-spi.o

> I would think this code should live with drivers/spi.c  And similar
> for the i2c implementation.

There was a bit about this in the cover mail - it's the interace
stability issue again, the plan is to move them once we're more
confident that the interface used will stay stable.

> > +{
> > +	struct spi_device *spi = to_spi_device(dev);
> > +	struct spi_message m;
> > +	struct spi_transfer t[2];

> > +	spi_message_init(&m);
> > +
> > +	memset(&t, 0, sizeof(t));

> If you do:

> 	struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
> 				     {.tx_buf = val, .len = val_len}};

> Then the memset() and t[0]/t[1] lines can all be culled.

That does the init to zero?

> > +	t[1].tx_buf = val;
> > +	t[1].len = val_len;
> > +	spi_message_add_tail(&t[0], &m);

> t[0]?

Yes, that's been fixed in git for a little while now.

Re: [PATCH 3/4] regmap: Add SPI bus support

[Grant Likely]

On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jul 14, 2011 at 08:53:27PM -0600, Grant Likely wrote:
>> On Sat, Jul 09, 2011 at 01:50:43PM +0900, Mark Brown wrote:
>
>> >  obj-$(CONFIG_I2C) += regmap-i2c.o
>> > +obj-$(CONFIG_SPI) += regmap-spi.o
>
>> I would think this code should live with drivers/spi.c  And similar
>> for the i2c implementation.
>
> There was a bit about this in the cover mail - it's the interace
> stability issue again, the plan is to move them once we're more
> confident that the interface used will stay stable.

I've got no problem with churn in drivers/spi, and I'm fine to have
changes in drivers/spi merged via other trees if it means the code is
in the logical place.

>
>> > +{
>> > +   struct spi_device *spi = to_spi_device(dev);
>> > +   struct spi_message m;
>> > +   struct spi_transfer t[2];
>
>> > +   spi_message_init(&m);
>> > +
>> > +   memset(&t, 0, sizeof(t));
>
>> If you do:
>
>>       struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
>>                                    {.tx_buf = val, .len = val_len}};
>
>> Then the memset() and t[0]/t[1] lines can all be culled.
>
> That does the init to zero?

You might want to double check, but I believe it gets implemented as a
memcpy of a static initializer.

>
>> > +   t[1].tx_buf = val;
>> > +   t[1].len = val_len;
>> > +   spi_message_add_tail(&t[0], &m);
>
>> t[0]?
>
> Yes, that's been fixed in git for a little while now.
>



-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH 3/4] regmap: Add SPI bus support

[Mark Brown]

On Thu, Jul 14, 2011 at 11:04:27PM -0600, Grant Likely wrote:
> On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:

[Regmap API I2C bus code.]
> > There was a bit about this in the cover mail - it's the interace
> > stability issue again, the plan is to move them once we're more
> > confident that the interface used will stay stable.

> I've got no problem with churn in drivers/spi, and I'm fine to have
> changes in drivers/spi merged via other trees if it means the code is
> in the logical place.

Can I add an ack from you for that?  If/when it does get merged I'd
appreciate it if you pushed any code to me for review.

> >>       struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
> >>                                    {.tx_buf = val, .len = val_len}};

> >> Then the memset() and t[0]/t[1] lines can all be culled.

> > That does the init to zero?

> You might want to double check, but I believe it gets implemented as a
> memcpy of a static initializer.

Hrm, given that neither of us is 100% sure I think it's safer to leave
it written out long hand and avoid confusing anyone else.

Re: [PATCH 3/4] regmap: Add SPI bus support

[Jonathan Cameron]

On 07/15/11 06:09, Mark Brown wrote:
> On Thu, Jul 14, 2011 at 11:04:27PM -0600, Grant Likely wrote:
>> On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
> 
> [Regmap API I2C bus code.]
>>> There was a bit about this in the cover mail - it's the interace
>>> stability issue again, the plan is to move them once we're more
>>> confident that the interface used will stay stable.
> 
>> I've got no problem with churn in drivers/spi, and I'm fine to have
>> changes in drivers/spi merged via other trees if it means the code is
>> in the logical place.
> 
> Can I add an ack from you for that?  If/when it does get merged I'd
> appreciate it if you pushed any code to me for review.
> 
>>>>       struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
>>>>                                    {.tx_buf = val, .len = val_len}};
> 
>>>> Then the memset() and t[0]/t[1] lines can all be culled.
> 
>>> That does the init to zero?
> 
>> You might want to double check, but I believe it gets implemented as a
>> memcpy of a static initializer.
> 
> Hrm, given that neither of us is 100% sure I think it's safer to leave
> it written out long hand and avoid confusing anyone else.

It does the init to zero. The relevant bit of the spec is designated initializers.

See c99 spec (draft here as I'm not paying for it ;) WG14/N1124
6.7.8 

Clause 19: The initialization shall occur in initializer list order, each initializer provided for a
particular subobject overriding any previously listed initializer for the same subobject;130)
all subobjects that are not initialized explicitly shall be initialized implicitly the same as
objects that have static storage duration.

So here they will be initialized the same as static objects. For completeness that's
covered in clause 10:
If an object that has automatic storage duration is not initialized explicitly, its value is
indeterminate. If an object that has static storage duration is not initialized explicitly,
then:
— if it has pointer type, it is initialized to a null pointer;
— if it has arithmetic type, it is initialized to (positive or unsigned) zero;
— if it is an aggregate, every member is initialized (recursively) according to these rules;
— if it is a union, the first named member is initialized (recursively) according to these
rules.


Jonathan

Re: [PATCH 3/4] regmap: Add SPI bus support

[Grant Likely]

On Fri, Jul 15, 2011 at 10:01:36AM +0100, Jonathan Cameron wrote:
> On 07/15/11 06:09, Mark Brown wrote:
> > On Thu, Jul 14, 2011 at 11:04:27PM -0600, Grant Likely wrote:
> >> On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
> >> <broonie@opensource.wolfsonmicro.com> wrote:
> > 
> > [Regmap API I2C bus code.]
> >>> There was a bit about this in the cover mail - it's the interace
> >>> stability issue again, the plan is to move them once we're more
> >>> confident that the interface used will stay stable.
> > 
> >> I've got no problem with churn in drivers/spi, and I'm fine to have
> >> changes in drivers/spi merged via other trees if it means the code is
> >> in the logical place.
> > 
> > Can I add an ack from you for that?  If/when it does get merged I'd
> > appreciate it if you pushed any code to me for review.
> > 
> >>>>       struct spi_transfer t[2] = { {.tx_buf = reg, .len = reg_len},
> >>>>                                    {.tx_buf = val, .len = val_len}};
> > 
> >>>> Then the memset() and t[0]/t[1] lines can all be culled.
> > 
> >>> That does the init to zero?
> > 
> >> You might want to double check, but I believe it gets implemented as a
> >> memcpy of a static initializer.
> > 
> > Hrm, given that neither of us is 100% sure I think it's safer to leave
> > it written out long hand and avoid confusing anyone else.
> 
> It does the init to zero. The relevant bit of the spec is designated initializers.
> 
> See c99 spec (draft here as I'm not paying for it ;) WG14/N1124
> 6.7.8 

Okay, good.  I would prefer the initializer form.

g.

[PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Mark Brown]

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Acked-by: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   98 +++++++++-----------------------
 2 files changed, 28 insertions(+), 71 deletions(-)

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 065892a..8128db3 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -235,6 +235,7 @@ config REGULATOR_TPS6105X
 config REGULATOR_TPS65023
 	tristate "TI TPS65023 Power regulators"
 	depends on I2C
+	select REGMAP
 	help
 	  This driver supports TPS65023 voltage regulator chips. TPS65023 provides
 	  three step-down converters and two general-purpose LDO voltage regulators.
diff --git a/drivers/regulator/tps65023-regulator.c b/drivers/regulator/tps65023-regulator.c
index fbddc15..3fa3ba5 100644
--- a/drivers/regulator/tps65023-regulator.c
+++ b/drivers/regulator/tps65023-regulator.c
@@ -25,6 +25,7 @@
 #include <linux/i2c.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/regmap.h>
 
 /* Register definitions */
 #define	TPS65023_REG_VERSION		0
@@ -125,93 +126,35 @@ struct tps_pmic {
 	struct i2c_client *client;
 	struct regulator_dev *rdev[TPS65023_NUM_REGULATOR];
 	const struct tps_info *info[TPS65023_NUM_REGULATOR];
-	struct mutex io_lock;
+	struct regmap *regmap;
 };
 
-static inline int tps_65023_read(struct tps_pmic *tps, u8 reg)
-{
-	return i2c_smbus_read_byte_data(tps->client, reg);
-}
-
-static inline int tps_65023_write(struct tps_pmic *tps, u8 reg, u8 val)
-{
-	return i2c_smbus_write_byte_data(tps->client, reg, val);
-}
-
 static int tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask)
 {
-	int err, data;
-
-	mutex_lock(&tps->io_lock);
-
-	data = tps_65023_read(tps, reg);
-	if (data < 0) {
-		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
-		err = data;
-		goto out;
-	}
-
-	data |= mask;
-	err = tps_65023_write(tps, reg, data);
-	if (err)
-		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
-
-out:
-	mutex_unlock(&tps->io_lock);
-	return err;
+	return regmap_update_bits(tps->regmap, reg, mask, mask);
 }
 
 static int tps_65023_clear_bits(struct tps_pmic *tps, u8 reg, u8 mask)
 {
-	int err, data;
-
-	mutex_lock(&tps->io_lock);
-
-	data = tps_65023_read(tps, reg);
-	if (data < 0) {
-		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
-		err = data;
-		goto out;
-	}
-
-	data &= ~mask;
-
-	err = tps_65023_write(tps, reg, data);
-	if (err)
-		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
-
-out:
-	mutex_unlock(&tps->io_lock);
-	return err;
-
+	return regmap_update_bits(tps->regmap, reg, mask, 0);
 }
 
 static int tps_65023_reg_read(struct tps_pmic *tps, u8 reg)
 {
-	int data;
+	unsigned int val;
+	int ret;
 
-	mutex_lock(&tps->io_lock);
+	ret = regmap_read(tps->regmap, reg, &val);
 
-	data = tps_65023_read(tps, reg);
-	if (data < 0)
-		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
-
-	mutex_unlock(&tps->io_lock);
-	return data;
+	if (ret != 0)
+		return ret;
+	else
+		return val;
 }
 
 static int tps_65023_reg_write(struct tps_pmic *tps, u8 reg, u8 val)
 {
-	int err;
-
-	mutex_lock(&tps->io_lock);
-
-	err = tps_65023_write(tps, reg, val);
-	if (err < 0)
-		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
-
-	mutex_unlock(&tps->io_lock);
-	return err;
+	return regmap_write(tps->regmap, reg, val);
 }
 
 static int tps65023_dcdc_is_enabled(struct regulator_dev *dev)
@@ -463,6 +406,10 @@ static struct regulator_ops tps65023_ldo_ops = {
 	.list_voltage = tps65023_ldo_list_voltage,
 };
 
+static struct regmap_config tps65023_regmap_config = {
+	.reg_bits = 8, .val_bits = 8,
+};
+
 static int __devinit tps_65023_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -488,7 +435,13 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
 	if (!tps)
 		return -ENOMEM;
 
-	mutex_init(&tps->io_lock);
+	tps->regmap = regmap_init(&client->dev, &tps65023_regmap_config);
+	if (IS_ERR(tps->regmap)) {
+		error = PTR_ERR(tps->regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			error);
+		goto fail_alloc;
+	}
 
 	/* common for all regulators */
 	tps->client = client;
@@ -523,10 +476,12 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
 
 	return 0;
 
- fail:
+fail:
 	while (--i >= 0)
 		regulator_unregister(tps->rdev[i]);
 
+	regmap_exit(tps->regmap);
+fail_alloc:
 	kfree(tps);
 	return error;
 }
@@ -545,6 +500,7 @@ static int __devexit tps_65023_remove(struct i2c_client *client)
 	for (i = 0; i < TPS65023_NUM_REGULATOR; i++)
 		regulator_unregister(tps->rdev[i]);
 
+	regmap_exit(tps->regmap);
 	kfree(tps);
 
 	return 0;
-- 
1.7.5.4

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Grant Likely]

On Sat, Jul 09, 2011 at 01:50:44PM +0900, Mark Brown wrote:
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Liam Girdwood <lrg@ti.com>
> ---
>  drivers/regulator/Kconfig              |    1 +
>  drivers/regulator/tps65023-regulator.c |   98 +++++++++-----------------------
>  2 files changed, 28 insertions(+), 71 deletions(-)

Nice diffstat.  :-)

> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 065892a..8128db3 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -235,6 +235,7 @@ config REGULATOR_TPS6105X
>  config REGULATOR_TPS65023
>  	tristate "TI TPS65023 Power regulators"
>  	depends on I2C
> +	select REGMAP
>  	help
>  	  This driver supports TPS65023 voltage regulator chips. TPS65023 provides
>  	  three step-down converters and two general-purpose LDO voltage regulators.
> diff --git a/drivers/regulator/tps65023-regulator.c b/drivers/regulator/tps65023-regulator.c
> index fbddc15..3fa3ba5 100644
> --- a/drivers/regulator/tps65023-regulator.c
> +++ b/drivers/regulator/tps65023-regulator.c
> @@ -25,6 +25,7 @@
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/regmap.h>
>  
>  /* Register definitions */
>  #define	TPS65023_REG_VERSION		0
> @@ -125,93 +126,35 @@ struct tps_pmic {
>  	struct i2c_client *client;
>  	struct regulator_dev *rdev[TPS65023_NUM_REGULATOR];
>  	const struct tps_info *info[TPS65023_NUM_REGULATOR];
> -	struct mutex io_lock;
> +	struct regmap *regmap;
>  };
>  
> -static inline int tps_65023_read(struct tps_pmic *tps, u8 reg)
> -{
> -	return i2c_smbus_read_byte_data(tps->client, reg);
> -}
> -
> -static inline int tps_65023_write(struct tps_pmic *tps, u8 reg, u8 val)
> -{
> -	return i2c_smbus_write_byte_data(tps->client, reg, val);
> -}
> -
>  static int tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask)
>  {
> -	int err, data;
> -
> -	mutex_lock(&tps->io_lock);
> -
> -	data = tps_65023_read(tps, reg);
> -	if (data < 0) {
> -		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
> -		err = data;
> -		goto out;
> -	}
> -
> -	data |= mask;
> -	err = tps_65023_write(tps, reg, data);
> -	if (err)
> -		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
> -
> -out:
> -	mutex_unlock(&tps->io_lock);
> -	return err;
> +	return regmap_update_bits(tps->regmap, reg, mask, mask);
>  }
>  
>  static int tps_65023_clear_bits(struct tps_pmic *tps, u8 reg, u8 mask)
>  {
> -	int err, data;
> -
> -	mutex_lock(&tps->io_lock);
> -
> -	data = tps_65023_read(tps, reg);
> -	if (data < 0) {
> -		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
> -		err = data;
> -		goto out;
> -	}
> -
> -	data &= ~mask;
> -
> -	err = tps_65023_write(tps, reg, data);
> -	if (err)
> -		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
> -
> -out:
> -	mutex_unlock(&tps->io_lock);
> -	return err;
> -
> +	return regmap_update_bits(tps->regmap, reg, mask, 0);
>  }
>  
>  static int tps_65023_reg_read(struct tps_pmic *tps, u8 reg)
>  {
> -	int data;
> +	unsigned int val;
> +	int ret;
>  
> -	mutex_lock(&tps->io_lock);
> +	ret = regmap_read(tps->regmap, reg, &val);
>  
> -	data = tps_65023_read(tps, reg);
> -	if (data < 0)
> -		dev_err(&tps->client->dev, "Read from reg 0x%x failed\n", reg);
> -
> -	mutex_unlock(&tps->io_lock);
> -	return data;
> +	if (ret != 0)
> +		return ret;
> +	else
> +		return val;
>  }
>  
>  static int tps_65023_reg_write(struct tps_pmic *tps, u8 reg, u8 val)
>  {
> -	int err;
> -
> -	mutex_lock(&tps->io_lock);
> -
> -	err = tps_65023_write(tps, reg, val);
> -	if (err < 0)
> -		dev_err(&tps->client->dev, "Write for reg 0x%x failed\n", reg);
> -
> -	mutex_unlock(&tps->io_lock);
> -	return err;
> +	return regmap_write(tps->regmap, reg, val);
>  }
>  
>  static int tps65023_dcdc_is_enabled(struct regulator_dev *dev)
> @@ -463,6 +406,10 @@ static struct regulator_ops tps65023_ldo_ops = {
>  	.list_voltage = tps65023_ldo_list_voltage,
>  };
>  
> +static struct regmap_config tps65023_regmap_config = {
> +	.reg_bits = 8, .val_bits = 8,
> +};
> +
>  static int __devinit tps_65023_probe(struct i2c_client *client,
>  				     const struct i2c_device_id *id)
>  {
> @@ -488,7 +435,13 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
>  	if (!tps)
>  		return -ENOMEM;
>  
> -	mutex_init(&tps->io_lock);
> +	tps->regmap = regmap_init(&client->dev, &tps65023_regmap_config);

Yeah, if this usage is typical, the caller will always know exactly
what kind of regmap it needs to set up because it had an i2c_client or
an spi_device instance.  I think it would be better to drop the
central registration of regmap bus types and use bus-specific init
variant.  It just makes for one more bit of registration
infrastructure that needs to be setup before any drivers us it.

> +	if (IS_ERR(tps->regmap)) {
> +		error = PTR_ERR(tps->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			error);
> +		goto fail_alloc;
> +	}
>  
>  	/* common for all regulators */
>  	tps->client = client;
> @@ -523,10 +476,12 @@ static int __devinit tps_65023_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> - fail:
> +fail:
>  	while (--i >= 0)
>  		regulator_unregister(tps->rdev[i]);
>  
> +	regmap_exit(tps->regmap);
> +fail_alloc:
>  	kfree(tps);
>  	return error;
>  }
> @@ -545,6 +500,7 @@ static int __devexit tps_65023_remove(struct i2c_client *client)
>  	for (i = 0; i < TPS65023_NUM_REGULATOR; i++)
>  		regulator_unregister(tps->rdev[i]);
>  
> +	regmap_exit(tps->regmap);
>  	kfree(tps);
>  
>  	return 0;
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Mark Brown]

On Thu, Jul 14, 2011 at 08:53:28PM -0600, Grant Likely wrote:
> On Sat, Jul 09, 2011 at 01:50:44PM +0900, Mark Brown wrote:

> > -	mutex_init(&tps->io_lock);
> > +	tps->regmap = regmap_init(&client->dev, &tps65023_regmap_config);

> Yeah, if this usage is typical, the caller will always know exactly
> what kind of regmap it needs to set up because it had an i2c_client or
> an spi_device instance.  I think it would be better to drop the
> central registration of regmap bus types and use bus-specific init
> variant.  It just makes for one more bit of registration
> infrastructure that needs to be setup before any drivers us it.

Right, the driver is always going to know exactly what bus it registered
on but for many drivers we don't really have any bus-specific code once
we factor out the register I/O.

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Grant Likely]

On Fri, Jul 15, 2011 at 01:48:05PM +0900, Mark Brown wrote:
> On Thu, Jul 14, 2011 at 08:53:28PM -0600, Grant Likely wrote:
> > On Sat, Jul 09, 2011 at 01:50:44PM +0900, Mark Brown wrote:
> 
> > > -	mutex_init(&tps->io_lock);
> > > +	tps->regmap = regmap_init(&client->dev, &tps65023_regmap_config);
> 
> > Yeah, if this usage is typical, the caller will always know exactly
> > what kind of regmap it needs to set up because it had an i2c_client or
> > an spi_device instance.  I think it would be better to drop the
> > central registration of regmap bus types and use bus-specific init
> > variant.  It just makes for one more bit of registration
> > infrastructure that needs to be setup before any drivers us it.
> 
> Right, the driver is always going to know exactly what bus it registered
> on but for many drivers we don't really have any bus-specific code once
> we factor out the register I/O.

Exactly my point.  The only time a global registration list is needed
is at setup time.  Once the regmap structure is initialized, it
doesn't need to reference the list.  I far prefer the simplicity of an
explicit bus type initialization than the implicit method with the
extra indirection code needed to implement it, by my measure, ~60-70
lines of code.

Also, making is explicit guarantees that there is no question about
the i2c or spi module of regmap getting loaded before it gets used.

g.

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Mark Brown]

On Fri, Jul 15, 2011 at 12:29:54PM -0600, Grant Likely wrote:
> On Fri, Jul 15, 2011 at 01:48:05PM +0900, Mark Brown wrote:

> > Right, the driver is always going to know exactly what bus it registered
> > on but for many drivers we don't really have any bus-specific code once
> > we factor out the register I/O.

> Exactly my point.  The only time a global registration list is needed
> is at setup time.  Once the regmap structure is initialized, it
> doesn't need to reference the list.  I far prefer the simplicity of an
> explicit bus type initialization than the implicit method with the

I'm just not seeing massive complexity here, and I am writing a lot of
multi-bus drivers with bolierplate code.  My ideal world would be one
line probe and exit functions for the buses, or better yet a single ops
structure assigned to both buses, as with my user hat on if I'm telling
the API the struct device to use I really shouldn't be having to tell it
what bus the device is on since I just told it that.

> extra indirection code needed to implement it, by my measure, ~60-70
> lines of code.

If you're talking about pure lines of code here then obviously you don't
need to convert terribly many drivers before you end up with a saving
overall.

> Also, making is explicit guarantees that there is no question about
> the i2c or spi module of regmap getting loaded before it gets used.

The bus must obviously be ready for the device to probe in the first
place and the regmap API is static only so it'll always be there.  We
just need to load the regmap hooks along with the bus core and we're
fine.

Still, the merge window is getting near so I guess I'll make the change :/

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Grant Likely]

On Sat, Jul 16, 2011 at 10:47:09AM +0900, Mark Brown wrote:
> On Fri, Jul 15, 2011 at 12:29:54PM -0600, Grant Likely wrote:
> > On Fri, Jul 15, 2011 at 01:48:05PM +0900, Mark Brown wrote:
> 
> > > Right, the driver is always going to know exactly what bus it registered
> > > on but for many drivers we don't really have any bus-specific code once
> > > we factor out the register I/O.
> 
> > Exactly my point.  The only time a global registration list is needed
> > is at setup time.  Once the regmap structure is initialized, it
> > doesn't need to reference the list.  I far prefer the simplicity of an
> > explicit bus type initialization than the implicit method with the
> 
> I'm just not seeing massive complexity here, and I am writing a lot of
> multi-bus drivers with bolierplate code.  My ideal world would be one
> line probe and exit functions for the buses, or better yet a single ops
> structure assigned to both buses, as with my user hat on if I'm telling
> the API the struct device to use I really shouldn't be having to tell it
> what bus the device is on since I just told it that.
> 
> > extra indirection code needed to implement it, by my measure, ~60-70
> > lines of code.
> 
> If you're talking about pure lines of code here then obviously you don't
> need to convert terribly many drivers before you end up with a saving
> overall.
> 
> > Also, making is explicit guarantees that there is no question about
> > the i2c or spi module of regmap getting loaded before it gets used.
> 
> The bus must obviously be ready for the device to probe in the first
> place and the regmap API is static only so it'll always be there.  We
> just need to load the regmap hooks along with the bus core and we're
> fine.
> 
> Still, the merge window is getting near so I guess I'll make the change :/

:)  Indeed.  We can merge without and then enjoy the argument at our
leisure after the merge window.

g.

Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API

[Mark Brown]

On Fri, Jul 15, 2011 at 08:06:18PM -0600, Grant Likely wrote:
> On Sat, Jul 16, 2011 at 10:47:09AM +0900, Mark Brown wrote:

> > Still, the merge window is getting near so I guess I'll make the change :/

> :)  Indeed.  We can merge without and then enjoy the argument at our
> leisure after the merge window.

I've actually already made the change, just testing now.

Re: [PATCH 1/4] regmap: Add generic non-memory mapped register access API

[Greg KH]

On Sat, Jul 09, 2011 at 01:50:41PM +0900, Mark Brown wrote:
> There are many places in the tree where we implement register access for
> devices on non-memory mapped buses, especially I2C and SPI. Since hardware
> designers seem to have settled on a relatively consistent set of register
> interfaces this can be effectively factored out into shared code.  There
> are a standard set of formats for marshalling data for exchange with the
> device, with the actual I/O mechanisms generally being simple byte
> streams.
> 
> We create an abstraction for marshaling data into formats which can be
> sent on the control interfaces, and create a standard method for
> plugging in actual transport underneath that.
> 
> This is mostly a refactoring and renaming of the bottom level of the
> existing code for sharing register I/O which we have in ASoC. A
> subsequent patch in this series converts ASoC to use this.  The main
> difference in interface is that reads return values by writing to a
> location provided by a pointer rather than in the return value, ensuring
> we can use the full range of the type for register data.  We also use
> unsigned types rather than ints for the same reason.
> 
> As some of the devices can have very large register maps the existing
> ASoC code also contains infrastructure for managing register caches.
> This cache work will be moved over in a future stage to allow for
> separate review, the current patch only deals with the physical I/O.
> We also only deal with access to a single register at a time initially
> as this is the most common case.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Liam Girdwood <lrg@ti.com>

Acked-by: Greg Kroah-Hartman <gregkh@suse.de>

Re: [PATCH 1/4] regmap: Add generic non-memory mapped register access API

[Grant Likely]

On Sat, Jul 09, 2011 at 01:50:41PM +0900, Mark Brown wrote:
> There are many places in the tree where we implement register access for
> devices on non-memory mapped buses, especially I2C and SPI. Since hardware
> designers seem to have settled on a relatively consistent set of register
> interfaces this can be effectively factored out into shared code.  There
> are a standard set of formats for marshalling data for exchange with the
> device, with the actual I/O mechanisms generally being simple byte
> streams.
> 
> We create an abstraction for marshaling data into formats which can be
> sent on the control interfaces, and create a standard method for
> plugging in actual transport underneath that.
> 
> This is mostly a refactoring and renaming of the bottom level of the
> existing code for sharing register I/O which we have in ASoC. A
> subsequent patch in this series converts ASoC to use this.  The main
> difference in interface is that reads return values by writing to a
> location provided by a pointer rather than in the return value, ensuring
> we can use the full range of the type for register data.  We also use
> unsigned types rather than ints for the same reason.
> 
> As some of the devices can have very large register maps the existing
> ASoC code also contains infrastructure for managing register caches.
> This cache work will be moved over in a future stage to allow for
> separate review, the current patch only deals with the physical I/O.
> We also only deal with access to a single register at a time initially
> as this is the most common case.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Acked-by: Liam Girdwood <lrg@ti.com>

Hey Mark.

I've missed most of the discussion on this patch, but I support the
idea.  I've got some concerns about the API and the way it is intended
to be used.  Comments below...

> ---
>  MAINTAINERS                  |    7 +
>  drivers/base/Kconfig         |    2 +
>  drivers/base/Makefile        |    1 +
>  drivers/base/regmap/Kconfig  |    6 +
>  drivers/base/regmap/Makefile |    2 +
>  drivers/base/regmap/regmap.c |  481 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/regmap.h       |   76 +++++++
>  7 files changed, 575 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/regmap/Kconfig
>  create mode 100644 drivers/base/regmap/Makefile
>  create mode 100644 drivers/base/regmap/regmap.c
>  create mode 100644 include/linux/regmap.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index bf0e348..0cf0f9f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -5343,6 +5343,13 @@ L:	reiserfs-devel@vger.kernel.org
>  S:	Supported
>  F:	fs/reiserfs/
>  
> +REGISTER MAP ABSTRACTION
> +M:	Mark Brown <broonie@opensource.wolfsonmicro.com>
> +T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
> +S:	Supported
> +F:	drivers/base/regmap/
> +F:	include/linux/regmap.h
> +
>  RFKILL
>  M:	Johannes Berg <johannes@sipsolutions.net>
>  L:	linux-wireless@vger.kernel.org
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index fa64fa0..21cf46f 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -172,4 +172,6 @@ config SYS_HYPERVISOR
>  	bool
>  	default n
>  
> +source "drivers/base/regmap/Kconfig"
> +
>  endmenu
> diff --git a/drivers/base/Makefile b/drivers/base/Makefile
> index 4c5701c..dd4f9b2 100644
> --- a/drivers/base/Makefile
> +++ b/drivers/base/Makefile
> @@ -18,6 +18,7 @@ ifeq ($(CONFIG_SYSFS),y)
>  obj-$(CONFIG_MODULES)	+= module.o
>  endif
>  obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
> +obj-$(CONFIG_REGMAP)	+= regmap/
>  
>  ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
>  
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> new file mode 100644
> index 0000000..fc0eb1d
> --- /dev/null
> +++ b/drivers/base/regmap/Kconfig
> @@ -0,0 +1,6 @@
> +# Generic register map support.  There are no user servicable options here,
> +# this is an API intended to be used by other kernel subsystems.  These
> +# subsystems should select the appropriate symbols.
> +
> +config REGMAP
> +	bool
> diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile
> new file mode 100644
> index 0000000..1e5037e
> --- /dev/null
> +++ b/drivers/base/regmap/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_REGMAP) += regmap.o
> +
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> new file mode 100644
> index 0000000..f6a83e9
> --- /dev/null
> +++ b/drivers/base/regmap/regmap.c
> @@ -0,0 +1,481 @@
> +/*
> + * Register map access API
> + *
> + * Copyright 2011 Wolfson Microelectronics plc
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +
> +#include <linux/regmap.h>
> +
> +struct regmap;
> +
> +static DEFINE_MUTEX(regmap_bus_lock);
> +static LIST_HEAD(regmap_bus_list);
> +
> +struct regmap_format {
> +	size_t buf_size;
> +	size_t reg_bytes;
> +	size_t val_bytes;
> +	void (*format_write)(struct regmap *map,
> +			     unsigned int reg, unsigned int val);
> +	void (*format_reg)(void *buf, unsigned int reg);
> +	void (*format_val)(void *buf, unsigned int val);
> +	unsigned int (*parse_val)(void *buf);
> +};
> +
> +struct regmap {
> +	struct mutex lock;
> +
> +	struct device *dev; /* Device we do I/O on */
> +	void *work_buf;     /* Scratch buffer used to format I/O */
> +	struct regmap_format format;  /* Buffer format */
> +	const struct regmap_bus *bus;
> +};

Some /** kerneldoc on these structures would be helpful.

> +
> +static void regmap_format_4_12_write(struct regmap *map,
> +				     unsigned int reg, unsigned int val)
> +{
> +	__be16 *out = map->work_buf;
> +	*out = cpu_to_be16((reg << 12) | val);
> +}
> +
> +static void regmap_format_7_9_write(struct regmap *map,
> +				    unsigned int reg, unsigned int val)
> +{
> +	__be16 *out = map->work_buf;
> +	*out = cpu_to_be16((reg << 9) | val);
> +}
> +
> +static void regmap_format_8(void *buf, unsigned int val)
> +{
> +	u8 *b = buf;
> +
> +	b[0] = val;
> +}
> +
> +static void regmap_format_16(void *buf, unsigned int val)
> +{
> +	__be16 *b = buf;
> +
> +	b[0] = cpu_to_be16(val);
> +}
> +
> +static unsigned int regmap_parse_8(void *buf)
> +{
> +	u8 *b = buf;
> +
> +	return b[0];
> +}
> +
> +static unsigned int regmap_parse_16(void *buf)
> +{
> +	__be16 *b = buf;
> +
> +	b[0] = be16_to_cpu(b[0]);
> +
> +	return b[0];
> +}
> +
> +/**
> + * remap_init: Initialise register map

nit: "remap_init() - Initialise reigster map"

> + *
> + * @dev: Device that will be interacted with
> + * @config: Configuration for register map
> + *
> + * The return value will be an ERR_PTR() on error or a valid pointer
> + * to a struct regmap.
> + */
> +struct regmap *regmap_init(struct device *dev,
> +			   const struct regmap_config *config)
> +{
> +	struct regmap *map;
> +	int ret = -EINVAL;
> +
> +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> +	if (map == NULL) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}

Wouldn't it be appropriate to allow drivers to embed the regmap in a
private data structure?

> +
> +	mutex_init(&map->lock);
> +	map->format.buf_size = (config->reg_bits + config->val_bits) / 8;
> +	map->format.reg_bytes = config->reg_bits / 8;
> +	map->format.val_bytes = config->val_bits / 8;
> +	map->dev = dev;
> +
> +	switch (config->reg_bits) {
> +	case 4:
> +		switch (config->val_bits) {
> +		case 12:
> +			map->format.format_write = regmap_format_4_12_write;
> +			break;
> +		default:
> +			goto err_map;
> +		}
> +		break;
> +
> +	case 7:
> +		switch (config->val_bits) {
> +		case 9:
> +			map->format.format_write = regmap_format_7_9_write;
> +			break;
> +		default:
> +			goto err_map;
> +		}
> +		break;
> +
> +	case 8:
> +		map->format.format_reg = regmap_format_8;
> +		break;
> +
> +	case 16:
> +		map->format.format_reg = regmap_format_16;
> +		break;
> +
> +	default:
> +		goto err_map;
> +	}
> +
> +	switch (config->val_bits) {
> +	case 8:
> +		map->format.format_val = regmap_format_8;
> +		map->format.parse_val = regmap_parse_8;
> +		break;
> +	case 16:
> +		map->format.format_val = regmap_format_16;
> +		map->format.parse_val = regmap_parse_16;
> +		break;
> +	}

Rather than a big case statement, could this be cleaner with a lookup
table?

I'm a bit concerned that the overall structure of the init function
will be inflexible in the long run.  Specifically, the way the
instance structure is hidden from the caller, there isn't any way for
a device driver to explicitly override the accessor functions if it
needs to.  One of the things I like about the generic irq_chip work
that tglx did was that it set up a lot of things for the irq
controller, but the controller could still provide its own hooks when
necessary.

> +
> +	if (!map->format.format_write &&
> +	    !(map->format.format_reg && map->format.format_val))
> +		goto err_map;
> +
> +	/* Figure out which bus to use, and also grab a lock on the
> +	 * module supplying it. */
> +	mutex_lock(&regmap_bus_lock);
> +	list_for_each_entry(map->bus, &regmap_bus_list, list)
> +		if (map->bus->type == dev->bus &&
> +		    try_module_get(map->bus->owner))
> +			break;
> +	mutex_unlock(&regmap_bus_lock);

Hmmmm.  Two things here.  First, grabbing a reference to the module
mostly works, but it highlights a deficiency in the driver model.
Really what we want here is a refcount on the /bound/ instance of the
driver+device.  The driver model does actually support unbinding a
device without removing the module.

Second, combining a device reference with the register access library
conflates two things which seem to me to be separate.  Actually, which
it seems odd, it doesn't bother me too much, but it looks completely
wrong for this library to need specific bus-type knowledge.  Why does
the bus type need to be known (other than to choose the appropriate IO
routines)?  If it is only for setting up the io accessors, I'd rather
see the caller use a bus-specific setup funtion and pass the
appropriate {i2c,spi,...}_device which will take care of type
checking.

> +
> +	if (map->bus == NULL) {
> +		ret = -EINVAL;
> +		goto err_map;
> +	}
> +
> +	map->work_buf = kmalloc(map->format.buf_size, GFP_KERNEL);
> +	if (map->work_buf == NULL) {
> +		ret = -ENOMEM;
> +		goto err_bus;
> +	}
> +
> +	return map;
> +
> +err_bus:
> +	module_put(map->bus->owner);
> +err_map:
> +	kfree(map);
> +err:
> +	return ERR_PTR(ret);

Personal soapbox: I'm not at all a fan of the ERR_PTR() pattern
because it provides no clues to the caller (gcc) if a failure is
returned as a NULL or as a 0.  Unless there is a really strong
argument for needing to differentiate between "no data" and "things
went bad", I'd rather see APIs that return NULL on failure.

> +}
> +EXPORT_SYMBOL_GPL(regmap_init);
> +
> +/**
> + * regmap_exit: Free a previously allocated register map
> + */
> +void regmap_exit(struct regmap *map)
> +{
> +	kfree(map->work_buf);
> +	module_put(map->bus->owner);
> +	kfree(map);
> +}
> +EXPORT_SYMBOL_GPL(regmap_exit);
> +
> +static int _regmap_raw_write(struct regmap *map, unsigned int reg,
> +			     const void *val, size_t val_len)
> +{
> +	void *buf;
> +	int ret = -ENOTSUPP;
> +	size_t len;
> +
> +	map->format.format_reg(map->work_buf, reg);
> +
> +	/* Try to do a gather write if we can */
> +	if (map->bus->gather_write)
> +		ret = map->bus->gather_write(map->dev, map->work_buf,
> +					     map->format.reg_bytes,
> +					     val, val_len);
> +
> +	/* Otherwise fall back on linearising by hand. */
> +	if (ret == -ENOTSUPP) {
> +		len = map->format.reg_bytes + val_len;
> +		buf = kmalloc(len, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +
> +		memcpy(buf, map->work_buf, map->format.reg_bytes);
> +		memcpy(buf + map->format.reg_bytes, val, val_len);
> +		ret = map->bus->write(map->dev, buf, len);
> +
> +		kfree(buf);
> +	}
> +
> +	return ret;
> +}
> +
> +static int _regmap_write(struct regmap *map, unsigned int reg,
> +			 unsigned int val)
> +{
> +	BUG_ON(!map->format.format_write && !map->format.format_val);
> +
> +	if (map->format.format_write) {
> +		map->format.format_write(map, reg, val);
> +
> +		return map->bus->write(map->dev, map->work_buf,
> +				       map->format.buf_size);
> +	} else {
> +		map->format.format_val(map->work_buf + map->format.reg_bytes,
> +				       val);
> +		return _regmap_raw_write(map, reg,
> +					 map->work_buf + map->format.reg_bytes,
> +					 map->format.val_bytes);
> +	}
> +}
> +
> +/**
> + * regmap_write: Write a value to a single register
> + *
> + * @map: Register map to write to
> + * @reg: Register to write to
> + * @val: Value to be written
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +int regmap_write(struct regmap *map, unsigned int reg, unsigned int val)
> +{
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +
> +	ret = _regmap_write(map, reg, val);
> +
> +	mutex_unlock(&map->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_write);
> +
> +/**
> + * regmap_raw_write: Write raw values to one or more registers
> + *
> + * @map: Register map to write to
> + * @reg: Initial register to write to
> + * @val: Block of data to be written, laid out for direct transmission to the
> + *       device
> + * @val_len: Length of data pointed to by val.
> + *
> + * This function is intended to be used for things like firmware
> + * download where a large block of data needs to be transferred to the
> + * device.  No formatting will be done on the data provided.
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +int regmap_raw_write(struct regmap *map, unsigned int reg,
> +		     const void *val, size_t val_len)
> +{
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +
> +	ret = _regmap_raw_write(map, reg, val, val_len);
> +
> +	mutex_unlock(&map->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_raw_write);
> +
> +static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> +			    unsigned int val_len)
> +{
> +	u8 *u8 = map->work_buf;
> +	int ret;
> +
> +	map->format.format_reg(map->work_buf, reg);
> +
> +	/*
> +	 * Some buses flag reads by setting the high bits in the
> +	 * register addresss; since it's always the high bits for all
> +	 * current formats we can do this here rather than in
> +	 * formatting.  This may break if we get interesting formats.
> +	 */
> +	if (map->bus->read_flag_mask)
> +		u8[0] |= map->bus->read_flag_mask;
> +
> +	ret = map->bus->read(map->dev, map->work_buf, map->format.reg_bytes,
> +			     val, map->format.val_bytes);
> +	if (ret != 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int _regmap_read(struct regmap *map, unsigned int reg,
> +			unsigned int *val)
> +{
> +	int ret;
> +
> +	if (!map->format.parse_val)
> +		return -EINVAL;
> +
> +	ret = _regmap_raw_read(map, reg, map->work_buf, map->format.val_bytes);
> +	if (ret == 0)
> +		*val = map->format.parse_val(map->work_buf);
> +
> +	return ret;
> +}
> +
> +/**
> + * regmap_read: Read a value from a single register
> + *
> + * @map: Register map to write to
> + * @reg: Register to be read from
> + * @val: Pointer to store read value
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
> +{
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +
> +	ret = _regmap_read(map, reg, val);
> +
> +	mutex_unlock(&map->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_read);
> +
> +/**
> + * regmap_raw_read: Read raw data from the device
> + *
> + * @map: Register map to write to
> + * @reg: First register to be read from
> + * @val: Pointer to store read value
> + * @val_len: Size of data to read
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +int regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> +		    size_t val_len)
> +{
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +
> +	ret = _regmap_raw_read(map, reg, val, val_len);
> +
> +	mutex_unlock(&map->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_raw_read);
> +
> +/**
> + * regmap_bulk_read: Read multiple registers from the device
> + *
> + * @map: Register map to write to
> + * @reg: First register to be read from
> + * @val: Pointer to store read value, in native register size for device
> + * @val_count: Number of registers to read
> + *
> + * A value of zero will be returned on success, a negative errno will
> + * be returned in error cases.
> + */
> +int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
> +		     size_t val_count)
> +{
> +	int ret, i;
> +	size_t val_bytes = map->format.val_bytes;
> +
> +	if (!map->format.parse_val)
> +		return -EINVAL;
> +
> +	ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
> +	if (ret != 0)
> +		return ret;
> +
> +	for (i = 0; i < val_count * val_bytes; i += val_bytes)
> +		map->format.parse_val(val + i);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(regmap_bulk_read);
> +
> +/**
> + * remap_update_bits: Perform a read/modify/write cycle on the register map
> + *
> + * @map: Register map to update
> + * @reg: Register to update
> + * @mask: Bitmask to change
> + * @val: New value for bitmask
> + *
> + * Returns zero for success, a negative number on error.
> + */
> +int regmap_update_bits(struct regmap *map, unsigned int reg,
> +		       unsigned int mask, unsigned int val)
> +{
> +	int ret;
> +	unsigned int tmp;
> +
> +	mutex_lock(&map->lock);
> +
> +	ret = _regmap_read(map, reg, &tmp);
> +	if (ret != 0)
> +		goto out;
> +
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +
> +	ret = _regmap_write(map, reg, tmp);
> +
> +out:
> +	mutex_unlock(&map->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(regmap_update_bits);
> +
> +void regmap_add_bus(struct regmap_bus *bus)
> +{
> +	mutex_lock(&regmap_bus_lock);
> +	list_add(&bus->list, &regmap_bus_list);
> +	mutex_unlock(&regmap_bus_lock);
> +}
> +EXPORT_SYMBOL_GPL(regmap_add_bus);
> +
> +void regmap_del_bus(struct regmap_bus *bus)
> +{
> +	mutex_lock(&regmap_bus_lock);
> +	list_del(&bus->list);
> +	mutex_unlock(&regmap_bus_lock);
> +}
> +EXPORT_SYMBOL_GPL(regmap_del_bus);
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> new file mode 100644
> index 0000000..b033979
> --- /dev/null
> +++ b/include/linux/regmap.h
> @@ -0,0 +1,76 @@
> +#ifndef __LINUX_REGMAP_H
> +#define __LINUX_REGMAP_H
> +
> +/*
> + * Register map access API
> + *
> + * Copyright 2011 Wolfson Microelectronics plc
> + *
> + * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> +
> +struct regmap_config {
> +	int reg_bits;
> +	int val_bits;
> +};
> +
> +typedef int (*regmap_hw_write)(struct device *dev, const void *data,
> +			       size_t count);
> +typedef int (*regmap_hw_gather_write)(struct device *dev,
> +				      const void *reg, size_t reg_len,
> +				      const void *val, size_t val_len);
> +typedef int (*regmap_hw_read)(struct device *dev,
> +			      const void *reg_buf, size_t reg_size,
> +			      void *val_buf, size_t val_size);
> +
> +/**
> + * Description of a hardware bus for the register map infrastructure.
> + *
> + * @list: Internal use.
> + * @type: Bus type, used to identify bus to be used for a device.
> + * @write: Write operation.
> + * @gather_write: Write operation with split register/value, return -ENOTSUPP
> + *                if not implemented  on a given device.
> + * @read: Read operation.  Data is returned in the buffer used to transmit
> + *         data.
> + * @owner: Module with the bus implementation, used to pin the implementation
> + *         in memory.
> + * @read_flag_mask: Mask to be set in the top byte of the register when doing
> + *                  a read.
> + */
> +struct regmap_bus {
> +	struct list_head list;
> +	struct bus_type *type;
> +	regmap_hw_write write;
> +	regmap_hw_gather_write gather_write;
> +	regmap_hw_read read;
> +	struct module *owner;
> +	u8 read_flag_mask;
> +};

Further to my comment from earlier, the need for a linked list of bus
types goes away if the driver calls a bus-specific setup routine, and
there doesn't need to be any regmap add/del management.

> +
> +struct regmap *regmap_init(struct device *dev,
> +			   const struct regmap_config *config);
> +void regmap_exit(struct regmap *map);
> +int regmap_write(struct regmap *map, unsigned int reg, unsigned int val);
> +int regmap_raw_write(struct regmap *map, unsigned int reg,
> +		     const void *val, size_t val_len);
> +int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val);
> +int regmap_raw_read(struct regmap *map, unsigned int reg,
> +		    void *val, size_t val_len);
> +int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
> +		     size_t val_count);
> +int regmap_update_bits(struct regmap *map, unsigned int reg,
> +		       unsigned int mask, unsigned int val);
> +
> +void regmap_add_bus(struct regmap_bus *bus);
> +void regmap_del_bus(struct regmap_bus *bus);
> +
> +#endif
> -- 
> 1.7.5.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/4] regmap: Add generic non-memory mapped register access API

[Mark Brown]

On Thu, Jul 14, 2011 at 08:53:26PM -0600, Grant Likely wrote:

Please delete unneeded context, I may have missed some of your comments
due to skimming.

> > +struct regmap {
> > +	struct mutex lock;
> > +
> > +	struct device *dev; /* Device we do I/O on */
> > +	void *work_buf;     /* Scratch buffer used to format I/O */
> > +	struct regmap_format format;  /* Buffer format */
> > +	const struct regmap_bus *bus;
> > +};

> Some /** kerneldoc on these structures would be helpful.

I didn't kerneldoc it because it's internal only and shouldn't appear in
the API reference for the API.

> > +/**
> > + * remap_init: Initialise register map

> nit: "remap_init() - Initialise reigster map"

> > +struct regmap *regmap_init(struct device *dev,
> > +			   const struct regmap_config *config)
> > +{
> > +	struct regmap *map;
> > +	int ret = -EINVAL;
> > +
> > +	map = kzalloc(sizeof(*map), GFP_KERNEL);
> > +	if (map == NULL) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}

> Wouldn't it be appropriate to allow drivers to embed the regmap in a
> private data structure?

Not until we're more confident of the internals, I'd rather not have to
worry about what people are doing with them until they seem more stable.

> Rather than a big case statement, could this be cleaner with a lookup
> table?

Yeah, I did it that way initially but it got annoying to write.  I'm
swithering about going back to that, though.  The problem was the 4x12
and 7x9 cases.  I don't really like either way and this just happened to
be the last one that got implemented.

> I'm a bit concerned that the overall structure of the init function
> will be inflexible in the long run.  Specifically, the way the

That's fine, we can rewrite it.  Like I said I'm not

> instance structure is hidden from the caller, there isn't any way for
> a device driver to explicitly override the accessor functions if it
> needs to.  One of the things I like about the generic irq_chip work
> that tglx did was that it set up a lot of things for the irq
> controller, but the controller could still provide its own hooks when
> necessary.

This is mostly the API internals thing again.

For overrides my plan was to let drivers set the hooks in the
regmap_config they pass in, but I'd like to try to get as far as we can
without needing to do that.  Specifically I'd like to get the cache
stuff in before exposing the internals as I think that that may suggest
some refactorings.

> > +	/* Figure out which bus to use, and also grab a lock on the
> > +	 * module supplying it. */
> > +	mutex_lock(&regmap_bus_lock);
> > +	list_for_each_entry(map->bus, &regmap_bus_list, list)
> > +		if (map->bus->type == dev->bus &&
> > +		    try_module_get(map->bus->owner))
> > +			break;
> > +	mutex_unlock(&regmap_bus_lock);

> Hmmmm.  Two things here.  First, grabbing a reference to the module
> mostly works, but it highlights a deficiency in the driver model.
> Really what we want here is a refcount on the /bound/ instance of the
> driver+device.  The driver model does actually support unbinding a
> device without removing the module.

I know, but all the other subsystems are doing it :)

> Second, combining a device reference with the register access library
> conflates two things which seem to me to be separate.  Actually, which
> it seems odd, it doesn't bother me too much, but it looks completely
> wrong for this library to need specific bus-type knowledge.  Why does
> the bus type need to be known (other than to choose the appropriate IO
> routines)?  If it is only for setting up the io accessors, I'd rather
> see the caller use a bus-specific setup funtion and pass the
> appropriate {i2c,spi,...}_device which will take care of type
> checking.

It's for the I/O accessors at present, though it will also get used for
trace and diagnostics - experience with ASoC has been that it's very
useful to have an off the shelf set of register I/O trace and dump
facilities that you can turn on easily.  For example, once we get the
drivers telling us more about their register maps I'd expect to
replicate the ASoC debugfs dump access.  For that stuff being able to
say what device we're talking about is obviously helpful.

The reasoning behind the I/O is that for all the I2C/SPI devices you
usually end up with a common probe()/exit() function which gets called
by the bus-specific ones and doing things this way means that for most
devices we move an extra call and check block from the bus specific code
into the common code.

> > +err:
> > +	return ERR_PTR(ret);

> Personal soapbox: I'm not at all a fan of the ERR_PTR() pattern
> because it provides no clues to the caller (gcc) if a failure is
> returned as a NULL or as a 0.  Unless there is a really strong
> argument for needing to differentiate between "no data" and "things
> went bad", I'd rather see APIs that return NULL on failure.

We should just switch to C++ and use exceptions :)

Re: [PATCH 1/4] regmap: Add generic non-memory mapped register access API

[Grant Likely]

On Thu, Jul 14, 2011 at 9:25 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Jul 14, 2011 at 08:53:26PM -0600, Grant Likely wrote:
>> > +err:
>> > +   return ERR_PTR(ret);
>
>> Personal soapbox: I'm not at all a fan of the ERR_PTR() pattern
>> because it provides no clues to the caller (gcc) if a failure is
>> returned as a NULL or as a 0.  Unless there is a really strong
>> argument for needing to differentiate between "no data" and "things
>> went bad", I'd rather see APIs that return NULL on failure.
>
> We should just switch to C++ and use exceptions :)

heeheehee.

g.

[PATCH 0/4] regmap: Generic I2C and SPI register map library

[Mark Brown]

This is a version of a series I've been posting for most of this release
cycle which abstracts out a bunch of widely duplicated code for doing
register I/O on devices with control slow buses like I2C and SPI.  As
well as saving us code for the register I/O itself this should also make
it easier to factor out further higher level code for this class of
devices.

The main change in this version of the series is that I've moved the
bus-specific files into the relevant drivers/ directories.  Hopefully
Jean, Ben and Grant will be OK with this and with some churn in this
code from the regmap tree that may be created as we build out the core
API and possibly refactor the interface to the bus code.

I realise it's a bit late in the cycle but it'd be really good if we
could get this merged in the next merge window, or accepted for about
then so we can create a stable branch we can merge into subsystem trees
in order to get drivers using this merged for 3.2.  As there's only one
user included the series it should be pretty low risk.

The code is in a subdirectory because I'm anticipating it'll grow quite
a bit as we add facilities like fancy cache mechanisms on top of it.

A more verbose cover letter:

Many I2C and SPI based devices implement register maps on top of the raw
wire interface.  This is generally done in a very standard fashion by
devices, resulting in a lot of very similar code in drivers.  For some
time now ASoC has factored this code out into the subsystem but that's
only useful for audio devices.  The intention with this series is to
generalise the concept so that it can be used throughout the kernel.

It's not intended that this be suitable for all devices - some devices
have things that are hard to generalise like registers with variable
size and paging which are hard to support genericly.  At the minute the
code is focused on the common cases.  It is likely that the same code
could be used with other buses with similar properties to I2C and SPI.

Currently only physical I/O is handled, the intention is that once this
support has been reviewed and merged the generic register cache code
that ASoC includes will also be factored out too, along with logging and
diagnostic infrastructure for register maps.  For devices with read
heavy workloads (especially those that need a lot of read/modify/write
cycles) or which don't support read at all this can provide a useful
performance improvement and for sparse register maps there's a lot of
benefit in relatively complex cache code.

I'm not entirely happy with the implementation currently but am fairly
happy with the external interfaces.

A bunch more drivers have been converted to use this framework,
including the existing ASoC register I/O code, but due to various -next
dependencies these don't apply easily so I've omitted them from this
posting of the series - you can see them in the mailing list archives,
they've not been updated during the review.

There's a git branch at:

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

I've also created a regmap-asoc branch there which merges current ASoC
code with the regmap code.

The following changes since commit 620917de59eeb934b9f8cf35cc2d95c1ac8ed0fc:

  Linux 3.0-rc7 (2011-07-11 16:51:52 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Mark Brown (4):
      regmap: Add generic non-memory mapped register access API
      regmap: Add I2C bus support
      regmap: Add SPI bus support
      regulator: Convert tps65023 to use regmap API

 MAINTAINERS                            |    7 +
 drivers/base/Kconfig                   |    2 +
 drivers/base/Makefile                  |    1 +
 drivers/base/regmap/Kconfig            |    6 +
 drivers/base/regmap/Makefile           |    1 +
 drivers/base/regmap/regmap.c           |  481 ++++++++++++++++++++++++++++++++
 drivers/i2c/Kconfig                    |    4 +
 drivers/i2c/Makefile                   |    1 +
 drivers/i2c/i2c-regmap.c               |  111 ++++++++
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   98 ++-----
 drivers/spi/Kconfig                    |    4 +
 drivers/spi/Makefile                   |    1 +
 drivers/spi/spi-regmap.c               |   76 +++++
 include/linux/regmap.h                 |   76 +++++
 15 files changed, 799 insertions(+), 71 deletions(-)
 create mode 100644 drivers/base/regmap/Kconfig
 create mode 100644 drivers/base/regmap/Makefile
 create mode 100644 drivers/base/regmap/regmap.c
 create mode 100644 drivers/i2c/i2c-regmap.c
 create mode 100644 drivers/spi/spi-regmap.c
 create mode 100644 include/linux/regmap.h

[PATCH 0/4] regmap: Generic I2C and SPI register map library

[Mark Brown]

This is a version of a series I've been posting for most of this release
cycle which abstracts out a bunch of widely duplicated code for doing
register I/O on devices with control slow buses like I2C and SPI.  As
well as saving us code for the register I/O itself this should also make
it easier to factor out further higher level code for this class of
devices.

This version of the series includes a number of fairly small updates
from review by Grant and Jean and also a moderately invasive change to
have bus-specific registration functions at Grant's request.

I realise it's a bit late in the cycle but it'd be really good if we
could get this merged in the next merge window, or accepted for about
then so we can create a stable branch we can merge into subsystem trees
in order to get drivers using this merged for 3.2.  As there's only one
user included the series it should be pretty low risk.

The code is in a subdirectory because I'm anticipating it'll grow quite
a bit as we add facilities like fancy cache mechanisms on top of it.

A more verbose cover letter:

Many I2C and SPI based devices implement register maps on top of the raw
wire interface.  This is generally done in a very standard fashion by
devices, resulting in a lot of very similar code in drivers.  For some
time now ASoC has factored this code out into the subsystem but that's
only useful for audio devices.  The intention with this series is to
generalise the concept so that it can be used throughout the kernel.

It's not intended that this be suitable for all devices - some devices
have things that are hard to generalise like registers with variable
size and paging which are hard to support genericly.  At the minute the
code is focused on the common cases.  It is likely that the same code
could be used with other buses with similar properties to I2C and SPI.

Currently only physical I/O is handled, the intention is that once this
support has been reviewed and merged the generic register cache code
that ASoC includes will also be factored out too, along with logging and
diagnostic infrastructure for register maps.  For devices with read
heavy workloads (especially those that need a lot of read/modify/write
cycles) or which don't support read at all this can provide a useful
performance improvement and for sparse register maps there's a lot of
benefit in relatively complex cache code.
 
I'm not entirely happy with the implementation currently but am fairly
happy with the external interfaces.

A bunch more drivers have been converted to use this framework,
including the existing ASoC register I/O code, but due to various -next
dependencies these don't apply easily so I've omitted them from this
posting of the series - you can see them in the mailing list archives,
they've not been updated during the review.

There's a git branch at:

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

I've also created a regmap-asoc branch there which merges current ASoC
code with the regmap code.

The following changes since commit 620917de59eeb934b9f8cf35cc2d95c1ac8ed0fc:

  Linux 3.0-rc7 (2011-07-11 16:51:52 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Mark Brown (4):
      regmap: Add generic non-memory mapped register access API
      regmap: Add I2C bus support
      regmap: Add SPI bus support
      regulator: Convert tps65023 to use regmap API

 MAINTAINERS                            |    9 +
 drivers/base/Kconfig                   |    2 +
 drivers/base/Makefile                  |    1 +
 drivers/base/regmap/Kconfig            |    6 +
 drivers/base/regmap/Makefile           |    1 +
 drivers/base/regmap/regmap.c           |  455 ++++++++++++++++++++++++++++++++
 drivers/i2c/Kconfig                    |    4 +
 drivers/i2c/Makefile                   |    1 +
 drivers/i2c/i2c-regmap.c               |  115 ++++++++
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   97 ++-----
 drivers/spi/Kconfig                    |    4 +
 drivers/spi/Makefile                   |    1 +
 drivers/spi/spi-regmap.c               |   73 +++++
 include/linux/regmap.h                 |   79 ++++++
 15 files changed, 779 insertions(+), 70 deletions(-)
 create mode 100644 drivers/base/regmap/Kconfig
 create mode 100644 drivers/base/regmap/Makefile
 create mode 100644 drivers/base/regmap/regmap.c
 create mode 100644 drivers/i2c/i2c-regmap.c
 create mode 100644 drivers/spi/spi-regmap.c
 create mode 100644 include/linux/regmap.h

[PATCH 0/4] regmap: Generic I2C and SPI register map library

[Mark Brown]

This is a version of a series I've been posting for most of this release
cycle which abstracts out a bunch of widely duplicated code for doing
register I/O on devices with control slow buses like I2C and SPI.  As
well as saving us code for the register I/O itself this should also make
it easier to factor out further higher level code for this class of
devices.

The only update in this version is that I've moved the bus-specific code
back into the regmap directory.

I realise it's a bit late in the cycle but it'd be really good if we
could get this merged in the next merge window, or accepted for about
then so we can create a stable branch we can merge into subsystem trees
in order to get drivers using this merged for 3.2.  As there's only one
user included the series it should be pretty low risk.

The code is in a subdirectory because I'm anticipating it'll grow quite
a bit as we add facilities like fancy cache mechanisms on top of it.

A more verbose cover letter:

Many I2C and SPI based devices implement register maps on top of the raw
wire interface.  This is generally done in a very standard fashion by
devices, resulting in a lot of very similar code in drivers.  For some
time now ASoC has factored this code out into the subsystem but that's
only useful for audio devices.  The intention with this series is to
generalise the concept so that it can be used throughout the kernel.

It's not intended that this be suitable for all devices - some devices
have things that are hard to generalise like registers with variable
size and paging which are hard to support genericly.  At the minute the
code is focused on the common cases.  It is likely that the same code
could be used with other buses with similar properties to I2C and SPI.

Currently only physical I/O is handled, the intention is that once this
support has been reviewed and merged the generic register cache code
that ASoC includes will also be factored out too, along with logging and
diagnostic infrastructure for register maps.  For devices with read
heavy workloads (especially those that need a lot of read/modify/write
cycles) or which don't support read at all this can provide a useful
performance improvement and for sparse register maps there's a lot of
benefit in relatively complex cache code.
 
I'm not entirely happy with the implementation currently but am fairly
happy with the external interfaces.

A bunch more drivers have been converted to use this framework,
including the existing ASoC register I/O code, but due to various -next
dependencies these don't apply easily so I've omitted them from this
posting of the series - you can see them in the mailing list archives,
they've not been updated during the review.
 
There's a git branch at:

    git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

I've also created a regmap-asoc branch there which merges current ASoC
code with the regmap code.

The following changes since commit 620917de59eeb934b9f8cf35cc2d95c1ac8ed0fc:

  Linux 3.0-rc7 (2011-07-11 16:51:52 -0700)

are available in the git repository at:
  git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next

Mark Brown (4):
      regmap: Add generic non-memory mapped register access API
      regmap: Add I2C bus support
      regmap: Add SPI bus support
      regulator: Convert tps65023 to use regmap API

 MAINTAINERS                            |    9 +
 drivers/base/Kconfig                   |    2 +
 drivers/base/Makefile                  |    1 +
 drivers/base/regmap/Kconfig            |   14 +
 drivers/base/regmap/Makefile           |    3 +
 drivers/base/regmap/regmap-i2c.c       |  115 ++++++++
 drivers/base/regmap/regmap-spi.c       |   73 +++++
 drivers/base/regmap/regmap.c           |  455 ++++++++++++++++++++++++++++++++
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   97 ++-----
 include/linux/regmap.h                 |   79 ++++++
 11 files changed, 779 insertions(+), 70 deletions(-)
 create mode 100644 drivers/base/regmap/Kconfig
 create mode 100644 drivers/base/regmap/Makefile
 create mode 100644 drivers/base/regmap/regmap-i2c.c
 create mode 100644 drivers/base/regmap/regmap-spi.c
 create mode 100644 drivers/base/regmap/regmap.c
 create mode 100644 include/linux/regmap.h