Hi, Mike Saywell wrote: > Apologies if the code is a bit rough, it's the first time I've ever done > any driver code! ;) I have a few comments: > +static int digitv_alps_tded4_pll_set(struct dvb_frontend* fe, struct dvb_frontend_parameters* params, u8* pllbuf) > +{ > + u32 div; > + struct dvb_ofdm_parameters *op = ¶ms->u.ofdm; > + > + #define IF_FREQUENCYx6 217 > + div = (((params->frequency + 83333) * 3) / 500000) + IF_FREQUENCYx6; please don't indent #defines > + if (params->frequency < 470000000) { > + pllbuf[4] = 0x02; > + } no { }, please > + printk(KERN_WARNING "digitv_alps_tded4: Init Error - Can't Reset DVR " > + "(%i)\n", ret); this line wrap looks rather odd, and since you have many lines that are longer it is pointless > + /* FIXME: Should probably choose the frontend intelligently rather than trying each in turn. */ if you can't do it based on PCI/USB ids then there's no other way, so please drop the comment > + if (card->fe != NULL) { > + dprintk ("dvb_bt8xx: a mt352 was not detected on your digitv card\n"); either the != NULL is wrong or the printk > break; > } > break; double break please don't add whitespace at end-of-line Thanks, Johannes