| Message ID | 20250324-wcd-gpiod-v1-1-27afa472e331@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: codec: wcd93xx: Convert to GPIO descriptors | expand |
Hi Peng, thanks for your patch, and thanks a *LOT* for working on the gpio descriptor task!! On Mon, Mar 24, 2025 at 3:28 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > From: Peng Fan <peng.fan@nxp.com> > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > - Use dev_gpiod_get to get GPIO descriptor. > - Use gpiod_set_value to configure output value. > > With legacy of_gpio API, the driver set gpio value 0 to assert reset, > and 1 to deassert reset. And the reset-gpios use GPIO_ACTIVE_LOW flag in > DTS, so set GPIOD_ASIS when get GPIO descriptors, and set value 1 means > output low, set value 0 means output high with gpiod API. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> The patch as such is perfect: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> However we need to watch out for users: $ git grep qcom,wcd939 arch/arm64/boot/dts/qcom/sm8650-hdk.dts: compatible = "qcom,wcd9395-codec", "qcom,wcd9390-codec"; arch/arm64/boot/dts/qcom/sm8650-hdk.dts: compatible = "qcom,wcd9395-usbss", "qcom,wcd9390-usbss"; reset-gpios = <&tlmm 107 GPIO_ACTIVE_LOW>; arch/arm64/boot/dts/qcom/sm8650-qrd.dts: compatible = "qcom,wcd9395-codec", "qcom,wcd9390-codec"; arch/arm64/boot/dts/qcom/sm8650-qrd.dts: compatible = "qcom,wcd9395-usbss", "qcom,wcd9390-usbss"; reset-gpios = <&tlmm 107 GPIO_ACTIVE_LOW>; Mention in the commit message that the in-tree DTS files have the right polarity set up already so we can expect this to "just work". I would also mention that the current code in the driver is quite ugly: it doesn't even request the GPIO before starting to use it :/ Yours, Linus Walleij
diff --git a/sound/soc/codecs/wcd939x.c b/sound/soc/codecs/wcd939x.c index 0a87a79772da6c0ed3c7dd7d098e949b9cead2a4..f8d7d03f84fcbca57826dd43e9281aff5b225525 100644 --- a/sound/soc/codecs/wcd939x.c +++ b/sound/soc/codecs/wcd939x.c @@ -15,7 +15,6 @@ #include <linux/pm_runtime.h> #include <linux/component.h> #include <sound/tlv.h> -#include <linux/of_gpio.h> #include <linux/of_graph.h> #include <linux/of.h> #include <sound/jack.h> @@ -201,7 +200,7 @@ struct wcd939x_priv { u32 hph_mode; u32 tx_mode[TX_ADC_MAX]; int variant; - int reset_gpio; + struct gpio_desc *reset_gpio; u32 micb1_mv; u32 micb2_mv; u32 micb3_mv; @@ -3239,10 +3238,11 @@ static int wcd939x_populate_dt_data(struct wcd939x_priv *wcd939x, struct device #endif /* CONFIG_TYPEC */ int ret; - wcd939x->reset_gpio = of_get_named_gpio(dev->of_node, "reset-gpios", 0); - if (wcd939x->reset_gpio < 0) - return dev_err_probe(dev, wcd939x->reset_gpio, - "Failed to get reset gpio\n"); + wcd939x->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS); + if (IS_ERR(wcd939x->reset_gpio)) { + ret = PTR_ERR(wcd939x->reset_gpio); + return dev_err_probe(dev, ret, "Failed to get reset gpio\n"); + } wcd939x->supplies[0].supply = "vdd-rxtx"; wcd939x->supplies[1].supply = "vdd-io"; @@ -3290,10 +3290,10 @@ static int wcd939x_populate_dt_data(struct wcd939x_priv *wcd939x, struct device static int wcd939x_reset(struct wcd939x_priv *wcd939x) { - gpio_direction_output(wcd939x->reset_gpio, 0); + gpiod_set_value(wcd939x->reset_gpio, 1); /* 20us sleep required after pulling the reset gpio to LOW */ usleep_range(20, 30); - gpio_set_value(wcd939x->reset_gpio, 1); + gpiod_set_value(wcd939x->reset_gpio, 0); /* 20us sleep required after pulling the reset gpio to HIGH */ usleep_range(20, 30);