Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751613Ab2KSHJQ (ORCPT ); Mon, 19 Nov 2012 02:09:16 -0500 Received: from mail-pa0-f46.google.com ([209.85.220.46]:44236 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751239Ab2KSHJP (ORCPT ); Mon, 19 Nov 2012 02:09:15 -0500 Date: Sun, 18 Nov 2012 23:06:05 -0800 From: Anton Vorontsov To: Qing Xu Cc: dwmw2@infradead.org, sameo@linux.intel.com, grant.likely@secretlab.ca, rob.herring@calxeda.com, haojian.zhuang@gmail.com, cxie4@marvell.com, linux-kernel@vger.kernel.org, devicetree-discuss@lists.ozlabs.org Subject: Re: [PATCH v2 4/7] mfd: max8925: support dt for power supply Message-ID: <20121119070605.GA7294@lizard> References: <1353307944-9805-1-git-send-email-qingx@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1353307944-9805-1-git-send-email-qingx@marvell.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4307 Lines: 137 On Mon, Nov 19, 2012 at 02:52:24PM +0800, Qing Xu wrote: > From: Qing Xu > > Signed-off-by: Qing Xu > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/