Hi Steve, On Sun, Nov 13, 2005 Steve Toth wrote: > This adds support for the two new DVB-S products. Your patch has a few coding style issues. Would you mind cleaning them up? Most are minor whitespace things, and I feel a bit silly to comment on them, but these are important to some. See http://lwn.net/Articles/145362/. > diff -Nur dvb-kernel/linux/drivers/media/dvb/frontends/cx24123.c dvb-kernel-new/linux/drivers/media/dvb/frontends/cx24123.c > --- dvb-kernel/linux/drivers/media/dvb/frontends/cx24123.c 1969-12-31 19:00:00.000000000 -0500 > +++ dvb-kernel-new/linux/drivers/media/dvb/frontends/cx24123.c 2005-11-11 23:48:31.000000000 -0500 > @@ -0,0 +1,763 @@ > + empty fist line > +/* fixme: remove the undocumted regs */ :-( (plus typo) > +static struct {u8 reg; u8 data;} cx24123_regdata[]= > +{ > + {0x00,0x03}, /* Reset system */ make that: static struct { u8 reg; u8 data; } cx24123_regdata[] = { { 0x00, 0x03 }, /* Reset system */ > +static int cx24123_writereg (struct cx24123_state* state, int reg, int data) no space before ( > + u8 buf [] = { reg, data }; no space before [ > + dprintk ("%s: writereg error (err == %i, reg == 0x%02x," no space before ( > + dprintk("%s: reg=0x%02x writing=0x%x\n",__FUNCTION__,reg,data); need space after , > + struct i2c_msg msg [] = { > + { .addr = state->config->demod_address, .flags = 0, .buf = b0, .len = 1 }, > + { .addr = state->config->demod_address, .flags = I2C_M_RD, .buf = b1, .len = 1 } }; last } on a separate line and align to "struct" > + cx24123_writereg(state,0x0e,cx24123_readreg(state,0x0e)&0x7f); needs space after , and between operators > + switch(fec) > + { > + // Hardware has 5/11 and 3/5 but are never unused > + case FEC_NONE: return cx24123_writereg(state,0x0f,0x01); don't indent case labels > + default: > + return -EOPNOTSUPP; indent one tab > + for(i=0;i<sizeof(cx24123_AGC_vals)/sizeof(cx24123_AGC_vals[0]);i++) where did all the whitespace go? > + if ((cx24123_AGC_vals[i].symbolrate_low <= p->u.qpsk.symbol_rate) && > + (cx24123_AGC_vals[i].symbolrate_high >= p->u.qpsk.symbol_rate) ) wrong indentation on continuation line > + /* Determine the N/A dividers for the requested lband freq (in kHz) */ > + /* 10111 (kHz) Crystal Freq, 10 divider */ > + ndiv = ( ((p->frequency * vco_div) / (10111 / 10) / 2) / 32) & 0x1ff; > + adiv = ( ((p->frequency * vco_div) / (10111 / 10) / 2) % 32) & 0x1f; > + last line adds trailing whitespace > + while ( (cx24123_readreg(state,0x20)&0x40)==0 ) no space after ( > + /* Configure the LNB for 14V */ > + cx24123_writelnbreg(state,0x0, 0x2a); also adds trailing whitespace > + struct cx24123_state *state = (struct cx24123_state*) fe->demodulator_priv; superflous cast from void * > +struct cx24123_config > +{ > + /* the demodulator's i2c address */ > + u8 demod_address; > + > + /* PLL maintenance */ > + int (*pll_init)(struct dvb_frontend* fe); > + int (*pll_set)(struct dvb_frontend* fe, struct dvb_frontend_parameters* params); > + > + /* Need to set device param for start_dma */ > + int (*set_ts_params)(struct dvb_frontend* fe, int is_punctured); why three different indents? > +} cx24123_AGC_vals[] = > +{ > + { > + .symbolrate_low = 1000000, > + .symbolrate_high = 4999999, > + .VCAslope = 0x07, > + .VCAoffset = 0x0f, > + .VGA1offset = 0x1f8, > + .VGA2offset = 0x1f8, > + .VGAprogdata = (2 << 18) | (0x1f8 << 9) | 0x1f8, > + .VCAprogdata = (4 << 18) | (0x07 << 9) | 0x07, > + }, is there any logic in the number of indents here? > + { //bs3 > +// .freq_low = 1178000, > + .freq_low = 1228000, > +// .freq_high = 1295999, > + .freq_high = 1349999, what are the commented out values for? Regards, Johannes
