public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3 0/8] Resource-managed extcon device register function
@ 2014-04-18  0:32 Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 1/8] extcon: Add resource-managed extcon " Sangjung Woo
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

These patches add resource-managed extcon device register functions for
developers' convenience and apply them to related device driver files.
This work can make the code more tidy since extcon device is automatically
unregistered on driver detach so tiresome managing codes could be removed.


Changelog:

v3:
* send the right version instead of previous v1
* add the credit for reviewers according to the review rules

v2:
* modify and clean up all unnecessary code reported by Chanwoo
* fix the bug reported by Seung-Woo
* add the credit for reviewers

v1:
* initial version

Sangjung Woo (8):
  extcon: Add resource-managed extcon register function
  extcon: adc-jack: Use devm_extcon_dev_register()
  extcon: gpio: Use devm_extcon_dev_register()
  extcon: max14577: Use devm_extcon_dev_register()
  extcon: max77693: Use devm_extcon_dev_register()
  extcon: max8997: Use devm_extcon_dev_register()
  extcon: palmas: Use devm_extcon_dev_register()
  extcon: arizona: Use devm_extcon_dev_register()

 drivers/extcon/extcon-adc-jack.c |   30 +++++-----------
 drivers/extcon/extcon-arizona.c  |   12 +++----
 drivers/extcon/extcon-class.c    |   72 ++++++++++++++++++++++++++++++++++++++
 drivers/extcon/extcon-gpio.c     |   16 +++------
 drivers/extcon/extcon-max14577.c |    9 ++---
 drivers/extcon/extcon-max77693.c |    7 ++--
 drivers/extcon/extcon-max8997.c  |    4 +--
 drivers/extcon/extcon-palmas.c   |   15 +++-----
 include/linux/extcon.h           |   17 +++++++++
 9 files changed, 116 insertions(+), 66 deletions(-)

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCHv3 1/8] extcon: Add resource-managed extcon register function
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-19  7:13   ` Chanwoo Choi
  2014-04-18  0:32 ` [PATCHv3 2/8] extcon: adc-jack: Use devm_extcon_dev_register() Sangjung Woo
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Add resource-managed extcon device register function for convenience.
For example, if a extcon device is attached with new
devm_extcon_dev_register(), that extcon device is automatically
unregistered on driver detach.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/extcon/extcon-class.c |   72 +++++++++++++++++++++++++++++++++++++++++
 include/linux/extcon.h        |   17 ++++++++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 7ab21aa..e177edb6 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
 }
 EXPORT_SYMBOL_GPL(extcon_dev_unregister);
 
+
+/*
+ * Device resource management
+ */
+static void devm_extcon_dev_release(struct device *dev, void *res)
+{
+	struct extcon_dev *devres = res;
+
+	extcon_dev_unregister(devres);
+}
+
+static int devm_extcon_dev_match(struct device *dev, void *res, void *data)
+{
+	struct extcon_dev *devres = res;
+	struct extcon_dev *match = data;
+	return devres == match;
+}
+
+/**
+ * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
+ * @dev:	device to allocate extcon device
+ * @edev:	the new extcon device to register
+ *
+ * Managed extcon_dev_register() function. If extcon device is attached with
+ * this function, that extcon device is automatically unregistered on driver
+ * detach. Internally this function calls extcon_dev_register() function.
+ * To get more information, refer that function.
+ *
+ * If extcon device is registered with this function and the device needs to be
+ * unregistered separately, devm_extcon_dev_unregister() should be used.
+ *
+ * RETURNS:
+ * 0 on success, negative error number on failure.
+ */
+int devm_extcon_dev_register(struct device *dev, struct extcon_dev *edev)
+{
+	struct extcon_dev *devres;
+	int ret;
+
+	devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres),
+			GFP_KERNEL);
+	if (!devres)
+		return -ENOMEM;
+
+	ret = extcon_dev_register(edev);
+	if (ret) {
+		devres_free(devres);
+		return ret;
+	}
+
+	devres = edev;
+	devres_add(dev, devres);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
+
+/**
+ * devm_extcon_dev_unregister() - Resource-managed extcon_dev_unregister()
+ * @dev:	device the extcon belongs to
+ * @edev:	the extcon device to unregister
+ *
+ * Unregister extcon device that is registered with devm_extcon_dev_register()
+ * function.
+ */
+void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev *edev)
+{
+	WARN_ON(devres_release(dev, devm_extcon_dev_release,
+			devm_extcon_dev_match, edev));
+}
+EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
+
 #ifdef CONFIG_OF
 /*
  * extcon_get_edev_by_phandle - Get the extcon device from devicetree
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index f488145..35f3343 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev *edev);
 extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
 
 /*
+ * Resource-managed extcon device register function.
+ */
+extern int devm_extcon_dev_register(struct device *dev,
+				struct extcon_dev *edev);
+extern void devm_extcon_dev_unregister(struct device *dev,
+				struct extcon_dev *edev);
+
+/*
  * get/set/update_state access the 32b encoded state value, which represents
  * states of all possible cables of the multistate port. For example, if one
  * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
@@ -254,6 +262,15 @@ static inline int extcon_dev_register(struct extcon_dev *edev)
 
 static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
 
+static inline devm_extcon_dev_register(struct device *dev,
+		struct extcon_dev *edev)
+{
+	return 0;
+}
+
+static inline devm_extcon_dev_unregister(struct device *dev,
+		struct extcon_dev *edev) { }
+
 static inline u32 extcon_get_state(struct extcon_dev *edev)
 {
 	return 0;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 2/8] extcon: adc-jack: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 1/8] extcon: Add resource-managed extcon " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 3/8] extcon: gpio: " Sangjung Woo
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/extcon/extcon-adc-jack.c |   30 +++++++++---------------------
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/drivers/extcon/extcon-adc-jack.c b/drivers/extcon/extcon-adc-jack.c
index e23f1c2..549d820 100644
--- a/drivers/extcon/extcon-adc-jack.c
+++ b/drivers/extcon/extcon-adc-jack.c
@@ -105,9 +105,8 @@ static int adc_jack_probe(struct platform_device *pdev)
 	data->edev.name = pdata->name;
 
 	if (!pdata->cable_names) {
-		err = -EINVAL;
 		dev_err(&pdev->dev, "error: cable_names not defined.\n");
-		goto out;
+		return -EINVAL;
 	}
 
 	data->edev.dev.parent = &pdev->dev;
@@ -117,18 +116,16 @@ static int adc_jack_probe(struct platform_device *pdev)
 	for (i = 0; data->edev.supported_cable[i]; i++)
 		;
 	if (i == 0 || i > SUPPORTED_CABLE_MAX) {
-		err = -EINVAL;
 		dev_err(&pdev->dev, "error: pdata->cable_names size = %d\n",
 				i - 1);
-		goto out;
+		return -EINVAL;
 	}
 	data->num_cables = i;
 
 	if (!pdata->adc_conditions ||
 			!pdata->adc_conditions[0].state) {
-		err = -EINVAL;
 		dev_err(&pdev->dev, "error: adc_conditions not defined.\n");
-		goto out;
+		return -EINVAL;
 	}
 	data->adc_conditions = pdata->adc_conditions;
 
@@ -138,10 +135,8 @@ static int adc_jack_probe(struct platform_device *pdev)
 	data->num_conditions = i;
 
 	data->chan = iio_channel_get(&pdev->dev, pdata->consumer_channel);
-	if (IS_ERR(data->chan)) {
-		err = PTR_ERR(data->chan);
-		goto out;
-	}
+	if (IS_ERR(data->chan))
+		return PTR_ERR(data->chan);
 
 	data->handling_delay = msecs_to_jiffies(pdata->handling_delay_ms);
 
@@ -149,15 +144,14 @@ static int adc_jack_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, data);
 
-	err = extcon_dev_register(&data->edev);
+	err = devm_extcon_dev_register(&pdev->dev, &data->edev);
 	if (err)
-		goto out;
+		return err;
 
 	data->irq = platform_get_irq(pdev, 0);
 	if (!data->irq) {
 		dev_err(&pdev->dev, "platform_get_irq failed\n");
-		err = -ENODEV;
-		goto err_irq;
+		return -ENODEV;
 	}
 
 	err = request_any_context_irq(data->irq, adc_jack_irq_thread,
@@ -165,15 +159,10 @@ static int adc_jack_probe(struct platform_device *pdev)
 
 	if (err < 0) {
 		dev_err(&pdev->dev, "error: irq %d\n", data->irq);
-		goto err_irq;
+		return err;
 	}
 
 	return 0;
-
-err_irq:
-	extcon_dev_unregister(&data->edev);
-out:
-	return err;
 }
 
 static int adc_jack_remove(struct platform_device *pdev)
@@ -182,7 +171,6 @@ static int adc_jack_remove(struct platform_device *pdev)
 
 	free_irq(data->irq, data);
 	cancel_work_sync(&data->handler.work);
-	extcon_dev_unregister(&data->edev);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 3/8] extcon: gpio: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 1/8] extcon: Add resource-managed extcon " Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 2/8] extcon: adc-jack: Use devm_extcon_dev_register() Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 4/8] extcon: max14577: " Sangjung Woo
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/extcon/extcon-gpio.c |   16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 13d5222..43af34c 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -121,34 +121,27 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 				msecs_to_jiffies(pdata->debounce);
 	}
 
-	ret = extcon_dev_register(&extcon_data->edev);
+	ret = devm_extcon_dev_register(&pdev->dev, &extcon_data->edev);
 	if (ret < 0)
 		return ret;
 
 	INIT_DELAYED_WORK(&extcon_data->work, gpio_extcon_work);
 
 	extcon_data->irq = gpio_to_irq(extcon_data->gpio);
-	if (extcon_data->irq < 0) {
-		ret = extcon_data->irq;
-		goto err;
-	}
+	if (extcon_data->irq < 0)
+		return extcon_data->irq;
 
 	ret = request_any_context_irq(extcon_data->irq, gpio_irq_handler,
 				      pdata->irq_flags, pdev->name,
 				      extcon_data);
 	if (ret < 0)
-		goto err;
+		return ret;
 
 	platform_set_drvdata(pdev, extcon_data);
 	/* Perform initial detection */
 	gpio_extcon_work(&extcon_data->work.work);
 
 	return 0;
-
-err:
-	extcon_dev_unregister(&extcon_data->edev);
-
-	return ret;
 }
 
 static int gpio_extcon_remove(struct platform_device *pdev)
@@ -157,7 +150,6 @@ static int gpio_extcon_remove(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&extcon_data->work);
 	free_irq(extcon_data->irq, extcon_data);
-	extcon_dev_unregister(&extcon_data->edev);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 4/8] extcon: max14577: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
                   ` (2 preceding siblings ...)
  2014-04-18  0:32 ` [PATCHv3 3/8] extcon: gpio: " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 5/8] extcon: max77693: " Sangjung Woo
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/extcon/extcon-max14577.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
index 1fef08d..c6166e7 100644
--- a/drivers/extcon/extcon-max14577.c
+++ b/drivers/extcon/extcon-max14577.c
@@ -675,7 +675,7 @@ static int max14577_muic_probe(struct platform_device *pdev)
 	}
 	info->edev->name = DEV_NAME;
 	info->edev->supported_cable = max14577_extcon_cable;
-	ret = extcon_dev_register(info->edev);
+	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register extcon device\n");
 		return ret;
@@ -694,7 +694,7 @@ static int max14577_muic_probe(struct platform_device *pdev)
 			MAX14577_REG_DEVICEID, &id);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to read revision number\n");
-		goto err_extcon;
+		return ret;
 	}
 	dev_info(info->dev, "device ID : 0x%x\n", id);
 
@@ -714,10 +714,6 @@ static int max14577_muic_probe(struct platform_device *pdev)
 			delay_jiffies);
 
 	return ret;
-
-err_extcon:
-	extcon_dev_unregister(info->edev);
-	return ret;
 }
 
 static int max14577_muic_remove(struct platform_device *pdev)
@@ -725,7 +721,6 @@ static int max14577_muic_remove(struct platform_device *pdev)
 	struct max14577_muic_info *info = platform_get_drvdata(pdev);
 
 	cancel_work_sync(&info->irq_work);
-	extcon_dev_unregister(info->edev);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 5/8] extcon: max77693: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
                   ` (3 preceding siblings ...)
  2014-04-18  0:32 ` [PATCHv3 4/8] extcon: max14577: " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 6/8] extcon: max8997: " Sangjung Woo
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/extcon/extcon-max77693.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
index 39cd095..f0f18e2 100644
--- a/drivers/extcon/extcon-max77693.c
+++ b/drivers/extcon/extcon-max77693.c
@@ -1185,7 +1185,7 @@ static int max77693_muic_probe(struct platform_device *pdev)
 	info->edev->name = DEV_NAME;
 	info->edev->dev.parent = &pdev->dev;
 	info->edev->supported_cable = max77693_extcon_cable;
-	ret = extcon_dev_register(info->edev);
+	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register extcon device\n");
 		goto err_irq;
@@ -1267,7 +1267,7 @@ static int max77693_muic_probe(struct platform_device *pdev)
 			MAX77693_MUIC_REG_ID, &id);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to read revision number\n");
-		goto err_extcon;
+		goto err_irq;
 	}
 	dev_info(info->dev, "device ID : 0x%x\n", id);
 
@@ -1288,8 +1288,6 @@ static int max77693_muic_probe(struct platform_device *pdev)
 
 	return ret;
 
-err_extcon:
-	extcon_dev_unregister(info->edev);
 err_irq:
 	while (--i >= 0)
 		free_irq(muic_irqs[i].virq, info);
@@ -1305,7 +1303,6 @@ static int max77693_muic_remove(struct platform_device *pdev)
 		free_irq(muic_irqs[i].virq, info);
 	cancel_work_sync(&info->irq_work);
 	input_unregister_device(info->dock);
-	extcon_dev_unregister(info->edev);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 6/8] extcon: max8997: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
                   ` (4 preceding siblings ...)
  2014-04-18  0:32 ` [PATCHv3 5/8] extcon: max77693: " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 7/8] extcon: palmas: " Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 8/8] extcon: arizona: " Sangjung Woo
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/extcon/extcon-max8997.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/extcon/extcon-max8997.c b/drivers/extcon/extcon-max8997.c
index 223e6b0..804a446 100644
--- a/drivers/extcon/extcon-max8997.c
+++ b/drivers/extcon/extcon-max8997.c
@@ -709,7 +709,7 @@ static int max8997_muic_probe(struct platform_device *pdev)
 	info->edev->name = DEV_NAME;
 	info->edev->dev.parent = &pdev->dev;
 	info->edev->supported_cable = max8997_extcon_cable;
-	ret = extcon_dev_register(info->edev);
+	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register extcon device\n");
 		goto err_irq;
@@ -790,8 +790,6 @@ static int max8997_muic_remove(struct platform_device *pdev)
 		free_irq(muic_irqs[i].virq, info);
 	cancel_work_sync(&info->irq_work);
 
-	extcon_dev_unregister(info->edev);
-
 	return 0;
 }
 
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 7/8] extcon: palmas: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
                   ` (5 preceding siblings ...)
  2014-04-18  0:32 ` [PATCHv3 6/8] extcon: max8997: " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  2014-04-18  0:32 ` [PATCHv3 8/8] extcon: arizona: " Sangjung Woo
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/extcon/extcon-palmas.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
index 51db5bc..1a770e0 100644
--- a/drivers/extcon/extcon-palmas.c
+++ b/drivers/extcon/extcon-palmas.c
@@ -192,7 +192,7 @@ static int palmas_usb_probe(struct platform_device *pdev)
 	palmas_usb->edev.name = kstrdup(node->name, GFP_KERNEL);
 	palmas_usb->edev.mutually_exclusive = mutually_exclusive;
 
-	status = extcon_dev_register(&palmas_usb->edev);
+	status = devm_extcon_dev_register(&pdev->dev, &palmas_usb->edev);
 	if (status) {
 		dev_err(&pdev->dev, "failed to register extcon device\n");
 		kfree(palmas_usb->edev.name);
@@ -209,7 +209,8 @@ static int palmas_usb_probe(struct platform_device *pdev)
 		if (status < 0) {
 			dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
 					palmas_usb->id_irq, status);
-			goto fail_extcon;
+			kfree(palmas_usb->edev.name);
+			return status;
 		}
 	}
 
@@ -223,26 +224,20 @@ static int palmas_usb_probe(struct platform_device *pdev)
 		if (status < 0) {
 			dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
 					palmas_usb->vbus_irq, status);
-			goto fail_extcon;
+			kfree(palmas_usb->edev.name);
+			return status;
 		}
 	}
 
 	palmas_enable_irq(palmas_usb);
 	device_set_wakeup_capable(&pdev->dev, true);
 	return 0;
-
-fail_extcon:
-	extcon_dev_unregister(&palmas_usb->edev);
-	kfree(palmas_usb->edev.name);
-
-	return status;
 }
 
 static int palmas_usb_remove(struct platform_device *pdev)
 {
 	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
 
-	extcon_dev_unregister(&palmas_usb->edev);
 	kfree(palmas_usb->edev.name);
 
 	return 0;
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCHv3 8/8] extcon: arizona: Use devm_extcon_dev_register()
  2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
                   ` (6 preceding siblings ...)
  2014-04-18  0:32 ` [PATCHv3 7/8] extcon: palmas: " Sangjung Woo
@ 2014-04-18  0:32 ` Sangjung Woo
  7 siblings, 0 replies; 13+ messages in thread
From: Sangjung Woo @ 2014-04-18  0:32 UTC (permalink / raw)
  To: MyungJoo Ham, Chanwoo Choi
  Cc: linux-kernel, Sangjung Woo, Krzysztof Kozlowski, Seung-Woo Kim

Use the resource-managed extcon device register function (i.e.
devm_extcon_dev_register()) instead of extcon_dev_register(). If extcon device
is attached with this function, that extcon device is automatically unregistered
on driver detach. That reduces tiresome managing code.

Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
---
 drivers/extcon/extcon-arizona.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 98a14f6..f63fa6f 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -1105,15 +1105,14 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info) {
 		dev_err(&pdev->dev, "Failed to allocate memory\n");
-		ret = -ENOMEM;
-		goto err;
+		return -ENOMEM;
 	}
 
 	info->micvdd = devm_regulator_get(arizona->dev, "MICVDD");
 	if (IS_ERR(info->micvdd)) {
 		ret = PTR_ERR(info->micvdd);
 		dev_err(arizona->dev, "Failed to get MICVDD: %d\n", ret);
-		goto err;
+		return ret;
 	}
 
 	mutex_init(&info->lock);
@@ -1155,11 +1154,11 @@ static int arizona_extcon_probe(struct platform_device *pdev)
 	info->edev.dev.parent = arizona->dev;
 	info->edev.supported_cable = arizona_cable;
 
-	ret = extcon_dev_register(&info->edev);
+	ret = devm_extcon_dev_register(&pdev->dev, &info->edev);
 	if (ret < 0) {
 		dev_err(arizona->dev, "extcon_dev_register() failed: %d\n",
 			ret);
-		goto err;
+		return ret;
 	}
 
 	info->input = devm_input_allocate_device(&pdev->dev);
@@ -1410,8 +1409,6 @@ err_rise:
 err_input:
 err_register:
 	pm_runtime_disable(&pdev->dev);
-	extcon_dev_unregister(&info->edev);
-err:
 	return ret;
 }
 
@@ -1445,7 +1442,6 @@ static int arizona_extcon_remove(struct platform_device *pdev)
 	regmap_update_bits(arizona->regmap, ARIZONA_JACK_DETECT_ANALOGUE,
 			   ARIZONA_JD1_ENA, 0);
 	arizona_clk32k_disable(arizona);
-	extcon_dev_unregister(&info->edev);
 
 	return 0;
 }
-- 
1.7.9.5


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function
  2014-04-18  0:32 ` [PATCHv3 1/8] extcon: Add resource-managed extcon " Sangjung Woo
@ 2014-04-19  7:13   ` Chanwoo Choi
  2014-04-19  9:50     ` Sangjung
  0 siblings, 1 reply; 13+ messages in thread
From: Chanwoo Choi @ 2014-04-19  7:13 UTC (permalink / raw)
  To: Sangjung Woo
  Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel, Krzysztof Kozlowski,
	Seung-Woo Kim

Hi Sangjung,

On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@samsung.com> wrote:
> Add resource-managed extcon device register function for convenience.
> For example, if a extcon device is attached with new
> devm_extcon_dev_register(), that extcon device is automatically
> unregistered on driver detach.
>
> Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
> ---
>  drivers/extcon/extcon-class.c |   72 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/extcon.h        |   17 ++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 7ab21aa..e177edb6 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  }
>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> +
> +/*
> + * Device resource management
> + */

This comment is un-necessary because this patchset(v3) remove 'struct
extcon_devres'.

> +static void devm_extcon_dev_release(struct device *dev, void *res)
> +{
> +       struct extcon_dev *devres = res;
> +
> +       extcon_dev_unregister(devres);

I prefer following function call withou defining separate 'devres' variable.
But, this casting on the first argument is only for readability.
  extcon_dev_unregister((strcut extcon_dev *)res);

> +}
> +
> +static int devm_extcon_dev_match(struct device *dev, void *res, void *data)
> +{
> +       struct extcon_dev *devres = res;
> +       struct extcon_dev *match = data;
> +       return devres == match;

To simplify code, I think you could change checking code as following:
            return res == data;

> +}
> +
> +/**
> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
> + * @dev:       device to allocate extcon device
> + * @edev:      the new extcon device to register
> + *
> + * Managed extcon_dev_register() function. If extcon device is attached with
> + * this function, that extcon device is automatically unregistered on driver
> + * detach. Internally this function calls extcon_dev_register() function.
> + * To get more information, refer that function.
> + *
> + * If extcon device is registered with this function and the device needs to be
> + * unregistered separately, devm_extcon_dev_unregister() should be used.
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev *edev)
> +{
> +       struct extcon_dev *devres;

The 'devres' in this function don't mean 'device resource structure'.
So I think it is not proper name.
I think you should use other general name (e.g.,  'ptr' or 'res'
instead of 'devres')

> +       int ret;
> +
> +       devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)

Other subsytem used double pointer to get device resource from
devres_alloc() instead of
single pointer.(devres is single pointer)  I can't find subsystem
using single pointer of devm function.
First of all, We have to analyze the correct reason using only double
pointer instead of single pointer whether single pointer use is good
or not.

> +                       GFP_KERNEL);
> +       if (!devres)
> +               return -ENOMEM;
> +
> +       ret = extcon_dev_register(edev);
> +       if (ret) {
> +               devres_free(devres);
> +               return ret;
> +       }
> +
> +       devres = edev;
> +       devres_add(dev, devres);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
> +
> +/**
> + * devm_extcon_dev_unregister() - Resource-managed extcon_dev_unregister()
> + * @dev:       device the extcon belongs to
> + * @edev:      the extcon device to unregister
> + *
> + * Unregister extcon device that is registered with devm_extcon_dev_register()
> + * function.
> + */
> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev *edev)
> +{
> +       WARN_ON(devres_release(dev, devm_extcon_dev_release,
> +                       devm_extcon_dev_match, edev));
> +}
> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
> +
>  #ifdef CONFIG_OF
>  /*
>   * extcon_get_edev_by_phandle - Get the extcon device from devicetree
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index f488145..35f3343 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev *edev);
>  extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
>
>  /*
> + * Resource-managed extcon device register function.
> + */
> +extern int devm_extcon_dev_register(struct device *dev,
> +                               struct extcon_dev *edev);
> +extern void devm_extcon_dev_unregister(struct device *dev,
> +                               struct extcon_dev *edev);

I prefer that this function meet indentation of function definition
needs as following:

extern int devm_extcon_dev_register(struct device *dev,

struct extcon_dev *edev);
extern void devm_extcon_dev_unregister(struct device *dev,

  struct extcon_dev *edev);

> +
> +/*
>   * get/set/update_state access the 32b encoded state value, which represents
>   * states of all possible cables of the multistate port. For example, if one
>   * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
> @@ -254,6 +262,15 @@ static inline int extcon_dev_register(struct extcon_dev *edev)
>
>  static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
>
> +static inline devm_extcon_dev_register(struct device *dev,
> +               struct extcon_dev *edev)

Missing the type of return value.
-> static inline int devm_extcon_dev_register(...

Also, ditto about function definition identification.

> +{
> +       return 0;
> +}
> +
> +static inline devm_extcon_dev_unregister(struct device *dev,

ditto.
-> static inline void devm_extcon_dev_unregister(...

Thanks,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function
  2014-04-19  7:13   ` Chanwoo Choi
@ 2014-04-19  9:50     ` Sangjung
  2014-04-19 13:36       ` Chanwoo Choi
  0 siblings, 1 reply; 13+ messages in thread
From: Sangjung @ 2014-04-19  9:50 UTC (permalink / raw)
  To: Chanwoo Choi
  Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel, Krzysztof Kozlowski,
	Seung-Woo Kim, again4you

Hi Chanwoo.

Thanks for your comments. I also add my opinion too.


On 04/19/2014 04:13 PM, Chanwoo Choi wrote:
> Hi Sangjung,
>
> On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@samsung.com> wrote:
>> Add resource-managed extcon device register function for convenience.
>> For example, if a extcon device is attached with new
>> devm_extcon_dev_register(), that extcon device is automatically
>> unregistered on driver detach.
>>
>> Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
>> ---
>>   drivers/extcon/extcon-class.c |   72 +++++++++++++++++++++++++++++++++++++++++
>>   include/linux/extcon.h        |   17 ++++++++++
>>   2 files changed, 89 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
>> index 7ab21aa..e177edb6 100644
>> --- a/drivers/extcon/extcon-class.c
>> +++ b/drivers/extcon/extcon-class.c
>> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>   }
>>   EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>>
>> +
>> +/*
>> + * Device resource management
>> + */
> This comment is un-necessary because this patchset(v3) remove 'struct
> extcon_devres'.
>
>> +static void devm_extcon_dev_release(struct device *dev, void *res)
>> +{
>> +       struct extcon_dev *devres = res;
>> +
>> +       extcon_dev_unregister(devres);
> I prefer following function call withou defining separate 'devres' variable.
> But, this casting on the first argument is only for readability.
>    extcon_dev_unregister((strcut extcon_dev *)res);
>
OK. I'll fix it.

>> +}
>> +
>> +static int devm_extcon_dev_match(struct device *dev, void *res, void *data)
>> +{
>> +       struct extcon_dev *devres = res;
>> +       struct extcon_dev *match = data;
>> +       return devres == match;
> To simplify code, I think you could change checking code as following:
>              return res == data;
Right. Simple is better than others.

>> +}
>> +
>> +/**
>> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
>> + * @dev:       device to allocate extcon device
>> + * @edev:      the new extcon device to register
>> + *
>> + * Managed extcon_dev_register() function. If extcon device is attached with
>> + * this function, that extcon device is automatically unregistered on driver
>> + * detach. Internally this function calls extcon_dev_register() function.
>> + * To get more information, refer that function.
>> + *
>> + * If extcon device is registered with this function and the device needs to be
>> + * unregistered separately, devm_extcon_dev_unregister() should be used.
>> + *
>> + * RETURNS:
>> + * 0 on success, negative error number on failure.
>> + */
>> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev *edev)
>> +{
>> +       struct extcon_dev *devres;
> The 'devres' in this function don't mean 'device resource structure'.
> So I think it is not proper name.
> I think you should use other general name (e.g.,  'ptr' or 'res'
> instead of 'devres')
>
>> +       int ret;
>> +
>> +       devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)
> Other subsytem used double pointer to get device resource from
> devres_alloc() instead of
> single pointer.(devres is single pointer)  I can't find subsystem
> using single pointer of devm function.
> First of all, We have to analyze the correct reason using only double
> pointer instead of single pointer whether single pointer use is good
> or not.

IMO, other subsystem should return the memory pointer that is allocated 
by devres_alloc().
However, in our case, we need not do that since the pointer is used only 
in extcon core.
You can refer the way that I did to gpio subsystem (devm_gpio_request() 
in /drivers/gpio/devres.c).

>> +                       GFP_KERNEL);
>> +       if (!devres)
>> +               return -ENOMEM;
>> +
>> +       ret = extcon_dev_register(edev);
>> +       if (ret) {
>> +               devres_free(devres);
>> +               return ret;
>> +       }
>> +
>> +       devres = edev;
>> +       devres_add(dev, devres);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
>> +
>> +/**
>> + * devm_extcon_dev_unregister() - Resource-managed extcon_dev_unregister()
>> + * @dev:       device the extcon belongs to
>> + * @edev:      the extcon device to unregister
>> + *
>> + * Unregister extcon device that is registered with devm_extcon_dev_register()
>> + * function.
>> + */
>> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev *edev)
>> +{
>> +       WARN_ON(devres_release(dev, devm_extcon_dev_release,
>> +                       devm_extcon_dev_match, edev));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
>> +
>>   #ifdef CONFIG_OF
>>   /*
>>    * extcon_get_edev_by_phandle - Get the extcon device from devicetree
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index f488145..35f3343 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev *edev);
>>   extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
>>
>>   /*
>> + * Resource-managed extcon device register function.
>> + */
>> +extern int devm_extcon_dev_register(struct device *dev,
>> +                               struct extcon_dev *edev);
>> +extern void devm_extcon_dev_unregister(struct device *dev,
>> +                               struct extcon_dev *edev);
> I prefer that this function meet indentation of function definition
> needs as following:
>
> extern int devm_extcon_dev_register(struct device *dev,
>
> struct extcon_dev *edev);
> extern void devm_extcon_dev_unregister(struct device *dev,
>
>    struct extcon_dev *edev);

I have a question about the indentation issue
since my Thunderbird email client does not show me the below line tidy.

You want me to adjust the start point of the second line
to the right after parentheses of the first line. right?

>> +
>> +/*
>>    * get/set/update_state access the 32b encoded state value, which represents
>>    * states of all possible cables of the multistate port. For example, if one
>>    * calls extcon_set_state(edev, 0x7), it may mean that all the three cables
>> @@ -254,6 +262,15 @@ static inline int extcon_dev_register(struct extcon_dev *edev)
>>
>>   static inline void extcon_dev_unregister(struct extcon_dev *edev) { }
>>
>> +static inline devm_extcon_dev_register(struct device *dev,
>> +               struct extcon_dev *edev)
> Missing the type of return value.
> -> static inline int devm_extcon_dev_register(...
>
> Also, ditto about function definition identification.

Right. I made a mistake.
>
>> +{
>> +       return 0;
>> +}
>> +
>> +static inline devm_extcon_dev_unregister(struct device *dev,
> ditto.
> -> static inline void devm_extcon_dev_unregister(...
>
> Thanks,
> Chanwoo Choi
>


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function
  2014-04-19  9:50     ` Sangjung
@ 2014-04-19 13:36       ` Chanwoo Choi
  2014-04-19 14:29         ` Chanwoo Choi
  0 siblings, 1 reply; 13+ messages in thread
From: Chanwoo Choi @ 2014-04-19 13:36 UTC (permalink / raw)
  To: Sangjung
  Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel, Krzysztof Kozlowski,
	Seung-Woo Kim, again4you

Hi,

On Sat, Apr 19, 2014 at 6:50 PM, Sangjung <sangjung.woo@samsung.com> wrote:
> Hi Chanwoo.
>
> Thanks for your comments. I also add my opinion too.
>
>
>
> On 04/19/2014 04:13 PM, Chanwoo Choi wrote:
>>
>> Hi Sangjung,
>>
>> On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@samsung.com>
>> wrote:
>>>
>>> Add resource-managed extcon device register function for convenience.
>>> For example, if a extcon device is attached with new
>>> devm_extcon_dev_register(), that extcon device is automatically
>>> unregistered on driver detach.
>>>
>>> Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
>>> ---
>>>   drivers/extcon/extcon-class.c |   72
>>> +++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/extcon.h        |   17 ++++++++++
>>>   2 files changed, 89 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-class.c
>>> b/drivers/extcon/extcon-class.c
>>> index 7ab21aa..e177edb6 100644
>>> --- a/drivers/extcon/extcon-class.c
>>> +++ b/drivers/extcon/extcon-class.c
>>> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>>>
>>> +
>>> +/*
>>> + * Device resource management
>>> + */
>>
>> This comment is un-necessary because this patchset(v3) remove 'struct
>> extcon_devres'.
>>
>>> +static void devm_extcon_dev_release(struct device *dev, void *res)
>>> +{
>>> +       struct extcon_dev *devres = res;
>>> +
>>> +       extcon_dev_unregister(devres);
>>
>> I prefer following function call withou defining separate 'devres'
>> variable.
>> But, this casting on the first argument is only for readability.
>>    extcon_dev_unregister((strcut extcon_dev *)res);
>>
> OK. I'll fix it.
>
>
>>> +}
>>> +
>>> +static int devm_extcon_dev_match(struct device *dev, void *res, void
>>> *data)
>>> +{
>>> +       struct extcon_dev *devres = res;
>>> +       struct extcon_dev *match = data;
>>> +       return devres == match;
>>
>> To simplify code, I think you could change checking code as following:
>>              return res == data;
>
> Right. Simple is better than others.
>
>
>>> +}
>>> +
>>> +/**
>>> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
>>> + * @dev:       device to allocate extcon device
>>> + * @edev:      the new extcon device to register
>>> + *
>>> + * Managed extcon_dev_register() function. If extcon device is attached
>>> with
>>> + * this function, that extcon device is automatically unregistered on
>>> driver
>>> + * detach. Internally this function calls extcon_dev_register()
>>> function.
>>> + * To get more information, refer that function.
>>> + *
>>> + * If extcon device is registered with this function and the device
>>> needs to be
>>> + * unregistered separately, devm_extcon_dev_unregister() should be used.
>>> + *
>>> + * RETURNS:
>>> + * 0 on success, negative error number on failure.
>>> + */
>>> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev
>>> *edev)
>>> +{
>>> +       struct extcon_dev *devres;
>>
>> The 'devres' in this function don't mean 'device resource structure'.
>> So I think it is not proper name.
>> I think you should use other general name (e.g.,  'ptr' or 'res'
>> instead of 'devres')
>>
>>> +       int ret;
>>> +
>>> +       devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)
>>
>> Other subsytem used double pointer to get device resource from
>> devres_alloc() instead of
>> single pointer.(devres is single pointer)  I can't find subsystem
>> using single pointer of devm function.
>> First of all, We have to analyze the correct reason using only double
>> pointer instead of single pointer whether single pointer use is good
>> or not.
>
>
> IMO, other subsystem should return the memory pointer that is allocated by
> devres_alloc().
> However, in our case, we need not do that since the pointer is used only in
> extcon core.
> You can refer the way that I did to gpio subsystem (devm_gpio_request() in
> /drivers/gpio/devres.c).

As you comment, I checked 'devm_gpio_request' in drivers/gpio/devres.c
. There are a little difference between devm_extcon_dev_register and
devm_gpio_request.

The second argument (unsigned gpio) is not pointer type in
devm_gpio_request() But, devm_extcon_dev_register() needs the pointer
type for second argument(struct extcon_dev *edev).

>
>
>>> +                       GFP_KERNEL);
>>> +       if (!devres)
>>> +               return -ENOMEM;
>>> +
>>> +       ret = extcon_dev_register(edev);
>>> +       if (ret) {
>>> +               devres_free(devres);
>>> +               return ret;
>>> +       }
>>> +
>>> +       devres = edev;
>>> +       devres_add(dev, devres);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_register);
>>> +
>>> +/**
>>> + * devm_extcon_dev_unregister() - Resource-managed
>>> extcon_dev_unregister()
>>> + * @dev:       device the extcon belongs to
>>> + * @edev:      the extcon device to unregister
>>> + *
>>> + * Unregister extcon device that is registered with
>>> devm_extcon_dev_register()
>>> + * function.
>>> + */
>>> +void devm_extcon_dev_unregister(struct device *dev, struct extcon_dev
>>> *edev)
>>> +{
>>> +       WARN_ON(devres_release(dev, devm_extcon_dev_release,
>>> +                       devm_extcon_dev_match, edev));
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_extcon_dev_unregister);
>>> +
>>>   #ifdef CONFIG_OF
>>>   /*
>>>    * extcon_get_edev_by_phandle - Get the extcon device from devicetree
>>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>>> index f488145..35f3343 100644
>>> --- a/include/linux/extcon.h
>>> +++ b/include/linux/extcon.h
>>> @@ -188,6 +188,14 @@ extern void extcon_dev_unregister(struct extcon_dev
>>> *edev);
>>>   extern struct extcon_dev *extcon_get_extcon_dev(const char
>>> *extcon_name);
>>>
>>>   /*
>>> + * Resource-managed extcon device register function.
>>> + */
>>> +extern int devm_extcon_dev_register(struct device *dev,
>>> +                               struct extcon_dev *edev);
>>> +extern void devm_extcon_dev_unregister(struct device *dev,
>>> +                               struct extcon_dev *edev);
>>
>> I prefer that this function meet indentation of function definition
>> needs as following:
>>
>> extern int devm_extcon_dev_register(struct device *dev,
>>
>> struct extcon_dev *edev);
>> extern void devm_extcon_dev_unregister(struct device *dev,
>>
>>    struct extcon_dev *edev);
>
>
> I have a question about the indentation issue
> since my Thunderbird email client does not show me the below line tidy.
>
> You want me to adjust the start point of the second line
> to the right after parentheses of the first line. right?

Right.

Thanks,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCHv3 1/8] extcon: Add resource-managed extcon register function
  2014-04-19 13:36       ` Chanwoo Choi
@ 2014-04-19 14:29         ` Chanwoo Choi
  0 siblings, 0 replies; 13+ messages in thread
From: Chanwoo Choi @ 2014-04-19 14:29 UTC (permalink / raw)
  To: Sangjung
  Cc: MyungJoo Ham, Chanwoo Choi, linux-kernel, Krzysztof Kozlowski,
	Seung-Woo Kim, again4you

Hi,

On Sat, Apr 19, 2014 at 10:36 PM, Chanwoo Choi <cwchoi00@gmail.com> wrote:
> Hi,
>
> On Sat, Apr 19, 2014 at 6:50 PM, Sangjung <sangjung.woo@samsung.com> wrote:
>> Hi Chanwoo.
>>
>> Thanks for your comments. I also add my opinion too.
>>
>>
>>
>> On 04/19/2014 04:13 PM, Chanwoo Choi wrote:
>>>
>>> Hi Sangjung,
>>>
>>> On Fri, Apr 18, 2014 at 9:32 AM, Sangjung Woo <sangjung.woo@samsung.com>
>>> wrote:
>>>>
>>>> Add resource-managed extcon device register function for convenience.
>>>> For example, if a extcon device is attached with new
>>>> devm_extcon_dev_register(), that extcon device is automatically
>>>> unregistered on driver detach.
>>>>
>>>> Signed-off-by: Sangjung Woo <sangjung.woo@samsung.com>
>>>> ---
>>>>   drivers/extcon/extcon-class.c |   72
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>   include/linux/extcon.h        |   17 ++++++++++
>>>>   2 files changed, 89 insertions(+)
>>>>
>>>> diff --git a/drivers/extcon/extcon-class.c
>>>> b/drivers/extcon/extcon-class.c
>>>> index 7ab21aa..e177edb6 100644
>>>> --- a/drivers/extcon/extcon-class.c
>>>> +++ b/drivers/extcon/extcon-class.c
>>>> @@ -819,6 +819,78 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>>>>   }
>>>>   EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>>>>
>>>> +
>>>> +/*
>>>> + * Device resource management
>>>> + */
>>>
>>> This comment is un-necessary because this patchset(v3) remove 'struct
>>> extcon_devres'.
>>>
>>>> +static void devm_extcon_dev_release(struct device *dev, void *res)
>>>> +{
>>>> +       struct extcon_dev *devres = res;
>>>> +
>>>> +       extcon_dev_unregister(devres);
>>>
>>> I prefer following function call withou defining separate 'devres'
>>> variable.
>>> But, this casting on the first argument is only for readability.
>>>    extcon_dev_unregister((strcut extcon_dev *)res);
>>>
>> OK. I'll fix it.
>>
>>
>>>> +}
>>>> +
>>>> +static int devm_extcon_dev_match(struct device *dev, void *res, void
>>>> *data)
>>>> +{
>>>> +       struct extcon_dev *devres = res;
>>>> +       struct extcon_dev *match = data;
>>>> +       return devres == match;
>>>
>>> To simplify code, I think you could change checking code as following:
>>>              return res == data;
>>
>> Right. Simple is better than others.
>>
>>
>>>> +}
>>>> +
>>>> +/**
>>>> + * devm_extcon_dev_register() - Resource-managed extcon_dev_register()
>>>> + * @dev:       device to allocate extcon device
>>>> + * @edev:      the new extcon device to register
>>>> + *
>>>> + * Managed extcon_dev_register() function. If extcon device is attached
>>>> with
>>>> + * this function, that extcon device is automatically unregistered on
>>>> driver
>>>> + * detach. Internally this function calls extcon_dev_register()
>>>> function.
>>>> + * To get more information, refer that function.
>>>> + *
>>>> + * If extcon device is registered with this function and the device
>>>> needs to be
>>>> + * unregistered separately, devm_extcon_dev_unregister() should be used.
>>>> + *
>>>> + * RETURNS:
>>>> + * 0 on success, negative error number on failure.
>>>> + */
>>>> +int devm_extcon_dev_register(struct device *dev, struct extcon_dev
>>>> *edev)
>>>> +{
>>>> +       struct extcon_dev *devres;
>>>
>>> The 'devres' in this function don't mean 'device resource structure'.
>>> So I think it is not proper name.
>>> I think you should use other general name (e.g.,  'ptr' or 'res'
>>> instead of 'devres')
>>>
>>>> +       int ret;
>>>> +
>>>> +       devres = devres_alloc(devm_extcon_dev_release, sizeof(*devres)
>>>
>>> Other subsytem used double pointer to get device resource from
>>> devres_alloc() instead of
>>> single pointer.(devres is single pointer)  I can't find subsystem
>>> using single pointer of devm function.
>>> First of all, We have to analyze the correct reason using only double
>>> pointer instead of single pointer whether single pointer use is good
>>> or not.
>>
>>
>> IMO, other subsystem should return the memory pointer that is allocated by
>> devres_alloc().
>> However, in our case, we need not do that since the pointer is used only in
>> extcon core.
>> You can refer the way that I did to gpio subsystem (devm_gpio_request() in
>> /drivers/gpio/devres.c).
>
> As you comment, I checked 'devm_gpio_request' in drivers/gpio/devres.c
> . There are a little difference between devm_extcon_dev_register and
> devm_gpio_request.
>
> The second argument (unsigned gpio) is not pointer type in
> devm_gpio_request() But, devm_extcon_dev_register() needs the pointer
> type for second argument(struct extcon_dev *edev).

This patch have to use double pointer for extcon device from devres_alloc().

Because in this following case:
struct extcon_dev *ptr;

ptr = devres_alloc(...)
...
ptr = edev;
-> edev would override the 'ptr' which is allocated memory from devres_alloc()
In result, this patch cannot access original allocated memory for 'ptr'.

Thanks,
Chanwoo Choi

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2014-04-19 14:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-18  0:32 [PATCHv3 0/8] Resource-managed extcon device register function Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 1/8] extcon: Add resource-managed extcon " Sangjung Woo
2014-04-19  7:13   ` Chanwoo Choi
2014-04-19  9:50     ` Sangjung
2014-04-19 13:36       ` Chanwoo Choi
2014-04-19 14:29         ` Chanwoo Choi
2014-04-18  0:32 ` [PATCHv3 2/8] extcon: adc-jack: Use devm_extcon_dev_register() Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 3/8] extcon: gpio: " Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 4/8] extcon: max14577: " Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 5/8] extcon: max77693: " Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 6/8] extcon: max8997: " Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 7/8] extcon: palmas: " Sangjung Woo
2014-04-18  0:32 ` [PATCHv3 8/8] extcon: arizona: " Sangjung Woo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox