Re: [PATCH v2 4/7] mfd: max8925: support dt for power supply

[Anton Vorontsov <cbouatmailru@gmail.com>]

On Mon, Nov 19, 2012 at 02:52:24PM +0800, Qing Xu wrote:
> From: Qing Xu <qingx@marvell.com>
> 
> Signed-off-by: Qing Xu <qingx@marvell.com>
> ---
>  drivers/power/max8925_power.c |   61 +++++++++++++++++++++++++++++++++++++---
>  1 files changed, 56 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
> index daa333b..aca3f68 100644
> --- a/drivers/power/max8925_power.c
> +++ b/drivers/power/max8925_power.c
> @@ -426,6 +426,57 @@ static __devexit int max8925_deinit_charger(struct max8925_power_info *info)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static struct max8925_power_pdata*

Need a whitespace before *.

> +max8925_power_dt_init(	struct platform_device *pdev)

Unneeded tab.

> +{
> +	struct device_node *nproot = pdev->dev.parent->of_node, *np;

Hmmm. No, this is cryptic. One variable declaration per line please.

> +	int batt_detect;
> +	int topoff_threshold;
> +	int fast_charge;
> +	int no_temp_support;
> +	int no_insert_detect;
> +	struct max8925_power_pdata *pdata;
> +
> +	if (!nproot)
> +		return pdev->dev.platform_data;
> +
> +	np = of_find_node_by_name(nproot, "charger");
> +	if (!np) {
> +		dev_err(&pdev->dev, "failed to find charger node\n");
> +		return NULL;
> +	}
> +
> +	pdata = devm_kzalloc(&pdev->dev,
> +			sizeof(struct max8925_power_pdata),
> +			GFP_KERNEL);
> +
> +	of_property_read_u32(np, "topoff-threshold", &topoff_threshold);
> +	of_property_read_u32(np, "batt-detect", &batt_detect);
> +	of_property_read_u32(np, "fast-charge", &fast_charge);
> +	of_property_read_u32(np, "no-insert-detect", &no_insert_detect);
> +	of_property_read_u32(np, "no-temp-support", &no_temp_support);
> +
> +	pdata->batt_detect = batt_detect;
> +	pdata->fast_charge = fast_charge;
> +	pdata->topoff_threshold = topoff_threshold;
> +	pdata->no_insert_detect = no_insert_detect;
> +	pdata->no_temp_support = no_temp_support;
> +
> +	return pdata;
> +}
> +#else
> +static struct max8925_power_pdata*
> +max8925_power_dt_init(	struct platform_device *pdev)

Unneeded tab.

> +{
> +	return pdev->dev.platform_data;
> +}
> +#endif
> +
> +static char *power_supplicants[] = {

The rest of the driver uses max8925_ prefix. Also, should it be const char
*?

> +	"max8925-battery",
> +};
> +
>  static __devinit int max8925_power_probe(struct platform_device *pdev)
>  {
>  	struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
> @@ -433,7 +484,7 @@ static __devinit int max8925_power_probe(struct platform_device *pdev)
>  	struct max8925_power_info *info;
>  	int ret;
>  
> -	pdata = pdev->dev.platform_data;
> +	pdata = max8925_power_dt_init(pdev);
>  	if (!pdata) {
>  		dev_err(&pdev->dev, "platform data isn't assigned to "
>  			"power supply\n");
> @@ -453,8 +504,8 @@ static __devinit int max8925_power_probe(struct platform_device *pdev)
>  	info->ac.properties = max8925_ac_props;
>  	info->ac.num_properties = ARRAY_SIZE(max8925_ac_props);
>  	info->ac.get_property = max8925_ac_get_prop;
> -	info->ac.supplied_to = pdata->supplied_to;
> -	info->ac.num_supplicants = pdata->num_supplicants;
> +	info->ac.supplied_to = power_supplicants;

So you no longer able to change supplied_to via platform data. This is a
backwards-incompatible change.

Commit message should at least explain why this is safe to do (no users in
the kernel?).

> +	info->ac.num_supplicants = ARRAY_SIZE(power_supplicants);
>  	ret = power_supply_register(&pdev->dev, &info->ac);
>  	if (ret)
>  		goto out;
> @@ -465,8 +516,8 @@ static __devinit int max8925_power_probe(struct platform_device *pdev)
>  	info->usb.properties = max8925_usb_props;
>  	info->usb.num_properties = ARRAY_SIZE(max8925_usb_props);
>  	info->usb.get_property = max8925_usb_get_prop;
> -	info->usb.supplied_to = pdata->supplied_to;
> -	info->usb.num_supplicants = pdata->num_supplicants;
> +	info->usb.supplied_to = power_supplicants;

Ditto.

> +	info->usb.num_supplicants = ARRAY_SIZE(power_supplicants);
>  
>  	ret = power_supply_register(&pdev->dev, &info->usb);
>  	if (ret)
> -- 
> 1.7.0.4

Thanks,
Anton.