All of lore.kernel.org
 help / color / mirror / Atom feed
From: daniel.thompson@linaro.org (Daniel Thompson)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/9 v2] coresight: add CoreSight core layer framework
Date: Thu, 03 Jul 2014 10:12:08 +0100	[thread overview]
Message-ID: <53B51E68.7090308@linaro.org> (raw)
In-Reply-To: <CANLsYkxKZvb4cOeEfwFB8CbK_PKy6daPL63L00Sx=NvtDjGbfw@mail.gmail.com>

On 02/07/14 20:06, Mathieu Poirier wrote:
>>> +struct dentry *cs_debugfs_parent = NULL;
>>> +
>>> +static int curr_sink = NO_SINK;
>>> +static LIST_HEAD(coresight_orph_conns);
>>> +static LIST_HEAD(coresight_devs);
>>> +static DEFINE_SEMAPHORE(coresight_mutex);
>>
>> Why is coresight_mutex a semaphore?
> 
> Bad naming convention.

Really? I only saw it used like a mutex, in other words I thought it was
incorrectly typed, rather than incorrectly named.


>>> +static void coresight_disable_link(struct coresight_device *csdev)
>>> +{
>>> +     int link_subtype;
>>> +     int refport, inport, outport;
>>> +
>>> +     inport = coresight_find_link_inport(csdev);
>>> +     outport = coresight_find_link_outport(csdev);
>>> +
>>> +     link_subtype = csdev->subtype.link_subtype;
>>> +     if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
>>> +             refport = inport;
>>> +     else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
>>> +             refport = outport;
>>> +     else
>>> +             refport = 0;
>>
>> I already read these 7 lines once...
> 
> It is really worth spinning off a function to save 5 lines?

Pretty marginal really. If the code here stays as it is I don't care
enough to raise this point a second time.


Daniel.

WARNING: multiple messages have this Message-ID (diff)
From: Daniel Thompson <daniel.thompson@linaro.org>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: "Linus Walleij" <linus.walleij@linaro.org>,
	"Will Deacon" <will.deacon@arm.com>,
	"Russell King - ARM Linux" <linux@arm.linux.org.uk>,
	"Rob Herring" <robherring2@gmail.com>,
	"Arve Hjønnevåg" <arve@android.com>,
	"John Stultz" <john.stultz@linaro.org>,
	"Pratik Patel" <pratikp@codeaurora.org>,
	"Vikas Varshney" <varshney@ti.com>, "Al Grant" <Al.Grant@arm.com>,
	"Jonas Svennebring" <jonas.svennebring@avagotech.com>,
	"James King" <james.king@linaro.org>,
	"Panchaxari Prasannamurthy Tumkur"
	<panchaxari.prasannamurthy@linaro.org>,
	"Arnd Bergmann" <arnd@linaro.org>,
	"Marcin Jabrzyk" <marcin.jabrzyk@gmail.com>,
	r.sengupta@samsung.com,
	"Robert Marklund" <robbelibobban@gmail.com>,
	"Tony Armitstead" <Tony.Armitstead@arm.com>,
	"Patch Tracking" <patches@linaro.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/9 v2] coresight: add CoreSight core layer framework
Date: Thu, 03 Jul 2014 10:12:08 +0100	[thread overview]
Message-ID: <53B51E68.7090308@linaro.org> (raw)
In-Reply-To: <CANLsYkxKZvb4cOeEfwFB8CbK_PKy6daPL63L00Sx=NvtDjGbfw@mail.gmail.com>

On 02/07/14 20:06, Mathieu Poirier wrote:
>>> +struct dentry *cs_debugfs_parent = NULL;
>>> +
>>> +static int curr_sink = NO_SINK;
>>> +static LIST_HEAD(coresight_orph_conns);
>>> +static LIST_HEAD(coresight_devs);
>>> +static DEFINE_SEMAPHORE(coresight_mutex);
>>
>> Why is coresight_mutex a semaphore?
> 
> Bad naming convention.

Really? I only saw it used like a mutex, in other words I thought it was
incorrectly typed, rather than incorrectly named.


>>> +static void coresight_disable_link(struct coresight_device *csdev)
>>> +{
>>> +     int link_subtype;
>>> +     int refport, inport, outport;
>>> +
>>> +     inport = coresight_find_link_inport(csdev);
>>> +     outport = coresight_find_link_outport(csdev);
>>> +
>>> +     link_subtype = csdev->subtype.link_subtype;
>>> +     if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_MERG)
>>> +             refport = inport;
>>> +     else if (link_subtype == CORESIGHT_DEV_SUBTYPE_LINK_SPLIT)
>>> +             refport = outport;
>>> +     else
>>> +             refport = 0;
>>
>> I already read these 7 lines once...
> 
> It is really worth spinning off a function to save 5 lines?

Pretty marginal really. If the code here stays as it is I don't care
enough to raise this point a second time.


Daniel.

  reply	other threads:[~2014-07-03  9:12 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-06-27 18:04 [PATCH 0/9 v2] Coresight framework and drivers mathieu.poirier at linaro.org
2014-06-27 18:04 ` mathieu.poirier
2014-06-27 18:04 ` [PATCH 1/9 v2] coresight: add CoreSight core layer framework mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-27 22:01   ` Rob Herring
2014-06-27 22:01     ` Rob Herring
2014-07-02 17:06     ` Mathieu Poirier
2014-07-02 17:06       ` Mathieu Poirier
2014-07-15 20:52     ` Mathieu Poirier
2014-07-15 20:52       ` Mathieu Poirier
2014-06-30 10:53   ` Dirk Behme
2014-06-30 10:53     ` Dirk Behme
2014-07-02 17:18     ` Mathieu Poirier
2014-07-02 17:18       ` Mathieu Poirier
2014-07-02  9:38   ` Daniel Thompson
2014-07-02  9:38     ` Daniel Thompson
2014-07-02 19:06     ` Mathieu Poirier
2014-07-02 19:06       ` Mathieu Poirier
2014-07-03  9:12       ` Daniel Thompson [this message]
2014-07-03  9:12         ` Daniel Thompson
2014-06-27 18:04 ` [PATCH 2/9 v2] coresight-tmc: add CoreSight TMC driver mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-07-02 15:47   ` Daniel Thompson
2014-07-02 15:47     ` Daniel Thompson
2014-06-27 18:04 ` [PATCH 3/9 v2] coresight-tpiu: add CoreSight TPIU driver mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-27 18:04 ` [PATCH 4/9 v2] coresight-etb: add CoreSight ETB driver mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-27 18:04 ` [PATCH 5/9 v2] coresight-funnel: add CoreSight Funnel driver mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-27 18:04 ` [PATCH 6/9 v2] coresight-etm: add CoreSight ETM/PTM driver mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-30 11:01   ` Dirk Behme
2014-06-30 11:01     ` Dirk Behme
2014-06-30 16:03     ` Mathieu Poirier
2014-06-30 16:03       ` Mathieu Poirier
2014-06-27 18:04 ` [PATCH 7/9 v2] coresight: adding support for beagle and beagleXM mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-06-27 18:04 ` [PATCH 8/9 v2] coresight: adding basic support for Vexpress TC2 mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-07-01  9:19   ` Dirk Behme
2014-07-01  9:19     ` Dirk Behme
2014-06-27 18:04 ` [PATCH 9/9 v2] ARM: removing support for etb/etm in "arch/arm/kernel/" mathieu.poirier at linaro.org
2014-06-27 18:04   ` mathieu.poirier
2014-07-01 10:32 ` [PATCH 0/9 v2] Coresight framework and drivers Al Grant
2014-07-01 10:32   ` Al Grant
2014-07-02 19:32   ` Mathieu Poirier

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=53B51E68.7090308@linaro.org \
    --to=daniel.thompson@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.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 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.