LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Axel Lin <axel.lin@ingics.com>
To: Olliver Schinagl <oliver@schinagl.nl>
Cc: Mark Brown <broonie@kernel.org>, 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 08:22:53 +0800	[thread overview]
Message-ID: <CAFRkauAPGEBxj7k2GvmFwhyc7ZuofhXou17b75F3G-mUWVaf9A@mail.gmail.com> (raw)
In-Reply-To: <24E35288-677D-4223-B94A-52A4F37792A8@schinagl.nl>

Olliver Schinagl <oliver@schinagl.nl> 於 2019年2月21日 週四 上午6:57寫道:
>
> Hey Axel,
>
> 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.

Now, take a look at below code:
static const struct regulator_linear_range axp803_dcdc234_ranges[] = {
       REGULATOR_LINEAR_RANGE(500000,
                              AXP803_DCDC234_500mV_START,
                              AXP803_DCDC234_500mV_END,
                              10000),
       REGULATOR_LINEAR_RANGE(1220000,
                              AXP803_DCDC234_1220mV_START,
                              AXP803_DCDC234_1220mV_END,
                              20000),
};
1st linear range:
I have to check what is AXP803_DCDC234_500mV_START for min_sel,
then check what is AXP803_DCDC234_500mV_END for max_sel,
and AXP803_DCDC234_500mV_END is AXP803_DCDC234_500mV_START +
AXP803_DCDC234_500mV_STEPS
so I have to check if AXP803_DCDC234_500mV_STEPS is correct or not.
The same for 2nd linear range.
It's unnecessary complexity.
And it's just for 2 linear ranges, for axp20x_ldo4_ranges with 5 linear range,
it's really hard to check the settings.

IMHO, In such case, the defines does not help.
It's a rare case that I suggest to remove the defines.

>
> Maybe you can explain what your problem is, as it appears you are trying to debug something?
well, I just read the code.
I just found it's very hard to read the linear range setting for this driver.

Regards,
Axel

  reply	other threads:[~2019-02-21  0:23 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 [this message]
2019-02-21  9:42     ` Mark Brown
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=CAFRkauAPGEBxj7k2GvmFwhyc7ZuofhXou17b75F3G-mUWVaf9A@mail.gmail.com \
    --to=axel.lin@ingics.com \
    --cc=broonie@kernel.org \
    --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).