Hi,
On Sat, 2005-07-09 at 11:38 -0400, Taylor Jacob wrote:
> This patch adds support for the LGDT3303 VSB/QAM chipset. I used the same SNR
> code as the LGDT3302 chipset that was recently added to cvs. Any questions
> please feel free to ask. This will add support for the BBTI Air2PC card due to
> ship very soon that utilizes this chipset.
Sending the Files inline would have made commenting easier. Some
"standard" comments follow below:
> /*
> Support for BBTI Technisat Air2PC - ATSC - 3rd Gen
>
> Copyright (C) 2005 Taylor Jacob <rtjacob@xxxxxxxxxxxxx>
>
> 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 <linux/init.h>
> #include <linux/module.h>
> #include <linux/moduleparam.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> #include <asm/div64.h>
>
> #include "dvb_frontend.h"
> #include "lgdt3303.h"
>
> struct lgdt3303_state {
>
> struct i2c_adapter* i2c;
> struct dvb_frontend_ops ops;
> const struct lgdt3303_config* config;
> struct dvb_frontend frontend;
> u8 current_modulation;
>
> /* demodulator private data */
> u8 initialised:1;
Don't use bit fields if not necessary.
> };
>
> static int debug;
> #define dprintk(args...) \
> do { \
> if (debug) printk(KERN_DEBUG "lgdt3303: " args); \
> } while (0)
>
> static int i2c_writebytes (struct lgdt3303_state* state, u8 reg, u8 *buf, u8 len)
> {
> /* probbably a much better way or doing this */
> u8 buf2 [256],x;
Should probably be "u8 buf2[256], x;".
> int err;
> struct i2c_msg msg = { .addr = state->config->demod_address, .flags = 0, .buf = buf2, .len = len + 1 };
>
> buf2[0] = reg;
> for (x = 0 ; x < len ; x++)
> buf2[x+1] = buf[x];
Use memcpy().
>
> if ((err = i2c_transfer (state->i2c, &msg, 1)) != 1) {
> printk ("%s: i2c write error (addr %02x, err == %i)\n",
> __FUNCTION__, state->config->demod_address, err);
> return -EREMOTEIO;
"err" should be returned if it is negative.
> }
>
> return 0;
> }
>
> static int i2c_tunerwritebytes (struct lgdt3303_state* state, u8 *buf, u8 len)
> {
> /* probbably a much better way or doing this */
> int err;
> struct i2c_msg msg = { .addr = state->config->tuner_address, .flags = 0, .buf = buf, .len = len };
>
> if ((err = i2c_transfer (state->i2c, &msg, 1)) != 1) {
> printk ("%s: i2c write error (addr %02x, err == %i)\n",
> __FUNCTION__, state->config->demod_address, err);
> return -EREMOTEIO;
same as above
> }
>
> return 0;
> }
>
> static u8 i2c_readbytes (struct lgdt3303_state* state, u8 reg, u8* buf, u8 len)
> {
> u8 reg2 [] = { reg };
>
> struct i2c_msg msg [] = { { .addr = state->config->demod_address, .flags = 0, .buf = reg2, .len = 1 },
> { .addr = state->config->demod_address, .flags = I2C_M_RD, .buf = buf, .len = len } };
>
> int err;
>
> if ((err = i2c_transfer (state->i2c, msg, 2)) != 2) {
> printk ("%s: i2c read error (addr %02x, err == %i)\n",
> __FUNCTION__, state->config->demod_address, err);
> return -EREMOTEIO;
again
> }
>
> return 0;
> }
>
> static void lgdt3303_soft_reset(struct lgdt3303_state* state)
> {
> u8 buf;
> dprintk("%s\n", __FUNCTION__);
>
> buf = 0x00;
> i2c_writebytes(state,0x02,&buf,1);
Please use some white space to separate parameters, also in many other places.
>
> buf = 0x01;
> i2c_writebytes(state,0x02,&buf,1);
>
> return;
No need for that return.
> }
>
> static int lgdt3303_setup_frontend_parameters (struct dvb_frontend* fe,
> struct dvb_frontend_parameters *p)
Your code mixes foo* bar and foo *bar style. The latter is more common in kernel
code. Please try to keep it consistent.
> {
> struct lgdt3303_state* state = fe->demodulator_priv;
> u32 freq = 0;
> u16 tunerfreq = 0;
> u8 buf[6];
>
> freq = 44000000 + p->frequency;
>
>
> tunerfreq = freq / 62500;
>
> buf[0] = (tunerfreq >> 8) & 0x7F;
> buf[1] = (tunerfreq & 0xFF);
> buf[2] = 0x86;
>
> if (p->frequency < 160000000) {
> buf[3] = 0x01;
> } else if (p->frequency > 445000000) {
> buf[3] = 0x04;
> } else {
> buf[3] = 0x02;
> }
>
> /* Cut and paste from BBTI Windows driver Important? */
> buf[4] = buf[2] | 0x18;
> buf[5] = 0x50;
>
> i2c_tunerwritebytes(state,buf,4);
>
> i2c_tunerwritebytes(state,&buf[4],2);
>
> /* invert clock */
> buf[0] = 0xF3;
> i2c_writebytes(state,0x87,buf,1);
>
> state->current_modulation = p->u.vsb.modulation;
>
> switch (p->u.vsb.modulation)
> {
> case VSB_8:
> buf[0] = 0x03;
> i2c_writebytes(state,0x00,buf,1);
One level of indentation could probably be removed.
> buf[0] = 0x40;
> i2c_writebytes(state,0x0d,buf,1);
> buf[0] = 0x87;
> i2c_writebytes(state,0x0e,buf,1);
> buf[0] = 0x8e;
> i2c_writebytes(state,0x0f,buf,1);
> buf[0] = 0x01;
> i2c_writebytes(state,0x10,buf,1);
> buf[0] = 0x88;
> i2c_writebytes(state,0x47,buf,1);
> buf[0] = 0x14;
> i2c_writebytes(state,0x4c,buf,1);
> break;
>
> case QAM_64:
> buf[0] = 0x00;
> i2c_writebytes(state,0x00,buf,1);
> i2c_writebytes(state,0x0d,buf,1);
> i2c_writebytes(state,0x0e,buf,1);
> i2c_writebytes(state,0x0f,buf,1);
> i2c_writebytes(state,0x10,buf,1);
> buf[0] = 0x63;
> i2c_writebytes(state,0x51,buf,1);
> buf[0] = 0x66;
> i2c_writebytes(state,0x47,buf,1);
> buf[0] = 0x66;
> i2c_writebytes(state,0x48,buf,1);
> buf[0] = 0x1a;
> i2c_writebytes(state,0x4d,buf,1);
> buf[0] = 0x14;
> i2c_writebytes(state,0x4c,buf,1);
> buf[0] = 0x08;
> i2c_writebytes(state,0x49,buf,1);
> buf[0] = 0x9b;
> i2c_writebytes(state,0x4a,buf,1);
> break;
>
> case QAM_256:
> buf[0] = 0x01;
> i2c_writebytes(state,0x00,buf,1);
> buf[0] = 0x00;
> i2c_writebytes(state,0x0d,buf,1);
> i2c_writebytes(state,0x0e,buf,1);
> i2c_writebytes(state,0x0f,buf,1);
> i2c_writebytes(state,0x10,buf,1);
> buf[0] = 0x63;
> i2c_writebytes(state,0x51,buf,1);
> buf[0] = 0x66;
> i2c_writebytes(state,0x47,buf,1);
> buf[0] = 0x66;
> i2c_writebytes(state,0x48,buf,1);
> buf[0] = 0x1a;
> i2c_writebytes(state,0x4d,buf,1);
> buf[0] = 0x14;
> i2c_writebytes(state,0x4c,buf,1);
> buf[0] = 0x08;
> i2c_writebytes(state,0x49,buf,1);
> buf[0] = 0x9b;
> i2c_writebytes(state,0x4a,buf,1);
> break;
> default:
> break;
> }
>
> lgdt3303_soft_reset(state);
>
> return 0;
> }
>
> static int lgdt3303_read_status(struct dvb_frontend* fe, fe_status_t* status)
> {
> struct lgdt3303_state* state = fe->demodulator_priv;
> u8 lock;
> *status = 0;
>
> i2c_readbytes(state,0x1C,&lock,1);
> if (lock & 0x80) {
> *status |= FE_HAS_SIGNAL;
> *status |= FE_HAS_CARRIER;
> }
>
> i2c_readbytes(state,0x58,&lock,1);
> if (lock & 0x01) {
> *status |= FE_HAS_VITERBI;
> }
>
> if (lock & 0x02) {
> *status |= FE_HAS_SYNC;
> *status |= FE_HAS_LOCK;
> }
> return 0;
> }
>
> static int lgdt3303_read_ber(struct dvb_frontend* fe, u32* ber)
> {
>
> /* Not implimented in frontend */
implemented
> *ber = 0;
>
> return 0;
> }
>
> static int lgdt3303_read_signal_strength(struct dvb_frontend* fe, u16* strength)
> {
> struct lgdt3303_state* state = fe->demodulator_priv;
> u8 b[] = {0,0,0};
Only two bytes are needed.
> u16 temp = 0;
>
> i2c_readbytes(state,0x52,&b[0],1);
> i2c_readbytes(state,0x54,&b[1],1);
>
> /* This math was given from example from the windows bbti driver */
> temp = 1700 - ((b[0] & 0x07) << 8 | b[1]);
>
> *strength = temp * (0xFFFF/1700);
>
> return 0;
> }
>
> static int lgdt3303_read_snr(struct dvb_frontend* fe, u16* snr)
> {
>
> struct lgdt3303_state* state = fe->demodulator_priv;
> u8 b[] = {0,0,0};
> u32 noise = 0;
>
> /* This code cut and pasted from the LGDT3302.C driver and ought
> * to be put in a common place at some point */
>
> /*
> * Spec sheet shows formula for SNR_EQ = 10 log10(25 * 24**2 / noise)
> * and SNR_PH = 10 log10(25 * 32**2 / noise) for equalizer and phase tracker
> * respectively. The following tables are built on these formulas.
> * The usual definition is SNR = 20 log10(signal/noise)
> * If the specification is wrong the value retuned is 1/2 the actual SNR in db.
> *
> * This table is a an ordered list of noise values computed by the
> * formula from the spec sheet such that the index into the table
> * starting at 43 or 45 is the SNR value in db. There are duplicate noise
> * value entries at the beginning because the SNR varies more than
> * 1 db for a change of 1 digit in noise at very small values of noise.
> *
> * Examples from SNR_EQ table:
> * noise SNR
> * 0 43
> * 1 42
> * 2 39
> * 3 37
> * 4 36
> * 5 35
> * 6 34
> * 7 33
> * 8 33
> * 9 32
> * 10 32
> * 11 31
> * 12 31
> * 13 30
> */
>
> static const u32 SNR_EQ[] =
> { 1, 2, 2, 2, 3, 3, 4, 4, 5, 7,
> 9, 11, 13, 17, 21, 26, 33, 41, 52, 65,
> 81, 102, 129, 162, 204, 257, 323, 406, 511, 644,
> 810, 1020, 1284, 1616, 2035, 2561, 3224, 4059, 5110, 6433,
> 8098, 10195, 12835, 16158, 20341, 25608, 32238, 40585, 51094, 64323,
> 80978, 101945, 128341, 161571, 203406, 256073, 0x40000
> };
>
> static const u32 SNR_PH[] =
> { 1, 2, 2, 2, 3, 3, 4, 5, 6, 8,
> 10, 12, 15, 19, 23, 29, 37, 46, 58, 73,
> 91, 115, 144, 182, 229, 288, 362, 456, 574, 722,
> 909, 1144, 1440, 1813, 2282, 2873, 3617, 4553, 5732, 7216,
> 9084, 11436, 14396, 18124, 22817, 28724, 36161, 45524, 57312, 72151,
> 90833, 114351, 143960, 181235, 228161, 0x040000
> };
>
> static u32 snr_db; /* index into SNR_EQ[] */
>
> if (state->current_modulation == VSB_8) {
>
> /* Equalizer Mean-Square Error Register for VSB */
>
> i2c_readbytes(state,0x6E,&b[0],1);
> i2c_readbytes(state,0x71,&b[1],1);
> i2c_readbytes(state,0x72,&b[2],1);
>
> noise = ((b[0] & 0x07) << 16) | (b[1] << 8) | b[2];
>
> /*
> * Look up noise value in table.
> * A better search algorithm could be used...
> * watch out there are duplicate entries.
> */
> for (snr_db = 0; snr_db < sizeof(SNR_EQ); snr_db++) {
> if (noise < SNR_EQ[snr_db]) {
> *snr = (43 - snr_db) * (0xFFFF/43);
>
> break;
> }
> }
> } else {
> /* Phase Tracker Mean-Square Error Register for QAM */
>
> i2c_readbytes(state,0x6F,&b[0],1);
> i2c_readbytes(state,0x70,&b[1],1);
> i2c_readbytes(state,0x71,&b[2],1);
> noise = ((b[0] & 7<<3) << 13) | (b[1] << 8) | b[2];
>
> /* Look up noise value in table. */
> for (snr_db = 0; snr_db < sizeof(SNR_PH); snr_db++) {
> if (noise < SNR_PH[snr_db]) {
> *snr = (45 - snr_db) * (0xFFFF/45);
> break;
> }
> }
> }
>
> return 0;
> }
>
> static int lgdt3303_read_ucblocks(struct dvb_frontend* fe, u32* ucblocks)
> {
> struct lgdt3303_state* state = fe->demodulator_priv;
> u8 b[] = {0,0,0};
Same as above.
>
> i2c_readbytes(state,0x8B,&b[0],1);
> i2c_readbytes(state,0x8C,&b[1],1);
>
> *ucblocks = (b[0] << 8) | b[1];
>
> return 0;
> }
>
> static int lgdt3303_sleep(struct dvb_frontend* fe)
> {
> return 0;
> }
Are empty functions needed?
>
> static int lgdt3303_init(struct dvb_frontend* fe)
> {
> return 0;
> }
>
> static int lgdt3303_get_tune_settings(struct dvb_frontend* fe, struct dvb_frontend_tune_settings* fesettings)
> {
> fesettings->min_delay_ms = 500;
> fesettings->step_size = 0;
> fesettings->max_drift = 0;
> return 0;
> }
>
> static void lgdt3303_release(struct dvb_frontend* fe)
> {
> struct lgdt3303_state* state = fe->demodulator_priv;
> kfree(state);
> }
>
> static struct dvb_frontend_ops lgdt3303_ops;
>
> struct dvb_frontend* lgdt3303_attach(const struct lgdt3303_config* config,
> struct i2c_adapter* i2c)
> {
> struct lgdt3303_state* state = NULL;
> u8 buf = 0;
>
> /* allocate memory for the internal state */
> state = kmalloc(sizeof(struct lgdt3303_state), GFP_KERNEL);
> if (state == NULL) goto error;
if (state == NULL)
return NULL;
>
> /* setup the state */
> state->config = config;
> state->i2c = i2c;
> state->current_modulation = VSB_8;
> memcpy(&state->ops, &lgdt3303_ops, sizeof(struct dvb_frontend_ops));
>
> i2c_readbytes(state, 0x85, &buf, 1);
> if (buf != 0x20) goto error;
Please don't put the "goto" onto the same line as the "if".
>
> i2c_readbytes(state, 0x86, &buf, 1);
> if (buf != 0x40) goto error;
>
> /* create dvb_frontend */
> state->frontend.ops = &state->ops;
> state->frontend.demodulator_priv = state;
> return &state->frontend;
>
> error:
> kfree(state);
> return NULL;
> }
>
> static struct dvb_frontend_ops lgdt3303_ops = {
>
> .info = {
> .name = "LGDT3303 VSB/QAM frontend",
> .type = FE_ATSC,
> .frequency_min = 54000000,
> .frequency_max = 860000000,
> /* stepsize is just a guess */
> .frequency_stepsize = 166666,
> .caps = FE_CAN_FEC_1_2 | FE_CAN_FEC_2_3 | FE_CAN_FEC_3_4 |
> FE_CAN_FEC_5_6 | FE_CAN_FEC_7_8 | FE_CAN_FEC_AUTO |
> FE_CAN_8VSB | FE_CAN_QAM_64 | FE_CAN_QAM_256
> },
>
> .release = lgdt3303_release,
>
> .init = lgdt3303_init,
> .sleep = lgdt3303_sleep,
Empty function pointers should probably not be set. I don't know how dvb-core
handles that.
>
> .set_frontend = lgdt3303_setup_frontend_parameters,
> .get_tune_settings = lgdt3303_get_tune_settings,
>
> .read_status = lgdt3303_read_status,
> .read_ber = lgdt3303_read_ber,
> .read_signal_strength = lgdt3303_read_signal_strength,
> .read_snr = lgdt3303_read_snr,
> .read_ucblocks = lgdt3303_read_ucblocks,
>
> };
>
> module_param(debug, int, 0644);
> MODULE_PARM_DESC(debug, "Turn on/off frontend debugging (default:off).");
>
> MODULE_DESCRIPTION("LGDT3303 ATSC (8VSB & ITU J83 AnnexB FEC QAM64/256) demodulator driver");
> MODULE_AUTHOR("Taylor Jacob");
> MODULE_LICENSE("GPL");
>
> EXPORT_SYMBOL(lgdt3303_attach);
Regards,
Andreas
