lkml.org 
[lkml]   [2010]   [Jun]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] drivers: regulator: add Maxim 8998 driver
    From
    On Fri, Jun 11, 2010 at 7:58 PM, Mark Brown
    <broonie@opensource.wolfsonmicro.com> wrote:
    > On Fri, Jun 11, 2010 at 09:02:45AM +0200, Marek Szyprowski wrote:
    >
    >> This patch adds voltage regulator driver for Maxim 8998 chip. This chip
    >> is used on Samsung Aquila and GONI boards.
    >
    > Overall this looks pretty good - some comments below, though.
    >
    > A few things in the code make it look like the driver should be using
    > the MFD framework - there's references in here for things like a battery
    > charger which should be being supported via the power subsystem, for
    > example.

    Exactly, also it supports the RTC. Okay try to re-organize the PMIC drivers.

    >
    >> +     ret = i2c_smbus_read_byte_data(client, reg);
    >> +     if (ret < 0)
    >> +             return -EIO;
    >
    > It probably makes more sense to pass back the actual error you got from
    > the I2C subsystem here.

    Will fix it.

    >
    >> +static void max8998_cache_register_init(struct i2c_client *client)
    >> +{
    >> +     u8 value;
    >> +     int ret;
    >> +
    >> +     ret = max8998_i2c_read(client, MAX8998_REG_STATUS1, &value);
    >> +     if (ret)
    >> +             return;
    >> +     ret = max8998_i2c_read(client, MAX8998_REG_STATUS2, &value);
    >> +     if (ret)
    >> +             return;
    >
    > Should these registers really be cached at all?  They're not used but
    > the name and the fact that you read them dynamically makes
    >
    > Also, it looks like you're initialising things like the voltage settings
    > and regulator enables from the cache rather than from the chip - this
    > seems like it'll cause problems if the bootloader or similar has done
    > something to the chip prior to the driver taking control.  For PMICs and
    > regulators I'd generally expect to see the driver initialise itself from
    > the chip rather than fixed defaults.
    >
    >> +static const int ldo23_voltage_map[] = {
    >> +      800,  850,  900,  950, 1000,
    >> +     1050, 1100, 1150, 1200, 1250,
    >> +     1300,
    >
    > I may have missed something in these tables but they all look like
    > simple functions of the register value - perhaps they could be replaced
    > with calculations?

    Good idea, create the generic voltage map something like this

    int generic_get_voltage_map(base, step, index)
    where base is 800, step is 50, and actual index.

    Before call the generic_get_voltage_map() we should check the index.

    >
    >> +static int max8998_get_ldo(struct regulator_dev *rdev)
    >> +{
    >> +     return rdev_get_id(rdev);
    >> +}
    >
    > Probably worth shoving an inline in there, though I'm not sure if the
    > function is really adding anything.
    >
    >> +     value = max8998_read_reg(max8998, reg);
    >> +     value |= (1 << shift);
    >> +     ret = max8998_write_reg(max8998, reg, value);
    >
    > This is racy - there's nothing preventing another thread coming in and
    > running the same code so you get something like:
    >
    >        reg_read(1)
    >        reg_read(2)
    >        reg_write(1)
    >        reg_write(2)
    >
    > You could fix this with an atomic max8998_update_bits() function.
    >
    >> +static int max8998_set_voltage(struct regulator_dev *rdev,
    >> +                             int min_uV, int max_uV)
    >> +{
    >> +     struct max8998_data *max8998 = rdev_get_drvdata(rdev);
    >> +     int min_vol = min_uV / 1000, max_vol = max_uV / 1000;
    >> +     int ldo = max8998_get_ldo(rdev);
    >> +     const int *vol_map = ldo_voltage_map[ldo];
    >> +     int reg, shift = -1, mask, value, ret;
    >> +     int i;
    >> +
    >> +     for (i = 0; i < vol_map[i]; i++) {
    >> +             if (vol_map[i] >= min_vol)
    >
    > This vol_map[] checking is pretty obscure...  Are you sure the check
    > you're using in the for loop shouldn't be based on the table size for
    > the voltage map rather than on the values in the table?
    >
    >> +                     break;
    >> +     }
    >> +
    >> +     if (!vol_map[i] || vol_map[i] > max_vol)
    >> +             return -EINVAL;
    >
    > Your voltage maps aren't null terminated so the check for vol_map[i]
    > doesn't do what you think it does - you should be checking to see if you
    > fell off the end of the arary, not to see if you have a zero value.
    >
    >> +static irqreturn_t max8998_ono_irq(int irq, void *data)
    >> +{
    >> +     return IRQ_HANDLED;
    >> +}
    >
    > This needs at least a comment explaining why you don't need to do
    > anything for the interrupt.

    We just remove it. it's unused function actually.

    >
    >> +     if (gpio_is_valid(max8998->ono_pin)) {
    >> +             ret = gpio_request(max8998->ono_pin, "MAX8998 nONO");
    >> +             if (ret)
    >> +                     goto out_ono;
    >> +             irq = gpio_to_irq(max8998->ono_pin);
    >> +             ret = request_irq(irq, max8998_ono_irq,
    >> +                             IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
    >> +                             "max8998 nPower", max8998);
    >> +             if (ret) {
    >> +                     dev_err(&client->dev, "Can't get interrupt pin\n");
    >> +                     goto out_ono_irq;
    >> +             }
    >> +
    >> +             /* enable wakeup source for power button */
    >> +             set_irq_wake(irq, 1);
    >> +             max8998->ono_irq = irq;
    >> +     }
    >
    > Should this not just be specified as an IRQ?  The gpio API doesn't
    > appear to be being used at all by the driver.

    Okay we will check it.

    >
    >> +     i2c_set_clientdata(client, max8998);
    >> +
    >> +     max8998_cache_register_init(client);
    >
    > I'd expect the cache initialisation to be done before the regulators are
    > initialised so that the regulator API can use the cache while it does
    > the setup.  This will improve performance on startup.

    Thank you,
    Kyungmin Park
    --
    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: 2010-06-11 14:51    [W:0.041 / U:61.888 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site