lkml.org 
[lkml]   [2012]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] New Nokia RX-51 power supply battery driver
On Tue, Oct 30, 2012 at 09:04:55PM +0100, Pali Rohár wrote:
> I'm working on full BME replacement for Nokia N900. BME is Nokia
> proprietary buggy userspace software which charging battery.
[...]
> Except Nokia RX-51 Schematics there is no available documentation
> about battery, charging and madc channels (for n900), so all
> information was RE.
[...]
> Thanks to Joerg Reisenweber and Ivaylo Dimitrov for collecting needed
> data, help with some problems and fixing errors!

Pali and guys, a huge thank you goes out to you, awesome work!

The driver looks great, just a few cosmetic issues.

(Please include a Signed-off-by tag on the next resend.)

> --- /dev/null
> +++ kernel-power/drivers/power/rx51_battery.c
> @@ -0,0 +1,257 @@
> +/*
> + rx51_battery.c - Nokia RX-51 battery driver
> + Copyright (C) 2012 Pali Rohár <pali.rohar@gmail.com>
> +
> + 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.,
> + 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +*/

The canonical multi-line comments are like this:

/*
* 1
* 2
*/

> +
> +#include <linux/module.h>
> +#include <linux/param.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/i2c/twl4030-madc.h>
> +
> +struct rx51_device_info {
> + struct device *dev;
> + struct power_supply bat;
> +};
> +
> +/*
> + Read ADCIN channel value, code copied from maemo kernel
> + */

Ditto, style.

> +static int rx51_battery_read_adc(int channel)
> +{
> + struct twl4030_madc_request req;
> +
> + req.channels = (1 << channel);

Parenthesis are not necessary here.

> + req.do_avg = 1;
> + req.method = TWL4030_MADC_SW1;
> + req.func_cb = NULL;
> + req.type = TWL4030_MADC_WAIT;
> +
> + if (twl4030_madc_conversion(&req) > 0)
> + return req.rbuf[channel];
> + else

No need for 'else'.

Plus, to make it look more like a conventional error-handling, I'd
write it as

if (twl4030_madc_conversion(&req) <= 0)
return -ENODATA;

return req.rbuf[channel];

> + return -ENODATA;
> +}
> +
> +/*
> + Read ADCIN channel 12 (voltage) and convert RAW value to micro voltage
> + This conversion formula was extracted from maemo program bsi-read
> + */

Style.

> +static int rx51_battery_read_voltage(struct rx51_device_info *di)
> +{
> + int voltage = rx51_battery_read_adc(12);

Need an empty line here.

> + if (voltage < 0)
> + return voltage;
> + return 1000 * (10000 * voltage / 1705);
> +}
> +
> +/*
> + Temperature look-up tables
> + TEMP = (1/(t1 + 1/298) - 273.15)
> + Where t1 = (1/B) * ln((RAW_ADC_U * 2.5)/(R * I * 255))
> + Formula is based on experimental data, RX-51 CAL data, maemo program bme
> + and formula from da9052 driver with values R = 100, B = 3380, I = 0.00671
> + */
> +
> +/*
> + Table1 (temperature for first 25 RAW values)
> + Usage: TEMP = rx51_temp_table1[RAW]
> + RAW is between 1 and 24
> + TEMP is between 201 C and 55 C
> + */
> +static u8 rx51_temp_table1[] = {
> + 255, 201, 159, 138, 124, 114, 106, 99, 94, 89, 85, 82, 78, 75,
> + 73, 70, 68, 66, 64, 62, 61, 59, 57, 56, 55
> +};
> +
> +/*
> + Table2 (lowest RAW value for temperature)
> + Usage: RAW = rx51_temp_table2[TEMP-rx51_temp_table2_first]
> + TEMP is between 53 C and -32 C
> + RAW is between 25 and 993
> + */
> +#define rx51_temp_table2_first 53
> +static u16 rx51_temp_table2[] = {
> + 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 39,
> + 40, 41, 43, 44, 46, 48, 49, 51, 53, 55, 57, 59, 61, 64,
> + 66, 69, 71, 74, 77, 80, 83, 86, 90, 94, 97, 101, 106, 110,
> + 115, 119, 125, 130, 136, 141, 148, 154, 161, 168, 176, 184, 202, 211,
> + 221, 231, 242, 254, 266, 279, 293, 308, 323, 340, 357, 375, 395, 415,
> + 437, 460, 485, 511, 539, 568, 600, 633, 669, 706, 747, 790, 836, 885,
> + 937, 993, 1024
> +};
> +
> +/*
> + Read ADCIN channel 0 (battery temp) and convert value to tenths of Celsius
> + Use Temperature look-up tables for conversation
> + */
> +static int rx51_battery_read_temperature(struct rx51_device_info *di)
> +{
> + int min = 0;
> + int max = ARRAY_SIZE(rx51_temp_table2)-1;

Need spaces around '-'.

> + int raw = rx51_battery_read_adc(0);
> +
> + /* Zero and negative values are undefined */
> + if (raw <= 0)
> + return INT_MAX;
> +
> + /* ADC channels are 10 bit, higher value are undefined */
> + if (raw >= (1 << 10))
> + return INT_MIN;
> +
> + /* First check for temperature in first direct table */
> + if (raw < ARRAY_SIZE(rx51_temp_table1))
> + return rx51_temp_table1[raw] * 100;
> +
> + /* Binary search RAW value in second inverse table */
> + while (max-min > 1) {

Spaces around '-'.

> + int mid = (max+min)/2;

Need an empty line here, spaces around '+' and '/'.

> + if (rx51_temp_table2[mid] <= raw)
> + min = mid;
> + else if (rx51_temp_table2[mid] > raw)
> + max = mid;
> + if (rx51_temp_table2[mid] == raw)
> + break;
> + }
> +
> + return (rx51_temp_table2_first - min) * 100;
> +}
> +
> +/*
> + Read ADCIN channel 4 (BSI) and convert RAW value to micro Ah
> + This conversion formula was extracted from maemo program bsi-read
> + */
> +static int rx51_battery_read_capacity(struct rx51_device_info *di)
> +{
> + int capacity = rx51_battery_read_adc(4);

Empty line here.

> + if (capacity < 0)
> + return capacity;
> + return 1280 * (1200 * capacity)/(1024 - capacity);
> +}
> +
> +/*
> + Return power_supply property
> + */
> +static int rx51_battery_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *val)
> +{
> + struct rx51_device_info *di = container_of((psy),
> + struct rx51_device_info, bat);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_TECHNOLOGY:
> + val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
> + val->intval = 4200000;
> + break;
> + case POWER_SUPPLY_PROP_PRESENT:
> + val->intval = rx51_battery_read_voltage(di) ? 1 : 0;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = rx51_battery_read_voltage(di);
> + break;
> + case POWER_SUPPLY_PROP_TEMP:
> + val->intval = rx51_battery_read_temperature(di);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> + val->intval = rx51_battery_read_capacity(di);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (val->intval == INT_MAX || val->intval == INT_MIN)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static enum power_supply_property rx51_battery_props[] = {
> + POWER_SUPPLY_PROP_TECHNOLOGY,
> + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_VOLTAGE_NOW,
> + POWER_SUPPLY_PROP_TEMP,
> + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> +};
> +
> +static int __devinit rx51_battery_probe(struct platform_device *pdev)
> +{
> + struct rx51_device_info *di;
> + int ret;
> +
> + di = kzalloc(sizeof(*di), GFP_KERNEL);
> + if (!di)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, di);
> +
> + di->bat.name = dev_name(&pdev->dev);
> + di->bat.type = POWER_SUPPLY_TYPE_BATTERY;
> + di->bat.properties = rx51_battery_props;
> + di->bat.num_properties = ARRAY_SIZE(rx51_battery_props);
> + di->bat.get_property = rx51_battery_get_property;
> +
> + ret = power_supply_register(di->dev, &di->bat);
> + if (ret) {
> + platform_set_drvdata(pdev, NULL);
> + kfree(di);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int __devexit rx51_battery_remove(struct platform_device *pdev)
> +{
> + struct rx51_device_info *di = platform_get_drvdata(pdev);
> +
> + power_supply_unregister(&di->bat);
> + platform_set_drvdata(pdev, NULL);
> + kfree(di);
> +
> + return 0;
> +}
> +
> +static struct platform_driver rx51_battery_driver = {
> + .probe = rx51_battery_probe,
> + .remove = __devexit_p(rx51_battery_remove),
> + .driver = {
> + .name = "rx51-battery",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init rx51_battery_init(void)
> +{
> + return platform_driver_register(&rx51_battery_driver);
> +}
> +module_init(rx51_battery_init);
> +
> +static void __exit rx51_battery_exit(void)
> +{
> + platform_driver_unregister(&rx51_battery_driver);
> +}
> +module_exit(rx51_battery_exit);

I guess these two calls can be replaced by module_platform_driver() macro.

> +MODULE_ALIAS("platform:rx51-battery");
> +MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
> +MODULE_DESCRIPTION("Nokia RX-51 battery driver");
> +MODULE_LICENSE("GPL");

Otherwise, it looks very good!

Thanks,
Anton.
--
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/

\
 
 \ /
  Last update: 2012-10-31 00:21    [W:0.152 / U:0.928 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site