| Message ID | 20250328113918.1981069-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: codec: ak5386: Convert to GPIO descriptors | expand |
On Fri, Mar 28, 2025 at 07:39:17PM +0800, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > of_gpio.h is deprecated, update the driver to use GPIO descriptors. > - Use dev_gpiod_get_optional to get GPIO descriptor. devm > - Use gpiod_set_value to configure output value. > > With legacy of_gpio API, the driver set gpio value 1 to power up sets GPIO > AK5386, and set value 0 to power down. > Per datasheet for PDN(reset_gpio in the driver): > Power Down & Reset Mode Pin > “H”: Power up, “L”: Power down & Reset > The AK5386 must be reset once upon power-up. > > There is no in-tree DTS using this codec, and the dt-bindings not bindings does not > specify polarity. Per driver and datasheet, the gpio polarity should be GPIO > active-high which is to power up the codec. So using GPIOD_OUT_LOW > when get the GPIO descriptor matches GPIOF_OUT_INIT_LOW when using > of_gpio API. ... > The Documentation/devicetree/bindings/sound/ak5386.txt not specify > polarity(this seems bug), so per code and datasheet, I think it > should be active-high. I could add a quirk in gpiolib-of to force > active-high or acitive-low if you think needed. I don't think we need a quirk as long as the default is the same, I mean if the DTS is written without setting polarity, would it be active-high or active-low? ... > + if (priv->reset_gpio) Redundant as it duplicates the one in the below call. > + gpiod_set_value(priv->reset_gpio, 1); ... > + if (priv->reset_gpio) Ditto. > + gpiod_set_value(priv->reset_gpio, 0); ... > + priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); > + if (IS_ERR(priv->reset_gpio)) > + return dev_err_probe(dev, PTR_ERR(priv->reset_gpio), + dev_printk.h // or even device.h, depending on the current code base + err.h > + "Failed to get AK5386 reset GPIO\n"); > + gpiod_set_consumer_name(priv->reset_gpio, "AK5386 Reset");
On Fri, Mar 28, 2025 at 03:33:11PM +0200, Andy Shevchenko wrote: >On Fri, Mar 28, 2025 at 07:39:17PM +0800, Peng Fan (OSS) wrote: >> From: Peng Fan <peng.fan@nxp.com> >> >> of_gpio.h is deprecated, update the driver to use GPIO descriptors. >> - Use dev_gpiod_get_optional to get GPIO descriptor. > >devm > >> - Use gpiod_set_value to configure output value. >> >> With legacy of_gpio API, the driver set gpio value 1 to power up > >sets GPIO > >> AK5386, and set value 0 to power down. >> Per datasheet for PDN(reset_gpio in the driver): >> Power Down & Reset Mode Pin >> “H”: Power up, “L”: Power down & Reset >> The AK5386 must be reset once upon power-up. >> >> There is no in-tree DTS using this codec, and the dt-bindings not > >bindings does not > >> specify polarity. Per driver and datasheet, the gpio polarity should be > >GPIO > >> active-high which is to power up the codec. So using GPIOD_OUT_LOW >> when get the GPIO descriptor matches GPIOF_OUT_INIT_LOW when using >> of_gpio API. > >... > >> The Documentation/devicetree/bindings/sound/ak5386.txt not specify >> polarity(this seems bug), so per code and datasheet, I think it >> should be active-high. I could add a quirk in gpiolib-of to force >> active-high or acitive-low if you think needed. > >I don't think we need a quirk as long as the default is the same, >I mean if the DTS is written without setting polarity, would it be >active-high or active-low? Per current gpio driver, of_gpio_n_cells should at least be 2, Not find any driver using 1 in current linux tree. Without polarity, I think of_xlate will not work as expected. Regards, Peng > >... > >> + if (priv->reset_gpio) > >Redundant as it duplicates the one in the below call. > >> + gpiod_set_value(priv->reset_gpio, 1); > >... > >> + if (priv->reset_gpio) > >Ditto. > >> + gpiod_set_value(priv->reset_gpio, 0); > >... > >> + priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); >> + if (IS_ERR(priv->reset_gpio)) >> + return dev_err_probe(dev, PTR_ERR(priv->reset_gpio), > >+ dev_printk.h // or even device.h, depending on the current code base >+ err.h > >> + "Failed to get AK5386 reset GPIO\n"); > >> + gpiod_set_consumer_name(priv->reset_gpio, "AK5386 Reset"); > >-- >With Best Regards, >Andy Shevchenko > >
On Mon, Mar 31, 2025 at 02:03:59PM +0800, Peng Fan wrote: > On Fri, Mar 28, 2025 at 03:33:11PM +0200, Andy Shevchenko wrote: > >On Fri, Mar 28, 2025 at 07:39:17PM +0800, Peng Fan (OSS) wrote: ... > >> The Documentation/devicetree/bindings/sound/ak5386.txt not specify > >> polarity(this seems bug), so per code and datasheet, I think it > >> should be active-high. I could add a quirk in gpiolib-of to force > >> active-high or acitive-low if you think needed. > > > >I don't think we need a quirk as long as the default is the same, > >I mean if the DTS is written without setting polarity, would it be > >active-high or active-low? > > Per current gpio driver, of_gpio_n_cells should at least be 2, > Not find any driver using 1 in current linux tree. Yes... > Without polarity, I think of_xlate will not work as expected. ...and yes, but how is it related to my comment? The default should be sane to make it work, since there is no in-kernel user with wrong polarity there is nothing to fix.
On Mon, Mar 31, 2025 at 10:19:12AM +0300, Andy Shevchenko wrote: >On Mon, Mar 31, 2025 at 02:03:59PM +0800, Peng Fan wrote: >> On Fri, Mar 28, 2025 at 03:33:11PM +0200, Andy Shevchenko wrote: >> >On Fri, Mar 28, 2025 at 07:39:17PM +0800, Peng Fan (OSS) wrote: > >... > >> >> The Documentation/devicetree/bindings/sound/ak5386.txt not specify >> >> polarity(this seems bug), so per code and datasheet, I think it >> >> should be active-high. I could add a quirk in gpiolib-of to force >> >> active-high or acitive-low if you think needed. >> > >> >I don't think we need a quirk as long as the default is the same, >> >I mean if the DTS is written without setting polarity, would it be >> >active-high or active-low? >> >> Per current gpio driver, of_gpio_n_cells should at least be 2, >> Not find any driver using 1 in current linux tree. > >Yes... > >> Without polarity, I think of_xlate will not work as expected. > >...and yes, but how is it related to my comment? The default should be sane >to make it work, since there is no in-kernel user with wrong polarity there >is nothing to fix. I see, nothing to fix. Thanks for help reviewing. Thanks, Peng > >-- >With Best Regards, >Andy Shevchenko > > >
diff --git a/sound/soc/codecs/ak5386.c b/sound/soc/codecs/ak5386.c index 21a44476f48d..ff635141b37f 100644 --- a/sound/soc/codecs/ak5386.c +++ b/sound/soc/codecs/ak5386.c @@ -6,11 +6,10 @@ * (c) 2013 Daniel Mack <zonque@gmail.com> */ +#include <linux/gpio/consumer.h> #include <linux/module.h> -#include <linux/slab.h> -#include <linux/of.h> -#include <linux/of_gpio.h> #include <linux/regulator/consumer.h> +#include <linux/slab.h> #include <sound/soc.h> #include <sound/pcm.h> #include <sound/initval.h> @@ -20,7 +19,7 @@ static const char * const supply_names[] = { }; struct ak5386_priv { - int reset_gpio; + struct gpio_desc *reset_gpio; struct regulator_bulk_data supplies[ARRAY_SIZE(supply_names)]; }; @@ -110,8 +109,8 @@ static int ak5386_hw_params(struct snd_pcm_substream *substream, * the AK5386 in power-down mode (PDN pin = “L”). */ - if (gpio_is_valid(priv->reset_gpio)) - gpio_set_value(priv->reset_gpio, 1); + if (priv->reset_gpio) + gpiod_set_value(priv->reset_gpio, 1); return 0; } @@ -122,8 +121,8 @@ static int ak5386_hw_free(struct snd_pcm_substream *substream, struct snd_soc_component *component = dai->component; struct ak5386_priv *priv = snd_soc_component_get_drvdata(component); - if (gpio_is_valid(priv->reset_gpio)) - gpio_set_value(priv->reset_gpio, 0); + if (priv->reset_gpio) + gpiod_set_value(priv->reset_gpio, 0); return 0; } @@ -177,14 +176,12 @@ static int ak5386_probe(struct platform_device *pdev) if (ret < 0) return ret; - priv->reset_gpio = of_get_named_gpio(dev->of_node, - "reset-gpio", 0); + priv->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); + if (IS_ERR(priv->reset_gpio)) + return dev_err_probe(dev, PTR_ERR(priv->reset_gpio), + "Failed to get AK5386 reset GPIO\n"); - if (gpio_is_valid(priv->reset_gpio)) - if (devm_gpio_request_one(dev, priv->reset_gpio, - GPIOF_OUT_INIT_LOW, - "AK5386 Reset")) - priv->reset_gpio = -EINVAL; + gpiod_set_consumer_name(priv->reset_gpio, "AK5386 Reset"); return devm_snd_soc_register_component(dev, &soc_component_ak5386, &ak5386_dai, 1);