linux-kernel - Re: [PATCH v3 2/4] clk: mediatek: Add initial common clock support for Mediatek SoCs.

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>] [day] [month] [year] [list]

Message-ID: <20150119162808.22722.33140@quantum>
Date:	Mon, 19 Jan 2015 08:28:08 -0800
From:	Mike Turquette <mturquette@...aro.org>
To:	James Liao <jamesjj.liao@...iatek.com>,
	"Matthias Brugger" <matthias.bgg@...il.com>
Cc:	"Rob Herring" <robh+dt@...nel.org>,
	"srv_heupstream" <srv_heupstream@...iatek.com>,
	"Sascha Hauer" <kernel@...gutronix.de>,
	"huang eddie" <eddie.huang@...iatek.com>,
	"HenryC Chen" <henryc.chen@...iatek.com>,
	"Pawel Moll" <pawel.moll@....com>,
	"Mark Rutland" <mark.rutland@....com>,
	"Ian Campbell" <ijc+devicetree@...lion.org.uk>,
	"Kumar Gala" <galak@...eaurora.org>,
	"Russell King" <linux@....linux.org.uk>,
	"Catalin Marinas" <catalin.marinas@....com>,
	"Vladimir Murzin" <vladimir.murzin@....com>,
	"Ashwin Chaugule" <ashwin.chaugule@...aro.org>,
	"Joe.C" <yingjoe.chen@...iatek.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 2/4] clk: mediatek: Add initial common clock support for
 Mediatek SoCs.

Quoting James Liao (2015-01-07 18:55:01)
> Hi Matthias,
> 
> On Wed, 2015-01-07 at 18:22 +0100, Matthias Brugger wrote:
> > 2015-01-07 4:25 GMT+01:00 James Liao <jamesjj.liao@...iatek.com>:
> > > +
> > > +static void cg_set_mask(struct mtk_clk_gate *cg, u32 mask)
> > 
> > Please add mtk_ prefix to all functions generic for the mediatek SoCs.
> 
> OK.
> 
> > 
> > > +       if (cg->flags & CLK_GATE_NO_SETCLR_REG) {
> > 
> > Is the CLK_GATE_NO_SETCLR_REG ever used?
> > As far as I can see, in this patch set it is not.
> 
> No, this flag is not used in this patch. I'll remove it or add clocks
> which use this flag in next patch.
> 
> > > +
> > > +       if (cg->flags & CLK_GATE_INVERSE)
> > > +               cg_set_mask(cg, mask);
> > > +       else
> > > +               cg_clr_mask(cg, mask);
> > > +
> > > +       mtk_clk_unlock(flags);
> > > +
> > > +       return 0;
> > > +}
> > 
> > Actually we should use CLK_GATE_SET_TO_DISABLE instead of inventing a
> > new bit, right?
> 
> CLK_GATE_SET_TO_DISABLE is used by struct clk_gate, which is different
> from struct mtk_clk_gate. Should we use the same constant in these 2
> different implementation? If yes, how do we avoid bit conflict between
> clk_gate and mtk_clk_gate if we both add more flags in the future?

I think that CLK_GATE_INVERSE is fine. This clock gate implementation is
sufficiently different from the simple drivers/clk/clk-gate.c
implementation (e.g. separate registers for setting bits, clearing bits
and getting status).

Regards,
Mike

> 
> 
> > > +       pr_debug("%s(): %d, %s, bit[%d]\n",
> > > +               __func__, r, __clk_get_name(hw->clk), (int)cg->bit);
> > 
> > Same here. Please review all debug messages.
> 
> OK, I'll remove them in next patch.
> 
> 
> > > +
> > > +#define CLK_DEBUG              0
> > > +#define DUMMY_REG_TEST         0
> > 
> > This defines are not used, delete them.
> 
> OK.
> 
> > > +
> > > +extern spinlock_t *get_mtk_clk_lock(void);
> > > +
> > > +#define mtk_clk_lock(flags)    spin_lock_irqsave(get_mtk_clk_lock(), flags)
> > > +#define mtk_clk_unlock(flags)  \
> > > +       spin_unlock_irqrestore(get_mtk_clk_lock(), flags)
> > 
> > Please use the spinlock directly without this akward defines.
> 
> Do you mean we should export clk_ops_lock (from clk-mtk.c) directly
> instead of get_mtk_clk_lock()? Or simply remove mtk_clk_lock/unlock()?
> 
> 
> Best regards,
> 
> James
> 
> 
> 
--
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/