Re: Subject: [PATCH] [Trivial] Staging: go7007: wis-tw2804 upstyle to v4l2

[Dan Carpenter <dan.carpenter@oracle.com>]

On Thu, Apr 19, 2012 at 03:26:17PM +0400, volokh wrote:
>  static int write_reg(struct i2c_client *client, u8 reg, u8 value, int channel)
>  {
> -	return i2c_smbus_write_byte_data(client, reg | (channel << 6), value);
> +	int i;
> +
> +	for (i = 0; i < 10; i++)
> +		/*return */if (i2c_smbus_write_byte_data(client,
> +				reg|(channel<<6), value) < 0)
> +			return -1;
> +	return 0;
>  }

There are several style problems with this function.
1) Bogus comment doesn't add any information.
2) Multi-line indents get curly parens, for stlye reasons even
   though it's not needed for semantic reasons.
3) Preserve the return codes from lower levels.
4) Don't return -1.  -1 means -EPERM and this is not a permision
   issue.
5) Put spaces around math operators.  These were correct in the
   original code.

This function should look like:

static int write_reg(struct i2c_client *client, u8 reg, u8 value, int channel)
{
	int ret;
	int i;

	for (i = 0; i < 10; i++) {
		ret = i2c_smbus_write_byte_data(client,	reg | (channel << 6),
						value);
		if (ret)
			return ret;
	}
	return 0;
}

Now that the function is readable, why are we writing to the
register 10 times?

>  
> +/**static u8 read_reg(struct i2c_client *client, u8 reg, int channel)
> +{
> +  return i2c_smbus_read_byte_data(client,reg|(channel<<6));
> +}*/
> +

Bogus comment adds nothing.

>  static int write_regs(struct i2c_client *client, u8 *regs, int channel)
>  {
>  	int i;
>  
>  	for (i = 0; regs[i] != 0xff; i += 2)
> -		if (i2c_smbus_write_byte_data(client,
> -				regs[i] | (channel << 6), regs[i + 1]) < 0)
> +		if (i2c_smbus_write_byte_data(client
> +				, regs[i] | (channel << 6), regs[i + 1]) < 0)

The comma was in the correct place in the original code...  This
change is wrong.

>  			return -1;
>  	return 0;
>  }
>  
> -static int wis_tw2804_command(struct i2c_client *client,
> -				unsigned int cmd, void *arg)
> +static int wis_tw2804_command(
> +	struct i2c_client *client,
> +	unsigned int cmd,
> +	void *arg)

The style was correct in the original code.  This change is wrong.

>  {
> -	struct wis_tw2804 *dec = i2c_get_clientdata(client);
> +	struct v4l2_subdev *sd = i2c_get_clientdata(client);
> +	struct wis_tw2804 *dec = to_state(sd);
> +	int *input;
> +
> +	printk(KERN_INFO"wis-tw2804: call command %d\n", cmd);

This seems like a very spammy printk().  :/  Put a space after the
KERN_INFO.

>  
>  	if (cmd == DECODER_SET_CHANNEL) {
> -		int *input = arg;

The input was better here, where it was declared originally.

> +		printk(KERN_INFO"wis-tw2804: DecoderSetChannel call command %d\n", cmd);
> +
> +		input = arg;
>  
>  		if (*input < 0 || *input > 3) {
> -			printk(KERN_ERR "wis-tw2804: channel %d is not "
> -					"between 0 and 3!\n", *input);
> +			printk(KERN_ERR"wis-tw2804: channel %d is not between 0 and 3!\n", *input);

These kinds of unrelated changes don't belong in a new feature
patch.  Cleanups, fixes, and features don't mix.  In this situation,
I would just leave it as is.  I know checkpatch.pl complains, but
it's up to the maintainer to decide what to do.  If you decide to
change it (in a separate patch) the format would be:

			printk(KERN_ERR
			       "wis-tw2804: channel %d is not between 0 and 3!\n",
			       *input);

When people submit big patches there is a lot to complain about and
they don't get merged.  I'm a hundred lines into the review and I
haven't even got to any changes which matter or are improvements.

regards,
dan carpenter