Message-ID: <CAHp75Vd_hnnuHQxmiPTkS5GdpEf3iMik9=51x55_Xgr+7LDJ3Q@mail.gmail.com>
Date: Fri, 19 Jan 2024 19:24:45 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>
Cc: broonie@...nel.org, lee@...nel.org, robh+dt@...nel.org, 
	krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	linus.walleij@...aro.org, vkoul@...nel.org, lgirdwood@...il.com, 
	yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com, 
	pierre-louis.bossart@...ux.intel.com, alsa-devel@...a-project.org, 
	patches@...nsource.cirrus.com, devicetree@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-spi@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 6/6] ASoC: cs42l43: Add support for the cs42l43

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

..

> > > +   BUILD_BUG_ON(ARRAY_SIZE(cs42l43_jack_override_modes) !=
> > > +                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
> == rather than !=? 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 = 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] = ret;
> > > +   else
> > > +           ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
> >
> > and why not positive check?
> >
> > > +   return ret;
> >
> > Or even simply as
> >
> >       if (ret > 0)
> >               ret = cs42l43_dapm_get_volsw(kcontrol, ucontrol);
> >       else if (ret == 0)
> >               ucontrol->value.integer.value[0] = 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 = ...
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 /= 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 = 0;
 while (x > CONST) {
   x /= 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 = fls(x) - fls(CONST); // roughly looks like this, needs careful checking

..

> > > +   // Don't use devm as we need to get against the MFD device
> >
> > This is weird...
> >
> > > +   priv->mclk = 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 = devm_snd_soc_register_component(priv->dev, &cs42l43_component_drv,
> > > +                                         cs42l43_dais, ARRAY_SIZE(cs42l43_dais));
> > > +   if (ret) {
> > > +           dev_err_probe(priv->dev, ret, "Failed to register component\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 = platform_get_drvdata(pdev);
> > > +
> > > +   clk_put(priv->mclk);
> >
> > You have clocks put before anything else, and your remove order is broken 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).

-- 
With Best Regards,
Andy Shevchenko

Powered by blists - more mailing lists