linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Woithe <jwoithe@just42.net>
To: Micha?? K??pie?? <kernel@kempniu.pl>
Cc: Darren Hart <dvhart@infradead.org>,
	Andy Shevchenko <andy@infradead.org>,
	platform-driver-x86@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 0/7] fujitsu-laptop: Consistent naming of constants
Date: Sun, 4 Mar 2018 16:07:11 +1030	[thread overview]
Message-ID: <20180304053711.GB3129@marvin.atrad.com.au> (raw)
In-Reply-To: <20180227211539.5708-1-kernel@kempniu.pl>

Hi Michal

On Tue, Feb 27, 2018 at 10:15:32PM +0100, Micha?? K??pie?? wrote:
> This patch series is an attempt to organize all the named constants used
> throughout fujitsu-laptop so that their names more clearly convey their
> purpose: a set of prefixes is introduced to "map" constant names to
> call_fext_func() parameters.

While fairly superficial in nature I think this patch set is worthwhile. 
Features have been progressively added to fujitsu-laptop over the last 10 or
so years from a number of sources but a consistent naming methodology for
constants has not emerged.  Having at least some consistency across the
constant names will help clarify the intent of the code.

> Some changes (like those in patches 4/7 and 5/7) may be perceived as
> bikeshedding, so please just think of them as proposals, not fixes.

Patches 4 and 5 don't bother me within the context of the patch series as a
whole.

> Finally, patch 7/7 again [1] proposes a set of helper functions which
> seem to be making quite a difference in terms of code readability in
> certain places (especially in long conditional expressions).  YMMV,
> though, feel free to disagree.

As before, I can't see any strong arguments one way or the other.  The
helper functions certainly save a line in many of the call sites, but
whether they provide significant advantages I cannot say (I personally have
no fundamental problems with either version).  I guess if pushed I'd
probably come down on the side of leaving things as they are: in principle
defining a bunch of thin wrapper functions to save one parameter isn't
something I tend to do since it doesn't seem worth it.  However, what has
changed since the last iteration of this idea is the use of identifiers
rather than numbers for many of call_fext_func()'s parameters, which adds
length to each call site.

If there is general agreement that using these functions is beneficial in
this context I certainly won't block it.

Regards
  jonathan

  parent reply	other threads:[~2018-03-04  5:37 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27 21:15 [PATCH 0/7] fujitsu-laptop: Consistent naming of constants Michał Kępień
2018-02-27 21:15 ` [PATCH 1/7] platform/x86: fujitsu-laptop: Define constants for FUNC operations Michał Kępień
2018-02-28 16:08   ` Andy Shevchenko
2018-03-04  5:08     ` Jonathan Woithe
2018-03-04 19:44       ` Michał Kępień
2018-03-04 22:49         ` Jonathan Woithe
2018-03-05 23:16         ` Darren Hart
2018-03-06  9:37           ` Andy Shevchenko
2018-03-06 20:59             ` Michał Kępień
2018-03-07 12:34               ` Andy Shevchenko
2018-03-10 20:10                 ` Michał Kępień
2018-03-12  9:27                   ` Andy Shevchenko
2018-02-27 21:15 ` [PATCH 2/7] platform/x86: fujitsu-laptop: Define constants for FUNC features Michał Kępień
2018-02-27 21:15 ` [PATCH 3/7] platform/x86: fujitsu-laptop: Define constants for FUNC feature states Michał Kępień
2018-02-27 21:15 ` [PATCH 4/7] platform/x86: fujitsu-laptop: Rename constants defining hotkey codes Michał Kępień
2018-02-27 21:15 ` [PATCH 5/7] platform/x86: fujitsu-laptop: Tweak how constants are commented and laid out Michał Kępień
2018-02-27 21:15 ` [PATCH 6/7] platform/x86: fujitsu-laptop: More accurately represent the hotkey ring buffer managed by firmware Michał Kępień
2018-02-28 16:13   ` Andy Shevchenko
2018-03-04 19:57     ` Michał Kępień
2018-02-27 21:15 ` [PATCH 7/7] platform/x86: fujitsu-laptop: Introduce fext_*() helper functions Michał Kępień
2018-03-04  5:37 ` Jonathan Woithe [this message]
2018-03-21 23:25 ` [PATCH 0/7] fujitsu-laptop: Consistent naming of constants Darren Hart

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=20180304053711.GB3129@marvin.atrad.com.au \
    --to=jwoithe@just42.net \
    --cc=andy@infradead.org \
    --cc=dvhart@infradead.org \
    --cc=kernel@kempniu.pl \
    --cc=linux-kernel@vger.kernel.org \
    --cc=platform-driver-x86@vger.kernel.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
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).