Generic I2C and SPI register map library

[This reposting of the series should address all the review comments on the series so far.]

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.

Mark Brown (8):
regmap: Add generic non-memory mapped register access API
regmap: Add I2C bus support
regmap: Add SPI bus support
ASoC: Use new register map API for ASoC generic physical I/O
mfd: Convert WM831x to use regmap API
mfd: Convert WM8994 to use new register map API
mfd: Convert pcf50633 to use new register map API
regulator: Convert tps65023 to use regmap API

 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            |  113 ++++++++
 drivers/regmap/regmap-spi.c            |   75 +++++
 drivers/regmap/regmap.c                |  484 ++++++++++++++++++++++++++++++++
 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                 |   77 +++++
 include/sound/soc.h                    |    2 +
 sound/soc/Kconfig                      |    1 +
 sound/soc/soc-io.c                     |  316 ++-------------------
 23 files changed, 929 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 |  484 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h  |   77 ++++++++
 7 files changed, 578 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 |  113 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+), 1 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

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     |    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

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 6b0babc..9782296 100644
@@ -389,6 +389,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

@@ -400,6 +401,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 9782296..97e01b4 100644
@@ -475,6 +475,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 97e01b4..c5edb8b 100644
@@ -488,6 +488,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 a1dffd9..5e8707a 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

The code has the sematic I intended; you can debate if it's the best semantic and looking at the documentation again it's not entirely clear so at least that should be updated.

Well, then the spi regmap driver needs to use read_flag_bit = 7 instead of read_flag_bit = 8 and the BUG_ON should also be updated to read_flag_bit >= 8.

And the current code also disallows to use the first bit as the read_flag. So in my opinion using a one-based index here is a good idea.

It's already on my todo list.

Bah, so it should.

Disallowing that case was half the intention, the main issue being that if you're flagging in the bottom bit of the top byte on the bus then you'll be in the middle of the bitfield used by the register which is going to confuse things.

If something really needs to use the bottom bit we probably need to add a second form of mangling which uses the bottom of the register rather than the top, or allow buses to have bus specific register formatting.

Although come to think of it this doesn't actually help your cases at all as those are per-device settings and this is a per bus setting so I'm inclined to just revert back to the original code and say that for a first pass those devices are being sufficiently odd that we don't support them, we can add support as an extension later.

On Wed, 2011-06-22 at 19:45 +0100, Mark Brown wrote:
[...]

[...]

	return be16_to_cpu(b[0]);

Missing '@' before the parameter names.

[...]

Ben Hutchings
If the facts do not conform to your theory, they must be disposed of.

sparse is silent on whatever the issue is here and the assignment is used (we both modify the buffer in place and return the result of the conversion to make life more convenient for users). Which isn't urgently tasteful but felt less tasteless than anything else I thought of. Data manging doesn't lend itself to elegance.

You're looking at an old version of the code.

It's marginal, and given that it's vanishingly rare to hit this case (essentially all the interesting controllers for this API can do the gather write) I didn't feel it was worth it for a first pass. We can always do the optimization later if someone finds it important. I suspect any controller that hits the issue is going to be slow enough to dwarf the cost of the allocation with the I/O cost.

This should be obvious, none of the relevant buses can be interacted with in anything other than process context anyway as they need to sleep.

lib feels wrong, it's going to be peering into the drivers directory too much.

drivers/base is a good point, but then the code will grow quite a bit once we start layering cache support on top of it and until the code settles down I don't really want to have to deal with cross tree issues if we decide we need to update the interface to the bus code, that's definitely a first pass thing right now. The latter is the main thing that made me do things this way rather than pushing things into the buses, it's definitely not the finished product right now internally - the client API should be reasonably stable so not have these issues.

My inclination is to start off isolating the code until we've reached the point where things are more static. I suppose we could put it in a subdirectory of drivers/base in the meantime, though. Will have a think for the next repost.

Fixed everything else, thanks.

you should look at SMBus
there seems to be quite some code to share.
in particular i2c rtc devices seem to be using SMBus functions to get the exact same semantics you are providing here.

I just added support for such a device today, and it struck me, that this API was necessary.

i did not look into it very deeply. and still dont really understand the diff between SMBus and i2c...

hopefully, the caching can be selective.
keep rtcs in mind. huge potential for throwing code away there ;)

torben Hohn

SMBus is an I2C subset with a definition for 8 bit registers layered onto it, pretty much.

If you're only using SMBus then the I2C interface includes SMBus based register support which should do what you want. With the regmap API it ought to work as a device with 8 bit registers and 8 bit values.

The kicker here is that quite a few hosts only support the smbus subset. How should that be handled here?

For 8x8 registers we *should* end up only using the SMBus subset so it should just work.