Re: [PATCH 1/7] regulator: act8865: Expose act8600 registers via debugfs

[Mark Brown <broonie@kernel.org>]

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

On Sun, Feb 28, 2016 at 04:53:23PM +0100, Maarten ter Huurne wrote:

> The read/write/volatile configuration is valid also when debugfs is
> not enabled, but it doesn't add any value then.

Please write changelogs that accurately describe what your changes do.
Any debugfs changes are at best a second order side effect of changes
here, what this appears to do is some combination of defining one or
more new registers (which don't seem to ever be referred to...) and
providing some access maps.

> +#ifdef CONFIG_DEBUG_FS
> +

No, this is broken.  The access information for a device is not affected
by debugfs and does have an effect on how we work with the device.
Having access maps that depend on random build settings like this is a
recipie for hard to diagnose bugs.

> +static const struct regmap_range act8600_reg_ranges[] = {
> +	{ 0x00, 0x01 },
> +	{ 0x10, 0x10 }, { 0x12, 0x12 },
> +	{ 0x20, 0x20 }, { 0x22, 0x22 },
> +	{ 0x30, 0x30 }, { 0x32, 0x32 },

Your formatting here is just weird and confusing, randomly grouping
things on lines with no obvious .

> +};
> +static const struct regmap_range act8600_reg_ro_ranges[] = {

Missing blank lines between variables.

> +static const struct regmap_range act8600_reg_volatile_ranges[] = {
> +	{ 0x00, 0x01 },

For ranges you should explicitly use .start and .end otherwise these
look like they're intended to be register default tables.

> @@ -421,6 +470,11 @@ static int act8865_pmic_probe(struct i2c_client *client,
>  	struct device_node **of_node;
>  	int i, ret, num_regulators;
>  	struct act8865 *act8865;
> +	struct regmap_config regmap_config = {
> +		.reg_bits = 8,
> +		.val_bits = 8,
> +		.max_register = 0xFF,
> +	};

Why have you moved this from a global static variable where it normally
is to a local variable?  This is unusual and confusing...

> +#ifdef CONFIG_DEBUG_FS
> +		regmap_config.wr_table = &act8600_write_ranges_table;
> +		regmap_config.rd_table = &act8600_read_ranges_table;
> +		regmap_config.volatile_table = &act8600_volatile_ranges_table;
> +#endif

...and the fact that you're doing things like this ought to be a warning
that there's a problem with the way you're handling the access maps.

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