On Tue, Oct 02, 2018 at 03:26:22PM +0200, Giulio Benetti wrote:
> Il 01/10/2018 11:36, Dan Carpenter ha scritto:
> > Hello Giulio Benetti,
> >
> > The patch 490cda5a3c82: "drm/sun4i: Handle DRM_BUS_FLAG_PIXDATA_*EDGE
> > checking if panel is used." from Jul 18, 2018, leads to the following
> > static checker warning:
> > drivers/gpu/drm/sun4i/sun4i_tcon.c:558 sun4i_tcon0_mode_set_rgb()
> > warn: 'tcon->panel' isn't an ERR_PTR
> >
> > drivers/gpu/drm/sun4i/sun4i_tcon.c
> > 481 const struct drm_display_mode *mode)
> > 482 {
> > 483 unsigned int bp, hsync, vsync;
> > 484 u8 clk_delay;
> > 485 u32 val = 0;
> > 486
> > 487 WARN_ON(!tcon->quirks->has_channel_0);
> > 488
> > 489 tcon->dclk_min_div = 6;
> > 490 tcon->dclk_max_div = 127;
> > 491 sun4i_tcon0_mode_set_common(tcon, mode);
> > 492
> > 493 /* Set dithering if needed */
> > 494 sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
> > ^^^^^^^^^^^^^
>
> Here there should be something like:
> if (!tcon->panel) {
> sun4i_tcon0_mode_set_dithering(tcon, tcon->panel->connector);
> }
>
> > 495
> > 496 /* Adjust clock delay */
> > 497 clk_delay = sun4i_tcon_get_clk_delay(mode, 0);
> > 498 regmap_update_bits(tcon->regs, SUN4I_TCON0_CTL_REG,
> > 499 SUN4I_TCON0_CTL_CLK_DELAY_MASK,
> > 500 SUN4I_TCON0_CTL_CLK_DELAY(clk_delay));
> > 501
> > 502 /*
> > 503 * This is called a backporch in the register documentation,
> > 504 * but it really is the back porch + hsync
> > 505 */
> > 506 bp = mode->crtc_htotal - mode->crtc_hsync_start;
> > 507 DRM_DEBUG_DRIVER("Setting horizontal total %d, backporch %d\n",
> > 508 mode->crtc_htotal, bp);
> > 509
> > 510 /* Set horizontal display timings */
> > 511 regmap_write(tcon->regs, SUN4I_TCON0_BASIC1_REG,
> > 512 SUN4I_TCON0_BASIC1_H_TOTAL(mode->crtc_htotal) |
> > 513 SUN4I_TCON0_BASIC1_H_BACKPORCH(bp));
> > 514
> > 515 /*
> > 516 * This is called a backporch in the register documentation,
> > 517 * but it really is the back porch + hsync
> > 518 */
> > 519 bp = mode->crtc_vtotal - mode->crtc_vsync_start;
> > 520 DRM_DEBUG_DRIVER("Setting vertical total %d, backporch %d\n",
> > 521 mode->crtc_vtotal, bp);
> > 522
> > 523 /* Set vertical display timings */
> > 524 regmap_write(tcon->regs, SUN4I_TCON0_BASIC2_REG,
> > 525 SUN4I_TCON0_BASIC2_V_TOTAL(mode->crtc_vtotal * 2) |
> > 526 SUN4I_TCON0_BASIC2_V_BACKPORCH(bp));
> > 527
> > 528 /* Set Hsync and Vsync length */
> > 529 hsync = mode->crtc_hsync_end - mode->crtc_hsync_start;
> > 530 vsync = mode->crtc_vsync_end - mode->crtc_vsync_start;
> > 531 DRM_DEBUG_DRIVER("Setting HSYNC %d, VSYNC %d\n", hsync, vsync);
> > 532 regmap_write(tcon->regs, SUN4I_TCON0_BASIC3_REG,
> > 533 SUN4I_TCON0_BASIC3_V_SYNC(vsync) |
> > 534 SUN4I_TCON0_BASIC3_H_SYNC(hsync));
> > 535
> > 536 /* Setup the polarity of the various signals */
> > 537 if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > 538 val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE;
> > 539
> > 540 if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > 541 val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE;
> > 542
> > 543 /*
> > 544 * On A20 and similar SoCs, the only way to achieve Positive Edge
> > 545 * (Rising Edge), is setting dclk clock phase to 2/3(240°).
> > 546 * By default TCON works in Negative Edge(Falling Edge),
> > 547 * this is why phase is set to 0 in that case.
> > 548 * Unfortunately there's no way to logically invert dclk through
> > 549 * IO_POL register.
> > 550 * The only acceptable way to work, triple checked with scope,
> > 551 * is using clock phase set to 0° for Negative Edge and set to 240°
> > 552 * for Positive Edge.
> > 553 * On A33 and similar SoCs there would be a 90° phase option,
> > 554 * but it divides also dclk by 2.
> > 555 * Following code is a way to avoid quirks all around TCON
> > 556 * and DOTCLOCK drivers.
> > 557 */
> > 558 if (!IS_ERR(tcon->panel)) {
> > ^^^^^^^^^^^^^^^^^^
> > Unpossible to be an error pointer!
>
> So maybe I should use IS_ERR_OR_NULL() instead of IS_ERR(), or maybe simply:
> if (!tcon->panel) {
>
> Right?
>
> Anyway I'm going to test it with "sparse".
> Is it the same tool you've used for this?
>
This is an unreleased Smatch test because:
1) Some things return error pointers depending on the config.
2) Pointless IS_ERR() checks are quite common.
I haven't looked at the context to see how to fix this. My guess is
that ->panel can't be NULL nor error pointer, so the check should be
removed. But again, I haven't looked at the code.
Btw, when you're mixing error pointers and NULL then normally NULL is a
special kind of success... Like maybe you want to enable some hardware
if it's there and the function returns error pointers if memory
allocations fail but NULL if the hardware isn't there and isn't
required.
regards,
dan carpenter
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
