lkml.org 
[lkml]   [2012]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/4] REGULATOR: regulator driver for Palmas series chips
    On Mon, May 14, 2012 at 10:58:31AM +0900, Graeme Gregory wrote:

    Looks good, quite a few of the things below are updates for very
    recently introduced APIs.

    > +static int palmas_smps_read(struct palmas *palmas, unsigned int reg,
    > + unsigned int *dest)
    > +{
    > + int slave;
    > + unsigned int addr;
    > +
    > + slave = PALMAS_BASE_TO_SLAVE(PALMAS_SMPS_BASE);
    > + addr = PALMAS_BASE_TO_REG(PALMAS_SMPS_BASE, reg);
    > +
    > + return regmap_read(palmas->regmap[slave], addr, dest);
    > +}

    Looks like the LDO and SPMS regulators both use the same regmap?

    > + .get_voltage = palmas_get_voltage_smps,

    Why not get_voltage_sel?

    > + .set_voltage_sel = palmas_set_voltage_smps_sel,
    > + .list_voltage = palmas_list_voltage_smps,

    Implementing map_voltage() too would be nice.

    > +static int palmas_is_enabled_smps10(struct regulator_dev *dev)
    > +{
    > + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
    > + unsigned int reg;
    > +
    > + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_STATUS, &reg);
    > +
    > + reg &= SMPS10_BOOST_EN;
    > +

    Should be able to use regulator_is_enabled_regmap() and friends.

    > +static int palmas_list_voltage_smps10(struct regulator_dev *dev,
    > + unsigned selector)
    > +{
    > + if (selector)
    > + return 5000000;
    > +
    > + return 3750000;

    This could be written a little more transparently!

    > +static int palmas_get_voltage_smps10(struct regulator_dev *dev)
    > +{
    > + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
    > + int selector;
    > + unsigned int reg;
    > +
    > + palmas_smps_read(pmic->palmas, PALMAS_SMPS10_CTRL, &reg);
    > +
    > + selector = (reg & SMPS10_VSEL) >> 3;
    > +
    > + return palmas_list_voltage_smps10(dev, selector);

    Should be get_voltage_sel() really, or beter still should be using
    regulator_set_voltage_sel_regmap(). Similarly for the set (which is
    already using _sel()).

    > +static int palmas_enable_ldo(struct regulator_dev *dev)
    > +{
    > + struct palmas_pmic *pmic = rdev_get_drvdata(dev);
    > + int id = rdev_get_id(dev);
    > + unsigned int reg;
    > +
    > + palmas_ldo_read(pmic->palmas, palmas_regs_info[id].ctrl_addr, &reg);
    > +
    > + reg |= LDO1_CTRL_MODE_ACTIVE;
    > +
    > + palmas_ldo_write(pmic->palmas, palmas_regs_info[id].ctrl_addr, reg);

    Could use the core regmap stuff for the LDOs too.

    > + /* Adjust selector to match list_voltage ranges */
    > + if (selector > 49)
    > + selector = 49;
    > +
    > + return palmas_list_voltage_ldo(dev, selector);

    get_voltage_sel().

    > + if (!pdata)
    > + return -EINVAL;
    > + if (!pdata->reg_data)
    > + return -EINVAL;

    I'd expect the driver to be able to register the regulators with no
    platform data at all.

    > + pmic = kzalloc(sizeof(*pmic), GFP_KERNEL);
    > + if (!pmic)
    > + return -ENOMEM;

    devm_kzalloc()

    > + pmic->desc = kcalloc(PALMAS_NUM_REGS,
    > + sizeof(struct regulator_desc), GFP_KERNEL);
    > + if (!pmic->desc) {
    > + ret = -ENOMEM;
    > + goto err_free_pmic;
    > + }

    Just embed this in the pmic struct? It's always the same size.

    > + pmic->rdev = kcalloc(PALMAS_NUM_REGS,
    > + sizeof(struct regulator_dev *), GFP_KERNEL);

    Similarly heere.
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2012-05-14 11:41    [W:0.041 / U:30.236 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site