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

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Sun, Aug 06, 2017 at 11:22:15PM -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.
> 
> In debug mode we now scan bq27xxx_chip_data[n].regs/props/dm_regs for
> duplicates.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---

This should be done by static code analysis using coccinelle or
something similar, so that it's catched by build bots and not by
users. Even worse the loop is called for each bq27xxx battery in the
system.

-- Sebastian

>  drivers/power/supply/bq27xxx_battery.c | 40 +++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 8e535890..d85f9ec2 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -885,7 +885,7 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
>  	.props = ref##_props,			\
>  	.props_size = ARRAY_SIZE(ref##_props) }
>  
> -static struct {
> +static struct bq27xxx_chip_datum {
>  	u32 opts;
>  	u32 unseal_key;
>  	u8 *regs;
> @@ -919,6 +919,40 @@ static struct {
>  	[BQ27621]   = BQ27XXX_DATA(bq27621,   0x80008000, BQ27XXX_O_UTOT | BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
>  };
>  
> +static void __maybe_unused bq27xxx_battery_dbg_dupes(struct bq27xxx_device_info *di)
> +{
> +	static bool once = false;
> +	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;
> +
> +	if (once)
> +		return;
> +	once = true;
> +
> +	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");
> +		}
> +	}
> +}
> +
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>  
> @@ -1976,6 +2010,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  		.drv_data = di,
>  	};
>  
> +#ifdef DEBUG
> +	bq27xxx_battery_dbg_dupes(di);
> +#endif
> +
>  	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>  	mutex_init(&di->lock);
>  
> -- 
> 2.13.2
> 

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