Re: [PATCH V2 5/6] Thermal: Add ST-Ericsson DB8500 thermal driver.
On Thu Oct 25 11:13:59 2012, Hongbo Zhang wrote:
> From: "hongbo.zhang"
>
> This diver is based on the thermal management framework in thermal_sys.c. A
> thermal zone device is created with the trip points to which cooling devices
> can be bound, the current cooling device is cpufreq, e.g. CPU frequency is
> clipped down to cool the CPU, and other cooling devices can be added and bound
> to the trip points dynamically. The platform specific PRCMU interrupts are
> used to active thermal update when trip points are reached.
>
> Signed-off-by: hongbo.zhang
> ---
> .../devicetree/bindings/thermal/db8500-thermal.txt | 40 ++
> drivers/thermal/Kconfig| 20 +
> drivers/thermal/Makefile | 2 +
> drivers/thermal/db8500_cpufreq_cooling.c | 121 +
> drivers/thermal/db8500_thermal.c | 523
> +
> include/linux/platform_data/db8500_thermal.h | 38 ++
> 6 files changed, 744 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/thermal/db8500-thermal.txt
> create mode 100644 drivers/thermal/db8500_cpufreq_cooling.c
> create mode 100644 drivers/thermal/db8500_thermal.c
> create mode 100644 include/linux/platform_data/db8500_thermal.h
>
> diff --git a/Documentation/devicetree/bindings/thermal/db8500-thermal.txt
> b/Documentation/devicetree/bindings/thermal/db8500-thermal.txt
> new file mode 100644
> index 000..abe08d7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/db8500-thermal.txt
> @@ -0,0 +1,40 @@
> +* ST-Ericsson DB8500 Thermal
> +
> +** Thermal node properties:
> +
> +- compatible : "stericsson,db8500-thermal";
> +- reg : address range of the thermal sensor registers;
> +- interrupts : interrupts generated form PRCMU;
s/form/from
[...]
> diff --git a/drivers/thermal/db8500_cpufreq_cooling.c
> b/drivers/thermal/db8500_cpufreq_cooling.c
> new file mode 100644
> index 000..f6a8d24
> --- /dev/null
> +++ b/drivers/thermal/db8500_cpufreq_cooling.c
> @@ -0,0 +1,121 @@
> +/*
> + * db8500_cpufreq_cooling.c - db8500 cpufreq works as cooling device.
> + *
> + * Copyright (C) 2012 ST-Ericsson
> + * Copyright (C) 2012 Linaro Ltd.
> + *
> + * Author: Hongbo Zhang
Why do you keep writing an incorrect email address? :)
I'm referring to the spelling "hognbo" instead of "hongbo".
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static LIST_HEAD(db8500_cpufreq_cdev_list);
> +
> +struct db8500_cpufreq_cdev {
> + struct thermal_cooling_device *cdev;
> + struct list_head node;
> +};
> +
> +static int db8500_cpufreq_cooling_probe(struct platform_device *pdev)
> +{
> + struct db8500_cpufreq_cdev *cooling_dev;
> + struct cpumask mask_val;
> +
> + cooling_dev = devm_kzalloc(&pdev->dev, sizeof(*cooling_dev),
> + GFP_KERNEL);
> + if (!cooling_dev)
> + return -ENOMEM;
> +
> + cpumask_set_cpu(0, &mask_val);
> + cooling_dev->cdev = cpufreq_cooling_register(&mask_val);
> +
> + if (IS_ERR_OR_NULL(cooling_dev->cdev)) {
> + dev_err(&pdev->dev, "Failed to register cooling device\n");
> + return PTR_ERR(cooling_dev->cdev);
> + }
> +
> + platform_set_drvdata(pdev, cooling_dev->cdev);
> + list_add_tail(&cooling_dev->node, &db8500_cpufreq_cdev_list);
> + dev_info(&pdev->dev, "Cooling device registered: %s\n",
> + cooling_dev->cdev->type);
> +
> + return 0;
> +}
> +
> +static int db8500_cpufreq_cooling_remove(struct platform_device *pdev)
> +{
> + struct db8500_cpufreq_cdev *cooling_dev;
> +
> + list_for_each_entry(cooling_dev, &db8500_cpufreq_cdev_list, node)
> + if (cooling_dev->cdev == platform_get_drvdata(pdev)) {
> + cpufreq_cooling_unregister(cooling_dev->cdev);
> + list_del(&cooling_dev->node);
> + }
> +
> + return 0;
> +}
There is no need to keep an internal list of cooling devices, and the
struct db8500_cpufreq_cdev definition is not needed either.
In probe() you can simply call platform_set_drvdata() with the pointer
returned by cpufreq_cooling_register(); conversely, in remove() you can
simply call cpufreq_cooling_unregister() with the pointer returned by
platform_get_drvdata().
> +
> +static int db8500_cpufreq_cooling_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + retur
Re: [PATCH 1/4] mfd: ab8500: add devicetree support for fuelgauge
On 10/25/2012 08:30 AM, Rajanikanth H.V wrote:
> From: "Rajanikanth H.V"
>
> - 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.
>
> Signed-off-by: Rajanikanth H.V
[...]
> diff --git a/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt
> b/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt
> new file mode 100644
> index 000..28eaf35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power_supply/ab8500/fg.txt
> @@ -0,0 +1,58 @@
> +=== AB8500 Fuel Gauge Driver ===
> +
> +AB8500 is a mixed signal multimedia and power management
> +device comprising: power and energy-management-module,
> +wall-charger, usb-charger, audio codec, general purpose adc,
> +tvout, clock management and sim card interface.
> +
> +Fuelgauge support is part of energy-management-modules, other
> +components of this module are:
> +main-charger, usb-combo-charger and battery-temperature-monitoring.
> +
> +The properties below describes the node for fuelgauge driver.
> +
> +Required Properties:
> +- compatible = This shall be: "stericsson,ab8500-fg"
> +- battery = Shall be battery specific information
> + Example:
> + ab8500_fg {
> + compatible = "stericsson,ab8500-fg";
> + battery= <&ab8500_battery>;
> + };
> +
> +dependent node:
> + ab8500_battery: ab8500_battery {
> + };
> + This node will provide information on 'thermistor interface' and
> + 'battery technology type' used.
> +
> +Properties of this node are:
> +thermistor-on-batctrl:
> + A boolean value indicating thermistor interface to battery
> +
> + Note:
> + 'btemp' and 'batctrl' are the pins interfaced for battery temperature
> + measurement, 'btemp' signal is used when NTC(negative temperature
> + coefficient) resister is interfaced external to battery whereas
> + 'batctrl' pin is used when NTC resister is internal to battery.
> +
> + Example:
> + ab8500_battery: ab8500_battery {
> + thermistor-on-batctrl;
> + };
> + indiactes: NTC resister is internal to battery, 'batctrl' is used
s/indiactes/indicates
[...]
> +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;
> + const char *bat_tech;
> + int i, thermistor;
> +
> + *battery = &ab8500_bm_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 = of_get_property(np_bat_supply,
> + "stericsson,battery-type", NULL);
> + if (!bat_tech)
> + dev_warn(dev, "missing property battery-name/type\n");
> +
> + if (strncmp(bat_tech, "LION", 4) == 0) {
What if bat_tech is NULL?
[...]
> diff --git a/drivers/power/ab8500_fg.c b/drivers/power/ab8500_fg.c
> index bf02225..e117920 100644
> --- a/drivers/power/ab8500_fg.c
> +++ b/drivers/power/ab8500_fg.c
> @@ -22,15 +22,16 @@
> #include
> #include
> #include
> -#include
> -#include
> #include
> -#include
> #include
> -#include
> -#include
> #include
> +#include
> #include
> +#include
> +#include
> +#include
> +#include
> +#include
>
> #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;
pdata should be removed from the description of the struct members as well.
--
Francesco
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/4] mfd: ab8500: add devicetree support for btemp
On 10/25/2012 08:30 AM, Rajanikanth H.V wrote: > From: "Rajanikanth H.V" > > This patch adds device tree support for > battery-temperature-monitor driver > > Signed-off-by: Rajanikanth H.V > --- > Documentation/devicetree/bindings/mfd/ab8500.txt |6 ++ > .../bindings/power_supply/ab8500/btemp.txt | 16 + > arch/arm/boot/dts/dbx5x0.dtsi |5 ++ > drivers/mfd/ab8500-core.c |5 ++ > drivers/power/Kconfig |6 -- > drivers/power/ab8500_bmdata.c |4 +- > drivers/power/ab8500_btemp.c | 66 > > 7 files changed, 73 insertions(+), 35 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/power_supply/ab8500/btemp.txt > > diff --git a/Documentation/devicetree/bindings/mfd/ab8500.txt > b/Documentation/devicetree/bindings/mfd/ab8500.txt > index 6ca8d81..179c802 100644 > --- a/Documentation/devicetree/bindings/mfd/ab8500.txt > +++ b/Documentation/devicetree/bindings/mfd/ab8500.txt > @@ -30,6 +30,12 @@ ab8500-fg: : > vddadc : Fuel Gauge >: LOW_BAT_F: : LOW threshold > battery voltage >: CC_INT_CALIB : : Coulomb > Counter Internal Calibration >: CCEOC: : Coulomb > Counter End of Conversion > +ab8500-btemp : : vtvout : Battery > Temperature > + : BAT_CTRL_INDB: : Battery > Removal Indicator > + : BTEMP_LOW: : Btemp < > BtempLow, if battery temperature is lower than -10°C > + : BTEMP_HIGH : : BtempLow < > Btemp < BtempMedium,if battery temperature is between -10 and 0°C > + : BTEMP_LOW_MEDIUM : : BtempMedium < > Btemp < BtempHigh,if battery temperature is between 0°C and“MaxTemp > + : BTEMP_MEDIUM_HIGH: : Btemp > > BtempHigh, if battery temperature is higher than “MaxTemp” BTEMP_HIGH, BTEMP_LOW_MEDIUM and BTEMP_MEDIUM_HIGH are in the wrong order and don't correspond to their description. -- Francesco ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 3/4] mfd: ab8500: add devicetree support for charger
On 10/25/2012 08:30 AM, Rajanikanth H.V wrote:
> From: "Rajanikanth H.V"
>
> This patch adds device tree support for ab8500-charger
> driver
>
> Signed-off-by: Rajanikanth H.V
[...]
> diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
> index 78a730c..956943e 100644
> --- a/drivers/power/ab8500_charger.c
> +++ b/drivers/power/ab8500_charger.c
> @@ -23,6 +23,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -183,7 +185,6 @@ struct ab8500_charger_usb_state {
> * @autopowerIndicate if we should have automatic pwron
> after pwrloss
> * @parent: Pointer to the struct ab8500
> * @gpadc: Pointer to the struct gpadc
> - * @pdata: Pointer to the abx500_charger platform data
> * @bat: Pointer to the abx500_bm platform data
> * @flags: Structure for information about events triggered
> * @usb_state: Structure for usb stack information
> @@ -218,9 +219,9 @@ struct ab8500_charger {
> int vbat;
> int old_vbat;
> bool autopower;
> + bool autopower_cfg;
autopower_cfg should be added in the description of the struct members
as well.
--
Francesco
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 4/4] mfd: ab8500: add devicetree support for chargalg
On 10/25/2012 08:30 AM, Rajanikanth H.V wrote:
> From: "Rajanikanth H.V"
>
> This patch adds device tree support for charging algorithm
> driver
>
> Signed-off-by: Rajanikanth H.V
[...]
> diff --git a/drivers/power/abx500_chargalg.c b/drivers/power/abx500_chargalg.c
> index 88b5cc1..829fcfd 100644
> --- a/drivers/power/abx500_chargalg.c
> +++ b/drivers/power/abx500_chargalg.c
> @@ -21,6 +21,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
> #include
> @@ -231,7 +233,6 @@ struct abx500_chargalg {
> struct abx500_chargalg_charger_info chg_info;
> struct abx500_chargalg_battery_data batt_data;
> struct abx500_chargalg_suspension_status susp_status;
> - struct abx500_bmdevs_plat_data *pdata;
pdata should be removed from the description of the struct members as well.
> struct abx500_bm_data *bat;
> struct power_supply chargalg_psy;
> struct ux500_charger *ac_chg;
> @@ -1795,25 +1796,45 @@ static int __devexit abx500_chargalg_remove(struct
> platform_device *pdev)
> flush_scheduled_work();
> power_supply_unregister(&di->chargalg_psy);
> platform_set_drvdata(pdev, NULL);
> - kfree(di);
>
> return 0;
> }
>
> +static char *supply_interface[] = {
> + "ab8500_fg",
> +};
> +
> static int __devinit abx500_chargalg_probe(struct platform_device *pdev)
> {
> - struct abx500_bmdevs_plat_data *plat_data;
> + struct device_node *np = pdev->dev.of_node;
> + struct abx500_chargalg *di;
> int ret = 0;
>
> - struct abx500_chargalg *di =
> - kzalloc(sizeof(struct abx500_chargalg), GFP_KERNEL);
> - if (!di)
> + di = devm_kzalloc(&pdev->dev, sizeof(*di), GFP_KERNEL);
> + if (!di) {
> + dev_err(&pdev->dev, "%s no mem for ab8500_chargalg\n",
> __func__);
> return -ENOMEM;
> + }
> + di->bat = pdev->mfd_cell->platform_data;
> + 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");
> + return ret;
> + }
> + } else {
> + dev_err(&pdev->dev, "missing dt node for
> ab8500_chargalg\n");
> + return -EINVAL;
> + }
> + } else {
> + dev_info(&pdev->dev, "falling back to legacy platform data\n");
> + printk("%s falling back to legacy platform data\n", __func__);
You forgot to remove the printk() call.
--
Francesco
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/4] mfd: ab8500: add devicetree support for fuelgauge
On 27 October 2012 20:37, Francesco Lavra wrote:
> On 10/25/2012 08:30 AM, Rajanikanth H.V wrote:
>> From: "Rajanikanth H.V"
>> + bat_tech = of_get_property(np_bat_supply,
>> + "stericsson,battery-type", NULL);
>> + if (!bat_tech)
>> + dev_warn(dev, "missing property battery-name/type\n");
>> +
>> + if (strncmp(bat_tech, "LION", 4) == 0) {
>
> What if bat_tech is NULL?
It will be UNKNOWN
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/4] mfd: ab8500: add devicetree support for fuelgauge
On 10/27/2012 06:00 PM, Rajanikanth HV wrote:
> On 27 October 2012 20:37, Francesco Lavra wrote:
>> On 10/25/2012 08:30 AM, Rajanikanth H.V wrote:
>>> From: "Rajanikanth H.V"
>>> + bat_tech = of_get_property(np_bat_supply,
>>> + "stericsson,battery-type", NULL);
>>> + if (!bat_tech)
>>> + dev_warn(dev, "missing property battery-name/type\n");
>>> +
>>> + if (strncmp(bat_tech, "LION", 4) == 0) {
>>
>> What if bat_tech is NULL?
> It will be UNKNOWN
I wanted to draw your attention to the fact that if bat_tech is NULL you
are passing a NULL pointer to strncmp(), which is not good.
So you should assign a default value to bat_tech in case the battery
type property is not found in the DT, as below:
if (!bat_tech) {
dev_warn(dev, "missing property battery-name/type\n");
bat_tech = "UNKNOWN";
}
--
Francesco
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
