Received-SPF: pass (google.com: domain of linux-kernel+bounces-31421-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20230804104602.395892-1-ckeepax@opensource.cirrus.com>
 <20230804104602.395892-7-ckeepax@opensource.cirrus.com> <Zali4qxdegY7H6eY@surfacebook.localdomain>
 <20240119165917.GC16899@ediswmail.ad.cirrus.com>
In-Reply-To: <20240119165917.GC16899@ediswmail.ad.cirrus.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Fri, 19 Jan 2024 19:24:45 +0200
Message-ID: <CAHp75Vd_hnnuHQxmiPTkS5GdpEf3iMik9=51x55_Xgr+7LDJ3Q@mail.gmail.com>
Subject: Re: [PATCH v7 6/6] ASoC: cs42l43: Add support for the cs42l43
To: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org, 
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, 
	linus.walleij@linaro.org, vkoul@kernel.org, lgirdwood@gmail.com, 
	yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com, 
	pierre-louis.bossart@linux.intel.com, alsa-devel@alsa-project.org, 
	patches@opensource.cirrus.com, devicetree@vger.kernel.org, 
	linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Jan 19, 2024 at 6:59=E2=80=AFPM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Jan 18, 2024 at 07:41:54PM +0200, andy.shevchenko@gmail.com wrote=
:
> > Fri, Aug 04, 2023 at 11:46:02AM +0100, Charles Keepax kirjoitti:

..

> > > +   BUILD_BUG_ON(ARRAY_SIZE(cs42l43_jack_override_modes) !=3D
> > > +                ARRAY_SIZE(cs42l43_jack_text) - 1);
> >
> > Use static_assert() instead.
>
> I am happy either way, but for my own education what is the
> reason to prefer static_assert here, is it just to be able to use
> =3D=3D rather than !=3D? Or is there in general a preference to use
> static_assert, there is no obvious since BUILD_BUG_ON is being
> deprecated?

It's generally preferred since there are (known) issues with it:
- it can't be put without the scope (globally);
- it produces a lot of a noise and hard to read error report;
- ...anything I forgot / don't know (yet) about...

BUILD_BUG_ON() might be useful in some cases, but I don't see how.

..

> > > +   ret =3D cs42l43_shutter_get(priv, CS42L43_STATUS_MIC_SHUTTER_MUTE=
_SHIFT);
> > > +   if (ret < 0)
> > > +           return ret;
> > > +   else if (!ret)
> >
> > Reundant 'else'
> >
> > > +           ucontrol->value.integer.value[0] =3D ret;
> > > +   else
> > > +           ret =3D cs42l43_dapm_get_volsw(kcontrol, ucontrol);
> >
> > and why not positive check?
> >
> > > +   return ret;
> >
> > Or even simply as
> >
> >       if (ret > 0)
> >               ret =3D cs42l43_dapm_get_volsw(kcontrol, ucontrol);
> >       else if (ret =3D=3D 0)
> >               ucontrol->value.integer.value[0] =3D ret;
> >
> >       return ret;
>
> Yeah will update, that is definitely neater.

Note before doing that the last one has a downside from the

if (ret < 0)
  return ret;
if (ret)
  ret =3D ...
else
  ...
return ret;

as it assumes that there will be no additional code in between
'if-else-if' and last 'return'. Purely a maintenance aspect, but
still... So, think about it which one you would prefer,

..

> > > +   while (freq > cs42l43_pll_configs[ARRAY_SIZE(cs42l43_pll_configs)=
 - 1].freq) {
> > > +           div++;
> > > +           freq /=3D 2;
> > > +   }
> >
> > fls() / fls_long()?
>
> Apologies but I might need a little bit more of a pointer here.
> We need to scale freq down to under 3.072MHz and I am struggling
> a little to see how to do that with fls.

The second argument of > operator is invariant to the loop, correct?
So it can be written as (pseudocode)

 y =3D 0;
 while (x > CONST) {
   x /=3D 2;
   y++;
 }

This is basically the open coded 'find the scale of x against CONST as
power of 2 value'. Okay, it might be not directly fls(), but something
along those types of bit operations (I believe something similar is
used in spi-pxa2xx.c for calculating the divider for the Intel Quark
case).

y =3D fls(x) - fls(CONST); // roughly looks like this, needs careful checki=
ng

..

> > > +   // Don't use devm as we need to get against the MFD device
> >
> > This is weird...
> >
> > > +   priv->mclk =3D clk_get_optional(cs42l43->dev, "mclk");
> > > +   if (IS_ERR(priv->mclk)) {
> > > +           dev_err_probe(priv->dev, PTR_ERR(priv->mclk), "Failed to =
get mclk\n");
> > > +           goto err_pm;
> > > +   }
> > > +
> > > +   ret =3D devm_snd_soc_register_component(priv->dev, &cs42l43_compo=
nent_drv,
> > > +                                         cs42l43_dais, ARRAY_SIZE(cs=
42l43_dais));
> > > +   if (ret) {
> > > +           dev_err_probe(priv->dev, ret, "Failed to register compone=
nt\n");
> > > +           goto err_clk;
> > > +   }
> > > +
> > > +   pm_runtime_mark_last_busy(priv->dev);
> > > +   pm_runtime_put_autosuspend(priv->dev);
> > > +
> > > +   return 0;
> > > +
> > > +err_clk:
> > > +   clk_put(priv->mclk);
> > > +err_pm:
> > > +   pm_runtime_put_sync(priv->dev);
> > > +
> > > +   return ret;
> > > +}
> > > +
> > > +static int cs42l43_codec_remove(struct platform_device *pdev)
> > > +{
> > > +   struct cs42l43_codec *priv =3D platform_get_drvdata(pdev);
> > > +
> > > +   clk_put(priv->mclk);
> >
> > You have clocks put before anything else, and your remove order is brok=
en now.
> >
> > To fix this (in case you may not used devm_clk_get() call), you should =
drop
> > devm calls all way after the clk_get(). Do we have
> > snd_soc_register_component()? If yes, use it to fix.
> >
> > I believe you never tested rmmod/modprobe in a loop.
>
> Hmm... will need to think this through a little bit, so might take
> a little longer on this one. But I guess this only becomes a problem
> if you attempt to remove the driver whilst you are currently playing
> audio, and I would expect the card tear down would stop the clock
> running before we get here.

I don't know the HW, it is up to you how to address this. The issue
really exists and might become a hard to hunt bug (e.g., if there is
an IRQ fired or async work which would like to access the HW with
clock off and hang the system).

--=20
With Best Regards,
Andy Shevchenko