On Monday 08 December 2008, Manikandan Pillai wrote:
> +++ b/drivers/regulator/tps6235x-regulator.c
> @@ -0,0 +1,557 @@
> +/*
> + * tps6235x-regulator.c -- support regulators in tps6235x family chips
> + *
> + * Copyright (C) 2008 David Brownell
> + * Author : Manikandan Pillai<mani.pillai@xxxxxx>
> + *
No, not copyright by me. I didn't write this. Take my
name off of your code.
Note that your patch [2/2] should have been merged with the
relevant parts of this patch ... which, as Mark noted,
is all over the map and needs to partitioned according
to structure.
A patch supporting *JUST* the regulator will seemingly
touch drivers/regulator/{Kconfig,Makefile} and create
a new drivers/regulator/tps6235x-regulator.c file; that
would be a good first patch for your set.
I suggest your Kconfig not mention the PR785 card; there
are surely plenty of other products using these chips and
there's no way you can (or should!) mention them all. If
you're tempted to have pr785-specific logic in the
generic regulator code, think again: that's a sign you
are not actually writing generic, reusable code. And
for something as *SIMPLE* as the $SUBJECT family of
regulators, there's no reason to write anything except
fully reusable code which implements the regulator API.
(With one key exception. Floor/ceiling support for
dynamic voltage switching is not part of that API.
I suggest you treat the regulator calls as applying
to the ceiling, with "floor" support as an extension.)
> +struct tps_6235x_info {
> + unsigned int state;
> + unsigned int tps_i2c_addr;
> + struct i2c_client *client;
> + struct device *i2c_dev;
> + /* platform data holder */
> + void *pdata;
> +};
> +
> +static struct tps_6235x_info tps_6235x_infodata[2];
I'm not following this.
- The tps_info should be accessed by i2c_get_clientdata(),
and initialized by i2c_set_clientdata(). It's instance
specific data, and that's how to manage such data.
- tps_i2c_addr is completely un-needed; client->addr is
the same thing.
- ditto i2c_dev; &client->dev is the same thing.
- and pdata is client->dev.platform_data.
Plus there's no reason to limit the system to two of
these regulators ... get rid of that global.
And you should remove all the pr785-specific code
from the regulator driver. That includes the vdd1
and vdd2 consumers -- none of that belongs as part
of a generic regulator driver. Needing to have those
board-specific constraints should have been a Sign...
> +
> +static
> +int tps_6235x_probe(struct i2c_client *client, const struct i2c_device_id
> *id) +{
> + struct tps_6235x_info *info;
> + struct regulator_dev *rdev = NULL;
> + unsigned char reg_val;
> +
> + printk(KERN_INFO "tps_6235x_probe:i2c_addr = %x\n", (int)client->addr);
That's a debug message. Use dev_dbg(); and plan to remove that
one before this merges.
> +
> + info = &tps_6235x_infodata[id->driver_data];
> + /* Device probed is TPS62352 CORE pwr chip if driver_data = 0
> + Device probed is TPS62353 MPU pwr chip if driver_data = 1 */
It would be a lot easier to just have initted driver_data to
point to the "info" you wanted.
But you're also mis-understanding some things here.
The id->driver_data should be *generic* data ... as in,
if you had three '52 chips (maybe on different busses),
they'd all have the same driver_data, stuff holding true
for every instance of one of those chips. Regulator
ops vectors; maybe some tables they use, which help
interpret register bitfields. Read-only goodies.
If you want instance-specific data, there are two
flavors of that. One is from client->dev.platform_data,
full of board-specific state you treat as read-only.
The other is stuff that you collect over time; an
example might be statistics. Call this "mutable state".
The convention is that probe() will kzalloc() a
struct holding "mutable state", then stuff that
with goodies. Like maybe the i2c_client, so it's
always available; stuff from id->driver_data, so
chip parameters don't get forgotten; and whatever
else you need.
You're turning the generic/"type description"
data into per-instance mutable state, and then
limiting this to two instances total!
- Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
