On Thursday 27 November 2008, Manikandan Pillai wrote:
> Implements the basic driver for TPS6235x devices populated on the
> PR785 board. tps6235x.c contains the driver code for TPS devices used on
> PR785 boards. Driver code is added it to the build.
>
> Signed-off-by: Manikandan Pillai <mani.pillai@xxxxxx>
> ---
> drivers/i2c/chips/Makefile | 1 +
> drivers/i2c/chips/tps6235x.c | 416 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 417 insertions(+), 0 deletions(-)
Really quick comments only ... key point: plan to merge this to
mainline instead of just the OMAP tree. Which means you should
be structuring the driver (and patch) in more standard ways from
the first, notably:
- Both drivers/i2c/chips/Makefile and its Kconfig sibling
are very clear: don't add new drivers to that directory!
- Patches that add drivers should be standalone and, as much
as possible, complete. In this case, it's missing Kconfig.
In this case, the driver seems to most naturally live in the
new drivers/regulator subtree, and implement the regulator
interface. If it doesn't fit well ... that's a reason to
suggest updates to that regulator framework(*).
Using the regulator framework will also affect how you merge
board support. Wanting a pr785_enbl_dcdc() board-specific
call is a clue that you're doing something wrong... in this
case, regulator_enable() and regulator_disable() will do
the job much more politely.
A subsequent patch iteration should probably go to LKML.
You'll get this comment before long, so I'll say it right
now: use i2c_smbus_{read,write}_byte_data() calls instead
of the lower level I2C calls. It'll be more portable, and
use less code in this driver ... even if it does increase
the runtime cost of these (infrequent) calls.
> +static const struct i2c_device_id tps_6235x_id[] = {
> + { "tps62352_core_pwr", 0},
> + { "tps62353_mpu_pwr", 1},
> + {},
> +};
Strike the "_core_pwr" and "_mpu_pwr" suffixes. Those are
not inherent tasks of the chip; they are policies set up
by board designs, and recognized in software by how the
components get set up. This driver shouldn't try to care
about such board-specific policies.
The data sheet covers TPS6254x for x in 0..6 but this driver
omits most of those. Best to support them all.
- Dave
(*) The regulator framework is new. It does, and will,
need changes to make sure it does what it needs to do.
You might not encounger such issues. Or, you might...
--
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
