| Message ID | 20171211190157.12371-1-afd@ti.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [1/4] ASoC: codecs: tas5720: add basic support for TAS5722 devices | expand |
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 Mon, Dec 11, 2017 at 01:01:57PM -0600, Andrew F. Davis wrote: > + /* Configure TDM slot width. This is only applicable to TAS5722. */ > + if (tas5720->devtype == TAS5722) { > + ret = snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG, > + TAS5722_TDM_SLOT_16B, > + slot_width == 16 ? > + TAS5722_TDM_SLOT_16B : 0); > + if (ret < 0) > + goto error_snd_soc_update_bits; > + } Use a switch statement, that way additional variants can be handled more sensibly.
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. -- 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 12/12/2017 06:02 AM, Mark Brown wrote: > On Mon, Dec 11, 2017 at 01:01:57PM -0600, Andrew F. Davis wrote: > >> + /* Configure TDM slot width. This is only applicable to TAS5722. */ >> + if (tas5720->devtype == TAS5722) { >> + ret = snd_soc_update_bits(codec, TAS5722_DIGITAL_CTRL2_REG, >> + TAS5722_TDM_SLOT_16B, >> + slot_width == 16 ? >> + TAS5722_TDM_SLOT_16B : 0); >> + if (ret < 0) >> + goto error_snd_soc_update_bits; >> + } > > Use a switch statement, that way additional variants can be handled more > sensibly. > Will fix. -- 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, 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/Documentation/devicetree/bindings/sound/tas5720.txt b/Documentation/devicetree/bindings/sound/tas5720.txt index 40d94f82beb3..7481653fe8e3 100644 --- a/Documentation/devicetree/bindings/sound/tas5720.txt +++ b/Documentation/devicetree/bindings/sound/tas5720.txt @@ -6,10 +6,12 @@ audio playback. For more product information please see the links below: http://www.ti.com/product/TAS5720L http://www.ti.com/product/TAS5720M +http://www.ti.com/product/TAS5722L Required properties: -- compatible : "ti,tas5720" +- compatible : "ti,tas5720", + "ti,tas5722" - reg : I2C slave address - dvdd-supply : phandle to a 3.3-V supply for the digital circuitry - pvdd-supply : phandle to a supply used for the Class-D amp and the analog diff --git a/sound/soc/codecs/tas5720.c b/sound/soc/codecs/tas5720.c index a736a2a6976c..5def54d1336d 100644 --- a/sound/soc/codecs/tas5720.c +++ b/sound/soc/codecs/tas5720.c @@ -36,6 +36,11 @@ /* Define how often to check (and clear) the fault status register (in ms) */ #define TAS5720_FAULT_CHECK_INTERVAL 200 +enum tas572x_type { + TAS5720, + TAS5722, +}; + static const char * const tas5720_supply_names[] = { "dvdd", /* Digital power supply. Connect to 3.3-V supply. */ "pvdd", /* Class-D amp and analog power supply (connected). */ @@ -47,6 +52,7 @@ struct tas5720_data { struct snd_soc_codec *codec; struct regmap *regmap; struct i2c_client *tas5720_client; + enum tas572x_type devtype; struct regulator_bulk_data supplies[TAS5720_NUM_SUPPLIES]; struct delayed_work fault_check_work; unsigned int last_fault; @@ -264,7 +270,7 @@ static void tas5720_fault_check_work(struct work_struct *work) static int tas5720_codec_probe(struct snd_soc_codec *codec) { struct tas5720_data *tas5720 = snd_soc_codec_get_drvdata(codec); - unsigned int device_id; + unsigned int device_id, expected_device_id; int ret; tas5720->codec = codec; @@ -276,6 +282,11 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) return ret; } + /* + * Take a liberal approach to checking the device ID to allow the + * driver to be used even if the device ID does not match, however + * issue a warning if there is a mismatch. + */ ret = regmap_read(tas5720->regmap, TAS5720_DEVICE_ID_REG, &device_id); if (ret < 0) { dev_err(codec->dev, "failed to read device ID register: %d\n", @@ -283,13 +294,22 @@ static int tas5720_codec_probe(struct snd_soc_codec *codec) goto probe_fail; } - if (device_id != TAS5720_DEVICE_ID) { - dev_err(codec->dev, "wrong device ID. expected: %u read: %u\n", - TAS5720_DEVICE_ID, device_id); - ret = -ENODEV; - goto probe_fail; + switch (tas5720->devtype) { + case TAS5720: + expected_device_id = TAS5720_DEVICE_ID; + break; + case TAS5722: + expected_device_id = TAS5722_DEVICE_ID; + break; + default: + dev_err(codec->dev, "unexpected private driver data\n"); + return -EINVAL; } + if (device_id != expected_device_id) + dev_warn(codec->dev, "wrong device ID. expected: %u read: %u\n", + expected_device_id, device_id); + /* Set device to mute */ ret = snd_soc_update_bits(codec, TAS5720_DIGITAL_CTRL2_REG, TAS5720_MUTE, TAS5720_MUTE); @@ -552,6 +572,8 @@ static int tas5720_probe(struct i2c_client *client, return -ENOMEM; data->tas5720_client = client; + data->devtype = id->driver_data; + data->regmap = devm_regmap_init_i2c(client, &tas5720_regmap_config); if (IS_ERR(data->regmap)) { ret = PTR_ERR(data->regmap); @@ -592,7 +614,8 @@ static int tas5720_remove(struct i2c_client *client) } static const struct i2c_device_id tas5720_id[] = { - { "tas5720", 0 }, + { "tas5720", TAS5720 }, + { "tas5722", TAS5722 }, { } }; MODULE_DEVICE_TABLE(i2c, tas5720_id); @@ -600,6 +623,7 @@ MODULE_DEVICE_TABLE(i2c, tas5720_id); #if IS_ENABLED(CONFIG_OF) static const struct of_device_id tas5720_of_match[] = { { .compatible = "ti,tas5720", }, + { .compatible = "ti,tas5722", }, { }, }; MODULE_DEVICE_TABLE(of, tas5720_of_match); diff --git a/sound/soc/codecs/tas5720.h b/sound/soc/codecs/tas5720.h index 3d077c779b12..bef802afcc69 100644 --- a/sound/soc/codecs/tas5720.h +++ b/sound/soc/codecs/tas5720.h @@ -32,6 +32,7 @@ /* TAS5720_DEVICE_ID_REG */ #define TAS5720_DEVICE_ID 0x01 +#define TAS5722_DEVICE_ID 0x12 /* TAS5720_POWER_CTRL_REG */ #define TAS5720_DIG_CLIP_MASK GENMASK(7, 2)