Generic I2C and SPI register map library

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 semi-random set of driver conversions have been included in the series with various degrees of testing including the subsystem wide ASoC code. Far more drivers could use the code, these have mostly just been done as samples in a minimally invasive fashion.

There's a git branch at:

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

but due to various -next dependencies this does not currently include all of the patches posted here. I've also created a regmap-asoc branch there which merges current ASoC code with the regmap code.

 MAINTAINERS                            |    7 +
 drivers/Kconfig                        |    1 +
 drivers/Makefile                       |    1 +
 drivers/mfd/Kconfig                    |    4 +
 drivers/mfd/pcf50633-core.c            |  114 ++------
 drivers/mfd/wm831x-core.c              |   97 +++----
 drivers/mfd/wm831x-i2c.c               |   56 ----
 drivers/mfd/wm831x-spi.c               |   48 ----
 drivers/mfd/wm8994-core.c              |  175 ++----------
 drivers/regmap/Kconfig                 |    6 +
 drivers/regmap/Makefile                |    3 +
 drivers/regmap/regmap-i2c.c            |  112 ++++++++
 drivers/regmap/regmap-spi.c            |   75 +++++
 drivers/regmap/regmap.c                |  478 ++++++++++++++++++++++++++++++++
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   98 ++-----
 include/linux/mfd/pcf50633/core.h      |    3 +-
 include/linux/mfd/wm831x/core.h        |    7 +-
 include/linux/mfd/wm8994/core.h        |    9 +-
 include/linux/regmap.h                 |   75 +++++
 include/sound/soc.h                    |    2 +
 sound/soc/Kconfig                      |    1 +
 sound/soc/soc-io.c                     |  316 ++-------------------
 23 files changed, 920 insertions(+), 769 deletions(-)

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.

---
 MAINTAINERS             |    7 +
 drivers/Kconfig         |    1 +
 drivers/Makefile        |    1 +
 drivers/regmap/Kconfig  |    6 +
 drivers/regmap/Makefile |    2 +
 drivers/regmap/regmap.c |  478 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h  |   75 ++++++++
 7 files changed, 570 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regmap/Kconfig
 create mode 100644 drivers/regmap/Makefile
 create mode 100644 drivers/regmap/regmap.c
 create mode 100644 include/linux/regmap.h

index a26e9ec..bd2ec03 100644
@@ -5318,6 +5318,13 @@ L:	[email protected]
 S:	Supported
 F:	fs/reiserfs/
 
+REGISTER MAP ABSTRACTION
+M:	Mark Brown <[email protected]>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git
+S:	Supported
+F:	drivers/regmap/
+F:	include/linux/regmap.h
+
 RFKILL
 M:	Johannes Berg <[email protected]>
 L:	[email protected]

index 3bb154d..8c55790 100644
@@ -126,4 +126,5 @@ source "drivers/hwspinlock/Kconfig"
 
 source "drivers/clocksource/Kconfig"
 
+source "drivers/regmap/Kconfig"
 endmenu

index 09f3232..78ab0d6 100644
@@ -116,6 +116,7 @@ obj-$(CONFIG_BCMA)		+= bcma/
 obj-$(CONFIG_VHOST_NET)		+= vhost/
 obj-$(CONFIG_VLYNQ)		+= vlynq/
 obj-$(CONFIG_STAGING)		+= staging/
+obj-$(CONFIG_REGMAP)		+= regmap/
 obj-y				+= platform/
 obj-y				+= ieee802154/
 #common clk code

new file mode 100644
index 0000000..fc0eb1d
@@ -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

new file mode 100644
index 0000000..1e5037e
@@ -0,0 +1,2 @@
+obj-$(CONFIG_REGMAP) += regmap.o
+

1.7.5.4

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.

---
 drivers/regmap/Makefile     |    2 +-
 drivers/regmap/regmap-i2c.c |  112 +++++++++++++++++++++++++++++++++++++++++++
 drivers/regmap/regmap.c     |    4 +-
 3 files changed, 115 insertions(+), 3 deletions(-)
 create mode 100644 drivers/regmap/regmap-i2c.c

index 1e5037e..641c20a 100644
@@ -1,2 +1,2 @@
 obj-$(CONFIG_REGMAP) += regmap.o
-
+obj-$(CONFIG_I2C) += regmap-i2c.o

index a8610c7..896b710 100644
@@ -461,7 +461,7 @@ out:
 }
 EXPORT_SYMBOL_GPL(regmap_update_bits);
 
-int regmap_add_bus(struct regmap_bus *bus)
+void regmap_add_bus(struct regmap_bus *bus)
 {
 	mutex_lock(&regmap_bus_lock);
 	list_add(&bus->list, &regmap_bus_list);

@@ -469,7 +469,7 @@ int regmap_add_bus(struct regmap_bus *bus)
 }
 EXPORT_SYMBOL_GPL(regmap_add_bus);
 
-int regmap_del_bus(struct regmap_bus *bus)
+void regmap_del_bus(struct regmap_bus *bus)
 {
 	mutex_lock(&regmap_bus_lock);
 	list_del(&bus->list);

1.7.5.4

For...?

Hrm, yeah, probably not.

postcore_initcall and module_exit

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.

---
 drivers/regmap/Makefile     |    1 +
 drivers/regmap/regmap-spi.c |   75 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regmap/regmap-spi.c

index 641c20a..74c6680 100644
@@ -1,2 +1,3 @@
 obj-$(CONFIG_REGMAP) += regmap.o
 obj-$(CONFIG_I2C) += regmap-i2c.o
+obj-$(CONFIG_SPI) += regmap-spi.o

1.7.5.4

I've got this horrible feeling that if we try that we'll discover that the reason the SPI API does this internally is just as valid here - if I remember correctly it's doing this due to restrictions on DMA from the stack and I'd strongly expect val to end up on the stack for registers. Or to look at it from the other point of view if we don't need the bounce buffers then why does spi_write_then_read() need them?

hm, right, I overlooked that val could be on the stack. I though we were always using some kind of bounce buffer internally.

Remove all the ASoC specific physical I/O code and replace it with calls into the regmap API. The bulk write code can only be used safely if all regmap calls are locked with the CODEC lock, we need to add bulk support to the regmap API or replace the code with an open coded loop (though currently it has no users...).

---
 include/sound/soc.h |    2 +
 sound/soc/Kconfig   |    1 +
 sound/soc/soc-io.c  |  316 ++++-----------------------------------------------
 3 files changed, 28 insertions(+), 291 deletions(-)

index 6424b10..40436b8 100644
@@ -19,6 +19,7 @@
 #include <linux/workqueue.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
+#include <linux/regmap.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/control.h>

@@ -570,6 +571,7 @@ struct snd_soc_codec {
 	const void *reg_def_copy;
 	const struct snd_soc_cache_ops *cache_ops;
 	struct mutex cache_rw_mutex;
+	int val_bytes;
 
 	/* dapm */
 	struct snd_soc_dapm_context dapm;

index 8224db5..32e5fd4 100644
@@ -7,6 +7,7 @@ menuconfig SND_SOC
 	select SND_PCM
 	select AC97_BUS if SND_SOC_AC97_BUS
 	select SND_JACK if INPUT=y || INPUT=SND
+	select REGMAP
 	---help---
 
 	  If you want ASoC support, you should say Y here and also to the

1.7.5.4

Factor out the register read/write code to use the register map API. We still need some wm831x specific code and locking in place to check that the user key is handled correctly but only on the write side, reads are not affected by the key.

---
 drivers/mfd/Kconfig             |    2 +
 drivers/mfd/wm831x-core.c       |   97 ++++++++++++++------------------------
 drivers/mfd/wm831x-i2c.c        |   56 ----------------------
 drivers/mfd/wm831x-spi.c        |   48 -------------------
 include/linux/mfd/wm831x/core.h |    7 +--
 5 files changed, 40 insertions(+), 170 deletions(-)

index 0f09c05..487c3be 100644
@@ -367,6 +367,7 @@ config MFD_WM831X_I2C
 	bool "Support Wolfson Microelectronics WM831x/2x PMICs with I2C"
 	select MFD_CORE
 	select MFD_WM831X
+	select REGMAP
 	depends on I2C=y && GENERIC_HARDIRQS
 	help
 	  Support for the Wolfson Microelecronics WM831x and WM832x PMICs

@@ -378,6 +379,7 @@ config MFD_WM831X_SPI
 	bool "Support Wolfson Microelectronics WM831x/2x PMICs with SPI"
 	select MFD_CORE
 	select MFD_WM831X
+	select REGMAP
 	depends on SPI_MASTER && GENERIC_HARDIRQS
 	help
 	  Support for the Wolfson Microelecronics WM831x and WM832x PMICs

index 8dda8de..bd0615b 100644
@@ -18,6 +18,7 @@
 #include <linux/completion.h>
 #include <linux/interrupt.h>
 #include <linux/list.h>
+#include <linux/regmap.h>
 
 /*
  * Register values.

@@ -361,12 +362,8 @@ struct wm831x {
 	struct mutex io_lock;
 
 	struct device *dev;
-	int (*read_dev)(struct wm831x *wm831x, unsigned short reg,
-			int bytes, void *dest);
-	int (*write_dev)(struct wm831x *wm831x, unsigned short reg,
-			 int bytes, void *src);
 
-	void *control_data;
+	struct regmap *regmap;
 
 	int irq;  /* Our chip IRQ */
 	struct mutex irq_lock;

1.7.5.4

---
 drivers/mfd/Kconfig             |    1 +
 drivers/mfd/wm8994-core.c       |  175 +++++++--------------------------------
 include/linux/mfd/wm8994/core.h |    9 +--
 3 files changed, 34 insertions(+), 151 deletions(-)

index 487c3be..8d788ee 100644
@@ -453,6 +453,7 @@ config MFD_WM8350_I2C
 config MFD_WM8994
 	bool "Support Wolfson Microelectronics WM8994"
 	select MFD_CORE
+	select REGMAP
 	depends on I2C=y && GENERIC_HARDIRQS
 	help
 	  The WM8994 is a highly integrated hi-fi CODEC designed for

index bfb221b..6268091 100644
@@ -25,6 +25,7 @@ enum wm8994_type {
 
 struct regulator_dev;
 struct regulator_bulk_data;
+struct regmap;
 
 #define WM8994_NUM_GPIO_REGS 11
 #define WM8994_NUM_LDO_REGS   2

@@ -51,18 +52,12 @@ struct regulator_bulk_data;
 #define WM8994_IRQ_GPIO(x) (x + WM8994_IRQ_TEMP_WARN)
 
 struct wm8994 {
-	struct mutex io_lock;
 	struct mutex irq_lock;
 
 	enum wm8994_type type;
 
 	struct device *dev;
-	int (*read_dev)(struct wm8994 *wm8994, unsigned short reg,
-			int bytes, void *dest);
-	int (*write_dev)(struct wm8994 *wm8994, unsigned short reg,
-			 int bytes, const void *src);
-
-	void *control_data;
+	struct regmap *regmap;
 
 	int gpio_base;
 	int irq_base;

1.7.5.4

---
 drivers/mfd/Kconfig               |    1 +
 drivers/mfd/pcf50633-core.c       |  114 +++++++++---------------------------
 include/linux/mfd/pcf50633/core.h |    3 +-
 3 files changed, 32 insertions(+), 86 deletions(-)

index 8d788ee..e0951c6 100644
@@ -466,6 +466,7 @@ config MFD_WM8994
 config MFD_PCF50633
 	tristate "Support for NXP PCF50633"
 	depends on I2C
+	select REGMAP
 	help
 	  Say yes here if you have NXP PCF50633 chip on your board.
 	  This core driver provides register access and IRQ handling

index 50d4a04..a808407 100644
@@ -21,6 +21,7 @@
 #include <linux/mfd/pcf50633/backlight.h>
 
 struct pcf50633;
+struct regmap;
 
 #define PCF50633_NUM_REGULATORS	11
 

@@ -134,7 +135,7 @@ enum {
 
 struct pcf50633 {
 	struct device *dev;
-	struct i2c_client *i2c_client;
+	struct regmap *regmap;
 
 	struct pcf50633_platform_data *pdata;
 	int irq;

1.7.5.4

---
 drivers/regulator/Kconfig              |    1 +
 drivers/regulator/tps65023-regulator.c |   98 +++++++++-----------------------
 2 files changed, 28 insertions(+), 71 deletions(-)

index d7ed20f..c8dc5f0 100644
@@ -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.

1.7.5.4

Very nice!

	*out = cpu_to_be16((reg << map->format.val_bits) | val);

Hrm, yeah.

I'd rather add it when we need it, it's not much work to refactor and it's entirely in this file. Right now I like being sure I know what knows about what bit of data mangling.

I'm not sure it's worth it for the two cases we know about, and as with passing the map into these I like keeping the byte oriented assumption in the interfaces as much as possible as it makes things easier to think about.

I like init, especially considering the plan to add cache support as there's more work in setting that up once you start doing the advanced caches.

Can you say what device this is? I'm just going on the devices we've seen in the kernel so far here... Still easy enough to do.

Sure, though for stuff like this I'd say just implement them when we need them.

Oh, meh. It used to.

I thought about that but what it actually ends up meaning for most of the current users is that they then need to either implement their own bounce buffer or do some cross tree updates to switch from using the native type to ints. Neither seemed terribly appealing and it did seem reasonable to expect devices to know how big their own registers are.

It doesn't seem particularly appropriate as we're working with a bit count not the usual byte count where size_t gets used and I can't see it ever making any difference.

It's painful to implement efficiently due to the need to mangle the data on the way down, and most of the time if you're doing a bulk operation it's because you want it to be efficient. I'm punting it until someone really wants it so we can have a separate think about what makes sense.

Hm, ok I guess we can wait with this until there are actually other similar formats which follow this scheme.

If you take a look at other kernel apis _alloc is usually used if the structure is allocated (and initialized) inside the function and _init is used when the function initializes an already existing structure. And it also matches better with regmap_free.

The ad1836 for example really only uses 10 bits for data instead of 12. The 11th bit is reserved and the 12th bit is used to indicate whether it is an read or write. Though since gaps between register and data are not supported data was made 12 bits wide and the upper 2 bits are always set to 0.

The adav801, which is going to be submitted upstream soon, uses a similar scheme. First seven bits are register address, then 1 bit which indicates read/write and then 8 bits data. We are currently using the same trick here and made data 9 bits wide.

I suggested it because the byte count in the regmap_format struct is size_t and it calculated from the bit count.

That more applies to split alloc/init models - here there's a single operation that does both. To me alloc() generally means that it just allocates a structure and leaves initialization still to be done while.

That's true.

In the soc-cache code we use _init and _exit respectively. Might be worth considering instead of _free?

Thanks,
Dimitris

Err, yes - that's what I said :)

Yes.