Messages in this thread | | | From | "Mahalingam, Nithish" <> | Date | Fri, 15 Jan 2010 17:31:55 +0530 | Subject | RE: [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
| |