LinuxLists.cc - [PATCH 0/8] Resource-managed extcon device register function

2014-04-16 10:27:06

Subject: [PATCH 0/8] Resource-managed extcon device register function

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.

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 | 13 ++----
drivers/extcon/extcon-class.c | 83 ++++++++++++++++++++++++++++++++++++++
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 | 8 ++++
9 files changed, 118 insertions(+), 67 deletions(-)

--
1.7.9.5

2014-04-16 10:27:09

by Sangjung

[permalink] [raw]

Subject: [PATCH 1/8] extcon: Add resource-managed extcon register function

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 <[email protected]>
---
drivers/extcon/extcon-class.c | 83 +++++++++++++++++++++++++++++++++++++++++
include/linux/extcon.h | 8 ++++
2 files changed, 91 insertions(+)

diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
index 7ab21aa..accb49c 100644
--- a/drivers/extcon/extcon-class.c
+++ b/drivers/extcon/extcon-class.c
@@ -32,6 +32,7 @@
#include <linux/slab.h>
#include <linux/sysfs.h>
#include <linux/of.h>
+#include <linux/gfp.h>

/*
* extcon_cable_name suggests the standard cable names for commonly used
@@ -819,6 +820,88 @@ void extcon_dev_unregister(struct extcon_dev *edev)
}
EXPORT_SYMBOL_GPL(extcon_dev_unregister);

+
+/*
+ * Device resource management
+ */
+
+struct extcon_devres {
+ struct extcon_dev *edev;
+};
+
+static void devm_extcon_release(struct device *dev, void *res)
+{
+ struct extcon_devres *dr = (struct extcon_devres *)res;
+
+ extcon_dev_unregister(dr->edev);
+}
+
+static int devm_extcon_match(struct device *dev, void *res, void *data)
+{
+ struct extcon_devres *dr = (struct extcon_devres *)res;
+ struct extcon_devres *match = (struct extcon_devres *)data;
+
+ return dr->edev == match->edev;
+}
+
+/**
+ * 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_devres *dr;
+ int rc;
+
+ dr = devres_alloc(devm_extcon_release, sizeof(struct extcon_devres),
+ GFP_KERNEL);
+ if (!dr)
+ return -ENOMEM;
+
+ rc = extcon_dev_register(edev);
+ if (rc) {
+ devres_free(dr);
+ return rc;
+ }
+
+ dr->edev = edev;
+ devres_add(dev, dr);
+
+ 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)
+{
+ struct extcon_devres match_dr = { edev };
+
+ WARN_ON(devres_destroy(dev, devm_extcon_release,
+ devm_extcon_match, &match_dr));
+
+ extcon_dev_unregister(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..e1e85a1 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
--
1.7.9.5

2014-04-16 10:27:39

by Sangjung

[permalink] [raw]

Subject: [PATCH 4/8] extcon: max14577: Use devm_extcon_dev_register()

2014-04-16 10:27:36

by Sangjung

[permalink] [raw]

Subject: [PATCH 8/8] extcon: arizona: Use devm_extcon_dev_register()

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 <[email protected]>
---
drivers/extcon/extcon-arizona.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
index 98a14f6..40e6c0b 100644
--- a/drivers/extcon/extcon-arizona.c
+++ b/drivers/extcon/extcon-arizona.c
@@ -1105,15 +1105,13 @@ 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 PTR_ERR(info->micvdd);
}

mutex_init(&info->lock);
@@ -1155,11 +1153,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 +1408,6 @@ err_rise:
err_input:
err_register:
pm_runtime_disable(&pdev->dev);
- extcon_dev_unregister(&info->edev);
-err:
return ret;
}

@@ -1445,7 +1441,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

2014-04-16 10:27:33

by Sangjung

[permalink] [raw]

Subject: [PATCH 7/8] extcon: palmas: Use devm_extcon_dev_register()

2014-04-16 10:27:31

by Sangjung

[permalink] [raw]

Subject: [PATCH 6/8] extcon: max8997: Use devm_extcon_dev_register()

2014-04-16 10:27:28

by Sangjung

[permalink] [raw]

Subject: [PATCH 3/8] extcon: gpio: Use devm_extcon_dev_register()

2014-04-16 10:30:50

by Sangjung

[permalink] [raw]

Subject: [PATCH 5/8] extcon: max77693: Use devm_extcon_dev_register()

2014-04-16 10:31:47

by Sangjung

[permalink] [raw]

Subject: [PATCH 2/8] extcon: adc-jack: Use devm_extcon_dev_register()

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 <[email protected]>
---
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

2014-04-16 10:44:26

by Seung-Woo Kim

[permalink] [raw]

Subject: Re: [PATCH 8/8] extcon: arizona: Use devm_extcon_dev_register()

Hi,

On 2014?? 04?? 16?? 19:27, Sangjung Woo wrote:
> 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 <[email protected]>
> ---
> drivers/extcon/extcon-arizona.c | 13 ++++---------
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
> index 98a14f6..40e6c0b 100644
> --- a/drivers/extcon/extcon-arizona.c
> +++ b/drivers/extcon/extcon-arizona.c
> @@ -1105,15 +1105,13 @@ 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);

Assignment to ret is removed but it is still used here.

> - goto err;
> + return PTR_ERR(info->micvdd);
> }
>
> mutex_init(&info->lock);
> @@ -1155,11 +1153,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 +1408,6 @@ err_rise:
> err_input:
> err_register:
> pm_runtime_disable(&pdev->dev);
> - extcon_dev_unregister(&info->edev);
> -err:
> return ret;
> }
>
> @@ -1445,7 +1441,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;
> }
>

--
Seung-Woo Kim
Samsung Software R&D Center
--

2014-04-16 11:23:14

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH 5/8] extcon: max77693: Use devm_extcon_dev_register()

On śro, 2014-04-16 at 19:27 +0900, Sangjung Woo wrote:
> 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 <[email protected]>
> ---
> drivers/extcon/extcon-max77693.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

>
> 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;
> }

2014-04-16 11:24:06

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH 6/8] extcon: max8997: Use devm_extcon_dev_register()

On śro, 2014-04-16 at 19:27 +0900, Sangjung Woo wrote:
> 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 <[email protected]>
> ---
> drivers/extcon/extcon-max8997.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

> 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;
> }
>

2014-04-16 11:40:25

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH 4/8] extcon: max14577: Use devm_extcon_dev_register()

On śro, 2014-04-16 at 19:26 +0900, Sangjung Woo wrote:
> 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 <[email protected]>
> ---
> drivers/extcon/extcon-max14577.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

>
> 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;
> }

2014-04-17 01:00:33

by Sangjung

[permalink] [raw]

Subject: Re: [PATCH 8/8] extcon: arizona: Use devm_extcon_dev_register()

To Seung-Woo.

On 04/16/2014 07:44 PM, Seung-Woo Kim wrote:
> Hi,
>
> On 2014?? 04?? 16?? 19:27, Sangjung Woo wrote:
>> 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 <[email protected]>
>> ---
>> drivers/extcon/extcon-arizona.c | 13 ++++---------
>> 1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-arizona.c b/drivers/extcon/extcon-arizona.c
>> index 98a14f6..40e6c0b 100644
>> --- a/drivers/extcon/extcon-arizona.c
>> +++ b/drivers/extcon/extcon-arizona.c
>> @@ -1105,15 +1105,13 @@ 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);
> Assignment to ret is removed but it is still used here.

You're right.
I will fix and send it as second version.

Thank you for your comment.

BRs,
Sangjung

2014-04-17 05:35:52

by Chanwoo Choi

[permalink] [raw]

Subject: Re: [PATCH 1/8] extcon: Add resource-managed extcon register function

Hi Sangjung,

Thanks for your contribution.

On 04/16/2014 07:26 PM, Sangjung Woo 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 <[email protected]>
> ---
> drivers/extcon/extcon-class.c | 83 +++++++++++++++++++++++++++++++++++++++++
> include/linux/extcon.h | 8 ++++
> 2 files changed, 91 insertions(+)
>
> diff --git a/drivers/extcon/extcon-class.c b/drivers/extcon/extcon-class.c
> index 7ab21aa..accb49c 100644
> --- a/drivers/extcon/extcon-class.c
> +++ b/drivers/extcon/extcon-class.c
> @@ -32,6 +32,7 @@
> #include <linux/slab.h>
> #include <linux/sysfs.h>
> #include <linux/of.h>
> +#include <linux/gfp.h>

It is not necessary because 'device.h' already includes 'gfp.h' header file.

>
> /*
> * extcon_cable_name suggests the standard cable names for commonly used
> @@ -819,6 +820,88 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> }
> EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>
> +
> +/*
> + * Device resource management
> + */
> +

Should delete blank line.

> +struct extcon_devres {
> + struct extcon_dev *edev;
> +};
> +
> +static void devm_extcon_release(struct device *dev, void *res)

Need to change function name as following to sustain consistency of existing extcon functions.
- devm_extcon_release -> devm_extcon_dev_release()

> +{
> + struct extcon_devres *dr = (struct extcon_devres *)res;
> +
> + extcon_dev_unregister(dr->edev);
> +}
> +
> +static int devm_extcon_match(struct device *dev, void *res, void *data)

ditto.
- devm_extcon_match -> devm_extcon_dev_match

> +{
> + struct extcon_devres *dr = (struct extcon_devres *)res;
> + struct extcon_devres *match = (struct extcon_devres *)data;

I think that this function don't need explicit casting
because as I knew, casting is automatically about tool-chain.

> +
> + return dr->edev == match->edev;
> +}
> +
> +/**
> + * 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_devres *dr;

To improve readability, I prefer to change 'dr' variable name (e.g., dr ->devres)

> + int rc;

I think 'rc' variable name is ambiguous.
I prefer to change variable name for return value. (rc -> ret)

> +
> + dr = devres_alloc(devm_extcon_release, sizeof(struct extcon_devres),

ditto.
- devm_extcon_release -> devm_extcon_dev_release

We chan modify it as following:
sizeof(struct extcon_devres) -> sizeof(*dr)

> + GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + rc = extcon_dev_register(edev);
> + if (rc) {
> + devres_free(dr);
> + return rc;
> + }
> +
> + dr->edev = edev;
> + devres_add(dev, dr);
> +
> + 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)
> +{
> + struct extcon_devres match_dr = { edev };

Should we define 'match_dr' variable? I think it is not necessary.
Maybe it could use 'edev' directly without casting.

> +
> + WARN_ON(devres_destroy(dev, devm_extcon_release,
> + devm_extcon_match, &match_dr));

I think that devres_release() is more proper than devres_destroy.

> +
> + extcon_dev_unregister(edev);

If you use devres_release() instead of devres_destroy(), don't need to call
extcon_dev_unregister() function separately because devres_release() function
would call 'release' function.

> +}
> +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..e1e85a1 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);

This functions need to consider if CONFIG_OF is disabled.
IF CONFIG_OF is disabled, devm_extcon_dev_register/unregister() function
must need static inline function for dummy function.

Thanks,
Chanwoo Choi