Re: [PATCH v2] add MAX17040 Fuel Gauge driver

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

On Thu, 4 Jun 2009 14:46:45 +0530, Trilok Soni wrote:
> Hi Minkyu Kang,
> 
> Adding linux-i2c mailing list, so not deleting any code.
> 
> On Thu, Jun 4, 2009 at 2:25 PM, Minkyu Kang <mk7.kang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> > The MAX17040 is a I2C interfaced Fuel Gauge systems for lithium-ion batteries
> > This patch adds support the MAX17040 Fuel Gauge
> >
> > Cc: Anton Vorontsov <cbou-JGs/UdohzUI@public.gmane.org>
> > Signed-off-by: Minkyu Kang <mk7.kang-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/power/Kconfig            |    8 +
> >  drivers/power/Makefile           |    3 +-
> >  drivers/power/max17040_battery.c |  306 ++++++++++++++++++++++++++++++++++++++
> >  include/linux/max17040_battery.h |   19 +++
> >  4 files changed, 335 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/power/max17040_battery.c
> >  create mode 100644 include/linux/max17040_battery.h
> > (...)
> > +static u8 max17040_read_reg(int reg)
> > +{
> > +       int ret;
> > +
> > +       ret = i2c_smbus_read_byte_data(max17040->client, reg);
> > +
> > +       if (ret < 0) {
> > +               dev_err(&max17040->client->dev,
> > +                               "%s: reg 0x%x, err %d\n",
> > +                               __func__, reg, ret);
> > +       }
> > +
> > +       return ret;
> > +}

In case of error, this will wrap the error code into a u8 and the
caller won't notice. So you'll return a random value, depending on the
actual error which happened. No good.

You should either return an int there, and have the caller check for
errors, or if you don't want to care about errors, return an arbitrary
value on error (e.g. 0.)

> > (...)
> > +static int __devinit max17040_probe(struct i2c_client *client,
> > +                       const struct i2c_device_id *id)
> > +{
> > +       struct max17040_chip *chip;
> > +       int ret;
> > +
> > +       chip = kzalloc(sizeof(struct max17040_chip), GFP_KERNEL);
> > +       if (!chip)
> > +               return -ENOMEM;
> > +
> > +       chip->client = client;
> > +       pdata = client->dev.platform_data;
> > +
> > +       i2c_set_clientdata(client, chip);
> 
> Please add i2c_check_functionality check before doing any smbus
> read/write operations.
> 
> > +       max17040 = chip;
> 
> This means that we support only one instance of this chip, right?
> 
> > +
> > +       max17040_reset();
> > +
> > +       max17040_get_version();
> > +
> > +       INIT_DELAYED_WORK_DEFERRABLE(&chip->work, max17040_work);
> > +       schedule_delayed_work(&chip->work, MAX17040_DELAY);
> > +
> > +       bat_ps.properties = max17040_battery_props;
> > +       bat_ps.num_properties = ARRAY_SIZE(max17040_battery_props);
> > +
> > +       ret = power_supply_register(&client->dev, &bat_ps);
> > +       if (ret) {
> > +               dev_err(&max17040->client->dev,
> > +                               "failed: power supply register\n");
> > +               cancel_delayed_work(&chip->work);
> > +               i2c_set_clientdata(client, NULL);
> > +               kfree(chip);
> > +               max17040 = NULL;
> > +               return -1;

Please come up with a better error code.

> > +       }
> > +
> > +       return 0;
> > +}

-- 
Jean Delvare