Hi
On Sun, Aug 30, 2015 at 7:47 AM, Mark Brown broonie@kernel.org wrote:
On Fri, Aug 28, 2015 at 04:37:50PM -0700, Anatol Pomozov wrote:
SOC_SINGLE_TLV("Frontend PGA Gain", NAU8825_REG_POWER_UP_CONTROL,8, 37, 0, fepga_gain_tlv),All volume controls need to end in Volume as per ControlNames.txt.
Done.
default:return -EINVAL;}regmap_update_bits(nau8825->regmap, NAU8825_REG_I2S_PCM_CTRL1,Blank line here.
Done.
/* Bias and clock are needed for button detection */regmap_update_bits(nau8825->regmap, NAU8825_REG_BIAS_ADJ,NAU8825_BIAS_VMID, NAU8825_BIAS_VMID);regmap_update_bits(nau8825->regmap, NAU8825_REG_BOOST,NAU8825_GLOBAL_BIAS_EN, NAU8825_GLOBAL_BIAS_EN);These sound like DAPM widgets which should be being eneabled when jack detection is enabled.
The bias/vmid need to be enabled before clock/interruptions. Moving it to set_bias or to a widget makes interrupts non-functional. bias/vmid is also needed for capture and playback. So I think it is ok to enable it here. Later Nuvoton engineers will look at power management closer and might find better place for bias setup.
/* Ground HP Outputs[1:0], needed for headset auto detection* Enable Automatic Mic/Gnd switching reading on insert interrupt[6] */regmap_update_bits(regmap, NAU8825_REG_HSD_CTRL,NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L,NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L);I'm still not happy to see the unconditional enabling of detection and switching.
Moved to enable_jack_detection(). Note that it looses "auto mic type detection" functionality if jack_detection is disabled. Such auto mic detection might be useful even in case if there is no physical jack and input/output routes are soldered statically.
if (!nau8825->mclk_freq)clk_prepare_enable(nau8825->mclk);You should be checking the return value here.
Done.
if (nau8825->mclk_freq != freq) {nau8825->mclk_freq = freq;freq = clk_round_rate(nau8825->mclk, freq);clk_set_rate(nau8825->mclk, freq);Likewise here and in other cases where you're setting clocks.
nau8825->jkdet_enable = device_property_read_bool(dev,"nuvoton,jkdet-enable");This DT parsing stuff should really be moved out into a separate function
Thanks. Moved.
which is only called (or only does anything) if there is actually DT data (if there is an of_node set).
device_property_read_* handles both DTS and ACPI that covers most (all?) modern systems.
Or are you talking about case when a platform driver passes pre-initialized data from using dev_get_platdata() (like what rt5677 does)? I assume this is a deprecated style that predates DTS/ACPI style configuration.
if (nau8825->irq) {int ret = devm_request_threaded_irq(dev, nau8825->irq, NULL,nau8825_interrupt, IRQF_TRIGGER_LOW | IRQF_ONESHOT,"nau8825", nau8825);if (ret) {dev_err(dev, "Cannot request irq %d (%d)\n",nau8825->irq, ret);return ret;}}So we actually handle systems with no interrupt wired up?
I think it worth adding such configuration. Without the IRQ line this chip looses large part of its functionality (jack detection, mic auto detect, buttons, cross-talk configuration) but at least playback should work.
That seems to make all the non-optional enabling of the jack detection interrupts a bit odd.
Moved jack irq enabling to enable_jack_detection()
+static struct i2c_driver nau8825_driver = {
.driver = {.name = "nau8825",.owner = THIS_MODULE,},.probe = nau8825_i2c_probe,.remove = nau8825_i2c_remove,.id_table = nau8825_i2c_ids,+};
This has a DT binding but no OF IDs defined. It is true that Linux will currently try to guess a suitable driver based on the I2C ID table but it is bad practice to rely on this behaviour, we do see ID collisions sometimes (for example both WonderMedia and Wolfson using WM for their parts).
Done. Thanks for the pointer.