[PATCH v1] media: dvb: Add devm_dvb_register_adapter

[PATCH v1] media: dvb: Add devm_dvb_register_adapter

[Marc Gonzalez]

Add devm* variant for automagic resource release.
---
 drivers/media/dvb-core/dvbdev.c | 21 +++++++++++++++++++++
 include/media/dvbdev.h          |  3 +++
 2 files changed, 24 insertions(+)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index a3393cd4e584..c7a5302e8840 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -885,6 +885,27 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name,
 }
 EXPORT_SYMBOL(dvb_register_adapter);
 
+static void unregister_adapter(void *adap)
+{
+	dvb_unregister_adapter(adap);
+}
+
+int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap,
+		const char *name, struct module *module, short *adapter_nums)
+{
+	int v1, v2;
+
+	v1 = dvb_register_adapter(adap, name, module, dev, adapter_nums);
+	if (v1 < 0)
+		return v1;
+
+	v2 = devm_add_action_or_reset(dev, unregister_adapter, adap);
+	if (v2 < 0)
+		return v2;
+
+	return v1;
+}
+EXPORT_SYMBOL(devm_dvb_register_adapter);
 
 int dvb_unregister_adapter(struct dvb_adapter *adap)
 {
diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h
index 551325858de3..32f8d1626319 100644
--- a/include/media/dvbdev.h
+++ b/include/media/dvbdev.h
@@ -202,6 +202,9 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name,
 			 struct module *module, struct device *device,
 			 short *adapter_nums);
 
+int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap,
+		const char *name, struct module *module, short *adapter_nums);
+
 /**
  * dvb_unregister_adapter - Unregisters a DVB adapter
  *
-- 
2.17.1

Re: [PATCH v1] media: dvb: Add devm_dvb_register_adapter

[Jonathan Neuschäfer]

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

Hi,

some comments below.

On Fri, Jul 12, 2019 at 04:19:20PM +0200, Marc Gonzalez wrote:
> Add devm* variant for automagic resource release.

S-o-b missing.

> +int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap,
> +		const char *name, struct module *module, short *adapter_nums)
> +{
> +	int v1, v2;
> +
> +	v1 = dvb_register_adapter(adap, name, module, dev, adapter_nums);
> +	if (v1 < 0)
> +		return v1;
> +
> +	v2 = devm_add_action_or_reset(dev, unregister_adapter, adap);
> +	if (v2 < 0)
> +		return v2;
> +
> +	return v1;
> +}
> +EXPORT_SYMBOL(devm_dvb_register_adapter);

What non-negative numbers can dvb_register_adapter and
devm_add_action_or_reset return, and what are their meanings? Why should
devm_dvb_register_adapter return the (non-negative) return value of
dvb_register_adapter?
(I really don't know, because I'm not familiar with the media/DVB subsystem)

*If* the non-negative return values don't matter, I'd simplify the code
to something like this:

	int res;

	res = dvb_register_adapter(adap, name, module, dev, adapter_nums);
	if (res < 0)
		return res;

	res = devm_add_action_or_reset(dev, unregister_adapter, adap);
	if (res < 0)
		return res;

	return 0;

(or even 'return devm_add_action_or_reset(...)' directly)


> +int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap,
> +		const char *name, struct module *module, short *adapter_nums);

I think this function should also be added to
Documentation/driver-model/devres.rst (previously called devres.txt),
considering that most (or all?) devm_ functions are listed there.


Thanks,
Jonthan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] media: dvb: Add devm_dvb_register_adapter

[Marc Gonzalez]

On 12/07/2019 18:26, Jonathan Neuschäfer wrote:

> On Fri, Jul 12, 2019 at 04:19:20PM +0200, Marc Gonzalez wrote:
>
>> Add devm* variant for automagic resource release.
> 
> S-o-b missing.
> 
>> +int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap,
>> +		const char *name, struct module *module, short *adapter_nums)
>> +{
>> +	int v1, v2;
>> +
>> +	v1 = dvb_register_adapter(adap, name, module, dev, adapter_nums);
>> +	if (v1 < 0)
>> +		return v1;
>> +
>> +	v2 = devm_add_action_or_reset(dev, unregister_adapter, adap);
>> +	if (v2 < 0)
>> +		return v2;
>> +
>> +	return v1;
>> +}
>> +EXPORT_SYMBOL(devm_dvb_register_adapter);
> 
> What non-negative numbers can dvb_register_adapter and
> devm_add_action_or_reset return, and what are their meanings? Why should
> devm_dvb_register_adapter return the (non-negative) return value of
> dvb_register_adapter?
> (I really don't know, because I'm not familiar with the media/DVB subsystem)

It seems the return values of dvb_register_adapter() are not documented in
include/media/dvbdev.h -- Oh well...

Based on my reading of the implementation, dvb_register_adapter() returns
- either -ENFILE on error, or
- an index >= 0 in the dvb_adapter_list

Based on a small sampling of callers, it seems like most don't care about
the index. I'll spin a v2 that returns only 0 on success as you suggested.

I'll update Documentation/driver-model/devres.rst as you pointed out.

Regards.