Subject: [PATCH] [Trivial] Staging: go7007: wis-tw2804 upstyle to v4l2
Subsystems (2)
Some update to new v4l2 controls
--- drivers/staging/media/go7007/wis-tw2804.c | 378 +++++++++++++++++------------ 1 files changed, 228 insertions(+), 150 deletions(-)
drivers/staging/media/go7007/wis-tw2804.c +228 -150
@@ -21,10 +21,13 @@#include <linux/videodev2.h> #include <linux/ioctl.h> #include <linux/slab.h> +#include <media/v4l2-subdev.h> +#include <media/v4l2-device.h> #include "wis-i2c.h" struct wis_tw2804 { +struct v4l2_subdev sd;int channel;int norm;int brightness;
@@ -33,6 +36,13 @@ struct wis_tw2804 {int hue; }; +static inline struct wis_tw2804 *to_state (struct v4l2_subdev *sd) +{ + return container_of(sd, struct wis_tw2804, sd); +} + +static int tw2804_log_status (struct v4l2_subdev *sd); +static u8 global_registers[] = {0x39 ,0x00 ,0x3a ,0xff ,
@@ -105,223 +115,291 @@ static u8 channel_registers[] = {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 ; } +/**static u8 read_reg(struct i2c_client *client, u8 reg, int channel) +{ +return i2c_smbus_read_byte_data(client,reg|(channel<<6)); +}*/ +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 )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) { -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);if (cmd ==DECODER_SET_CHANNEL ) { -int *input = arg; +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);return 0 ; } dec->channel = *input; -printk (KERN_DEBUG "wis-tw2804: initializing TW2804 " -"channel %d\n" , dec->channel); -if (dec->channel ==0 && -write_regs (client, global_registers,0 ) <0 ) { -printk (KERN_ERR "wis-tw2804: error initializing " -"TW2804 global registers\n" ); +printk (KERN_DEBUG "wis-tw2804: initializing TW2804 channel %d\n" , dec->channel); +if (dec->channel ==0 &&write_regs (client, + global_registers,0 ) <0 ) { +printk (KERN_ERR "wis-tw2804: error initializing TW2804 global registers\n" );return 0 ; }if (write_regs (client, channel_registers, dec->channel) <0 ) { -printk (KERN_ERR "wis-tw2804: error initializing " -"TW2804 channel %d\n" , dec->channel); +printk (KERN_ERR "wis-tw2804: error initializing TW2804 channel %d\n" , dec->channel);return 0 ; }return 0 ; }if (dec->channel <0 ) { -printk (KERN_DEBUG "wis-tw2804: ignoring command %08x until " -"channel number is set\n" , cmd); +printk (KERN_DEBUG "wis-tw2804: ignoring command %08x until channel number is set\n" , cmd);return 0 ; } -switch (cmd) { -case VIDIOC_S_STD : - { -v4l2_std_id *input = arg; -u8 regs[] = { -0x01 , *input &V4L2_STD_NTSC ?0xc4 :0x84 , -0x09 , *input &V4L2_STD_NTSC ?0x07 :0x04 , -0x0a , *input &V4L2_STD_NTSC ?0xf0 :0x20 , -0x0b , *input &V4L2_STD_NTSC ?0x07 :0x04 , -0x0c , *input &V4L2_STD_NTSC ?0xf0 :0x20 , -0x0d , *input &V4L2_STD_NTSC ?0x40 :0x4a , -0x16 , *input &V4L2_STD_NTSC ?0x00 :0x40 , -0x17 , *input &V4L2_STD_NTSC ?0x00 :0x40 , -0x20 , *input &V4L2_STD_NTSC ?0x07 :0x0f , -0x21 , *input &V4L2_STD_NTSC ?0x07 :0x0f , -0xff ,0xff , - }; -write_regs (client, regs, dec->channel); - dec->norm = *input; -break ; +return 0 ; +} + +static int tw2804_log_status (struct v4l2_subdev *sd) +{ +struct wis_tw2804 *state =to_state (sd); +v4l2_info (sd,"Standard: %s\n" , state->norm ==V4L2_STD_NTSC + ?"NTSC" : state->norm ==V4L2_STD_PAL + ?"PAL" : state->norm ==V4L2_STD_SECAM + ?"SECAM" :"unknown" ); +v4l2_info (sd,"Input: %d\n" , state->channel); +v4l2_info (sd,"Brightness: %d\n" , state->brightness); +v4l2_info (sd,"Contrast: %d\n" , state->contrast); +v4l2_info (sd,"Saturation: %d\n" , state->saturation); +v4l2_info (sd,"Hue: %d\n" , state->hue); +return 0 ; +} + +static int tw2804_queryctrl ( +struct v4l2_subdev *sd, +struct v4l2_queryctrl *query) +{ +static const u32 user_ctrls[] = { +V4L2_CID_USER_CLASS , +V4L2_CID_BRIGHTNESS , +V4L2_CID_CONTRAST , +V4L2_CID_SATURATION , +V4L2_CID_HUE , +0 + }; + +static const u32 *ctrl_classes[] = { + user_ctrls, +NULL + }; + + query->id =v4l2_ctrl_next (ctrl_classes, query->id); + +switch (query->id) { +case V4L2_CID_USER_CLASS : +return v4l2_ctrl_query_fill (query,0 ,0 ,0 ,0 ); +case V4L2_CID_BRIGHTNESS : +return v4l2_ctrl_query_fill (query,0 ,255 ,1 ,128 ); +case V4L2_CID_CONTRAST : +return v4l2_ctrl_query_fill (query,0 ,255 ,1 ,128 ); +case V4L2_CID_SATURATION : +return v4l2_ctrl_query_fill (query,0 ,255 ,1 ,128 ); +case V4L2_CID_HUE : +return v4l2_ctrl_query_fill (query,0 ,255 ,1 ,128 ); +default : +return -EINVAL ; } -case VIDIOC_QUERYCTRL : - { -struct v4l2_queryctrl *ctrl = arg; - -switch (ctrl->id) { -case V4L2_CID_BRIGHTNESS : - ctrl->type =V4L2_CTRL_TYPE_INTEGER ; -strncpy (ctrl->name,"Brightness" ,sizeof (ctrl->name)); - ctrl->minimum =0 ; - ctrl->maximum =255 ; - ctrl->step =1 ; - ctrl->default_value =128 ; - ctrl->flags =0 ; -break ; -case V4L2_CID_CONTRAST : - ctrl->type =V4L2_CTRL_TYPE_INTEGER ; -strncpy (ctrl->name,"Contrast" ,sizeof (ctrl->name)); - ctrl->minimum =0 ; - ctrl->maximum =255 ; - ctrl->step =1 ; - ctrl->default_value =128 ; - ctrl->flags =0 ; -break ; -case V4L2_CID_SATURATION : - ctrl->type =V4L2_CTRL_TYPE_INTEGER ; -strncpy (ctrl->name,"Saturation" ,sizeof (ctrl->name)); - ctrl->minimum =0 ; - ctrl->maximum =255 ; - ctrl->step =1 ; - ctrl->default_value =128 ; - ctrl->flags =0 ; -break ; -case V4L2_CID_HUE : - ctrl->type =V4L2_CTRL_TYPE_INTEGER ; -strncpy (ctrl->name,"Hue" ,sizeof (ctrl->name)); - ctrl->minimum =0 ; - ctrl->maximum =255 ; - ctrl->step =1 ; - ctrl->default_value =128 ; - ctrl->flags =0 ; -break ; - } +} + +static int tw2804_s_ctrl (struct v4l2_subdev *sd,struct v4l2_control *ctrl) +{ +struct wis_tw2804 *dec =to_state (sd); +struct i2c_client *client =v4l2_get_subdevdata (sd); + +int Addr =0x00 ; + +switch (ctrl->id) { +case V4L2_CID_BRIGHTNESS : + Addr =0x12 ;break ; - } -case VIDIOC_S_CTRL : - { -struct v4l2_control *ctrl = arg; - -switch (ctrl->id) { -case V4L2_CID_BRIGHTNESS : -if (ctrl->value >255 ) - dec->brightness =255 ; -else if (ctrl->value <0 ) - dec->brightness =0 ; -else - dec->brightness = ctrl->value; -write_reg (client,0x12 , dec->brightness, dec->channel); -break ; -case V4L2_CID_CONTRAST : -if (ctrl->value >255 ) - dec->contrast =255 ; -else if (ctrl->value <0 ) - dec->contrast =0 ; -else - dec->contrast = ctrl->value; -write_reg (client,0x11 , dec->contrast, dec->channel); -break ; -case V4L2_CID_SATURATION : -if (ctrl->value >255 ) - dec->saturation =255 ; -else if (ctrl->value <0 ) - dec->saturation =0 ; -else - dec->saturation = ctrl->value; -write_reg (client,0x10 , dec->saturation, dec->channel); -break ; -case V4L2_CID_HUE : -if (ctrl->value >255 ) - dec->hue =255 ; -else if (ctrl->value <0 ) - dec->hue =0 ; -else - dec->hue = ctrl->value; -write_reg (client,0x0f , dec->hue, dec->channel); -break ; - } +case V4L2_CID_CONTRAST : + Addr =0x11 ;break ; - } -case VIDIOC_G_CTRL : - { -struct v4l2_control *ctrl = arg; - -switch (ctrl->id) { -case V4L2_CID_BRIGHTNESS : - ctrl->value = dec->brightness; -break ; -case V4L2_CID_CONTRAST : - ctrl->value = dec->contrast; -break ; -case V4L2_CID_SATURATION : - ctrl->value = dec->saturation; -break ; -case V4L2_CID_HUE : - ctrl->value = dec->hue; -break ; - } +case V4L2_CID_SATURATION : + Addr =0x10 ; +break ; +case V4L2_CID_HUE : + Addr =0x0f ;break ; +default : +return -EINVAL ; + } + ctrl->value = ctrl->value >255 + ?255 : (ctrl->value <0 ?0 : ctrl->value); + Addr =write_reg (client, Addr, ctrl->value, dec->channel); + +if (Addr <0 ) { +printk (KERN_INFO "wis_tw2804: can`t set_ctrl value:id=%d;value=%d" + , ctrl->id, ctrl->value); +return Addr; } +printk (KERN_INFO "wis_tw2804: set_ctrl value:id=%d;value=%d" + , ctrl->id, ctrl->value); + +switch (ctrl->id) { +case V4L2_CID_BRIGHTNESS : + dec->brightness = ctrl->value; +break ; +case V4L2_CID_CONTRAST : + dec->contrast = ctrl->value; +break ; +case V4L2_CID_SATURATION : + dec->saturation = ctrl->value; +break ; +case V4L2_CID_HUE : + dec->hue = ctrl->value; +break ;default : +return -EINVAL ; + } +return 0 ; +} + +static int tw2804_g_ctrl (struct v4l2_subdev *sd,struct v4l2_control *ctrl) +{ +struct wis_tw2804 *state =to_state (sd); +/*/ struct i2c_client *client=v4l2_get_subdevdata(sd);*/ + +switch (ctrl->id) { +case V4L2_CID_BRIGHTNESS : + ctrl->value = state->brightness; +/*=read_reg(client,0x12,state->channel);*/ +break ; +case V4L2_CID_CONTRAST : + ctrl->value = state->contrast; +/*=read_reg(client,0x11,state->channel);*/ break ; +case V4L2_CID_SATURATION : + ctrl->value = state->saturation; +/*=read_reg(client,0x10,state->channel);*/ +break ; +case V4L2_CID_HUE : + ctrl->value = state->hue; +/*=read_reg(client,0x0f,state->channel);*/ +break ; +default : +return -EINVAL ; }return 0 ; } +static int tw2804_s_std (struct v4l2_subdev *sd,v4l2_std_id norm) +{ +struct wis_tw2804 *dec =to_state (sd); +struct i2c_client *client =v4l2_get_subdevdata (sd); + +/*/ v4l2_std_id *input=arg;*/ +u8 regs[] = { +0x01 , norm&V4L2_STD_NTSC ?0xc4 :0x84 , +0x09 , norm&V4L2_STD_NTSC ?0x07 :0x04 , +0x0a , norm&V4L2_STD_NTSC ?0xf0 :0x20 , +0x0b , norm&V4L2_STD_NTSC ?0x07 :0x04 , +0x0c , norm&V4L2_STD_NTSC ?0xf0 :0x20 , +0x0d , norm&V4L2_STD_NTSC ?0x40 :0x4a , +0x16 , norm&V4L2_STD_NTSC ?0x00 :0x40 , +0x17 , norm&V4L2_STD_NTSC ?0x00 :0x40 , +0x20 , norm&V4L2_STD_NTSC ?0x07 :0x0f , +0x21 , norm&V4L2_STD_NTSC ?0x07 :0x0f , +0xff ,0xff , + }; +write_regs (client, regs, dec->channel); + dec->norm = norm; +return 0 ; +} + +static const struct v4l2_subdev_core_ops tw2804_core_ops = { + .log_status = tw2804_log_status, + .g_ctrl = tw2804_g_ctrl, + .s_ctrl = tw2804_s_ctrl, + .queryctrl = tw2804_queryctrl, + .s_std = tw2804_s_std, +}; + +/* +static const struct v4l2_subdev_video_ops tw2804_video_ops = { +.s_routing = tw2804_s_video_routing, +.s_fmt = tw2804_s_fmt, +};*/ + +static const struct v4l2_subdev_ops tw2804_ops = { + .core = &tw2804_core_ops, +/*/ .audio = &s2250_audio_ops,*/ +/*/ .video = &s2250_video_ops,*/ +}; +static int wis_tw2804_probe (struct i2c_client *client, -const struct i2c_device_id *id) +const struct i2c_device_id *id) {struct i2c_adapter *adapter = client->adapter;struct wis_tw2804 *dec; +struct v4l2_subdev *sd; + +printk (KERN_DEBUG "wis_tw2804 :probing %s adapter %s" , id->name + , client->adapter->name);if (!i2c_check_functionality (adapter,I2C_FUNC_SMBUS_BYTE_DATA ))return -ENODEV ; dec =kmalloc (sizeof (struct wis_tw2804 ),GFP_KERNEL ); +if (dec ==NULL )return -ENOMEM ; + sd = &dec->sd; dec->channel =-1 ; dec->norm =V4L2_STD_NTSC ; dec->brightness =128 ; dec->contrast =128 ; dec->saturation =128 ; dec->hue =128 ; -i2c_set_clientdata (client, dec); +v4l2_i2c_subdev_init (sd, client, &tw2804_ops); +v4l2_info (sd,"initializing %s at address 0x%x on %s\n" + ,"tw 2804" , client->addr, client->adapter->name); -printk (KERN_DEBUG "wis-tw2804: creating TW2804 at address %d on %s\n" , - client->addr, adapter->name); +printk (KERN_DEBUG "wis-tw2804: creating TW2804 at address %d on %s\n" + , client->addr, adapter->name);return 0 ; }static int wis_tw2804_remove (struct i2c_client *client) { -struct wis_tw2804 *dec =i2c_get_clientdata (client); - -kfree (dec); +struct v4l2_subdev *sd =i2c_get_clientdata (client); +printk (KERN_INFO "wis_tw2804: remove" ); +v4l2_device_unregister_subdev (sd); +kfree (to_state (sd));return 0 ; }
Signature · 1 line
1.7.7.6
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 125
Quoted 11 lines · volokh
static int
write_reg(struct i2c_client *client, u8 reg, u8 value, int channel){drivers/staging/media/go7007/wis-tw2804.c +7
-1-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?
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 131
Quoted 6 lines
drivers/staging/media/go7007/wis-tw2804.c +5
-0+/**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.
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 138
Quoted 9 lines
static int
write_regs(struct i2c_client *client, u8 *regs, int channel){drivers/staging/media/go7007/wis-tw2804.c +2
-2int 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.
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 146
Quoted 10 lines
drivers/staging/media/go7007/wis-tw2804.c +4
-2return -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.
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 152
Quoted 7 lines
{
drivers/staging/media/go7007/wis-tw2804.c +5
-1-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.
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 128
Quoted 3 lines
drivers/staging/media/go7007/wis-tw2804.c +0
-1if (cmd ==DECODER_SET_CHANNEL ) { -int *input = arg;
The input was better here, where it was declared originally.
drivers/staging/media/go7007/wis-tw2804.c static u8 channel_registers[] = { · line 160
Quoted 8 lines
drivers/staging/media/go7007/wis-tw2804.c +4
-2+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