| Message ID | 1446657164-25012-1-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: > The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the > kernel, declare a gpio chip. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- You should probably make a seperate driver within GPIO for this and then tie the two together, using an MFD. I appreciate that is more work but it is likely a nicer solution overall. Thanks, Charles
Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the >> kernel, declare a gpio chip. >> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> >> --- > > You should probably make a seperate driver within GPIO for this > and then tie the two together, using an MFD. I appreciate that is > more work but it is likely a nicer solution overall. I'd like to first have a confirmation from : - Mark (Brown) - and Lee (Jones) The confirmation I'm looking for states that : - the wm9713 should have a part in the mfd tree - the gpio part should be in drivers/gpio - the sound soc codecs will remain as is - if the future driver/mfd/wm9713.c is technically sound, it will be accepted I remember at least one example where the MFD approach was rejected from mfd tree for pxa gpios, so I won't work unless I have a confirmation from both maintainers. Thanks.
On Wed, Nov 04, 2015 at 08:35:18PM +0100, Robert Jarzmik wrote: > Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: > > > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: > >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the > >> kernel, declare a gpio chip. > >> > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > >> --- > > > > You should probably make a seperate driver within GPIO for this > > and then tie the two together, using an MFD. I appreciate that is > > more work but it is likely a nicer solution overall. > > I'd like to first have a confirmation from : > - Mark (Brown) > - and Lee (Jones) That seems like a good idea to me as well :-) Thanks, Charles
On Wed, Nov 04, 2015 at 06:33:22PM +0000, Charles Keepax wrote: > You should probably make a seperate driver within GPIO for this > and then tie the two together, using an MFD. I appreciate that is > more work but it is likely a nicer solution overall. GPIO chips are small and simple enough that we've not worried about it historically. However my question here is why this is wm9713 specific - the GPIO functionality is part of the AC'97 spec and present on many AC'97 chips so it seems sensible to have something at the AC'97 level. Unfortunately a quick check seems not to show anything indicating this in the AC'97 capability information so we might want to make it opt in.
On Fri, Nov 06, 2015 at 09:29:13AM +0000, Lee Jones wrote: > On Wed, 04 Nov 2015, Robert Jarzmik wrote: > > > Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: > > > > > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: > > >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the > > >> kernel, declare a gpio chip. > > >> > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > >> --- > > > > > > You should probably make a seperate driver within GPIO for this > > > and then tie the two together, using an MFD. I appreciate that is > > > more work but it is likely a nicer solution overall. > > > > I'd like to first have a confirmation from : > > - Mark (Brown) > > - and Lee (Jones) > > > > The confirmation I'm looking for states that : > > - the wm9713 should have a part in the mfd tree > > - the gpio part should be in drivers/gpio > > - the sound soc codecs will remain as is > > - if the future driver/mfd/wm9713.c is technically sound, it will be accepted > > > > I remember at least one example where the MFD approach was rejected from mfd > > tree for pxa gpios, so I won't work unless I have a confirmation from both > > maintainers. > > I have no idea what you're talking about. Context please? Apologies Lee, we are discussing a patch that adds a GPIO driver into an AC97 CODEC. I had suggested that perhaps we should put the GPIO driver as a seperate driver under GPIO and link the two with an MFD. But Mark has already replied in the thread to say that he doesn't think that will be necessary. Although he did raise some concerns that perhaps it could be done more generally as it should apply to other AC97 CODECs as well. So I think you can probably safely ignore this for now, sorry for the noise. Thanks, Charles
On Wed, 04 Nov 2015, Robert Jarzmik wrote: > Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: > > > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: > >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the > >> kernel, declare a gpio chip. > >> > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > >> --- > > > > You should probably make a seperate driver within GPIO for this > > and then tie the two together, using an MFD. I appreciate that is > > more work but it is likely a nicer solution overall. > > I'd like to first have a confirmation from : > - Mark (Brown) > - and Lee (Jones) > > The confirmation I'm looking for states that : > - the wm9713 should have a part in the mfd tree > - the gpio part should be in drivers/gpio > - the sound soc codecs will remain as is > - if the future driver/mfd/wm9713.c is technically sound, it will be accepted > > I remember at least one example where the MFD approach was rejected from mfd > tree for pxa gpios, so I won't work unless I have a confirmation from both > maintainers. I have no idea what you're talking about. Context please?
On Fri, 06 Nov 2015, Charles Keepax wrote: > On Fri, Nov 06, 2015 at 09:29:13AM +0000, Lee Jones wrote: > > On Wed, 04 Nov 2015, Robert Jarzmik wrote: > > > > > Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: > > > > > > > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: > > > >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the > > > >> kernel, declare a gpio chip. > > > >> > > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > > > >> --- > > > > > > > > You should probably make a seperate driver within GPIO for this > > > > and then tie the two together, using an MFD. I appreciate that is > > > > more work but it is likely a nicer solution overall. > > > > > > I'd like to first have a confirmation from : > > > - Mark (Brown) > > > - and Lee (Jones) > > > > > > The confirmation I'm looking for states that : > > > - the wm9713 should have a part in the mfd tree > > > - the gpio part should be in drivers/gpio > > > - the sound soc codecs will remain as is > > > - if the future driver/mfd/wm9713.c is technically sound, it will be accepted > > > > > > I remember at least one example where the MFD approach was rejected from mfd > > > tree for pxa gpios, so I won't work unless I have a confirmation from both > > > maintainers. > > > > I have no idea what you're talking about. Context please? > > Apologies Lee, we are discussing a patch that adds a GPIO driver > into an AC97 CODEC. I had suggested that perhaps we should put > the GPIO driver as a seperate driver under GPIO and link the two > with an MFD. But Mark has already replied in the thread to say > that he doesn't think that will be necessary. Although he did > raise some concerns that perhaps it could be done more generally > as it should apply to other AC97 CODECs as well. > > So I think you can probably safely ignore this for now, sorry > for the noise. Roger that. Thanks for the explanation.
Lee Jones <lee.jones@linaro.org> writes: > On Fri, 06 Nov 2015, Charles Keepax wrote: > >> On Fri, Nov 06, 2015 at 09:29:13AM +0000, Lee Jones wrote: >> > On Wed, 04 Nov 2015, Robert Jarzmik wrote: >> > >> > > Charles Keepax <ckeepax@opensource.wolfsonmicro.com> writes: >> > > >> > > > On Wed, Nov 04, 2015 at 06:12:44PM +0100, Robert Jarzmik wrote: >> > > >> The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the >> > > >> kernel, declare a gpio chip. >> > > >> >> > > >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> >> >> Apologies Lee, we are discussing a patch that adds a GPIO driver >> into an AC97 CODEC. I had suggested that perhaps we should put >> the GPIO driver as a seperate driver under GPIO and link the two >> with an MFD. But Mark has already replied in the thread to say >> that he doesn't think that will be necessary. Although he did >> raise some concerns that perhaps it could be done more generally >> as it should apply to other AC97 CODECs as well. >> >> So I think you can probably safely ignore this for now, sorry >> for the noise. Ok, so where I should target this code at ? Should this land in sound/soc/soc-ac97.c ? Or somewhere else ? I'd like to see where you think the init_gpio() and free_gpio() should be put. Cheers.
On Fri, Nov 06, 2015 at 09:47:35PM +0100, Robert Jarzmik wrote: > Ok, so where I should target this code at ? Should this land in > sound/soc/soc-ac97.c ? Or somewhere else ? I'd like to see where you think the > init_gpio() and free_gpio() should be put. Sounds like a reasonable place, or possibly even the ALSA level AC'97 stuff (though I'd question if anyone would ever use it outside of an ASoC system).
diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index 79e143625ac3..904fe4fc5bf1 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -19,6 +19,7 @@ #include <linux/slab.h> #include <linux/module.h> #include <linux/device.h> +#include <linux/gpio/driver.h> #include <linux/regmap.h> #include <sound/core.h> #include <sound/pcm.h> @@ -33,11 +34,21 @@ #define WM9713_VENDOR_ID 0x574d4c13 #define WM9713_VENDOR_ID_MASK 0xffffffff +#define AC97_GPIO_PUSH_PULL 0x58 + +struct wm9713_gpio_priv { +#ifdef CONFIG_GPIOLIB + struct gpio_chip gpio_chip; +#endif + struct snd_soc_codec *codec; +}; + struct wm9713_priv { struct snd_ac97 *ac97; u32 pll_in; /* PLL input frequency */ unsigned int hp_mixer[2]; struct mutex lock; + struct wm9713_gpio_priv *gpio_priv; }; #define HPL_MIXER 0 @@ -1202,10 +1213,116 @@ static int wm9713_soc_resume(struct snd_soc_codec *codec) return ret; } +#ifdef CONFIG_GPIOLIB +static inline struct snd_soc_codec *gpio_to_codec(struct gpio_chip *chip) +{ + struct wm9713_gpio_priv *gpio_priv = + container_of(chip, struct wm9713_gpio_priv, gpio_chip); + + return gpio_priv->codec; +} + +static int wm9713_gpio_request(struct gpio_chip *chip, unsigned offset) +{ + if (offset >= WM9713_NUM_GPIOS) + return -EINVAL; + + return 0; +} + +static int wm9713_gpio_direction_in(struct gpio_chip *chip, unsigned offset) +{ + struct snd_soc_codec *codec = gpio_to_codec(chip); + + return snd_soc_update_bits(codec, AC97_GPIO_CFG, + 1 << (offset + 1), 1 << (offset + 1)); +} + +static int wm9713_gpio_get(struct gpio_chip *chip, unsigned offset) +{ + struct snd_soc_codec *codec = gpio_to_codec(chip); + int ret; + + ret = snd_soc_read(codec, AC97_GPIO_STATUS); + + return ret < 0 ? ret : ret & (1 << (offset + 1)); +} + +static void wm9713_gpio_set(struct gpio_chip *chip, unsigned offset, int value) +{ + struct snd_soc_codec *codec = gpio_to_codec(chip); + + snd_soc_update_bits(codec, AC97_GPIO_PUSH_PULL, + 1 << offset | (1 << (offset + 8)), + 1 << (offset + 8 * !!value)); +} + +static int wm9713_gpio_direction_out(struct gpio_chip *chip, + unsigned offset, int value) +{ + struct snd_soc_codec *codec = gpio_to_codec(chip); + + wm9713_gpio_set(chip, offset, value); + + return snd_soc_update_bits(codec, AC97_GPIO_CFG, 1 << (offset + 1), 0); +} + +static struct gpio_chip wm9713_gpio_chip = { + .label = "wm9713", + .owner = THIS_MODULE, + .request = wm9713_gpio_request, + .direction_input = wm9713_gpio_direction_in, + .get = wm9713_gpio_get, + .direction_output = wm9713_gpio_direction_out, + .set = wm9713_gpio_set, + .can_sleep = 1, +}; + +static int wm9713_init_gpio(struct snd_soc_codec *codec) +{ + struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec); + struct wm9713_gpio_priv *gpio_priv; + int ret; + + gpio_priv = devm_kzalloc(codec->dev, sizeof(*wm9713->gpio_priv), + GFP_KERNEL); + if (!gpio_priv) + return -ENOMEM; + wm9713->gpio_priv = gpio_priv; + gpio_priv->codec = codec; + gpio_priv->gpio_chip = wm9713_gpio_chip; + gpio_priv->gpio_chip.ngpio = WM9713_NUM_GPIOS; + gpio_priv->gpio_chip.dev = codec->dev; + gpio_priv->gpio_chip.base = -1; + + ret = gpiochip_add(&gpio_priv->gpio_chip); + if (ret != 0) + dev_err(codec->dev, "Failed to add GPIOs: %d\n", ret); + return ret; +} + +static void wm9713_free_gpio(struct snd_soc_codec *codec) +{ + struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec); + + gpiochip_remove(&wm9713->gpio_priv->gpio_chip); +} +#else +static int wm9713_init_gpio(struct snd_soc_codec *codec) +{ + return 0; +} + +static void wm9713_free_gpio(struct snd_soc_codec *codec) +{ +} +#endif + static int wm9713_soc_probe(struct snd_soc_codec *codec) { struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec); struct regmap *regmap; + int ret; wm9713->ac97 = snd_soc_new_ac97_codec(codec, WM9713_VENDOR_ID, WM9713_VENDOR_ID_MASK); @@ -1223,6 +1340,11 @@ static int wm9713_soc_probe(struct snd_soc_codec *codec) /* unmute the adc - move to kcontrol */ snd_soc_update_bits(codec, AC97_CD, 0x7fff, 0x0000); + ret = wm9713_init_gpio(codec); + if (ret) { + snd_soc_free_ac97_codec(wm9713->ac97); + return ret; + } return 0; } @@ -1230,6 +1352,7 @@ static int wm9713_soc_remove(struct snd_soc_codec *codec) { struct wm9713_priv *wm9713 = snd_soc_codec_get_drvdata(codec); + wm9713_free_gpio(codec); snd_soc_codec_exit_regmap(codec); snd_soc_free_ac97_codec(wm9713->ac97); return 0; diff --git a/sound/soc/codecs/wm9713.h b/sound/soc/codecs/wm9713.h index 53df11b1f727..d72f96e653d1 100644 --- a/sound/soc/codecs/wm9713.h +++ b/sound/soc/codecs/wm9713.h @@ -45,4 +45,5 @@ #define WM9713_DAI_AC97_AUX 1 #define WM9713_DAI_PCM_VOICE 2 +#define WM9713_NUM_GPIOS 8 #endif
The Wolfson WM9713 provides 8 GPIOs. If the gpiolib is compiled in the kernel, declare a gpio chip. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- sound/soc/codecs/wm9713.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++ sound/soc/codecs/wm9713.h | 1 + 2 files changed, 124 insertions(+)