On Fri, Jan 19, 2024 at 6:59 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jan 18, 2024 at 07:41:54PM +0200, andy.shevchenko@xxxxxxxxx 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
