Le 11/06/2024 à 09:45, Mohammad Rafi Shaik a écrit :
From: Prasad Kumpatla <quic_pkumpatl@xxxxxxxxxxx>
This patch adds basic SoundWire codec driver to support for
WCD9370/WCD9375 TX and RX devices.
The WCD9370/WCD9375 has Multi Button Headset Control hardware to
support Headset insertion, type detection, 8 headset buttons detection,
Over Current detection and Impedence measurements.
This patch adds support for this using wcd-mbhc apis.
Co-developed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
Signed-off-by: Prasad Kumpatla <quic_pkumpatl@xxxxxxxxxxx>
Co-developed-by: Mohammad Rafi Shaik <quic_mohs@xxxxxxxxxxx>
Signed-off-by: Mohammad Rafi Shaik <quic_mohs@xxxxxxxxxxx>
---
Hi,
this patch has reached -next, but I have a question.
If I'm correct, I can send a patch, but if the fix can be folded
somewhere, this is also fine for me.
...
+static int wcd937x_probe(struct platform_device *pdev)
+{
+ struct component_match *match = NULL;
+ struct device *dev = &pdev->dev;
+ struct wcd937x_priv *wcd937x;
+ struct wcd_mbhc_config *cfg;
+ int ret;
+
+ wcd937x = devm_kzalloc(dev, sizeof(*wcd937x), GFP_KERNEL);
+ if (!wcd937x)
+ return -ENOMEM;
+
+ dev_set_drvdata(dev, wcd937x);
+ mutex_init(&wcd937x->micb_lock);
+
+ wcd937x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(wcd937x->reset_gpio))
+ return dev_err_probe(dev, PTR_ERR(wcd937x->reset_gpio),
+ "failed to reset wcd gpio\n");
+
+ wcd937x->us_euro_gpio = devm_gpiod_get_optional(dev, "us-euro", GPIOD_OUT_LOW);
+ if (IS_ERR(wcd937x->us_euro_gpio))
+ return dev_err_probe(dev, PTR_ERR(wcd937x->us_euro_gpio),
+ "us-euro swap Control GPIO not found\n");
+
+ cfg = &wcd937x->mbhc_cfg;
+ cfg->swap_gnd_mic = wcd937x_swap_gnd_mic;
+
+ wcd937x->supplies[0].supply = "vdd-rxtx";
+ wcd937x->supplies[1].supply = "vdd-px";
+ wcd937x->supplies[2].supply = "vdd-mic-bias";
+
+ ret = devm_regulator_bulk_get(dev, WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to get supplies\n");
+
+ ret = regulator_bulk_enable(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable supplies\n");
+
+ /* Get the buck separately, as it needs special handling */
+ wcd937x->buck_supply = devm_regulator_get(dev, "vdd-buck");
+ if (IS_ERR(wcd937x->buck_supply))
regulator_bulk_disable() is missing here...
+ return dev_err_probe(dev, PTR_ERR(wcd937x->buck_supply),
+ "Failed to get buck supply\n");
+
+ ret = regulator_enable(wcd937x->buck_supply);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable buck supply\n");
... and here...
Also, should 'buck_supply' be disabled in the error handling path of the
probe and in the remove function? (and maybe even somewhere else related
to wcd937x_codec_enable_vdd_buck())
+
+ wcd937x_dt_parse_micbias_info(dev, wcd937x);
+
+ cfg->mbhc_micbias = MIC_BIAS_2;
+ cfg->anc_micbias = MIC_BIAS_2;
+ cfg->v_hs_max = WCD_MBHC_HS_V_MAX;
+ cfg->num_btn = WCD937X_MBHC_MAX_BUTTONS;
+ cfg->micb_mv = wcd937x->micb2_mv;
+ cfg->linein_th = 5000;
+ cfg->hs_thr = 1700;
+ cfg->hph_thr = 50;
+
+ wcd_dt_parse_mbhc_data(dev, &wcd937x->mbhc_cfg);
+
+ ret = wcd937x_add_slave_components(wcd937x, dev, &match);
+ if (ret)
+ return ret;
... and here...
+
+ wcd937x_reset(wcd937x);
+
+ ret = component_master_add_with_match(dev, &wcd937x_comp_ops, match);
+ if (ret)
+ return ret;
... and here.
Maybe a devm_add_action_ior_reset() could help?
+
+ pm_runtime_set_autosuspend_delay(dev, 1000);
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_set_active(dev);
+ pm_runtime_enable(dev);
+ pm_runtime_idle(dev);
+
+ return ret;
+}
+
+static void wcd937x_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct wcd937x_priv *wcd937x = dev_get_drvdata(dev);
+
+ component_master_del(&pdev->dev, &wcd937x_comp_ops);
+
+ pm_runtime_disable(dev);
+ pm_runtime_set_suspended(dev);
+ pm_runtime_dont_use_autosuspend(dev);
+
+ regulator_bulk_disable(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
+ regulator_bulk_free(WCD937X_MAX_BULK_SUPPLY, wcd937x->supplies);
This has been allocated with devm_regulator_bulk_get(), so this call
looks redundant (but harmless).
+}
...
