add battery driver in st1.5

add battery driver in st1.5 

BUG=NONE 
TEST=Built and ran on st1.5 
Signed-off-by: Horace Fu <horace.fu@quantatw.com>;

[pelcan, 2010-04-14 20:29:13]

http://codereview.chromium.org/1637005/diff/1/2
File drivers/power/Kconfig (right):

http://codereview.chromium.org/1637005/diff/1/2#newcode111
drivers/power/Kconfig:111: Say Y here if you want to use the Quanta battery
driver for ST15
If this is specific for ST15, should you make it depend on ARCH_MSM_SCORPION?

http://codereview.chromium.org/1637005/diff/1/4
File drivers/power/qci_battery.c (right):

http://codereview.chromium.org/1637005/diff/1/4#newcode33
drivers/power/qci_battery.c:33: struct qci_bat_info {
Can this be part of i2cbat_drv_data and be allocated in the init function?

http://codereview.chromium.org/1637005/diff/1/4#newcode64
drivers/power/qci_battery.c:64: int ret = 0;
Align

http://codereview.chromium.org/1637005/diff/1/4#newcode69
drivers/power/qci_battery.c:69: else
Align

http://codereview.chromium.org/1637005/diff/1/4#newcode71
drivers/power/qci_battery.c:71: break;
Align

http://codereview.chromium.org/1637005/diff/1/4#newcode98
drivers/power/qci_battery.c:98: static int qbat_get_status(union
power_supply_propval *val, uint8_t ec_byte)
what is the purpose of ec_byte?

http://codereview.chromium.org/1637005/diff/1/4#newcode131
drivers/power/qci_battery.c:131: uint16_t ec_word)
ec_word should be a local variable since it's not passed back to the caller. 
And similarly in functions below

http://codereview.chromium.org/1637005/diff/1/4#newcode277
drivers/power/qci_battery.c:277: return ;
return not needed

http://codereview.chromium.org/1637005/diff/1/4#newcode292
drivers/power/qci_battery.c:292: context->qcibat_gpio = BATTERY_INT_GPIO;
the gpio number should come from the board file via the platform data

http://codereview.chromium.org/1637005/diff/1/4#newcode350
drivers/power/qci_battery.c:350: platform_device_unregister(bat_pdev);
free context, free gpio, set clientdata to NULL.

http://codereview.chromium.org/1637005/diff/1/5
File drivers/power/qci_battery.h (right):

http://codereview.chromium.org/1637005/diff/1/5#newcode62
drivers/power/qci_battery.h:62: extern struct i2c_client
*wpce_get_i2c_client(void);
This should be defined in the wpce driver.

[Horace, 2010-04-15 07:37:26]

On 2010/04/14 20:29:13, pelcan wrote:

Dear All,
Please review it again, thanks.

[pelcan, 2010-04-15 19:53:17]

lgtm

[nleeder, 2010-04-23 19:43:34]

http://codereview.chromium.org/1637005/diff/6001/7003
File drivers/power/qci_battery.c (right):

http://codereview.chromium.org/1637005/diff/6001/7003#newcode203
drivers/power/qci_battery.c:203: return ret;
Not sure what this is doing: ret is initialized to 0 and nothing changes it so
this code seems redundant.

http://codereview.chromium.org/1637005/diff/6001/7003#newcode216
drivers/power/qci_battery.c:216: return ret;
same comment as above

http://codereview.chromium.org/1637005/diff/6001/7003#newcode220
drivers/power/qci_battery.c:220: return ret;
same comment as above

http://codereview.chromium.org/1637005/diff/6001/7003#newcode287
drivers/power/qci_battery.c:287: return err;
should probably return -ENOMEM

http://codereview.chromium.org/1637005/diff/6001/7003#newcode288
drivers/power/qci_battery.c:288: context->bi2c_client = wpce_get_i2c_client();
Should check that this is non-NULL

[nleeder, 2010-04-29 18:59:39]

+msb

Please can we get a review for this - thanks.

[Mandeep Singh Baines, 2010-05-03 22:53:20]

http://codereview.chromium.org/1637005/diff/13001/14003
File drivers/power/qci_battery.c (right):

http://codereview.chromium.org/1637005/diff/13001/14003#newcode117
drivers/power/qci_battery.c:117: if ((context->bif.mbat_status &
MAIN_BATTERY_STATUS_BAT_IN))
extra parens

http://codereview.chromium.org/1637005/diff/13001/14003#newcode126
drivers/power/qci_battery.c:126: if (!(context->bif.mbat_status &
MAIN_BATTERY_STATUS_BAT_IN))
avoid inverse logic if possible

http://codereview.chromium.org/1637005/diff/13001/14003#newcode142
drivers/power/qci_battery.c:142: if (!(context->bif.mbat_status &
MAIN_BATTERY_STATUS_BAT_IN))
avoid inverse logic if possible

http://codereview.chromium.org/1637005/diff/13001/14003#newcode151
drivers/power/qci_battery.c:151: if (!(context->bif.mbat_status &
MAIN_BATTERY_STATUS_BAT_IN))
avoid inverse logic if possible

http://codereview.chromium.org/1637005/diff/13001/14003#newcode167
drivers/power/qci_battery.c:167: if (!(context->bif.mbat_status &
MAIN_BATTERY_STATUS_BAT_IN))
avoid inverse logic if possible

http://codereview.chromium.org/1637005/diff/13001/14003#newcode236
drivers/power/qci_battery.c:236: static struct power_supply qci_bat = {
.name = "battery",
.type = POWER_SUPPLY_TYPE_BATTERY,

http://codereview.chromium.org/1637005/diff/13001/14003#newcode279
drivers/power/qci_battery.c:279: context = kzalloc(sizeof(struct
i2cbat_drv_data), GFP_KERNEL);
Why dynamically allocate this? There is only one instance. You could use a
single static.

http://codereview.chromium.org/1637005/diff/13001/14003#newcode352
drivers/power/qci_battery.c:352: late_initcall(qbat_init);
Why not module_init?

[Mandeep Singh Baines, 2010-05-03 23:06:44]

http://codereview.chromium.org/1637005/diff/13001/14003
File drivers/power/qci_battery.c (right):

http://codereview.chromium.org/1637005/diff/13001/14003#newcode309
drivers/power/qci_battery.c:309: pr_err("[BAT] err gpio request\n");
dev_err

http://codereview.chromium.org/1637005/diff/13001/14003#newcode316
drivers/power/qci_battery.c:316: pr_err("[BAT] unable to get IRQ\n");
dev_err

[Horace, 2010-05-04 08:01:28]

On 2010/05/03 23:06:44, msb wrote:

Please review again, thanks.

[Mandeep Singh Baines, 2010-05-11 16:59:07]

http://codereview.chromium.org/1637005/diff/20001/21003
File drivers/power/qci_battery.c (right):

http://codereview.chromium.org/1637005/diff/20001/21003#newcode282
drivers/power/qci_battery.c:282: context.bi2c_client = wpce_get_i2c_client();
It would be better if wpce775x exported a high-level API instead of this driver
using its i2_client.

http://codereview.chromium.org/1637005/diff/20001/21003#newcode350
drivers/power/qci_battery.c:350: late_initcall(qbat_init);
Not sure this is a good idea. Another way to guarantee initcall order would be
to write this driver as a platform driver using the device model and have it
register with the main EC driver. See wpce775x review for details.