Re: [RFC v1 5/6] power: supply: bq27xxx: Flag identical chip data when in debug mode

[Sebastian Reichel <sebastian.reichel@collabora.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]

Hi,

On Sat, Jul 08, 2017 at 07:16:59PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> The driver has 13 unique register maps, several of which are shared
> by multiple chips. When adding support for a new chip, it's easy to
> add a duplicate map by mistake.

Which is not that bad. Sure, some bytes wasted, but your code also
wastes some bytes. This is something, that should normally tested
@ compile time using static checkers. Independently the patch has
some issues:

> In debug mode we now scan bq27xxx_chip_data[n].regs/props/dm_regs for
> duplicates.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 36 +++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 5d3893a9..54755c88 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -883,7 +883,7 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
>  	.props = bq27##ref##_props,		\
>  	.props_size = ARRAY_SIZE(bq27##ref##_props) }
>  
> -static struct {
> +static struct bq27xxx_chip_datum {
>  	u32 opts;
>  	int acts_like; //todo drop this when opts fully implemented
>  	u32 unseal_key;
> @@ -918,6 +918,38 @@ static struct {
>  	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
>  };
>  
> +static void __maybe_unused bq27xxx_battery_dbg_dupes(struct bq27xxx_device_info *di)

If there are multiple bq27xxx batteries installed in the system the
code is executed multiple times, so let's modify this a bit:

> +{
> +	const size_t max = ARRAY_SIZE(bq27xxx_chip_data);
> +	const char * const msg = "bq27xxx_chip_data[%d].%s & [%d].%s are identical\n";
> +	struct bq27xxx_chip_datum *a, *b;
> +	int i, j;

static checked = false;

if (checked)
    return;

> +	for (i = 1; i < max-1; i++) {
> +		a = bq27xxx_chip_data + i;
> +
> +		for (j = i+1; j < max; j++) {
> +			b = bq27xxx_chip_data + j;
> +
> +			if (a->regs != b->regs &&
> +			    !memcmp(a->regs, b->regs, sizeof(bq27000_regs)))
> +				dev_warn(di->dev, msg, i, "regs", j, "regs");
> +
> +			if (a->props != b->props &&
> +			    a->props_size == b->props_size &&
> +			    !memcmp(a->props, b->props, a->props_size))
> +				dev_warn(di->dev, msg, i, "props", j, "props");
> +
> +			if (a->dm_regs != b->dm_regs &&
> +			    !memcmp(a->dm_regs, b->dm_regs, sizeof(bq27500_dm_regs)))
> +				dev_warn(di->dev, msg, i, "dm_regs", j, "dm_regs");
> +		}
> +	}

checked = true;

> +}
> +#ifndef DEBUG
> +#define bq27xxx_battery_dbg_dupes(di)
> +#endif
> +
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>  
> @@ -1989,6 +2021,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  		.drv_data = di,
>  	};
>  
> +	bq27xxx_battery_dbg_dupes(di);

Add #ifdef DEBUG here and drop the above.

>  	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>  	mutex_init(&di->lock);
>  
> -- 
> 2.13.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]