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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20120419123314.GP6498@mwanda>
Date:	Thu, 19 Apr 2012 15:33:14 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	volokh <my84@...ru>
Cc:	volokh@...ros.ru, devel@...verdev.osuosl.org,
	Jiri Kosina <trivial@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@...radead.org>,
	Pradheep Shrinivasan <pradheep.sh@...il.com>,
	linux-media@...r.kernel.org
Subject: Re: Subject: [PATCH] [Trivial] Staging: go7007: wis-tw2804 upstyle
 to v4l2

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/