Re: [PATCHv1 4/11] Power: Battery module of DA9052 PMIC driver

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Apr 06, 2011 at 06:28:03PM +0530, Ashish Jangam wrote:

> +static u16 filter_sample(u16 *buffer)
> +{
> +       u8 count;
> +       u16 tempvalue = 0;
> +       u16 ret;
> +
> +       if (buffer == NULL)
> +               return -EINVAL;
> +
> +       for (count = 0; count < DA9052_FILTER_SIZE; count++)
> +               tempvalue = tempvalue + *(buffer + count);
> +
> +       ret = tempvalue/DA9052_FILTER_SIZE;
> +       return ret;

It's probably as well to pass the size of buffer in as an argument so
that it's less surprising that you don't handle a partially filled
buffer.

You probably want to call this _average() or something, it's not
actually doing any filtering, it's just taking an average.  This is
perfectly sensible but it's a bit confusing.

> +static irqreturn_t da9052_tbat_irq(int irq, void *data)
> +{
> +       struct da9052_charger_device *chg_device =
> +                                       (struct da9052_charger_device *)data;
> +
> +       chg_device->health = POWER_SUPPLY_HEALTH_OVERHEAT;
> +
> +       return IRQ_HANDLED;
> +}

Shouldn't you also be telling the core about the status change so it can
go notify userspace?

> +/* STATIC CONFIGURATION */
> +#define DA9052_LOOK_UP_TABLE_SIZE              68
> +#define DA9052_NO_OF_LOOKUP_TABLE              3
> +#define DA9052_FILTER_SIZE                     4
> +#define DA9052_NUMBER_OF_STORE_CURENT_READING  4

> +static u32 const vbat_vs_capacity_look_up[DA9052_NO_OF_LOOKUP_TABLE]
> +                                       [DA9052_LOOK_UP_TABLE_SIZE][2] = {

You probably want to be using ARRAY_SIZE() for some of this.