LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Mark Brown <broonie@kernel.org>
To: Axel Lin <axel.lin@ingics.com>
Cc: Olliver Schinagl <oliver@schinagl.nl>,
	Chen-Yu Tsai <wens@csie.org>, Priit Laes <plaes@plaes.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines
Date: Thu, 21 Feb 2019 09:42:37 +0000	[thread overview]
Message-ID: <20190221094237.GA5970@sirena.org.uk> (raw)
In-Reply-To: <CAFRkauAPGEBxj7k2GvmFwhyc7ZuofhXou17b75F3G-mUWVaf9A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1463 bytes --]

On Thu, Feb 21, 2019 at 08:22:53AM +0800, Axel Lin wrote:
> Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月21日 週四 上午6:57寫道:
> > On February 20, 2019 5:50:13 PM GMT+01:00, Axel Lin <axel.lin@ingics.com> wrote:

> > >The AXP20X_xxx_START/END/STEPS defines make the code hard to read and
> > >very hard to check the linear range settings because it needs to check
> > >the defines one-by-one.
> > >The original code without the defines is very good in readability
> > >as the meaning of each field of REGULATOR_LINEAR_RANGE is clear.
> > >So I suggest to get rid of AXP20X_xxx_START/END/STEPS defines.

> > Are you suggesting that magic values and hex numbers are more readable?

> For example:
> static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
>        REGULATOR_LINEAR_RANGE(500000, 0x0, 0x46, 10000),
>        REGULATOR_LINEAR_RANGE(1220000, 0x47, 0x4b, 20000),
> };
> Above looks very clear to me as it describes the linear ranges:
> 1st linear range: min_uV is 500000, from selector 0 ~ 0x46, the uV_step is 10000
> 2nd linear range: min_uV is 1220000, from selector 0x47 ~ 0x4b, the
> uV_step is 20000
> And it's easy to check the min_sel and max_sel in each linear range.

Frankly I tend to agree with Axel here - the defines are used in exactly
one place so their main impact is that you can't read the ranges
directly in the array but have to jump around to read through the
defines.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

  reply	other threads:[~2019-02-21  9:42 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-20 16:50 Axel Lin
2019-02-20 21:38 ` Olliver Schinagl
2019-02-21  0:22   ` Axel Lin
2019-02-21  9:42     ` Mark Brown [this message]
2019-02-23  7:55       ` Olliver Schinagl
2019-02-23 12:54         ` Axel Lin
2019-02-23 20:37           ` Olliver Schinagl
2019-02-25 17:25             ` Mark Brown
2019-02-27 19:41               ` Olliver Schinagl
2019-02-27 20:05                 ` Mark Brown
2019-02-27 20:26                   ` Olliver Schinagl
2019-03-23 13:41             ` Axel Lin

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=20190221094237.GA5970@sirena.org.uk \
    --to=broonie@kernel.org \
    --cc=axel.lin@ingics.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oliver@schinagl.nl \
    --cc=plaes@plaes.org \
    --cc=wens@csie.org \
    --subject='Re: [PATCH] regulator: axp20x: Get rid of AXP20X_xxx_START/END/STEPS defines' \
    /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

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).