Re: [Intel-gfx] [PATCH v4] drm/kmb: Add support for KeemBay Display
Hi Anitha.
On Mon, Aug 03, 2020 at 09:02:24PM +, Chrisanthus, Anitha wrote:
> Hi Sam,
> I installed codespell, but the dictionary.txt in
> usr/share/codespell/dictionary.txt
> seems to be different from yours. Mine is version 1.8. Where can I get the
> dictionary.txt
> that you are using?
I dunno.
$ apt info codespell
Package: codespell
Version: 1.16.0-2
Priority: optional
Section: universe/devel
Origin: Ubuntu
Maintainer: Ubuntu Developers
Original-Maintainer: Debian Python Modules Team
Bugs: https://bugs.launchpad.net/ubuntu/+filebug
Installed-Size: 572 kB
Depends: python3, python3-chardet, python3:any
Homepage: https://github.com/codespell-project/codespell/
Download-Size: 118 kB
APT-Manual-Installed: yes
APT-Sources: http://dk.archive.ubuntu.com/ubuntu focal/universe amd64 Packages
Description: Find and fix common misspellings in text files
codespell is designed to find and fix common misspellings in text files.
It is designed primarily for checking misspelled words in source code,
but it can be used with other files as well.
> I have corrected the relevant spelling warnings from your email and have sent
> v5.
The spelling mistakes was the least relevant warnings.
Please see examples in the following.
> > -:146: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open
> > parenthesis
> > #146: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:58:
> > + kmb_clr_bitmask_lcd(kmb, LCD_INT_ENABLE,
> > + LCD_INT_VERT_COMP);
Here we want LCD_INT_VERT_COMP to be aligned right after the opening
'('. It must be indented with a number of tabs followed by the necessary
spaces to achive this indent. Always uses tabs for indent if possible.
So in other words 8 spaces are not OK, then use a tab.
Same goes for similar warnings.
> > -:427: CHECK:LINE_SPACING: Please don't use multiple blank lines
> > #427: FILE: drivers/gpu/drm/kmb/kmb_drv.c:74:
> > +
> > +
> >
Do not use two consecutive blank lines.
> > -:463: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
> > #463: FILE: drivers/gpu/drm/kmb/kmb_drv.c:110:
> > + kmb->sys_clk_mhz = clk_get_rate(kmb_clk.clk_pll0)/100;
> > ^
Spaces around all operatoers - so space before and after '/' here.
Same goes for following warnings of the same type.
> > -:688: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
> > #688: FILE: drivers/gpu/drm/kmb/kmb_drv.c:335:
> > + if (status & LCD_INT_EOF) {
> > +
As the warning says - no empty line after opening '{'
> >
> > -:701: CHECK:CAMELCASE: Avoid CamelCase:
> > #701: FILE: drivers/gpu/drm/kmb/kmb_drv.c:348:
> > + LCD_LAYERn_DMA_CFG
> >
If you have a reason to use CamelCase then this can be ignored.
A good reason could be that this is how it is done in the datasheet.
In this case maybe use LCD_LAYER_N_DMA_CFG or similar.
> > -:957: CHECK:BRACES: braces {} should be used on all arms of this statement
> > #957: FILE: drivers/gpu/drm/kmb/kmb_drv.c:604:
> > + if (adv_bridge == ERR_PTR(-EPROBE_DEFER))
> > [...]
> > + else if (IS_ERR(adv_bridge)) {
> > [...]
If we use {} in one arm of the statement use it in all arms.
This, as the other tidbits, improve readability.
Same for all similar warnings.
> > -:1026: WARNING:UNDOCUMENTED_DT_STRING: DT compatible string
> > "intel,kmb_display" appears un-documented -- check
> > ./Documentation/devicetree/bindings/
> > #1026: FILE: drivers/gpu/drm/kmb/kmb_drv.c:673:
> > + {.compatible = "intel,kmb_display"},
Binding is missing - we cannot apply a driver for an unknown binding.
The binding must be in DT-schema (yaml) format.
> >
> > -:1122: CHECK:UNCOMMENTED_DEFINITION: spinlock_t definition without
> > comment
> > #1122: FILE: drivers/gpu/drm/kmb/kmb_drv.h:35:
> > + spinlock_t irq_lock;
Add comment.
And consider a more specific name like kmb_irq_lock - allows for easier
grepping.
> >
> > -:1360: CHECK:PREFER_KERNEL_TYPES: Prefer kernel type 'u16' over 'uint16_t'
> > #1360: FILE: drivers/gpu/drm/kmb/kmb_dsi.c:95:
> > + uint16_t default_bit_rate_mbps;
As the warning says. This goes again later.
> > -:1947: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written
> > "fg_cfg->sections[i]"
> > #1947: FILE: drivers/gpu/drm/kmb/kmb_dsi.c:682:
> > + if (fg_cfg->sections[i] != NULL)
Hmm, I like the current code. But better please checkpatch here.
I did not go through them all.
The point is that all the warnings from checkpatch should be considered,
and for the most of them they are legit and should be fixed.
Sam
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4] drm/kmb: Add support for KeemBay Display
Hi Anitha.
On Thu, Jul 30, 2020 at 01:44:44PM -0700, Anitha Chrisanthus wrote:
> This is a basic KMS atomic modesetting display driver for KeemBay family of
> SOCs. Driver has no 2D or 3D graphics.It calls into the ADV bridge
> driver at the connector level.
>
> Single CRTC with LCD controller->mipi DSI-> ADV bridge
>
> Only 1080p resolution and single plane is supported at this time.
>
> v2: moved extern to .h, removed license text
> use drm_dev_init, upclassed dev_private, removed HAVE_IRQ.
>
> v3: Squashed all 59 commits to one
>
> v4: review changes from Sam Ravnborg
> renamed dev_p to kmb
> moved clocks under kmb_clock, consolidated clk initializations
> use drmm functions
> use DRM_GEM_CMA_DRIVER_OPS_VMAP
>
I have not found time neither energy to take a look at v4.
But I applied locally and ran it through checkpatch
with my options:
checkpatch -q --emacs --strict --show-types --codespell --codespellfile
/usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt
The options are from the dim script used when maintaining drm-misc-next with
codespell options added.
Please try to reproduce locally and fix relevant warnings.
Sam
-:146: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#146: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:58:
+ kmb_clr_bitmask_lcd(kmb, LCD_INT_ENABLE,
+ LCD_INT_VERT_COMP);
-:173: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#173: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:85:
+ drm_info(dev,
+ "vfp= %d vbp= %d vsyc_len=%d hfp=%d hbp=%d hsync_len=%d\n",
-:194: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#194: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:106:
+ drm_dbg(dev, "%s : %dactive height= %d vbp=%d vfp=%d vsync-w=%d
h-active=%d h-bp=%d h-fp=%d hysnc-l=%d",
+ __func__, __LINE__,
-:199: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#199: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:111:
+ kmb_write_lcd(kmb, LCD_V_ACTIVEHEIGHT,
+ m->crtc_vdisplay - 1);
-:204: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#204: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:116:
+ kmb_write_lcd(kmb, LCD_H_ACTIVEWIDTH,
+ m->crtc_hdisplay - 1);
-:217: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#217: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:129:
+ kmb_write_lcd(kmb,
+ LCD_V_BACKPORCH_EVEN, vm.vback_porch);
-:219: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#219: FILE: drivers/gpu/drm/kmb/kmb_crtc.c:131:
+ kmb_write_lcd(kmb,
+ LCD_V_FRONTPORCH_EVEN, vm.vfront_porch);
-:413: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#413: FILE: drivers/gpu/drm/kmb/kmb_drv.c:60:
+ drm_err(>drm,
+ "Failed to enable MIPI_ECFG clock: %d\n", ret);
-:420: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#420: FILE: drivers/gpu/drm/kmb/kmb_drv.c:67:
+ drm_err(>drm,
+ "Failed to enable MIPI_CFG clock: %d\n", ret);
-:427: CHECK:LINE_SPACING: Please don't use multiple blank lines
#427: FILE: drivers/gpu/drm/kmb/kmb_drv.c:74:
+
+
-:463: CHECK:SPACING: spaces preferred around that '/' (ctx:VxV)
#463: FILE: drivers/gpu/drm/kmb/kmb_drv.c:110:
+ kmb->sys_clk_mhz = clk_get_rate(kmb_clk.clk_pll0)/100;
^
-:470: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#470: FILE: drivers/gpu/drm/kmb/kmb_drv.c:117:
+ drm_err(>drm, "failed to set to clk_lcd to %d\n",
+ KMB_LCD_DEFAULT_CLK);
-:479: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#479: FILE: drivers/gpu/drm/kmb/kmb_drv.c:126:
+ drm_err(>drm, "failed to set to clk_mipi to %d\n",
+ KMB_MIPI_DEFAULT_CLK);
-:506: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#506: FILE: drivers/gpu/drm/kmb/kmb_drv.c:153:
+ drm_err(>drm,
+ "failed to set clk_mipi_cfg to %d\n",
-:511: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#511: FILE: drivers/gpu/drm/kmb/kmb_drv.c:158:
+ drm_info(>drm,
+ "Get clk_mipi_cfg after set = %ld\n", clk);
-:561: CHECK:LINE_SPACING: Please don't use multiple blank lines
#561: FILE: drivers/gpu/drm/kmb/kmb_drv.c:208:
+
+
-:688: CHECK:BRACES: Blank lines aren't necessary after an open brace '{'
#688: FILE: drivers/gpu/drm/kmb/kmb_drv.c:335:
+ if (status & LCD_INT_EOF) {
+
-:701: CHECK:CAMELCASE: Avoid CamelCase:
#701: FILE: drivers/gpu/drm/kmb/kmb_drv.c:348:
+
