Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function

From: "Mylène Josserand" <mylene.josserand@bootlin.com>
To: Maxime Ripard <maxime.ripard@bootlin.com>
Cc: linux@armlinux.org.uk, wens@csie.org, robh+dt@kernel.org,
	mark.rutland@arm.com, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, clabbe.montjoie@gmail.com,
	thomas.petazzoni@bootlin.com, quentin.schulz@bootlin.com
Subject: Re: [PATCH v4 01/10] ARM: sun9i: smp: Add sun9i dt parsing function
Date: Sun, 25 Feb 2018 16:19:01 +0100	[thread overview]
Message-ID: <20180225161901.207b69fe@dell-desktop.home> (raw)
In-Reply-To: <20180223145423.5og5bdhtznu5qlw7@flea.lan>

Hi,

On Fri, 23 Feb 2018 15:54:23 +0100
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

> Hi,
> 
> On Fri, Feb 23, 2018 at 02:37:33PM +0100, Mylène Josserand wrote:
> > To prepare the support for sun8i-a83t, create a new function
> > that handles all the resources needed on sun9i-a80 (because
> > it will be different from sun8i-a83t).
> > 
> > All the parsing of device tree is moved into this new function:
> > sun9i_dt_parsing. Create also a function to release the resources
> > retrieved during the dt parsing in case there is an error in init
> > function.
> > 
> > Signed-off-by: Mylène Josserand <mylene.josserand@bootlin.com>
> > ---
> >  arch/arm/mach-sunxi/mc_smp.c | 99 ++++++++++++++++++++++++++------------------
> >  1 file changed, 58 insertions(+), 41 deletions(-)
> > 
> > diff --git a/arch/arm/mach-sunxi/mc_smp.c b/arch/arm/mach-sunxi/mc_smp.c
> > index 11e46c6efb90..650a2ad4398f 100644
> > --- a/arch/arm/mach-sunxi/mc_smp.c
> > +++ b/arch/arm/mach-sunxi/mc_smp.c
> > @@ -684,35 +684,22 @@ static int __init sunxi_mc_smp_lookback(void)
> >  	return ret;
> >  }
> >  
> > -static int __init sunxi_mc_smp_init(void)
> > +static int sun9i_dt_parsing(struct resource res)  
> 
> Like I told you in the previous version, this should be _parse_dt, and
> not _dt_parsing.

Sorry, I forgot this review.

> 
> Also, I'm not sure why you are passing by copy the resource structure?
> 
> >  {
> > -	struct device_node *cpucfg_node, *sram_node, *node;
> > -	struct resource res;
> > +	struct device_node *prcm_node, *cpucfg_node, *sram_node;
> >  	int ret;
> >  
> > -	if (!of_machine_is_compatible("allwinner,sun9i-a80"))
> > -		return -ENODEV;
> > -
> > -	if (!sunxi_mc_smp_cpu_table_init())
> > -		return -EINVAL;
> > -
> > -	if (!cci_probed()) {
> > -		pr_err("%s: CCI-400 not available\n", __func__);
> > -		return -ENODEV;
> > -	}
> > -
> > -	node = of_find_compatible_node(NULL, NULL, "allwinner,sun9i-a80-prcm");
> > -	if (!node) {
> > -		pr_err("%s: PRCM not available\n", __func__);
> > +	prcm_node = of_find_compatible_node(NULL, NULL,
> > +				       "allwinner,sun9i-a80-prcm");
> > +	if (!prcm_node)
> >  		return -ENODEV;
> > -	}
> >  
> >  	/*
> >  	 * Unfortunately we can not request the I/O region for the PRCM.
> >  	 * It is shared with the PRCM clock.
> >  	 */
> > -	prcm_base = of_iomap(node, 0);
> > -	of_node_put(node);
> > +	prcm_base = of_iomap(prcm_node, 0);  
> 
> So it does more a bit more than just parsing the DT?
> 
> Can you maybe just fill the DT nodes and have the common part map the
> memory regions if the pointer is not NULL?

Yep, I will rework this function to only parse device tree nodes.

Thanks,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com