regmap: Generic I2C and SPI register map library

drivers/base/regmap/regmap-i2c.c line 15

Quoted 3 lines · Mark Brown

drivers/base/regmap/regmap-i2c.c new file +3 -0

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

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

Review

Quoted 2 lines

(quoted diff) +2 -0

+	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).

Quoted 7 lines · Wolfram Sang

drivers/base/regmap/regmap-i2c.c line 15

Quoted 3 lines · Mark Brown

drivers/base/regmap/regmap-i2c.c new file +3 -0

+#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?

Quoted 1 line · Mark Brown

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.

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

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.

Quoted 2 lines

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.

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 <[email protected]>

Acked-by Liam Girdwood <[email protected]>

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

drivers/base/regmap/Makefile +0 -0

index 641c20a..74c6680 100644

drivers/base/regmap/regmap-spi.c new file +23 -0

new file mode 100644
index 0000000..04328c7
@@ -0,0 +1,75 @@
+/*
+ * Register map access API - SPI support
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 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,

drivers/base/regmap/regmap-spi.c new file +23 -0

+}
+
+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);

Signature · 7 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 2 lines · Mark Brown

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.

drivers/base/regmap/regmap-spi.c line 33

Quoted 8 lines

drivers/base/regmap/regmap-spi.c new file +7 -0

+{
+	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.

drivers/base/regmap/regmap-spi.c line 41

Quoted 3 lines

drivers/base/regmap/regmap-spi.c new file +3 -0

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

t[0]?

Quoted 4 lines · Grant Likely

Quoted 2 lines · Mark Brown

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

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

drivers/base/regmap/regmap-spi.c new file +7 -0

+{
+   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));

Quoted 6 lines

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?

Quoted 6 lines

Quoted 4 lines

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

On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
<[email protected]> wrote:

Quoted 3 lines

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.

Quoted 6 lines

Quoted 4 lines

      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.

Quoted 2 lines · Grant Likely

On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
<[email protected]> wrote:

[Regmap API I2C bus code.]

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

      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.

Quoted 4 lines

Quoted 1 line

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.

Quoted 26 lines · Mark Brown

Quoted 2 lines · Grant Likely

On Thu, Jul 14, 2011 at 10:39 PM, Mark Brown
<[email protected]> wrote:

[Regmap API I2C bus code.]

Quoted 7 lines

Quoted 3 lines

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.

Quoted 9 lines

Quoted 6 lines

Quoted 4 lines

      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.

Quoted 2 lines

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

Signed-off-by Mark Brown <[email protected]>

Acked-by Liam Girdwood <[email protected]>

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

drivers/regulator/Kconfig +1 -0

index 065892a..8128db3 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.

drivers/regulator/tps65023-regulator.c +16 -69

index fbddc15..3fa3ba5 100644
@@ -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)
 {

drivers/regulator/tps65023-regulator.c +10 -1

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

Signature · 7 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

drivers/regulator/tps65023-regulator.c static int __devinit tps_65023_probe(struct i2c_client *client, · line 438

Quoted 2 lines · Mark Brown

drivers/regulator/tps65023-regulator.c +1 -1

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

Quoted 11 lines · Grant Likely

drivers/regulator/tps65023-regulator.c static int __devinit tps_65023_probe(struct i2c_client *client, · line 438

Quoted 2 lines · Mark Brown

drivers/regulator/tps65023-regulator.c +1 -1

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

Quoted 3 lines · Mark Brown

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.

Quoted 10 lines · Grant Likely

Quoted 3 lines · Mark Brown

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.

Quoted 2 lines

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.

Quoted 2 lines

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

Quoted 1 line · Mark Brown

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.

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 <[email protected]>

Acked-by Liam Girdwood <[email protected]>

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 <[email protected]>

Acked-by Liam Girdwood <[email protected]>

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

MAINTAINERS +7 -0

index bf0e348..0cf0f9f 100644
@@ -5343,6 +5343,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/base/regmap/
+F:	include/linux/regmap.h
+
 RFKILL
 M:	Johannes Berg <[email protected]>
 L:	[email protected]

drivers/base/Kconfig +2 -0

index fa64fa0..21cf46f 100644
@@ -172,4 +172,6 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+source "drivers/base/regmap/Kconfig"
+
 endmenu

drivers/base/Makefile +1 -0

index 4c5701c..dd4f9b2 100644
@@ -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
 

drivers/base/regmap/Kconfig new file +6 -0

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

drivers/base/regmap/Makefile new file +2 -0

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

drivers/base/regmap/regmap.c new file +33 -0

new file mode 100644
index 0000000..f6a83e9
@@ -0,0 +1,481 @@
+/*
+ * Register map access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 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);

drivers/base/regmap/regmap.c new file +37 -0

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

drivers/base/regmap/regmap.c new file +8 -0

+ *
+ * @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,

drivers/base/regmap/regmap.c new file +40 -0

+
+	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;
+	}
+

drivers/base/regmap/regmap.c new file +10 -0

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

drivers/base/regmap/regmap.c new file +290 -0

+}
+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);

include/linux/regmap.h new file +47 -0

new file mode 100644
index 0000000..b033979
@@ -0,0 +1,76 @@
+#ifndef __LINUX_REGMAP_H
+#define __LINUX_REGMAP_H
+
+/*
+ * Register map access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Mark Brown <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 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.

include/linux/regmap.h new file +19 -0

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

Signature · 7 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

drivers/base/regmap/regmap.c line 43

Quoted 8 lines

drivers/base/regmap/regmap.c new file +8 -0

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

Quoted 2 lines

+/**
+ * remap_init: Initialise register map

nit: "remap_init() - Initialise reigster map"

drivers/base/regmap/regmap.c line 108

Quoted 11 lines

drivers/base/regmap/regmap.c new file +11 -0

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

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.

Quoted 6 lines

drivers/base/regmap/regmap.c new file +6 -0

+	/* 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))

Review

Quoted 2 lines

(quoted diff) +2 -0

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

drivers/base/regmap/regmap.c line 191

Quoted 2 lines

drivers/base/regmap/regmap.c new file +2 -0

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

Quoted 8 lines · Grant Likely

Quoted 2 lines

+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 :)