Re: [PATCH V1 1/2] mfd: tps65910: use regmap for device register access.

[Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>]

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

On Wed, Feb 08, 2012 at 04:16:24PM +0530, Laxman Dewangan wrote:

> +static bool is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	struct tps65910 *tps65910 = dev_get_drvdata(dev);
> +	return test_bit(reg, tps65910->cache_reg) ? false : true;
> +}

This is *really* odd.  Why is this not static data (or mostly static
data), why does it vary at runtime?

> +static bool regmap_volatile_range(struct tps65910 *tps65910,
> +			unsigned int reg, unsigned int bytes)
> +{
> +	unsigned int i;
> +	for (i = 0; i < bytes; i++)
> +		if (!is_volatile_reg(tps65910->dev, reg + i))
> +			return false;
> +	return true;
> +}

I don't think this should be here (the naming is a bit of a clue - it's
named like a core function), see below where you're using it...

> +	unsigned char *wbuf = src;
> +	unsigned int ival;
> +
> +	if (regmap_volatile_range(tps65910, reg, bytes))
> +		return regmap_raw_write(tps65910->regmap, reg, src, bytes);
> +
> +	/* If any of register is non-volatile then use byte-wise transfer */
> +	for (i = 0; i < bytes; ++i) {
> +		ival = (unsigned int) (*wbuf++);
> +		ret = regmap_write(tps65910->regmap, reg, ival);
> +		if (ret < 0)
> +			return ret;
> +	}

There's nothing specific to the driver about this, if this is a good
idea add support for it to the core.

> +void tps65910_enable_reg_cache(struct tps65910 *tps65910, int reg)
> +{
> +	set_bit(reg, tps65910->cache_reg);
> +}
> +EXPORT_SYMBOL_GPL(tps65910_enable_reg_cache);

Why are you doing this?  This looks very icky, and if it is needed there
needs to be more code here to make sure the register cache and device
are in sync.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]