| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20121030223922.GA29061@lizard>
Date: Tue, 30 Oct 2012 15:39:23 -0700
From: Anton Vorontsov <cbouatmailru@...il.com>
To: Pali Rohár <pali.rohar@...il.com>
Cc: David Woodhouse <dwmw2@...radead.org>,
linux-kernel@...r.kernel.org,
Joerg Reisenweber <joerg@...nmoko.org>,
Ivaylo Dimitrov <freemangordon@....bg>
Subject: Re: [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@...il.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@...il.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@...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