linux-kernel - Re: [PATCH 2/9] mtd: pxa3xx_nand: add a non mandatory ECC clock

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: <20150208205505.316f2624@bbrezillon>
Date:	Sun, 8 Feb 2015 20:55:05 +0100
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Antoine Tenart <antoine.tenart@...e-electrons.com>
Cc:	Andrew Lunn <andrew@...n.ch>, thomas.petazzoni@...e-electrons.com,
	zmxu@...vell.com, linux-kernel@...r.kernel.org,
	linux-mtd@...ts.infradead.org, ezequiel.garcia@...e-electrons.com,
	jszhang@...vell.com, computersforpeace@...il.com,
	dwmw2@...radead.org, linux-arm-kernel@...ts.infradead.org,
	sebastian.hesselbarth@...il.com
Subject: Re: [PATCH 2/9] mtd: pxa3xx_nand: add a non mandatory ECC clock

Hi Antoine,

On Wed, 28 Jan 2015 15:14:51 +0100
Antoine Tenart <antoine.tenart@...e-electrons.com> wrote:

> Hi Andrew,
> 
> On Tue, Jan 27, 2015 at 04:50:47PM +0100, Andrew Lunn wrote:
> > On Tue, Jan 27, 2015 at 03:10:09PM +0100, Antoine Tenart wrote:
> > > Some controllers (as the coming Berlin nand controller) need to enable
> > > an ECC clock. Add support for this clock in the pxa3xx nand driver, and
> > > leave it as non mandatory.
> > 
> > It would be good to document this clock in the device tree binding.
> 
> Yes! I'll add the ECC clock into the documentation.
> 
> > 
> > > Signed-off-by: Antoine Tenart <antoine.tenart@...e-electrons.com>
> > > ---
> > >  drivers/mtd/nand/pxa3xx_nand.c | 26 +++++++++++++++++++-------
> > >  1 file changed, 19 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> > > index d00ac392d1c4..2681ec4abafa 100644
> > > --- a/drivers/mtd/nand/pxa3xx_nand.c
> > > +++ b/drivers/mtd/nand/pxa3xx_nand.c
> > > @@ -180,7 +180,7 @@ struct pxa3xx_nand_info {
> > >  	struct nand_hw_control	controller;
> > >  	struct platform_device	 *pdev;
> > >  
> > > -	struct clk		*clk;
> > > +	struct clk		*clk, *ecc_clk;
> > >  	void __iomem		*mmio_base;
> > >  	unsigned long		mmio_phys;
> > >  	struct completion	cmd_complete, dev_ready;
> > > @@ -1608,7 +1608,7 @@ static int alloc_nand_resource(struct platform_device *pdev)
> > >  
> > >  	spin_lock_init(&chip->controller->lock);
> > >  	init_waitqueue_head(&chip->controller->wq);
> > > -	info->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	info->clk = devm_clk_get(&pdev->dev, "nfc");
> > 
> > Does this keep backwards compatibility? Not all the existing users
> > have there clocks named. What i think happens here is that
> > of_clk_get_by_name() calls of_property_match_string(np, "clock-names",
> > name); That returns -EINVAL, and then we ask of_clk_get() to get the
> > -EINVAL'th clock. I don't think that is going to end well.
> > 
> > I think you need to keep the NULL here, but should use "ecc" for the
> > second clock and require the use of the clock-names property when an
> > ecc clock is required.
> 
> I agree, I'll go back to NULL in v2.

I'd rather try with "nfc", and if it fails try with NULL as the name
argument, because passing NULL in all cases will enforce the clock
declaration order (which is not something we usually do when naming
clocks).
If you keep the NULL name, please document the clock order in your DT
documentation.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
--
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/