dri-devel Archive on lore.kernel.org
 help / color / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: "Chrisanthus, Anitha" <anitha.chrisanthus@intel.com>
Cc: "Vetter, Daniel" <daniel.vetter@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Dea, Edmund J" <edmund.j.dea@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v4] drm/kmb: Add support for KeemBay Display
Date: Wed, 5 Aug 2020 20:04:54 +0200
Message-ID: <20200805180454.GA794025@ravnborg.org> (raw)
In-Reply-To: <BY5PR11MB4182B032DC268C4156B4EEE58C4D0@BY5PR11MB4182.namprd11.prod.outlook.com>

Hi Anitha.

On Mon, Aug 03, 2020 at 09:02:24PM +0000, 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 <ubuntu-devel-discuss@lists.ubuntu.com>
Original-Maintainer: Debian Python Modules Team <python-modules-team@lists.alioth.debian.org>
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)/1000000;
> >  	                                                 ^

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: <LCD_LAYERn_DMA_CFG>
> > #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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

      reply index

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-30 20:44 [PATCH v4] Add support for KeemBay DRM driver Anitha Chrisanthus
2020-07-30 20:44 ` [PATCH v4] drm/kmb: Add support for KeemBay Display Anitha Chrisanthus
2020-08-02 15:23   ` Sam Ravnborg
2020-08-03 21:02     ` Chrisanthus, Anitha
2020-08-05 18:04       ` Sam Ravnborg [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200805180454.GA794025@ravnborg.org \
    --to=sam@ravnborg.org \
    --cc=anitha.chrisanthus@intel.com \
    --cc=daniel.vetter@intel.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=edmund.j.dea@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git