Re: [PATCH 2/4] power_supply: bq27200: separate bq27200-specific driver

[Felipe Balbi <me@felipebalbi.com>]

On Sun, Oct 19, 2008 at 01:46:35AM +0400, Anton Vorontsov wrote:
> > diff --git a/drivers/power/bq27200.c b/drivers/power/bq27200.c
> > new file mode 100644
> > index 0000000..ef03743
> > --- /dev/null
> > +++ b/drivers/power/bq27200.c
> > @@ -0,0 +1,202 @@
> > +/*
> > + * bq27200.c - BQ27200 battery driver
> > + *
> > + * Copyright (C) 2008 Texas Instruments, Inc.
> > + * Copyright (C) 2008 Nokia Corporation
> > + *
> > + * Author: Texas Instruments
> > + *
> > + * This package 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.
> > + *
> > + * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> > + * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> > + * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> > + *
> > + */
> > +#include <linux/module.h>
> > +#include <linux/param.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/power_supply.h>
> > +#include <linux/i2c.h>
> > +
> > +#include "bq27x00.h"
> > +
> > +static struct i2c_client *bq_client;
> 
> :-(
> 
> Platform device approach would eliminate need for this...

sure... that looks nice.

> > +static inline int bq27200_read(u8 reg, int *rt_value, int b_single,
> > +	struct bq27x00_device_info *di)
> > +{
> > +	struct i2c_client *client = bq_client;
> > +	struct i2c_msg msg[1];
> > +	unsigned char data[2];
> > +	int err;
> > +
> > +	if (!client->adapter)
> > +		return -ENODEV;
> > +
> > +	msg->addr = client->addr;
> > +	msg->flags = 0;
> > +	msg->len = 1;
> > +	msg->buf = data;
> > +
> > +	data[0] = reg;
> > +	err = i2c_transfer(client->adapter, msg, 1);
> > +
> > +	if (err >= 0) {
> 
> Would look better if you swap the success/fail cases:
> 
> if (err < 0) {
> 	dev_err();
> 	return err;
> }
> 
> success-code-here;

yeah, brain fart.

> > +static int __init bq27200_probe(struct i2c_client *client,
> > +		const struct i2c_device_id *id)
> > +{
> > +	struct bq27x00_device_info *di;
> > +	struct bq27x00_access_methods *bus;
> > +	int retval = 0;
> > +
> > +	di = kzalloc(sizeof(*di), GFP_KERNEL);
> > +	if (!di) {
> > +		dev_dbg(&client->dev, "could not allocate dev info's memory\n");
> > +		retval = -ENOMEM;
> > +		goto err_di;
> > +	}
> > +
> > +	bus = kzalloc(sizeof(*bus), GFP_KERNEL);
> > +	if (!bus) {
> > +		dev_dbg(&client->dev, "could not allocate bus' memory\n");
> > +		retval = -ENOMEM;
> > +		goto err_bus;
> > +	}
> > +
> > +	i2c_set_clientdata(client, di);
> > +	di->dev = &client->dev;
> > +	di->bat.name = "bq27200";
> > +	bus->read = &bq27200_read;
> > +	di->bus = bus;
> > +	bq_client = client;
> > +
> > +	bq27x00_powersupply_init(di);
> > +
> > +	retval = power_supply_register(&client->dev, &di->bat);
> > +	if (retval) {
> > +		dev_dbg(&client->dev, "could not register power_supply, %d\n",
> > +				retval);
> > +		goto err_psy;
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&di->monitor_work, bq27200_work);
> > +	schedule_delayed_work(&di->monitor_work, 100);
> 
> This should be done before registering the power supply. Otherwise
> code may use the di->monitor_work before it has initialized.

that's right, good catch.

> > +static const struct i2c_device_id bq27200_id[] = {
> > +	{ "bq27200", 0 },
> 
> No need for ", 0".

doesn't the static variables automatic initialization to 0 (or NULL)
only work with the gnu style struct declaration ??

I mean:

static const struct i2c_device_id b27200_id[] = {
	{
		.name	= "bq27200",
	},
};

Anyways, I'm only following the standard used by Jean (i2c maintainer).

-- 
balbi