[PATCH 1/7] mfd: pcf50633 disable unnecessary shutdown on lowsys

[PATCH 1/7] mfd: pcf50633 disable unnecessary shutdown on lowsys

[Paul Fertser]

On gta02 hardware revision A5 it can actually bring the system down
during normal operating conditions so we disable it.

Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index d26d774..6efe5c3 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -345,6 +345,9 @@ static void pcf50633_irq_worker(struct work_struct *work)
 		goto out;
 	}
 
+	/* defeat 8s death from lowsys on A5 */
+	pcf50633_reg_write(pcf, PCF50633_REG_OOCSHDWN,  0x04);
+
 	/* We immediately read the usb and adapter status. We thus make sure
 	 * only of USBINS/USBREM IRQ handlers are called */
 	if (pcf_int[0] & (PCF50633_INT1_USBINS | PCF50633_INT1_USBREM)) {
-- 
1.6.0.6

[PATCH 2/7] mfd: pcf50633: make suspend/resume belong to i2c_driver

[Paul Fertser]

From: Lars-Peter Clausen <lars@metafoo.de>

When not using the i2c suspend/resume callbacks the i2c client resumed
before the i2c master.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 6efe5c3..a844445 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -485,13 +485,13 @@ pcf50633_client_dev_register(struct pcf50633 *pcf, const char *name,
 }
 
 #ifdef CONFIG_PM
-static int pcf50633_suspend(struct device *dev, pm_message_t state)
+static int pcf50633_suspend(struct i2c_client *client, pm_message_t state)
 {
 	struct pcf50633 *pcf;
 	int ret = 0, i;
 	u8 res[5];
 
-	pcf = dev_get_drvdata(dev);
+	pcf = i2c_get_clientdata(client);
 
 	/* Make sure our interrupt handlers are not called
 	 * henceforth */
@@ -526,12 +526,12 @@ out:
 	return ret;
 }
 
-static int pcf50633_resume(struct device *dev)
+static int pcf50633_resume(struct i2c_client *client)
 {
 	struct pcf50633 *pcf;
 	int ret;
 
-	pcf = dev_get_drvdata(dev);
+	pcf = i2c_get_clientdata(client);
 
 	/* Write the saved mask registers */
 	ret = pcf50633_write_block(pcf, PCF50633_REG_INT1M,
@@ -689,12 +689,12 @@ static struct i2c_device_id pcf50633_id_table[] = {
 static struct i2c_driver pcf50633_driver = {
 	.driver = {
 		.name	= "pcf50633",
-		.suspend = pcf50633_suspend,
-		.resume	= pcf50633_resume,
 	},
 	.id_table = pcf50633_id_table,
 	.probe = pcf50633_probe,
 	.remove = __devexit_p(pcf50633_remove),
+	.suspend = pcf50633_suspend,
+	.resume	= pcf50633_resume,
 };
 
 static int __init pcf50633_init(void)
-- 
1.6.0.6

[PATCH 3/7] mfd: pcf50633: move messages to appropriate log levels

[Paul Fertser]

From: Arnaud Patard <arnaud.patard@rtp-net.org>

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index a844445..48776d3 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -290,7 +290,7 @@ out:
 
 int pcf50633_irq_mask(struct pcf50633 *pcf, int irq)
 {
-	dev_info(pcf->dev, "Masking IRQ %d\n", irq);
+	dev_dbg(pcf->dev, "Masking IRQ %d\n", irq);
 
 	return __pcf50633_irq_mask_set(pcf, irq, 1);
 }
@@ -298,7 +298,7 @@ EXPORT_SYMBOL_GPL(pcf50633_irq_mask);
 
 int pcf50633_irq_unmask(struct pcf50633 *pcf, int irq)
 {
-	dev_info(pcf->dev, "Unmasking IRQ %d\n", irq);
+	dev_dbg(pcf->dev, "Unmasking IRQ %d\n", irq);
 
 	return __pcf50633_irq_mask_set(pcf, irq, 0);
 }
-- 
1.6.0.6

[PATCH 4/7] mfd: pcf50633: Fix memleak in pcf50633_client_dev_register

[Paul Fertser]

From: Lars-Peter Clausen <lars@metafoo.de>

Since platform_device_add_data copies the passed data, the allocated
subdev_pdata is never freed. A simple fix would be to either free subdev_pdata
or put it onto the stack. But since the pcf50633 child devices can rely on
beeing children of the pcf50633 core device it's much more elegant to get access
to pcf50633 core structure through that link. This allows to get completly rid
of pcf5033_subdev_pdata.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/input/misc/pcf50633-input.c |    7 +++----
 drivers/mfd/pcf50633-adc.c          |    5 ++---
 drivers/mfd/pcf50633-core.c         |   10 ----------
 drivers/power/pcf50633-charger.c    |    3 +--
 drivers/rtc/rtc-pcf50633.c          |    5 +----
 include/linux/mfd/pcf50633/core.h   |   10 +++++-----
 6 files changed, 12 insertions(+), 28 deletions(-)

diff --git a/drivers/input/misc/pcf50633-input.c b/drivers/input/misc/pcf50633-input.c
index 039dcb0..008de0c 100644
--- a/drivers/input/misc/pcf50633-input.c
+++ b/drivers/input/misc/pcf50633-input.c
@@ -55,7 +55,6 @@ pcf50633_input_irq(int irq, void *data)
 static int __devinit pcf50633_input_probe(struct platform_device *pdev)
 {
 	struct pcf50633_input *input;
-	struct pcf50633_subdev_pdata *pdata = pdev->dev.platform_data;
 	struct input_dev *input_dev;
 	int ret;
 
@@ -71,7 +70,7 @@ static int __devinit pcf50633_input_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, input);
-	input->pcf = pdata->pcf;
+	input->pcf = dev_to_pcf50633(pdev->dev.parent);
 	input->input_dev = input_dev;
 
 	input_dev->name = "PCF50633 PMU events";
@@ -85,9 +84,9 @@ static int __devinit pcf50633_input_probe(struct platform_device *pdev)
 		kfree(input);
 		return ret;
 	}
-	pcf50633_register_irq(pdata->pcf, PCF50633_IRQ_ONKEYR,
+	pcf50633_register_irq(input->pcf, PCF50633_IRQ_ONKEYR,
 				pcf50633_input_irq, input);
-	pcf50633_register_irq(pdata->pcf, PCF50633_IRQ_ONKEYF,
+	pcf50633_register_irq(input->pcf, PCF50633_IRQ_ONKEYF,
 				pcf50633_input_irq, input);
 
 	return 0;
diff --git a/drivers/mfd/pcf50633-adc.c b/drivers/mfd/pcf50633-adc.c
index 3d31e97..6d2e846 100644
--- a/drivers/mfd/pcf50633-adc.c
+++ b/drivers/mfd/pcf50633-adc.c
@@ -209,17 +209,16 @@ static void pcf50633_adc_irq(int irq, void *data)
 
 static int __devinit pcf50633_adc_probe(struct platform_device *pdev)
 {
-	struct pcf50633_subdev_pdata *pdata = pdev->dev.platform_data;
 	struct pcf50633_adc *adc;
 
 	adc = kzalloc(sizeof(*adc), GFP_KERNEL);
 	if (!adc)
 		return -ENOMEM;
 
-	adc->pcf = pdata->pcf;
+	adc->pcf = dev_to_pcf50633(pdev->dev.parent);
 	platform_set_drvdata(pdev, adc);
 
-	pcf50633_register_irq(pdata->pcf, PCF50633_IRQ_ADCRDY,
+	pcf50633_register_irq(adc->pcf, PCF50633_IRQ_ADCRDY,
 					pcf50633_adc_irq, adc);
 
 	mutex_init(&adc->queue_mutex);
diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 48776d3..69cdbdc 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -456,7 +456,6 @@ static void
 pcf50633_client_dev_register(struct pcf50633 *pcf, const char *name,
 						struct platform_device **pdev)
 {
-	struct pcf50633_subdev_pdata *subdev_pdata;
 	int ret;
 
 	*pdev = platform_device_alloc(name, -1);
@@ -465,15 +464,6 @@ pcf50633_client_dev_register(struct pcf50633 *pcf, const char *name,
 		return;
 	}
 
-	subdev_pdata = kmalloc(sizeof(*subdev_pdata), GFP_KERNEL);
-	if (!subdev_pdata) {
-		dev_err(pcf->dev, "Error allocating subdev pdata\n");
-		platform_device_put(*pdev);
-	}
-
-	subdev_pdata->pcf = pcf;
-	platform_device_add_data(*pdev, subdev_pdata, sizeof(*subdev_pdata));
-
 	(*pdev)->dev.parent = pcf->dev;
 
 	ret = platform_device_add(*pdev);
diff --git a/drivers/power/pcf50633-charger.c b/drivers/power/pcf50633-charger.c
index e8b278f..6a84a8e 100644
--- a/drivers/power/pcf50633-charger.c
+++ b/drivers/power/pcf50633-charger.c
@@ -303,7 +303,6 @@ static const u8 mbc_irq_handlers[] = {
 static int __devinit pcf50633_mbc_probe(struct platform_device *pdev)
 {
 	struct pcf50633_mbc *mbc;
-	struct pcf50633_subdev_pdata *pdata = pdev->dev.platform_data;
 	int ret;
 	int i;
 	u8 mbcs1;
@@ -313,7 +312,7 @@ static int __devinit pcf50633_mbc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, mbc);
-	mbc->pcf = pdata->pcf;
+	mbc->pcf = dev_to_pcf50633(pdev->dev.parent);
 
 	/* Set up IRQ handlers */
 	for (i = 0; i < ARRAY_SIZE(mbc_irq_handlers); i++)
diff --git a/drivers/rtc/rtc-pcf50633.c b/drivers/rtc/rtc-pcf50633.c
index f4dd87e..0f045d7 100644
--- a/drivers/rtc/rtc-pcf50633.c
+++ b/drivers/rtc/rtc-pcf50633.c
@@ -276,16 +276,13 @@ static void pcf50633_rtc_irq(int irq, void *data)
 
 static int __devinit pcf50633_rtc_probe(struct platform_device *pdev)
 {
-	struct pcf50633_subdev_pdata *pdata;
 	struct pcf50633_rtc *rtc;
 
-
 	rtc = kzalloc(sizeof(*rtc), GFP_KERNEL);
 	if (!rtc)
 		return -ENOMEM;
 
-	pdata = pdev->dev.platform_data;
-	rtc->pcf = pdata->pcf;
+	rtc->pcf = dev_to_pcf50633(pdev->dev.parent);
 	platform_set_drvdata(pdev, rtc);
 	rtc->rtc_dev = rtc_device_register("pcf50633-rtc", &pdev->dev,
 				&pcf50633_rtc_ops, THIS_MODULE);
diff --git a/include/linux/mfd/pcf50633/core.h b/include/linux/mfd/pcf50633/core.h
index 9aba7b7..d9034cc 100644
--- a/include/linux/mfd/pcf50633/core.h
+++ b/include/linux/mfd/pcf50633/core.h
@@ -40,10 +40,6 @@ struct pcf50633_platform_data {
 	u8 resumers[5];
 };
 
-struct pcf50633_subdev_pdata {
-	struct pcf50633 *pcf;
-};
-
 struct pcf50633_irq {
 	void (*handler) (int, void *);
 	void *data;
@@ -217,5 +213,9 @@ enum pcf50633_reg_int5 {
 #define PCF50633_REG_LEDCTL 0x2a
 #define PCF50633_REG_LEDDIM 0x2b
 
-#endif
+static inline struct pcf50633 *dev_to_pcf50633(struct device *dev)
+{
+	return dev_get_drvdata(dev);
+}
 
+#endif
-- 
1.6.0.6

[PATCH 5/7] mfd: pcf50633: Cleanup pcf50633_probe error handling

[Paul Fertser]

From: Lars-Peter Clausen <lars@metafoo.de>

Currently the child devices were not freed if the irq could not be requested.
This patch restructures the function, that in case of an error all previously
allocated resources are freed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |   47 +++++++++++++++++++++++++-----------------
 1 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 69cdbdc..2e7f903 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -553,9 +553,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 {
 	struct pcf50633 *pcf;
 	struct pcf50633_platform_data *pdata = client->dev.platform_data;
-	int i, ret = 0;
+	int i, ret;
 	int version, variant;
 
+	if (!client->irq) {
+		dev_err(&client->dev, "Missing IRQ\n");
+		return -ENOENT;
+	}
+
 	pcf = kzalloc(sizeof(*pcf), GFP_KERNEL);
 	if (!pcf)
 		return -ENOMEM;
@@ -570,6 +575,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	pcf->irq = client->irq;
 	pcf->work_queue = create_singlethread_workqueue("pcf50633");
 
+	if (!pcf->work_queue) {
+		dev_err(&client->dev, "Failed to alloc workqueue\n");
+		ret = -ENOMEM;
+		goto err_free;
+	}
+
 	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
 
 	version = pcf50633_reg_read(pcf, 0);
@@ -577,7 +588,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	if (version < 0 || variant < 0) {
 		dev_err(pcf->dev, "Unable to probe pcf50633\n");
 		ret = -ENODEV;
-		goto err;
+		goto err_destroy_workqueue;
 	}
 
 	dev_info(pcf->dev, "Probed device version %d variant %d\n",
@@ -591,6 +602,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	pcf50633_reg_write(pcf, PCF50633_REG_INT4M, 0x00);
 	pcf50633_reg_write(pcf, PCF50633_REG_INT5M, 0x00);
 
+	ret = request_irq(client->irq, pcf50633_irq,
+					IRQF_TRIGGER_LOW, "pcf50633", pcf);
+
+	if (ret) {
+		dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
+		goto err_destroy_workqueue;
+	}
+
 	/* Create sub devices */
 	pcf50633_client_dev_register(pcf, "pcf50633-input",
 						&pcf->input_pdev);
@@ -606,7 +625,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 
 		pdev = platform_device_alloc("pcf50633-regltr", i);
 		if (!pdev) {
-			dev_err(pcf->dev, "Cannot create regulator\n");
+			dev_err(pcf->dev, "Cannot create regulator %d\n", i);
 			continue;
 		}
 
@@ -618,35 +637,25 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 		platform_device_add(pdev);
 	}
 
-	if (client->irq) {
-		ret = request_irq(client->irq, pcf50633_irq,
-				IRQF_TRIGGER_LOW, "pcf50633", pcf);
-
-		if (ret) {
-			dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
-			goto err;
-		}
-	} else {
-		dev_err(pcf->dev, "No IRQ configured\n");
-		goto err;
-	}
-
 	if (enable_irq_wake(client->irq) < 0)
-		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
+		dev_info(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
 			"in this hardware revision", client->irq);
 
 	ret = sysfs_create_group(&client->dev.kobj, &pcf_attr_group);
 	if (ret)
-		dev_err(pcf->dev, "error creating sysfs entries\n");
+		dev_info(pcf->dev, "Failed to create sysfs entries\n");
 
 	if (pdata->probe_done)
 		pdata->probe_done(pcf);
 
 	return 0;
 
-err:
+err_destroy_workqueue:
 	destroy_workqueue(pcf->work_queue);
+err_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf);
+
 	return ret;
 }
 
-- 
1.6.0.6

[PATCH 6/7] mfd: pcf50633: Use platform_device_add_data to set regulator platform data

[Paul Fertser]

From: Lars-Peter Clausen <lars@metafoo.de>

Platform devices allocated with platform_device_alloc should use
platform_device_add_data to set the platform data, because kfree will be called
on the platform_data when the device is released.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 2e7f903..4604ba5 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -630,7 +630,8 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 		}
 
 		pdev->dev.parent = pcf->dev;
-		pdev->dev.platform_data = &pdata->reg_init_data[i];
+		platform_device_add_data(pdev, &pdata->reg_init_data[i],
+					sizeof(pdata->reg_init_data[i]));
 		dev_set_drvdata(&pdev->dev, pcf);
 		pcf->regulator_pdev[i] = pdev;
 
-- 
1.6.0.6

[PATCH 7/7] mfd: pcf50633: Fix pcf50633-regulator drvdata usage

[Paul Fertser]

From: Lars-Peter Clausen <lars@metafoo.de>

Currently the pcf50633-regulator driver data is set to the pcf50633 core
structure, but the pcf50633-regulator remove handler assumes that it is set to
the regulator device. This patch fixes the issue by accessing the pcf506533
core structure through its parent device and setting the driver data to the
regulator device.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c            |    1 -
 drivers/regulator/pcf50633-regulator.c |    5 ++++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 4604ba5..a86b0c6 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -632,7 +632,6 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 		pdev->dev.parent = pcf->dev;
 		platform_device_add_data(pdev, &pdata->reg_init_data[i],
 					sizeof(pdata->reg_init_data[i]));
-		dev_set_drvdata(&pdev->dev, pcf);
 		pcf->regulator_pdev[i] = pdev;
 
 		platform_device_add(pdev);
diff --git a/drivers/regulator/pcf50633-regulator.c b/drivers/regulator/pcf50633-regulator.c
index 0803ffe..c8f41dc 100644
--- a/drivers/regulator/pcf50633-regulator.c
+++ b/drivers/regulator/pcf50633-regulator.c
@@ -314,13 +314,15 @@ static int __devinit pcf50633_regulator_probe(struct platform_device *pdev)
 	struct pcf50633 *pcf;
 
 	/* Already set by core driver */
-	pcf = platform_get_drvdata(pdev);
+	pcf = dev_to_pcf50633(pdev->dev.parent);
 
 	rdev = regulator_register(&regulators[pdev->id], &pdev->dev,
 				  pdev->dev.platform_data, pcf);
 	if (IS_ERR(rdev))
 		return PTR_ERR(rdev);
 
+	platform_set_drvdata(pdev, rdev);
+
 	if (pcf->pdata->regulator_registered)
 		pcf->pdata->regulator_registered(pcf, pdev->id);
 
@@ -331,6 +333,7 @@ static int __devexit pcf50633_regulator_remove(struct platform_device *pdev)
 {
 	struct regulator_dev *rdev = platform_get_drvdata(pdev);
 
+	platform_set_drvdata(pdev, NULL);
 	regulator_unregister(rdev);
 
 	return 0;
-- 
1.6.0.6

Re: [PATCH 5/7] mfd: pcf50633: Cleanup pcf50633_probe error handling

[Samuel Ortiz]

Hi Paul,

On Wed, Oct 14, 2009 at 02:12:34AM +0400, Paul Fertser wrote:
> From: Lars-Peter Clausen <lars@metafoo.de>
> 
> Currently the child devices were not freed if the irq could not be requested.
> This patch restructures the function, that in case of an error all previously
> allocated resources are freed.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
>  drivers/mfd/pcf50633-core.c |   47 +++++++++++++++++++++++++-----------------
>  1 files changed, 28 insertions(+), 19 deletions(-)
[...]
>  	if (enable_irq_wake(client->irq) < 0)
> -		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> +		dev_info(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
>  			"in this hardware revision", client->irq);
2 things here: This doesnt really belong to this patch, and also I'd prefer to
keep that as an error message.


>  	ret = sysfs_create_group(&client->dev.kobj, &pcf_attr_group);
>  	if (ret)
> -		dev_err(pcf->dev, "error creating sysfs entries\n");
> +		dev_info(pcf->dev, "Failed to create sysfs entries\n");
Same here.

Otherwise, this patch looks good to me.



-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 5/7] mfd: pcf50633: Cleanup pcf50633_probe error handling

[Paul Fertser]

[-- Attachment #1: Type: text/plain, Size: 999 bytes --]

Hi Samuel,

Big thanks for your comments, next time i send anything upstream i
will certainly provide a minimal changelog for every patch :)

On Mon, Oct 19, 2009 at 05:08:13PM +0200, Samuel Ortiz wrote:
> On Wed, Oct 14, 2009 at 02:12:34AM +0400, Paul Fertser wrote:
> >  	if (enable_irq_wake(client->irq) < 0)
> > -		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> > +		dev_info(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> >  			"in this hardware revision", client->irq);
> 2 things here: This doesnt really belong to this patch, and also I'd prefer to
> keep that as an error message.
[...]
> >  	ret = sysfs_create_group(&client->dev.kobj, &pcf_attr_group);
> >  	if (ret)
> > -		dev_err(pcf->dev, "error creating sysfs entries\n");
> > +		dev_info(pcf->dev, "Failed to create sysfs entries\n");
> Same here.

Sure. Please find amended version in attachement.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

[-- Attachment #2: 0005-mfd-pcf50633-Cleanup-pcf50633_probe-error-handling.patch --]
[-- Type: text/plain, Size: 3510 bytes --]

>From 7f982d01515371fcbab0086bf77448f351b09a42 Mon Sep 17 00:00:00 2001
From: Lars-Peter Clausen <lars@metafoo.de>
Date: Thu, 8 Oct 2009 00:24:54 +0200
Subject: [PATCH] mfd: pcf50633: Cleanup pcf50633_probe error handling

Currently the child devices were not freed if the irq could not be requested.
This patch restructures the function, that in case of an error all previously
allocated resources are freed.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 drivers/mfd/pcf50633-core.c |   43 ++++++++++++++++++++++++++-----------------
 1 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
index 69cdbdc..5aed527 100644
--- a/drivers/mfd/pcf50633-core.c
+++ b/drivers/mfd/pcf50633-core.c
@@ -553,9 +553,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 {
 	struct pcf50633 *pcf;
 	struct pcf50633_platform_data *pdata = client->dev.platform_data;
-	int i, ret = 0;
+	int i, ret;
 	int version, variant;
 
+	if (!client->irq) {
+		dev_err(&client->dev, "Missing IRQ\n");
+		return -ENOENT;
+	}
+
 	pcf = kzalloc(sizeof(*pcf), GFP_KERNEL);
 	if (!pcf)
 		return -ENOMEM;
@@ -570,6 +575,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	pcf->irq = client->irq;
 	pcf->work_queue = create_singlethread_workqueue("pcf50633");
 
+	if (!pcf->work_queue) {
+		dev_err(&client->dev, "Failed to alloc workqueue\n");
+		ret = -ENOMEM;
+		goto err_free;
+	}
+
 	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
 
 	version = pcf50633_reg_read(pcf, 0);
@@ -577,7 +588,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	if (version < 0 || variant < 0) {
 		dev_err(pcf->dev, "Unable to probe pcf50633\n");
 		ret = -ENODEV;
-		goto err;
+		goto err_destroy_workqueue;
 	}
 
 	dev_info(pcf->dev, "Probed device version %d variant %d\n",
@@ -591,6 +602,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 	pcf50633_reg_write(pcf, PCF50633_REG_INT4M, 0x00);
 	pcf50633_reg_write(pcf, PCF50633_REG_INT5M, 0x00);
 
+	ret = request_irq(client->irq, pcf50633_irq,
+					IRQF_TRIGGER_LOW, "pcf50633", pcf);
+
+	if (ret) {
+		dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
+		goto err_destroy_workqueue;
+	}
+
 	/* Create sub devices */
 	pcf50633_client_dev_register(pcf, "pcf50633-input",
 						&pcf->input_pdev);
@@ -606,7 +625,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 
 		pdev = platform_device_alloc("pcf50633-regltr", i);
 		if (!pdev) {
-			dev_err(pcf->dev, "Cannot create regulator\n");
+			dev_err(pcf->dev, "Cannot create regulator %d\n", i);
 			continue;
 		}
 
@@ -618,19 +637,6 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 		platform_device_add(pdev);
 	}
 
-	if (client->irq) {
-		ret = request_irq(client->irq, pcf50633_irq,
-				IRQF_TRIGGER_LOW, "pcf50633", pcf);
-
-		if (ret) {
-			dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
-			goto err;
-		}
-	} else {
-		dev_err(pcf->dev, "No IRQ configured\n");
-		goto err;
-	}
-
 	if (enable_irq_wake(client->irq) < 0)
 		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
 			"in this hardware revision", client->irq);
@@ -644,9 +650,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
 
 	return 0;
 
-err:
+err_destroy_workqueue:
 	destroy_workqueue(pcf->work_queue);
+err_free:
+	i2c_set_clientdata(client, NULL);
 	kfree(pcf);
+
 	return ret;
 }
 
-- 
1.6.0.6

Re: [PATCH 5/7] mfd: pcf50633: Cleanup pcf50633_probe error handling

[Samuel Ortiz]

On Tue, Oct 20, 2009 at 03:09:52AM +0400, Paul Fertser wrote:
> Hi Samuel,
> 
> Big thanks for your comments, next time i send anything upstream i
> will certainly provide a minimal changelog for every patch :)
> 
> On Mon, Oct 19, 2009 at 05:08:13PM +0200, Samuel Ortiz wrote:
> > On Wed, Oct 14, 2009 at 02:12:34AM +0400, Paul Fertser wrote:
> > >  	if (enable_irq_wake(client->irq) < 0)
> > > -		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> > > +		dev_info(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> > >  			"in this hardware revision", client->irq);
> > 2 things here: This doesnt really belong to this patch, and also I'd prefer to
> > keep that as an error message.
> [...]
> > >  	ret = sysfs_create_group(&client->dev.kobj, &pcf_attr_group);
> > >  	if (ret)
> > > -		dev_err(pcf->dev, "error creating sysfs entries\n");
> > > +		dev_info(pcf->dev, "Failed to create sysfs entries\n");
> > Same here.
> 
> Sure. Please find amended version in attachement.
Patch applied, thanks a lot.

Cheers,
Samuel.
 
> -- 
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com

> From 7f982d01515371fcbab0086bf77448f351b09a42 Mon Sep 17 00:00:00 2001
> From: Lars-Peter Clausen <lars@metafoo.de>
> Date: Thu, 8 Oct 2009 00:24:54 +0200
> Subject: [PATCH] mfd: pcf50633: Cleanup pcf50633_probe error handling
> 
> Currently the child devices were not freed if the irq could not be requested.
> This patch restructures the function, that in case of an error all previously
> allocated resources are freed.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
>  drivers/mfd/pcf50633-core.c |   43 ++++++++++++++++++++++++++-----------------
>  1 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index 69cdbdc..5aed527 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -553,9 +553,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  {
>  	struct pcf50633 *pcf;
>  	struct pcf50633_platform_data *pdata = client->dev.platform_data;
> -	int i, ret = 0;
> +	int i, ret;
>  	int version, variant;
>  
> +	if (!client->irq) {
> +		dev_err(&client->dev, "Missing IRQ\n");
> +		return -ENOENT;
> +	}
> +
>  	pcf = kzalloc(sizeof(*pcf), GFP_KERNEL);
>  	if (!pcf)
>  		return -ENOMEM;
> @@ -570,6 +575,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	pcf->irq = client->irq;
>  	pcf->work_queue = create_singlethread_workqueue("pcf50633");
>  
> +	if (!pcf->work_queue) {
> +		dev_err(&client->dev, "Failed to alloc workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_free;
> +	}
> +
>  	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
>  
>  	version = pcf50633_reg_read(pcf, 0);
> @@ -577,7 +588,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	if (version < 0 || variant < 0) {
>  		dev_err(pcf->dev, "Unable to probe pcf50633\n");
>  		ret = -ENODEV;
> -		goto err;
> +		goto err_destroy_workqueue;
>  	}
>  
>  	dev_info(pcf->dev, "Probed device version %d variant %d\n",
> @@ -591,6 +602,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	pcf50633_reg_write(pcf, PCF50633_REG_INT4M, 0x00);
>  	pcf50633_reg_write(pcf, PCF50633_REG_INT5M, 0x00);
>  
> +	ret = request_irq(client->irq, pcf50633_irq,
> +					IRQF_TRIGGER_LOW, "pcf50633", pcf);
> +
> +	if (ret) {
> +		dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
> +		goto err_destroy_workqueue;
> +	}
> +
>  	/* Create sub devices */
>  	pcf50633_client_dev_register(pcf, "pcf50633-input",
>  						&pcf->input_pdev);
> @@ -606,7 +625,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  
>  		pdev = platform_device_alloc("pcf50633-regltr", i);
>  		if (!pdev) {
> -			dev_err(pcf->dev, "Cannot create regulator\n");
> +			dev_err(pcf->dev, "Cannot create regulator %d\n", i);
>  			continue;
>  		}
>  
> @@ -618,19 +637,6 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  		platform_device_add(pdev);
>  	}
>  
> -	if (client->irq) {
> -		ret = request_irq(client->irq, pcf50633_irq,
> -				IRQF_TRIGGER_LOW, "pcf50633", pcf);
> -
> -		if (ret) {
> -			dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
> -			goto err;
> -		}
> -	} else {
> -		dev_err(pcf->dev, "No IRQ configured\n");
> -		goto err;
> -	}
> -
>  	if (enable_irq_wake(client->irq) < 0)
>  		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
>  			"in this hardware revision", client->irq);
> @@ -644,9 +650,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> -err:
> +err_destroy_workqueue:
>  	destroy_workqueue(pcf->work_queue);
> +err_free:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(pcf);
> +
>  	return ret;
>  }
>  
> -- 
> 1.6.0.6
> 


-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 1/7] mfd: pcf50633 disable unnecessary shutdown on lowsys

[Samuel Ortiz]

Hi Paul,

I applied all patches except patch 5, on which I commented about.
Also, patch 3 didnt have any changelog execpt the SOB lines. I expect to get
something even for such trivial patches.

Thanks for your work.

Cheers,
Samuel.

On Wed, Oct 14, 2009 at 02:12:30AM +0400, Paul Fertser wrote:
> On gta02 hardware revision A5 it can actually bring the system down
> during normal operating conditions so we disable it.
> 
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
>  drivers/mfd/pcf50633-core.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index d26d774..6efe5c3 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -345,6 +345,9 @@ static void pcf50633_irq_worker(struct work_struct *work)
>  		goto out;
>  	}
>  
> +	/* defeat 8s death from lowsys on A5 */
> +	pcf50633_reg_write(pcf, PCF50633_REG_OOCSHDWN,  0x04);
> +
>  	/* We immediately read the usb and adapter status. We thus make sure
>  	 * only of USBINS/USBREM IRQ handlers are called */
>  	if (pcf_int[0] & (PCF50633_INT1_USBINS | PCF50633_INT1_USBREM)) {
> -- 
> 1.6.0.6
> 

-- 
Intel Open Source Technology Centre
http://oss.intel.com/