linux-kernel - Re: [PATCH 2/3] clk: samsung: Add clock driver for s5pc100

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <3392871.UAAWoO4asY@amdc1227>
Date:	Thu, 26 Sep 2013 16:00:28 +0200
From:	Tomasz Figa <t.figa@...sung.com>
To:	Yadwinder Singh Brar <yadi.brar01@...il.com>
Cc:	Mateusz Krawczuk <m.krawczuk@...tner.samsung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Mark Rutland <mark.rutland@....com>,
	devicetree <devicetree@...r.kernel.org>,
	Yadwinder Singh <yadi.brar@...sung.com>,
	linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
	Mike Turquette <mturquette@...aro.org>, rob@...dley.net,
	Pawel Moll <pawel.moll@....com>,
	Stephen Warren <swarren@...dotorg.org>,
	ijc+devicetree@...lion.org.uk,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Rob Herring <rob.herring@...xeda.com>, ben-linux@...ff.org,
	s.nawrocki@...sung.com,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/3] clk: samsung: Add clock driver for s5pc100

Hi Yadwinder,

I haven't reviewed this series yet, but let me clarify some things from
your comments.

On Thursday 26 of September 2013 17:38:58 Yadwinder Singh Brar wrote:
> > +
> > +/* Helper macros to define clock arrays. */
> > +#define FIXED_RATE_CLOCKS(name)        \
> > +               static struct samsung_fixed_rate_clock name[]
> > +#define MUX_CLOCKS(name)       \
> > +               static struct samsung_mux_clock name[]
> > +#define DIV_CLOCKS(name)       \
> > +               static struct samsung_div_clock name[]
> > +#define GATE_CLOCKS(name)      \
> > +               static struct samsung_gate_clock name[]
> > +
> 
> These macros seems little bit odd in our common practice,
> perhaps these are making code harder to read below.
> 

They allow array declaration to fit into single line. I agree that it is
not particularly easy to read at first sight, but shouldn't really be
much of nuisance. In addition, most of this driver is based on macros
like this, e.g. GATE(), MUX(), PNAME(), etc.

> > +PNAME(mout_i2s_2_p) = {
> > +       "fout_epll",
> > +       "i2scdclk0",
> > +       "dout_audio0",
> > +       "none"
> > +};
> > +
> 
> Using one line per parent isn't increasing length of file unnecessarily?

I believe this improves readability. Do we really care about size of
source code that much, over readability?

> > +       ALIAS(SCLK_AUDIO0, "soc-audio.0", "sclk_audio"),
> > +       ALIAS(SCLK_AUDIO1, "soc-audio.1", "sclk_audio"),
> > +       ALIAS(SCLK_AUDIO2, "soc-audio.2", "sclk_audio"),
> > +       ALIAS(KEYIF, NULL, "keypad"),
> > +
> > +       ALIAS(MFC, "s5p-mfc", "sclk_mfc"),
> > +       ALIAS(G2D, "s5p-g2d", "fimg2d"),
> > +
> > +};
> > +
> 
> Any reason/hidden advantage for using a separate of ALIAS,
> instead of using MUX_A/GATE_A ?

Yes, not even hidden. Alias is not a property of clock. One clock can
have multiple aliases, e.g. the same clock being input to multiple
devices.

In addition, as soon as we fully move s5pv210 to device tree, the whole
array of aliases will be dropped, without the need to touch the main
clock arrays at all.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/