Re: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
Thanks for the review.
Inlined questions before going further.
Best Regards
-Bud
2014-05-18 4:03 GMT+09:00 Antti Palosaari cr...@iki.fi:
Overall tc90522 driver looks very complex and there was multiple issues. One
reason of complexiness is that HW algo used. I cannot see any reason why it
is used, just change default SW algo and implement things more likely others
are doing.
diff --git a/drivers/media/dvb-frontends/tc90522.c
b/drivers/media/dvb-frontends/tc90522.c
new file mode 100644
index 000..a767600
--- /dev/null
+++ b/drivers/media/dvb-frontends/tc90522.c
@@ -0,0 +1,539 @@
+/*
+ * Earthsoft PT3 demodulator frontend Toshiba TC90522XBG
OFDM(ISDB-T)/8PSK(ISDB-S)
That is, or at least should be, general DTV demod driver. So lets call it
Toshiba TC90522 or whatever the chipset name is.
FYI, the only document available is SDK from PT3 card maker, Earthsoft.
No guarantee this driver works in other cards.
+int tc90522_write(struct dvb_frontend *fe, const u8 *data, int len)
+{
+ struct tc90522 *demod = fe-demodulator_priv;
+ struct i2c_msg msg[3];
+ u8 buf[6];
+
+ if (data) {
+ msg[0].addr = demod-addr_demod;
+ msg[0].buf = (u8 *)data;
+ msg[0].flags = 0; /* write */
+ msg[0].len = len;
+
+ return i2c_transfer(demod-i2c, msg, 1) == 1 ? 0 :
-EREMOTEIO;
+ } else {
+ u8 addr_tuner = (len 8) 0xff,
+ addr_data = len 0xff;
+ if (len 16) {/* read tuner
without address */
+ buf[0] = TC90522_PASSTHROUGH;
+ buf[1] = (addr_tuner 1) | 1;
+ msg[0].buf = buf;
+ msg[0].len = 2;
+ msg[0].addr = demod-addr_demod;
+ msg[0].flags = 0; /* write */
+
+ msg[1].buf = buf + 2;
+ msg[1].len = 1;
+ msg[1].addr = demod-addr_demod;
+ msg[1].flags = I2C_M_RD;/* read */
+
+ return i2c_transfer(demod-i2c, msg, 2) == 2 ?
buf[2] : -EREMOTEIO;
+ } else {/* read tuner */
+ buf[0] = TC90522_PASSTHROUGH;
+ buf[1] = addr_tuner 1;
+ buf[2] = addr_data;
+ msg[0].buf = buf;
+ msg[0].len = 3;
+ msg[0].addr = demod-addr_demod;
+ msg[0].flags = 0; /* write */
+
+ buf[3] = TC90522_PASSTHROUGH;
+ buf[4] = (addr_tuner 1) | 1;
+ msg[1].buf = buf + 3;
+ msg[1].len = 2;
+ msg[1].addr = demod-addr_demod;
+ msg[1].flags = 0; /* write */
+
+ msg[2].buf = buf + 5;
+ msg[2].len = 1;
+ msg[2].addr = demod-addr_demod;
+ msg[2].flags = I2C_M_RD;/* read */
+
+ return i2c_transfer(demod-i2c, msg, 3) == 3 ?
buf[5] : -EREMOTEIO;
+ }
+ }
+}
That routine is mess. I read it many times without understanding what it
does, when and why. It is not register write over I2C as expected. For
example parameter named len is abused for tuner I2C even that is demod
driver...
Tuners need to read data through demod, and there is no read callback available
in dvb_frontend.h (only write is provided).
The above routine provides R/W access.
+int tc90522_write_data(struct dvb_frontend *fe, u8 addr_data, u8 *data,
u8 len)
+{
+ u8 buf[len + 1];
+ buf[0] = addr_data;
+ memcpy(buf + 1, data, len);
+ return tc90522_write(fe, buf, len + 1);
+}
+
+int tc90522_read(struct tc90522 *demod, u8 addr, u8 *buf, u8 buflen)
+{
+ struct i2c_msg msg[2];
+ if (!buf || !buflen)
+ return -EINVAL;
+
+ buf[0] = addr;
+ msg[0].addr = demod-addr_demod;
+ msg[0].flags = 0; /* write */
+ msg[0].buf = buf;
just give a addr pointer, no need to store it to buf first.
+ msg[0].len = 1;
+
+ msg[1].addr = demod-addr_demod;
+ msg[1].flags = I2C_M_RD;/* read */
+ msg[1].buf = buf;
+ msg[1].len = buflen;
+
+ return i2c_transfer(demod-i2c, msg, 2) == 2 ? 0 : -EREMOTEIO;
+}
All in all, it looks like demod is using just most typical register access
for both register write and read, where first byte is register address and
value(s) are after that. Register read is done using repeated START.
I encourage you to use RegMap API as it covers all that boilerplate stuff -
and forces you implement things correctly (no
Re: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
Moikka!
On 05/20/2014 01:19 AM, ほち wrote:
Thanks for the review.
Inlined questions before going further.
Lets see, I will answer my best. But one thing, that is very big
driver/patch, containing one PCI driver, one (or two?) demod drivers and
two tuner drivers. So it could take a some time to get reviewed those
all. I cannot even review PCI driver as I have simply no knowledge, but
I will review that demod (and hope someone else could take care of those
tuner drivers).
Best Regards
-Bud
2014-05-18 4:03 GMT+09:00 Antti Palosaari cr...@iki.fi:
Overall tc90522 driver looks very complex and there was multiple issues. One
reason of complexiness is that HW algo used. I cannot see any reason why it
is used, just change default SW algo and implement things more likely others
are doing.
diff --git a/drivers/media/dvb-frontends/tc90522.c
b/drivers/media/dvb-frontends/tc90522.c
new file mode 100644
index 000..a767600
--- /dev/null
+++ b/drivers/media/dvb-frontends/tc90522.c
@@ -0,0 +1,539 @@
+/*
+ * Earthsoft PT3 demodulator frontend Toshiba TC90522XBG
OFDM(ISDB-T)/8PSK(ISDB-S)
That is, or at least should be, general DTV demod driver. So lets call it
Toshiba TC90522 or whatever the chipset name is.
FYI, the only document available is SDK from PT3 card maker, Earthsoft.
No guarantee this driver works in other cards.
+int tc90522_write(struct dvb_frontend *fe, const u8 *data, int len)
+{
+ struct tc90522 *demod = fe-demodulator_priv;
+ struct i2c_msg msg[3];
+ u8 buf[6];
+
+ if (data) {
+ msg[0].addr = demod-addr_demod;
+ msg[0].buf = (u8 *)data;
+ msg[0].flags = 0; /* write */
+ msg[0].len = len;
+
+ return i2c_transfer(demod-i2c, msg, 1) == 1 ? 0 :
-EREMOTEIO;
+ } else {
+ u8 addr_tuner = (len 8) 0xff,
+ addr_data = len 0xff;
+ if (len 16) {/* read tuner
without address */
+ buf[0] = TC90522_PASSTHROUGH;
+ buf[1] = (addr_tuner 1) | 1;
+ msg[0].buf = buf;
+ msg[0].len = 2;
+ msg[0].addr = demod-addr_demod;
+ msg[0].flags = 0; /* write */
+
+ msg[1].buf = buf + 2;
+ msg[1].len = 1;
+ msg[1].addr = demod-addr_demod;
+ msg[1].flags = I2C_M_RD;/* read */
+
+ return i2c_transfer(demod-i2c, msg, 2) == 2 ?
buf[2] : -EREMOTEIO;
+ } else {/* read tuner */
+ buf[0] = TC90522_PASSTHROUGH;
+ buf[1] = addr_tuner 1;
+ buf[2] = addr_data;
+ msg[0].buf = buf;
+ msg[0].len = 3;
+ msg[0].addr = demod-addr_demod;
+ msg[0].flags = 0; /* write */
+
+ buf[3] = TC90522_PASSTHROUGH;
+ buf[4] = (addr_tuner 1) | 1;
+ msg[1].buf = buf + 3;
+ msg[1].len = 2;
+ msg[1].addr = demod-addr_demod;
+ msg[1].flags = 0; /* write */
+
+ msg[2].buf = buf + 5;
+ msg[2].len = 1;
+ msg[2].addr = demod-addr_demod;
+ msg[2].flags = I2C_M_RD;/* read */
+
+ return i2c_transfer(demod-i2c, msg, 3) == 3 ?
buf[5] : -EREMOTEIO;
+ }
+ }
+}
That routine is mess. I read it many times without understanding what it
does, when and why. It is not register write over I2C as expected. For
example parameter named len is abused for tuner I2C even that is demod
driver...
Tuners need to read data through demod, and there is no read callback available
in dvb_frontend.h (only write is provided).
The above routine provides R/W access.
That is called I2C-gate. I2C bus is wired through demodulator to tuner
and there is gate which is controlled/owned by demod. That's very norm
coupling. It is to prevent digital noise traveling to tuner IC and harm
performance.
IMHO, the most correct implementation solution is use of I2C mux.
https://i2c.wiki.kernel.org/index.php/I2C_bus_multiplexing
See implementation example from rtl2832 and si2168 drivers.
+int tc90522_write_data(struct dvb_frontend *fe, u8 addr_data, u8 *data,
u8 len)
+{
+ u8 buf[len + 1];
+ buf[0] = addr_data;
+ memcpy(buf + 1, data, len);
+ return tc90522_write(fe, buf, len + 1);
+}
+
+int tc90522_read(struct tc90522 *demod, u8 addr, u8 *buf, u8 buflen)
+{
+ struct i2c_msg msg[2];
+ if (!buf || !buflen)
+ return -EINVAL;
+
+ buf[0] = addr;
+ msg[0].addr = demod-addr_demod;
+
Re: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
Hi Bud, It seems that the tuner modules use fe-ops.write() for i2c communications, which is set by parent module. but I'm afraid fe-ops.write() is not for the purpose. Shouldn't they use i2c_adapters passed from _attach() etc. instead? regards, akihiro -- 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: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
On 20.12.2013 01:14, Guest wrote: From: Bud R knightri...@are.ma *** Is this okay? *** No, that is huge patch bomb with a lot of things that should be implement differently. First of all lets take a look of hardware in a level what chips there is and how those are connected. MaxLinear MxL301RF multimode silicon RF tuner Sharp QM1D1C0042 satellite silicon RF tuner Toshiba TC90522 ISDB-S/T demodulator Altera Cyclone IV FPGA, PCI-bridge * Cyclone IV is FPGA, runs custom device vendor specific logic. * TC90522 can stream multiple TS, ISDB-S and ISDB-T, at same time. I am not sure if that device could do it, but it should be taken into account when implementing demod driver. A DVB driver for Earthsoft PT3 (ISDB-S/T) receiver PCI Express cards, based on 1. PT3 chardev driver https://github.com/knight-rider/ptx/tree/master/pt3_drv https://github.com/m-tsudo/pt3 2. PT1/PT2 DVB driver drivers/media/pci/pt1 It behaves similarly as PT1 DVB, plus some tuning enhancements: 1. in addition to the real frequency: ISDB-S : freq. channel ID ISDB-T : freq# (I/O# +128), ch#, ch# +64 for CATV 2. in addition to TSID: ISDB-S : slot# Feature changes: - dropped DKMS standalone compile - dropped verbosity (debug levels), use single level -DDEBUG instead - changed SNR (.read_snr) to CNR (.read_signal_strength) - moved FE to drivers/media/dvb-frontends - moved demodulator tuners to drivers/media/tuners Those are not moved. - translated to standard (?) I2C protocol - dropped unused features The full package (buildable as standalone, DKMS or tree embedded module) is available at https://github.com/knight-rider/ptx/tree/master/pt3_dvb Signed-off-by: Bud R knightri...@are.ma --- drivers/media/dvb-frontends/Kconfig | 10 +- drivers/media/dvb-frontends/Makefile | 2 + drivers/media/dvb-frontends/mxl301rf.c | 332 ++ drivers/media/dvb-frontends/mxl301rf.h | 27 ++ drivers/media/tuners/ drivers/media/dvb-frontends/pt3_common.h | 95 drivers/media/dvb-frontends/qm1d1c0042.c | 413 ++ drivers/media/dvb-frontends/qm1d1c0042.h | 34 ++ drivers/media/tuners/ drivers/media/dvb-frontends/tc90522.c| 724 +++ drivers/media/dvb-frontends/tc90522.h| 48 ++ drivers/media/pci/Kconfig| 2 +- drivers/media/pci/Makefile | 1 + drivers/media/pci/pt3/Kconfig| 10 + drivers/media/pci/pt3/Makefile | 6 + drivers/media/pci/pt3/pt3.c | 543 +++ drivers/media/pci/pt3/pt3.h | 23 + drivers/media/pci/pt3/pt3_dma.c | 335 ++ drivers/media/pci/pt3/pt3_dma.h | 48 ++ drivers/media/pci/pt3/pt3_i2c.c | 183 drivers/media/pci/pt3/pt3_i2c.h | 30 ++ +EXPORT_SYMBOL(mxl301rf_set_freq); +EXPORT_SYMBOL(mxl301rf_set_sleep); You should bind attach tuner directly to the DVB frontend. +EXPORT_SYMBOL(qm1d1c0042_set_freq); +EXPORT_SYMBOL(qm1d1c0042_set_sleep); +EXPORT_SYMBOL(qm1d1c0042_tuner_init); +EXPORT_SYMBOL(tc90522_attach); +EXPORT_SYMBOL(tc90522_init); +EXPORT_SYMBOL(tc90522_set_powers); First of all that driver should be converted to Kernel DVB driver model. It works something like: You have a PCI driver (pt3). Then you call from attach(TC90522) from pt3 in order to get frontend. After that you attach tuner to frontend calling attach(MxL301RF) or/and attach(QM1D1C0042). In that case it is a little bit tricky as you have a *physically* single demod and 2 RF tuners. But what I looked that demod has itself 2 demods integrated to one package which could even operate same time. So, it means you have to register 2 frontends, one for ISDB-S and one for ISDB-T and attach correct tuner per frontend. I know some developers may prefer to registering 2 multimode frontends as a newer single frontend model and then select operating mode using delivery-system command. Anyhow, that makes some extra headache as you should switch RF tuner per selected frontend standard. IMHO better to forgot fuss about single frontend model in that case and switch to older model where is two different standard frontends registered. 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: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
On Tue, Nov 5, 2013 at 5:30 PM, ほち knightri...@are.ma wrote: Michael Krufky mkrufky at linuxtv.org writes: As the DVB maintainer, I am telling you that I won't merge this as a monolithic driver. The standard is to separate the driver into modules where possible, unless there is a valid reason for doing otherwise. I understand that you used the PT1 driver as a reference, but we're trying to enforce a standard of codingstyle within the kernel. I recommend looking at the other DVB drivers as well. OK Sir. Any good / latest examples? There are plenty of DVB drivers to look at under drivers/media/ ... you may notice that there are v4l and dvb device drivers together under this hierarchy. It's easy to tell which drivers support DVB when you look at the source. I could name a few specific ones, but i'd really recommend for you to take a look at a bunch of them. No single driver should be considered a 'prefect example' as they are all under constant maintenance. Also, many of these drivers are for devices that support both v4l and DVB interfaces. One example is the cx23885 driver. Still, please try to look over the entire media tree, as that would give a better idea of how the drivers are structured. Best regards, Mike Krufky -- 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: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
On 06.11.2013 15:14, Michael Krufky wrote: On Tue, Nov 5, 2013 at 5:30 PM, ほち knightri...@are.ma wrote: Michael Krufky mkrufky at linuxtv.org writes: As the DVB maintainer, I am telling you that I won't merge this as a monolithic driver. The standard is to separate the driver into modules where possible, unless there is a valid reason for doing otherwise. I understand that you used the PT1 driver as a reference, but we're trying to enforce a standard of codingstyle within the kernel. I recommend looking at the other DVB drivers as well. OK Sir. Any good / latest examples? There are plenty of DVB drivers to look at under drivers/media/ ... you may notice that there are v4l and dvb device drivers together under this hierarchy. It's easy to tell which drivers support DVB when you look at the source. I could name a few specific ones, but i'd really recommend for you to take a look at a bunch of them. No single driver should be considered a 'prefect example' as they are all under constant maintenance. Also, many of these drivers are for devices that support both v4l and DVB interfaces. One example is the cx23885 driver. Still, please try to look over the entire media tree, as that would give a better idea of how the drivers are structured. I will also try explain that modular chipset driver architecture what I could :) If you look normal digital television device there is always 3 chips, usually those exists in physically, but some cases multiple chips are integrated to same packet. Those chips are: 1) bus interface (USB/PCIe/firewire bridge) 2) demodulator 3) RF tuner (we call it usually just tuner) There has been multiple cases where people has submitted one big driver and afterwards some new devices appeared having same chips. It is almost impossible to separate those drivers afterwards as you will need original hardware and so. That has led to situation we have some overlapping drivers. To avoid these problems, we have specified some rules to new drivers: RFCv2: Second draft of guidelines for submitting patches to linux-media http://lwn.net/Articles/529490/ I search some pictures from that device to see what are used chips. Here is blog having some pictures: http://hidepod.blog.shinobi.jp/iyh-/%E5%98%98%E3%81%A0%EF%BC%81 What I see: 1) PCI-bridge. Custom Altera Cyclone IV FPGA. (heh, that is familiar chip for me. I have used older Cyclone II for some digital technique course exercises). 2) Toshiba demodulator 3) Sharp tuner module (there is some tuner chip inside, which needs driver) So those are the parts and each one needs own driver. 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: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
see inline 2013/11/6 Michael Krufky mkru...@linuxtv.org: responding inline: Mauro Carvalho Chehab asked to put tuner code as an I2C driver, under drivers/media/tuners, frontends at drivers/media/dvb- However, to keep package integrity compatibility with PT1/PT2 user apps, FE etc. are still placed in the same directory. A userspace application doesn't care where file are places within the kernel tree. You are to use standard linux-dvb api's and the codingstyle of the driver shall comply with that of the linux kernel's DVB subsystem, including the proper placement of files. As stated in my (previous) mails, I took PT1 module as a reference. Everything is located in single directory ...pt1/, including the frontends. They are built as a single integrated module, earth-pt1.ko Sure I can split the FEs out to .../dvb-frontends/, build there as a separated (FE only) module that would be hot-linked with the main module. However I'm afraid ( sure) this will only make people confused with complex dependencies leading to annoying bugs... The simpler the better... Guys I need more opinions from other people before splitting the module. IMHO even Linus won't like this... diff --git a/drivers/media/pci/pt3_dvb/Kconfig b/drivers/media/pci/pt3_dvb/Kconfig new file mode 100644 index 000..f9ba00d --- /dev/null +++ b/drivers/media/pci/pt3_dvb/Kconfig @@ -0,0 +1,12 @@ +config PT3_DVB + tristate Earthsoft PT3 cards + depends on DVB_CORE PCI + help + Support for Earthsoft PT3 PCI-Express cards. + + Since these cards have no MPEG decoder onboard, they transmit + only compressed MPEG data over the PCI bus, so you need + an external software decoder to watch TV on your computer. + + Say Y or M if you own such a device and want to use it. Very few of these digital tuner boards have onboard mpeg decoders. We can do without this extra information here. ok, will change to: These cards transmit only compressed MPEG data over the PCI bus. You need external software decoder to watch TV on your computer. diff --git a/drivers/media/pci/pt3_dvb/Makefile b/drivers/media/pci/pt3_dvb/Makefile new file mode 100644 index 000..7087c90 --- /dev/null +++ b/drivers/media/pci/pt3_dvb/Makefile @@ -0,0 +1,6 @@ +pt3_dvb-objs := pt3.o pt3_fe.o pt3_dma.o pt3_tc.o pt3_i2c.o pt3_bus.o + +obj-$(CONFIG_PT3_DVB) += pt3_dvb.o + +ccflags-y += -Idrivers/media/dvb-core -Idrivers/media/dvb-frontends -Idrivers/media/tuners + Ususally, the driver would be named 'pt3.ko' and the object file that handles the DVB api would be called pt3_dvb.o This isn't any rule, but the way that you've named them seems a bit awkward to me. I don't require that you change this, just stating my awkward feeling on the matter. FYI, there is another version of PT3 driver, named pt3_drv.ko, that utilize character devices as the I/O. I'd rather use pt3_dvb.ko to distinguish. Maybe I'd like to change the dirname: drivers/media/pci/pt3_dvb = drivers/media/pci/pt3 +static int lnb = 2;/* used if not set by frontend / the value is invalid */ +module_param(lnb, int, 0); +MODULE_PARM_DESC(lnb, LNB level (0:OFF 1:+11V 2:+15V)); Take these above three statements out of the header file and move them into a .c file OK Sir +struct dvb_frontend *pt3_fe_s_attach(struct pt3_adapter *adap); +struct dvb_frontend *pt3_fe_t_attach(struct pt3_adapter *adap); Please create separate headers corresponding to the .c file that contains the function. Don't put them all in one, as the tuner and demodulator should be separate modules Splitting the protos? Well I will consider... every source file and header file should include GPLv2 license headers. Roger: not very crucial though... +#define PT3_QM_INIT_DUMMY_RESET 0x0c it's nicer when these macros are defined in one place, but its not a requirement. It's OK to leave it here if you really want to, but I suggest instead to create a _reg.h file containing all register #defines Will consider... -- 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: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
On Tue, Nov 5, 2013 at 3:56 PM, ほち knightri...@are.ma wrote: see inline 2013/11/6 Michael Krufky mkru...@linuxtv.org: responding inline: Mauro Carvalho Chehab asked to put tuner code as an I2C driver, under drivers/media/tuners, frontends at drivers/media/dvb- However, to keep package integrity compatibility with PT1/PT2 user apps, FE etc. are still placed in the same directory. A userspace application doesn't care where file are places within the kernel tree. You are to use standard linux-dvb api's and the codingstyle of the driver shall comply with that of the linux kernel's DVB subsystem, including the proper placement of files. As stated in my (previous) mails, I took PT1 module as a reference. Everything is located in single directory ...pt1/, including the frontends. They are built as a single integrated module, earth-pt1.ko Sure I can split the FEs out to .../dvb-frontends/, build there as a separated (FE only) module that would be hot-linked with the main module. However I'm afraid ( sure) this will only make people confused with complex dependencies leading to annoying bugs... The simpler the better... Guys I need more opinions from other people before splitting the module. IMHO even Linus won't like this... As the DVB maintainer, I am telling you that I won't merge this as a monolithic driver. The standard is to separate the driver into modules where possible, unless there is a valid reason for doing otherwise. I understand that you used the PT1 driver as a reference, but we're trying to enforce a standard of codingstyle within the kernel. I recommend looking at the other DVB drivers as well. diff --git a/drivers/media/pci/pt3_dvb/Kconfig b/drivers/media/pci/pt3_dvb/Kconfig new file mode 100644 index 000..f9ba00d --- /dev/null +++ b/drivers/media/pci/pt3_dvb/Kconfig @@ -0,0 +1,12 @@ +config PT3_DVB + tristate Earthsoft PT3 cards + depends on DVB_CORE PCI + help + Support for Earthsoft PT3 PCI-Express cards. + + Since these cards have no MPEG decoder onboard, they transmit + only compressed MPEG data over the PCI bus, so you need + an external software decoder to watch TV on your computer. + + Say Y or M if you own such a device and want to use it. Very few of these digital tuner boards have onboard mpeg decoders. We can do without this extra information here. ok, will change to: These cards transmit only compressed MPEG data over the PCI bus. You need external software decoder to watch TV on your computer. This is superfluous information. Please look at the Kconfig description for the many other DVB drivers supported in the kernel. There are very few that contain their own hardware decoders, and almosy all of them require a software decoder for playback on a PC. Please shorten it to something more along the lines of: Support for Earthsoft PT3 PCI-Express cards. Say Y or M to enable support for this device. diff --git a/drivers/media/pci/pt3_dvb/Makefile b/drivers/media/pci/pt3_dvb/Makefile new file mode 100644 index 000..7087c90 --- /dev/null +++ b/drivers/media/pci/pt3_dvb/Makefile @@ -0,0 +1,6 @@ +pt3_dvb-objs := pt3.o pt3_fe.o pt3_dma.o pt3_tc.o pt3_i2c.o pt3_bus.o + +obj-$(CONFIG_PT3_DVB) += pt3_dvb.o + +ccflags-y += -Idrivers/media/dvb-core -Idrivers/media/dvb-frontends -Idrivers/media/tuners + Ususally, the driver would be named 'pt3.ko' and the object file that handles the DVB api would be called pt3_dvb.o This isn't any rule, but the way that you've named them seems a bit awkward to me. I don't require that you change this, just stating my awkward feeling on the matter. FYI, there is another version of PT3 driver, named pt3_drv.ko, that utilize character devices as the I/O. I'd rather use pt3_dvb.ko to distinguish. we're not interested in multiple drivers for the same hardware. Only one will be merged into the kernel, if any at all, and users need not think about the names of these drivers. One of the beauties of merging a driver into the kernel is that users gain automatic support for the hardware without having to think or care about the name of the driver. Maybe I'd like to change the dirname: drivers/media/pci/pt3_dvb = drivers/media/pci/pt3 not a bad idea ;-) +static int lnb = 2;/* used if not set by frontend / the value is invalid */ +module_param(lnb, int, 0); +MODULE_PARM_DESC(lnb, LNB level (0:OFF 1:+11V 2:+15V)); Take these above three statements out of the header file and move them into a .c file OK Sir +struct dvb_frontend *pt3_fe_s_attach(struct pt3_adapter *adap); +struct dvb_frontend *pt3_fe_t_attach(struct pt3_adapter *adap); Please create separate headers corresponding to the .c file that contains the function. Don't put them all in one, as the tuner and demodulator should be separate modules Splitting the protos? Well I will consider... No need to consider it for very long
Re: [PATCH] Full DVB driver package for Earthsoft PT3 (ISDB-S/T) cards
Michael Krufky mkrufky at linuxtv.org writes: As the DVB maintainer, I am telling you that I won't merge this as a monolithic driver. The standard is to separate the driver into modules where possible, unless there is a valid reason for doing otherwise. I understand that you used the PT1 driver as a reference, but we're trying to enforce a standard of codingstyle within the kernel. I recommend looking at the other DVB drivers as well. OK Sir. Any good / latest examples? Please shorten it to something more along the lines of: Support for Earthsoft PT3 PCI-Express cards. Say Y or M to enable support for this device. Roger FYI, there is another version of PT3 driver, named pt3_drv.ko, that utilize character devices as the I/O. I'd rather use pt3_dvb.ko to distinguish. we're not interested in multiple drivers for the same hardware. Only one will be merged into the kernel, if any at all, and users need not think about the names of these drivers. One of the beauties of merging a driver into the kernel is that users gain automatic support for the hardware without having to think or care about the name of the driver. pt3_drv.ko is a public domain (old-fashioned) chardev driver for PT3, and does not conform to standard DVB platform. It doesn't seem to be merged into the mainstreem kernel tree. Maybe I'd like to change the dirname: drivers/media/pci/pt3_dvb = drivers/media/pci/pt3 not a bad idea every source file and header file should include GPLv2 license headers. Roger: not very crucial though... entirely crucial if you're looking to merge into the kernel. ...or did we misunderstand your request? I meant: not a big task... is the following enough? /* * DVB driver for Earthsoft PT3 PCI-E ISDB-S/T card * * Copyright (C) 2013 xxx...@zng.info * * 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. */ +#define PT3_QM_INIT_DUMMY_RESET 0x0c it's nicer when these macros are defined in one place, but its not a requirement. It's OK to leave it here if you really want to, but I suggest instead to create a _reg.h file containing all register #defines Will consider... -- 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
