On 14/05/12 17:52, Mark Brown wrote: > 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? Yes have tidied this up a little in latest version. >> + .get_voltage = palmas_get_voltage_smps, > Why not get_voltage_sel? Mild amnesia is my excuse, is fixed now! >> + .set_voltage_sel = palmas_set_voltage_smps_sel, >> + .list_voltage = palmas_list_voltage_smps, > Implementing map_voltage() too would be nice. I have implemented it for smps. >> +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 &= SMPS10_BOOST_EN; >> + > Should be able to use regulator_is_enabled_regmap() and friends. Has been converted thanks! >> +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! Have re-written it hopefully clearer. >> +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, ®); >> + >> + 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()). I have implemented it using the regmap functions thanks for tip! >> +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 |= 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. Enable/Disable now use regmap but is_enabled cant as it reads from a different bitmask. >> + /* Adjust selector to match list_voltage ranges */ >> + if (selector > 49) >> + selector = 49; >> + >> + return palmas_list_voltage_ldo(dev, selector); > get_voltage_sel(). Has been fixed! >> + 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. Ok, I misunderstood how the pdata was used, hopefully I have got it correct in the new version. >> + pmic = kzalloc(sizeof(*pmic), GFP_KERNEL); >> + if (!pmic) >> + return -ENOMEM; > devm_kzalloc() Fixed! >> + 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. Have embedded, this was a left over from when I was originally planning the muxing handling. >> + pmic->rdev = kcalloc(PALMAS_NUM_REGS, >> + sizeof(struct regulator_dev *), GFP_KERNEL); > Similarly heere. Same as above. Thanks for taking time to review. Am sending v2 patches soon. Graeme -- 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