Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Juha-Matti Tilli <juha-matti.tilli@iki.fi>
To: Mikko Perttunen <mikko.perttunen@kapsi.fi>
Cc: Mikko Perttunen <cyndis@kapsi.fi>,
	linux-pm@vger.kernel.org, swarren@wwwdotorg.org,
	linux-kernel@vger.kernel.org,
	Mikko Perttunen <mperttunen@nvidia.com>,
	edubezval@gmail.com, Thierry Reding <thierry.reding@gmail.com>,
	linux-tegra@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver
Date: Sat, 27 Sep 2014 15:06:49 +0300
Message-ID: <20140927120649.GA70809@sandfort.unics.fi> (raw)
In-Reply-To: <5425CC6F.2020309@kapsi.fi>

On Fri, Sep 26, 2014 at 11:28:31PM +0300, Mikko Perttunen wrote:
> > I think a more idiomatic way to write this would be:
> >
> > static int
> > calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
> >                               struct tsensor_shared_calibration shared,
> >                               u32 *calib)
[snip]
> >
> > While at it, perhaps make shared a const * instead of passing it in by
> > value?
> 
> That is possible, but I'm not sure what the difference would be. Is 
> there a style rule forbidding by-value compound types? (Also if I change 
> the style, it would go over 80 characters by even more.)

I guess the idea is that it's more space- and time-efficient to pass
compound types as pointers instead of by value. Kernel stack is quite
limited in size, so allocating structs from stack in this way quickly
eats up the stack. Furthermore, there's less machine language
instructions in the function call if passed as a pointer, because when
passed as a value, each member of the struct needs to be pushed to the
stack separately (unless the member size is smaller than word length of
the machine architecture, in which case the compiler may optimize a
bit). So it's faster, too, to pass by pointer. In the case of kernel
coding, of these problems the limited stack size is far more serious.

In this case, however, the current struct is only 16 bytes vs 4/8 bytes
for a pointer, so it shouldn't matter that much. But IMO as a general
rule it's a far better style to pass compound types as pointers. I would
definitely pass by pointer here instead of passing by value even given
that the advantages in this particular case are limited. You should
consider the possibility that in a future driver version struct
tsensor_shared_calibration may become larger, and thus the code will be
closer to stack overflow if the one who increases the size of struct
tsensor_shared_calibration doesn't notice that it is passed by value.

There are always ways to structure the code so that it looks fine and
the additional "const *" will not cause the line length to become >80
chars. In my opinion, line length considerations should never play a
role on deciding whether to pass by value or pass by pointer. I don't
understand why you think this particular function would have line length
problems. In my opinion, the following is 78 chars max:

static int 
calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
                              const struct tsensor_shared_calibration *shared,
                              u32 *calib)

Of course, if you want to have "static int" on the same line as the
function name, then you'll have problems but even those can be avoided
so that the code still looks fine:

static int calculate_tsensor_calibration(const struct tegra_tsensor *sensor,
                                         const struct
                                             tsensor_shared_calibration *shared,
                                         u32 *calib)

Anyway, the root cause of line length problems here is overlong
identifiers. For example, "calculate_tsensor_calibration" is in my
opinion too long for a function name. You could easily abbreviate it to
"calc_tsensor_calib" without losing any information. Similarly, "struct
tsensor_shared_calibration" could be easily abbreviated to "struct
tsensor_shared_calib". Then you could have easily:

static int calc_tsensor_calib(const struct tegra_tsensor *sensor,
                              const struct tsensor_shared_calib *shared,
                              u32 *calib)

without being even close to the 80-character line length limitation.

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-26  9:43 [PATCH v6 0/4] Tegra124 soctherm driver Mikko Perttunen
     [not found] ` <1411724593-4037-1-git-send-email-cyndis-/1wQRMveznE@public.gmane.org>
2014-09-26  9:43   ` [PATCH v6 1/4] of: Add bindings for nvidia,tegra124-soctherm Mikko Perttunen
2014-09-26  9:43 ` [PATCH v6 2/4] ARM: tegra: Add soctherm and thermal zones to Tegra124 device tree Mikko Perttunen
2014-09-26  9:43 ` [PATCH v6 3/4] ARM: tegra: Add thermal trip points for Jetson TK1 Mikko Perttunen
2014-09-26  9:43 ` [PATCH v6 4/4] thermal: Add Tegra SOCTHERM thermal management driver Mikko Perttunen
2014-09-26 11:45   ` Thierry Reding
2014-09-26 20:28     ` Mikko Perttunen
2014-09-27 12:06       ` Juha-Matti Tilli [this message]
     [not found]         ` <20140927120649.GA70809-Xv4IVSKz7SWQi9Q/X01l6/UpdFzICT1y@public.gmane.org>
2014-09-29 13:42           ` Mikko Perttunen
2014-09-29  8:14       ` Peter De Schrijver
     [not found]       ` <5425CC6F.2020309-/1wQRMveznE@public.gmane.org>
2014-09-29  8:29         ` Thierry Reding
2014-09-29 13:37           ` Mikko Perttunen
2014-09-29 14:17   ` [PATCH v7 " Mikko Perttunen
2014-10-15 10:05     ` Mikko Perttunen
     [not found]       ` <543E46DF.8060705-/1wQRMveznE@public.gmane.org>
2014-11-07 15:54         ` Eduardo Valentin
2014-11-08  1:11           ` Mikko Perttunen
2014-09-26 10:19 ` [PATCH v6 0/4] Tegra124 soctherm driver Thierry Reding
2014-09-26 10:22   ` Mikko Perttunen
     [not found]     ` <54253E7C.9080704-/1wQRMveznE@public.gmane.org>
2014-09-26 11:48       ` Thierry Reding
2014-09-26 12:00         ` Mikko Perttunen
     [not found]           ` <5425554B.7060102-/1wQRMveznE@public.gmane.org>
2014-09-26 12:05             ` Thierry Reding
2014-09-26 12:09               ` Mikko Perttunen

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=20140927120649.GA70809@sandfort.unics.fi \
    --to=juha-matti.tilli@iki.fi \
    --cc=cyndis@kapsi.fi \
    --cc=edubezval@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mikko.perttunen@kapsi.fi \
    --cc=mperttunen@nvidia.com \
    --cc=swarren@wwwdotorg.org \
    --cc=thierry.reding@gmail.com \
    /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

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/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 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


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