lkml.org 
[lkml]   [2012]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: 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




\
 
 \ /
  Last update: 2012-04-19 14:35    [W:0.035 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site