Re: [PATCH 4/4] tps65912: add regulator driver

[Margarita Olaya]

Hi Mark,

On Tue, May 10, 2011 at 3:53 PM, Mark Brown
<broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Tue, May 10, 2011 at 03:25:59PM -0500, Margarita Olaya wrote:
>
> A lot of this driver looks awfully similar to a bunch of the other TI
> PMIC drivers posted recently - is it possible to reuse some of driver
> code here?
>

911/10 and 912 are separate IC families, and there will be future
patches that differentiate the code much further.

Regards,
Margarita

>> +/* Supported voltage values for regulators */
>> +static const u16 VDCDCx_VSEL0_table[] = {
>> +     5000, 5125, 5250, 5375, 5500,
>> +     5625, 5750, 5875, 6000, 6125,
>
> All these tables look like they're nice regular voltage steps and could
> be replaced with simple calculations in the code.  That's better as it
> means we don't have to iterate through the table every time we want to
> do anything.
>
>> +static inline int tps65912_read(struct tps65912_reg *pmic, u8 reg)
>> +{
>> +     u8 val;
>> +     int err;
>> +
>> +     err = pmic->mfd->read(pmic->mfd, reg, 1, &val);
>> +     if (err < 0)
>> +             return err;
>> +
>> +     return val;
>> +}
>
> Shouldn't the MFD driver just export this function?  Both the IRQ and
> GPIO drivers also need it.  Similarly for write() and the reg_ versions
> of them.
>
>> +             reg1_val |= DCDCCTRL_DCDC_MODE_MASK;
>> +             tps65912_reg_write(pmic, reg1, reg1_val);
>> +             reg2_val &= ~DCDC_AVS_ECO_MASK;
>> +             tps65912_reg_write(pmic, reg2, reg2_val);
>
> There's set_bits() and clear_bits() ops in the core...
>
>> +     switch (range) {
>> +     case 0:
>> +             /* 0.5 - 1.2875V in 12.5mV steps */
>> +             voltage = tps65912_vsel_to_uv_range0(vsel);
>> +             break;
>
> Just implement get_voltage_sel() and let the core do the lookup.
>
>> +static int tps65912_set_voltage_dcdc(struct regulator_dev *dev,
>> +                                             unsigned selector)
>> +{
>> +     struct tps65912_reg *pmic = rdev_get_drvdata(dev);
>> +     int id = rdev_get_id(dev);
>> +     int value;
>> +     const u16 *table;
>> +     u8 table_len, reg;
>> +
>> +     table = pmic->info[id]->table;
>> +     table_len = pmic->info[id]->table_len;
>> +
>> +     reg = tps65912_get_dcdc_sel_register(pmic, id);
>> +     value = tps65912_reg_read(pmic, reg);
>> +     value &= 0xC0;
>> +     return tps65912_reg_write(pmic, reg, selector | value);
>
> You're not actually doing anything with table here...
>
>> +static int tps65912_get_voltage_ldo(struct regulator_dev *dev)
>> +{
>> +     struct tps65912_reg *pmic = rdev_get_drvdata(dev);
>> +     int id = rdev_get_id(dev), voltage = 0;
>> +     int vsel = 0;
>> +     u8 reg;
>> +
>> +     reg = tps65912_get_ldo_sel_register(pmic, id);
>> +     vsel = tps65912_reg_read(pmic, reg);
>> +     vsel &= 0x3F;
>> +
>> +     /* 0.5 - 1.2875V in 12.5mV steps */
>> +     voltage = LDO_VSEL_table[vsel];
>> +
>> +     return voltage * 100;
>
> Just make this into a get_voltage_sel().
>
>> +static int tps65912_list_voltage_dcdc(struct regulator_dev *dev,
>> +                                     unsigned selector)
>> +{
>> +     struct tps65912_reg *pmic = rdev_get_drvdata(dev);
>> +     int id = rdev_get_id(dev);
>> +     const u16 *table;
>> +     u8 table_len;
>> +
>> +     table = pmic->info[id]->table;
>> +     table_len = pmic->info[id]->table_len;
>> +     if (selector >= table_len)
>> +             return -EINVAL;
>> +     else
>> +             return table[selector] * 100;
>> +}
>
> There were a bunch of vsel_to_uV functions further up the driver which
> do the same thing - one of the two implementations should go.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/