Re: [PATCH v3 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier

From: Charles Keepax <ckeepax@opensource.cirrus.com>
To: David Rhodes <drhodes@opensource.cirrus.com>
Cc: robh@kernel.org, brian.austin@cirrus.com,
	patches@opensource.cirrus.com, alsa-devel@alsa-project.org,
	david.rhodes@cirrus.com, Mark Brown <broonie@kernel.org>
Subject: Re: [PATCH v3 1/2] ASoC: cs35l41: CS35L41 Boosted Smart Amplifier
Date: Wed, 21 Jul 2021 09:01:04 +0000	[thread overview]
Message-ID: <20210721090104.GJ9223@ediswmail.ad.cirrus.com> (raw)
In-Reply-To: <5ab8fd33-3164-8bf5-2fb4-4819ef01a815@opensource.cirrus.com>

On Tue, Jul 20, 2021 at 05:18:09PM -0500, David Rhodes wrote:
> On 7/12/21 10:03 AM, Mark Brown wrote:
> >On Fri, Jul 09, 2021 at 06:11:28PM -0500, David Rhodes wrote:
> >>On 7/5/21 2:20 PM, Mark Brown wrote:
> >
> >>>I can't see any references to GLOBAL_EN outside this function, why might
> >>>it not be set?
> >
> >>This check prevents an incorrect 'PDN Failed' message if DAPM initializes
> >>and turns the widget off at boot (with no prior power-up).
> >
> >In what way does it prevent this message and why is this a good approach
> >to the issue?
> >
> 
> There is a better approach that I will include in v4.
> The chip has a bit field that indicates if GLOBAL_EN has been asserted.
> The 'PDN Failed' message can be skipped if this bit is not set.

I am not sure that really addresses the comment here. The issue isn't
so much the way you are avoiding printing that error message, but
why that is necessary in the first place. AFAIK DAPM won't attempt
to power down a widget without powering it up first. So, at least
to me, its not clear how you get to this code without GLOBAL_EN
having first been set by the power up?

Often with comments like this what people are looking for is for
you to explain more about why the code does what it does, rather
than just jump to a new implementation. After the reasoning is well
understood then its much easier to have a discussion about if it
is the right way to solve the problem.

Thanks,
Charles