Intel MID platform battery driver.
PATCH 1/1 Intel MID platform battery driver.
From: Nithish Mahalingam <[email protected]>
The PMIC Battery driver provides battery charging and battery gauge functionality on Intel MID platforms. This provides the basic functions. There are some USB drivers to merge before the selection of charging between the different USB power levels can be enabled.
Moved to a platform device by Alek Du.
---
drivers/power/Kconfig | 7 drivers/power/Makefile | 1 drivers/power/intel_mid_battery.c | 813 +++++++++++++++++++++++++++++++++++++ 3 files changed, 821 insertions(+), 0 deletions(-) create mode 100644 drivers/power/intel_mid_battery.c
drivers/power/Kconfig +7 -0
@@ -142,4 +142,11 @@ config CHARGER_PCF50633 help Say Y to include support for NXP PCF50633 Main Battery Charger. +config BATTERY_INTEL_MID + tristate "Battery driver for Intel MID platforms" + depends on INTEL_SCU_IPC && SPI + help + Say Y here to enable the battery driver on Intel MID + platforms. + endif # POWER_SUPPLY
drivers/power/Makefile +1 -0
@@ -34,3 +34,4 @@ obj-$(CONFIG_BATTERY_DA9030) += da9030_battery.o obj-$(CONFIG_BATTERY_MAX17040) += max17040_battery.o obj-$(CONFIG_BATTERY_Z2) += z2_battery.o obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o +obj-$(CONFIG_BATTERY_INTEL_MID) += intel_mid_battery.o
drivers/power/intel_mid_battery.c new file +813 -0
813 lines not on this page · raw message
Quoted 11 lines · Alan Cox
From: Nithish Mahalingam <[email protected]>
The PMIC Battery driver provides battery charging and battery gauge functionality on Intel MID platforms. This provides the basic functions. There are some USB drivers to merge before the selection of charging between the different USB power levels can be enabled.
Moved to a platform device by Alek Du.
Signed-off-by Nithish Mahalingam <[email protected]>Signed-off-by Alan Cox <[email protected]>
Thanks for the patch.
I reviewed this driver already:
http://lkml.org/lkml/2010/1/11/277
and I see that some (most) comments aren't addressed.
[...]
drivers/power/intel_mid_battery.c line 107
Quoted 13 lines
drivers/power/intel_mid_battery.c new file +13
-0+struct pmic_power_module_info { +bool is_dev_info_updated; +struct device *dev; +/* 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 */
pmic_battery_get_property() returns these mAS values, which is wrong.
Please see include/linux/power_supply.h andDocumentation/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.
FYI, this is the only real "merge-stopper" for me, because it's actually ABI thing.
Quoted 32 lines
drivers/power/intel_mid_battery.c new file +32
-0+struct power_supply usb; +struct power_supply batt; +int irq;/* GPE_ID or IRQ# */ +struct workqueue_struct *monitor_wqueue; +struct delayed_work monitor_battery; +struct work_struct handler; +}; + +static unsigned int delay_time =2000 ;/* in ms */ + +/* +* pmic ac properties +*/ +static enum power_supply_property pmic_usb_props[] = { +POWER_SUPPLY_PROP_PRESENT , +POWER_SUPPLY_PROP_HEALTH , +}; + +/* +* pmic battery properties +*/ +static enum power_supply_property pmic_battery_props[] = { +POWER_SUPPLY_PROP_STATUS , +POWER_SUPPLY_PROP_HEALTH , +POWER_SUPPLY_PROP_PRESENT , +POWER_SUPPLY_PROP_CHARGE_NOW , +POWER_SUPPLY_PROP_CHARGE_FULL , +POWER_SUPPLY_PROP_CHARGE_AVG , +}; + + +/*
drivers/power/intel_mid_battery.c line 150
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+* Glue functions for talking to the IPC +*/ + +struct battery_property { +u32 capacity;/* Charger capacity */ +u8 crnt;/* Quick charge current value*/ +u8 volt;/* Fine adjustment of constant charge voltage */ +u8 prot;/* CHRGPROT register value */ +u8 prot2;/* CHRGPROT1 register value */ +u8 timer;/* Charging timer */
Whitespace issue(s?).
Quoted 4 lines
drivers/power/intel_mid_battery.c new file +4
-0+}; + +#define IPCMSG_BATTERY 0xEF +
[...]
drivers/power/intel_mid_battery.c line 263
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+static void pmic_battery_read_status (struct pmic_power_module_info *pbi) +{ +unsigned int update_time_intrvl; +unsigned int chrg_val; +u32 ccval; +u8 r8; +struct battery_property batt_prop; +int batt_present =0 ; +int usb_present =0 ; +int batt_exception =0 ;
Stray whitespace.
[...]
drivers/power/intel_mid_battery.c line 540
Quoted 12 lines
drivers/power/intel_mid_battery.c new file +12
-0+/** +* 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;
The type cast isn't needed.
drivers/power/intel_mid_battery.c line 542
Quoted 1 line
drivers/power/intel_mid_battery.c new file +1
-0+schedule_work (&pbi->handler);
In the previous review I mentioned that you probably could use request_threaded_irq().
I'm not insisting on using it, i.e. I'm OK with schedule_work() for now, but I'm just curious why you still don't use threaded IRQ handlers.
Quoted 13 lines
drivers/power/intel_mid_battery.c new file +13
-0+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. +*/
drivers/power/intel_mid_battery.c line 566
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+static void pmic_battery_handle_intrpt (struct work_struct *work) +{ +struct pmic_power_module_info *pbi =container_of (work, + struct pmic_power_module_info, handler); +enum batt_charge_type chrg; +u8 r8; + +/* check if pmic_power_module_info is initialized */ +if (!pbi) +return ;
This check is useless. container_of always returns non-NULL result.
Quoted 41 lines
drivers/power/intel_mid_battery.c new file +41
-0+if (intel_scu_ipc_ioread8 (PMIC_BATT_CHR_SCHRGINT_ADDR , &r8)) { +dev_warn (pbi->dev,"%s(): ipc pmic read failed\n" , + __func__); +return ; + } +/* find the cause of the interrupt */ +if (r8 &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 (r8 &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 (r8 &PMIC_BATT_CHR_SCOMP_MASK ) { +u32 ccval; + pbi->batt_status =POWER_SUPPLY_STATUS_FULL ; + +if (pmic_scu_ipc_battery_cc_read (&ccval)) { +dev_warn (pbi->dev,"%s(): ipc config cmd " +"failed\n" , __func__); +return ; + } + pbi->batt_prev_charge_full = ccval & +PMIC_BATT_ADC_ACCCHRGVAL_MASK ; +return ; + } + +if (r8 &PMIC_BATT_CHR_SUSBDET_MASK ) {
drivers/power/intel_mid_battery.c line 618
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+ 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 */ + +#if 0
Dead code? Is this a leftover, or you're going to use it soon?
Quoted 14 lines
drivers/power/intel_mid_battery.c new file +14
-0+/* check usb otg power capability and set charger accordingly */ + retval =langwell_udc_maxpower (&power); +if (retval) { +dev_warn (pbi->dev,"%s(): usb otg power query failed " +"with error code %d\n" , __func__, retval); +return ; + } + +if (power >=500 ) + chrg =BATT_USBOTG_500MA_CHARGE ; + else +#endif + chrg =BATT_USBOTG_TRICKLE_CHARGE ; +
drivers/power/intel_mid_battery.c line 642
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+/* enable battery charging */ +if (pmic_battery_set_charger (pbi, chrg)) { +dev_warn (pbi->dev, +"%s(): failed to set up battery charging\n" , __func__); +return ; + } + +if (debug) +printk (KERN_INFO "pmic-battery: %s() - setting up battery " +"charger successful\n" , __func__);
dev_dbg()?
Quoted 5 lines
drivers/power/intel_mid_battery.c new file +5
-0+} + +/** +* pmic_battery_probe - pmic battery initialize +* @irq: pmic battery device irq
drivers/power/intel_mid_battery.c line 657
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+* @dev: pmic battery device structure +* Context: can sleep +* +* PMIC battery initializes its internal data structue and other +* infrastructure components for it to work as expected. +*/ +static __devinit int probe (int irq,struct device *dev) +{ +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.
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (debug) + printk(KERN_INFO "pmic-battery: found pmic battery device\n");
dev_dbg()?
drivers/power/intel_mid_battery.c line 674
Quoted 13 lines
drivers/power/intel_mid_battery.c new file +13
-0+ pbi =kzalloc (sizeof (*pbi),GFP_KERNEL ); +if (!pbi) { +dev_err (dev,"%s(): memory allocation failed\n" , + __func__); +return -ENOMEM ; + } + + pbi->dev = dev; + pbi->irq = irq; +dev_set_drvdata (dev, pbi); + +/* initialize all required framework before enabling interrupts */ +INIT_WORK (&pbi->handler, (void *)pmic_battery_handle_intrpt);
No need for the (void *) cast.
drivers/power/intel_mid_battery.c line 686
Quoted 12 lines
drivers/power/intel_mid_battery.c new file +12
-0+INIT_DELAYED_WORK (&pbi->monitor_battery, pmic_battery_monitor); + pbi->monitor_wqueue = +create_singlethread_workqueue (dev_name (dev)); +if (!pbi->monitor_wqueue) { +dev_err (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.
drivers/power/intel_mid_battery.c line 693
Quoted 7 lines
drivers/power/intel_mid_battery.c new file +7
-0+if (retval) { +dev_err (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(..));
Quoted 6 lines
drivers/power/intel_mid_battery.c new file +6
-0+ 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 (dev, &pbi->batt); +if (retval) {
drivers/power/intel_mid_battery.c line 709
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+dev_err (dev, +"%s(): failed to register pmic battery device with power supply subsystem\n" , + __func__); + goto power_reg_failed; + } + +if (debug) +printk (KERN_INFO "pmic-battery: %s() - pmic battery device " +"registration with power supply subsystem " +"successful\n" , __func__);
dev_info()?
drivers/power/intel_mid_battery.c line 714
Quoted 4 lines
drivers/power/intel_mid_battery.c new file +4
-0+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(..));
Quoted 6 lines
drivers/power/intel_mid_battery.c new file +6
-0+ 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 (dev, &pbi->usb); +if (retval) {
drivers/power/intel_mid_battery.c line 730
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+dev_err (dev, +"%s(): failed to register pmic usb device with power supply subsystem\n" , + __func__); + goto power_reg_failed_1; + } + +if (debug) +printk (KERN_INFO "pmic-battery: %s() - pmic usb device " +"registration with power supply subsystem successful\n" , + __func__);
dev_info()?
Quoted 21 lines
drivers/power/intel_mid_battery.c new file +21
-0+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; +} + +static int __devinit platform_pmic_battery_probe (struct platform_device *pdev) +{ +return probe (pdev->id, &pdev->dev); +} + +/**
drivers/power/intel_mid_battery.c line 762
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+* pmic_battery_remove - pmic battery finalize +* @dev: pmic battery 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 __devexit remove(struct device *dev)
Looks like too broad identifier.
drivers/power/intel_mid_battery.c line 766
Quoted 4 lines
drivers/power/intel_mid_battery.c new file +4
-0+{ +struct pmic_power_module_info *pbi =dev_get_drvdata (dev); + +if (pbi) {
The check isn't needed. _remove() won't run if device didn't probe successfully.
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+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 ();
drivers/power/intel_mid_battery.c line 786
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+ +kfree (pbi); + } + +return 0 ; +} + +static int __devexit platform_pmic_battery_remove(struct platform_device *pdev) +{ +return remove (&pdev->dev);
This can be merged with remove() above.
drivers/power/intel_mid_battery.c line 795
Quoted 9 lines
drivers/power/intel_mid_battery.c new file +9
-0+} + +static struct platform_driver platform_pmic_battery_driver = { + .driver = { + .name =DRIVER_NAME , + .owner =THIS_MODULE , + }, + .probe = platform_pmic_battery_probe, + .remove = platform_pmic_battery_remove,
__devexit_p(platform_pmic_battery_remove)?
Quoted 8 lines
drivers/power/intel_mid_battery.c new file +8
-0+}; + +static int __init platform_pmic_battery_module_init (void ) +{ +return platform_driver_register (&platform_pmic_battery_driver); +} + +static void __exit platform_pmic_battery_module_exit(void)
drivers/power/intel_mid_battery.c line 813
Quoted 10 lines
drivers/power/intel_mid_battery.c new file +10
-0+{ +platform_driver_unregister (&platform_pmic_battery_driver); +} + +module_init (platform_pmic_battery_module_init); +module_exit (platform_pmic_battery_module_exit); + +MODULE_AUTHOR ("Nithish Mahalingam <[email protected]>" ); +MODULE_DESCRIPTION ("Intel Moorestown PMIC Battery Driver" ); +MODULE_LICENSE ("GPL" );
Thanks!
Signature · 3 lines
Anton Vorontsov email: [email protected] irc://irc.freenode.net/bd2