Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753371AbYLRU4G (ORCPT ); Thu, 18 Dec 2008 15:56:06 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1752136AbYLRUzx (ORCPT ); Thu, 18 Dec 2008 15:55:53 -0500 Received: from rtsoft3.corbina.net ([85.21.88.6]:27035 "EHLO buildserver.ru.mvista.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752126AbYLRUzw (ORCPT ); Thu, 18 Dec 2008 15:55:52 -0500 Date: Thu, 18 Dec 2008 23:55:50 +0300 From: Anton Vorontsov To: Mike Rapoport Cc: Samuel Ortiz , LKML , eric miao , cbou@mail.ru, David Woodhouse , Jonathan Cameron , Andrew Morton Subject: Re: [RFC PATCH 2/2] Add Dialog DA9030 battery charger driver Message-ID: <20081218205550.GA25717@oksana.dev.rtsoft.ru> Reply-To: avorontsov@ru.mvista.com References: <4948B09A.70108@compulab.co.il> <20081218101713.GB2517@sortiz.org> <494A35BC.4090905@compulab.co.il> MIME-Version: 1.0 Content-Type: text/plain; charset=windows-1251 Content-Disposition: inline In-Reply-To: <494A35BC.4090905@compulab.co.il> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi all, On Thu, Dec 18, 2008 at 01:36:28PM +0200, Mike Rapoport wrote: [...] > > 2 remarks though: > > 1) I'd like to get Anton Ack on it. Samuel, as it depends on the MFD part, feel free to merge it into your tree. Acked-by: Anton Vorontsov > > 2) This patch wont build, as the Makefile is trying to build da903x_battery.o > > while da9030_battery.c is created. I guess it'd make sense to rename it > > da903x_battery.c. > > Last minute changes... > I actually prefer to keep it da9030 for now because I have no idea how much > DA9034 charger differs from DA9030. > So, here's the updated patch that fixes Kconfig and Makefile entries and > addresses the points Andrew raised at [1] Mike, Thanks for the patch, looks really good. Few comments down below. [...] > +++ b/drivers/power/da9030_battery.c > @@ -0,0 +1,593 @@ > +/* > + * Battery charger driver for Dialog Semiconductor DA9030 > + * > + * Copyright (C) 2008 Compulab, Ltd. > + * Mike Rapoport > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + The code should explicitly include all the needed headers. #include (for container_of, sprintf, ...) #include (initcalls) #include (uint8_t) #include (struct device) #include (delayedwork) ^^^ This is the only real comment. All the rest are nitpicks, which you can freely ignore if you like. > +#include > +#include > +#include > +#include > + > +#include > +#include > + > +#define DA9030_STATUS_CHDET (1 << 3) > + > +#define DA9030_FAULT_LOG 0x0a > +#define DA9030_FAULT_LOG_OVER_TEMP (1 << 7) > +#define DA9030_FAULT_LOG_VBAT_OVER (1 << 4) > + > +#define DA9030_CHARGE_CONTROL 0x28 > +#define DA9030_CHRG_CHARGER_ENABLE (1 << 7) > + > +#define DA9030_ADC_MAN_CONTROL 0x30 > +#define DA9030_ADC_TBATREF_ENABLE (1 << 5) > +#define DA9030_ADC_LDO_INT_ENABLE (1 << 4) > + > +#define DA9030_ADC_AUTO_CONTROL 0x31 > +#define DA9030_ADC_TBAT_ENABLE (1 << 5) > +#define DA9030_ADC_VBAT_IN_TXON (1 << 4) > +#define DA9030_ADC_VCH_ENABLE (1 << 3) > +#define DA9030_ADC_ICH_ENABLE (1 << 2) > +#define DA9030_ADC_VBAT_ENABLE (1 << 1) > +#define DA9030_ADC_AUTO_SLEEP_ENABLE (1 << 0) > + > +#define DA9030_VBATMON 0x32 > +#define DA9030_VBATMONTXON 0x33 > +#define DA9030_TBATHIGHP 0x34 > +#define DA9030_TBATHIGHN 0x35 > +#define DA9030_TBATLOW 0x36 > + > +#define DA9030_VBAT_RES 0x41 > +#define DA9030_VBATMIN_RES 0x42 > +#define DA9030_VBATMINTXON_RES 0x43 > +#define DA9030_ICHMAX_RES 0x44 > +#define DA9030_ICHMIN_RES 0x45 > +#define DA9030_ICHAVERAGE_RES 0x46 > +#define DA9030_VCHMAX_RES 0x47 > +#define DA9030_VCHMIN_RES 0x48 > +#define DA9030_TBAT_RES 0x49 > + > +struct da9030_adc_res { > + uint8_t vbat_res; > + uint8_t vbatmin_res; > + uint8_t vbatmintxon; > + uint8_t ichmax_res; > + uint8_t ichmin_res; > + uint8_t ichaverage_res; > + uint8_t vchmax_res; > + uint8_t vchmin_res; > + uint8_t tbat_res; > + uint8_t adc_in4_res; > + uint8_t adc_in5_res; > +}; > + > +struct da9030_battery_thresholds { > + int tbat_low; > + int tbat_high; > + int tbat_restart; > + > + int vbat_low; > + int vbat_crit; > + int vbat_charge_start; > + int vbat_charge_stop; > + int vbat_charge_restart; > + > + int vcharge_min; > + int vcharge_max; > +}; > + > +struct da9030_charger { > + struct power_supply psy; > + > + struct device *master; > + > + struct da9030_adc_res adc; > + struct delayed_work work; > + unsigned int interval; > + > + struct power_supply_info *battery_info; > + > + struct da9030_battery_thresholds thresholds; > + > + unsigned int charge_milliamp; > + unsigned int charge_millivolt; > + > + /* charger status */ > + int chdet:1; In kernel we can use bool type, and true/false constants. > + uint8_t fault; > + int mA; > + int mV; > + int is_on; > + > + struct notifier_block nb; > + > + /* platform callbacks for battery low and critical events */ > + void (*battery_low)(void); > + void (*battery_critical)(void); > + > + struct dentry *debug_file; > +}; > + > +static inline int da9030_reg_to_mV(int reg) > +{ > + return ((reg * 2650) >> 8) + 2650; > +} > + > +static inline int da9030_millivolt_to_reg(int mV) > +{ > + return ((mV - 2650) << 8) / 2650; > +} > + > +static inline int da9030_reg_to_mA(int reg) > +{ > + return ((reg * 24000) >> 8) / 15; > +} > + > +#ifdef CONFIG_DEBUG_FS > + (1) > +static int bat_debug_show(struct seq_file *s, void *data) > +{ > + struct da9030_charger *charger = s->private; > + > + seq_printf(s, "charger is %s\n", charger->is_on ? "on" : "off"); > + if (charger->chdet) { > + seq_printf(s, "iset = %dmA, vset = %dmV\n", > + charger->mA, charger->mV); > + } > + > + seq_printf(s, "vbat_res = %d (%dmV)\n", > + charger->adc.vbat_res, > + da9030_reg_to_mV(charger->adc.vbat_res)); > + seq_printf(s, "vbatmin_res = %d (%dmV)\n", > + charger->adc.vbatmin_res, > + da9030_reg_to_mV(charger->adc.vbatmin_res)); > + seq_printf(s, "vbatmintxon = %d (%dmV)\n", > + charger->adc.vbatmintxon, > + da9030_reg_to_mV(charger->adc.vbatmintxon)); > + seq_printf(s, "ichmax_res = %d (%dmA)\n", > + charger->adc.ichmax_res, > + da9030_reg_to_mV(charger->adc.ichmax_res)); > + seq_printf(s, "ichmin_res = %d (%dmA)\n", > + charger->adc.ichmin_res, > + da9030_reg_to_mA(charger->adc.ichmin_res)); > + seq_printf(s, "ichaverage_res = %d (%dmA)\n", > + charger->adc.ichaverage_res, > + da9030_reg_to_mA(charger->adc.ichaverage_res)); > + seq_printf(s, "vchmax_res = %d (%dmV)\n", > + charger->adc.vchmax_res, > + da9030_reg_to_mA(charger->adc.vchmax_res)); > + seq_printf(s, "vchmin_res = %d (%dmV)\n", > + charger->adc.vchmin_res, > + da9030_reg_to_mV(charger->adc.vchmin_res)); > + > + return 0; > +} > + > +static int debug_open(struct inode *inode, struct file *file) > +{ > + return single_open(file, bat_debug_show, inode->i_private); > +} > + > +static const struct file_operations bat_debug_fops = { > + .open = debug_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = single_release, > +}; > + > +static struct dentry *da9030_bat_create_debugfs(struct da9030_charger *charger) > +{ > + charger->debug_file = debugfs_create_file("charger", 0666, 0, charger, > + &bat_debug_fops); > + return charger->debug_file; > +} > + > +static void da9030_bat_remove_debugfs(struct da9030_charger *charger) > +{ > + debugfs_remove(charger->debug_file); > +} I'd put an empty line here, or remove empty line at (1). Just for consistency. > +#else > +static inline struct dentry *da9030_bat_create_debugfs(struct da9030_charger *charger) > +{ > + return NULL; > +} > +static inline void da9030_bat_remove_debugfs(struct da9030_charger *charger) > +{ > +} > +#endif > + > +static inline void da9030_read_adc(struct da9030_charger *charger, > + struct da9030_adc_res *adc) > +{ > + da903x_reads(charger->master, DA9030_VBAT_RES, > + sizeof(*adc), (uint8_t *)adc); > +} > + > +static void da9030_charger_update_state(struct da9030_charger *charger) > +{ > + uint8_t val; > + > + da903x_read(charger->master, DA9030_CHARGE_CONTROL, &val); > + charger->is_on = (val & DA9030_CHRG_CHARGER_ENABLE) ? 1 : 0; > + charger->mA = ((val >> 3) & 0xf) * 100; > + charger->mV = (val & 0x7) * 50 + 4000; > + > + da9030_read_adc(charger, &charger->adc); > + da903x_read(charger->master, DA9030_FAULT_LOG, &charger->fault); > + charger->chdet = da903x_query_status(charger->master, > + DA9030_STATUS_CHDET); > +} > + > +static void da9030_set_charge(struct da9030_charger *charger, int on) > +{ > + uint8_t val = 0; = 0 isn't necessary here. > + > + if (on) { > + val = DA9030_CHRG_CHARGER_ENABLE; > + val |= (charger->charge_milliamp / 100) << 3; > + val |= (charger->charge_millivolt - 4000) / 50; > + charger->is_on = 1; > + } else { > + val = 0; > + charger->is_on = 0; > + } > + > + da903x_write(charger->master, DA9030_CHARGE_CONTROL, val); > +} > + > +static void da9030_charger_check_state(struct da9030_charger *charger) > +{ > + da9030_charger_update_state(charger); > + > + /* we wake or boot with external power on */ > + if (!charger->is_on) { > + if ((charger->chdet) && > + (charger->adc.vbat_res < > + charger->thresholds.vbat_charge_start)) { > + da9030_set_charge(charger, 1); > + } > + } else { > + if (charger->adc.vbat_res >= > + charger->thresholds.vbat_charge_stop) { > + da9030_set_charge(charger, 0); > + da903x_write(charger->master, DA9030_VBATMON, > + charger->thresholds.vbat_charge_restart); > + } else if (charger->adc.vbat_res > > + charger->thresholds.vbat_low) { > + /* we are charging and passed LOW_THRESH, > + so upate DA9030 VBAT threshold > + */ > + da903x_write(charger->master, DA9030_VBATMON, > + charger->thresholds.vbat_low); > + } > + if (charger->adc.vchmax_res > charger->thresholds.vcharge_max || > + charger->adc.vchmin_res < charger->thresholds.vcharge_min || > + /* Tempreture readings are negative */ > + charger->adc.tbat_res < charger->thresholds.tbat_high || > + charger->adc.tbat_res > charger->thresholds.tbat_low) { > + /* disable charger */ > + da9030_set_charge(charger, 0); > + } > + } > +} > + > +static void da9030_charging_monitor(struct work_struct *work) > +{ > + struct da9030_charger *charger; > + > + charger = container_of(work, struct da9030_charger, work.work); > + > + da9030_charger_check_state(charger); > + > + /* reschedule for the next time */ > + schedule_delayed_work(&charger->work, charger->interval); > +} > + > +static enum power_supply_property da9030_battery_props[] = { > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_HEALTH, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_CURRENT_AVG, > +}; > + > +static void da9030_battery_check_status(struct da9030_charger *charger, > + union power_supply_propval *val) > +{ > + if (charger->chdet) { > + if (charger->is_on) > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + } else { > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + } > +} > + > +static void da9030_battery_check_health(struct da9030_charger *charger, > + union power_supply_propval *val) > +{ > + if (charger->fault & DA9030_FAULT_LOG_OVER_TEMP) > + val->intval = POWER_SUPPLY_HEALTH_OVERHEAT; > + else if (charger->fault & DA9030_FAULT_LOG_VBAT_OVER) > + val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE; > + else > + val->intval = POWER_SUPPLY_HEALTH_GOOD; > +} > + > +static int da9030_battery_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct da9030_charger *charger; > + charger = container_of(psy, struct da9030_charger, psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + da9030_battery_check_status(charger, val); > + break; > + case POWER_SUPPLY_PROP_HEALTH: > + da9030_battery_check_health(charger, val); > + break; > + case POWER_SUPPLY_PROP_TECHNOLOGY: > + val->intval = charger->battery_info->technology; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + val->intval = charger->battery_info->voltage_max_design; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + val->intval = charger->battery_info->voltage_min_design; > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = da9030_reg_to_mV(charger->adc.vbat_res) * 1000; > + break; > + case POWER_SUPPLY_PROP_CURRENT_AVG: > + val->intval = > + da9030_reg_to_mA(charger->adc.ichaverage_res) * 1000; > + break; > + case POWER_SUPPLY_PROP_MODEL_NAME: > + val->strval = charger->battery_info->name; > + break; > + default: > + break; > + } > + > + return 0; > +} > + > +static void da9030_battery_vbat_event(struct da9030_charger *charger) > +{ > + da9030_read_adc(charger, &charger->adc); > + > + if (!charger->is_on) { I'd write it as: if (charger->is_on) return; the-rest; Saves one indent level. > + if (charger->adc.vbat_res < charger->thresholds.vbat_low) { > + /* set VBAT threshold for critical */ > + da903x_write(charger->master, DA9030_VBATMON, > + charger->thresholds.vbat_crit); > + if (charger->battery_low) > + charger->battery_low(); > + } else if (charger->adc.vbat_res < > + charger->thresholds.vbat_crit) { > + /* notify the system of battery critical */ > + if (charger->battery_critical) > + charger->battery_critical(); > + } > + } > +} > + > +static int da9030_battery_event(struct notifier_block *nb, unsigned long event, > + void *data) > +{ > + struct da9030_charger *charger = > + container_of(nb, struct da9030_charger, nb); > + int status; > + > + switch (event) { > + case DA9030_EVENT_CHDET: > + status = da903x_query_status(charger->master, > + DA9030_STATUS_CHDET); > + da9030_set_charge(charger, status); > + break; > + case DA9030_EVENT_VBATMON: > + da9030_battery_vbat_event(charger); > + break; > + case DA9030_EVENT_CHIOVER: > + case DA9030_EVENT_TBAT: > + da9030_set_charge(charger, 0); > + break; > + } > + > + return 0; > +} > + > +static void da9030_battery_convert_thresholds(struct da9030_charger *charger, > + struct da9030_battery_info *pdata) > +{ > + charger->thresholds.tbat_low = pdata->tbat_low; > + charger->thresholds.tbat_high = pdata->tbat_high; > + charger->thresholds.tbat_restart = pdata->tbat_restart; > + > + charger->thresholds.vbat_low = > + da9030_millivolt_to_reg(pdata->vbat_low); > + charger->thresholds.vbat_crit = > + da9030_millivolt_to_reg(pdata->vbat_crit); > + charger->thresholds.vbat_charge_start = > + da9030_millivolt_to_reg(pdata->vbat_charge_start); > + charger->thresholds.vbat_charge_stop = > + da9030_millivolt_to_reg(pdata->vbat_charge_stop); > + charger->thresholds.vbat_charge_restart = > + da9030_millivolt_to_reg(pdata->vbat_charge_restart); > + > + charger->thresholds.vcharge_min = > + da9030_millivolt_to_reg(pdata->vcharge_min); > + charger->thresholds.vcharge_max = > + da9030_millivolt_to_reg(pdata->vcharge_max); > +} > + > +static void da9030_battery_setup_psy(struct da9030_charger *charger) > +{ > + struct power_supply *psy = &charger->psy; > + struct power_supply_info *info = charger->battery_info; > + > + psy->name = info->name; > + psy->use_for_apm = info->use_for_apm; > + psy->type = POWER_SUPPLY_TYPE_BATTERY; > + psy->get_property = da9030_battery_get_property; > + > + psy->properties = da9030_battery_props; > + psy->num_properties = ARRAY_SIZE(da9030_battery_props);; One semicolon is enough. > +}; > + > +static int da9030_battery_charger_init(struct da9030_charger *charger) > +{ > + char v[5]; > + int ret; > + > + v[0] = v[1] = charger->thresholds.vbat_low; > + v[2] = charger->thresholds.tbat_high; > + v[3] = charger->thresholds.tbat_restart; > + v[4] = charger->thresholds.tbat_low; > + > + ret = da903x_writes(charger->master, DA9030_VBATMON, 5, v); > + if (ret) > + return ret; > + > + /* enable reference voltage supply for ADC from the LDO_INTERNAL > + regulator. Must be set before ADC measurements can be made. */ I would suggest using canonical-style comments: /* * Not a big deal though, I'm * just nitpicking. ;-) */ Thanks, -- Anton Vorontsov email: cbouatmailru@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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/