* [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
@ 2018-03-01 9:59 Dan Carpenter
2018-03-02 14:20 ` jacopo mondi
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2018-03-01 9:59 UTC (permalink / raw)
To: jacopo+renesas; +Cc: linux-media
[ 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
2018-03-01 9:59 [bug report] media: i2c: Copy tw9910 soc_camera sensor driver Dan Carpenter
@ 2018-03-02 14:20 ` jacopo mondi
2018-03-05 7:21 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: jacopo mondi @ 2018-03-02 14:20 UTC (permalink / raw)
To: Dan Carpenter; +Cc: jacopo+renesas, linux-media
Hi Dan,
On Thu, Mar 01, 2018 at 12:59:54PM +0300, Dan Carpenter wrote:
> [ 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
>
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
2018-03-02 14:20 ` jacopo mondi
@ 2018-03-05 7:21 ` Dan Carpenter
2018-03-05 8:51 ` jacopo mondi
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2018-03-05 7:21 UTC (permalink / raw)
To: jacopo mondi; +Cc: jacopo+renesas, linux-media
On Fri, Mar 02, 2018 at 03:20:16PM +0100, jacopo mondi wrote:
> Hi Dan,
>
> On Thu, Mar 01, 2018 at 12:59:54PM +0300, Dan Carpenter wrote:
> > [ 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.
> I would instead keep them for sake of readability
> and accordance with register description in the video decoder manual.
>
> Thanks
> j
>
[ 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));
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
2018-03-05 7:21 ` Dan Carpenter
@ 2018-03-05 8:51 ` jacopo mondi
2018-03-05 9:47 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: jacopo mondi @ 2018-03-05 8:51 UTC (permalink / raw)
To: Dan Carpenter; +Cc: jacopo+renesas, linux-media
Hi Dan,
On Mon, Mar 05, 2018 at 10:21:09AM +0300, Dan Carpenter wrote:
> On Fri, Mar 02, 2018 at 03:20:16PM +0100, jacopo mondi wrote:
> > Hi Dan,
> >
> > On Thu, Mar 01, 2018 at 12:59:54PM +0300, Dan Carpenter wrote:
> > > [ 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.
>
> > 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!
> > Thanks
> > j
> >
>
> [ 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));
>
> 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.
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:
> (((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
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] media: i2c: Copy tw9910 soc_camera sensor driver
2018-03-05 8:51 ` jacopo mondi
@ 2018-03-05 9:47 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2018-03-05 9:47 UTC (permalink / raw)
To: jacopo mondi; +Cc: jacopo+renesas, linux-media
On Mon, Mar 05, 2018 at 09:51:48AM +0100, jacopo mondi wrote:
> In your suggested fix:
>
> > (((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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-03-05 9:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 9:59 [bug report] media: i2c: Copy tw9910 soc_camera sensor driver Dan Carpenter
2018-03-02 14:20 ` jacopo mondi
2018-03-05 7:21 ` Dan Carpenter
2018-03-05 8:51 ` jacopo mondi
2018-03-05 9:47 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.