Re: [PATCH 1/2] mfd: dt: tps6586x: Add power off control

[Thierry Reding <thierry.reding@avionic-design.de>]

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

On Fri, Aug 17, 2012 at 02:16:28AM -0700, Bill Huang wrote:
[...]
> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
[...]
> @@ -505,6 +519,11 @@ static int __devinit tps6586x_i2c_probe(struct i2c_client *client,
>  		goto err_add_devs;
>  	}
>  
> +	tps6586x_dev = &client->dev;
> +
> +	if (pdata->pm_off && !pm_power_off)
> +		pm_power_off = tps6586x_power_off;
> +

I think the assignment of tps6586x_dev needs to go inside the if block
as well. Otherwise it might be overwritten by another instance for
systems that actually have more than one tps6586x. Since currently the
driver can't be built as a module it probably makes little sense to
clean this up in .remove(), but it might still be worth adding so it
isn't forgotten if and when somebody tries to convert the driver to a
module.

I should note that I don't like very much how the pm_power_off works.
Maybe this should really be changed to allow passing a context for the
function to work from. Something like:

	pm_power_off = tps6586x_power_off;
	pm_power_off_data = &client->dev;

Where pm_power_off() would receive pm_power_off_data as an argument.

Even that's not very pretty. On the other hand this doesn't really buy
us much because only the storage location of the variable would change
and nothing else. But it would still make the association of the data
clearer.

Thierry

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