On Fri, Feb 14, 2014 at 2:43 PM, Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Fri, Feb 14, 2014 at 01:24:19PM +0000, Marek Belisko wrote:
>> Signed-off-by: Marek Belisko <marek@xxxxxxxxxxxxx>
>> ---
>> .../bindings/power_supply/twl4030_madc_battery.txt | 15 +++
>> drivers/power/twl4030_madc_battery.c | 109 +++++++++++++++++++++
>> 2 files changed, 124 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> new file mode 100644
>> index 0000000..bebc876
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power_supply/twl4030_madc_battery.txt
>> @@ -0,0 +1,15 @@
>> +twl4030_madc_battery
>> +
>> +Required properties:
>> + - compatible : "ti,twl4030-madc-battery"
>> + - capacity : battery capacity in uAh
>> + - charging-calibration-data : list of voltage(mV):level(%) values for charging calibration (see example)
>> + - discharging-calibration-data : list of voltage(mV):level(%) values for discharging calibration (see example)
>> +
>> +Example:
>> + madc-battery {
>> + compatible = "ti,twl4030-madc-battery";
>> + capacity = <1200000>;
>> + charging-calibration-data = <4200 100 4100 75 4000 55 3900 25 3800 5 3700 2 3600 1 3300 0>;
>> + discharging-calibration-data = <4200 100 4100 95 4000 70 3800 50 3700 10 3600 5 3300 0>;
>
> Please bracket list entries individually.
OK.
>
>> + };
>> diff --git a/drivers/power/twl4030_madc_battery.c b/drivers/power/twl4030_madc_battery.c
>> index 7ef445a..2843382 100644
>> --- a/drivers/power/twl4030_madc_battery.c
>> +++ b/drivers/power/twl4030_madc_battery.c
>> @@ -19,6 +19,7 @@
>> #include <linux/sort.h>
>> #include <linux/i2c/twl4030-madc.h>
>> #include <linux/power/twl4030_madc_battery.h>
>> +#include <linux/of.h>
>>
>> struct twl4030_madc_battery {
>> struct power_supply psy;
>> @@ -188,6 +189,110 @@ static int twl4030_cmp(const void *a, const void *b)
>> ((struct twl4030_madc_bat_calibration *)a)->voltage;
>> }
>>
>> +#ifdef CONFIG_OF
>> +static struct twl4030_madc_bat_platform_data *
>> + twl4030_madc_dt_probe(struct platform_device *pdev)
>> +{
>> + struct twl4030_madc_bat_platform_data *pdata;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct property *prop;
>> + int ret;
>> + int sz, i, j = 0;
>> +
>> + pdata = devm_kzalloc(&pdev->dev,
>> + sizeof(struct twl4030_madc_bat_platform_data),
>> + GFP_KERNEL);
>> + if (!pdata)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ret = of_property_read_u32(np, "capacity", &pdata->capacity);
>> + if (ret != 0)
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* parse and prepare charging data */
>> + prop = of_find_property(np, "charging-calibration-data", &sz);
>> + if (!prop)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (sz % 2) {
>> + dev_warn(&pdev->dev, "Count of charging-calibration-data must be even!\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>
> As sz is in bytes this checks that the property is a multiple of 2
> bytes, not that it has an even number of u32 elements.
>
> Heiko Stübner recently added of_property_count_u32_elems [1,2]. Use that
> instead.
OK I'll convert it to that approach. Then seems code will be much
easier. Thanks for pointing to it.
>
>> +
>> + sz /= sizeof(u32);
>> +
>> + {
>> + u32 data[sz];
>> +
>> + ret = of_property_read_u32_array(np,
>> + "charging-calibration-data", &data[0], sz);
>> + if (ret)
>> + return ERR_PTR(ret);
>
> Why not just allocate then try to read, possibly having to free if the
> read fails?
>
> Otherwise we're trying to put an arbitrarily large value onto the stack
> for no good reason.
>
>> +
>> + pdata->charging = devm_kzalloc(&pdev->dev,
>> + sizeof(struct twl4030_madc_bat_calibration) * (sz / 2),
>> + GFP_KERNEL);
>> +
>> + for (i = 0; i < sz; i += 2) {
>> + pdata->charging[j].voltage = data[i];
>> + pdata->charging[j].level = data[i+1];
>> + j++;
>
> Why not have (i = 0; i < sz/2; i++), and get rid of j?
>
>> + }
>> +
>> + pdata->charging_size = sz / 2;
>> + }
>> +
>> + /* parse and prepare discharging data */
>> + prop = of_find_property(np, "discharging-calibration-data", &sz);
>> + if (!prop)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (sz % 2) {
>> + dev_warn(&pdev->dev, "Count of discharging-calibration-data must be even!\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>
> This has the same issues as with charging-calibration-data.
>
> Thanks,
> Mark.
>
> [1] http://www.spinics.net/lists/devicetree/msg21358.html
> [2] http://www.spinics.net/lists/devicetree/msg21502.html
BR,
marek
--
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer
Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
