[linux-dvb] [Patch] Added Nova-S-Plus and Nova-SE2 DVB-S support

[Johannes Stezenbach]

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