Hi, Pls find my comments inlined.
Regards -----Original Message----- From: David Brownell [mailto:davi...@pacbell.net] Sent: Monday, December 08, 2008 7:04 PM To: Pillai, Manikandan Cc: linux-omap@vger.kernel.org; Mark Brown Subject: Re: [PATCH 1/2] TPS6235x driver added into the power regulator framework 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.pil...@ti.com> > + * No, not copyright by me. I didn't write this. Take my name off of your code. [Pillai, Manikandan] OK 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. [Pillai, Manikandan] OK will merge it 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. [Pillai, Manikandan] OK 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. [Pillai, Manikandan] I can removed the PR785 mention from Drivers/regulator dirs. I believe the PR785 option should Be selected in arch/arm/mach-omap2/Kconfig along with TWL-4030 Core. Pls let me know your suggestions on this one. (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.) [Pillai, Manikandan] Not clear on this comment > +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... [Pillai, Manikandan] OK > + > +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. [Pillai, Manikandan] OK > + > + 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. [Pillai, Manikandan] OK 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! [Pillai, Manikandan] OK - Dave -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
