media: i2c: Copy tw9910 soc_camera sensor driver
[ I know you're just copying files, but you might have a fix for these
since you're looking at the code. - dan ]
Hello Jacopo Mondi,
The patch e0d76c3842ee: "media: i2c: Copy tw9910 soc_camera sensor driver" from Feb 21, 2018, leads to the following static checker warning:
drivers/media/i2c/tw9910.c:395 tw9910_set_hsync()
warn: odd binop '0x260 & 0x7'
drivers/media/i2c/tw9910.c:396 tw9910_set_hsync()
warn: odd binop '0x300 & 0x7'
drivers/media/i2c/tw9910.c:538 tw9910_s_std()
warn: odd binop '0x0 & 0xc'drivers/media/i2c/tw9910.c
374 static int tw9910_set_hsync(struct i2c_client *client) 375 {
376 struct tw9910_priv *priv = to_tw9910(client);
377 int ret;
378
379 /* bit 10 - 3 */
380 ret = i2c_smbus_write_byte_data(client, HSBEGIN,
381 (HSYNC_START & 0x07F8) >> 3);
382 if (ret < 0)
383 return ret;
384
385 /* bit 10 - 3 */
386 ret = i2c_smbus_write_byte_data(client, HSEND,
387 (HSYNC_END & 0x07F8) >> 3);
388 if (ret < 0)
389 return ret;
390
391 /* So far only revisions 0 and 1 have been seen */
392 /* bit 2 - 0 */
393 if (priv->revision == 1)
394 ret = tw9910_mask_set(client, HSLOWCTL, 0x77,
395 (HSYNC_START & 0x0007) << 4 |
^^^^^^^^^^^^^^^^^^^^
396 (HSYNC_END & 0x0007));
^^^^^^^^^^^^^^^^^^^^These always mask to zero.
397
398 return ret;
399 }
[ snip ]
511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm) 512 {
513 struct i2c_client *client = v4l2_get_subdevdata(sd);
514 struct tw9910_priv *priv = to_tw9910(client);
515 const unsigned int hact = 720;
516 const unsigned int hdelay = 15;
^^^^^^^^^^^
517 unsigned int vact;
518 unsigned int vdelay;
519 int ret;
520
521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL)))
522 return -EINVAL;
523
524 priv->norm = norm;
525 if (norm & V4L2_STD_525_60) {
526 vact = 240;
527 vdelay = 18;
528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10);
529 } else {
530 vact = 288;
531 vdelay = 24;
532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00);
533 }
534 if (!ret)
535 ret = i2c_smbus_write_byte_data(client, CROP_HI,
536 ((vdelay >> 2) & 0xc0) |
537 ((vact >> 4) & 0x30) |
538 ((hdelay >> 6) & 0x0c) |
^^^^^^^^^^^15 >> 6 is zero.
539 ((hact >> 8) & 0x03));
540 if (!ret)
541 ret = i2c_smbus_write_byte_data(client, VDELAY_LO,
542 vdelay & 0xff);
543 if (!ret)
544 ret = i2c_smbus_write_byte_data(client, VACTIVE_LO,
545 vact & 0xff);
546
547 return ret;
548 }
regards,
dan carpenter
Hi Dan,
Quoted 2 lines · Dan Carpenter
[ I know you're just copying files, but you might have a fix for these
since you're looking at the code. - dan ]
According to the static analyzer I should simply substitute all those expressions with 0s. I would instead keep them for sake of readability and accordance with register description in the video decoder manual.
Thanks
j
Quoted 95 lines
Hello Jacopo Mondi,
The patch
e0d76c3842ee: "media: i2c: Copy tw9910soc_camerasensor driver" from Feb 21, 2018, leads to the following static checker warning:drivers/media/i2c/tw9910.c:395 tw9910_set_hsync() warn: odd binop '0x260 & 0x7' drivers/media/i2c/tw9910.c:396 tw9910_set_hsync() warn: odd binop '0x300 & 0x7' drivers/media/i2c/tw9910.c:538 tw9910_s_std() warn: odd binop '0x0 & 0xc'
drivers/media/i2c/tw9910.c
374 static inttw9910_set_hsync(struct i2c_client *client)375 { 376 struct tw9910_priv *priv = to_tw9910(client); 377 int ret; 378 379 /* bit 10 - 3 */ 380 ret = i2c_smbus_write_byte_data(client, HSBEGIN, 381 (HSYNC_START & 0x07F8) >> 3); 382 if (ret < 0) 383 return ret; 384 385 /* bit 10 - 3 */ 386 ret = i2c_smbus_write_byte_data(client, HSEND, 387 (HSYNC_END & 0x07F8) >> 3); 388 if (ret < 0) 389 return ret; 390 391 /* So far only revisions 0 and 1 have been seen */ 392 /* bit 2 - 0 */ 393 if (priv->revision == 1) 394 ret = tw9910_mask_set(client, HSLOWCTL, 0x77, 395 (HSYNC_START & 0x0007) << 4 | ^^^^^^^^^^^^^^^^^^^^ 396 (HSYNC_END & 0x0007)); ^^^^^^^^^^^^^^^^^^^^These always mask to zero.
397 398 return ret; 399 }[ snip ]
511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm) 512 { 513 struct i2c_client *client = v4l2_get_subdevdata(sd); 514 struct tw9910_priv *priv = to_tw9910(client); 515 const unsigned int hact = 720; 516 const unsigned int hdelay = 15; ^^^^^^^^^^^ 517 unsigned int vact; 518 unsigned int vdelay; 519 int ret; 520 521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL))) 522 return -EINVAL; 523 524 priv->norm = norm; 525 if (norm & V4L2_STD_525_60) { 526 vact = 240; 527 vdelay = 18; 528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10); 529 } else { 530 vact = 288; 531 vdelay = 24; 532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00); 533 } 534 if (!ret) 535 ret = i2c_smbus_write_byte_data(client, CROP_HI, 536 ((vdelay >> 2) & 0xc0) | 537 ((vact >> 4) & 0x30) | 538 ((hdelay >> 6) & 0x0c) | ^^^^^^^^^^^15 >> 6 is zero.
539 ((hact >> 8) & 0x03)); 540 if (!ret) 541 ret = i2c_smbus_write_byte_data(client, VDELAY_LO, 542 vdelay & 0xff); 543 if (!ret) 544 ret = i2c_smbus_write_byte_data(client, VACTIVE_LO, 545 vact & 0xff); 546 547 return ret; 548 }regards,
dan carpenter
Quoted 8 lines · jacopo mondi
Hi Dan,
Quoted 2 lines · Dan Carpenter
[ I know you're just copying files, but you might have a fix for these
since you're looking at the code. - dan ]According to the static analyzer I should simply substitute all those expressions with 0s.
I really try not to print warnings for stuff which is just white space complaints like that. For example, Smatch ignores inconsistent NULL checking if every caller passes non-NULL parameters or Smatch ignores comparing unsigned with zero if it's just clamping to between zero and max.
Quoted 6 lines
I would instead keep them for sake of readability
and accordance with register description in the video decoder manual.Thanks
j
[ snip ]
Quoted 32 lines
Quoted 32 lines
511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm) 512 { 513 struct i2c_client *client = v4l2_get_subdevdata(sd); 514 struct tw9910_priv *priv = to_tw9910(client); 515 const unsigned int hact = 720; 516 const unsigned int hdelay = 15; ^^^^^^^^^^^ 517 unsigned int vact; 518 unsigned int vdelay; 519 int ret; 520 521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL))) 522 return -EINVAL; 523 524 priv->norm = norm; 525 if (norm & V4L2_STD_525_60) { 526 vact = 240; 527 vdelay = 18; 528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10); 529 } else { 530 vact = 288; 531 vdelay = 24; 532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00); 533 } 534 if (!ret) 535 ret = i2c_smbus_write_byte_data(client, CROP_HI, 536 ((vdelay >> 2) & 0xc0) | 537 ((vact >> 4) & 0x30) | 538 ((hdelay >> 6) & 0x0c) | ^^^^^^^^^^^15 >> 6 is zero.
Quoted 1 line
Quoted 1 line
539 ((hact >> 8) & 0x03));
I looked at the spec and it seems to me that we should doing something like:
(((vdelay >> 8) & 0x3) << 6) | (((vact >> 8) & 0x3) << 4) | (((hedelay >> 8) & 0x3) << 2) | ((hact >> 8) & 0x03);
But this is the first time I've looked and it and I can't even be sure I'm looking in the right place.
regards,
dan carpenter
Hi Dan,
Quoted 7 lines · Dan Carpenter
Quoted 6 lines · jacopo mondi
Hi Dan,
Quoted 2 lines · Dan Carpenter
[ I know you're just copying files, but you might have a fix for these
since you're looking at the code. - dan ]
Quoted 12 lines
Quoted 2 lines
According to the static analyzer I should simply substitute all those expressions with 0s.
I really try not to print warnings for stuff which is just white space complaints like that. For example, Smatch ignores inconsistent NULL checking if every caller passes non-NULL parameters or Smatch ignores comparing unsigned with zero if it's just clamping to between zero and max.
Quoted 3 lines
I would instead keep them for sake of readability
and accordance with register description in the video decoder manual.
Sorry, I did not make myself clear, see below!
Quoted 43 lines
Quoted 3 lines
Thanks
j[ snip ]
Quoted 32 lines
Quoted 32 lines
511 static int tw9910_s_std(struct v4l2_subdev *sd, v4l2_std_id norm) 512 { 513 struct i2c_client *client = v4l2_get_subdevdata(sd); 514 struct tw9910_priv *priv = to_tw9910(client); 515 const unsigned int hact = 720; 516 const unsigned int hdelay = 15; ^^^^^^^^^^^ 517 unsigned int vact; 518 unsigned int vdelay; 519 int ret; 520 521 if (!(norm & (V4L2_STD_NTSC | V4L2_STD_PAL))) 522 return -EINVAL; 523 524 priv->norm = norm; 525 if (norm & V4L2_STD_525_60) { 526 vact = 240; 527 vdelay = 18; 528 ret = tw9910_mask_set(client, VVBI, 0x10, 0x10); 529 } else { 530 vact = 288; 531 vdelay = 24; 532 ret = tw9910_mask_set(client, VVBI, 0x10, 0x00); 533 } 534 if (!ret) 535 ret = i2c_smbus_write_byte_data(client, CROP_HI, 536 ((vdelay >> 2) & 0xc0) | 537 ((vact >> 4) & 0x30) | 538 ((hdelay >> 6) & 0x0c) | ^^^^^^^^^^^15 >> 6 is zero.
Quoted 1 line
Quoted 1 line
539 ((hact >> 8) & 0x03));I looked at the spec and it seems to me that we should doing something like:
Quoted 8 lines
(((vdelay >> 8) & 0x3) << 6) | (((vact >> 8) & 0x3) << 4) | (((hedelay >> 8) & 0x3) << 2) | ((hact >> 8) & 0x03);But this is the first time I've looked and it and I can't even be sure I'm looking in the right place.
That's correct. I admit I haven't looked at the register composition in detail, I just didn't want to substitute the whole expressions with 0s as it hides what values the register is composed of and that was the "accordance with register description" I mentioned...
In your suggested fix:
Quoted 5 lines
(((vdelay >> 8) & 0x3) << 6) | (((vact >> 8) & 0x3) << 4) | (((hedelay >> 8) & 0x3) << 2) | ((hact >> 8) & 0x03);
Won't your analyzer in that case point out that
"15 >> 8 is zero" again? I may have been underestimating it though
Thanks for noticing this!
j
Quoted 4 lines
regards,
dan carpenter
Quoted 11 lines · jacopo mondi
In your suggested fix:
Quoted 5 lines
(((vdelay >> 8) & 0x3) << 6) | (((vact >> 8) & 0x3) << 4) | (((hedelay >> 8) & 0x3) << 2) | ((hact >> 8) & 0x03);Won't your analyzer in that case point out that
"15 >> 8 is zero" again? I may have been underestimating it though
It will complain, yes, but it's a pretty common false positive and I have it in the back of my head to teach the static checker to look for that situation. Eventually I will get around to it.
regards,
dan carpenter