Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 15:07:02 +0200
From:   Herve Codina <herve.codina@bootlin.com>
To:     Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-iio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices
Message-ID: <20230526150702.2555143c@bootlin.com>
In-Reply-To: <87v8ghgtyu.wl-kuninori.morimoto.gx@renesas.com>
References: <20230523151223.109551-1-herve.codina@bootlin.com>
        <20230523151223.109551-10-herve.codina@bootlin.com>
        <87mt1u7fql.wl-kuninori.morimoto.gx@renesas.com>
        <20230524141411.28765782@bootlin.com>
        <87v8ghgtyu.wl-kuninori.morimoto.gx@renesas.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 25 May 2023 00:01:14 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Herve
> 
> Thank you for your reply.
> 
> > So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
> > not correct as it has nothing to do with DAI links and must be call once
> > per Card.  
> 
> My biggest concern is that this code is calling same code multiple times.
> It is easy to forget such thing when updating in this kind of code.
> We don't forget / take mistake if these are merged.
> But we have such code everywhere ;) this is just my concern, not a big deal.
> 
> 	static int __simple_for_each_link (...)
> 	{
> 		...
> =>		add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> 		...
> 	}
> 
> 	static int simple_populate_aux(...)
> 	{
> 		...
> =>		node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");  
> 		...
> 	}
> 

Well, of_get_child_by_name() is called twice to retrieve the additional-devs
node but for very different reason.

In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI.
In simple_populate_aux() to take care of the devices declared in the node.

I am not sure that we should avoid that.
It will lead to a more complex code and flags just to avoid this call.

Not sure that it will be better.
__simple_for_each_link() is called multiple times and is supposed to look at links.
To avoid the of_get_child_by_name() filter-out call, __simple_for_each_link()
will look at link *and* populate devices calling simple_populate_aux().
And to do that correctly it will use a flag to be sure that simple_populate_aux()
was called only once.


In order to avoid some kind of duplication (at least the node name):

	static struct device_node *simple_of_get_add_devs(struct device_node *node)
	{
		return of_get_child_by_name(node, PREFIX "additional-devs");
	}

	static int __simple_for_each_link (...)
	{
		...
=>		add_devs = simple_of_get_add_devs(top);  
		...
	}

	static int simple_populate_aux(...)
	{
		...
=>		node = simple_of_get_add_devs(dev->of_node);  
		...
	}


Does it look better ?

Best regards,
Hervé

> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto


-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com