On Fri, Jul 02, 2021 at 12:35:00AM +0800, Seven Lee wrote:
> The driver is for codec NAU88L21 of Nuvoton Technology Corporation.
> --- /dev/null
> +++ b/sound/soc/codecs/nau8821.c
> @@ -0,0 +1,1781 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Nuvoton NAU88L21 audio codec driver
Please make the entire comment a C++ one so things look more
intentional.
> +/**
> + * nau8821_sema_acquire - acquire the semaphore of nau8821
> + * @nau8821: component to register the codec private data with
> + * @timeout: how long in jiffies to wait before failure or zero to wait
> + * until release
So, this driver looks pretty good apart from this semaphore usage which
is both not very clearly documented centrally (in terms of the purpose
and what it's protecting) and just uses semaphores which is generally
unusual and worrying, there's a push to try to eliminate semaphores from
the kernel and replace them with clearer and more appropriate locking
primitives.
My understanding is that the main goal is to avoid any CODEC operations
running while jack detection runs during resume. I think this could be
done more cleanly by having a completion in the component level resume
function (not the I2C level one) - the core will resume the card in a
thread without blocking the main resume thread used for the rest of the
system which as far as I can tell is what you're trying to avoid
problems with here. set_bias_level() or a DAPM _PRE widget might also
serve. You could possibly even just do the resume and redetection from
the CODEC level resume callback and get the effect you're looking for.
It's possible I've misunderstood exactly what the goal is here so that
might not work, if you could clarify what's going on that might help.
It might be easiest to split this into two patches, one for the main
driver then another adding jack detection (and anything else affected by
the semaphore) - that way the main driver can be reviewed and hopefully
merged quickly while we figure out what's going on with the jack
detection bit.
Otherwise this looks good, a few small issues but nothing major:
> +static int nau8821_digital_mute(struct snd_soc_dai *dai, int mute, int direction)
> +{
> + struct snd_soc_component *component = dai->component;
> + struct nau8821 *nau8821 = snd_soc_component_get_drvdata(component);
> + unsigned int val;
> +
> + val = mute ? NAU8821_DAC_SOFT_MUTE : 0;
Please use normal conditional statements to improve legibility.
> +static int nau8821_component_probe(struct snd_soc_component *component)
> +{
> + struct nau8821 *nau8821 = snd_soc_component_get_drvdata(component);
> + struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> +
> + nau8821->dapm = dapm;
> +
> + snd_soc_dapm_sync(nau8821->dapm);
The core should sync everything at the end of card bringup, no need to
do that in the driver.
> + "nuvoton,jkdet-enable");
> + nau8821->jkdet_pull_enable = device_property_read_bool(dev,
> + "nuvoton,jkdet-pull-enable");
> + nau8821->jkdet_pull_up = device_property_read_bool(dev,
> + "nuvoton,jkdet-pull-up");
> + ret = device_property_read_u32(dev, "nuvoton,jkdet-polarity",
> + &nau8821->jkdet_polarity);
These properties need a DT binding document.
> +static void nau8821_init_regs(struct nau8821 *nau8821)
> +{
> + struct regmap *regmap = nau8821->regmap;
> + /* Disable Boost Driver, Automatic Short circuit protection enable */
> + regmap_update_bits(regmap, NAU8821_R76_BOOST,
> + NAU8821_PRECHARGE_DIS | NAU8821_HP_BOOST_DIS |
I'd expect the boost driver disable and some of the other stuff like DAC
disable to be done automatically by DAPM.
> + /**/
> + regmap_update_bits(regmap, NAU8821_R13_DMIC_CTRL,
Missing comment?
> + /* Pull up IRQ pin */
> + regmap_update_bits(regmap, NAU8821_R0F_INTERRUPT_MASK,
> + NAU8821_IRQ_PIN_PULL_UP | NAU8821_IRQ_PIN_PULL_EN |
> + NAU8821_IRQ_OUTPUT_EN, NAU8821_IRQ_PIN_PULL_UP |
> + NAU8821_IRQ_PIN_PULL_EN | NAU8821_IRQ_OUTPUT_EN);
Should this be a device property?
> + ret = devm_snd_soc_register_component(&i2c->dev, &nau8821_component_driver,
> + &nau8821_dai, 1);
> + pr_err("%s exit ret:%d\n", __func__, ret);
> + return ret;
Remove the pr_err(), guess it's just leftover debugging.
> +static int nau8821_i2c_remove(struct i2c_client *client)
> +{
> + return 0;
> +}
This is empty so can be removed.
Attachment:
signature.asc
Description: PGP signature
