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