Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
On 14-12-2011 20:57, Antti Palosaari wrote:
Mauro,
Please PULL that new driver to the Kernel 3.3!
Antti
The following changes since commit 6dbc13e364ad49deb9dd93c4ab84af53ffb52435:
mxl5007t: implement .get_if_frequency() (2011-10-10 00:57:07 +0300)
are available in the git repository at:
git://linuxtv.org/anttip/media_tree.git hdic
Antti Palosaari (1):
HDIC HD29L2 DMB-TH demodulator driver
From: Antti Palosaari cr...@iki.fi
Date: Mon, 7 Nov 2011 01:01:13 +0200
Subject: HDIC HD29L2 DMB-TH demodulator driver
Signed-off-by: Antti Palosaari cr...@iki.fi
---
drivers/media/dvb/frontends/Kconfig |7 +
drivers/media/dvb/frontends/Makefile |1 +
drivers/media/dvb/frontends/hd29l2.c | 863
+
drivers/media/dvb/frontends/hd29l2.h | 66 +++
drivers/media/dvb/frontends/hd29l2_priv.h | 314 +++
5 files changed, 1251 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/dvb/frontends/hd29l2.c
create mode 100644 drivers/media/dvb/frontends/hd29l2.h
create mode 100644 drivers/media/dvb/frontends/hd29l2_priv.h
diff --git a/drivers/media/dvb/frontends/Kconfig
b/drivers/media/dvb/frontends/Kconfig
index 4a2d2e6..ebb5ed7 100644
--- a/drivers/media/dvb/frontends/Kconfig
+++ b/drivers/media/dvb/frontends/Kconfig
@@ -404,6 +404,13 @@ config DVB_EC100
help
Say Y when you want to support this frontend.
+config DVB_HD29L2
+ tristate HDIC HD29L2
+ depends on DVB_CORE I2C
+ default m if DVB_FE_CUSTOMISE
+ help
+ Say Y when you want to support this frontend.
+
config DVB_STV0367
tristate ST STV0367 based
depends on DVB_CORE I2C
diff --git a/drivers/media/dvb/frontends/Makefile
b/drivers/media/dvb/frontends/Makefile
index f639f67..00a2063 100644
--- a/drivers/media/dvb/frontends/Makefile
+++ b/drivers/media/dvb/frontends/Makefile
@@ -84,6 +84,7 @@ obj-$(CONFIG_DVB_STV090x) += stv090x.o
obj-$(CONFIG_DVB_STV6110x) += stv6110x.o
obj-$(CONFIG_DVB_ISL6423) += isl6423.o
obj-$(CONFIG_DVB_EC100) += ec100.o
+obj-$(CONFIG_DVB_HD29L2) += hd29l2.o
obj-$(CONFIG_DVB_DS3000) += ds3000.o
obj-$(CONFIG_DVB_MB86A16) += mb86a16.o
obj-$(CONFIG_DVB_MB86A20S) += mb86a20s.o
diff --git a/drivers/media/dvb/frontends/hd29l2.c
b/drivers/media/dvb/frontends/hd29l2.c
new file mode 100644
index 000..a85ed47
--- /dev/null
+++ b/drivers/media/dvb/frontends/hd29l2.c
@@ -0,0 +1,863 @@
+/*
+ * HDIC HD29L2 DMB-TH demodulator driver
+ *
+ * Copyright (C) 2011 Metropolia University of Applied Sciences, Electria RD
+ *
+ * Author: Antti Palosaari cr...@iki.fi
+ *
+ *This program is free software; you can redistribute it and/or modify
+ *it under the terms of the GNU General Public License as published by
+ *the Free Software Foundation; either version 2 of the License, or
+ *(at your option) any later version.
+ *
+ *This program is distributed in the hope that it will be useful,
+ *but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ *GNU General Public License for more details.
+ *
+ *You should have received a copy of the GNU General Public License
+ *along with this program; if not, write to the Free Software
+ *Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include hd29l2_priv.h
+
+int hd29l2_debug;
+module_param_named(debug, hd29l2_debug, int, 0644);
+MODULE_PARM_DESC(debug, Turn on/off frontend debugging (default:off).);
+
+/* write multiple registers */
+static int hd29l2_wr_regs(struct hd29l2_priv *priv, u8 reg, u8 *val, int len)
+{
+ int ret;
+ u8 buf[2+len];
CodingStyle. It should be, instead:
u8 buf[2 + len]
+ struct i2c_msg msg[1] = {
+ {
+ .addr = priv-cfg.i2c_addr,
+ .flags = 0,
+ .len = sizeof(buf),
+ .buf = buf,
+ }
+ };
+
+ buf[0] = 0x00;
+ buf[1] = reg;
+ memcpy(buf[2], val, len);
+
+ ret = i2c_transfer(priv-i2c, msg, 1);
+ if (ret == 1) {
+ ret = 0;
+ } else {
+ warn(i2c wr failed=%d reg=%02x len=%d, ret, reg, len);
+ ret = -EREMOTEIO;
+ }
+
+ return ret;
+}
+
+/* read multiple registers */
+static int hd29l2_rd_regs(struct hd29l2_priv *priv, u8 reg, u8 *val, int len)
+{
+ int ret;
+ u8 buf[2] = { 0x00, reg };
+ struct i2c_msg msg[2] = {
+ {
+ .addr = priv-cfg.i2c_addr,
+ .flags = 0,
+ .len = 2,
+ .buf = buf,
+ }, {
+ .addr = priv-cfg.i2c_addr,
+ .flags = I2C_M_RD,
+ .len = len,
+ .buf = val,
+ }
+ };
+
+
Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
On 20-12-2011 12:52, Antti Palosaari wrote:
On 12/20/2011 03:39 PM, Mauro Carvalho Chehab wrote:
+
+/* ensure modulation validy */
+/* 0=QAM4_NR, 1=QAM4, 2=QAM16, 3=QAM32, 4=QAM64 */
+if (modulation 4) {
Please, don't use magic values.
Instead, it should be something like:
ARRAY_SIZE(reg_mod_vals_tab[0].val)
?
Why 4 and not 8 (or whatever)? You're using 4 to avoid a buffer
overflow on your table. As such, please explicitly code it with
ARRAY_SIZE, instead of using a 4 magic number.
Still, I don't understand why modulation should be QAM64 when the
auto_mode is disabled. Shouldn't it be provided via a DVB property?
API does not support DMB-TH params. See my earlier comment.
Ok, add support for it then ;)
+break;
+case 1: /* QAM4 */
+str_constellation = QAM4;
+c-modulation = QPSK; /* FIXME */
QPSK and QAM4 are two names for the same encoding.
I wasn't 100% sure if those are same or not, thats why added comment.
Maybe we should add #define QAM4 QPSK to API since QAM4 is much more commonly
used.
I think QPSK is seen mostly when dealing with DVB-T.
I would just add a comment at the frontends.h file. Otherwise, we would need
to replace it on every place.
+break;
+case 2:
+str_constellation = QAM16;
+c-modulation = QAM_16;
+break;
+case 3:
+str_constellation = QAM32;
+c-modulation = QAM_32;
+break;
+case 4:
+str_constellation = QAM64;
+c-modulation = QAM_64;
+break;
Please, avoid magic numbers. Instead, use macros for each
value.
I disagree that. Those numbers are coming from demodulator
register value. Same way is used almost every driver that
supports reading current transmission params from the demod.
There are drivers that don't code it well, but it is always preferred
to use macros for register values. Good drivers have it.
Again, don't abuse over the API. Instead, add the proper guard
intervals into it.
API issue again. I did that because DMB-TH was not supported.
Anyhow, I would like to ask why you even mention those as those are commented
clearly to be not correctly?
That is very commonly used method of our demod drivers. Look all existing
DMB-TH drivers and you can see same.
The other DMB drivers are ugly and only support whatever they call auto.
I won't doubt that they implement only a subset of the existing
modulations, FEC's, etc.
I wouldn't be surprised if they only support whatever someone
discovered from sniffing the i2c traffic.
+/* reset demod */
+/* it is recommended to HW reset chip using RST_N pin */
+if (fe-callback) {
+ret = fe-callback(fe, 0, 0, 0);
This looks weird on my eyes. The fe-callback is tuner-dependent.
So, the command you should use there requires a test for the tuner
type.
In other words, if you're needing to use it, the code should be doing
something similar to:
if (fe-callback priv-tuner_type == TUNER_XC2028)
ret = fe-callback(fe, 0, XC2028_TUNER_RESET, 0);
(the actual coding will depend on how do you store the tuner type, and
what are the commands for the tuner you're using)
That's said, it probably makes sense to deprecate fe-callback in favor
or a more generic set of callbacks, like fe-tuner_reset().
Yes it is wrong because there was no DEMOD defined. But, the callback
itself is correctly. IIRC there was only TUNER defined and no DEMOD.
Look callback definition from the API. It is very simply to fix. But at
the time left it like that, because I wanted to avoid touching one file
more. I will fix it properly later (2 line change).
And it was not a bug, it was just my known decision. I just forget to comment
it as FIXME or TODO.
Feel free to touch on other files, provided that you fix that. There's
no default behavior for fe-callback, as it were written in order to
provide ways for the tuner to call the bridge driver for some device-specific
reasons. So, the commands are defined with macros, and the callback code
should be device-specific.
After all as I see there is no big bugs. Those findings are mostly related
of missing DMB-TH API support (and was even commented clearly). And 1-2
CodingStyle issues.
One issue is pure CodingStyle. The other no-API related aren't.
As there is still few other DMB-TH drivers having similar issues already in
the master I don't see why not to add that too. Anyhow, if you see that must
be put to staging until DMB-TH is defined to API it is OK for me.
Please fix the non-API related issues. If you ack to provide us the API
improvements
for DMB for 3.4, and get rid of auto_mode = true for all cases, I'm
ok on merging it after the fixes at drivers/media/dvb.
Regards,
Mauro
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
On 12/20/2011 05:26 PM, Mauro Carvalho Chehab wrote:
On 20-12-2011 12:52, Antti Palosaari wrote:
On 12/20/2011 03:39 PM, Mauro Carvalho Chehab wrote:
+break;
+case 2:
+str_constellation = QAM16;
+c-modulation = QAM_16;
+break;
+case 3:
+str_constellation = QAM32;
+c-modulation = QAM_32;
+break;
+case 4:
+str_constellation = QAM64;
+c-modulation = QAM_64;
+break;
Please, avoid magic numbers. Instead, use macros for each
value.
I disagree that. Those numbers are coming from demodulator
register value. Same way is used almost every driver that
supports reading current transmission params from the demod.
There are drivers that don't code it well, but it is always preferred
to use macros for register values. Good drivers have it.
I still disagree. Are we speaking same issue?
val = read_reg(rgister)
switch (val) {
case 2:
c-modulation = QAM_16;
break;
case 3:
c-modulation = QAM_32;
break;
}
Why I should define macros here?
Or do you mean I should define macros for the selecting correct bits
from the register?
Anyhow, for me that piece of code looks very clear. And it is used
similarly very many drivers.
After all as I see there is no big bugs. Those findings are mostly related
of missing DMB-TH API support (and was even commented clearly). And 1-2
CodingStyle issues.
One issue is pure CodingStyle. The other no-API related aren't.
As there is still few other DMB-TH drivers having similar issues already in
the master I don't see why not to add that too. Anyhow, if you see that must
be put to staging until DMB-TH is defined to API it is OK for me.
Please fix the non-API related issues. If you ack to provide us the API
improvements
for DMB for 3.4, and get rid of auto_mode = true for all cases, I'm
ok on merging it after the fixes at drivers/media/dvb.
If I don't add DMB-TH support to API you will push that to the staging?
Adding those to API is not mission impossible. Interleaver is only new
parameter and all the rest are just extending values. But my time is
limited... and I really would like to finally got Anysee smart card
reader integrated to USB serial first.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
Hi all, On Tuesday 20 December 2011 16:42:53 Antti Palosaari wrote: Adding those to API is not mission impossible. Interleaver is only new parameter and all the rest are just extending values. But my time is limited... and I really would like to finally got Anysee smart card reader integrated to USB serial first. And if it is added we should not forget to discuess whether DMB-TH is the right name. (If this has already been addressed in another thread please point me to it). I know this standard under at least 2 different names: CTTB and DTMB. Which is the one to choose? best regards, -- Patrick Kernel Labs Inc. http://www.kernellabs.com/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
On Tue, Dec 20, 2011 at 10:26 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
On 20-12-2011 12:52, Antti Palosaari wrote:
+ /* reset demod */
+ /* it is recommended to HW reset chip using RST_N pin */
+ if (fe-callback) {
+ ret = fe-callback(fe, 0, 0, 0);
This looks weird on my eyes. The fe-callback is tuner-dependent.
So, the command you should use there requires a test for the tuner
type.
In other words, if you're needing to use it, the code should be doing
something similar to:
if (fe-callback priv-tuner_type == TUNER_XC2028)
ret = fe-callback(fe, 0, XC2028_TUNER_RESET, 0);
(the actual coding will depend on how do you store the tuner type, and
what are the commands for the tuner you're using)
That's said, it probably makes sense to deprecate fe-callback in favor
or a more generic set of callbacks, like fe-tuner_reset().
Yes it is wrong because there was no DEMOD defined. But, the callback
itself is correctly. IIRC there was only TUNER defined and no DEMOD.
Look callback definition from the API. It is very simply to fix. But at
the time left it like that, because I wanted to avoid touching one file
more. I will fix it properly later (2 line change).
And it was not a bug, it was just my known decision. I just forget to
comment it as FIXME or TODO.
Feel free to touch on other files, provided that you fix that. There's
no default behavior for fe-callback, as it were written in order to
provide ways for the tuner to call the bridge driver for some device-specific
reasons. So, the commands are defined with macros, and the callback code
should be device-specific.
This generic callback was written for the BRIDGE driver to be called
by any frontend COMPONENT, not specifically the tuner, perhaps a demod
or LNB, but at the time of writing, we only needed it from the tuner
so the DVB_FRONTEND_COMPONENT_TUNER(0) was the only #define created at
the time. This was written with forward-compatibility in mind, so
lets please not deprecate it unless we actually have to -- I see
additional uses for this coming in the future.
In order to use fe callback properly, please add #define
DVB_FRONTEND_COMPONENT_DEMOD 1 to dvb-core/dvb_frontend.h , and
simply call your callback as fe-callback(adap_state,
DVB_FRONTEND_COMPONENT_DEMOD, CMD, ARG) ... This way, the callback
knows that it is being called by the demod and not the tuner, it is
receiving command CMD with argument ARG.
I can imagine a need one day to perhaps convert the int arg into a
void* arg, but such a need doesn't exist today. I don't think it
gets any more generic than this.
int (*callback)(void *adapter_priv, int component, int cmd, int arg);
Cheers,
Mike
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
On 12/21/2011 12:35 AM, Michael Krufky wrote:
On Tue, Dec 20, 2011 at 10:26 AM, Mauro Carvalho Chehab
mche...@redhat.com wrote:
On 20-12-2011 12:52, Antti Palosaari wrote:
+/* reset demod */
+/* it is recommended to HW reset chip using RST_N pin */
+if (fe-callback) {
+ret = fe-callback(fe, 0, 0, 0);
This looks weird on my eyes. The fe-callback is tuner-dependent.
So, the command you should use there requires a test for the tuner
type.
In other words, if you're needing to use it, the code should be doing
something similar to:
if (fe-callbackpriv-tuner_type == TUNER_XC2028)
ret = fe-callback(fe, 0, XC2028_TUNER_RESET, 0);
(the actual coding will depend on how do you store the tuner type, and
what are the commands for the tuner you're using)
That's said, it probably makes sense to deprecate fe-callback in favor
or a more generic set of callbacks, like fe-tuner_reset().
Yes it is wrong because there was no DEMOD defined. But, the callback
itself is correctly. IIRC there was only TUNER defined and no DEMOD.
Look callback definition from the API. It is very simply to fix. But at
the time left it like that, because I wanted to avoid touching one file
more. I will fix it properly later (2 line change).
And it was not a bug, it was just my known decision. I just forget to comment
it as FIXME or TODO.
Feel free to touch on other files, provided that you fix that. There's
no default behavior for fe-callback, as it were written in order to
provide ways for the tuner to call the bridge driver for some device-specific
reasons. So, the commands are defined with macros, and the callback code
should be device-specific.
This generic callback was written for the BRIDGE driver to be called
by any frontend COMPONENT, not specifically the tuner, perhaps a demod
or LNB, but at the time of writing, we only needed it from the tuner
so the DVB_FRONTEND_COMPONENT_TUNER(0) was the only #define created at
the time. This was written with forward-compatibility in mind, so
lets please not deprecate it unless we actually have to -- I see
additional uses for this coming in the future.
In order to use fe callback properly, please add #define
DVB_FRONTEND_COMPONENT_DEMOD 1 to dvb-core/dvb_frontend.h , and
simply call your callback as fe-callback(adap_state,
DVB_FRONTEND_COMPONENT_DEMOD, CMD, ARG) ... This way, the callback
knows that it is being called by the demod and not the tuner, it is
receiving command CMD with argument ARG.
I can imagine a need one day to perhaps convert the int arg into a
void* arg, but such a need doesn't exist today. I don't think it
gets any more generic than this.
int (*callback)(void *adapter_priv, int component, int cmd, int arg);
Cheers,
Mike
+1 for that. It was just what I also was thinking :) I will add that
demod component define to the internal API and it is fixed properly.
regards
Antti
--
http://palosaari.fi/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver
Mauro, Please PULL that new driver to the Kernel 3.3! Antti The following changes since commit 6dbc13e364ad49deb9dd93c4ab84af53ffb52435: mxl5007t: implement .get_if_frequency() (2011-10-10 00:57:07 +0300) are available in the git repository at: git://linuxtv.org/anttip/media_tree.git hdic Antti Palosaari (1): HDIC HD29L2 DMB-TH demodulator driver drivers/media/dvb/frontends/Kconfig |7 + drivers/media/dvb/frontends/Makefile |1 + drivers/media/dvb/frontends/hd29l2.c | 863 + drivers/media/dvb/frontends/hd29l2.h | 66 +++ drivers/media/dvb/frontends/hd29l2_priv.h | 314 +++ 5 files changed, 1251 insertions(+), 0 deletions(-) create mode 100644 drivers/media/dvb/frontends/hd29l2.c create mode 100644 drivers/media/dvb/frontends/hd29l2.h create mode 100644 drivers/media/dvb/frontends/hd29l2_priv.h -- http://palosaari.fi/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
