AverMedia 6 Eyes AVS6EYES support
- From: Andrew Morton <akpm (at) osdl.org>
- Date: Tue, 28 Mar 2006 14:39:26 -0800
Martin Samuelsson <sam (at) home.se> wrote:
>
> +
> + for (i = 0; i < sizeof(init) / 2; i += 2) {
> + bt866_write(encoder, init[i], init[i+1]);
> + }
You can use
for (i = 0; i < ARRAY_SIZE(init); i++)
here.
Also, please don't use braces arounf a single statment like this.
> + if (*iarg != 0) {
> + return -EINVAL;
> + }
And this.
> +static int bt866_write(struct bt866 *encoder,
> + unsigned char subaddr, unsigned char data)
> +{
> + unsigned char buffer[2];
> + int err;
> +
> + buffer[0] = subaddr;
> + buffer[1] = data;
> +
> + encoder->reg[subaddr] = data;
> +
> + DEBUG(printk
> + ( "%s: write 0x%02X = 0x%02X\n",
> + encoder->i2c->name, subaddr, data));
> +
> + for (err = 0; err < 3;) {
> + if (2 == i2c_master_send(encoder->i2c, buffer, 2))
The `if (constant == expr)' thing hurts my brain. Generally we don't
bother - gcc will generate a warning if someone accidentally uses `='.
> + break;
> + err++;
> + printk(KERN_WARNING "%s: I/O error #%d (write 0x%02x/0x%02x)\n",
> + encoder->i2c->name, err, encoder->addr, subaddr);
> + current->state = TASK_INTERRUPTIBLE;
> + schedule_timeout(HZ/10);
schedule_timeout_interruptible()
> + }
> + if (3 == err) {
See, it just looks weird, sorry.
> +static struct i2c_driver i2c_driver_bt866 = {
> + .driver = {
> + .name = BT866_DEVNAME,
> + },
We can use `.driver.name = BT866_DEVNAME' now. But people do it wither way.
> +static struct i2c_client bt866_client_tmpl =
> +{
> + name: "(nil)",
> + addr: 0,
> + adapter: NULL,
> + driver: &i2c_driver_bt866,
> + usage_count: 0
> +};
.name = value,
> +static int bt866_found_proc(struct i2c_adapter *adapter,
> + int addr, int kind)
> +{
> + struct bt866 *encoder;
> + struct i2c_client *client;
> +
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if(client == NULL)
> + return -ENOMEM;
Please put a space after `if'.
> + memcpy( client, &bt866_client_tmpl, sizeof(*client) );
> + encoder = kzalloc( sizeof(*encoder), GFP_KERNEL );
No spaces after '(' or before ')'
> + if( encoder == NULL ) {
Should be
if (encoder == NULL)
> + kfree( client );
No spaces.
> + if ( adapter->id == I2C_HW_B_ZR36067 )
More.
> +#ifdef MODULE
> +int init_module(void)
> +#else
> +int bt866_init(void)
> +#endif
> +{
> + i2c_add_driver(&i2c_driver_bt866);
> + return 0;
> +}
> +
> +#ifdef MODULE
> +void cleanup_module(void)
> +{
> + i2c_del_driver(&i2c_driver_bt866);
> +}
> +#endif
Not sure what all this is about. Please just do:
static int __init bt866_init(void)
{
i2c_add_driver(&i2c_driver_bt866);
return 0;
}
static void __exit bt866_exit(void)
{
i2c_del_driver(&i2c_driver_bt866);
}
module_init(bt866_init);
module_exit(bt866_exit);
> +static int debug = 0; /* insmod parameter */
Unneeded initialisation to zero.
> +static u8 reg_defaults[ 64 ];
Unneeded spaces.
> +static void init_reg_defaults(void)
> +{
> + u8* table = reg_defaults;
u8 *table = reg_deafults;
> +
> +static u8 ks0127_read( struct ks0127* ks, u8 reg )
static u8 ks0127_read(struct ks0127 *ks, u8 reg)
> +{
> + struct i2c_client *c = ks->client;
> + char val = 0;
> + struct i2c_msg msgs[] = {
> + { c->addr, 0, sizeof(reg), ® },
> + { c->addr, I2C_M_RD | I2C_M_NO_RD_ACK, sizeof(val), &val}};
> + int ret;
> +
> + ret = i2c_transfer(c->adapter, msgs, 2);
> + if (ret != 2)
> + dprintk( "ks0127_write error\n" );
Replace `2' with ARRAY_SIZE(msgs)
> +static void ks0127_write( struct ks0127* ks, u8 reg, u8 val )
static void ks0127_write(struct ks0127 *ks, u8 reg, u8 val)
> +{
> + char msg[] = { reg, val };
> +
> + if ( i2c_master_send(ks->client, msg, sizeof(msg) ) != sizeof(msg) ) {
> + dprintk( "ks0127_write error\n" );
> + }
> + ks->regs[ reg ] = val;
> +}
Coding style would require:
if (i2c_master_send(ks->client, msg, sizeof(msg)) != sizeof(msg))
dprintk("ks0127_write error\n");
ks->regs[reg] = val;
> +static void ks0127_and_or( struct ks0127* ks, u8 reg, u8 and_v, u8 or_v )
static void ks0127_and_or(struct ks0127 *ks, u8 reg, u8 and_v, u8 or_v)
> +{
> + u8 val = ks->regs[ reg ];
extra sapces
> + val = (val & and_v) | or_v;
> + ks0127_write( ks, reg, val );
Ditto
> +static void ks0127_reset( struct ks0127* ks )
static void ks0127_reset(struct ks0127 *ks)
> +{
> + int i;
> + u8* table = reg_defaults;
u8 *table = reg_defaults;
> + ks->ks_type = KS_TYPE_UNKNOWN;
> +
> + dprintk( "ks0127: reset\n" );
extra spaces
> + udelay(1000);
I think you can use msleep(1) here. A one-millisecond busywait is
undesirable.
> + /* initialize all registers to known values (except STAT, 0x21, 0x22, TEST and 0x38,0x39 ) */
> +
> + for(i = 1; i < 33; i++ )
> + ks0127_write( ks, i, table[i] );
> +
> + for(i = 35; i < 40; i++)
> + ks0127_write( ks, i, table[i] );
> +
> + for(i = 41; i < 56; i++)
> + ks0127_write( ks, i, table[i] );
> +
> + for(i = 58; i < 64; i++)
> + ks0127_write( ks, i, table[i] );
Many coding style mistakes.
> +void ks0127_getcrop(struct ks0127 *ks, int *xstart, int *xend, int *ystart, int *yend)
Plese review entire patch, see whether any of its globally-visible symbols
can be made static.
> +static int
> +ks0127_command(struct i2c_client *client, unsigned int cmd, void *arg)
> +{
> + struct ks0127 *ks = i2c_get_clientdata(client);
Coding style is right here.
> + int *iarg = (int*)arg;
> +
> + int status;
> +
> + if( !ks )
But not here.
> + return -ENODEV;
> +
> + switch (cmd) {
> +
> + case DECODER_INIT:
> + dprintk( "ks0127: command DECODER_INIT\n" );
> + ks0127_reset( ks );
etc.
> +
> + int *iarg = arg;
> + int enable = (*iarg != 0);
> + if (enable) {
> + dprintk( "ks0127: command DECODER_ENABLE_OUTPUT on (%d)\n", enable );
> + ks0127_and_or( ks, KS_OFMTA, 0xcf, 0x30 ); // All output pins on
> + ks0127_and_or( ks, KS_CDEM, 0x7f, 0x00 ); // Obey the OEN pin
> + } else {
> + dprintk( "ks0127: command DECODER_ENABLE_OUTPUT off (%d)\n", enable );
> + ks0127_and_or( ks, KS_OFMTA, 0xcf, 0x00 ); // Video output pins off
> + ks0127_and_or( ks, KS_CDEM, 0x7f, 0x80 ); // Ignore the OEN pin
> + }
Please try to fit things into an 80-col display.
> +
> +static struct i2c_client ks0127_client_tmpl =
> +{
> + .name = "(ks0127 unset)",
> + .addr = 0,
> + .adapter = NULL,
> + .driver = &i2c_driver_ks0127,
> + .usage_count = 0
> +};
.name = value,
> +static int ks0127_found_proc(struct i2c_adapter *adapter, int addr, int kind)
> +{
> + struct ks0127 *ks;
> + struct i2c_client *client;
> +
> + client = kzalloc(sizeof(*client), GFP_KERNEL);
> + if(client == NULL)
> + return -ENOMEM;
> + memcpy( client, &ks0127_client_tmpl, sizeof(*client) );
> +
> + ks = kzalloc( sizeof(*ks), GFP_KERNEL );
> + if( ks == NULL ) {
> + kfree( client );
> + return -ENOMEM;
> + }
coding style.
>
> +static void
> +avs6eyes_init (struct zoran *zr)
Ditto.
--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request (at) redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list