| Message ID | 1429134141-17924-1-git-send-email-cernekee@chromium.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 04/15/2015 11:42 PM, Kevin Cernekee wrote: > Introduce a new codec driver for the Texas Instruments > TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically > used to take an I2S digital audio input and drive 10-20W into a pair of > speakers. > > Signed-off-by: Kevin Cernekee <cernekee@chromium.org> Looks pretty good. Comments inlune. [...] > -obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o Accidentally removed line > +obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o [...] > +++ b/sound/soc/codecs/tas571x.c > @@ -0,0 +1,456 @@ > +/* > + * TAS571x amplifier audio driver > + * > + * Copyright (C) 2015 Google, Inc. > + * Copyright (c) 2013 Daniel Mack <zonque@gmail.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/clk.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> There is no of specific GPIO code in the driver. [...] > + > +static int tas571x_set_bias_level(struct snd_soc_codec *codec, > + enum snd_soc_bias_level level) > +{ > + struct tas571x_private *priv = snd_soc_codec_get_drvdata(codec); > + int ret, assert_pdn = 0; > + > + if (priv->bias_level == level) > + return 0; The core already takes care that this function is only called if there is a actual change. > + > + switch (level) { > + case SND_SOC_BIAS_PREPARE: > + if (!IS_ERR(priv->mclk)) { > + ret = clk_prepare_enable(priv->mclk); > + if (ret) { > + dev_err(codec->dev, > + "Failed to enable master clock\n"); > + return ret; > + } > + } > + > + ret = tas571x_set_shutdown(priv, false); > + if (ret) > + return ret; > + break; > + case SND_SOC_BIAS_STANDBY: > + ret = tas571x_set_shutdown(priv, true); > + if (ret) > + return ret; > + > + if (!IS_ERR(priv->mclk)) > + clk_disable_unprepare(priv->mclk); > + break; > + case SND_SOC_BIAS_ON: > + break; > + case SND_SOC_BIAS_OFF: > + /* Note that this kills I2C accesses. */ > + assert_pdn = 1; > + break; > + } > + > + if (!IS_ERR(priv->pdn_gpio)) > + gpiod_set_value(priv->pdn_gpio, !assert_pdn); > + > + priv->bias_level = level; This should update codec->dapm.bias_level, otherwise the core will become confused about the actual level. > + return 0; > +} > + [...] > +static const unsigned int tas5711_volume_tlv[] = { > + TLV_DB_RANGE_HEAD(1), > + 0, 0xff, TLV_DB_SCALE_ITEM(-10350, 50, 1), > +}; For TLVs with a single item use DECLARE_TLV_DB_SCALE() > + [...] > +static const unsigned int tas5717_volume_tlv[] = { > + TLV_DB_RANGE_HEAD(1), > + 0x000, 0x1ff, TLV_DB_SCALE_ITEM(-10375, 25, 0), > +}; Same here. [...] > +static int tas571x_i2c_probe(struct i2c_client *client, > + const struct i2c_device_id *id) > +{ > + struct tas571x_private *priv; > + struct device *dev = &client->dev; > + int i; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + i2c_set_clientdata(client, priv); > + > + priv->mclk = devm_clk_get(dev, "mclk"); > + if (PTR_ERR(priv->mclk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; If you want to make the clock optional use if (PTR_ERR(priv->mclk) == -ENOENT) return PTR_ERR(priv->mclk); This makes sure that the case where the clock is specified, but there is a error with the specification (e.g. incorrect DT cells) is handled properly. > + > + for (i = 0; i < TAS571X_NUM_SUPPLIES; i++) > + priv->supplies[i].supply = tas571x_supply_names[i]; > + > + /* > + * This will fall back to the dummy regulator if nothing is specified > + * in DT. > + */ > + if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES, > + priv->supplies)) { Move the function outside the if condition and also pass the error condition to the caller. (And print it) > + dev_err(dev, "Failed to get supplies\n"); > + return -EINVAL; > + } > + if (regulator_bulk_enable(TAS571X_NUM_SUPPLIES, priv->supplies)) { Same here. > + dev_err(dev, "Failed to enable supplies\n"); > + return -EINVAL; > + } > + > + priv->regmap = devm_regmap_init(dev, NULL, client, &tas571x_regmap); > + if (IS_ERR(priv->regmap)) > + return PTR_ERR(priv->regmap); > + > + priv->pdn_gpio = devm_gpiod_get(dev, "pdn"); devm_gpiod_get_optional() ? Using gpiod_get without specifying the direction flags is deprecated. Should be ... = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH); and then drop the gpiod_direction_output(). > + if (!IS_ERR(priv->pdn_gpio)) { > + gpiod_direction_output(priv->pdn_gpio, 1); > + } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT && > + PTR_ERR(priv->pdn_gpio) != -ENOSYS) { > + dev_warn(dev, "error requesting pdn_gpio: %ld\n", > + PTR_ERR(priv->pdn_gpio)); If the GPIO can't be requested and it is not a optional GPIO that should be treated as an error. > + } > + > + priv->reset_gpio = devm_gpiod_get(dev, "reset"); Same as for the pdn_gpio. > + if (!IS_ERR(priv->reset_gpio)) { > + gpiod_direction_output(priv->reset_gpio, 0); > + usleep_range(100, 200); > + gpiod_set_value(priv->reset_gpio, 1); > + usleep_range(12000, 20000); > + } else if (PTR_ERR(priv->reset_gpio) != -ENOENT && > + PTR_ERR(priv->reset_gpio) != -ENOSYS) { > + dev_warn(dev, "error requesting reset_gpio: %ld\n", > + PTR_ERR(priv->reset_gpio)); Same as for the pdn_gpio. > + } > + > + priv->bias_level = SND_SOC_BIAS_STANDBY; > + > + if (regmap_write(priv->regmap, TAS571X_OSC_TRIM_REG, 0)) > + return -EIO; > + > + if (tas571x_set_shutdown(priv, true)) > + return -EIO; > + > + memcpy(&priv->codec_driver, &tas571x_codec, sizeof(priv->codec_driver)); > + priv->dev_id = id->driver_data; > + > + switch (id->driver_data) { > + case TAS571X_ID_5711: > + priv->codec_driver.controls = tas5711_controls; > + priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls); > + break; > + case TAS571X_ID_5717: > + case TAS571X_ID_5719: > + priv->codec_driver.controls = tas5717_controls; > + priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls); > + > + /* > + * The master volume defaults to 0x3ff (mute), but we ignore > + * (zero) the LSB because the hardware step size is 0.125 dB > + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. > + */ > + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) > + return -EIO; > + > + break; > + } Typically when a driver supports multiple chips with different control sets the snd_soc_codec_driver implements a probe callback in which the correct controls are registered. > + > + return snd_soc_register_codec(&client->dev, &priv->codec_driver, > + &tas571x_dai, 1); > +} > + [...] > + > +static const struct of_device_id tas571x_of_match[] = { > + { .compatible = "ti,tas5711", }, > + { .compatible = "ti,tas5717", }, > + { .compatible = "ti,tas5719", }, You should also specify the id data for the of table and get it from the of_data if of_node is non NULL in the probe function. I know that it works without, but that is a bit of a unintentional side-effect and might change in the future. > + { } > +}; > +MODULE_DEVICE_TABLE(of, tas571x_of_match); -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote:
> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply
This is clearly not correct - if there are three separate physical
supplies there should be three separate regulators requested. They may
all resolve to one physical regulator on the board you are working with
but that might not be true on other boards.
On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote: This looks mostly good but several things below, all of which should be straightforward to fix. > +static int tas571x_set_sysclk(struct snd_soc_dai *dai, > + int clk_id, unsigned int freq, int dir) > +{ > + /* > + * TAS5717 datasheet pg 21: "The DAP can autodetect and set the > + * internal clock-control logic to the appropriate settings for all > + * supported clock rates as defined in the clock control register." > + */ > + return 0; > +} Remove empty functions, at best they waste space at worst they break things. > + val += (clamp(params_width(params), 16, 24) >> 2) - 4; Please write this more clearly or comment it (preferably the former), it's hard to tell what it's supposed to do and therefore hard to tell if it's doing it correctly. > +static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown) > +{ > + return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG, > + TAS571X_SYS_CTRL_2_SDN_MASK, > + is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0); > +} > + ret = tas571x_set_shutdown(priv, false); > + if (ret) > + return ret; > + break; > + case SND_SOC_BIAS_STANDBY: > + ret = tas571x_set_shutdown(priv, true); > + if (ret) > + return ret; This looks like it'd be clearer just as direct register updates, I'm not sure a function to set a single bit is addinng much. > + case SND_SOC_BIAS_OFF: > + /* Note that this kills I2C accesses. */ > + assert_pdn = 1; No, the GPIO set associated with it kills I2C access. I'd also expect to see the regmap being marked cache only before we do this and a resync of the register map when we power back up (assuming that is actually a power down). > +static const struct snd_kcontrol_new tas5711_controls[] = { > + SOC_SINGLE_TLV("Master Volume", > + TAS571X_MVOL_REG, > + 0, 0xff, 1, tas5711_volume_tlv), All these controls will be brokenn if the I2C access goes away. > +static const struct snd_soc_codec_driver tas571x_codec = { > + .set_bias_level = tas571x_set_bias_level, > + .suspend_bias_off = true, Why not idle_bias_off? It looks like power up takes no meaningful time. > +static int tas571x_register_size(struct tas571x_private *priv, unsigned int reg) > +{ > + switch (reg) { > + case TAS571X_MVOL_REG: > + case TAS571X_CH1_VOL_REG: > + case TAS571X_CH2_VOL_REG: > + return priv->dev_id == TAS571X_ID_5711 ? 1 : 2; Nest switch statements please, that way things work better if another variant turns up. > + /* > + * This will fall back to the dummy regulator if nothing is specified > + * in DT. > + */ The driver doesn't care, it may not even be on a system using DT. > + if (devm_regulator_bulk_get(dev, TAS571X_NUM_SUPPLIES, > + priv->supplies)) { > + dev_err(dev, "Failed to get supplies\n"); > + return -EINVAL; > + } Don't discard error codes from functions you call, log them and provide them to calllers. The above is broken for probe deferral for example. > + priv->pdn_gpio = devm_gpiod_get(dev, "pdn"); > + if (!IS_ERR(priv->pdn_gpio)) { > + gpiod_direction_output(priv->pdn_gpio, 1); > + } else if (PTR_ERR(priv->pdn_gpio) != -ENOENT && > + PTR_ERR(priv->pdn_gpio) != -ENOSYS) { > + dev_warn(dev, "error requesting pdn_gpio: %ld\n", > + PTR_ERR(priv->pdn_gpio)); > + } This should at least be handling probe deferral, it's not clear why it doesn't just error out in the cases where it gets an error. Similarly for the reset GPIO. > + /* > + * The master volume defaults to 0x3ff (mute), but we ignore > + * (zero) the LSB because the hardware step size is 0.125 dB > + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. > + */ > + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) > + return -EIO; I don't understand this - is the LSB a mute bit or sommething? > +#ifndef _TAS571X_H > +#define _TAS571X_H > + > +#include <sound/pcm.h> Why is this needed in the header?
On Thu, Apr 16, 2015 at 02:57:49PM +0200, Lars-Peter Clausen wrote: > On 04/15/2015 11:42 PM, Kevin Cernekee wrote: > >+ case TAS571X_ID_5711: > >+ priv->codec_driver.controls = tas5711_controls; > >+ priv->codec_driver.num_controls = ARRAY_SIZE(tas5711_controls); > >+ break; > >+ case TAS571X_ID_5717: > >+ case TAS571X_ID_5719: > >+ priv->codec_driver.controls = tas5717_controls; > >+ priv->codec_driver.num_controls = ARRAY_SIZE(tas5717_controls); > Typically when a driver supports multiple chips with different control sets > the snd_soc_codec_driver implements a probe callback in which the correct > controls are registered. I'm fine with doing it with tables (though just having two static CODEC driver structures would be a bit cleaner). The pattern with probe() is usually that there's some base set of controls all the devices have which then gets device specific controls/routes/whatever added to it so you get benefits fromm sharing the table but in this case the table is so tiny anyway that I'm not sure it's worth caring.
On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 15, 2015 at 02:42:20PM -0700, Kevin Cernekee wrote: >> +static int tas571x_set_sysclk(struct snd_soc_dai *dai, >> + int clk_id, unsigned int freq, int dir) >> +{ >> + /* >> + * TAS5717 datasheet pg 21: "The DAP can autodetect and set the >> + * internal clock-control logic to the appropriate settings for all >> + * supported clock rates as defined in the clock control register." >> + */ >> + return 0; >> +} > > Remove empty functions, at best they waste space at worst they break > things. Without the empty function, we run into problems with drivers that abort when they get -ENOTSUPP here: sound/soc/atmel/atmel_wm8904.c: ret = snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set wm8904 SYSCLK\n", __func__); sound/soc/atmel/atmel_wm8904.c- return ret; sound/soc/atmel/atmel_wm8904.c- } Is there a stub version that I can use instead? Nothing jumped out at me when looking at the other codec drivers. >> +static int tas571x_set_shutdown(struct tas571x_private *priv, bool is_shutdown) >> +{ >> + return regmap_update_bits(priv->regmap, TAS571X_SYS_CTRL_2_REG, >> + TAS571X_SYS_CTRL_2_SDN_MASK, >> + is_shutdown ? TAS571X_SYS_CTRL_2_SDN_MASK : 0); >> +} > >> + ret = tas571x_set_shutdown(priv, false); >> + if (ret) >> + return ret; >> + break; >> + case SND_SOC_BIAS_STANDBY: >> + ret = tas571x_set_shutdown(priv, true); >> + if (ret) >> + return ret; > > This looks like it'd be clearer just as direct register updates, I'm not > sure a function to set a single bit is addinng much. It might be useful if another tas571x variant put the bit somewhere else, but that hasn't happened yet so I can nuke the helper function for now. >> + case SND_SOC_BIAS_OFF: >> + /* Note that this kills I2C accesses. */ >> + assert_pdn = 1; > > No, the GPIO set associated with it kills I2C access. I'd also expect > to see the regmap being marked cache only before we do this and a resync > of the register map when we power back up (assuming that is actually a > power down). Hmm, not sure if this actually resets the registers back to power-on defaults, but I'll check. >> + /* >> + * The master volume defaults to 0x3ff (mute), but we ignore >> + * (zero) the LSB because the hardware step size is 0.125 dB >> + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. >> + */ >> + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) >> + return -EIO; > > I don't understand this - is the LSB a mute bit or sommething? The 10-bit master volume field on 5717/5719 works like: 0x3ff: MUTE (power-on default) 0x3fe: -103.750 dB 0x3fd: -103.625 dB [lots more options, in 0.125 dB increments] 0x001: 23.875 dB 0x000: 24.000 dB Since we only have a resolution of 0.01 dB, the driver forces the LSB to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled through the dedicated per-channel soft mute register bits instead of the 0x3ff volume setting. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote: > On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown <broonie@kernel.org> wrote: > >> +static int tas571x_set_sysclk(struct snd_soc_dai *dai, > >> + int clk_id, unsigned int freq, int dir) > > Remove empty functions, at best they waste space at worst they break > > things. > Without the empty function, we run into problems with drivers that > abort when they get -ENOTSUPP here: > sound/soc/atmel/atmel_wm8904.c: ret = > snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, > sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); > sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { > sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set > wm8904 SYSCLK\n", __func__); > sound/soc/atmel/atmel_wm8904.c- return ret; > sound/soc/atmel/atmel_wm8904.c- } Someone trying to use the atmel_wm8904 driver with something other than a wm8904 shouldn't really be expecting a good experince... > Is there a stub version that I can use instead? Nothing jumped out at > me when looking at the other codec drivers. No, such a stub would make no sense - why would we put a stub in all the drivers rather than just making the core do the right thing? > >> + /* > >> + * The master volume defaults to 0x3ff (mute), but we ignore > >> + * (zero) the LSB because the hardware step size is 0.125 dB > >> + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. > >> + */ > >> + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) > >> + return -EIO; > > I don't understand this - is the LSB a mute bit or sommething? > The 10-bit master volume field on 5717/5719 works like: > 0x3ff: MUTE (power-on default) > 0x3fe: -103.750 dB > 0x3fd: -103.625 dB > [lots more options, in 0.125 dB increments] > 0x001: 23.875 dB > 0x000: 24.000 dB > Since we only have a resolution of 0.01 dB, the driver forces the LSB > to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled > through the dedicated per-channel soft mute register bits instead of > the 0x3ff volume setting. It's not entirely clear to me why we need to reset the bit, or why if we're just trying to update that one bit we write the entire register value rather than use _update_bits(). If the goal is just to change that one bit then _update_bits() would be a lot clearer.
On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown <broonie@kernel.org> wrote: > On Sat, Apr 18, 2015 at 09:16:36AM -0700, Kevin Cernekee wrote: >> On Sat, Apr 18, 2015 at 4:36 AM, Mark Brown <broonie@kernel.org> wrote: > >> >> +static int tas571x_set_sysclk(struct snd_soc_dai *dai, >> >> + int clk_id, unsigned int freq, int dir) > >> > Remove empty functions, at best they waste space at worst they break >> > things. > >> Without the empty function, we run into problems with drivers that >> abort when they get -ENOTSUPP here: > >> sound/soc/atmel/atmel_wm8904.c: ret = >> snd_soc_dai_set_sysclk(codec_dai, WM8904_CLK_FLL, >> sound/soc/atmel/atmel_wm8904.c- 0, SND_SOC_CLOCK_IN); >> sound/soc/atmel/atmel_wm8904.c- if (ret < 0) { >> sound/soc/atmel/atmel_wm8904.c- pr_err("%s -failed to set >> wm8904 SYSCLK\n", __func__); >> sound/soc/atmel/atmel_wm8904.c- return ret; >> sound/soc/atmel/atmel_wm8904.c- } > > Someone trying to use the atmel_wm8904 driver with something other than > a wm8904 shouldn't really be expecting a good experince... The same check shows up in numerous other drivers, including the one for the audio controller on my board. >> Is there a stub version that I can use instead? Nothing jumped out at >> me when looking at the other codec drivers. > > No, such a stub would make no sense - why would we put a stub in all the > drivers rather than just making the core do the right thing? AFAICT, implementing the set_sysclk callback is mandatory, even if it is a no-op on the codec side. If I delete the stub function, audio playback fails. >> >> + /* >> >> + * The master volume defaults to 0x3ff (mute), but we ignore >> >> + * (zero) the LSB because the hardware step size is 0.125 dB >> >> + * and TLV_DB_SCALE_ITEM has a resolution of 0.01 dB. >> >> + */ >> >> + if (regmap_write(priv->regmap, TAS571X_MVOL_REG, 0x3fe)) >> >> + return -EIO; > >> > I don't understand this - is the LSB a mute bit or sommething? > >> The 10-bit master volume field on 5717/5719 works like: > >> 0x3ff: MUTE (power-on default) >> 0x3fe: -103.750 dB >> 0x3fd: -103.625 dB >> [lots more options, in 0.125 dB increments] >> 0x001: 23.875 dB >> 0x000: 24.000 dB > >> Since we only have a resolution of 0.01 dB, the driver forces the LSB >> to 0 and uses 0.25 dB increments instead of 0.125 dB. Mute is handled >> through the dedicated per-channel soft mute register bits instead of >> the 0x3ff volume setting. > > It's not entirely clear to me why we need to reset the bit, or why if > we're just trying to update that one bit we write the entire register > value rather than use _update_bits(). If the goal is just to change > that one bit then _update_bits() would be a lot clearer. The default master volume setting on the TAS5717/5719 is 0x3ff (bits 9:0). We only ever update bits 9:1 when the volume changes, because the driver is only able to work with 0.25 dB resolution rather than the hardware's native 0.125 dB resolution. So this register write sets the master volume to an even number, which we know we can handle. Clearing just the LSB would accomplish the same thing, but would be less obvious IMO. It would also require an extra read, and the code is less concise. Is there a better way to handle this situation? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: > On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown <broonie@kernel.org> wrote: > > Someone trying to use the atmel_wm8904 driver with something other than > > a wm8904 shouldn't really be expecting a good experince... > The same check shows up in numerous other drivers, including the one > for the audio controller on my board. Sounds like either that (undisclosed) driver has a problem or you're using it inappropriately. > >> Is there a stub version that I can use instead? Nothing jumped out at > >> me when looking at the other codec drivers. > > No, such a stub would make no sense - why would we put a stub in all the > > drivers rather than just making the core do the right thing? > AFAICT, implementing the set_sysclk callback is mandatory, even if it > is a no-op on the codec side. If I delete the stub function, audio > playback fails. For the reasons I mentioned above having a set_sysclk() function is not mandatory and your driver will not be merged with a stub such as is currently present. As far as I can tell you are trying to bodge around some problem elsewhere in either the code or your usage of it. > Clearing just the LSB would accomplish the same thing, but would be > less obvious IMO. It would also require an extra read, and the code > is less concise. I don't think anyone is going to care about an extra read on system init, and in any case if the driver followed best practice and provided register defaults that read would be satisfied from cache.
On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown <broonie@kernel.org> wrote: > On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: >> On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown <broonie@kernel.org> wrote: > >> > Someone trying to use the atmel_wm8904 driver with something other than >> > a wm8904 shouldn't really be expecting a good experince... > >> The same check shows up in numerous other drivers, including the one >> for the audio controller on my board. > > Sounds like either that (undisclosed) driver has a problem or you're > using it inappropriately. I don't think this driver has been posted on the list yet, but the checks show up in these two functions: https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 Any suggestions on what to change? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kevin, On Mon, Apr 20, 2015 at 8:12 AM, Kevin Cernekee <cernekee@chromium.org> wrote: > On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown <broonie@kernel.org> wrote: >> On Sat, Apr 18, 2015 at 01:07:07PM -0700, Kevin Cernekee wrote: >>> On Sat, Apr 18, 2015 at 10:11 AM, Mark Brown <broonie@kernel.org> wrote: >> >>> > Someone trying to use the atmel_wm8904 driver with something other than >>> > a wm8904 shouldn't really be expecting a good experince... >> >>> The same check shows up in numerous other drivers, including the one >>> for the audio controller on my board. >> >> Sounds like either that (undisclosed) driver has a problem or you're >> using it inappropriately. > > I don't think this driver has been posted on the list yet, but the > checks show up in these two functions: > > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 > > Any suggestions on what to change? I think the card driver should be ignoring a return value of -ENOTSUPP from snd_soc_dai_set_sysclk() in that case. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 08:12:36AM -0700, Kevin Cernekee wrote: > On Mon, Apr 20, 2015 at 5:21 AM, Mark Brown <broonie@kernel.org> wrote: > >> The same check shows up in numerous other drivers, including the one > >> for the audio controller on my board. > > Sounds like either that (undisclosed) driver has a problem or you're > > using it inappropriately. > I don't think this driver has been posted on the list yet, but the > checks show up in these two functions: > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#108 > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.14/sound/soc/img/concerto-audio.c#147 > Any suggestions on what to change? That driver has not been posted previously (and looks like it could use some rework for mainline). If it doesn't care if it set the clock and is just trying for information it should be handling -ENOTSUPP, that's why the core returns that value.
On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > On 04/15/2015 11:42 PM, Kevin Cernekee wrote: >> >> Introduce a new codec driver for the Texas Instruments >> TAS5711/TAS5717/TAS5719 power amplifier chips. These chips are typically >> used to take an I2S digital audio input and drive 10-20W into a pair of >> speakers. >> >> Signed-off-by: Kevin Cernekee <cernekee@chromium.org> > > > Looks pretty good. Comments inlune. Thanks for the review. I'm working on a V2 incorporating the feedback from you and Mark. >> +static int tas571x_i2c_probe(struct i2c_client *client, >> + const struct i2c_device_id *id) >> +{ >> + struct tas571x_private *priv; >> + struct device *dev = &client->dev; >> + int i; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + i2c_set_clientdata(client, priv); >> + >> + priv->mclk = devm_clk_get(dev, "mclk"); >> + if (PTR_ERR(priv->mclk) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; > > > If you want to make the clock optional use > > if (PTR_ERR(priv->mclk) == -ENOENT) > return PTR_ERR(priv->mclk); > > This makes sure that the case where the clock is specified, but there is a > error with the specification (e.g. incorrect DT cells) is handled properly. Did you mean: > if (PTR_ERR(priv->mclk) != -ENOENT) > return PTR_ERR(priv->mclk); I don't see this in other codec drivers, but I do see the explicit EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 01:56:46PM -0700, Kevin Cernekee wrote: > On Thu, Apr 16, 2015 at 5:57 AM, Lars-Peter Clausen <lars@metafoo.de> wrote: > > On 04/15/2015 11:42 PM, Kevin Cernekee wrote: > > If you want to make the clock optional use > > if (PTR_ERR(priv->mclk) == -ENOENT) > > return PTR_ERR(priv->mclk); > > This makes sure that the case where the clock is specified, but there is a > > error with the specification (e.g. incorrect DT cells) is handled properly. > Did you mean: > > if (PTR_ERR(priv->mclk) != -ENOENT) > > return PTR_ERR(priv->mclk); > I don't see this in other codec drivers, but I do see the explicit > EPROBE_DEFER check in max98090, max98095, pcm512x, and wm8960. That's the most correct way of writing a check for an optional clock, best practice on this stuff is evolving over time (as is standardization in the clock API).
On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > > This is clearly not correct - if there are three separate physical > supplies there should be three separate regulators requested. They may > all resolve to one physical regulator on the board you are working with > but that might not be true on other boards. In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. But this combines a number of separate pins. On 5711 we have dedicated pins for: PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD On 5717 we have dedicated pins for: PVDD_AB PVDD_CD AVDD DVDD HPVDD I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD). I can add as many regulator entries as appropriate. What do you recommend? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Resending from the correct account. On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > > This is clearly not correct - if there are three separate physical > supplies there should be three separate regulators requested. They may > all resolve to one physical regulator on the board you are working with > but that might not be true on other boards. In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. But this combines a number of separate pins. On 5711 we have dedicated pins for: PVDD_A PVDD_B PVDD_C PVDD_D AVDD DVDD On 5717 we have dedicated pins for: PVDD_AB PVDD_CD AVDD DVDD HPVDD I didn't see anything in the datasheet suggesting it is OK to have different voltages or power states on the various supply pins (other than the special voltage on PVDD). I can add as many regulator entries as appropriate. What do you recommend? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote: > On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: > > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > > This is clearly not correct - if there are three separate physical > > supplies there should be three separate regulators requested. They may > > all resolve to one physical regulator on the board you are working with > > but that might not be true on other boards. > In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: > http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 > Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. > But this combines a number of separate pins. On 5711 we have > dedicated pins for: > PVDD_A > PVDD_B > PVDD_C > PVDD_D > AVDD > DVDD Yes, those are three separate supplies that are typically tied together - it looks like PVDD has high current draw so is tied through multiple pins. I'd not be surprised to see systems with AVDD tied to a separate pin, analogue supplies often benefit from low noise supplies separate to digital ones. Indeed if you look at the pin descriptions the analogue and digital supplies even have separate grounds. > I didn't see anything in the datasheet suggesting it is OK to have > different voltages or power states on the various supply pins (other > than the special voltage on PVDD). That's more likely to go wrong if they're not controlled separately than if they are since we will only be able to turn a single supply on or off.. > I can add as many regulator entries as appropriate. What do you recommend? Have the driver accurately reflect the hardware, request one regulator per supply.
On Mon, Apr 20, 2015 at 3:03 PM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Apr 20, 2015 at 02:18:56PM -0700, Kevin Cernekee wrote: >> On Sat, Apr 18, 2015 at 4:16 AM, Mark Brown <broonie@kernel.org> wrote: >> > On Wed, Apr 15, 2015 at 02:42:19PM -0700, Kevin Cernekee wrote: > >> >> +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply > >> > This is clearly not correct - if there are three separate physical >> > supplies there should be three separate regulators requested. They may >> > all resolve to one physical regulator on the board you are working with >> > but that might not be true on other boards. > >> In the "simplified diagram," TI shows a single AVDD/DVDD/HP_VDD supply: > >> http://www.ti.com/lit/ds/symlink/tas5717.pdf#2 > >> Page 20 also suggests the use of a single 3.3V supply for AVDD/DVDD/HP_VDD. > >> But this combines a number of separate pins. On 5711 we have >> dedicated pins for: > >> PVDD_A >> PVDD_B >> PVDD_C >> PVDD_D >> AVDD >> DVDD > > Yes, those are three separate supplies that are typically tied together > - it looks like PVDD has high current draw so is tied through multiple > pins. Each half-bridge output OUT_{A,B,C,D} has its own dedicated PVDD_{A,B,C,D} supply on 5711: http://www.ti.com/lit/ds/symlink/tas5711.pdf#7 I don't understand the statement: "those are three separate supplies that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can never be driven by the same regulator output as PVDD. Perhaps you were thinking of HPVDD/AVDD/DVDD on 5717/5719? The simplified application diagram on page 2 does show that AVDD/DVDD are typically tied together, and that PVDD_* are also typically tied together. But as you pointed out, there may be situations where that isn't the case, so that leaves us with up to 6 separate supplies on 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or 3, though. > I'd not be surprised to see systems with AVDD tied to a separate > pin, analogue supplies often benefit from low noise supplies separate to > digital ones. Indeed if you look at the pin descriptions the analogue > and digital supplies even have separate grounds. > >> I didn't see anything in the datasheet suggesting it is OK to have >> different voltages or power states on the various supply pins (other >> than the special voltage on PVDD). > > That's more likely to go wrong if they're not controlled separately than > if they are since we will only be able to turn a single supply on or > off.. > >> I can add as many regulator entries as appropriate. What do you recommend? > > Have the driver accurately reflect the hardware, request one regulator > per supply. So, request one regulator per VDD pin? Or group all of PVDD_* into a single regulator? -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 03:48:19PM -0700, Kevin Cernekee wrote: > I don't understand the statement: "those are three separate supplies > that are typically tied together." AVDD/DVDD are 3.3v fixed but PVDD > is 8V minimum (5711) or 4.5V minimum (5717/5719). So AVDD/DVDD can > never be driven by the same regulator output as PVDD. Perhaps you > were thinking of HPVDD/AVDD/DVDD on 5717/5719? Yes, all the supplies. I'd misremembered which ones you were saying were tied together on your board. > The simplified application diagram on page 2 does show that AVDD/DVDD > are typically tied together, and that PVDD_* are also typically tied > together. But as you pointed out, there may be situations where that > isn't the case, so that leaves us with up to 6 separate supplies on > 5711, or 5 on 5717/5719. The typical case is probably closer to 2 or > 3, though. To repeat, the typical case is *not* *relevant*. All that matters is the set of supplies the device has. > > Have the driver accurately reflect the hardware, request one regulator > > per supply. > So, request one regulator per VDD pin? Or group all of PVDD_* into a > single regulator? Please group them into supplies as documented in the datasheet. If they are documented as distinct things they should be distinct things, if they are just the same supply coming in through multiple pins for power reasons they should be one supply.
On Sat, Apr 18, 2015 at 9:16 AM, Kevin Cernekee <cernekee@chromium.org> wrote: >>> + case SND_SOC_BIAS_OFF: >>> + /* Note that this kills I2C accesses. */ >>> + assert_pdn = 1; >> >> No, the GPIO set associated with it kills I2C access. I'd also expect >> to see the regmap being marked cache only before we do this and a resync >> of the register map when we power back up (assuming that is actually a >> power down). > > Hmm, not sure if this actually resets the registers back to power-on > defaults, but I'll check. Hi Mark, I have reworked the driver to do the following: - set appropriate regmap default values on probe - enable idle_bias_off - use regcache_cache_only() to prevent accesses to I2C when in SND_SOC_BIAS_OFF state (pdn asserted) - use regcache_sync() when transitioning from SND_SOC_BIAS_OFF -> SND_SOC_BIAS_STANDBY This is mostly working OK, but regcache_sync() assumes that the hardware registers have been reset back to the default values. The "pdn" GPIO doesn't actually reset the state of the tas571x; it just makes I2C inaccessible and inhibits audio output. So if the factory default for mute is 0, corner cases like this fail: - enter SND_SOC_BIAS_ON (e.g. play a wav file) - set mute to 1 - enter SND_SOC_BIAS_OFF (e.g. playback ends) - set mute to 0 - re-enter SND_SOC_BIAS_ON - regcache_sync() incorrectly assumes that the hardware register is already 0, but in fact it needs to be refreshed from the cache Aside from unnecessarily pulsing the reset GPIO when transitioning back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can you think of a way to work around this? Thanks. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote: > This is mostly working OK, but regcache_sync() assumes that the > hardware registers have been reset back to the default values. The > "pdn" GPIO doesn't actually reset the state of the tas571x; it just > makes I2C inaccessible and inhibits audio output. So if the factory > default for mute is 0, corner cases like this fail: ... > Aside from unnecessarily pulsing the reset GPIO when transitioning > back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can > you think of a way to work around this? Do you need to work around it? If the register map is being perserved you don't need to sync so just don't do it - it's just that the normal expectation would be that power down would cause the register map to be reset.
On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown <broonie@kernel.org> wrote: > On Thu, Apr 23, 2015 at 05:47:49PM -0700, Kevin Cernekee wrote: > >> This is mostly working OK, but regcache_sync() assumes that the >> hardware registers have been reset back to the default values. The >> "pdn" GPIO doesn't actually reset the state of the tas571x; it just >> makes I2C inaccessible and inhibits audio output. So if the factory >> default for mute is 0, corner cases like this fail: > > ... > >> Aside from unnecessarily pulsing the reset GPIO when transitioning >> back from SND_SOC_BIAS_OFF or overriding regcache_default_sync(), can >> you think of a way to work around this? > > Do you need to work around it? If the register map is being perserved > you don't need to sync so just don't do it - it's just that the normal > expectation would be that power down would cause the register map to be > reset. How do I tell regcache to write out any updates that happened while the hardware was inaccessible? I see that regmap->cache_dirty is 1, but nothing flushes it automatically when exiting cache_only mode. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 24, 2015 at 06:52:01AM -0700, Kevin Cernekee wrote: > On Fri, Apr 24, 2015 at 2:28 AM, Mark Brown <broonie@kernel.org> wrote: > > Do you need to work around it? If the register map is being perserved > > you don't need to sync so just don't do it - it's just that the normal > > expectation would be that power down would cause the register map to be > > reset. > How do I tell regcache to write out any updates that happened while > the hardware was inaccessible? I see that regmap->cache_dirty is 1, > but nothing flushes it automatically when exiting cache_only mode. Oh, I see. A sync will be required, yes. Probably resetting the device is easiest.
diff --git a/Documentation/devicetree/bindings/sound/tas571x.txt b/Documentation/devicetree/bindings/sound/tas571x.txt new file mode 100644 index 000000000000..ac704c1f143d --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tas571x.txt @@ -0,0 +1,34 @@ +Texas Instruments TAS5711/TAS5717/TAS5719 stereo power amplifiers + +The codec is controlled through an I2C interface. It also has two other +signals that can be wired up to GPIOs: reset (strongly recommended), and +powerdown (optional). + +Required properties: + +- compatible: "ti,tas5711", "ti,tas5717", or "ti,tas5719" +- reg: The I2C address of the device +- #sound-dai-cells: must be equal to 0 + +Optional properties: + +- reset-gpios: GPIO specifier for the TAS571x's active low reset line +- pdn-gpios: GPIO specifier for the TAS571x's active low powerdown line +- clocks: clock phandle for the MCLK input +- clock-names: should be "mclk" +- VDD-supply: regulator phandle for the AVDD/DVDD/HP_VDD supply +- PVDD-supply: regulator phandle for the PVDD supply + +Example: + + tas5717: audio-codec@2a { + compatible = "ti,tas5717"; + reg = <0x2a>; + #sound-dai-cells = <0>; + + reset-gpios = <&gpio5 1 GPIO_ACTIVE_HIGH>; + pdn-gpios = <&gpio5 2 GPIO_ACTIVE_HIGH>; + + clocks = <&clk_core CLK_I2S>; + clock-names = "mclk"; + };
Document the bindings for the soon-to-be-added tas571x driver. Signed-off-by: Kevin Cernekee <cernekee@chromium.org> --- .../devicetree/bindings/sound/tas571x.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tas571x.txt