Message ID | 1644668672-29790-2-git-send-email-quic_srivasam@quicinc.com (mailing list archive) |
---|---|
State | Accepted |
Commit | 013cc2aea0f62f864c8497b8497299bed4a248fb |
Headers | show |
Series | Add Euro Headset support for wcd938x codec | expand |
Quoting Srinivasa Rao Mandadapu (2022-02-12 04:24:31) > diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c > index eff200a..08d16a9 100644 > --- a/sound/soc/codecs/wcd938x.c > +++ b/sound/soc/codecs/wcd938x.c > @@ -194,6 +194,7 @@ struct wcd938x_priv { > int ear_rx_path; > int variant; > int reset_gpio; > + int us_euro_gpio; > u32 micb1_mv; > u32 micb2_mv; > u32 micb3_mv; > @@ -4196,6 +4197,33 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri > dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); > } > > +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component, bool active) > +{ > + int value; > + > + struct wcd938x_priv *wcd938x; > + > + if (!component) { So component is NULL > + dev_err(component->dev, "%s component is NULL\n", __func__); And now we deref component. Great NULL pointer exception Batman! Please test your code and remove useless checks. It makes the code harder to read and slows things down. > + return false; > + } > + > + wcd938x = snd_soc_component_get_drvdata(component); > + if (!wcd938x) { > + dev_err(component->dev, "%s private data is NULL\n", __func__); Is this possible? I doubt it so can we just remove it? > + return false; > + } > + > + value = gpio_get_value(wcd938x->us_euro_gpio); > + > + gpio_set_value(wcd938x->us_euro_gpio, !value); > + /* 20us sleep required after changing the gpio state*/ Add a space before ending comment with */ > + usleep_range(20, 30); > + > + return true; > +} > + > + > static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device *dev) > { > struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; > @@ -4208,6 +4236,16 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device > return wcd938x->reset_gpio; > } > > + wcd938x->us_euro_gpio = of_get_named_gpio(dev->of_node, "us-euro-gpios", 0); Why do we need to use of GPIO APIs here? Can this driver be converted to use GPIO descriptors via the gpiod APIs? > + if (wcd938x->us_euro_gpio < 0) { > + dev_err(dev, "Failed to get us-euro-gpios gpio: err = %d\n", wcd938x->us_euro_gpio); > + } else { > + cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; > + gpio_direction_output(wcd938x->us_euro_gpio, 0); > + /* 20us sleep required after pulling the reset gpio to LOW */ > + usleep_range(20, 30); > + } > +
On 2/15/2022 3:17 AM, Stephen Boyd wrote: Thanks for your time Stephen!!! > Quoting Srinivasa Rao Mandadapu (2022-02-12 04:24:31) >> diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c >> index eff200a..08d16a9 100644 >> --- a/sound/soc/codecs/wcd938x.c >> +++ b/sound/soc/codecs/wcd938x.c >> @@ -194,6 +194,7 @@ struct wcd938x_priv { >> int ear_rx_path; >> int variant; >> int reset_gpio; >> + int us_euro_gpio; >> u32 micb1_mv; >> u32 micb2_mv; >> u32 micb3_mv; >> @@ -4196,6 +4197,33 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri >> dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); >> } >> >> +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component, bool active) >> +{ >> + int value; >> + >> + struct wcd938x_priv *wcd938x; >> + >> + if (!component) { > So component is NULL > >> + dev_err(component->dev, "%s component is NULL\n", __func__); > And now we deref component. Great NULL pointer exception Batman! Please > test your code and remove useless checks. It makes the code harder to > read and slows things down. Okay. In last minute, changed from pr_err to dev_err and overlooked this mistake. Will remove it. > >> + return false; >> + } >> + >> + wcd938x = snd_soc_component_get_drvdata(component); >> + if (!wcd938x) { >> + dev_err(component->dev, "%s private data is NULL\n", __func__); > Is this possible? I doubt it so can we just remove it? Okay. Will remove it. > >> + return false; >> + } >> + >> + value = gpio_get_value(wcd938x->us_euro_gpio); >> + >> + gpio_set_value(wcd938x->us_euro_gpio, !value); >> + /* 20us sleep required after changing the gpio state*/ > Add a space before ending comment with */ Okay. > >> + usleep_range(20, 30); >> + >> + return true; >> +} >> + >> + >> static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device *dev) >> { >> struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; >> @@ -4208,6 +4236,16 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device >> return wcd938x->reset_gpio; >> } >> >> + wcd938x->us_euro_gpio = of_get_named_gpio(dev->of_node, "us-euro-gpios", 0); > Why do we need to use of GPIO APIs here? Can this driver be converted to > use GPIO descriptors via the gpiod APIs? Okay. will look into it and proceed accordingly!!! > >> + if (wcd938x->us_euro_gpio < 0) { >> + dev_err(dev, "Failed to get us-euro-gpios gpio: err = %d\n", wcd938x->us_euro_gpio); >> + } else { >> + cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; >> + gpio_direction_output(wcd938x->us_euro_gpio, 0); >> + /* 20us sleep required after pulling the reset gpio to LOW */ >> + usleep_range(20, 30); >> + } >> +
diff --git a/sound/soc/codecs/wcd938x.c b/sound/soc/codecs/wcd938x.c index eff200a..08d16a9 100644 --- a/sound/soc/codecs/wcd938x.c +++ b/sound/soc/codecs/wcd938x.c @@ -194,6 +194,7 @@ struct wcd938x_priv { int ear_rx_path; int variant; int reset_gpio; + int us_euro_gpio; u32 micb1_mv; u32 micb2_mv; u32 micb3_mv; @@ -4196,6 +4197,33 @@ static void wcd938x_dt_parse_micbias_info(struct device *dev, struct wcd938x_pri dev_info(dev, "%s: Micbias4 DT property not found\n", __func__); } +static bool wcd938x_swap_gnd_mic(struct snd_soc_component *component, bool active) +{ + int value; + + struct wcd938x_priv *wcd938x; + + if (!component) { + dev_err(component->dev, "%s component is NULL\n", __func__); + return false; + } + + wcd938x = snd_soc_component_get_drvdata(component); + if (!wcd938x) { + dev_err(component->dev, "%s private data is NULL\n", __func__); + return false; + } + + value = gpio_get_value(wcd938x->us_euro_gpio); + + gpio_set_value(wcd938x->us_euro_gpio, !value); + /* 20us sleep required after changing the gpio state*/ + usleep_range(20, 30); + + return true; +} + + static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device *dev) { struct wcd_mbhc_config *cfg = &wcd938x->mbhc_cfg; @@ -4208,6 +4236,16 @@ static int wcd938x_populate_dt_data(struct wcd938x_priv *wcd938x, struct device return wcd938x->reset_gpio; } + wcd938x->us_euro_gpio = of_get_named_gpio(dev->of_node, "us-euro-gpios", 0); + if (wcd938x->us_euro_gpio < 0) { + dev_err(dev, "Failed to get us-euro-gpios gpio: err = %d\n", wcd938x->us_euro_gpio); + } else { + cfg->swap_gnd_mic = wcd938x_swap_gnd_mic; + gpio_direction_output(wcd938x->us_euro_gpio, 0); + /* 20us sleep required after pulling the reset gpio to LOW */ + usleep_range(20, 30); + } + wcd938x->supplies[0].supply = "vdd-rxtx"; wcd938x->supplies[1].supply = "vdd-io"; wcd938x->supplies[2].supply = "vdd-buck";