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 <[email protected]>");
> +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: [email protected]
irc://irc.freenode.net/bd2
> Few comments down below.
Thanks Anton. I will get back in a day's time with resolutions
to these comments.
Regards,
Nithish Mahalingam
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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: [email protected]
irc://irc.freenode.net/bd2
>>>> + 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
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Hi Anton,
First of all apologize for not sticking to my word to get back with
the patch as promised. Got tied up with some important work and could
not spend time on this patch.
I have started making changes today and will be testing it out after
the weekend. Will post the patch for review by mid next week for sure.
OK now to a question I had while working on the patch:
>>> + 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.
I have a very peculiar case - My hardware muxes 8 different types of
battery interrupts as one before forwarding it. I do the interrupt
de-muxing in the driver to find its real source; because I do this I
had used workqueue to make sure I do not keep the battery interrupt
source disabled for a long time and won't potential miss a battery
interrupt.
I was reading the request_threaded_irq code and found that only
when the threaded function returns interrupt should be re-enabled
(as it is suggested in the APIs function comment). Now if I do this,
won't I potential miss a interrupt?
Also I failed to understand what exact scenario request_threaded_irq
is useful over having a workqueue spawned inside a interrupt handler?
Is it only when you want to hold the interrupt EOI until the threaded
function returns?
Please shout back if I am making some stupid assumptions here.
Regards,
Nithish Mahalingam
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?