Date:	Thu, 17 Jun 2010 20:51:31 +0400
From:	Anton Vorontsov <cbouatmailru@...il.com>
To:	Alan Cox <alan@...ux.intel.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Intel MID platform battery driver.

On Thu, Jun 17, 2010 at 04:51:43PM +0100, Alan Cox wrote:
> From: Nithish Mahalingam <nithish.mahalingam@...el.com>
> 
> 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 <nithish.mahalingam@...el.com>
> Signed-off-by: Alan Cox <alan@...ux.intel.com>

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.

[...]
> +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 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.

FYI, this is the only real "merge-stopper" for me, because it's
actually ABI thing.

> +	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,
> +};
> +
> +
> +/*
> + * 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?).

> +};
> +
> +#define IPCMSG_BATTERY		0xEF
> +
[...]
> +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.

[...]
> +/**
> + * 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.

> +	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.

> +	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 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.

> +	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) {
> +		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?

> +	/* 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;
> +
> +	/* 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()?

> +}
> +
> +/**
> + * pmic_battery_probe - pmic battery initialize
> + * @irq: pmic battery device irq
> + * @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.

> +	if (debug)
> +		printk(KERN_INFO "pmic-battery: found pmic battery device\n");

dev_dbg()?

> +	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.

> +	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.

> +	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(..));

> +	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) {
> +		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()?

> +	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(dev, &pbi->usb);
> +	if (retval) {
> +		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()?

> +	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);
> +}
> +
> +/**
> + * 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.

> +{
> +	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.

> +		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;
> +}
> +
> +static int __devexit platform_pmic_battery_remove(struct platform_device *pdev)
> +{
> +	return remove(&pdev->dev);

This can be merged with remove() above.

> +}
> +
> +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)?

> +};
> +
> +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)
> +{
> +	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 <nithish.mahalingam@...el.com>");
> +MODULE_DESCRIPTION("Intel Moorestown PMIC Battery Driver");
> +MODULE_LICENSE("GPL");

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists