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.