[PATCH 0/3] power: max17042_battery: Add DT bindings

[PATCH 0/3] power: max17042_battery: Add DT bindings

[Karol Lewandowski]

Hi,

This series of patches adds bindings to max17042_battery driver making
it possible to instantiate it from device tree description.

First, convience patch, simply replaces kzalloc with devm_kzalloc
where possible.

Second, most intrusive patch adds variables orginally found only in
platform data to max17042_chip structure.  I decided it to do it that
way rather than allocating new platform data and then populating it
from device tree description.

Last patch adds DT bindings itself.

I would be grateful for any comments and I will be more than happy to
rework this patchset if needed.


Karol Lewandowski (3):
  max17042_battery: Use devm_kzalloc() where applicable
  max17042_battery: Preserve properties outside of platform data
  max17042_battery: Make it possible to instantiate driver from DT

 .../bindings/power_supply/max17042_battery.txt     |   18 +++++
 drivers/power/max17042_battery.c                   |   77 ++++++++++++++++----
 2 files changed, 79 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power_supply/max17042_battery.txt

-- 
1.7.8.3

[PATCH 1/3] max17042_battery: Use devm_kzalloc() where applicable

[Karol Lewandowski]

This allows us to simplify probe and exit function.

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/power/max17042_battery.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 86acee2..21a3650 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -220,7 +220,7 @@ static int __devinit max17042_probe(struct i2c_client *client,
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
 		return -EIO;
 
-	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	chip = devm_kzalloc(&client->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
@@ -246,7 +246,6 @@ static int __devinit max17042_probe(struct i2c_client *client,
 	ret = power_supply_register(&client->dev, &chip->battery);
 	if (ret) {
 		dev_err(&client->dev, "failed: power supply register\n");
-		kfree(chip);
 		return ret;
 	}
 
@@ -269,7 +268,6 @@ static int __devexit max17042_remove(struct i2c_client *client)
 	struct max17042_chip *chip = i2c_get_clientdata(client);
 
 	power_supply_unregister(&chip->battery);
-	kfree(chip);
 	return 0;
 }
 
-- 
1.7.8.3

Re: [PATCH 1/3] max17042_battery: Use devm_kzalloc() where applicable

[Anton Vorontsov]

On Wed, Feb 22, 2012 at 07:06:20PM +0100, Karol Lewandowski wrote:
> This allows us to simplify probe and exit function.
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---

Applied, thanks!

-- 
Anton Vorontsov
Email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

[PATCH 2/3] max17042_battery: Preserve properties outside of platform data

[Karol Lewandowski]

Add fields originally found in platform data back to max17042_chip,
as the former data structure might be not available on device
tree-based systems.

This commit makes it possible to safely declare platform data with
__initdata tag.

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/power/max17042_battery.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 21a3650..49c1377 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -33,7 +33,8 @@
 struct max17042_chip {
 	struct i2c_client *client;
 	struct power_supply battery;
-	struct max17042_platform_data *pdata;
+	bool enable_current_sense;
+	u32 r_sns;
 };
 
 static int max17042_write_reg(struct i2c_client *client, u8 reg, u16 value)
@@ -168,7 +169,7 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = val->intval * 10 / 256;
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
-		if (chip->pdata->enable_current_sense) {
+		if (chip->enable_current_sense) {
 			ret = max17042_read_reg(chip->client, MAX17042_Current);
 			if (ret < 0)
 				return ret;
@@ -180,13 +181,13 @@ static int max17042_get_property(struct power_supply *psy,
 				val->intval++;
 				val->intval *= -1;
 			}
-			val->intval *= 1562500 / chip->pdata->r_sns;
+			val->intval *= 1562500 / chip->r_sns;
 		} else {
 			return -EINVAL;
 		}
 		break;
 	case POWER_SUPPLY_PROP_CURRENT_AVG:
-		if (chip->pdata->enable_current_sense) {
+		if (chip->enable_current_sense) {
 			ret = max17042_read_reg(chip->client,
 						MAX17042_AvgCurrent);
 			if (ret < 0)
@@ -199,7 +200,7 @@ static int max17042_get_property(struct power_supply *psy,
 				val->intval++;
 				val->intval *= -1;
 			}
-			val->intval *= 1562500 / chip->pdata->r_sns;
+			val->intval *= 1562500 / chip->r_sns;
 		} else {
 			return -EINVAL;
 		}
@@ -215,6 +216,7 @@ static int __devinit max17042_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct max17042_chip *chip;
+	struct max17042_platform_data *pdata;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA))
@@ -225,7 +227,7 @@ static int __devinit max17042_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	chip->client = client;
-	chip->pdata = client->dev.platform_data;
+	pdata = client->dev.platform_data;
 
 	i2c_set_clientdata(client, chip);
 
@@ -235,13 +237,21 @@ static int __devinit max17042_probe(struct i2c_client *client,
 	chip->battery.properties	= max17042_battery_props;
 	chip->battery.num_properties	= ARRAY_SIZE(max17042_battery_props);
 
+	if (pdata) {
+		chip->r_sns = pdata->r_sns;
+		chip->enable_current_sense = pdata->enable_current_sense;
+	} else {
+		dev_warn(&client->dev, "no driver data provided\n");
+		return -ENODEV;
+	}
+
 	/* When current is not measured,
 	 * CURRENT_NOW and CURRENT_AVG properties should be invisible. */
-	if (!chip->pdata->enable_current_sense)
+	if (!chip->enable_current_sense)
 		chip->battery.num_properties -= 2;
 
-	if (chip->pdata->r_sns == 0)
-		chip->pdata->r_sns = MAX17042_DEFAULT_SNS_RESISTOR;
+	if (chip->r_sns == 0)
+		chip->r_sns = MAX17042_DEFAULT_SNS_RESISTOR;
 
 	ret = power_supply_register(&client->dev, &chip->battery);
 	if (ret) {
@@ -250,11 +260,11 @@ static int __devinit max17042_probe(struct i2c_client *client,
 	}
 
 	/* Initialize registers according to values from the platform data */
-	if (chip->pdata->init_data)
-		max17042_set_reg(client, chip->pdata->init_data,
-				 chip->pdata->num_init_data);
+	if (pdata && pdata->init_data)
+		max17042_set_reg(client, pdata->init_data,
+				 pdata->num_init_data);
 
-	if (!chip->pdata->enable_current_sense) {
+	if (!chip->enable_current_sense) {
 		max17042_write_reg(client, MAX17042_CGAIN, 0x0000);
 		max17042_write_reg(client, MAX17042_MiscCFG, 0x0003);
 		max17042_write_reg(client, MAX17042_LearnCFG, 0x0007);
-- 
1.7.8.3

Re: [PATCH 2/3] max17042_battery: Preserve properties outside of platform data

[Anton Vorontsov]

On Wed, Feb 22, 2012 at 07:06:21PM +0100, Karol Lewandowski wrote:
> Add fields originally found in platform data back to max17042_chip,
> as the former data structure might be not available on device
> tree-based systems.
> 
> This commit makes it possible to safely declare platform data with
> __initdata tag.
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/power/max17042_battery.c |   36 +++++++++++++++++++++++-------------
>  1 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> index 21a3650..49c1377 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -33,7 +33,8 @@
>  struct max17042_chip {
>  	struct i2c_client *client;
>  	struct power_supply battery;
> -	struct max17042_platform_data *pdata;
> +	bool enable_current_sense;
> +	u32 r_sns;

Dirk Brandewie implemented config_data in addition to these
fields, so this patch would have needed a rework. So I decided
to drop this particular patch and rework patch 3/3 to make OF
helper allocat pdata instead.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

Re: [PATCH 2/3] max17042_battery: Preserve properties outside of platform data

[Karol Lewandowski]

On 13.03.2012 20:21, Anton Vorontsov wrote:

> On Wed, Feb 22, 2012 at 07:06:21PM +0100, Karol Lewandowski wrote:
>> Add fields originally found in platform data back to max17042_chip,
>> as the former data structure might be not available on device
>> tree-based systems.
>>
>> This commit makes it possible to safely declare platform data with
>> __initdata tag.
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  drivers/power/max17042_battery.c |   36 +++++++++++++++++++++++-------------
>>  1 files changed, 23 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
>> index 21a3650..49c1377 100644
>> --- a/drivers/power/max17042_battery.c
>> +++ b/drivers/power/max17042_battery.c
>> @@ -33,7 +33,8 @@
>>  struct max17042_chip {
>>  	struct i2c_client *client;
>>  	struct power_supply battery;
>> -	struct max17042_platform_data *pdata;
>> +	bool enable_current_sense;
>> +	u32 r_sns;
> 
> Dirk Brandewie implemented config_data in addition to these
> fields, so this patch would have needed a rework. So I decided
> to drop this particular patch and rework patch 3/3 to make OF
> helper allocat pdata instead.


Actually, this is how I done it for first time.  However,
shortly after that I've seen some maintainers opposing use
of platform data at runtime.
This is why I've moved it to private data.

On the other hand I don't like duplicating same fields in private
and platform data.  Thus, I actually like reworked version
a bit more.

Thanks!
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

[PATCH 3/3] max17042_battery: Make it possible to instantiate driver from DT

[Karol Lewandowski]

Allow both device tree (preferred) and platform data-based driver
instantiation.

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../bindings/power_supply/max17042_battery.txt     |   18 ++++++++
 drivers/power/max17042_battery.c                   |   45 ++++++++++++++++++--
 2 files changed, 59 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power_supply/max17042_battery.txt

diff --git a/Documentation/devicetree/bindings/power_supply/max17042_battery.txt b/Documentation/devicetree/bindings/power_supply/max17042_battery.txt
new file mode 100644
index 0000000..5bc9b68
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/max17042_battery.txt
@@ -0,0 +1,18 @@
+max17042_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17042"
+
+Optional properties :
+ - maxim,rsns-microohm : Resistance of rsns resistor in micro Ohms
+                         (datasheet-recommended value is 10000).
+   Defining this property enables current-sense functionality.
+
+Example:
+
+	battery-charger@36 {
+		compatible = "maxim,max17042";
+		reg = <0x36>;
+		maxim,rsns-microohm = <10000>;
+	};
diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 49c1377..aec883b 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -29,6 +29,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/power_supply.h>
 #include <linux/power/max17042_battery.h>
+#include <linux/of.h>
 
 struct max17042_chip {
 	struct i2c_client *client;
@@ -211,6 +212,31 @@ static int max17042_get_property(struct power_supply *psy,
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static int max17042_dt_parse(struct max17042_chip *chip, struct device_node *np)
+{
+	u32 prop;
+
+	if (!np)
+		return -EINVAL;
+
+	/* require current sense resistor value to be specified for
+	   current-sense functionality to be enabled at all */
+	if (of_property_read_u32(np, "maxim,rsns-microohm", &prop) == 0) {
+		chip->r_sns = prop;
+		chip->enable_current_sense = true;
+	} else
+		chip->enable_current_sense = false;
+
+	return 0;
+}
+#else
+static int max17042_dt_parse(struct max17042_chip *chip, struct device_node *np)
+{
+	return -EINVAL;
+}
+#endif
+
 static int __devinit max17042_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -237,12 +263,14 @@ static int __devinit max17042_probe(struct i2c_client *client,
 	chip->battery.properties	= max17042_battery_props;
 	chip->battery.num_properties	= ARRAY_SIZE(max17042_battery_props);
 
-	if (pdata) {
+	ret = max17042_dt_parse(chip, client->dev.of_node);
+	if (ret < 0) {
+		if (!pdata) {
+			dev_warn(&client->dev, "no driver data provided\n");
+			return -ENODEV;
+		}
 		chip->r_sns = pdata->r_sns;
 		chip->enable_current_sense = pdata->enable_current_sense;
-	} else {
-		dev_warn(&client->dev, "no driver data provided\n");
-		return -ENODEV;
 	}
 
 	/* When current is not measured,
@@ -281,6 +309,14 @@ static int __devexit max17042_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id max17042_dt_match[] = {
+	{ .compatible = "maxim,max17042" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, max17042_dt_match);
+#endif
+
 static const struct i2c_device_id max17042_id[] = {
 	{ "max17042", 0 },
 	{ }
@@ -290,6 +326,7 @@ MODULE_DEVICE_TABLE(i2c, max17042_id);
 static struct i2c_driver max17042_i2c_driver = {
 	.driver	= {
 		.name	= "max17042",
+		.of_match_table = of_match_ptr(max17042_dt_match),
 	},
 	.probe		= max17042_probe,
 	.remove		= __devexit_p(max17042_remove),
-- 
1.7.8.3

Re: [PATCH 3/3] max17042_battery: Make it possible to instantiate driver from DT

[Anton Vorontsov]

On Wed, Feb 22, 2012 at 07:06:22PM +0100, Karol Lewandowski wrote:
> Allow both device tree (preferred) and platform data-based driver
> instantiation.
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---

Thanks for the patch!

I applied the following version:

commit 651bfc379546af756dc6abbc9ce678e4607220be
Author: Karol Lewandowski <k.lewandowsk@samsung.com>
Date:   Wed Feb 22 19:06:22 2012 +0100

    max17042_battery: Make it possible to instantiate driver from DT
    
    Allow both device tree (preferred) and platform data-based driver
    instantiation.
    
    Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

diff --git a/Documentation/devicetree/bindings/power_supply/max17042_battery.txt b/Documentation/devicetree/bindings/power_supply/max17042_battery.txt
new file mode 100644
index 0000000..5bc9b68
--- /dev/null
+++ b/Documentation/devicetree/bindings/power_supply/max17042_battery.txt
@@ -0,0 +1,18 @@
+max17042_battery
+~~~~~~~~~~~~~~~~
+
+Required properties :
+ - compatible : "maxim,max17042"
+
+Optional properties :
+ - maxim,rsns-microohm : Resistance of rsns resistor in micro Ohms
+                         (datasheet-recommended value is 10000).
+   Defining this property enables current-sense functionality.
+
+Example:
+
+	battery-charger@36 {
+		compatible = "maxim,max17042";
+		reg = <0x36>;
+		maxim,rsns-microohm = <10000>;
+	};
diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 872e9b4..e36763a 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -31,6 +31,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/power_supply.h>
 #include <linux/power/max17042_battery.h>
+#include <linux/of.h>
 
 /* Status register bits */
 #define STATUS_POR_BIT         (1 << 1)
@@ -608,6 +609,40 @@ static void max17042_init_worker(struct work_struct *work)
 	chip->init_complete = 1;
 }
 
+#ifdef CONFIG_OF
+static struct max17042_platform_data *
+max17042_get_pdata(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	u32 prop;
+	struct max17042_platform_data *pdata;
+
+	if (!np)
+		return dev->platform_data;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return NULL;
+
+	/*
+	 * Require current sense resistor value to be specified for
+	 * current-sense functionality to be enabled at all.
+	 */
+	if (of_property_read_u32(np, "maxim,rsns-microohm", &prop) == 0) {
+		pdata->r_sns = prop;
+		pdata->enable_current_sense = true;
+	}
+
+	return pdata;
+}
+#else
+static struct max17042_platform_data *
+max17042_get_pdata(struct device *dev)
+{
+	return dev->platform_data;
+}
+#endif
+
 static int __devinit max17042_probe(struct i2c_client *client,
 			const struct i2c_device_id *id)
 {
@@ -624,7 +659,11 @@ static int __devinit max17042_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	chip->client = client;
-	chip->pdata = client->dev.platform_data;
+	chip->pdata = max17042_get_pdata(&client->dev);
+	if (!chip->pdata) {
+		dev_err(&client->dev, "no platform data provided\n");
+		return -EINVAL;
+	}
 
 	i2c_set_clientdata(client, chip);
 
@@ -689,6 +728,14 @@ static int __devexit max17042_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id max17042_dt_match[] = {
+	{ .compatible = "maxim,max17042" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, max17042_dt_match);
+#endif
+
 static const struct i2c_device_id max17042_id[] = {
 	{ "max17042", 0 },
 	{ }
@@ -698,6 +745,7 @@ MODULE_DEVICE_TABLE(i2c, max17042_id);
 static struct i2c_driver max17042_i2c_driver = {
 	.driver	= {
 		.name	= "max17042",
+		.of_match_table = of_match_ptr(max17042_dt_match),
 	},
 	.probe		= max17042_probe,
 	.remove		= __devexit_p(max17042_remove),

Re: [PATCH 3/3] max17042_battery: Make it possible to instantiate driver from DT

[Karol Lewandowski]

On 13.03.2012 20:22, Anton Vorontsov wrote:

> On Wed, Feb 22, 2012 at 07:06:22PM +0100, Karol Lewandowski wrote:
>> Allow both device tree (preferred) and platform data-based driver
>> instantiation.
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> ---
> 
> Thanks for the patch!
> 
> I applied the following version:

[..]

I've verified that these patches[1] work correctly on both DT and
non-DT systems (samsung's nuri board).

[1] Taken directly from your git tree.

Thanks!
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform