Hi Rajanikanth,

On 10/16/2012 05:36 AM, Rajanikanth H.V wrote:
> From: "Rajanikanth H.V" <rajanikanth...@stericsson.com>
> 
> - This patch adds device tree support for fuelgauge driver
> - optimize bm devices platform_data usage and of_probe(...)
>   Note: of_probe() routine for battery managed devices is made
>   common across all bm drivers.
> - test status:
>   - interrupt numbers assigned differs between legacy and FDT mode.
> 
> Legacy platform_data Mode:
> root@ME:/ cat /proc/interrupts
>            CPU0       CPU1
> 483:          0          0    ab8500  ab8500-ponkey-dbf
> 484:          0          0    ab8500  ab8500-ponkey-dbr
> 485:          0          0    ab8500  BATT_OVV
> 494:          0          1    ab8500
> 495:          0          0    ab8500  ab8500-rtc
> 501:          0         13    ab8500  NCONV_ACCU
> 503:          7         22    ab8500  CCEOC
> 504:          0          1    ab8500  CC_INT_CALIB
> 505:          0          0    ab8500  LOW_BAT_F
> 516:          0         34    ab8500  ab8500-gpadc
> 556:          0          0    ab8500  usb-link-status
> 
> FDT Mode:
> root@ME:/ cat /proc/interrupts
>            CPU0       CPU1
>   6:          0          0    ab8500  ab8500-ponkey-dbf
>   7:          0          0    ab8500  ab8500-ponkey-dbr
>   8:          0          0    ab8500  BATT_OVV
> 162:          0          7    ab8500  ab8500-gpadc
> 163:          0          1    ab8500
> 164:          0          0    ab8500  ab8500-rtc
> 484:          0          0    ab8500  usb-link-status
> 499:          0          4    ab8500  NCONV_ACCU
> 500:          0          0    ab8500  LOW_BAT_F
> 502:          0          1    ab8500  CC_INT_CALIB
> 503:          0          6    ab8500  CCEOC
> 
> Signed-off-by: Rajanikanth H.V <rajanikanth...@stericsson.com>
[...]
> +int __devinit
> +bmdevs_of_probe(struct device *dev,
> +             struct device_node *np,
> +             struct abx500_bm_data **battery)
> +{
> +     struct  abx500_battery_type *btype;
> +     struct  device_node *np_bat_supply;
> +     struct  abx500_bm_data *bat;
> +     int     i, thermistor;
> +     char    *bat_tech = "UNKNOWN";

This initialization is useless.

> +
> +     *battery = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL);
> +     if (!*battery) {
> +             dev_err(dev, "%s no mem for plat_data\n", __func__);
> +             return -ENOMEM;
> +     }
> +     *battery = &ab8500_bm_data;

Looks like dynamic allocation here is not what you need, since you are
overwriting the pointer to the allocated data.

> +
> +     /* get phandle to 'battery-info' node */
> +     np_bat_supply = of_parse_phandle(np, "battery", 0);
> +     if (!np_bat_supply) {
> +             dev_err(dev, "missing property battery\n");
> +             return -EINVAL;
> +     }
> +     if (of_property_read_bool(np_bat_supply,
> +                     "thermistor-on-batctrl"))
> +             thermistor = NTC_INTERNAL;
> +     else
> +             thermistor = NTC_EXTERNAL;
> +
> +     bat = *battery;
> +     if (thermistor == NTC_EXTERNAL) {
> +             bat->n_btypes  = 4;
> +             bat->bat_type  = bat_type_ext_thermistor;
> +             bat->adc_therm = ABx500_ADC_THERM_BATTEMP;
> +     }
> +     bat_tech = (char *)of_get_property(
> +                     np_bat_supply, "stericsson,battery-type", NULL);

No need to cast a void * to a char *.

> +     if (!bat_tech) {
> +             dev_warn(dev, "missing property battery-name/type\n");
> +             strncpy(bat_tech, "UNKNOWN", 7);

You are writing at a NULL pointer here...

> +     }
> +     of_node_put(np_bat_supply);

You can't call of_node_put here, because you are going to use bat_tech
later on. You have to call it at the end of this function, after you are
done with bat_tech.

> +
> +     if (strncmp(bat_tech, "LION", 4) == 0) {
> +             bat->no_maintenance  = true;
> +             bat->chg_unknown_bat = true;
> +             bat->bat_type[BATTERY_UNKNOWN].charge_full_design = 2600;
> +             bat->bat_type[BATTERY_UNKNOWN].termination_vol    = 4150;
> +             bat->bat_type[BATTERY_UNKNOWN].recharge_vol       = 4130;
> +             bat->bat_type[BATTERY_UNKNOWN].normal_cur_lvl     = 520;
> +             bat->bat_type[BATTERY_UNKNOWN].normal_vol_lvl     = 4200;
> +     }
> +     /* select the battery resolution table */
> +     for (i = 0; i < bat->n_btypes; ++i) {
> +             btype = (bat->bat_type + i);
> +             if (thermistor == NTC_EXTERNAL) {
> +                     btype->batres_tbl =
> +                             temp_to_batres_tbl_ext_thermistor;
> +             } else if (strncmp(bat_tech, "LION", 4) == 0) {
> +                     btype->batres_tbl =
> +                             temp_to_batres_tbl_9100;
> +             } else {
> +                     btype->batres_tbl =
> +                             temp_to_batres_tbl_thermistor;
> +             }
> +     }
> +     return 0;
> +}
[...]
> diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c
> index bf02225..ff64dd4 100644
> --- a/drivers/power/ab8500_fg.c
> +++ b/drivers/power/ab8500_fg.c
> @@ -22,15 +22,16 @@
>  #include <linux/platform_device.h>
>  #include <linux/power_supply.h>
>  #include <linux/kobject.h>
> -#include <linux/mfd/abx500/ab8500.h>
> -#include <linux/mfd/abx500.h>
>  #include <linux/slab.h>
> -#include <linux/mfd/abx500/ab8500-bm.h>
>  #include <linux/delay.h>
> -#include <linux/mfd/abx500/ab8500-gpadc.h>
> -#include <linux/mfd/abx500.h>
>  #include <linux/time.h>
> +#include <linux/of.h>
>  #include <linux/completion.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/abx500.h>
> +#include <linux/mfd/abx500/ab8500.h>
> +#include <linux/mfd/abx500/ab8500-bm.h>
> +#include <linux/mfd/abx500/ab8500-gpadc.h>
>  
>  #define MILLI_TO_MICRO                       1000
>  #define FG_LSB_IN_MA                 1627
> @@ -212,7 +213,6 @@ struct ab8500_fg {
>       struct ab8500_fg_avg_cap avg_cap;
>       struct ab8500 *parent;
>       struct ab8500_gpadc *gpadc;
> -     struct abx500_fg_platform_data *pdata;
>       struct abx500_bm_data *bat;
>       struct power_supply fg_psy;
>       struct workqueue_struct *fg_wq;
> @@ -2416,6 +2416,8 @@ static int __devexit ab8500_fg_remove(struct 
> platform_device *pdev)
>       int ret = 0;
>       struct ab8500_fg *di = platform_get_drvdata(pdev);
>  
> +     of_node_put(pdev->dev.of_node);

This is wrong, the probe function doesn't increment the refcount of this
node, so you don't have to decrement it here.

> +
>       list_del(&di->node);
>  
>       /* Disable coulomb counter */
> @@ -2429,7 +2431,6 @@ static int __devexit ab8500_fg_remove(struct 
> platform_device *pdev)
>       flush_scheduled_work();
>       power_supply_unregister(&di->fg_psy);
>       platform_set_drvdata(pdev, NULL);
> -     kfree(di);
>       return ret;
>  }
>  
> @@ -2442,21 +2443,44 @@ static struct ab8500_fg_interrupts ab8500_fg_irq[] = {
>       {"CCEOC", ab8500_fg_cc_data_end_handler},
>  };
>  
> +char *supply_interface[] = {
> +     "ab8500_chargalg",
> +     "ab8500_usb",
> +};
> +
>  static int __devinit ab8500_fg_probe(struct platform_device *pdev)
>  {
> +     struct device_node *np = pdev->dev.of_node;
> +     struct ab8500_fg *di;
>       int i, irq;
>       int ret = 0;
> -     struct abx500_bm_plat_data *plat_data = pdev->dev.platform_data;
> -     struct ab8500_fg *di;
>  
> -     if (!plat_data) {
> -             dev_err(&pdev->dev, "No platform data\n");
> -             return -EINVAL;
> +     di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL);
> +     if (!di) {
> +             dev_err(&pdev->dev, "%s no mem for ab8500_fg\n", __func__);
> +             if (np) {
> +                     ret = -ENOMEM;
> +                     goto release_node;

Here and below, release_node is wrong for the same reason as I wrote for
the remove function, you don't have to call of_node_put().

> +             }
> +     }
> +     di->bat = (struct abx500_bm_data *)
> +                     pdev->mfd_cell->platform_data;

No need to cast a void *.

> +     if (!di->bat) {
> +             if (np) {
> +                     ret = bmdevs_of_probe(&pdev->dev, np, &di->bat);
> +                     if (ret) {
> +                             dev_err(&pdev->dev,
> +                                     "failed to get battery information\n");
> +                             goto release_node;
> +                     }
> +             } else {
> +                     dev_err(&pdev->dev, "missing dt node for ab8500_fg\n");
> +                     ret = -EINVAL;
> +                     goto release_node;
> +             }
> +     } else {
> +             dev_info(&pdev->dev, "falling back to legacy platform data\n");
>       }
> -
> -     di = kzalloc(sizeof(*di), GFP_KERNEL);
> -     if (!di)
> -             return -ENOMEM;
>  
>       mutex_init(&di->cc_lock);
>  
> @@ -2465,29 +2489,13 @@ static int __devinit ab8500_fg_probe(struct 
> platform_device *pdev)
>       di->parent = dev_get_drvdata(pdev->dev.parent);
>       di->gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
>  
> -     /* get fg specific platform data */
> -     di->pdata = plat_data->fg;
> -     if (!di->pdata) {
> -             dev_err(di->dev, "no fg platform data supplied\n");
> -             ret = -EINVAL;
> -             goto free_device_info;
> -     }
> -
> -     /* get battery specific platform data */
> -     di->bat = plat_data->battery;
> -     if (!di->bat) {
> -             dev_err(di->dev, "no battery platform data supplied\n");
> -             ret = -EINVAL;
> -             goto free_device_info;
> -     }
> -
>       di->fg_psy.name = "ab8500_fg";
>       di->fg_psy.type = POWER_SUPPLY_TYPE_BATTERY;
>       di->fg_psy.properties = ab8500_fg_props;
>       di->fg_psy.num_properties = ARRAY_SIZE(ab8500_fg_props);
>       di->fg_psy.get_property = ab8500_fg_get_property;
> -     di->fg_psy.supplied_to = di->pdata->supplied_to;
> -     di->fg_psy.num_supplicants = di->pdata->num_supplicants;
> +     di->fg_psy.supplied_to = supply_interface;
> +     di->fg_psy.num_supplicants = ARRAY_SIZE(supply_interface),
>       di->fg_psy.external_power_changed = ab8500_fg_external_power_changed;
>  
>       di->bat_cap.max_mah_design = MILLI_TO_MICRO *
> @@ -2506,7 +2514,8 @@ static int __devinit ab8500_fg_probe(struct 
> platform_device *pdev)
>       di->fg_wq = create_singlethread_workqueue("ab8500_fg_wq");
>       if (di->fg_wq == NULL) {
>               dev_err(di->dev, "failed to create work queue\n");
> -             goto free_device_info;
> +             ret = -ENOMEM;
> +             goto release_node;
>       }
>  
>       /* Init work for running the fg algorithm instantly */
> @@ -2605,12 +2614,17 @@ free_irq:
>       }
>  free_inst_curr_wq:
>       destroy_workqueue(di->fg_wq);
> -free_device_info:
> -     kfree(di);
>  
> +release_node:
> +     of_node_put(np);
>       return ret;
>  }

--
Francesco

_______________________________________________
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev

Reply via email to