| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50A9D816.7010208@marvell.com>
Date: Mon, 19 Nov 2012 14:56:22 +0800
From: Qing Xu <qingx@...vell.com>
To: Anton Vorontsov <anton.vorontsov@...aro.org>
Cc: "dwmw2@...radead.org" <dwmw2@...radead.org>,
"sameo@...ux.intel.com" <sameo@...ux.intel.com>,
"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
"haojian.zhuang@...il.com" <haojian.zhuang@...il.com>,
Chao Xie <cxie4@...vell.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"devicetree-discuss@...ts.ozlabs.org"
<devicetree-discuss@...ts.ozlabs.org>
Subject: Re: [PATCH 4/7] mfd: max8925: support dt for power supply
On 11/19/2012 08:24 AM, Anton Vorontsov wrote:
> On Tue, Nov 06, 2012 at 03:42:44PM +0800, Qing Xu wrote:
>> From: Qing Xu <qingx@...vell.com>
>>
>> Signed-off-by: Qing Xu <qingx@...vell.com>
>> ---
> W/ this patch I'm getting this warning:
>
> CC drivers/power/max8925_power.o
> drivers/power/max8925_power.c: In function ‘max8925_power_probe’:
> drivers/power/max8925_power.c:479:3: warning: statement with no effect
> [-Wunused-value]
>
>> drivers/power/max8925_power.c | 57 ++++++++++++++++++++++++++++++++++++----
>> 1 files changed, 51 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/max8925_power.c b/drivers/power/max8925_power.c
>> index daa333b..dd2ac2d 100644
>> --- a/drivers/power/max8925_power.c
>> +++ b/drivers/power/max8925_power.c
>> @@ -426,14 +426,58 @@ static __devexit int max8925_deinit_charger(struct max8925_power_info *info)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static int max8925_power_dt_init(struct platform_device *pdev,
>> + struct max8925_power_pdata *pdata)
>> +{
>> + struct device_node *nproot = pdev->dev.parent->of_node, *np;
>> + int batt_detect, topoff_threshold, fast_charge,
>> + no_temp_support, no_insert_detect;
> One variable declaration per line please.
>
>> +
>> + if (!nproot)
>> + return -ENODEV;
> Please add an empty line here.
>
>> + np = of_find_node_by_name(nproot, "charger");
>> + if (!np) {
>> + dev_err(&pdev->dev, "failed to find charger node\n");
>> + return -ENODEV;
>> + }
> ditto.
>
>> + 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;
> ditto
>
>> + pr_err("batt_detect%d, topoff_threshold%d, fast_charge%d,no_temp_support%d, no_insert_detect%d\n",
>> + batt_detect, topoff_threshold, fast_charge,
>> + no_temp_support, no_insert_detect);
> pr_err, are you sure? :)
>
>> + return 0;
>> +}
>> +#else
>> +#define max8925_power_dt_init(x, y) (-1)
> You should make it static inline function to get rid of the warning above.
>
>> +#endif
>> +
>> +static char *power_supplicants[] = {
>> + "max8925-battery",
>> +};
>> +
>> static __devinit int max8925_power_probe(struct platform_device *pdev)
>> {
>> struct max8925_chip *chip = dev_get_drvdata(pdev->dev.parent);
>> - struct max8925_power_pdata *pdata = NULL;
>> + struct max8925_power_pdata *pdata = pdev->dev.platform_data;
>> struct max8925_power_info *info;
>> int ret;
>>
>> - pdata = pdev->dev.platform_data;
>> + if (pdev->dev.parent->of_node && !pdata) {
>> + pdata = devm_kzalloc(&pdev->dev,
>> + sizeof(struct max8925_power_pdata),
>> + GFP_KERNEL);
>>
> Please move this logic into max8925_power_dt_init().
> So it will look like
>
> if (!pdata)
> pdata = max8925_power_dt_init(pdev);
> if (!pdata) {
> dev_err(...);
> return ...;
> }
>
>> + max8925_power_dt_init(pdev, pdata);
>> + }
>> if (!pdata) {
>> dev_err(&pdev->dev, "platform data isn't assigned to "
>> "power supply\n");
>> @@ -453,8 +497,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;
>> + info->ac.num_supplicants = ARRAY_SIZE(power_supplicants);
>> ret = power_supply_register(&pdev->dev, &info->ac);
>> if (ret)
>> goto out;
>> @@ -465,8 +509,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;
>> + info->usb.num_supplicants = ARRAY_SIZE(power_supplicants);
>>
>> ret = power_supply_register(&pdev->dev, &info->usb);
>> if (ret)
>> @@ -491,6 +535,7 @@ static __devinit int max8925_power_probe(struct platform_device *pdev)
>> info->no_insert_detect = pdata->no_insert_detect;
>>
>> max8925_init_charger(chip, info);
>> +
> This change is unrelated.
>
>> return 0;
>> out_battery:
>> power_supply_unregister(&info->battery);
>> --
>> 1.7.0.4
> Thanks,
> Anton.
Anton, thanks for your very careful review!
I updated in patch v2, please help to review it again.
Thanks,
Qing
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists