lkml.org 
[lkml]   [2010]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRE: [RFC] [PATCH] Adding Intel Moorestown PMIC Battery Driver
    > You can add lkml as well.

    Will do it going forward.


    >> +static int pmicbatteryDebug;
    >> +module_param(pmicbatteryDebug, int, 0444);
    >
    > Please don't use camelCase in kernel.

    My bad, sorry... I am planning to remove this code anyway.


    >> +MODULE_PARM_DESC(pmicbatteryDebug,
    >> + "Flag to enable PMIC Battery debug messages.");
    >> +
    >> +#define PMIC_BATT_DEBUG (pmicbatteryDebug)
    >
    > I think you don't need this. There is a dynamic debug
    > infrastructure exist (see include/linux/device.h, dev_dbg()
    > stuff).

    Exactly, realized it very late and missed to update the patch with the
    change. Will take care...


    >> +/* battery cca value */
    >> +struct batt_cca_data {
    >> + signed int cca_val;
    >
    > Why explicitly state that this is signed?

    My bad... was not intended. Will correct.


    >> + unsigned int batt_charge_now; /* in mAS */
    >> + unsigned int batt_prev_charge_full; /* in mAS */
    >> + unsigned int batt_charge_rate; /* in units per second */
    >
    > Per include/linux/power_supply.h and
    > Documentation/power/power_supply_class.txt
    >
    > * All voltages, currents, charges, energies, time and temperatures in uV,
    > * uA, uAh, uWh, seconds and tenths of degree Celsius unless otherwise
    > * stated. It's driver's job to convert its raw values to units in which
    > * this class operates.

    I just now checked the hardware spec and it is indeed mAh. I will correct
    the comment appropriately.



    >> + pmic_batt_cmd.ioc = TRUE;
    >> + pmic_batt_cmd.cmd = IPC_BATT_CCA_READ;
    >> + if (ipc_config_cmd(pmic_batt_cmd, sizeof(struct batt_cca_data),
    >
    > I'd write it as
    >
    > ret = ipc_config_cmd(...);
    > if (ret) {
    > dev_warn(..., "%s(): ipc config cmd failed %d\n", __func__, ret);
    > return;
    > }
    >
    > But that's a matter of taste...

    :-)... I will accept the comment as it improves readability.


    >> + /* set batt_status */
    >> + if ((batt_present) && (!batt_exception)) {
    >
    > No need for these parenthesis.

    OK, I tried to be extra cautious... unnecessary I guess.


    >> + /* set batt_charge_rate */
    >> + if ((pbi->is_dev_info_updated) && (batt_present) && (!batt_exception)) {
    >
    > Ditto.

    I will remove it..


    >> + /* batt_charge_now */
    >> + if ((batt_present) && (!batt_exception))
    >
    > The parenthesis aren't needed.

    Will take care...


    >> + struct pmic_power_module_info *pbi =
    >> + (struct pmic_power_module_info *)dev;
    >
    >This type casting isn't needed.

    Yup, redundant type case... will clean.


    >> + schedule_work(&pbi->handler);
    >
    > I think you can use threaded irq for this.
    >
    > See documentation for request_threaded_irq() in kernel/irq/manage.c.
    > And as an example of usage see drivers/mfd/wm8350-irq.c.

    Haa that is useful information... completely missed to read about this
    feature. Will modify the code to make use of threaded IRQ.


    >> + /* check if pmic_power_module_info is initialized */
    >> + if (!pbi)
    >
    > This check is useless. container_of will always return non-NULL
    > result.

    Hmm good point... I guess I need to have some other clean mechanism to
    check it the module is initialized. Will take care in my next patch.


    >> + if (PMIC_BATT_DEBUG)
    >> + printk(KERN_INFO "pmic-battery: %s() - setting up battery "
    >> + "charger successful\n", __func__);
    >
    > dev_dbg().

    Yes will take care of this in my next patch.


    >> +static int pmic_battery_probe(struct spi_device *spi)
    >> +{
    >> + int retval = 0;
    >> + struct pmic_power_module_info *pbi = 0;
    >
    > Do not initialize pointers with 0. Plus, you don't actually need
    > to initialize pbi here.

    Accepted. Will take care...


    >> + /* initialize all required framework before enabling interrupts */
    >> + INIT_WORK(&pbi->handler, (void *)pmic_battery_handle_intrpt);
    >
    > No need for (void *) cast.

    Yup, redundant type case... will clean.


    >> + /* register interrupt */
    >> + retval = request_irq(pbi->irq, pmic_battery_interrupt_handler,
    >> + 0, DRIVER_NAME, pbi);
    >
    > I think you'd better use dev_name() instead of DRIVER_NAME here,
    > to distinguish interrupts from several devices.

    Good point. Will make the change...


    >> +
    >> + /* register pmic-batt with power supply subsystem */
    >> + pbi->batt.name = "pmic-batt";
    >
    > This won't work if we have several pmic batteries. I think you need
    > kasprintf(GFP_KERNEL, "%s-batt", dev_name(..));

    Got it. Will change as suggested.


    >> + /* register pmic-usb with power supply subsystem */
    >> + pbi->usb.name = "pmic-usb";
    >
    > kasprintf(GFP_KERNEL, "%s-usb", dev_name(..));

    OK, will incorporate the change as suggested.


    >> +/**
    >> + * pmic_battery_remove - pmic battery finalize
    >> + * @spi: pmic battery spi device structure
    >> + * Context: can sleep
    >> + *
    >> + * PMIC battery finalizes its internal data structue and other
    >> + * infrastructure components that it initialized in
    >> + * pmic_battery_probe.
    >> + */
    >> +static int pmic_battery_remove(struct spi_device *spi)
    >
    > __devexit?

    Haa right... bad miss.


    >> +{
    >> + struct pmic_power_module_info *pbi = dev_get_drvdata(&spi->dev);
    >> +
    >> + if (pbi) {
    >
    > The check isn't needed. _remove() won't run if device didn't probe
    > successfully.

    Good point. Will remove it.


    >> +};
    >> +
    >> +
    >
    > No need for two empty lines.

    OK


    Thank a lot for the comments Anton. I will incorporate all of these and will re-test
    on the hardware before submitting it again.


    Regards,
    Nithish
    \
     
     \ /
      Last update: 2010-01-15 13:07    [W:0.034 / U:32.540 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site