regis.prevot wrote:
>The patch added knc1 tv star dvb-s support with philips tuner sd1878.
>
>Any comments are welcome.
>
>
I have some comments that I will mark inline below..... My comments are
just surface-level, as I havent messed around inside budget-av yet, so
others may have more to say...
BTW, your patch is whitepace-mangled.... after addressing the comments
from me (and any others) , please re-send your patch as a textfile
attachment.
>Index: linux/drivers/media/dvb/ttpci/budget-av.c
>===================================================================
>RCS file:
>/cvs/video4linux/v4l-dvb/linux/drivers/media/dvb/ttpci/budget-av.c,v
>retrieving revision 1.59
>diff -u -p -r1.59 budget-av.c
>--- linux/drivers/media/dvb/ttpci/budget-av.c 26 Nov 2005 23:46:56
>-0000 1.59
>+++ linux/drivers/media/dvb/ttpci/budget-av.c 22 Dec 2005 14:16:16 -0000
>@@ -864,8 +864,129 @@ static struct tda1004x_config philips_tu
> .request_firmware = philips_tu1216_request_firmware,
> };
>
>+static u8 philips_sd1878_inittab[] = {
>+ 0x01, 0x15,
>+ 0x02, 0x30,
>+ 0x03, 0x00,
>+ 0x04, 0x7d,
>+ 0x05, 0x35,
>+ 0x06, 0x40,
>+ 0x07, 0x00,
>+ 0x08, 0x43,
>+ 0x09, 0x02,
>+ 0x0C, 0x51,
>+ 0x0D, 0x82,
>+ 0x0E, 0x23,
>+ 0x10, 0x3f,
>+ 0x11, 0x84,
>+ 0x12, 0xb9,
>+ 0x15, 0xc9,
>+ 0x16, 0x19,
>+ 0x17, 0x8c,
>+ 0x18, 0x59,
>+ 0x19, 0xf8,
>+ 0x1a, 0xfe,
>+ 0x1c, 0x7f,
>+ 0x1d, 0x00,
>+ 0x1e, 0x00,
>+ 0x1f, 0x50,
>+ 0x20, 0x00,
>+ 0x21, 0x00,
>+ 0x22, 0x00,
>+ 0x23, 0x00,
>+ 0x28, 0x00,
>+ 0x29, 0x28,
>+ 0x2a, 0x14,
>+ 0x2b, 0x0f,
>+ 0x2c, 0x09,
>+ 0x2d, 0x09,
>+ 0x31, 0x1f,
>+ 0x32, 0x19,
>+ 0x33, 0xfc,
>+ 0x34, 0x93,
>+ 0xff, 0xff
>+};
>++static int philips_sd1878_tda8261_pll_set(struct dvb_frontend *fe,
>+ struct i2c_adapter *i2c,
>+ struct dvb_frontend_parameters *params)
>+{
>+ u8 msg[4];
>+ u16 div;
>+ struct i2c_msg tuner_msg =
>{.addr=0x60,.flags=0,.buf=msg,.len=sizeof(msg)};
>+
>+ if((params->frequency < 950000) || (params->frequency > 2150000))
>+ return -EINVAL;
>+
>+ div = (params->frequency + (500 - 1)) / 500;
>+
>+ msg[0] = (div >> 8) & 0x7f;
>+ msg[1] = div & 0xff;
>+ msg[2] = 0xc4;
>+
>+ if (params->frequency < 1250000)
>+ msg[3] = 0;
>+ else if (params->frequency < 1550000)
>+ msg[3] = 0x40;
>+ else if (params->frequency < 2050000)
>+ msg[3] = 0x80;
>+ else if (params->frequency < 2150000)
>+ msg[3] = 0xC0;
>+
>+ if (i2c_transfer(i2c, &tuner_msg, 1) != 1)
>+ return -EIO;
>+
>+ return 0;
>+}
>
>
^^^ Instead of doing this, can you please create a dvb_pll_desc so that
this tuner can be re-used by other modules? Then you can call it here
with something like:
dvb_pll_configure(&philips_sd1878_tda8261, buf, params->frequency, 0);
>+static int philips_sd1878_ci_set_symbol_rate(struct dvb_frontend *fe,
>+ u32 srate, u32 ratio)
>+{
>+ u8 aclk = 0;
>+ u8 bclk = 0;
>+ u8 m1;
>+
>+ aclk = 0xb5;
>+ if (srate < 2000000)
>+ bclk = 0x86;
>+ else if (srate < 5000000)
>+ bclk = 0x89;
>+ else if (srate < 15000000)
>+ bclk = 0x8f;
>+ else if (srate < 45000000)
>+ bclk = 0x95;
>+
>+ m1 = 0x14;
>+ if (srate < 4000000)
>+ m1 = 0x10;
>+
>+ stv0299_writereg(fe, 0x0e, 0x23);
>+ stv0299_writereg(fe, 0x0f, 0x94);
>+ stv0299_writereg(fe, 0x10, 0x39);
>+ stv0299_writereg(fe, 0x13, aclk);
>+ stv0299_writereg(fe, 0x14, bclk);
>+ stv0299_writereg(fe, 0x15, 0xc9);
>+ stv0299_writereg(fe, 0x1f, (ratio >> 16) & 0xff);
>+ stv0299_writereg(fe, 0x20, (ratio >> 8) & 0xff);
>+ stv0299_writereg(fe, 0x21, (ratio) & 0xf0);
>+ stv0299_writereg(fe, 0x0f, 0x80 | m1);
>+
>+ return 0;
>+}
>+static struct stv0299_config philips_sd1878_config = {
>+ .demod_address = 0x68,
>+ .inittab = philips_sd1878_inittab,
>+ .mclk = 88000000UL,
>+ .invert = 0,
>+ .skip_reinit = 0,
>+ .lock_output = STV0229_LOCKOUTPUT_1,
>+ .volt13_op0_op1 = STV0299_VOLT13_OP0,
>+ .min_delay_ms = 100,
>+ .set_symbol_rate = philips_sd1878_ci_set_symbol_rate,
>+ .pll_set = philips_sd1878_tda8261_pll_set,
>+};
>
> static u8 read_pwm(struct budget_av *budget_av)
> {
>@@ -883,10 +1004,13 @@ static u8 read_pwm(struct budget_av *bud
> }
>
> #define SUBID_DVBS_KNC1 0x0010
> #define SUBID_DVBS_KNC1_PLUS 0x0011
> #define SUBID_DVBS_TYPHOON 0x4f56
> #define SUBID_DVBS_CINERGY1200 0x1154
>
>+#define SUBID_DVBS_TV_STAR_CI 0x0016
>+#define SUBID_DVBS_TV_STAR 0x0014
> #define SUBID_DVBC_KNC1 0x0020
> #define SUBID_DVBC_KNC1_PLUS 0x0021
> #define SUBID_DVBC_CINERGY1200 0x1156
>@@ -922,6 +1046,12 @@ static void frontend_init(struct budget_
> }
> break;
>
>+ case SUBID_DVBS_TV_STAR:
>+ case SUBID_DVBS_TV_STAR_CI:
>+ fe = stv0299_attach(&philips_sd1878_config,
>+ &budget_av->budget.i2c_adap);
>+ break;
>+
> case SUBID_DVBS_KNC1_PLUS:
> case SUBID_DVBS_TYPHOON:
> fe = stv0299_attach(&typhoon_config,
>@@ -1169,6 +1299,7 @@ MAKE_BUDGET_INFO(knc1t, "KNC1 DVB-T", BU
> MAKE_BUDGET_INFO(knc1sp, "KNC1 DVB-S Plus", BUDGET_KNC1SP);
> MAKE_BUDGET_INFO(knc1cp, "KNC1 DVB-C Plus", BUDGET_KNC1CP);
> MAKE_BUDGET_INFO(knc1tp, "KNC1 DVB-T Plus", BUDGET_KNC1TP);
>+MAKE_BUDGET_INFO(kncxs, "KNC1 TV STAR DVB-S", BUDGET_TVSTAR);
> MAKE_BUDGET_INFO(cin1200s, "TerraTec Cinergy 1200 DVB-S", BUDGET_CIN1200S);
> MAKE_BUDGET_INFO(cin1200c, "Terratec Cinergy 1200 DVB-C", BUDGET_CIN1200C);
> MAKE_BUDGET_INFO(cin1200t, "Terratec Cinergy 1200 DVB-T", BUDGET_CIN1200T);
>@@ -1178,6 +1309,8 @@ static struct pci_device_id pci_tbl[] =
> MAKE_EXTENSION_PCI(knc1s, 0x1131, 0x0010),
> MAKE_EXTENSION_PCI(knc1s, 0x1894, 0x0010),
> MAKE_EXTENSION_PCI(knc1sp, 0x1131, 0x0011),
>+ MAKE_EXTENSION_PCI(kncxs, 0x1894, 0x0014),
>+ MAKE_EXTENSION_PCI(kncxs, 0x1894, 0x0016),
> MAKE_EXTENSION_PCI(knc1c, 0x1894, 0x0020),
> MAKE_EXTENSION_PCI(knc1cp, 0x1894, 0x0021),
> MAKE_EXTENSION_PCI(knc1t, 0x1894, 0x0030),
>Index: linux/drivers/media/dvb/ttpci/budget.h
>===================================================================
>RCS file: /cvs/video4linux/v4l-dvb/linux/drivers/media/dvb/ttpci/budget.h,v
>retrieving revision 1.25
>diff -u -p -r1.25 budget.h
>--- linux/drivers/media/dvb/ttpci/budget.h 26 Nov 2005 23:46:56 -0000 1.25
>+++ linux/drivers/media/dvb/ttpci/budget.h 22 Dec 2005 14:16:16 -0000
>@@ -95,6 +95,7 @@ static struct saa7146_pci_extension_data
> #define BUDGET_KNC1SP 11
> #define BUDGET_KNC1CP 12
> #define BUDGET_KNC1TP 13
>+#define BUDGET_TVSTAR 14
>
> #define BUDGET_VIDEO_PORTA 0
> #define BUDGET_VIDEO_PORTB 1
>
>
Everything else looks fine to me... Still, I would like someone else to
ACK this, since I dont know the budget-av module as well as others.
Please create that dvb_pll_desc .... It really helps for the future
development of any other devices that might use the same tuner. Of
course, you'll have to #include dvb-pll.h , etc.....
Cheers,
Michael Krufky
