regmap: Generic I2C and SPI register map library
Subsystems (5)
cover regmap: Generic I2C and SPI register map library
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 1/4 regmap: Add generic non-memory mapped register access API
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.
--- 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
@@ -5313,6 +5313,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
@@ -168,4 +168,6 @@ config SYS_HYPERVISOR bool default n +source "drivers/base/regmap/Kconfig" + endmenu
drivers/base/Makefile +1 -0
@@ -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
@@ -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
@@ -0,0 +1,2 @@ +obj-$(CONFIG_REGMAP) += regmap.o +
drivers/base/regmap/regmap.c new file +481 -0
481 lines not on this page · raw message
include/linux/regmap.h new file +76 -0
76 lines not on this page · raw message
Signature · 1 line
1.7.5.4
PATCH 2/4 regmap: Add I2C bus support
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/base/regmap/Makefile | 1 - drivers/i2c/Kconfig | 4 ++ drivers/i2c/Makefile | 1 + drivers/i2c/i2c-regmap.c | 111 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 1 deletions(-) create mode 100644 drivers/i2c/i2c-regmap.c
drivers/base/regmap/Makefile +0 -1
@@ -1,2 +1 @@ obj-$(CONFIG_REGMAP) += regmap.o -
drivers/i2c/Kconfig +4 -0
@@ -6,6 +6,7 @@ menuconfig I2C tristate "I2C support" depends on HAS_IOMEM select RT_MUTEXES + select I2C_REGMAP if REGMAP ---help--- I2C (pronounce: I-square-C) is a slow serial bus protocol used in many micro controller applications and developed by Philips. SMBus,
@@ -110,4 +111,7 @@ config I2C_DEBUG_BUS a problem with I2C support and want to see more of what is going on. +config I2C_REGMAP + tristate + endif # I2C
drivers/i2c/Makefile +1 -0
@@ -7,6 +7,7 @@ obj-$(CONFIG_I2C) += i2c-core.o obj-$(CONFIG_I2C_SMBUS) += i2c-smbus.o obj-$(CONFIG_I2C_CHARDEV) += i2c-dev.o obj-$(CONFIG_I2C_MUX) += i2c-mux.o +obj-$(CONFIG_I2C_REGMAP) += i2c-regmap.o obj-y += algos/ busses/ muxes/ ccflags-$(CONFIG_I2C_DEBUG_CORE) := -DDEBUG
drivers/i2c/i2c-regmap.c new file +111 -0
111 lines not on this page · raw message
Signature · 1 line
1.7.5.4
PATCH 3/4 regmap: Add SPI bus support
In reply to Mark BrownWe 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/spi/Kconfig | 4 ++ drivers/spi/Makefile | 1 + drivers/spi/spi-regmap.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 81 insertions(+), 0 deletions(-) create mode 100644 drivers/spi/spi-regmap.c
drivers/spi/Kconfig +4 -0
@@ -43,12 +43,16 @@ config SPI_MASTER # boolean "SPI Master Support" boolean default SPI + select SPI_REGMAP if REGMAP help If your system has an master-capable SPI controller (which provides the clock and chipselect), you can enable that controller and the protocol drivers for the SPI slave chips that are connected. +config SPI_REGMAP + boolean + if SPI_MASTER comment "SPI Master Controller Drivers"
drivers/spi/Makefile +1 -0
@@ -7,6 +7,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG # small core, mostly translating board-specific # config declarations into driver model code obj-$(CONFIG_SPI_MASTER) += spi.o +obj-$(CONFIG_SPI_REGMAP) += spi-regmap.o # SPI master controller drivers (bus) obj-$(CONFIG_SPI_ALTERA) += spi_altera.o
drivers/spi/spi-regmap.c new file +76 -0
76 lines not on this page · raw message
Signature · 1 line
1.7.5.4
PATCH 4/4 regulator: Convert tps65023 to use regmap API
In reply to Mark Brown--- drivers/regulator/Kconfig | 1 + drivers/regulator/tps65023-regulator.c | 98 +++++++++----------------------- 2 files changed, 28 insertions(+), 71 deletions(-)
drivers/regulator/Kconfig +1 -0
@@ -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 +27 -71
155 lines not on this page · raw message
Signature · 1 line
1.7.5.4
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Hi Mark,
Quoted 39 lines · Mark Brown
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@@ -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 +2
-1@@ -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; };
drivers/regulator/tps65023-regulator.c struct tps_pmic { · line 140
Quoted 10 lines
drivers/regulator/tps65023-regulator.c +0
-10-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); -} -
The driver was previously using the SMBus API and could thus work with SMBus (non-I2C) controllers. After your change, an I2C controller is mandatory. Are you sure this is OK for all users?
At least you should update the adapter functionality check in tps_65023_probe() accordingly.
As a general comment, this requirement will considerably limit the interest of regmap for I2C devices (at least in its current form.) Many systems out there only have SMBus controllers, and more importantly, most I2C device drivers are meant to be portable across systems and thus rely on the SMBus API. The i2c documentation encourages driver authors to do this.
Quoted 88 lines
drivers/regulator/tps65023-regulator.c +10
-58static 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)
drivers/regulator/tps65023-regulator.c static struct regulator_ops tps65023_ldo_ops = { · line 410
Quoted 5 lines
drivers/regulator/tps65023-regulator.c +2
-0@@ -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 ,
One value per line, please.
Quoted 21 lines
drivers/regulator/tps65023-regulator.c +9
-1+}; +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;
drivers/regulator/tps65023-regulator.c static int __devinit tps_65023_probe(struct i2c_client *client, · line 479
Quoted 5 lines
drivers/regulator/tps65023-regulator.c +1
-1@@ -523,10 +476,12 @@ static int __devinit tps_65023_probe(struct i2c_client *client,return 0 ; - fail: +fail:
The original style is usually preferred for labels. It lets diff -p skip them and only display actual function names.
Quoted 16 lines
drivers/regulator/tps65023-regulator.c +3
-0while (--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 · 1 line
Jean Delvare
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Quoted 3 lines · Jean Delvare
The driver was previously using the SMBus API and could thus work with SMBus (non-I2C) controllers. After your change, an I2C controller is mandatory. Are you sure this is OK for all users?
I'd be *utterly* astonished if it ever caused any issues, embedded systems like those that have regulators just don't have anything less than full I2C support, it's exceptionally rare to see any direct SMBus support in the hardware at all - the only example I can see in the tree is Blackfin and that is a proper I2C controller too, it just seems that someone overdesigned the hardware a little.
Basically, for something like a regulator it's just not an issue.
Quoted 6 lines
As a general comment, this requirement will considerably limit the interest of regmap for I2C devices (at least in its current form.) Many systems out there only have SMBus controllers, and more importantly, most I2C device drivers are meant to be portable across systems and thus rely on the SMBus API. The i2c documentation encourages driver authors to do this.
Right, so for pretty much anything except PCs the main reason for using the SMBus API is that it provides the sort of data mangling to the bus functionality that the regmap API provides. The main thing it's missing for drivers like this one is that it doesn't have a read/modify/write operation, otherwise there would be no current value in switching to the regmap API. As far as I can tell this isn't really a big deal except for those device classes like hwmon which are frequently deployed in PCs, anything else just won't care.
This should be reasonably simple to handle in the regmap API, just teach the I2C module to fall back to using the SMBus operations if the controller is limited to that. This should be done incrementally as it adds complexity, and ideally would be done by someone with some access to hardware that can use the SMBus API (I don't have any myself) so they can test.
Does that sound reasonable to you?
Quoted 3 lines
Review
Quoted 3 lines
static int
tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask){(quoted diff) +0
-1- int err, data;
*Please* could people delete unneeded context from mails?
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Quoted 6 lines · Mark Brown
Quoted 3 lines · Jean Delvare
The driver was previously using the SMBus API and could thus work with SMBus (non-I2C) controllers. After your change, an I2C controller is mandatory. Are you sure this is OK for all users?
Quoted 8 lines
I'd be *utterly* astonished if it ever caused any issues, embedded systems like those that have regulators just don't have anything less than full I2C support, it's exceptionally rare to see any direct SMBus support in the hardware at all - the only example I can see in the tree is Blackfin and that is a proper I2C controller too, it just seems that someone overdesigned the hardware a little.
Basically, for something like a regulator it's just not an issue.
OK, alright then.
Quoted 7 lines
Quoted 6 lines
As a general comment, this requirement will considerably limit the interest of regmap for I2C devices (at least in its current form.) Many systems out there only have SMBus controllers, and more importantly, most I2C device drivers are meant to be portable across systems and thus rely on the SMBus API. The i2c documentation encourages driver authors to do this.
Quoted 8 lines
Right, so for pretty much anything except PCs the main reason for using the SMBus API is that it provides the sort of data mangling to the bus functionality that the regmap API provides. The main thing it's missing for drivers like this one is that it doesn't have a read/modify/write operation, otherwise there would be no current value in switching to the regmap API. As far as I can tell this isn't really a big deal except for those device classes like hwmon which are frequently deployed in PCs, anything else just won't care.
Mostly correct. There are a few other devices which can also be found on PCs: multiplexers and EEPROMs at least.
Quoted 6 lines
This should be reasonably simple to handle in the regmap API, just teach the I2C module to fall back to using the SMBus operations if the controller is limited to that. This should be done incrementally as it adds complexity, and ideally would be done by someone with some access to hardware that can use the SMBus API (I don't have any myself) so they can test.
You do have access ;) The i2c core will translate SMBus to I2C as needed, so all you have to do is temporarily alter the regmap core to unconditionally translate to SMBus. This will be inefficient (regmap -> SMBus -> I2C) but should be suitable for testing.
Quoted 1 line
Does that sound reasonable to you?
Yes, no objection.
Quoted 5 lines
Quoted 3 lines
Review
Quoted 3 lines
static int
tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask){(quoted diff) +0
-1- int err, data;*Please* could people delete unneeded context from mails?
? There were more comments from me in the rest of the code, which is why I left it.
Signature · 1 line
Jean Delvare
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Quoted 8 lines · Jean Delvare
Quoted 3 lines · Mark Brown
regmap API. As far as I can tell this isn't really a big deal except for those device classes like hwmon which are frequently deployed in PCs, anything else just won't care.
Mostly correct. There are a few other devices which can also be found on PCs: multiplexers and EEPROMs at least.
I did say "device classes like" for a reason :)
Quoted 11 lines
Quoted 6 lines
This should be reasonably simple to handle in the regmap API, just teach the I2C module to fall back to using the SMBus operations if the controller is limited to that. This should be done incrementally as it adds complexity, and ideally would be done by someone with some access to hardware that can use the SMBus API (I don't have any myself) so they can test.
You do have access ;) The i2c core will translate SMBus to I2C as needed, so all you have to do is temporarily alter the regmap core to unconditionally translate to SMBus. This will be inefficient (regmap -> SMBus -> I2C) but should be suitable for testing.
I have access to host controllers obviously, what I don't have is access to any client devices with suitable register maps.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Review
Quoted 3 lines
static int
tps_65023_set_bits(struct tps_pmic *tps, u8 reg, u8 mask){(quoted diff) +0
-1- int err, data;*Please* could people delete unneeded context from mails?
? There were more comments from me in the rest of the code, which is why I left it.
You should delete all the text that isn't directly related to your comments, you appeared to have not deleted any content at all from the mail. There were pages and pages of stuff with no comments at all.
This really helpful both for finding the content (it's really easy to just page past stuff) and for those of us who read mail on mobile devices.
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
In reply to Jean DelvareQuoted 5 lines · Jean Delvare
Quoted 1 line · Mark Brown
Does that sound reasonable to you?
Yes, no objection.
BTW, if this does sound reasonable are you OK with adding your ack for the I2C bus interface patch or are there any updates you want me to do?
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Hi Mark,
Quoted 9 lines · Mark Brown
Quoted 5 lines · Jean Delvare
Quoted 1 line · Mark Brown
Does that sound reasonable to you?
Yes, no objection.
BTW, if this does sound reasonable are you OK with adding your ack for the I2C bus interface patch or are there any updates you want me to do?
I did not review the patch carefully, so I can't ask for updates. As none of "my" drivers will use it, I don't really feel qualified (nor interested, honestly) to review it.
I don't quite get why you put the i2c bindings into drivers/i2c/i2c-regmap.c. This means that Ben and I end up being the maintainers of that file, while it's your thing. And this module is a user of i2c, not a provider, so it doesn't really belong there anyway. Same goes with spi. And what's the rationale for putting the regmap core under drivers/base?
What's wrong with the more direct approach:drivers/regmap/regmap-core.cdrivers/regmap/regmap-i2c.cdrivers/regmap/regmap-spi.c
?
At least you would have everything in one place and under your control. With your current plan, every update is likely to spawn cross-subsystems, which always results in delays and conflicts.
Now if you have a good reason for the current design, that's OK with me, I can live with that. Simply it seems more complex than needed.
Also, your Kconfig setup is such that all bindings will be selected as soon as any driver needs one. And the selection (module vs. built-in) will be aligned on the core setting (e.g. CONFIG_I2C) rather than the drivers which use it. I'd rather have e.g. REGULATOR_TPS65023 select REGMAP_I2C, and in turn have REGMAP_I2C select REGMAP. This should address the issues I pointed out.
Signature · 1 line
Jean Delvare
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Quoted 30 lines · Jean Delvare
Hi Mark,
Quoted 9 lines · Mark Brown
Quoted 5 lines · Jean Delvare
Quoted 1 line · Mark Brown
Does that sound reasonable to you?
Yes, no objection.
BTW, if this does sound reasonable are you OK with adding your ack for the I2C bus interface patch or are there any updates you want me to do?
I did not review the patch carefully, so I can't ask for updates. As none of "my" drivers will use it, I don't really feel qualified (nor interested, honestly) to review it.
I don't quite get why you put the i2c bindings into
drivers/i2c/i2c-regmap.c. This means that Ben and I end up being the maintainers of that file, while it's your thing. And this module is a user of i2c, not a provider, so it doesn't really belong there anyway. Same goes with spi. And what's the rationale for putting the regmap core underdrivers/base?What's wrong with the more direct approach:
drivers/regmap/regmap-core.cdrivers/regmap/regmap-i2c.cdrivers/regmap/regmap-spi.c
?
Quoted 6 lines
At least you would have everything in one place and under your control. With your current plan, every update is likely to spawn cross-subsystems, which always results in delays and conflicts.
Now if you have a good reason for the current design, that's OK with me, I can live with that. Simply it seems more complex than needed.
Hahaha. Mark *did* do it that way and I suggested splitting up into the i2c and spi directories to keep bus specific infrastructure together. Very well, if you feel strongly about it then I withdraw my comment.
g.
Quoted 9 lines
Also, your
Kconfigsetup is such that all bindings will be selected as soon as any driver needs one. And the selection (module vs. built-in) will be aligned on the core setting (e.g.CONFIG_I2C) rather than the drivers which use it. I'd rather have e.g.REGULATOR_TPS65023selectREGMAP_I2C, and in turn haveREGMAP_I2Cselect REGMAP. This should address the issues I pointed out.Signature · 1 line
Jean Delvare
Re: [PATCH 4/4] regulator: Convert tps65023 to use regmap API
Quoted 11 lines · Grant Likely
Quoted 9 lines · Jean Delvare
What's wrong with the more direct approach:
drivers/regmap/regmap-core.cdrivers/regmap/regmap-i2c.cdrivers/regmap/regmap-spi.c
?At least you would have everything in one place and under your control. With your current plan, every update is likely to spawn cross-subsystems, which always results in delays and conflicts.
This was exactly what the orginal patch set looked like, there have just been a couple of git mvs since then.
Quoted 7 lines
Quoted 2 lines
Now if you have a good reason for the current design, that's OK with me, I can live with that. Simply it seems more complex than needed.
Hahaha. Mark *did* do it that way and I suggested splitting up into the i2c and spi directories to keep bus specific infrastructure together. Very well, if you feel strongly about it then I withdraw my comment.
Actually you weren't the only person to suggest this, but the other suggestions were pretty weak and like you say all together was my original preference. Since we seem to have achieved consensus I'll flip everything back.
The reason for drivers/base was that this is more utility code like the firmware loader than it is an actual thing.
Quoted 6 lines
Quoted 6 lines
Also, your
Kconfigsetup is such that all bindings will be selected as soon as any driver needs one. And the selection (module vs. built-in) will be aligned on the core setting (e.g.CONFIG_I2C) rather than the drivers which use it. I'd rather have e.g.REGULATOR_TPS65023selectREGMAP_I2C, and in turn haveREGMAP_I2Cselect REGMAP. This should address the issues I pointed out.
Yeah, the problem is that select doesn't actually work terribly well as it causes dependencies of the selected thing to get ignored which is annoying and means that the thing doing the selecting needs to select them too. The bus interfaces are small enough that I'm not really worried about the extra size and if need be we can always sort it out layer.