diff mbox

[6/6] DVB API: LNA documentation

Message ID 1345167310-8738-7-git-send-email-crope@iki.fi (mailing list archive)
State New, archived
Headers show

Commit Message

Antti Palosaari Aug. 17, 2012, 1:35 a.m. UTC 
Signed-off-by: Antti Palosaari <crope@iki.fi>
---
 Documentation/DocBook/media/dvb/dvbproperty.xml | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Mauro Carvalho Chehab Sept. 11, 2012, 6:38 p.m. UTC | #1 
Em 16-08-2012 22:35, Antti Palosaari escreveu:
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  Documentation/DocBook/media/dvb/dvbproperty.xml | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/Documentation/DocBook/media/dvb/dvbproperty.xml b/Documentation/DocBook/media/dvb/dvbproperty.xml
> index d188be9..2dfa6a0 100644
> --- a/Documentation/DocBook/media/dvb/dvbproperty.xml
> +++ b/Documentation/DocBook/media/dvb/dvbproperty.xml
> @@ -827,6 +827,17 @@ enum fe_interleaving {
>  };
>  	</programlisting>
>  	</section>
> +	<section id="DTV-LNA">
> +	<title><constant>DTV_LNA</constant></title>
> +	<para>Low-noise amplifier.</para>
> +	<para>Hardware might offer controllable LNA which can be set manually
> +		using that parameter. Usually LNA could be found only from
> +		terrestrial devices if at all.</para>
> +	<para>Possible values: 0, 1, INT_MIN</para>

Hmm... INT_MIN... are you sure it is portable on all Linux compilers?

I don't like the idea on trusting on whatever C/C++/Java/... compiler (or some interpreter)
would define as "INT_MIN".

The better is to define a value for that, or, instead, to define something
at the API header file that won't cause troubles with 32 bits or 64 bits
userspace, like defining it as:

#define DVB_AUTO_LNA ((u32)~0)

> +	<para>0, LNA off</para>
> +	<para>1, LNA on</para>
> +	<para>INT_MIN, LNA auto</para>
> +	</section>
>  </section>
>  	<section id="frontend-property-terrestrial-systems">
>  	<title>Properties used on terrestrial delivery systems</title>
> @@ -847,6 +858,7 @@ enum fe_interleaving {
>  				<listitem><para><link linkend="DTV-GUARD-INTERVAL"><constant>DTV_GUARD_INTERVAL</constant></link></para></listitem>
>  				<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
>  				<listitem><para><link linkend="DTV-HIERARCHY"><constant>DTV_HIERARCHY</constant></link></para></listitem>
> +				<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
>  			</itemizedlist>
>  		</section>
>  		<section id="dvbt2-params">
> @@ -870,6 +882,7 @@ enum fe_interleaving {
>  			<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-HIERARCHY"><constant>DTV_HIERARCHY</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-DVBT2-PLP-ID"><constant>DTV_DVBT2_PLP_ID</constant></link></para></listitem>
> +			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
>  		</itemizedlist>
>  		</section>
>  		<section id="isdbt">
> @@ -981,6 +994,7 @@ enum fe_interleaving {
>  				<listitem><para><link linkend="DTV-GUARD-INTERVAL"><constant>DTV_GUARD_INTERVAL</constant></link></para></listitem>
>  				<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
>  				<listitem><para><link linkend="DTV-INTERLEAVING"><constant>DTV_INTERLEAVING</constant></link></para></listitem>
> +				<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
>  			</itemizedlist>
>  		</section>
>  	</section>
> @@ -1001,6 +1015,7 @@ enum fe_interleaving {
>  			<listitem><para><link linkend="DTV-INVERSION"><constant>DTV_INVERSION</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-SYMBOL-RATE"><constant>DTV_SYMBOL_RATE</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-INNER-FEC"><constant>DTV_INNER_FEC</constant></link></para></listitem>
> +			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
>  		</itemizedlist>
>  	</section>
>  	<section id="dvbc-annex-b-params">
> @@ -1015,6 +1030,7 @@ enum fe_interleaving {
>  			<listitem><para><link linkend="DTV-FREQUENCY"><constant>DTV_FREQUENCY</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-MODULATION"><constant>DTV_MODULATION</constant></link></para></listitem>
>  			<listitem><para><link linkend="DTV-INVERSION"><constant>DTV_INVERSION</constant></link></para></listitem>
> +			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
>  		</itemizedlist>
>  	</section>
>  	</section>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Waring Sept. 12, 2012, 11:01 a.m. UTC | #2 
On 11/09/12 19:38, Mauro Carvalho Chehab wrote:
> Em 16-08-2012 22:35, Antti Palosaari escreveu:
>> [snip]
>> +	<para>Possible values: 0, 1, INT_MIN</para>
> 
> Hmm... INT_MIN... are you sure it is portable on all Linux compilers?
> 
> I don't like the idea on trusting on whatever C/C++/Java/... compiler (or some interpreter)
> would define as "INT_MIN".
> 
> The better is to define a value for that, or, instead, to define something
> at the API header file that won't cause troubles with 32 bits or 64 bits
> userspace, like defining it as:
> 
> #define DVB_AUTO_LNA ((u32)~0)
> 
INT_MIN is defined in limits.h which is an ISO standard header. Other
parts of the kernel also use INT_MIN, e.g. linux/cpu.h and
linux/netfilter_ipv4.h both reference INT_MIN from limits.h.
Mauro Carvalho Chehab Sept. 12, 2012, 12:45 p.m. UTC | #3 
Em 12-09-2012 08:01, David Waring escreveu:
> On 11/09/12 19:38, Mauro Carvalho Chehab wrote:
>> Em 16-08-2012 22:35, Antti Palosaari escreveu:
>>> [snip]
>>> +	<para>Possible values: 0, 1, INT_MIN</para>
>>
>> Hmm... INT_MIN... are you sure it is portable on all Linux compilers?
>>
>> I don't like the idea on trusting on whatever C/C++/Java/... compiler (or some interpreter)
>> would define as "INT_MIN".
>>
>> The better is to define a value for that, or, instead, to define something
>> at the API header file that won't cause troubles with 32 bits or 64 bits
>> userspace, like defining it as:
>>
>> #define DVB_AUTO_LNA ((u32)~0)
>>
> INT_MIN is defined in limits.h which is an ISO standard header. Other
> parts of the kernel also use INT_MIN, e.g. linux/cpu.h and
> linux/netfilter_ipv4.h both reference INT_MIN from limits.h.

The linux/cpu.h is a Kernel internal header. There's no public userspace API 
there. So, it uses kernel's own definition for INT_MIN.

You're right with regards to netfilter. Btw, it is only places where INT_MIN 
is used on an userspace-filtered headers are at the netfilter interface :

/usr/include/linux/netfilter_ipv6.h:#include <limits.h> /* for INT_MIN, INT_MAX */
/usr/include/linux/netfilter_ipv6.h:	NF_IP6_PRI_FIRST = INT_MIN,
/usr/include/linux/netfilter_ipv4.h:#include <limits.h> /* for INT_MIN, INT_MAX */
/usr/include/linux/netfilter_ipv4.h:	NF_IP_PRI_FIRST = INT_MIN,
/usr/include/linux/netfilter_decnet.h:#include <limits.h> /* for INT_MIN, INT_MAX */
/usr/include/linux/netfilter_decnet.h:	NF_DN_PRI_FIRST = INT_MIN,

Even so, it got renamed inside a priorities enum:

enum nf_ip_hook_priorities {
	NF_IP_PRI_FIRST = INT_MIN,

In the case of netfilter, as this is just a priority number, it actually
make sense to use INT_MIN as the lowest priority, as INT_MIN is the lowest
number that can be represented there.

What we're doing here is something else: we're defining a special value to be 
interpreted as "AUTO". In this case, if Kernel and userspace disagrees on what
value should be used, the KAPI will be deadly broken.

I might be wrong, but some C compilers on a few architectures (Tru64 C compiler comes
on my mind) define "int" as 64 bit integers, with will affect the definition of INT_MIN.
Ok, in this case, the definition will be compatible, but I'm wondering if some other
compiler might be doing something else here.

That's why I'm in favor of defining some constant for "AUTO" at the kernel headers,
in a way that we'll be sure that we won't have any bad surprises on userspace.

Regards,
Mauro


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Antti Palosaari Sept. 16, 2012, 12:36 a.m. UTC | #4 
On 09/12/2012 03:45 PM, Mauro Carvalho Chehab wrote:
> Em 12-09-2012 08:01, David Waring escreveu:
>> On 11/09/12 19:38, Mauro Carvalho Chehab wrote:
>>> Em 16-08-2012 22:35, Antti Palosaari escreveu:
>>>> [snip]
>>>> +	<para>Possible values: 0, 1, INT_MIN</para>
>>>
>>> Hmm... INT_MIN... are you sure it is portable on all Linux compilers?
>>>
>>> I don't like the idea on trusting on whatever C/C++/Java/... compiler (or some interpreter)
>>> would define as "INT_MIN".
>>>
>>> The better is to define a value for that, or, instead, to define something
>>> at the API header file that won't cause troubles with 32 bits or 64 bits
>>> userspace, like defining it as:
>>>
>>> #define DVB_AUTO_LNA ((u32)~0)
>>>
>> INT_MIN is defined in limits.h which is an ISO standard header. Other
>> parts of the kernel also use INT_MIN, e.g. linux/cpu.h and
>> linux/netfilter_ipv4.h both reference INT_MIN from limits.h.
>
> The linux/cpu.h is a Kernel internal header. There's no public userspace API
> there. So, it uses kernel's own definition for INT_MIN.
>
> You're right with regards to netfilter. Btw, it is only places where INT_MIN
> is used on an userspace-filtered headers are at the netfilter interface :
>
> /usr/include/linux/netfilter_ipv6.h:#include <limits.h> /* for INT_MIN, INT_MAX */
> /usr/include/linux/netfilter_ipv6.h:	NF_IP6_PRI_FIRST = INT_MIN,
> /usr/include/linux/netfilter_ipv4.h:#include <limits.h> /* for INT_MIN, INT_MAX */
> /usr/include/linux/netfilter_ipv4.h:	NF_IP_PRI_FIRST = INT_MIN,
> /usr/include/linux/netfilter_decnet.h:#include <limits.h> /* for INT_MIN, INT_MAX */
> /usr/include/linux/netfilter_decnet.h:	NF_DN_PRI_FIRST = INT_MIN,
>
> Even so, it got renamed inside a priorities enum:
>
> enum nf_ip_hook_priorities {
> 	NF_IP_PRI_FIRST = INT_MIN,
>
> In the case of netfilter, as this is just a priority number, it actually
> make sense to use INT_MIN as the lowest priority, as INT_MIN is the lowest
> number that can be represented there.
>
> What we're doing here is something else: we're defining a special value to be
> interpreted as "AUTO". In this case, if Kernel and userspace disagrees on what
> value should be used, the KAPI will be deadly broken.
>
> I might be wrong, but some C compilers on a few architectures (Tru64 C compiler comes
> on my mind) define "int" as 64 bit integers, with will affect the definition of INT_MIN.
> Ok, in this case, the definition will be compatible, but I'm wondering if some other
> compiler might be doing something else here.
>
> That's why I'm in favor of defining some constant for "AUTO" at the kernel headers,
> in a way that we'll be sure that we won't have any bad surprises on userspace.

Could you say clearly what it should be as I am not very familiar with 
API changes?

Also you mentioned with multistream support API changes that those 
values are unsigned numbers => not negative which makes me more unsure.
http://www.mail-archive.com/linux-media@vger.kernel.org/msg50772.html

I can say I tested it with Python script using value -2147483648 and it 
worked. Maybe it was still due to signed => unsigned conversion.

Antti
diff mbox

Patch

diff --git a/Documentation/DocBook/media/dvb/dvbproperty.xml b/Documentation/DocBook/media/dvb/dvbproperty.xml
index d188be9..2dfa6a0 100644
--- a/Documentation/DocBook/media/dvb/dvbproperty.xml
+++ b/Documentation/DocBook/media/dvb/dvbproperty.xml
@@ -827,6 +827,17 @@  enum fe_interleaving {
 };
 	</programlisting>
 	</section>
+	<section id="DTV-LNA">
+	<title><constant>DTV_LNA</constant></title>
+	<para>Low-noise amplifier.</para>
+	<para>Hardware might offer controllable LNA which can be set manually
+		using that parameter. Usually LNA could be found only from
+		terrestrial devices if at all.</para>
+	<para>Possible values: 0, 1, INT_MIN</para>
+	<para>0, LNA off</para>
+	<para>1, LNA on</para>
+	<para>INT_MIN, LNA auto</para>
+	</section>
 </section>
 	<section id="frontend-property-terrestrial-systems">
 	<title>Properties used on terrestrial delivery systems</title>
@@ -847,6 +858,7 @@  enum fe_interleaving {
 				<listitem><para><link linkend="DTV-GUARD-INTERVAL"><constant>DTV_GUARD_INTERVAL</constant></link></para></listitem>
 				<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
 				<listitem><para><link linkend="DTV-HIERARCHY"><constant>DTV_HIERARCHY</constant></link></para></listitem>
+				<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
 			</itemizedlist>
 		</section>
 		<section id="dvbt2-params">
@@ -870,6 +882,7 @@  enum fe_interleaving {
 			<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-HIERARCHY"><constant>DTV_HIERARCHY</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-DVBT2-PLP-ID"><constant>DTV_DVBT2_PLP_ID</constant></link></para></listitem>
+			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
 		</itemizedlist>
 		</section>
 		<section id="isdbt">
@@ -981,6 +994,7 @@  enum fe_interleaving {
 				<listitem><para><link linkend="DTV-GUARD-INTERVAL"><constant>DTV_GUARD_INTERVAL</constant></link></para></listitem>
 				<listitem><para><link linkend="DTV-TRANSMISSION-MODE"><constant>DTV_TRANSMISSION_MODE</constant></link></para></listitem>
 				<listitem><para><link linkend="DTV-INTERLEAVING"><constant>DTV_INTERLEAVING</constant></link></para></listitem>
+				<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
 			</itemizedlist>
 		</section>
 	</section>
@@ -1001,6 +1015,7 @@  enum fe_interleaving {
 			<listitem><para><link linkend="DTV-INVERSION"><constant>DTV_INVERSION</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-SYMBOL-RATE"><constant>DTV_SYMBOL_RATE</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-INNER-FEC"><constant>DTV_INNER_FEC</constant></link></para></listitem>
+			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
 		</itemizedlist>
 	</section>
 	<section id="dvbc-annex-b-params">
@@ -1015,6 +1030,7 @@  enum fe_interleaving {
 			<listitem><para><link linkend="DTV-FREQUENCY"><constant>DTV_FREQUENCY</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-MODULATION"><constant>DTV_MODULATION</constant></link></para></listitem>
 			<listitem><para><link linkend="DTV-INVERSION"><constant>DTV_INVERSION</constant></link></para></listitem>
+			<listitem><para><link linkend="DTV-LNA"><constant>DTV_LNA</constant></link></para></listitem>
 		</itemizedlist>
 	</section>
 	</section>