lkml.org 
[lkml]   [2012]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V1] TPS62360: Add tps62360 regulator driver
    On Wed, Jan 04, 2012 at 02:50:18PM +0530, Laxman Dewangan wrote:
    > Change-Id: I93739aad865401047738f899b2248f2714d824ea

    Don't include gerritt droppings in upstream submissions.

    > +/* Supported voltage values for regulators */
    > +static const u16 TPS62360_VOLTAGES[] = {
    > + 770, 780, 790, 800, 810, 820, 830, 840, 850, 860,
    > + 870, 880, 890, 900, 910, 920, 930, 940, 950, 960,
    > + 970, 980, 990, 1000, 1010, 1020, 1030, 1040, 1050, 1060,
    > + 1070, 1080, 1090, 1110, 1110, 1120, 1130, 1140, 1150, 1160,
    > + 1170, 1180, 1190, 1200, 1210, 1220, 1230, 1240, 1250, 1260,
    > + 1270, 1280, 1290, 1300, 1310, 1320, 1330, 1340, 1350, 1360,
    > + 1370, 1380, 1390, 1400,
    > +};

    Why are you using a lookup table here? Just calaculate the values.
    This is a pervasive problem with the TI drivers which leads me to
    suspect cut'n'paste and therefore an opportunity to factor some code
    out.

    > +static int tps62360_reg_modify_bits(struct tps62360_chip *tps, u8 reg,
    > + u8 set_mask, u8 clear_mask)
    > +{

    There's a lot of code in this driver that looks like it could be
    factored out by using the regmap API, not just the register I/O but also
    the cache.

    > +static int tps62360_dcdc_is_enabled(struct regulator_dev *dev)
    > +{
    > + /**
    > + * Always return 1 as the EN is not controlled by register
    > + * programming */
    > + return 1;
    > +}

    Don't implement unsupported operations.

    > +
    > + for (vsel = 0; vsel < tps->desc.n_voltages; ++vsel) {
    > + int uV = tps->voltages[vsel] * 1000;
    > + if (min_uV <= uV && uV <= max_uV) {

    Not only do all the drivers coming in for these devices use lookup
    tables they also all open code the lookup :/

    > + init_data = &pdata->reg_init_data;
    > + tps = kzalloc(sizeof(*tps), GFP_KERNEL);
    > + if (!tps) {

    Use devm_kzalloc(), saves having to worry about cleanup.

    > +static void tps62360_shutdown(struct i2c_client *client)
    > +{
    > + struct tps62360_chip *tps = i2c_get_clientdata(client);
    > + int st;
    > +
    > + if (!tps->en_discharge)
    > + return;
    > +
    > + /* Configure the output discharge path */
    > + st = tps62360_reg_modify_bits(tps, REG_RAMPCTRL, BIT(2), BIT(2));
    > + if (st < 0)
    > + dev_err(tps->dev, "%s() fails in updating reg %d\n",
    > + __func__, REG_RAMPCTRL);
    > +}

    I rather suspect that if this is worth doing it's also worth doing over
    suspend...

    > +MODULE_DESCRIPTION("TPS62360 voltage regulator driver");
    > +MODULE_LICENSE("GPL v2");
    > +MODULE_ALIAS("platform:tps62360");

    This isn't a platform driver, it's a driver for an I2C device.

    > + * @vsel: Select the voltage id register.

    What's this?

    > + * @init_uV: initial micro volts which need to be set.

    No, there's no way stuff like this should be being open coded in
    individual regulator drivers - this isn't at all specific to this
    regulator so it should be (and is) supported by the standard
    constraints.


    \
     
     \ /
      Last update: 2012-01-05 07:31    [W:0.026 / U:0.652 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site