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:   Wed, 24 May 2023 14:14:11 +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: <20230524141411.28765782@bootlin.com>
In-Reply-To: <87mt1u7fql.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>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Kuninori,

On Wed, 24 May 2023 00:08:50 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi
> 
> > An additional-devs subnode can be present in the simple-card top node.
> > This subnode is used to declared some "virtual" additional devices.
> > 
> > Create related devices from this subnode and avoid this subnode presence
> > to interfere with the already supported subnodes analysis.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---  
> 
> simple-card is used in many boards, but is old.
> Adding new feature on audio-graph-card/audio-graph-card2 instead of simple-card
> is my ideal, but it is OK.
> 
> simple-card is possible to handle multiple DAI links by using
> "dai-link" node on 1 Sound Card. see
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/sound/simple-card.yaml?h=v6.4-rc3#n294
> 
> Is this "additional-devs" available only one per 1 Card ?
> If it is possible to use 1 additional-devs per 1 DAI link, I think this patch want to
> care "dai-link".
> Or adding temporally NOTE or FIXME message like /* NOTE: it doesn't support dai-link so far */
> is good idea.

As you said on your other mail 1 "additional-devs" per 1 Card.
And further more, I think that "additional-devs" has nothing to do with
DAI link.
These additional devices are really related to the the Card itself and
not DAI links.

simple_populate_aux() needs to be called once per Card.

> 
> >  static int asoc_simple_probe(struct platform_device *pdev)
> >  {
> >  	struct asoc_simple_priv *priv;
> > @@ -688,6 +731,11 @@ static int asoc_simple_probe(struct platform_device *pdev)
> >  		return ret;
> >  
> >  	if (np && of_device_is_available(np)) {
> > +		ret = simple_populate_aux(priv);
> > +		if (ret < 0) {
> > +			dev_err_probe(dev, ret, "populate aux error\n");
> > +			goto err;
> > +		}
> >  
> >  		ret = simple_parse_of(priv, li);
> >  		if (ret < 0) {
> > -- 
> > 2.40.1
> >   
> 
> Calling simple_populate_aux() before calling simple_parse_of() is possible,
> but looks strange for me.
> see below
> 
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> > index 5a5e4ecd0f61..4992ab433d6a 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c  
> (snip)
> > @@ -359,6 +360,8 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
> >  		is_top = 1;
> >  	}
> >  
> > +	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> 
> I think better position to call simple_populate_aux() is here.
> But __simple_for_each_link() will be called multiple times for CPU and for Codec.
> So maybe you want to calling it under CPU turn.
> 
> 	 /* NOTE: it doesn't support dai-link so far */
> 	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
> 	if (add_devs && li->cpu) {
> 		ret = simple_populate_aux(priv);
> 		...
> 	}

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.

simple_populate_aux() could be called by simple_parse_of() itself or after
simple_parse_of() call.

I would prefer calling it before snd_soc_of_parse_aux_devs() because
this function parses aux-devs phandles and these phandles can refer an
auxiliary device present in the additional-devs node.

The current code has no issue with this but some evolution can lead to
EPROBE_DEFER if the device related to the phandle was not probed.
If simple_populate_aux() is called after snd_soc_of_parse_aux_devs(),
an EPROBE_DEFER related to the missing probe() call has no chance to
be resolved.

Tell be what you prefer:
  a) Call before simple_parse_of() (no changes)
or
  b) Call at the very beginning of simple_parse_of()
or
  c) Other suggestion ...


Thanks a lot for your review.
Best regards,
Hervé

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