| Message ID | 20171211190157.12371-3-afd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote: > The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB > steps on the TAS5720). Introduce a custom mixer control that allows taking > advantage of this finer output volume granularity. Don't do this, it's just making things more complicated. Instead do what other drivers do and register different sets of controls depending on which part you're working with. The normal thing is to have a big table for all the shared controls that are the same on all variants then register additional tables during probe with those that vary for the individul devices. > static const struct snd_kcontrol_new tas5720_snd_controls[] = { > SOC_SINGLE_TLV("Speaker Driver Playback Volume", > - TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv), > + TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv), > + SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, > + TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), As ever all volume controls should end in Volume (like the immediately adjacent control does).
On 12/12/2017 06:01 AM, Mark Brown wrote: > On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote: > >> The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB >> steps on the TAS5720). Introduce a custom mixer control that allows taking >> advantage of this finer output volume granularity. > > Don't do this, it's just making things more complicated. Instead do > what other drivers do and register different sets of controls depending > on which part you're working with. The normal thing is to have a big > table for all the shared controls that are the same on all variants then > register additional tables during probe with those that vary for the > individul devices. > That is what we are doing here, the reason for the custom mixer control is that the controlled bits span two registers in a odd way that is not supported by the standard handlers. >> static const struct snd_kcontrol_new tas5720_snd_controls[] = { >> SOC_SINGLE_TLV("Speaker Driver Playback Volume", >> - TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv), >> + TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv), >> + SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, >> + TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), > > As ever all volume controls should end in Volume (like the immediately > adjacent control does). > This was done so this table exactly matches the existing table. If you would like me to change this then I can, and can do it for the other table as well, up to you.
On Mon, Jan 15, 2018 at 08:50:09AM -0600, Andrew F. Davis wrote: > On 12/12/2017 06:01 AM, Mark Brown wrote: > > On Mon, Dec 11, 2017 at 01:01:56PM -0600, Andrew F. Davis wrote: > >> The TAS5722 supports modifying volume in 0.25dB steps (as opposed to 0.5dB > >> steps on the TAS5720). Introduce a custom mixer control that allows taking > >> advantage of this finer output volume granularity. > > Don't do this, it's just making things more complicated. Instead do > > what other drivers do and register different sets of controls depending > > on which part you're working with. The normal thing is to have a big > > table for all the shared controls that are the same on all variants then > > register additional tables during probe with those that vary for the > > individul devices. > That is what we are doing here, the reason for the custom mixer control > is that the controlled bits span two registers in a odd way that is not > supported by the standard handlers. That's not clear from the commit message, it sounds like you're introducing an an extra control rather than replacing the one that's currently there. > > As ever all volume controls should end in Volume (like the immediately > > adjacent control does). > This was done so this table exactly matches the existing table. If you > would like me to change this then I can, and can do it for the other > table as well, up to you. Of course fixes for bugs in existing code are welcome.
diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index f3006f301fe8..042068964afb 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -491,11 +491,59 @@ static const DECLARE_TLV_DB_RANGE(dac_analog_tlv, * setting the gain below -100 dB (register value <0x7) is effectively a MUTE * as per device datasheet. */ -static DECLARE_TLV_DB_SCALE(dac_tlv, -10350, 50, 0); +static DECLARE_TLV_DB_SCALE(tas5720_dac_tlv, -10350, 50, 0); + static const struct snd_kcontrol_new tas5720_snd_controls[] = { SOC_SINGLE_TLV("Speaker Driver Playback Volume", - TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, dac_tlv), + TAS5720_VOLUME_CTRL_REG, 0, 0xff, 0, tas5720_dac_tlv), + SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, + TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), +}; + +/* + * DAC digital volumes. From -103.5 to 24 dB in 0.25 dB steps. Note that + * setting the gain below -100 dB (register value <0x8) is effectively a MUTE + * as per device datasheet. + * + * Note that for the TAS5722 the digital volume controls are actually split + * over two registers, so we need custom getters/setters for access. + */ +static DECLARE_TLV_DB_SCALE(tas5722_dac_tlv, -10350, 25, 0); + +static int tas5722_volume_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + unsigned int val; + + val = snd_soc_read(codec, TAS5720_VOLUME_CTRL_REG); + ucontrol->value.integer.value[0] = val << 1; + + val = snd_soc_read(codec, TAS5722_DIGITAL_CTRL2_REG); + ucontrol->value.integer.value[0] |= val & TAS5722_VOL_CONTROL_LSB; + + return 0; +} + +static int tas5722_volume_set(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + unsigned int sel = ucontrol->value.integer.value[0]; + + snd_soc_write(codec, TAS5720_VOLUME_CTRL_REG, sel >> 1); + snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG, + TAS5722_VOL_CONTROL_LSB, sel); + + return 0; +} + +static const struct snd_kcontrol_new tas5722_snd_controls[] = { + SOC_SINGLE_EXT_TLV("Speaker Driver Playback Volume", + 0, 0, 511, 0, + tas5722_volume_get, tas5722_volume_set, + tas5722_dac_tlv), SOC_SINGLE_TLV("Speaker Driver Analog Gain", TAS5720_ANALOG_CTRL_REG, TAS5720_ANALOG_GAIN_SHIFT, 3, 0, dac_analog_tlv), }; @@ -528,6 +576,22 @@ static const struct snd_soc_codec_driver soc_codec_dev_tas5720 = { }, }; +static struct snd_soc_codec_driver soc_codec_dev_tas5722 = { + .probe = tas5720_codec_probe, + .remove = tas5720_codec_remove, + .suspend = tas5720_suspend, + .resume = tas5720_resume, + + .component_driver = { + .controls = tas5722_snd_controls, + .num_controls = ARRAY_SIZE(tas5722_snd_controls), + .dapm_widgets = tas5720_dapm_widgets, + .num_dapm_widgets = ARRAY_SIZE(tas5720_dapm_widgets), + .dapm_routes = tas5720_audio_map, + .num_dapm_routes = ARRAY_SIZE(tas5720_audio_map), + } +}; + /* PCM rates supported by the TAS5720 driver */ #define TAS5720_RATES (SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |\ SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000) @@ -614,9 +678,23 @@ static int tas5720_probe(struct i2c_client *client, dev_set_drvdata(dev, data); - ret = snd_soc_register_codec(&client->dev, - &soc_codec_dev_tas5720, - tas5720_dai, ARRAY_SIZE(tas5720_dai)); + switch (id->driver_data) { + case TAS5720: + ret = snd_soc_register_codec(&client->dev, + &soc_codec_dev_tas5720, + tas5720_dai, + ARRAY_SIZE(tas5720_dai)); + break; + case TAS5722: + ret = snd_soc_register_codec(&client->dev, + &soc_codec_dev_tas5722, + tas5720_dai, + ARRAY_SIZE(tas5720_dai)); + break; + default: + dev_err(dev, "unexpected private driver data\n"); + return -EINVAL; + } if (ret < 0) { dev_err(dev, "failed to register codec: %d\n", ret); return ret;