[PATCH] Adding Intel Moorestown PMIC Battery Driver
in [Kernel]
|
Prev: [PATCH 11/26] rtmutex: Convert wait_lock and pi_lock to raw_spinlock
Next: [resend][PATCH] mm, lockdep: annotate reclaim context to zone reclaim too
From: Anton Vorontsov on 11 Jan 2010 16:40 Thanks for the patch! On Mon, Jan 11, 2010 at 05:27:01AM +0530, Mahalingam, Nithish wrote: [...] > P.S. As I understand there is no mailing list for power supply subsystem, do > let me know if I need to add someone else for review. You can add lkml as well. Few comments down below. [...] > +#include <linux/module.h> > +#include <linux/init.h> > +#include <linux/err.h> > +#include <linux/interrupt.h> > +#include <linux/workqueue.h> > +#include <linux/jiffies.h> > +#include <linux/param.h> > +#include <linux/device.h> > +#include <linux/spi/spi.h> > +#include <linux/power_supply.h> > + > +#include <asm/ipc_defs.h> > +#include <linux/usb/langwell_udc.h> > + > + > +MODULE_AUTHOR("Nithish Mahalingam <nithish.mahalingam(a)intel.com>"); > +MODULE_DESCRIPTION("Intel Moorestown PMIC Battery Driver"); > +MODULE_LICENSE("GPL"); > + > +#define DRIVER_NAME "pmic_battery" > + > +/********************************************************************* > + * Generic defines > + *********************************************************************/ > + > +static int pmicbatteryDebug; > +module_param(pmicbatteryDebug, int, 0444); Please don't use camelCase in kernel. > +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). > + > +#define PMIC_BATT_DRV_INFO_UPDATED 1 > +#define PMIC_BATT_PRESENT 1 > +#define PMIC_BATT_NOT_PRESENT 0 > +#define PMIC_USB_PRESENT PMIC_BATT_PRESENT > +#define PMIC_USB_NOT_PRESENT PMIC_BATT_NOT_PRESENT > + > +/* pmic battery register related */ > +#define PMIC_BATT_CHR_SCHRGINT_ADDR 0xD2 > +#define PMIC_BATT_CHR_SBATOVP_MASK (1 << 1) > +#define PMIC_BATT_CHR_STEMP_MASK (1 << 2) > +#define PMIC_BATT_CHR_SCOMP_MASK (1 << 3) > +#define PMIC_BATT_CHR_SUSBDET_MASK (1 << 4) > +#define PMIC_BATT_CHR_SBATDET_MASK (1 << 5) > +#define PMIC_BATT_CHR_SDCLMT_MASK (1 << 6) > +#define PMIC_BATT_CHR_SUSBOVP_MASK (1 << 7) > +#define PMIC_BATT_CHR_EXCPT_MASK 0xC6 > +#define PMIC_BATT_ADC_ACCCHRG_MASK (1 << 31) > +#define PMIC_BATT_ADC_ACCCHRGVAL_MASK 0x7FFFFFFF > + > +/* pmic ipc related */ > +#define PMIC_BATT_CHR_IPC_CMDID 0xEF > +#define PMIC_BATT_CHR_IPC_FCHRG_SUBID 0x4 > +#define PMIC_BATT_CHR_IPC_TCHRG_SUBID 0x6 > + > +/* internal return values */ > +#define BATTSUCCESS 0 > +#define EBATTFAIL 1 > +#define EBATTERR 2 > + > +/* types of battery charging */ > +enum batt_charge_type { > + BATT_USBOTG_500MA_CHARGE, > + BATT_USBOTG_TRICKLE_CHARGE, > +}; > + > +/* valid battery events */ > +enum batt_event { > + BATT_EVENT_BATOVP_EXCPT, > + BATT_EVENT_USBOVP_EXCPT, > + BATT_EVENT_TEMP_EXCPT, > + BATT_EVENT_DCLMT_EXCPT, > + BATT_EVENT_EXCPT > +}; > + > +/* battery cca value */ > +struct batt_cca_data { > + signed int cca_val; Why explicitly state that this is signed? > +}; > + > +/* battery property structure */ > +struct batt_prop_data { > + unsigned int batt_capacity; > + char batt_chrg_crnt; > + char batt_chrg_volt; > + char batt_chrg_prot; > + char batt_chrg_prot2; > + char batt_chrg_timer; > +} __attribute__((packed)); > + > + > +/********************************************************************* > + * Battery properties > + *********************************************************************/ > + > +/* > + * pmic battery info > + */ > +struct pmic_power_module_info { > + bool is_dev_info_updated; > + struct spi_device *spi; > + /* pmic battery data */ > + unsigned long update_time; /* jiffies when data read */ > + unsigned int usb_is_present; > + unsigned int batt_is_present; > + unsigned int batt_health; > + unsigned int usb_health; > + unsigned int batt_status; > + 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. [...] > +static void pmic_battery_read_status(struct pmic_power_module_info *pbi) > +{ > + unsigned int update_time_intrvl = 0; > + unsigned int chrg_val = 0; > + struct ipc_pmic_reg_data pmic_batt_reg = {0}; > + struct ipc_cmd_type pmic_batt_cmd = {0}; > + struct batt_cca_data ccval = {0}; > + struct batt_prop_data batt_prop = {0}; > + int batt_present = 0; > + int usb_present = 0; > + int batt_exception = 0; > + > + /* make sure the last batt_status read happened delay_time before */ > + if (pbi->update_time && time_before(jiffies, pbi->update_time + > + msecs_to_jiffies(delay_time))) > + return; > + > + update_time_intrvl = jiffies_to_msecs(jiffies - pbi->update_time); > + pbi->update_time = jiffies; > + > + /* read coulomb counter registers and schrgint register */ > + > + 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... > + &ccval)) { > + dev_warn(&pbi->spi->dev, "%s(): ipc config cmd failed\n", > + __func__); > + return; > + } > + > + pmic_batt_reg.ioc = TRUE; > + pmic_batt_reg.pmic_reg_data[0].register_address = > + PMIC_BATT_CHR_SCHRGINT_ADDR; > + pmic_batt_reg.num_entries = 1; > + > + if (ipc_pmic_register_read(&pmic_batt_reg)) { > + dev_warn(&pbi->spi->dev, "%s(): ipc pmic read failed\n", > + __func__); > + return; > + } > + > + /* > + * set pmic_power_module_info members based on pmic register values > + * read. > + */ > + > + /* set batt_is_present */ > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SBATDET_MASK) { > + pbi->batt_is_present = PMIC_BATT_PRESENT; > + batt_present = 1; > + } else { > + pbi->batt_is_present = PMIC_BATT_NOT_PRESENT; > + pbi->batt_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + pbi->batt_status = POWER_SUPPLY_STATUS_UNKNOWN; > + } > + > + /* set batt_health */ > + if (batt_present) { > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SBATOVP_MASK) { > + pbi->batt_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + pbi->batt_status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + pmic_battery_log_event(BATT_EVENT_BATOVP_EXCPT); > + batt_exception = 1; > + } else if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SDCLMT_MASK) { > + pbi->batt_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + pbi->batt_status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + pmic_battery_log_event(BATT_EVENT_DCLMT_EXCPT); > + batt_exception = 1; > + } else if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_STEMP_MASK) { > + pbi->batt_health = POWER_SUPPLY_HEALTH_OVERHEAT; > + pbi->batt_status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + pmic_battery_log_event(BATT_EVENT_TEMP_EXCPT); > + batt_exception = 1; > + } else { > + pbi->batt_health = POWER_SUPPLY_HEALTH_GOOD; > + } > + } > + > + /* set usb_is_present */ > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SUSBDET_MASK) { > + pbi->usb_is_present = PMIC_USB_PRESENT; > + usb_present = 1; > + } else { > + pbi->usb_is_present = PMIC_USB_NOT_PRESENT; > + pbi->usb_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + } > + > + if (usb_present) { > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SUSBOVP_MASK) { > + pbi->usb_health = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + pmic_battery_log_event(BATT_EVENT_USBOVP_EXCPT); > + } else { > + pbi->usb_health = POWER_SUPPLY_HEALTH_GOOD; > + } > + } > + > + chrg_val = ccval.cca_val & PMIC_BATT_ADC_ACCCHRGVAL_MASK; > + > + /* set batt_prev_charge_full to battery capacity the first time */ > + if (!pbi->is_dev_info_updated) { > + pmic_batt_cmd.ioc = TRUE; > + pmic_batt_cmd.cmd = IPC_BATT_GET_PROP; > + if (ipc_config_cmd(pmic_batt_cmd, > + sizeof(struct batt_prop_data), &batt_prop)) { > + dev_warn(&pbi->spi->dev, "%s(): ipc config cmd " > + "failed\n", __func__); > + return; > + } > + pbi->batt_prev_charge_full = batt_prop.batt_capacity; > + } > + > + /* set batt_status */ > + if ((batt_present) && (!batt_exception)) { No need for these parenthesis. > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_SCOMP_MASK) { > + pbi->batt_status = POWER_SUPPLY_STATUS_FULL; > + pbi->batt_prev_charge_full = chrg_val; > + } else if (ccval.cca_val & PMIC_BATT_ADC_ACCCHRG_MASK) { > + pbi->batt_status = POWER_SUPPLY_STATUS_DISCHARGING; > + } else { > + pbi->batt_status = POWER_SUPPLY_STATUS_CHARGING; > + } > + } > + > + /* set batt_charge_rate */ > + if ((pbi->is_dev_info_updated) && (batt_present) && (!batt_exception)) { Ditto. > + if (pbi->batt_status == POWER_SUPPLY_STATUS_DISCHARGING) { > + if (pbi->batt_charge_now - chrg_val) { > + pbi->batt_charge_rate = ((pbi->batt_charge_now - > + chrg_val) * 1000 * 60) / > + update_time_intrvl; > + } > + } else if (pbi->batt_status == POWER_SUPPLY_STATUS_CHARGING) { > + if (chrg_val - pbi->batt_charge_now) { > + pbi->batt_charge_rate = ((chrg_val - > + pbi->batt_charge_now) * 1000 * 60) / > + update_time_intrvl; > + } > + } else > + pbi->batt_charge_rate = 0; > + } else { > + pbi->batt_charge_rate = -1; > + } > + > + /* batt_charge_now */ > + if ((batt_present) && (!batt_exception)) The parenthesis aren't needed. > + pbi->batt_charge_now = chrg_val; > + else > + pbi->batt_charge_now = -1; > + > + pbi->is_dev_info_updated = PMIC_BATT_DRV_INFO_UPDATED; > +} [...] > +/** > + * pmic_battery_interrupt_handler - pmic battery interrupt handler > + * Context: interrupt context > + * > + * PMIC battery interrupt handler which will be called with either > + * battery full condition occurs or usb otg & battery connect > + * condition occurs. > + */ > +static irqreturn_t pmic_battery_interrupt_handler(int id, void *dev) > +{ > + struct pmic_power_module_info *pbi = > + (struct pmic_power_module_info *)dev; This type casting isn't needed. > + 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. > + > + return IRQ_HANDLED; > +} > + > +/** > + * pmic_battery_handle_intrpt - pmic battery service interrupt > + * @work: work structure > + * Context: can sleep > + * > + * PMIC battery needs to either update the battery status as full > + * if it detects battery full condition caused the interrupt or needs > + * to enable battery charger if it detects usb and battery detect > + * caused the source of interrupt. > + */ > +static void pmic_battery_handle_intrpt(struct work_struct *work) > +{ > + struct ipc_pmic_reg_data pmic_batt_reg = {0}; > + struct pmic_power_module_info *pbi = container_of(work, > + struct pmic_power_module_info, handler); > + int power = 0; > + enum batt_charge_type chrg; > + int retval = 0; > + > + /* check if pmic_power_module_info is initialized */ > + if (!pbi) This check is useless. container_of will always return non-NULL result. > + return; > + > + /* read schrgint register to interpret cause of interrupt */ > + pmic_batt_reg.ioc = TRUE; > + pmic_batt_reg.pmic_reg_data[0].register_address = > + PMIC_BATT_CHR_SCHRGINT_ADDR; > + pmic_batt_reg.num_entries = 1; > + > + if (ipc_pmic_register_read(&pmic_batt_reg)) { > + dev_warn(&pbi->spi->dev, "%s(): ipc pmic read failed\n", > + __func__); > + return; > + } > + > + /* find the cause of the interrupt */ > + > + if (pmic_batt_reg.pmic_reg_data[0].value & PMIC_BATT_CHR_SBATDET_MASK) { > + pbi->batt_is_present = PMIC_BATT_PRESENT; > + } else { > + pbi->batt_is_present = PMIC_BATT_NOT_PRESENT; > + pbi->batt_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + pbi->batt_status = POWER_SUPPLY_STATUS_UNKNOWN; > + return; > + } > + > + if (pmic_batt_reg.pmic_reg_data[0].value & > + PMIC_BATT_CHR_EXCPT_MASK) { > + pbi->batt_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + pbi->batt_status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + pbi->usb_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + pmic_battery_log_event(BATT_EVENT_EXCPT); > + return; > + } else { > + pbi->batt_health = POWER_SUPPLY_HEALTH_GOOD; > + pbi->usb_health = POWER_SUPPLY_HEALTH_GOOD; > + } > + > + if (pmic_batt_reg.pmic_reg_data[0].value & PMIC_BATT_CHR_SCOMP_MASK) { > + struct ipc_cmd_type pmic_batt_cmd = {0}; > + struct batt_cca_data ccval = {0}; > + > + pbi->batt_status = POWER_SUPPLY_STATUS_FULL; > + 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), &ccval)) { > + dev_warn(&pbi->spi->dev, "%s(): ipc config cmd " > + "failed\n", __func__); > + return; > + } > + pbi->batt_prev_charge_full = ccval.cca_val & > + PMIC_BATT_ADC_ACCCHRGVAL_MASK; > + return; > + } > + > + if (pmic_batt_reg.pmic_reg_data[0].value & PMIC_BATT_CHR_SUSBDET_MASK) { > + pbi->usb_is_present = PMIC_USB_PRESENT; > + } else { > + pbi->usb_is_present = PMIC_USB_NOT_PRESENT; > + pbi->usb_health = POWER_SUPPLY_HEALTH_UNKNOWN; > + return; > + } > + > + /* setup battery charging */ > + > + /* check usb otg power capability and set charger accordingly */ > + retval = langwell_udc_maxpower(&power); > + if (retval) { > + dev_warn(&pbi->spi->dev, "%s(): usb otg power query failed " > + "with error code %d\n", __func__, retval); > + return; > + } > + > + if (power >= 500) > + chrg = BATT_USBOTG_500MA_CHARGE; > + else > + chrg = BATT_USBOTG_TRICKLE_CHARGE; > + > + /* enable battery charging */ > + if (pmic_battery_set_charger(pbi, chrg)) { > + dev_warn(&pbi->spi->dev, "%s(): failed to setup battery " > + "charging\n", __func__); > + return; > + } > + > + if (PMIC_BATT_DEBUG) > + printk(KERN_INFO "pmic-battery: %s() - setting up battery " > + "charger successful\n", __func__); dev_dbg(). > +} > + > +/** > + * pmic_battery_probe - pmic battery initialize > + * @spi: pmic battery spi structure > + * Context: can sleep > + * > + * PMIC battery initializes its internal data structue and other > + * infrastructure components for it to work as expected. > + */ > +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. > + > + if (PMIC_BATT_DEBUG) > + printk(KERN_INFO "pmic-battery: %s() - found pmic battery " > + "device\n", __func__); > + > + pbi = kzalloc(sizeof(*pbi), GFP_KERNEL); > + if (!pbi) { > + dev_err(&spi->dev, "%s(): memory allocation failed\n", > + __func__); > + return -ENOMEM; > + } > + > + pbi->spi = spi; > + pbi->irq = spi->irq; > + dev_set_drvdata(&spi->dev, pbi); > + > + /* initialize all required framework before enabling interrupts */ > + INIT_WORK(&pbi->handler, (void *)pmic_battery_handle_intrpt); No need for (void *) cast. > + INIT_DELAYED_WORK(&pbi->monitor_battery, pmic_battery_monitor); > + pbi->monitor_wqueue = > + create_singlethread_workqueue(dev_name(&spi->dev)); > + if (!pbi->monitor_wqueue) { > + dev_err(&spi->dev, "%s(): wqueue init failed\n", __func__); > + retval = -ESRCH; > + goto wqueue_failed; > + } > + > + /* 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. > + if (retval) { > + dev_err(&spi->dev, "%s(): cannot get IRQ\n", __func__); > + goto requestirq_failed; > + } > + > + /* 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(..)); > + pbi->batt.type = POWER_SUPPLY_TYPE_BATTERY; > + pbi->batt.properties = pmic_battery_props; > + pbi->batt.num_properties = ARRAY_SIZE(pmic_battery_props); > + pbi->batt.get_property = pmic_battery_get_property; > + retval = power_supply_register(&spi->dev, &pbi->batt); > + if (retval) { > + dev_err(&spi->dev, "%s(): failed to register pmic battery " > + "device with power supply subsystem\n", > + __func__); > + goto power_reg_failed; > + } > + > + if (PMIC_BATT_DEBUG) > + printk(KERN_INFO "pmic-battery: %s() - pmic battery device " > + "registration with power supply subsystem " > + "successful\n", __func__); > + > + queue_delayed_work(pbi->monitor_wqueue, &pbi->monitor_battery, HZ * 1); > + > + /* register pmic-usb with power supply subsystem */ > + pbi->usb.name = "pmic-usb"; kasprintf(GFP_KERNEL, "%s-usb", dev_name(..)); > + pbi->usb.type = POWER_SUPPLY_TYPE_USB; > + pbi->usb.properties = pmic_usb_props; > + pbi->usb.num_properties = ARRAY_SIZE(pmic_usb_props); > + pbi->usb.get_property = pmic_usb_get_property; > + retval = power_supply_register(&spi->dev, &pbi->usb); > + if (retval) { > + dev_err(&spi->dev, "%s(): failed to register pmic usb " > + "device with power supply subsystem\n", > + __func__); > + goto power_reg_failed_1; > + } > + > + if (PMIC_BATT_DEBUG) > + printk(KERN_INFO "pmic-battery: %s() - pmic usb device " > + "registration with power supply subsystem successful\n", > + __func__); > + > + return retval; > + > +power_reg_failed_1: > + power_supply_unregister(&pbi->batt); > +power_reg_failed: > + cancel_rearming_delayed_workqueue(pbi->monitor_wqueue, > + &pbi->monitor_battery); > +requestirq_failed: > + destroy_workqueue(pbi->monitor_wqueue); > +wqueue_failed: > + kfree(pbi); > + > + return retval; > +} > + > +/** > + * 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? > +{ > + 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. > + free_irq(pbi->irq, pbi); > + > + cancel_rearming_delayed_workqueue(pbi->monitor_wqueue, > + &pbi->monitor_battery); > + destroy_workqueue(pbi->monitor_wqueue); > + > + power_supply_unregister(&pbi->usb); > + power_supply_unregister(&pbi->batt); > + > + flush_scheduled_work(); > + > + kfree(pbi); > + } > + > + return 0; > +} > + > + > +/********************************************************************* > + * Driver initialisation and finalization > + *********************************************************************/ > + > +static struct spi_driver pmic_battery_driver = { > + .driver = { > + .name = DRIVER_NAME, > + .bus = &spi_bus_type, > + .owner = THIS_MODULE, > + }, > + .probe = pmic_battery_probe, > + .remove = __devexit_p(pmic_battery_remove), > +}; > + > + No need for two empty lines. > +static int __init pmic_battery_module_init(void) > +{ > + return spi_register_driver(&pmic_battery_driver); > +} > + > +static void __exit pmic_battery_module_exit(void) > +{ > + spi_unregister_driver(&pmic_battery_driver); > +} > + > +module_init(pmic_battery_module_init); > +module_exit(pmic_battery_module_exit); > -- > 1.6.5.2 Thanks! -- Anton Vorontsov email: cbouatmailru(a)gmail.com irc://irc.freenode.net/bd2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Mahalingam, Nithish on 12 Jan 2010 12:30 > Few comments down below. Thanks Anton. I will get back in a day's time with resolutions to these comments. Regards, Nithish Mahalingam
From: Mahalingam, Nithish on 15 Jan 2010 07:10 > 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
From: Mahalingam, Nithish on 15 Jan 2010 08:20 >>>> + 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. > > Note, if the hardware reports the values in mAh (milli), the driver > still have to convert them to uAh (micro) before reporting the values > to userspace. Yup will take care, sorry for not making it clear here. Regards, Nithish
From: Anton Vorontsov on 15 Jan 2010 08:20
On Fri, Jan 15, 2010 at 05:31:55PM +0530, Mahalingam, Nithish wrote: [...] > >> + 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. Note, if the hardware reports the values in mAh (milli), the driver still have to convert them to uAh (micro) before reporting the values to userspace. [...] > > 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. No wonder, it's just a new feature, not many know about it. ;-) Once again, thanks for the driver! -- Anton Vorontsov email: cbouatmailru(a)gmail.com irc://irc.freenode.net/bd2 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |