linux-kernel - Re: [PATCH v14 1/4] clk: mediatek: Add MT2701 clock support

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

Message-ID: <1477992234.8785.9.camel@mtksdaap41>
Date:   Tue, 1 Nov 2016 17:23:54 +0800
From:   James Liao <jamesjj.liao@...iatek.com>
To:     Stephen Boyd <sboyd@...eaurora.org>
CC:     Erin Lo <erin.lo@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Mike Turquette <mturquette@...libre.com>,
        "Rob Herring" <robh@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Sascha Hauer <kernel@...gutronix.de>,
        Daniel Kurtz <djkurtz@...omium.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        <devicetree@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <linux-clk@...r.kernel.org>,
        <srv_heupstream@...iatek.com>, <ms.chen@...iatek.com>,
        <robert.chou@...iatek.com>, Shunli Wang <shunli.wang@...iatek.com>
Subject: Re: [PATCH v14 1/4] clk: mediatek: Add MT2701 clock support

Hi Stephen,

On Mon, 2016-10-31 at 11:06 -0700, Stephen Boyd wrote:
> On 10/31, James Liao wrote:
> > On Thu, 2016-10-27 at 18:17 -0700, Stephen Boyd wrote:
> > > On 10/21, Erin Lo wrote:
> > > > @@ -244,3 +256,31 @@ void mtk_clk_register_composites(const struct mtk_composite *mcs,
> > > >  			clk_data->clks[mc->id] = clk;
> > > >  	}
> > > >  }
> > > > +
> > > > +void mtk_clk_register_dividers(const struct mtk_clk_divider *mcds,
> > > > +			int num, void __iomem *base, spinlock_t *lock,
> > > > +				struct clk_onecell_data *clk_data)
> > > > +{
> > > > +	struct clk *clk;
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i <  num; i++) {
> > > > +		const struct mtk_clk_divider *mcd = &mcds[i];
> > > > +
> > > > +		if (clk_data && !IS_ERR_OR_NULL(clk_data->clks[mcd->id]))
> > > 
> > > NULL is a valid clk. IS_ERR_OR_NULL is usually wrong.
> > 
> > Why NULL is a valid clk?
> 
> Perhaps at some point we'll want to return a NULL pointer to
> clk_get() callers so that they can handle things like optional
> clocks easily without having any storage requirements. I don't
> know if we'll ever do that, but that's just a possibility.
> 
> > 
> > clk_data is designed for multiple initialization from different clock
> > types, such as infra_clk_data in clk-mt2701.c. So it will ignore valid
> > clocks to avoid duplicated clock registration. Here I assume a clock
> > pointer with error code or NULL to be an invalid (not initialized)
> > clock.
> > 
> 
> Ok. Would it be possible to initialize the array with all error
> pointers? That would make things less error prone, but it

Yes. Current mtk_alloc_clk_data() implementation init all elements with
ERR_PTR(-ENOENT). 

> probably doesn't matter at all anyway because this is done during
> registration time. IS_ERR_OR_NULL makes me take a second look
> each time, because it's usually wrong.

I see. Although currently all Mediatek clk drivers use
mtk_alloc_clk_data() to allocate clk_data, I would like to keep the
flexibility to support zero-initialized clk_data such as a static
structure. So I prefer to treat a NULL pointer as an uninitialized
clock.


Best regards,

James