Messages in this thread | | | Date | Tue, 16 Dec 2008 01:38:11 +0300 | From | Anton Vorontsov <> | Subject | Re: [PATCH 5/7] power_supply: PCF50633 battery charger driver |
| |
Hello Balaji,
It's great to see OpenMoko patches submitted, much thanks for your work!
You can find few comments below.
On Sun, Dec 14, 2008 at 04:33:23PM +0530, Balaji Rao wrote: > Signed-off-by: Balaji Rao <balajirrao@openmoko.org> > Cc: Andy Green <andy@openmoko.com> > Cc: Anton Vorontsov <cbou@mail.ru> > Cc: David Woodhouse <dwmw2@infradead.org> > --- > drivers/power/Kconfig | 6 + > drivers/power/Makefile | 2 > drivers/power/pcf50633-charger.c | 285 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 293 insertions(+), 0 deletions(-) > create mode 100644 drivers/power/pcf50633-charger.c > > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig > index 52f8676..04f6316 100644 > --- a/drivers/power/Kconfig > +++ b/drivers/power/Kconfig > @@ -75,4 +75,10 @@ config BATTERY_BQ27x00 > help > Say Y here to enable support for batteries with BQ27200(I2C) chip. > > +config CHARGER_PCF50633 > + tristate "NXP PCF50633 MBC" > + depends on MFD_PCF50633 > + help > + Say Y to include support for NXP PCF50633 Main Battery Charger. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/Makefile b/drivers/power/Makefile > index e6f6865..62bcb76 100644 > --- a/drivers/power/Makefile > +++ b/drivers/power/Makefile > @@ -24,3 +24,5 @@ obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o > obj-$(CONFIG_BATTERY_TOSA) += tosa_battery.o > obj-$(CONFIG_BATTERY_WM97XX) += wm97xx_battery.o > obj-$(CONFIG_BATTERY_BQ27x00) += bq27x00_battery.o > + > +obj-$(CONFIG_CHARGER_PCF50633) += pcf50633-charger.o > diff --git a/drivers/power/pcf50633-charger.c b/drivers/power/pcf50633-charger.c > new file mode 100644 > index 0000000..d4a0de5 > --- /dev/null > +++ b/drivers/power/pcf50633-charger.c > @@ -0,0 +1,285 @@ > +/* NXP PCF50633 Main Battery Charger Driver > + * > + * (C) 2006-2008 by Openmoko, Inc. > + * Author: Balaji Rao <balajirrao@openmoko.org> > + * All rights reserved. > + * > + * Broken down from monstrous PCF50633 driver mainly by > + * Harald Welte, Andy Green and Werner Almesberger > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, > + * MA 02111-1307 USA > + */ > +
The code should explicitly include all the needed headers.
#include <linux/kernel.h> (for container_of, sprintf, ...) #include <linux/module.h> (MODULE_, EXPORT_SYMBOL macros) #include <linux/init.h> (initcalls) #include <linux/types.h> (ssize_t) #include <linux/device.h> (dev_info e.t.c.) #include <linux/platform_device.h> #include <linux/power_supply.h>
> +#include <linux/mfd/pcf50633/core.h> > +#include <linux/mfd/pcf50633/mbc.h> > + > +void pcf50633_mbc_usb_curlim_set(struct pcf50633 *pcf, int ma) > +{ > + int ret; > + u8 bits; > + > + if (ma >= 1000) > + bits = PCF50633_MBCC7_USB_1000mA; > + else if (ma >= 500) > + bits = PCF50633_MBCC7_USB_500mA; > + else if (ma >= 100) > + bits = PCF50633_MBCC7_USB_100mA; > + else > + bits = PCF50633_MBCC7_USB_SUSPEND; > + > + ret = pcf50633_reg_set_bit_mask(pcf, PCF50633_REG_MBCC7, > + PCF50633_MBCC7_USB_MASK, bits); > + if (ret) > + dev_err(pcf->dev, "error setting usb curlim to %d mA\n", ma); > + else > + dev_info(pcf->dev, "usb curlim to %d mA\n", ma); > + > + power_supply_changed(&pcf->mbc.usb); > +} > +EXPORT_SYMBOL_GPL(pcf50633_mbc_usb_curlim_set); > + > +static ssize_t > +show_chgmode(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct pcf50633 *pcf = dev_get_drvdata(dev); > +
Stray empty line.
> + u8 mbcs2 = pcf50633_reg_read(pcf, PCF50633_REG_MBCS2); > + u8 chgmod = (mbcs2 & PCF50633_MBCS2_MBC_MASK); > + > + return sprintf(buf, "%d\n", chgmod); > +} > +static DEVICE_ATTR(chgmode, S_IRUGO, show_chgmode, NULL); > + > +static ssize_t > +show_usblim(struct device *dev, struct device_attribute *attr, char *buf) > +{ > + struct pcf50633 *pcf = dev_get_drvdata(dev); > + u8 usblim = pcf50633_reg_read(pcf, PCF50633_REG_MBCC7) & > + PCF50633_MBCC7_USB_MASK; > + unsigned int ma; > + > + if (usblim == PCF50633_MBCC7_USB_1000mA) > + ma = 1000; > + else if (usblim == PCF50633_MBCC7_USB_500mA) > + ma = 500; > + else if (usblim == PCF50633_MBCC7_USB_100mA) > + ma = 100; > + else > + ma = 0; > + > + return sprintf(buf, "%u\n", ma); > +} > + > +static ssize_t set_usblim(struct device *dev, > + struct device_attribute *attr, const char *buf, size_t count) > +{ > + struct pcf50633 *pcf = dev_get_drvdata(dev); > + unsigned long ma; > + int ret; > + > + ret = strict_strtoul(buf, 10, &ma); > + if (ret) > + return -EINVAL; > + > + pcf50633_mbc_usb_curlim_set(pcf, ma); > + > + return count; > +} > + > +static DEVICE_ATTR(usb_curlim, S_IRUGO | S_IWUSR, show_usblim, set_usblim); > + > +static struct attribute *pcf50633_mbc_sysfs_entries[] = { > + &dev_attr_chgmode.attr, > + &dev_attr_usb_curlim.attr, > + NULL, > +}; > + > +static struct attribute_group mbc_attr_group = { > + .name = NULL, /* put in device directory */ > + .attrs = pcf50633_mbc_sysfs_entries, > +}; > + > +static void > +pcf50633_mbc_irq_handler(struct pcf50633 *pcf, int irq, void *unused) > +{ > + struct pcf50633_mbc *mbc; > + > + mbc = &pcf->mbc;
struct pcf50633_mbc *mbc = &pcf->mbc; would save us two lines.
> + > + /* USB */ > + if (irq == PCF50633_IRQ_USBINS) { > + mbc->usb_online = 1; > + } else if (irq == PCF50633_IRQ_USBREM) { > + mbc->usb_online = 0; > + mbc->usb_active = 0; > + pcf50633_mbc_usb_curlim_set(pcf, 0); > + } > + > + /* Adapter */ > + if (irq == PCF50633_IRQ_ADPINS) { > + pcf->mbc.adapter_online = 1; > + pcf->mbc.adapter_active = 1; > + } else if (irq == PCF50633_IRQ_ADPREM) { > + mbc->adapter_online = 0; > + mbc->adapter_active = 0; > + } > + > + if (irq == PCF50633_IRQ_BATFULL) { > + mbc->usb_active = 0; > + mbc->adapter_active = 0; > + } > + > + power_supply_changed(&mbc->usb); > + power_supply_changed(&mbc->adapter); > + > + if (pcf->pdata->mbc_event_callback) > + pcf->pdata->mbc_event_callback(pcf, irq); > +} > + > +static int adapter_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + int ret = 0; > + struct pcf50633_mbc *mbc = container_of(psy, struct pcf50633_mbc, usb); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + val->intval = mbc->adapter_online; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + return ret; > +} > + > +static int usb_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + int ret = 0; > + struct pcf50633_mbc *mbc = container_of(psy, struct pcf50633_mbc, usb); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_ONLINE: > + val->intval = mbc->usb_online; > + break; > + default: > + ret = -EINVAL; > + break; > + } > + return ret; > +} > + > +static enum power_supply_property power_props[] = { > + POWER_SUPPLY_PROP_ONLINE, > +}; > + > +static const u8 mbc_irq_handlers[] = { > + PCF50633_IRQ_ADPINS, > + PCF50633_IRQ_ADPREM, > + PCF50633_IRQ_USBINS, > + PCF50633_IRQ_USBREM, > + PCF50633_IRQ_BATFULL, > + PCF50633_IRQ_CHGHALT, > + PCF50633_IRQ_THLIMON, > + PCF50633_IRQ_THLIMOFF, > + PCF50633_IRQ_USBLIMON, > + PCF50633_IRQ_USBLIMOFF, > + PCF50633_IRQ_LOWSYS, > + PCF50633_IRQ_LOWBAT, > +}; > + > +int __init pcf50633_mbc_probe(struct platform_device *pdev) > +{ > + struct pcf50633 *pcf; > + struct pcf50633_mbc *mbc; > + int ret, i;
int ret; int i;
> + > + pcf = platform_get_drvdata(pdev); > + mbc = &pcf->mbc;
The two lines can be placed on the same lines with variables definitions.
> + > + /* Set up IRQ handlers */ > + for (i = 0; i < ARRAY_SIZE(mbc_irq_handlers); i++) > + pcf->irq_handler[mbc_irq_handlers[i]] = > + pcf50633_mbc_irq_handler;
Ugh. Is there any particular reason why you don't implement a chained interrupt controller in the PCF core? (as in drivers/mfd/asic3.c, for example).
That way you could do ordinary request_irq() for the MFD devices' interrupts.
(I can only guess: you didn't make it because PCF is on the I2C bus, and it's just easier to handle devices via single workqueue in the core driver?)
> + /* Create power supplies */ > + mbc->adapter.name = "adapter"; > + mbc->adapter.type = POWER_SUPPLY_TYPE_MAINS; > + mbc->adapter.properties = power_props; > + mbc->adapter.num_properties = ARRAY_SIZE(power_props); > + mbc->adapter.get_property = &adapter_get_property; > + mbc->adapter.supplied_to = pcf->pdata->batteries; > + mbc->adapter.num_supplicants = pcf->pdata->num_batteries; > + > + mbc->usb.name = "usb"; > + mbc->usb.type = POWER_SUPPLY_TYPE_USB; > + mbc->usb.properties = power_props; > + mbc->usb.num_properties = ARRAY_SIZE(power_props); > + mbc->usb.get_property = usb_get_property; > + mbc->usb.supplied_to = pcf->pdata->batteries; > + mbc->usb.num_supplicants = pcf->pdata->num_batteries; > + > + ret = power_supply_register(&pdev->dev, &mbc->adapter); > + if (ret) > + dev_err(pcf->dev, "failed to register adapter\n"); > + > + ret = power_supply_register(&pdev->dev, &mbc->usb); > + if (ret) > + dev_err(pcf->dev, "failed to register usb\n"); > + > + return sysfs_create_group(&pdev->dev.kobj, &mbc_attr_group);
If this fail we'll leak two registered power supplies...
> +} > + > +static int __devexit pcf50633_mbc_remove(struct platform_device *pdev) > +{ > + struct pcf50633 *pcf; > + > + pcf = platform_get_drvdata(pdev);
This could be placed on a single line.
> + > + /* Remove IRQ handlers */ > + for (i = 0; i < ARRAY_SIZE(mbc_irq_handlers); i++) > + pcf->irq_handler[mbc_irq_handlers[i]] = NULL; > + > + return 0;
The function doesn't handle power supplies unregistration?
> +} > + > +struct platform_driver pcf50633_mbc_driver = { > + .driver = { > + .name = "pcf50633-mbc", > + }, > + .probe = pcf50633_mbc_probe, > + .remove = __devexit_p(pcf50633_mbc_remove), > +}; > + > +static int __init pcf50633_mbc_init(void) > +{ > + return platform_driver_register(&pcf50633_mbc_driver);
One tab is enough.
> +} > +module_init(pcf50633_mbc_init); > + > +static void __exit pcf50633_mbc_exit(void) > +{ > + platform_driver_unregister(&pcf50633_mbc_driver);
Ditto.
> +} > +module_exit(pcf50633_mbc_exit); > + > +MODULE_AUTHOR("Balaji Rao <balajirrao@openmoko.org>"); > +MODULE_DESCRIPTION("PCF50633 mbc driver"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:pcf50633-mbc");
Thanks again,
-- Anton Vorontsov email: cbouatmailru@gmail.com irc://irc.freenode.net/bd2
| |