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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 20 Apr 2011 16:37:22 +0100
From:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
To:	Ashish Jangam <Ashish.Jangam@...tcummins.com>
Cc:	"cbou@...l.ru" <cbou@...l.ru>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCHv1 4/11] Power: Battery module of DA9052 PMIC driver

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.
--
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/