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

[Jean Delvare <khali@linux-fr.org>]

Hi Mark,

On Fri, 15 Jul 2011 22:17:51 +0900, Mark Brown wrote:
> On Fri, Jul 15, 2011 at 02:58:48PM +0200, Jean Delvare wrote:
> > On Fri, 15 Jul 2011 21:16:50 +0900, Mark Brown wrote:
> 
> > > 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.c
drivers/regmap/regmap-i2c.c
drivers/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.

-- 
Jean Delvare