| Message ID | 20230928-descriptors-asoc-rockchip-v1-1-a142a42d4787@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Convert Rockchip ASoC drivers to GPIO descriptors | expand |
On 27/09/2023 11:47 pm, Linus Walleij wrote: > This converts the Rockchip RK3288 HDMI driver to use GPIO > descriptors: > > - Look up the HP EN GPIO as a descriptor and handle it directly. > > - Let the Jack detection core obtain and handle the HP detection > GPIO, just pass the right name and gpiod_dev and it will > do the job. > > - As the probe() code is very insistent on getting valid > GPIOs out of the device before it will continue, there > is no point to carry all the code handling the GPIOs as > optional, drop all these checks. Isn't it allowing them to be optional as long as of_get_named_gpio() returns -ENODEV (which I guess may come out of the chip->of_xlate callback)? Or is it implied that that's never actually been able to happen? (just curious...) FWIW the DT binding does define them as optional, but the only in-tree user provides both, so it seems unlikely to pose a regression in practice even if we did just drop the notional support. Thanks, Robin. > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > sound/soc/rockchip/rk3288_hdmi_analog.c | 54 +++++++++++---------------------- > 1 file changed, 17 insertions(+), 37 deletions(-) > > diff --git a/sound/soc/rockchip/rk3288_hdmi_analog.c b/sound/soc/rockchip/rk3288_hdmi_analog.c > index 0c6bd9a019db..7199f991ec26 100644 > --- a/sound/soc/rockchip/rk3288_hdmi_analog.c > +++ b/sound/soc/rockchip/rk3288_hdmi_analog.c > @@ -12,8 +12,7 @@ > #include <linux/module.h> > #include <linux/platform_device.h> > #include <linux/slab.h> > -#include <linux/gpio.h> > -#include <linux/of_gpio.h> > +#include <linux/gpio/consumer.h> > #include <sound/core.h> > #include <sound/jack.h> > #include <sound/pcm.h> > @@ -26,8 +25,7 @@ > #define DRV_NAME "rk3288-snd-hdmi-analog" > > struct rk_drvdata { > - int gpio_hp_en; > - int gpio_hp_det; > + struct gpio_desc *gpio_hp_en; > }; > > static int rk_hp_power(struct snd_soc_dapm_widget *w, > @@ -35,11 +33,8 @@ static int rk_hp_power(struct snd_soc_dapm_widget *w, > { > struct rk_drvdata *machine = snd_soc_card_get_drvdata(w->dapm->card); > > - if (!gpio_is_valid(machine->gpio_hp_en)) > - return 0; > - > - gpio_set_value_cansleep(machine->gpio_hp_en, > - SND_SOC_DAPM_EVENT_ON(event)); > + gpiod_set_value_cansleep(machine->gpio_hp_en, > + SND_SOC_DAPM_EVENT_ON(event)); > > return 0; > } > @@ -113,24 +108,23 @@ static int rk_hw_params(struct snd_pcm_substream *substream, > } > > static struct snd_soc_jack_gpio rk_hp_jack_gpio = { > - .name = "Headphone detection", > + .name = "rockchip,hp-det", > .report = SND_JACK_HEADPHONE, > .debounce_time = 150 > }; > > static int rk_init(struct snd_soc_pcm_runtime *runtime) > { > - struct rk_drvdata *machine = snd_soc_card_get_drvdata(runtime->card); > + struct snd_soc_card *card = runtime->card; > + struct device *dev = card->dev; > > /* Enable Headset Jack detection */ > - if (gpio_is_valid(machine->gpio_hp_det)) { > - snd_soc_card_jack_new_pins(runtime->card, "Headphone Jack", > - SND_JACK_HEADPHONE, &headphone_jack, > - headphone_jack_pins, > - ARRAY_SIZE(headphone_jack_pins)); > - rk_hp_jack_gpio.gpio = machine->gpio_hp_det; > - snd_soc_jack_add_gpios(&headphone_jack, 1, &rk_hp_jack_gpio); > - } > + rk_hp_jack_gpio.gpiod_dev = dev; > + snd_soc_card_jack_new_pins(runtime->card, "Headphone Jack", > + SND_JACK_HEADPHONE, &headphone_jack, > + headphone_jack_pins, > + ARRAY_SIZE(headphone_jack_pins)); > + snd_soc_jack_add_gpios(&headphone_jack, 1, &rk_hp_jack_gpio); > > return 0; > } > @@ -182,24 +176,10 @@ static int snd_rk_mc_probe(struct platform_device *pdev) > > card->dev = &pdev->dev; > > - machine->gpio_hp_det = of_get_named_gpio(np, > - "rockchip,hp-det-gpios", 0); > - if (!gpio_is_valid(machine->gpio_hp_det) && machine->gpio_hp_det != -ENODEV) > - return machine->gpio_hp_det; > - > - machine->gpio_hp_en = of_get_named_gpio(np, > - "rockchip,hp-en-gpios", 0); > - if (!gpio_is_valid(machine->gpio_hp_en) && machine->gpio_hp_en != -ENODEV) > - return machine->gpio_hp_en; > - > - if (gpio_is_valid(machine->gpio_hp_en)) { > - ret = devm_gpio_request_one(&pdev->dev, machine->gpio_hp_en, > - GPIOF_OUT_INIT_LOW, "hp_en"); > - if (ret) { > - dev_err(card->dev, "cannot get hp_en gpio\n"); > - return ret; > - } > - } > + machine->gpio_hp_en = devm_gpiod_get(&pdev->dev, "rockchip,hp-en", GPIOD_OUT_LOW); > + if (IS_ERR(machine->gpio_hp_en)) > + return PTR_ERR(machine->gpio_hp_en); > + gpiod_set_consumer_name(machine->gpio_hp_en, "hp_en"); > > ret = snd_soc_of_parse_card_name(card, "rockchip,model"); > if (ret) { >
On Thu, Sep 28, 2023 at 01:34:56PM +0100, Robin Murphy wrote: > On 27/09/2023 11:47 pm, Linus Walleij wrote: > > - As the probe() code is very insistent on getting valid > > GPIOs out of the device before it will continue, there > > is no point to carry all the code handling the GPIOs as > > optional, drop all these checks. > Isn't it allowing them to be optional as long as of_get_named_gpio() returns > -ENODEV (which I guess may come out of the chip->of_xlate callback)? Or is > it implied that that's never actually been able to happen? Right, I *think* it's just trying to open code optional GPIO support (possibly it predates the core helpers?).
diff --git a/sound/soc/rockchip/rk3288_hdmi_analog.c b/sound/soc/rockchip/rk3288_hdmi_analog.c index 0c6bd9a019db..7199f991ec26 100644 --- a/sound/soc/rockchip/rk3288_hdmi_analog.c +++ b/sound/soc/rockchip/rk3288_hdmi_analog.c @@ -12,8 +12,7 @@ #include <linux/module.h> #include <linux/platform_device.h> #include <linux/slab.h> -#include <linux/gpio.h> -#include <linux/of_gpio.h> +#include <linux/gpio/consumer.h> #include <sound/core.h> #include <sound/jack.h> #include <sound/pcm.h> @@ -26,8 +25,7 @@ #define DRV_NAME "rk3288-snd-hdmi-analog" struct rk_drvdata { - int gpio_hp_en; - int gpio_hp_det; + struct gpio_desc *gpio_hp_en; }; static int rk_hp_power(struct snd_soc_dapm_widget *w, @@ -35,11 +33,8 @@ static int rk_hp_power(struct snd_soc_dapm_widget *w, { struct rk_drvdata *machine = snd_soc_card_get_drvdata(w->dapm->card); - if (!gpio_is_valid(machine->gpio_hp_en)) - return 0; - - gpio_set_value_cansleep(machine->gpio_hp_en, - SND_SOC_DAPM_EVENT_ON(event)); + gpiod_set_value_cansleep(machine->gpio_hp_en, + SND_SOC_DAPM_EVENT_ON(event)); return 0; } @@ -113,24 +108,23 @@ static int rk_hw_params(struct snd_pcm_substream *substream, } static struct snd_soc_jack_gpio rk_hp_jack_gpio = { - .name = "Headphone detection", + .name = "rockchip,hp-det", .report = SND_JACK_HEADPHONE, .debounce_time = 150 }; static int rk_init(struct snd_soc_pcm_runtime *runtime) { - struct rk_drvdata *machine = snd_soc_card_get_drvdata(runtime->card); + struct snd_soc_card *card = runtime->card; + struct device *dev = card->dev; /* Enable Headset Jack detection */ - if (gpio_is_valid(machine->gpio_hp_det)) { - snd_soc_card_jack_new_pins(runtime->card, "Headphone Jack", - SND_JACK_HEADPHONE, &headphone_jack, - headphone_jack_pins, - ARRAY_SIZE(headphone_jack_pins)); - rk_hp_jack_gpio.gpio = machine->gpio_hp_det; - snd_soc_jack_add_gpios(&headphone_jack, 1, &rk_hp_jack_gpio); - } + rk_hp_jack_gpio.gpiod_dev = dev; + snd_soc_card_jack_new_pins(runtime->card, "Headphone Jack", + SND_JACK_HEADPHONE, &headphone_jack, + headphone_jack_pins, + ARRAY_SIZE(headphone_jack_pins)); + snd_soc_jack_add_gpios(&headphone_jack, 1, &rk_hp_jack_gpio); return 0; } @@ -182,24 +176,10 @@ static int snd_rk_mc_probe(struct platform_device *pdev) card->dev = &pdev->dev; - machine->gpio_hp_det = of_get_named_gpio(np, - "rockchip,hp-det-gpios", 0); - if (!gpio_is_valid(machine->gpio_hp_det) && machine->gpio_hp_det != -ENODEV) - return machine->gpio_hp_det; - - machine->gpio_hp_en = of_get_named_gpio(np, - "rockchip,hp-en-gpios", 0); - if (!gpio_is_valid(machine->gpio_hp_en) && machine->gpio_hp_en != -ENODEV) - return machine->gpio_hp_en; - - if (gpio_is_valid(machine->gpio_hp_en)) { - ret = devm_gpio_request_one(&pdev->dev, machine->gpio_hp_en, - GPIOF_OUT_INIT_LOW, "hp_en"); - if (ret) { - dev_err(card->dev, "cannot get hp_en gpio\n"); - return ret; - } - } + machine->gpio_hp_en = devm_gpiod_get(&pdev->dev, "rockchip,hp-en", GPIOD_OUT_LOW); + if (IS_ERR(machine->gpio_hp_en)) + return PTR_ERR(machine->gpio_hp_en); + gpiod_set_consumer_name(machine->gpio_hp_en, "hp_en"); ret = snd_soc_of_parse_card_name(card, "rockchip,model"); if (ret) {
This converts the Rockchip RK3288 HDMI driver to use GPIO descriptors: - Look up the HP EN GPIO as a descriptor and handle it directly. - Let the Jack detection core obtain and handle the HP detection GPIO, just pass the right name and gpiod_dev and it will do the job. - As the probe() code is very insistent on getting valid GPIOs out of the device before it will continue, there is no point to carry all the code handling the GPIOs as optional, drop all these checks. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- sound/soc/rockchip/rk3288_hdmi_analog.c | 54 +++++++++++---------------------- 1 file changed, 17 insertions(+), 37 deletions(-)