lkml.org 
[lkml]   [2012]   [May]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 2/2] regulator: Add support for MAX77686.
    From
    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_value
    table supported by hardware, from platform_data which we write to
    ramp_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-kernel

    Thanks,
    Yadwinder.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2012-05-10 10:02    [W:0.037 / U:60.616 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site