| Message ID | 1414454528-24240-12-git-send-email-dbaryshkov@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: > Since LoCoMo driver has been converted to provide proper gpiolib > interface, make poodle ASoC platform driver use gpiolib API. Please use subject lines matching the style for the subsystem. > + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); > + if (ret) { > + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", > + ret); > + return ret; > + } I sense a need for devm_gpio_request_array() here. Otherwise this looks fine - ideally it'd move to gpiod but moving to gpiolib is a clear win so no need to block on this. Acked-by: Mark Brown <broonie@kernel.org> with at least the subject line fixed.
On 10/28/2014 05:58 PM, Mark Brown wrote: > On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: >> Since LoCoMo driver has been converted to provide proper gpiolib >> interface, make poodle ASoC platform driver use gpiolib API. > > Please use subject lines matching the style for the subsystem. > >> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); >> + if (ret) { >> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", >> + ret); >> + return ret; >> + } > > I sense a need for devm_gpio_request_array() here. Otherwise this looks > fine - ideally it'd move to gpiod but moving to gpiolib is a clear win > so no need to block on this. I like the idea of devm_gpio_request_array. But I would like not to add additional (core) patches to this patchset. > > Acked-by: Mark Brown <broonie@kernel.org> > > with at least the subject line fixed. Subject line fixed.
On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: >> Since LoCoMo driver has been converted to provide proper gpiolib >> interface, make poodle ASoC platform driver use gpiolib API. > > Please use subject lines matching the style for the subsystem. > >> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); >> + if (ret) { >> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", >> + ret); >> + return ret; >> + } > > I sense a need for devm_gpio_request_array() here. Otherwise this looks > fine - ideally it'd move to gpiod but moving to gpiolib is a clear win > so no need to block on this. I wish Dmitry took the opportunity to move this driver to the gpiod API, especially since doing so would be trivial for this driver. Not a critical requirement though, the present patch is already an improvement. But if you want to do that last step, please have a look at Documentation/gpio/consumer.txt and the "Platform Data" section of Documentation/gpio/board.txt. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote: > On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote: >> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: >>> Since LoCoMo driver has been converted to provide proper gpiolib >>> interface, make poodle ASoC platform driver use gpiolib API. >> >> Please use subject lines matching the style for the subsystem. >> >>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); >>> + if (ret) { >>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", >>> + ret); >>> + return ret; >>> + } >> >> I sense a need for devm_gpio_request_array() here. Otherwise this looks >> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win >> so no need to block on this. > > I wish Dmitry took the opportunity to move this driver to the gpiod > API, especially since doing so would be trivial for this driver. +1 on this. However this platform is not device tree, so this implies setting up a descriptor table for the affected driver(s) to work properly. See Documentation/gpio/board.txt Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: > On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote: >> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote: >>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: >>>> Since LoCoMo driver has been converted to provide proper gpiolib >>>> interface, make poodle ASoC platform driver use gpiolib API. >>> >>> Please use subject lines matching the style for the subsystem. >>> >>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); >>>> + if (ret) { >>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", >>>> + ret); >>>> + return ret; >>>> + } >>> >>> I sense a need for devm_gpio_request_array() here. Otherwise this looks >>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win >>> so no need to block on this. >> >> I wish Dmitry took the opportunity to move this driver to the gpiod >> API, especially since doing so would be trivial for this driver. > > +1 on this. > > However this platform is not device tree, so this implies setting up > a descriptor table for the affected driver(s) to work properly. > See Documentation/gpio/board.txt I checked the gpiod interfaces after original suggestion by Alexandre. Introducing those mapping tables (much like pinctrl tables) look like a duplicate effort if Russell will permit adding a DT support. So I thought that I will reconsider gpiod/pinctrl/etc after fixing LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK and (finally) thinking about device tree support.
On Fri, Oct 31, 2014 at 6:58 PM, Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> wrote: > 2014-10-31 12:52 GMT+03:00 Linus Walleij <linus.walleij@linaro.org>: >> On Wed, Oct 29, 2014 at 4:03 AM, Alexandre Courbot <gnurou@gmail.com> wrote: >>> On Tue, Oct 28, 2014 at 11:58 PM, Mark Brown <broonie@kernel.org> wrote: >>>> On Tue, Oct 28, 2014 at 03:02:04AM +0300, Dmitry Eremin-Solenikov wrote: >>>>> Since LoCoMo driver has been converted to provide proper gpiolib >>>>> interface, make poodle ASoC platform driver use gpiolib API. >>>> >>>> Please use subject lines matching the style for the subsystem. >>>> >>>>> + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); >>>>> + if (ret) { >>>>> + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", >>>>> + ret); >>>>> + return ret; >>>>> + } >>>> >>>> I sense a need for devm_gpio_request_array() here. Otherwise this looks >>>> fine - ideally it'd move to gpiod but moving to gpiolib is a clear win >>>> so no need to block on this. >>> >>> I wish Dmitry took the opportunity to move this driver to the gpiod >>> API, especially since doing so would be trivial for this driver. >> >> +1 on this. >> >> However this platform is not device tree, so this implies setting up >> a descriptor table for the affected driver(s) to work properly. >> See Documentation/gpio/board.txt > > I checked the gpiod interfaces after original suggestion by Alexandre. > > Introducing those mapping tables (much like pinctrl tables) look like > a duplicate effort if Russell will permit adding a DT support. So > I thought that I will reconsider gpiod/pinctrl/etc after fixing > LoCoMo, reiterating IRQ patches, possibly switching to COMMON_CLK > and (finally) thinking about device tree support. Note that the mapping tables are likely not going to end up being huge, and taking that step will allow you to convert to the gpiod interface, something you would probably want to do later when adding DT support anyway. So at the end of the day there would be very little wasted effort: once converting to DT, just add the GPIO properties into the appropriate node, remove the mapping tables, and you're done. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/sound/soc/pxa/poodle.c b/sound/soc/pxa/poodle.c index 21f3400..a593bff 100644 --- a/sound/soc/pxa/poodle.c +++ b/sound/soc/pxa/poodle.c @@ -20,12 +20,12 @@ #include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/platform_device.h> +#include <linux/gpio.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/soc.h> #include <asm/mach-types.h> -#include <asm/hardware/locomo.h> #include <mach/poodle.h> #include <mach/audio.h> @@ -48,16 +48,12 @@ static void poodle_ext_control(struct snd_soc_dapm_context *dapm) /* set up jack connection */ if (poodle_jack_func == POODLE_HP) { /* set = unmute headphone */ - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_L, 1); - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_R, 1); + gpio_set_value(POODLE_GPIO_MUTE_L, 1); + gpio_set_value(POODLE_GPIO_MUTE_R, 1); snd_soc_dapm_enable_pin(dapm, "Headphone Jack"); } else { - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_L, 0); - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_R, 0); + gpio_set_value(POODLE_GPIO_MUTE_L, 0); + gpio_set_value(POODLE_GPIO_MUTE_R, 0); snd_soc_dapm_disable_pin(dapm, "Headphone Jack"); } @@ -85,10 +81,8 @@ static int poodle_startup(struct snd_pcm_substream *substream) static void poodle_shutdown(struct snd_pcm_substream *substream) { /* set = unmute headphone */ - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_L, 1); - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_R, 1); + gpio_set_value(POODLE_GPIO_MUTE_L, 1); + gpio_set_value(POODLE_GPIO_MUTE_R, 1); } static int poodle_hw_params(struct snd_pcm_substream *substream, @@ -178,12 +172,7 @@ static int poodle_set_spk(struct snd_kcontrol *kcontrol, static int poodle_amp_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *k, int event) { - if (SND_SOC_DAPM_EVENT_ON(event)) - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_AMP_ON, 0); - else - locomo_gpio_write(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_AMP_ON, 1); + gpio_set_value(POODLE_GPIO_AMP_ON, !(SND_SOC_DAPM_EVENT_ON(event))); return 0; } @@ -263,25 +252,32 @@ static struct snd_soc_card poodle = { .num_dapm_routes = ARRAY_SIZE(poodle_audio_map), }; +struct gpio poodle_gpios[] = { + { POODLE_GPIO_AMP_ON, GPIOF_OUT_INIT_HIGH, "Amplifier" }, + { POODLE_GPIO_MUTE_L, GPIOF_OUT_INIT_LOW, "Mute left" }, + { POODLE_GPIO_MUTE_R, GPIOF_OUT_INIT_LOW, "Mute right" }, +}; + static int poodle_probe(struct platform_device *pdev) { struct snd_soc_card *card = &poodle; int ret; - locomo_gpio_set_dir(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_AMP_ON, 0); - /* should we mute HP at startup - burning power ?*/ - locomo_gpio_set_dir(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_L, 0); - locomo_gpio_set_dir(&poodle_locomo_device.dev, - POODLE_LOCOMO_GPIO_MUTE_R, 0); + ret = gpio_request_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); + if (ret) { + dev_err(&pdev->dev, "gpio_request_array() failed: %d\n", + ret); + return ret; + } card->dev = &pdev->dev; ret = snd_soc_register_card(card); - if (ret) + if (ret) { dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n", ret); + gpio_free_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); + } return ret; } @@ -290,6 +286,7 @@ static int poodle_remove(struct platform_device *pdev) struct snd_soc_card *card = platform_get_drvdata(pdev); snd_soc_unregister_card(card); + gpio_free_array(poodle_gpios, ARRAY_SIZE(poodle_gpios)); return 0; }
Since LoCoMo driver has been converted to provide proper gpiolib interface, make poodle ASoC platform driver use gpiolib API. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- sound/soc/pxa/poodle.c | 51 ++++++++++++++++++++++++-------------------------- 1 file changed, 24 insertions(+), 27 deletions(-)