`Hi Mark,On Thu, May 10, 2012 at 12:17 AM, Mark Brown<broonie@opensource.wolfsonmicro.com> wrote:> On Wed, May 09, 2012 at 09:54:55PM +0530, Yadwinder Singh wrote:>>> +/* Voltage maps in mV */>> +static const struct voltage_map_desc ldo_voltage_map_desc = {>> +     .min = 800,     .max = 3950,    .step = 50,     .n_bits = 6,>> +};                           /* LDO3 ~ 5, 9 ~ 14, 16 ~ 26 */>> Hrm, funnily enough I was just thinking about factoring this stuff out> into the core after a conversation with Graeme Gregory the other week.> Let's do that...>>> +     [MAX77686_EN32KHZ_AP] = NULL,>> +     [MAX77686_EN32KHZ_CP] = NULL,>> Now that the generic clock API is in mainline these should be moved over> to use it.>Sorry, I cann't get your point here. Please explain it little bit more.>> +static int max77686_get_voltage_unit(int rid)>> +{>> +     int unit = 0;>> +>> +     switch (rid) {>> +     case MAX77686_BUCK2...MAX77686_BUCK4:>> +             unit = 1;       /* BUCK2,3,4 is uV */>> +             break;>> +     default:>> +             unit = 1000;>> Why not just list everything in uV?>Yes, everything should be in uV, I will correct it.>> +static int max77686_get_voltage(struct regulator_dev *rdev)>> +{>> Implement get_voltage_sel().>>> +static inline int max77686_get_voltage_proper_val(const struct voltage_map_desc>> +                                               *desc, int min_vol,>> +                                               int max_vol)>> +{>> +     int i = 0;>> +>> +     if (desc == NULL)>> +             return -EINVAL;>> +>> +     if (max_vol < desc->min || min_vol > desc->max)>> +             return -EINVAL;>> +>> +     while (desc->min + desc->step * i < min_vol &&>> +            desc->min + desc->step * i < desc->max)>> +             i++;>> Why are you iterating here?  Calculate!  Though like I say let's factor> this out anyway.>Yes, I will do it.>> +     if (rid == MAX77686_BUCK2 || rid == MAX77686_BUCK3 ||>> +         rid == MAX77686_BUCK4) {>> +             /* If the voltage is increasing */>> +             if (org < i)>> +                     udelay(DIV_ROUND_UP(desc->step * (i - org),>> +                                         ramp[max77686->ramp_delay]));>> +     }>> Don't do this, implement set_voltage_time_sel().>Ok, I will implement it.>> +     .enable = max77686_reg_enable,>> +     .disable = max77686_reg_disable,>> +     .set_suspend_enable = max77686_reg_enable,>> +     .set_suspend_disable = max77686_reg_disable,>> You've got the same ops for suspend and non-suspend cases here, this is> clearly buggy.>>> +/* count the number of regulators to be supported in pmic */>> +     pdata->num_regulators = 0;>> Coding style.>>> +     if (iodev->dev->of_node) {>> +             ret = max77686_pmic_dt_parse_pdata(iodev, pdata);>> +             if (ret)>> +                     return ret;>> This ought to use of_regulator_match().>Ok, I will look into it.>> +     }>> +>> +     if (!pdata) {>> +             dev_err(&pdev->dev, "platform data not found\n");>> +             return -ENODEV;>> +     }>> This should be totally fine.>I will look into it.>> +     max77686 = kzalloc(sizeof(struct max77686_data), GFP_KERNEL);>> +     if (!max77686)>> +             return -ENOMEM;>> devm_kzalloc().>Yes, its better option.>> +     if (pdata->ramp_delay) {>> +             max77686->ramp_delay = pdata->ramp_delay;>> +             max77686_update_reg(i2c, MAX77686_REG_BUCK2CTRL1,>> +                     RAMP_VALUE, RAMP_MASK);>> This appears not to actually use the value passed in as platform_data.>It gets corresponding index of ramp_rate value in ramp_rate_valuetable supported by hardware, from platform_data which we write toramp_rate control bits of control registers.>> +>> +     for (i = 0; i < pdata->num_regulators; i++) {>> +             const struct voltage_map_desc *desc;>> +             int id = pdata->regulators[i].id;>> +>> +             desc = reg_voltage_map[id];>> +             if (desc)>> +                     regulators[id].n_voltages =>> +                         (desc->max - desc->min) / desc->step + 1;>> +>> +             rdev[i] = regulator_register(&regulators[id], max77686->dev,>> +                                          pdata->regulators[i].initdata,>> +                                          max77686, NULL);>> No, you should unconditionally register all regulators the device> physically has.  This is useful for debug and simplifies the code.>Ok. I will do it.> _______________________________________________> linux-arm-kernel mailing list> linux-arm-kernel@lists.infradead.org> http://lists.infradead.org/mailman/listinfo/linux-arm-kernelThanks,Yadwinder.--To unsubscribe from this list: send the line "unsubscribe linux-kernel" inthe body of a message to majordomo@vger.kernel.orgMore majordomo info at  http://vger.kernel.org/majordomo-info.htmlPlease read the FAQ at  http://www.tux.org/lkml/`