From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julia Lawall Subject: Re: [PATCH v3 0/5] bq27xxx_battery data memory update Date: Thu, 31 Aug 2017 13:33:25 +0200 (CEST) Message-ID: References: <20170824033617.20840-1-liam@networkimprov.net> <20170829105413.6wbejdaxxxd6hk7b@earth> <0bf4ba2f-18f9-1204-8241-8acb6ac6f490@ti.com> <20170829212259.gs4bljwscrprsfjl@earth> <20170830002939.ns43wldvffmwqhuv@earth> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Return-path: Received: from mail3-relais-sop.national.inria.fr ([192.134.164.104]:63941 "EHLO mail3-relais-sop.national.inria.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbdHaLdx (ORCPT ); Thu, 31 Aug 2017 07:33:53 -0400 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck Cc: Sebastian Reichel , Gilles Muller , Nicolas Palix , Michal Marek , cocci@systeme.lip6.fr, "Andrew F. Davis" , =?ISO-8859-15?Q?Pali_Roh=E1r?= , Linux PM mailing list On Thu, 31 Aug 2017, Liam Breck wrote: > On Thu, Aug 31, 2017 at 2:54 AM, Julia Lawall wrote: > > > > > > On Tue, 29 Aug 2017, Liam Breck wrote: > > > >> Coccinelle folks, > >> > >> On Tue, Aug 29, 2017 at 5:29 PM, Sebastian Reichel > >> wrote: > >> > Hi, > >> > > >> > On Tue, Aug 29, 2017 at 04:07:12PM -0700, Liam Breck wrote: > >> >> I don't see a Julia in CC list... > >> > > >> > <_< let's try that again. > >> > > >> >> On Tue, Aug 29, 2017 at 2:22 PM, Sebastian Reichel > >> >> wrote: > >> >> > [adding Julia to Cc for Coccinelle question] > >> >> > > >> >> > Hi, > >> >> > > >> >> > On Tue, Aug 29, 2017 at 10:31:57AM -0500, Andrew F. Davis wrote: > >> >> >> On 08/29/2017 05:54 AM, Sebastian Reichel wrote: > >> >> >> > On Wed, Aug 23, 2017 at 08:36:12PM -0700, Liam Breck wrote: > >> >> >> >> Overview: > >> >> >> >> * Reorganizes chip data definitions > >> >> >> >> * Enables features landed in these patches: > >> >> >> >> dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation > >> >> >> >> power: supply: bq27xxx: Add chip data memory read/write support > >> >> >> >> power: supply: bq27xxx: Add power_supply_battery_info support > >> >> >> >> * Supports the following chips (only BQ27425 is active) > >> >> >> >> BQ27500, 545, 425, 421, 441, 621 > >> >> >> >> > >> >> >> >> Changes in v3: > >> >> >> >> * BQ27425 tested; workaround minor chip bug > >> >> >> >> * Dropped driver_version > >> >> >> >> * Fixed dbg_dupes logic for .props & .dm_regs > >> >> >> >> * Dropped two props array dupes > >> >> >> >> > >> >> >> >> Changes in v2: > >> >> >> >> * Added di->opts flags for remaining chip features > >> >> >> >> * Commented out untested bq27xxx_dm_regs parameters > >> >> >> >> * Changed dbg_dupes to run only once > >> >> >> >> > >> >> >> >> Notes on v1: > >> >> >> >> * Not fully tested (hence RFC tag) > >> >> >> > > >> >> >> > Thanks, full series queued. > >> >> >> > > >> >> >> > -- Sebastian > >> >> >> > >> >> >> Anyway, I've not got the time to fight these changes anymore, but at > >> >> >> very least could you drop 4/5, it's static analysis code made into a > >> >> >> runtime check built into a kernel driver, if not at least add my > >> >> >> nacked-by. :) > >> >> > > >> >> > Since it's not critical at all and nobody depends on it, I dropped 4/5 > >> >> > for now. I agree, that checking it at runtime is not nice. On the other > >> >> > hand I do think a duplication check makes sense. Doing a static > >> >> > check should be possible, but I have no idea how to implement this > >> >> > (without much effort). I suspect Coccinelle can do it, so I added > >> >> > Julia. > >> >> > > >> >> > For reference this is the runtime check: > >> >> > https://patchwork.kernel.org/patch/9918953/ > >> > >> The data structures being checked start here: > >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n138 > >> > >> And are aggregated here: > >> https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git/tree/drivers/power/supply/bq27xxx_battery.c?h=for-next#n743 > > > > I worked a bit on this, but unfortunately there is a major parsing > > problem, and most of the definitions are ignored. Specifically, the > > definitions of the regs variables are interspersed with eg: > > > > #define bq27510g1_regs bq27500_regs > > You can transform the macros with sed to either of: > > static u8 *bq27510g1_regs = 0 // skip comparison if x == 0 > > static u8 bq27510g1_regs[BQ27XXX_REG_MAX] = { 0xFF } // skip > comparison if x[0] == 0xff > > Does that help? Not quite, because it's really a list of variable declarations, like int a, b, c, d; The following could be fine: *bq27510g1_regs = 0, julia > > > I'm not sure whether this would be convenient to fix, since parsing is > > rather delicate. Coccinelle doesn't run cpp first. It tries to parse > > macros definitions so that their code can be transformed as needed t the > > place of the definition. >