* [PATCH 0/2] TDA7419 audio processor driver @ 2018-02-27 22:51 Matt Porter 2018-02-27 22:51 ` [PATCH 1/2] ASoC: add tda7419 audio processor binding Matt Porter 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter 0 siblings, 2 replies; 14+ messages in thread From: Matt Porter @ 2018-02-27 22:51 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland Cc: alsa-devel, devicetree, linux-kernel This series adds an ASoC component driver for the ST TDA7419 audio processor which is commonly used in automotive audio applications. The datasheet can be found at http://www.st.com/resource/en/datasheet/tda7419.pdf Matt Porter (2): ASoC: add tda7419 audio processor binding ASoC: add tda7419 audio processor driver .../devicetree/bindings/sound/tda7419.txt | 15 + sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda7419.c | 571 +++++++++++++++++++++ 4 files changed, 594 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt create mode 100644 sound/soc/codecs/tda7419.c -- 2.11.0 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] ASoC: add tda7419 audio processor binding 2018-02-27 22:51 [PATCH 0/2] TDA7419 audio processor driver Matt Porter @ 2018-02-27 22:51 ` Matt Porter 2018-03-05 22:29 ` Rob Herring 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter 1 sibling, 1 reply; 14+ messages in thread From: Matt Porter @ 2018-02-27 22:51 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland Cc: alsa-devel, devicetree, linux-kernel DeviceTree binding for the tda7419 audio processor. Signed-off-by: Matt Porter <mporter@konsulko.com> --- Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt new file mode 100644 index 000000000000..919318315600 --- /dev/null +++ b/Documentation/devicetree/bindings/sound/tda7419.txt @@ -0,0 +1,15 @@ +TDA7419 audio processor + +This device supports I2C only. + +Required properties: + +- compatible : "st,tda7419" +- reg : the I2C address of the device. + +Example: + +ap: tda7419@44 { + compatible = "st,tda7419"; + reg = <0x44>; +}; -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: add tda7419 audio processor binding 2018-02-27 22:51 ` [PATCH 1/2] ASoC: add tda7419 audio processor binding Matt Porter @ 2018-03-05 22:29 ` Rob Herring 2018-03-09 14:39 ` Matt Porter 0 siblings, 1 reply; 14+ messages in thread From: Rob Herring @ 2018-03-05 22:29 UTC (permalink / raw) To: Matt Porter Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Mark Rutland, alsa-devel, devicetree, linux-kernel On Tue, Feb 27, 2018 at 05:51:27PM -0500, Matt Porter wrote: > DeviceTree binding for the tda7419 audio processor. > > Signed-off-by: Matt Porter <mporter@konsulko.com> > --- > Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt > > diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt > new file mode 100644 > index 000000000000..919318315600 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/tda7419.txt > @@ -0,0 +1,15 @@ > +TDA7419 audio processor > + > +This device supports I2C only. > + > +Required properties: > + > +- compatible : "st,tda7419" > +- reg : the I2C address of the device. For completeness: st,mute-gpios for MUTE pin? vdd-supply? > + > +Example: > + > +ap: tda7419@44 { > + compatible = "st,tda7419"; > + reg = <0x44>; > +}; > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: add tda7419 audio processor binding 2018-03-05 22:29 ` Rob Herring @ 2018-03-09 14:39 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-09 14:39 UTC (permalink / raw) To: Rob Herring Cc: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Mark Rutland, alsa-devel, devicetree, linux-kernel On Mon, Mar 05, 2018 at 04:29:54PM -0600, Rob Herring wrote: > On Tue, Feb 27, 2018 at 05:51:27PM -0500, Matt Porter wrote: > > DeviceTree binding for the tda7419 audio processor. > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > --- > > Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt > > > > diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt > > new file mode 100644 > > index 000000000000..919318315600 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/tda7419.txt > > @@ -0,0 +1,15 @@ > > +TDA7419 audio processor > > + > > +This device supports I2C only. > > + > > +Required properties: > > + > > +- compatible : "st,tda7419" > > +- reg : the I2C address of the device. > > For completeness: > > st,mute-gpios for MUTE pin? I'll add mute in as an optional property. > > vdd-supply? Yes, there's a required ~8.5V supply so I'll add the regulator reference. Thanks, Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] ASoC: add tda7419 audio processor binding @ 2018-03-09 14:39 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-09 14:39 UTC (permalink / raw) To: Rob Herring Cc: Mark Rutland, devicetree, alsa-devel, linux-kernel, Takashi Iwai, Liam Girdwood, Mark Brown On Mon, Mar 05, 2018 at 04:29:54PM -0600, Rob Herring wrote: > On Tue, Feb 27, 2018 at 05:51:27PM -0500, Matt Porter wrote: > > DeviceTree binding for the tda7419 audio processor. > > > > Signed-off-by: Matt Porter <mporter@konsulko.com> > > --- > > Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt > > > > diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt > > new file mode 100644 > > index 000000000000..919318315600 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/tda7419.txt > > @@ -0,0 +1,15 @@ > > +TDA7419 audio processor > > + > > +This device supports I2C only. > > + > > +Required properties: > > + > > +- compatible : "st,tda7419" > > +- reg : the I2C address of the device. > > For completeness: > > st,mute-gpios for MUTE pin? I'll add mute in as an optional property. > > vdd-supply? Yes, there's a required ~8.5V supply so I'll add the regulator reference. Thanks, Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-02-27 22:51 [PATCH 0/2] TDA7419 audio processor driver Matt Porter 2018-02-27 22:51 ` [PATCH 1/2] ASoC: add tda7419 audio processor binding Matt Porter @ 2018-02-27 22:51 ` Matt Porter 2018-02-28 11:00 ` Mark Brown ` (2 more replies) 1 sibling, 3 replies; 14+ messages in thread From: Matt Porter @ 2018-02-27 22:51 UTC (permalink / raw) To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland Cc: alsa-devel, devicetree, linux-kernel Component driver for the tda7419 audio processor. Signed-off-by: Matt Porter <mporter@konsulko.com> --- sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda7419.c | 571 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 579 insertions(+) create mode 100644 sound/soc/codecs/tda7419.c diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 2b331f7266ab..1553cf2b9445 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -151,6 +151,7 @@ config SND_SOC_ALL_CODECS select SND_SOC_TAS571X if I2C select SND_SOC_TAS5720 if I2C select SND_SOC_TAS6424 if I2C + select SND_SOC_TDA7419 if I2C select SND_SOC_TFA9879 if I2C select SND_SOC_TLV320AIC23_I2C if I2C select SND_SOC_TLV320AIC23_SPI if SPI_MASTER @@ -910,6 +911,11 @@ config SND_SOC_TAS6424 Enable support for Texas Instruments TAS6424 high-efficiency digital input quad-channel Class-D audio power amplifiers. +config SND_SOC_TDA7419 + tristate "ST TDA7419 audio processor" + depends on I2C + select REGMAP_I2C + config SND_SOC_TFA9879 tristate "NXP Semiconductors TFA9879 amplifier" depends on I2C diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index da1571336f1e..6cf3c3b92cb5 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -160,6 +160,7 @@ snd-soc-tas5086-objs := tas5086.o snd-soc-tas571x-objs := tas571x.o snd-soc-tas5720-objs := tas5720.o snd-soc-tas6424-objs := tas6424.o +snd-soc-tda7419-objs := tda7419.o snd-soc-tfa9879-objs := tfa9879.o snd-soc-tlv320aic23-objs := tlv320aic23.o snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o @@ -405,6 +406,7 @@ obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o obj-$(CONFIG_SND_SOC_TAS571X) += snd-soc-tas571x.o obj-$(CONFIG_SND_SOC_TAS5720) += snd-soc-tas5720.o obj-$(CONFIG_SND_SOC_TAS6424) += snd-soc-tas6424.o +obj-$(CONFIG_SND_SOC_TDA7419) += snd-soc-tda7419.o obj-$(CONFIG_SND_SOC_TFA9879) += snd-soc-tfa9879.o obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C) += snd-soc-tlv320aic23-i2c.o diff --git a/sound/soc/codecs/tda7419.c b/sound/soc/codecs/tda7419.c new file mode 100644 index 000000000000..97a7d21b8f2a --- /dev/null +++ b/sound/soc/codecs/tda7419.c @@ -0,0 +1,571 @@ +/* + * TDA7419 audio processor driver + * + * Copyright 2018 Konsulko Group + * + * Author: Matt Porter <mporter@konsulko.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * General Public License for more details. + */ + +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/regmap.h> +#include <sound/core.h> +#include <sound/control.h> +#include <sound/soc.h> +#include <sound/tlv.h> + +#define TDA7419_MAIN_SRC_REG 0x00 +#define TDA7419_LOUDNESS_REG 0x01 +#define TDA7419_MUTE_CLK_REG 0x02 +#define TDA7419_VOLUME_REG 0x03 +#define TDA7419_TREBLE_REG 0x04 +#define TDA7419_MIDDLE_REG 0x05 +#define TDA7419_BASS_REG 0x06 +#define TDA7419_SECOND_SRC_REG 0x07 +#define TDA7419_SUB_MID_BASS_REG 0x08 +#define TDA7419_MIXING_GAIN_REG 0x09 +#define TDA7419_ATTENUATOR_LF_REG 0x0a +#define TDA7419_ATTENUATOR_RF_REG 0x0b +#define TDA7419_ATTENUATOR_LR_REG 0x0c +#define TDA7419_ATTENUATOR_RR_REG 0x0d +#define TDA7419_MIXING_LEVEL_REG 0x0e +#define TDA7419_ATTENUATOR_SUB_REG 0x0f +#define TDA7419_SA_CLK_AC_REG 0x10 +#define TDA7419_TESTING_REG 0x11 + +#define TDA7419_MAIN_SRC_SEL 0 +#define TDA7419_MAIN_SRC_GAIN 3 +#define TDA7419_MAIN_SRC_AUTOZERO 7 + +#define TDA7419_LOUDNESS_ATTEN 0 +#define TDA7419_LOUDNESS_CENTER_FREQ 4 +#define TDA7419_LOUDNESS_BOOST 6 +#define TDA7419_LOUDNESS_SOFT_STEP 7 + +#define TDA7419_VOLUME_SOFT_STEP 7 + +#define TDA7419_SOFT_MUTE 0 +#define TDA7419_MUTE_INFLUENCE 1 +#define TDA7419_SOFT_MUTE_TIME 2 +#define TDA7419_SOFT_STEP_TIME 4 +#define TDA7419_CLK_FAST_MODE 7 + +#define TDA7419_TREBLE_CENTER_FREQ 5 +#define TDA7419_REF_OUT_SELECT 7 + +#define TDA7419_MIDDLE_Q_FACTOR 5 +#define TDA7419_MIDDLE_SOFT_STEP 7 + +#define TDA7419_BASS_Q_FACTOR 5 +#define TDA7419_BASS_SOFT_STEP 7 + +#define TDA7419_SECOND_SRC_SEL 0 +#define TDA7419_SECOND_SRC_GAIN 3 +#define TDA7419_REAR_SPKR_SRC 7 + +#define TDA7419_SUB_CUT_OFF_FREQ 0 +#define TDA7419_MIDDLE_CENTER_FREQ 2 +#define TDA7419_BASS_CENTER_FREQ 4 +#define TDA7419_BASS_DC_MODE 6 +#define TDA7419_SMOOTHING_FILTER 7 + +#define TDA7419_MIX_LF 0 +#define TDA7419_MIX_RF 1 +#define TDA7419_MIX_ENABLE 2 +#define TDA7419_SUB_ENABLE 3 +#define TDA7419_HPF_GAIN 4 + +#define TDA7419_SA_Q_FACTOR 0 +#define TDA7419_RESET_MODE 1 +#define TDA7419_SA_SOURCE 2 +#define TDA7419_SA_RUN 3 +#define TDA7419_RESET 4 +#define TDA7419_CLK_SOURCE 5 +#define TDA7419_COUPLING_MODE 6 + +struct tda7419_data { + struct regmap *regmap; +}; + +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg) +{ + return true; +} + +static bool tda7419_readable_reg(struct device *dev, unsigned int reg) +{ + return false; +} + +static const struct reg_default tda7419_regmap_defaults[] = { + { TDA7419_MAIN_SRC_REG, 0xfe }, + { TDA7419_LOUDNESS_REG, 0xfe }, + { TDA7419_MUTE_CLK_REG, 0xfe }, + { TDA7419_VOLUME_REG, 0xfe }, + { TDA7419_TREBLE_REG, 0xfe }, + { TDA7419_MIDDLE_REG, 0xfe }, + { TDA7419_BASS_REG, 0xfe }, + { TDA7419_SECOND_SRC_REG, 0xfe }, + { TDA7419_SUB_MID_BASS_REG, 0xfe }, + { TDA7419_MIXING_GAIN_REG, 0xfe }, + { TDA7419_ATTENUATOR_LF_REG, 0xfe }, + { TDA7419_ATTENUATOR_RF_REG, 0xfe }, + { TDA7419_ATTENUATOR_LR_REG, 0xfe }, + { TDA7419_ATTENUATOR_RR_REG, 0xfe }, + { TDA7419_MIXING_LEVEL_REG, 0xfe }, + { TDA7419_ATTENUATOR_SUB_REG, 0xfe }, + { TDA7419_SA_CLK_AC_REG, 0xfe }, + { TDA7419_TESTING_REG, 0xfe }, +}; + +static const struct regmap_config tda7419_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .max_register = TDA7419_TESTING_REG, + .cache_type = REGCACHE_RBTREE, + .writeable_reg = tda7419_writeable_reg, + .readable_reg = tda7419_readable_reg, + .reg_defaults = tda7419_regmap_defaults, + .num_reg_defaults = ARRAY_SIZE(tda7419_regmap_defaults), +}; + +struct tda7419_vol_control { + int min, max; + unsigned int reg, rreg, mask, thresh; + unsigned int invert:1; +}; + +static inline bool tda7419_vol_is_stereo(struct tda7419_vol_control *tvc) +{ + if (tvc->reg == tvc->rreg) + return 0; + + return 1; +} + +static int tda7419_vol_info(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_info *uinfo) +{ + struct tda7419_vol_control *tvc = + (struct tda7419_vol_control *)kcontrol->private_value; + + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; + uinfo->count = tda7419_vol_is_stereo(tvc) ? 2 : 1; + uinfo->value.integer.min = tvc->min; + uinfo->value.integer.max = tvc->max; + + return 0; +} + +static inline int tda7419_vol_get_value(int val, unsigned int mask, + int thresh, unsigned int invert) +{ + val &= mask; + if (val < thresh) { + if (invert) + val = 0 - val; + } else if (val > thresh) { + if (invert) + val = val - thresh; + else + val = thresh - val; + } + + return val; +} + +static int tda7419_vol_get(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = snd_kcontrol_chip(kcontrol); + struct tda7419_vol_control *tvc = + (struct tda7419_vol_control *)kcontrol->private_value; + unsigned int reg = tvc->reg; + unsigned int rreg = tvc->rreg; + unsigned int mask = tvc->mask; + int thresh = tvc->thresh; + unsigned int invert = tvc->invert; + int val; + int ret; + + ret = snd_soc_component_read(component, reg, &val); + if (ret < 0) + return ret; + ucontrol->value.integer.value[0] = + tda7419_vol_get_value(val, mask, thresh, invert); + + if (tda7419_vol_is_stereo(tvc)) { + ret = snd_soc_component_read(component, rreg, &val); + if (ret < 0) + return ret; + ucontrol->value.integer.value[1] = + tda7419_vol_get_value(val, mask, thresh, invert); + } + + return 0; +} + +static inline int tda7419_vol_put_value(int val, int thresh, + unsigned int invert) +{ + if (val < 0) { + if (invert) + val = abs(val); + else + val = thresh - val; + } else if ((val > 0) && invert) { + val += thresh; + } + + return val; +} + +static int tda7419_vol_put(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_component *component = + snd_kcontrol_chip(kcontrol); + struct tda7419_vol_control *tvc = + (struct tda7419_vol_control *)kcontrol->private_value; + unsigned int reg = tvc->reg; + unsigned int rreg = tvc->rreg; + unsigned int mask = tvc->mask; + int thresh = tvc->thresh; + unsigned int invert = tvc->invert; + int val; + int ret; + + val = tda7419_vol_put_value(ucontrol->value.integer.value[0], + thresh, invert); + ret = snd_soc_component_update_bits(component, reg, + mask, val); + if (ret < 0) + return ret; + + if (tda7419_vol_is_stereo(tvc)) { + val = tda7419_vol_put_value(ucontrol->value.integer.value[1], + thresh, invert); + ret = snd_soc_component_update_bits(component, rreg, + mask, val); + } + + return ret; +} + +#define TDA7419_SINGLE_VALUE(xreg, xmask, xmin, xmax, xthresh, xinvert) \ + ((unsigned long)&(struct tda7419_vol_control) \ + {.reg = xreg, .rreg = xreg, .mask = xmask, .min = xmin, \ + .max = xmax, .thresh = xthresh, .invert = xinvert}) + +#define TDA7419_DOUBLE_R_VALUE(xregl, xregr, xmask, xmin, xmax, xthresh, \ + xinvert) \ + ((unsigned long)&(struct tda7419_vol_control) \ + {.reg = xregl, .rreg = xregr, .mask = xmask, .min = xmin, \ + .max = xmax, .thresh = xthresh, .invert = xinvert}) + +#define TDA7419_SINGLE_TLV(xname, xreg, xmask, xmin, xmax, xthresh, \ + xinvert, xtlv_array) \ +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \ + .name = xname, \ + .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \ + SNDRV_CTL_ELEM_ACCESS_READWRITE, \ + .tlv.p = (xtlv_array), \ + .info = tda7419_vol_info, \ + .get = tda7419_vol_get, \ + .put = tda7419_vol_put, \ + .private_value = TDA7419_SINGLE_VALUE(xreg, xmask, xmin, \ + xmax, xthresh, xinvert), \ +} + +#define TDA7419_DOUBLE_R_TLV(xname, xregl, xregr, xmask, xmin, xmax, \ + xthresh, xinvert, xtlv_array) \ +{ .iface = SNDRV_CTL_ELEM_IFACE_MIXER, \ + .name = xname, \ + .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \ + SNDRV_CTL_ELEM_ACCESS_READWRITE, \ + .tlv.p = (xtlv_array), \ + .info = tda7419_vol_info, \ + .get = tda7419_vol_get, \ + .put = tda7419_vol_put, \ + .private_value = TDA7419_DOUBLE_R_VALUE(xregl, xregr, xmask, \ + xmin, xmax, xthresh, \ + xinvert), \ +} + +static const char * const enum_src_sel[] = { + "QD", "SE1", "SE2", "SE3", "SE", "Mute", "Mute", "Mute"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_main_src_sel, + TDA7419_MAIN_SRC_REG, TDA7419_MAIN_SRC_SEL, enum_src_sel); +static DECLARE_TLV_DB_SCALE(tlv_src_gain, 0, 100, 0); + +static DECLARE_TLV_DB_SCALE(tlv_loudness_atten, -1500, 100, 0); +static const char * const enum_loudness_center_freq[] = { + "Flat", "400 Hz", "800 Hz", "2400 Hz"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_loudness_center_freq, + TDA7419_LOUDNESS_REG, TDA7419_LOUDNESS_CENTER_FREQ, + enum_loudness_center_freq); +static const char * const enum_mute_influence[] = { + "Pin and IIC", "IIC"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_mute_influence, + TDA7419_MUTE_CLK_REG, TDA7419_MUTE_INFLUENCE, enum_mute_influence); +static const char * const enum_soft_mute_time[] = { + "0.48 ms", "0.96 ms", "123 ms", "123 ms"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_soft_mute_time, + TDA7419_MUTE_CLK_REG, TDA7419_SOFT_MUTE_TIME, enum_soft_mute_time); +static const char * const enum_soft_step_time[] = { + "0.160 ms", "0.321 ms", "0.642 ms", "1.28 ms", + "2.56 ms", "5.12 ms", "10.24 ms", "20.48 ms"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_soft_step_time, + TDA7419_MUTE_CLK_REG, TDA7419_SOFT_STEP_TIME, enum_soft_step_time); +static DECLARE_TLV_DB_SCALE(tlv_volume, -8000, 100, 1); +static const char * const enum_treble_center_freq[] = { + "10.0 kHz", "12.5 kHz", "15.0 kHz", "17.5 kHz"}; +static DECLARE_TLV_DB_SCALE(tlv_filter, -1500, 100, 0); +static SOC_ENUM_SINGLE_DECL(soc_enum_treble_center_freq, + TDA7419_TREBLE_REG, TDA7419_TREBLE_CENTER_FREQ, + enum_treble_center_freq); +static const char * const enum_ref_out_select[] = { + "External Vref (4 V)", "Internal Vref (3.3 V)"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_ref_out_select, + TDA7419_TREBLE_REG, TDA7419_REF_OUT_SELECT, enum_ref_out_select); +static const char * const enum_middle_q_factor[] = { + "0.5", "0.75", "1.0", "1.25"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_middle_q_factor, + TDA7419_MIDDLE_REG, TDA7419_MIDDLE_Q_FACTOR, enum_middle_q_factor); +static const char * const enum_bass_q_factor[] = { + "1.0", "1.25", "1.5", "2.0"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_bass_q_factor, + TDA7419_BASS_REG, TDA7419_BASS_Q_FACTOR, enum_bass_q_factor); +static SOC_ENUM_SINGLE_DECL(soc_enum_second_src_sel, + TDA7419_SECOND_SRC_REG, TDA7419_SECOND_SRC_SEL, enum_src_sel); +static const char * const enum_rear_spkr_src[] = { + "Main", "Second"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_rear_spkr_src, + TDA7419_SECOND_SRC_REG, TDA7419_REAR_SPKR_SRC, enum_rear_spkr_src); +static const char * const enum_sub_cut_off_freq[] = { + "Flat", "80 Hz", "120 Hz", "160 Hz"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_sub_cut_off_freq, + TDA7419_SUB_MID_BASS_REG, TDA7419_SUB_CUT_OFF_FREQ, + enum_sub_cut_off_freq); +static const char * const enum_middle_center_freq[] = { + "500 Hz", "1000 Hz", "1500 Hz", "2500 Hz"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_middle_center_freq, + TDA7419_SUB_MID_BASS_REG, TDA7419_MIDDLE_CENTER_FREQ, + enum_middle_center_freq); +static const char * const enum_bass_center_freq[] = { + "60 Hz", "80 Hz", "100 Hz", "200 Hz"}; +static SOC_ENUM_SINGLE_DECL(soc_enum_bass_center_freq, + TDA7419_SUB_MID_BASS_REG, TDA7419_BASS_CENTER_FREQ, + enum_bass_center_freq); +static DECLARE_TLV_DB_SCALE(tlv_hpf_gain, 400, 200, 0); +static const char * const enum_sa_q_factor[] = { + "3.5", "1.75" }; +static SOC_ENUM_SINGLE_DECL(soc_enum_sa_q_factor, + TDA7419_SA_CLK_AC_REG, TDA7419_SA_Q_FACTOR, enum_sa_q_factor); +static const char * const enum_reset_mode[] = { + "IIC", "Auto" }; +static SOC_ENUM_SINGLE_DECL(soc_enum_reset_mode, + TDA7419_SA_CLK_AC_REG, TDA7419_RESET_MODE, enum_reset_mode); +static const char * const enum_sa_src[] = { + "Bass", "In Gain" }; +static SOC_ENUM_SINGLE_DECL(soc_enum_sa_src, + TDA7419_SA_CLK_AC_REG, TDA7419_SA_SOURCE, enum_sa_src); +static const char * const enum_clk_src[] = { + "Internal", "External" }; +static SOC_ENUM_SINGLE_DECL(soc_enum_clk_src, + TDA7419_SA_CLK_AC_REG, TDA7419_CLK_SOURCE, enum_clk_src); +static const char * const enum_coupling_mode[] = { + "DC Coupling (without HPF)", "AC Coupling after In Gain", + "DC Coupling (with HPF)", "AC Coupling after Bass" }; +static SOC_ENUM_SINGLE_DECL(soc_enum_coupling_mode, + TDA7419_SA_CLK_AC_REG, TDA7419_COUPLING_MODE, enum_coupling_mode); + +/* ASoC Controls */ +static struct snd_kcontrol_new tda7419_controls[] = { +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), +SOC_SINGLE_TLV("Main Source Capture Volume", TDA7419_MAIN_SRC_REG, + TDA7419_MAIN_SRC_GAIN, 15, 0, tlv_src_gain), +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG, + TDA7419_MAIN_SRC_AUTOZERO, 1, 1), +SOC_SINGLE_TLV("Loudness Playback Volume", TDA7419_LOUDNESS_REG, + TDA7419_LOUDNESS_ATTEN, 15, 1, tlv_loudness_atten), +SOC_ENUM("Loudness Center Frequency", soc_enum_loudness_center_freq), +SOC_SINGLE("Loudness High Boost", TDA7419_LOUDNESS_REG, + TDA7419_LOUDNESS_BOOST, 1, 1), +SOC_SINGLE("Loudness Soft Step", TDA7419_LOUDNESS_REG, + TDA7419_LOUDNESS_SOFT_STEP, 1, 1), +SOC_SINGLE("Soft Mute", TDA7419_MUTE_CLK_REG, TDA7419_SOFT_MUTE, 1, 1), +SOC_ENUM("Mute Influence", soc_enum_mute_influence), +SOC_ENUM("Soft Mute Time", soc_enum_soft_mute_time), +SOC_ENUM("Soft Step Time", soc_enum_soft_step_time), +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG, + TDA7419_CLK_FAST_MODE, 1, 1), +TDA7419_SINGLE_TLV("Master Playback Volume", TDA7419_VOLUME_REG, + 0x7f, -80, 15, 0x10, 0, tlv_volume), +SOC_SINGLE("Volume Soft Step", TDA7419_VOLUME_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +TDA7419_SINGLE_TLV("Treble Playback Volume", TDA7419_TREBLE_REG, + 0x1f, -15, 15, 0x10, 1, tlv_filter), +SOC_ENUM("Treble Center Frequency", soc_enum_treble_center_freq), +SOC_ENUM("Reference Output Select", soc_enum_ref_out_select), +TDA7419_SINGLE_TLV("Middle Playback Volume", TDA7419_MIDDLE_REG, + 0x1f, -15, 15, 0x10, 1, tlv_filter), +SOC_ENUM("Middle Q Factor", soc_enum_middle_q_factor), +SOC_SINGLE("Middle Soft Step", TDA7419_MIDDLE_REG, + TDA7419_MIDDLE_SOFT_STEP, 1, 1), +TDA7419_SINGLE_TLV("Bass Playback Volume", TDA7419_BASS_REG, + 0x1f, -15, 15, 0x10, 1, tlv_filter), +SOC_ENUM("Bass Q Factor", soc_enum_bass_q_factor), +SOC_SINGLE("Bass Soft Step", TDA7419_BASS_REG, + TDA7419_BASS_SOFT_STEP, 1, 1), +SOC_ENUM("Second Source Select", soc_enum_second_src_sel), +SOC_SINGLE_TLV("Second Source Capture Volume", TDA7419_SECOND_SRC_REG, + TDA7419_SECOND_SRC_GAIN, 15, 0, tlv_src_gain), +SOC_ENUM("Rear Speaker Source", soc_enum_rear_spkr_src), +SOC_ENUM("Subwoofer Cut-off Frequency", soc_enum_sub_cut_off_freq), +SOC_ENUM("Middle Center Frequency", soc_enum_middle_center_freq), +SOC_ENUM("Bass Center Frequency", soc_enum_bass_center_freq), +SOC_SINGLE("Bass DC Mode", TDA7419_SUB_MID_BASS_REG, + TDA7419_BASS_DC_MODE, 1, 1), +SOC_SINGLE("Smoothing Filter", TDA7419_SUB_MID_BASS_REG, + TDA7419_SMOOTHING_FILTER, 1, 1), +SOC_SINGLE("Mix to LF Speaker", TDA7419_MIXING_GAIN_REG, + TDA7419_MIX_LF, 1, 1), +SOC_SINGLE("Mix to RF Speaker", TDA7419_MIXING_GAIN_REG, + TDA7419_MIX_RF, 1, 1), +SOC_SINGLE("Mix Enable", TDA7419_MIXING_GAIN_REG, TDA7419_MIX_ENABLE, 1, 1), +SOC_SINGLE("Subwoofer Enable", TDA7419_MIXING_GAIN_REG, + TDA7419_SUB_ENABLE, 1, 1), +SOC_SINGLE_TLV("HPF Filter Playback Volume", TDA7419_MIXING_GAIN_REG, + TDA7419_HPF_GAIN, 9, 0, tlv_hpf_gain), +TDA7419_DOUBLE_R_TLV("Front Playback Volume", TDA7419_ATTENUATOR_LF_REG, + TDA7419_ATTENUATOR_RF_REG, 0x7f, -80, 15, 0x10, 0, + tlv_volume), +SOC_SINGLE("Left Front Soft Step", TDA7419_ATTENUATOR_LF_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +SOC_SINGLE("Right Front Soft Step", TDA7419_ATTENUATOR_RF_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +TDA7419_DOUBLE_R_TLV("Rear Playback Volume", TDA7419_ATTENUATOR_LR_REG, + TDA7419_ATTENUATOR_RR_REG, 0x7f, -80, 15, 0x10, 0, + tlv_volume), +SOC_SINGLE("Left Rear Soft Step", TDA7419_ATTENUATOR_LR_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +SOC_SINGLE("Right Rear Soft Step", TDA7419_ATTENUATOR_RR_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +TDA7419_SINGLE_TLV("Mixing Capture Volume", TDA7419_MIXING_LEVEL_REG, + 0x7f, -80, 15, 0x10, 0, tlv_volume), +SOC_SINGLE("Mixing Level Soft Step", TDA7419_MIXING_LEVEL_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +TDA7419_SINGLE_TLV("Subwoofer Playback Volume", TDA7419_ATTENUATOR_SUB_REG, + 0x7f, -80, 15, 0x10, 0, tlv_volume), +SOC_SINGLE("Subwoofer Soft Step", TDA7419_ATTENUATOR_SUB_REG, + TDA7419_VOLUME_SOFT_STEP, 1, 1), +SOC_ENUM("Spectrum Analyzer Q Factor", soc_enum_sa_q_factor), +SOC_ENUM("Spectrum Analyzer Reset Mode", soc_enum_reset_mode), +SOC_ENUM("Spectrum Analyzer Source", soc_enum_sa_src), +SOC_SINGLE("Spectrum Analyzer Run", TDA7419_SA_CLK_AC_REG, + TDA7419_SA_RUN, 1, 1), +SOC_SINGLE("Spectrum Analyzer Reset", TDA7419_SA_CLK_AC_REG, + TDA7419_RESET, 1, 1), +SOC_ENUM("Clock Source", soc_enum_clk_src), +SOC_ENUM("Coupling Mode", soc_enum_coupling_mode), +}; + +static const struct snd_soc_component_driver tda7419_component_driver = { + .name = "tda7419", + .controls = tda7419_controls, + .num_controls = ARRAY_SIZE(tda7419_controls), +}; + +static int tda7419_probe(struct i2c_client *i2c, + const struct i2c_device_id *id) +{ + struct tda7419_data *tda7419; + int ret; + + tda7419 = devm_kzalloc(&i2c->dev, + sizeof(struct tda7419_data), + GFP_KERNEL); + if (tda7419 == NULL) + return -ENOMEM; + + i2c_set_clientdata(i2c, tda7419); + + tda7419->regmap = devm_regmap_init_i2c(i2c, &tda7419_regmap_config); + if (IS_ERR(tda7419->regmap)) { + ret = PTR_ERR(tda7419->regmap); + dev_err(&i2c->dev, "error initializing regmap: %d\n", + ret); + return ret; + } + + /* Configure registers */ + regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f); + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0); + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); + + ret = devm_snd_soc_register_component(&i2c->dev, + &tda7419_component_driver, NULL, 0); + if (ret < 0) { + dev_err(&i2c->dev, "error registering component: %d\n", + ret); + } + + return ret; +} + +static int tda7419_remove(struct i2c_client *i2c) +{ + int i; + struct tda7419_data *tda7419 = i2c_get_clientdata(i2c); + + /* Reset registers to defaults */ + for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++) + regmap_write(tda7419->regmap, + tda7419_regmap_defaults[i].reg, + tda7419_regmap_defaults[i].def); + + return 0; +} + +static const struct i2c_device_id tda7419_i2c_id[] = { + { "tda7419", 0 }, + { } +}; +MODULE_DEVICE_TABLE(i2c, tda7419_i2c_id); + +static const struct of_device_id tda7419_of_match[] = { + { .compatible = "st,tda7419" }, + { }, +}; + +static struct i2c_driver tda7419_driver = { + .driver = { + .name = "tda7419", + .of_match_table = tda7419_of_match, + }, + .probe = tda7419_probe, + .remove = tda7419_remove, + .id_table = tda7419_i2c_id, +}; + +module_i2c_driver(tda7419_driver); + +MODULE_AUTHOR("Matt Porter <mporter@konsulko.com>"); +MODULE_DESCRIPTION("TDA7419 audio processor driver"); +MODULE_LICENSE("GPL"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter @ 2018-02-28 11:00 ` Mark Brown 2018-03-09 14:35 ` Matt Porter 2018-03-02 22:48 ` kbuild test robot 2018-03-02 22:48 ` [PATCH] ASoC: fix boolreturn.cocci warnings kbuild test robot 2 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2018-02-28 11:00 UTC (permalink / raw) To: Matt Porter Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2663 bytes --] On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg) > +{ > + return true; > +} This is the default behaviour, may as well omit it (but equally it does no harm). > +static inline int tda7419_vol_get_value(int val, unsigned int mask, > + int thresh, unsigned int invert) > +{ > + val &= mask; > + if (val < thresh) { > + if (invert) > + val = 0 - val; > + } else if (val > thresh) { This feels like something some other device might want to use so might warrant pulling out into a general control at some point but I'd not insist on doing it now. > +static struct snd_kcontrol_new tda7419_controls[] = { > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), Should this be a DAPM route? > +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG, > + TDA7419_MAIN_SRC_AUTOZERO, 1, 1), There's a lot of on/off switches for various things in here - these should all have Switch at the end of their names so that userspace can see how it's expected to display them. Most of the controls with a max value of 1 probably fall into this category. > +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG, > + TDA7419_CLK_FAST_MODE, 1, 1), What does this do - should it be in set_sysclk() or something? > + /* Configure registers */ > + regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f); > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0); > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); This looks like it's setting default volumes - just leave those at the chip defaults and let userspace handle setting them, what works for one board may be totally inappropriate on another board and using the chip default means we've got some fixed thing we don't need to discuss. > +static int tda7419_remove(struct i2c_client *i2c) > +{ > + int i; > + struct tda7419_data *tda7419 = i2c_get_clientdata(i2c); > + > + /* Reset registers to defaults */ > + for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++) > + regmap_write(tda7419->regmap, > + tda7419_regmap_defaults[i].reg, > + tda7419_regmap_defaults[i].def); Why are we doing this? Other drivers don't do it... if anything I'd expect a reset on probe in case the bootloader or something left the chip configured. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-02-28 11:00 ` Mark Brown @ 2018-03-09 14:35 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-09 14:35 UTC (permalink / raw) To: Mark Brown Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote: > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > > > +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg) > > +{ > > + return true; > > +} > > This is the default behaviour, may as well omit it (but equally it does > no harm). Ok. > > > +static inline int tda7419_vol_get_value(int val, unsigned int mask, > > + int thresh, unsigned int invert) > > +{ > > + val &= mask; > > + if (val < thresh) { > > + if (invert) > > + val = 0 - val; > > + } else if (val > thresh) { > > This feels like something some other device might want to use so might > warrant pulling out into a general control at some point but I'd not > insist on doing it now. Ok, yeah, I was also thinking it should be moved to a general helper when the next user shows up. The most likely case is another part in this family may have a similar register layout. > > +static struct snd_kcontrol_new tda7419_controls[] = { > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), > > Should this be a DAPM route? Ultimately yes. I initially took the path of ignoring DAPM support in interests of getting some clean done. Is it ok to merge DAPM support later or do you prefer just having it in the intitial driver? For routes, it'll include Main/Second source selects, the Rear Source switch, and Mix enable at least. > > +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG, > > + TDA7419_MAIN_SRC_AUTOZERO, 1, 1), > > There's a lot of on/off switches for various things in here - these > should all have Switch at the end of their names so that userspace can > see how it's expected to display them. Most of the controls with a max > value of 1 probably fall into this category. I see. I'll fix that. > > +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG, > > + TDA7419_CLK_FAST_MODE, 1, 1), > > What does this do - should it be in set_sysclk() or something? This is where things get hazy, unfortunately. The datasheet is partially garbage. So there's no description or guidance on clock management at all. It's not clear what clock this is or what specifically "fast" does. Because there's no concept of an external clock, the sysclk is clearly internally generated. Because of the register labeling of clock generator, it's probably sysclk but I'm not 100% sure what the relevance is of fast mode. The working settings were divined from trial and error as well a couple microcontroller projects scattered across the interwebs. On second look at this, I think I should at least remove this switch and hard code it for now or move to set_sysclk(). I'm hesistant of the latter because of the lack of information on this setting. > > + /* Configure registers */ > > + regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); > > This looks like it's setting default volumes - just leave those at the > chip defaults and let userspace handle setting them, what works for one > board may be totally inappropriate on another board and using the chip > default means we've got some fixed thing we don't need to discuss. This is actually setting the default/cache to the first mute value due to the assumption in my implementation of the tda7419-specific get/set for these registers. It simplified the code a bit to have these initialized like this. e.g. for the attenuator group of registers, x11xxxxx are all mute values, so 0xe0 is setting these regs to that first mute value to simplify things. I'll take another look at eliminating this. As it is, it does not change the fact that the actual reset value of 0xff is also mute from a user POV. > > +static int tda7419_remove(struct i2c_client *i2c) > > +{ > > + int i; > > + struct tda7419_data *tda7419 = i2c_get_clientdata(i2c); > > + > > + /* Reset registers to defaults */ > > + for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++) > > + regmap_write(tda7419->regmap, > > + tda7419_regmap_defaults[i].reg, > > + tda7419_regmap_defaults[i].def); > > Why are we doing this? Other drivers don't do it... if anything I'd > expect a reset on probe in case the bootloader or something left the > chip configured. Good point, I'll move this into probe. The part doesn't have a soft reset provision so we need to do it manually like this. -Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver @ 2018-03-09 14:35 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-09 14:35 UTC (permalink / raw) To: Mark Brown Cc: Mark Rutland, devicetree, alsa-devel, linux-kernel, Takashi Iwai, Liam Girdwood, Rob Herring On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote: > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > > > +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg) > > +{ > > + return true; > > +} > > This is the default behaviour, may as well omit it (but equally it does > no harm). Ok. > > > +static inline int tda7419_vol_get_value(int val, unsigned int mask, > > + int thresh, unsigned int invert) > > +{ > > + val &= mask; > > + if (val < thresh) { > > + if (invert) > > + val = 0 - val; > > + } else if (val > thresh) { > > This feels like something some other device might want to use so might > warrant pulling out into a general control at some point but I'd not > insist on doing it now. Ok, yeah, I was also thinking it should be moved to a general helper when the next user shows up. The most likely case is another part in this family may have a similar register layout. > > +static struct snd_kcontrol_new tda7419_controls[] = { > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), > > Should this be a DAPM route? Ultimately yes. I initially took the path of ignoring DAPM support in interests of getting some clean done. Is it ok to merge DAPM support later or do you prefer just having it in the intitial driver? For routes, it'll include Main/Second source selects, the Rear Source switch, and Mix enable at least. > > +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG, > > + TDA7419_MAIN_SRC_AUTOZERO, 1, 1), > > There's a lot of on/off switches for various things in here - these > should all have Switch at the end of their names so that userspace can > see how it's expected to display them. Most of the controls with a max > value of 1 probably fall into this category. I see. I'll fix that. > > +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG, > > + TDA7419_CLK_FAST_MODE, 1, 1), > > What does this do - should it be in set_sysclk() or something? This is where things get hazy, unfortunately. The datasheet is partially garbage. So there's no description or guidance on clock management at all. It's not clear what clock this is or what specifically "fast" does. Because there's no concept of an external clock, the sysclk is clearly internally generated. Because of the register labeling of clock generator, it's probably sysclk but I'm not 100% sure what the relevance is of fast mode. The working settings were divined from trial and error as well a couple microcontroller projects scattered across the interwebs. On second look at this, I think I should at least remove this switch and hard code it for now or move to set_sysclk(). I'm hesistant of the latter because of the lack of information on this setting. > > + /* Configure registers */ > > + regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0); > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); > > This looks like it's setting default volumes - just leave those at the > chip defaults and let userspace handle setting them, what works for one > board may be totally inappropriate on another board and using the chip > default means we've got some fixed thing we don't need to discuss. This is actually setting the default/cache to the first mute value due to the assumption in my implementation of the tda7419-specific get/set for these registers. It simplified the code a bit to have these initialized like this. e.g. for the attenuator group of registers, x11xxxxx are all mute values, so 0xe0 is setting these regs to that first mute value to simplify things. I'll take another look at eliminating this. As it is, it does not change the fact that the actual reset value of 0xff is also mute from a user POV. > > +static int tda7419_remove(struct i2c_client *i2c) > > +{ > > + int i; > > + struct tda7419_data *tda7419 = i2c_get_clientdata(i2c); > > + > > + /* Reset registers to defaults */ > > + for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++) > > + regmap_write(tda7419->regmap, > > + tda7419_regmap_defaults[i].reg, > > + tda7419_regmap_defaults[i].def); > > Why are we doing this? Other drivers don't do it... if anything I'd > expect a reset on probe in case the bootloader or something left the > chip configured. Good point, I'll move this into probe. The part doesn't have a soft reset provision so we need to do it manually like this. -Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-03-09 14:35 ` Matt Porter (?) @ 2018-03-09 15:29 ` Mark Brown 2018-03-18 17:21 ` Matt Porter -1 siblings, 1 reply; 14+ messages in thread From: Mark Brown @ 2018-03-09 15:29 UTC (permalink / raw) To: Matt Porter Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1929 bytes --] On Fri, Mar 09, 2018 at 09:35:48AM -0500, Matt Porter wrote: > On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote: > > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > > > +static struct snd_kcontrol_new tda7419_controls[] = { > > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), > > Should this be a DAPM route? > Ultimately yes. I initially took the path of ignoring DAPM support in > interests of getting some clean done. Is it ok to merge DAPM support > later or do you prefer just having it in the intitial driver? For > routes, it'll include Main/Second source selects, the Rear Source > switch, and Mix enable at least. You definitely shouldn't be implementing things that should be in DAPM as non-DAPM controls. > > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); > > This looks like it's setting default volumes - just leave those at the > > chip defaults and let userspace handle setting them, what works for one > > board may be totally inappropriate on another board and using the chip > > default means we've got some fixed thing we don't need to discuss. > This is actually setting the default/cache to the first mute value due > to the assumption in my implementation of the tda7419-specific get/set > for these registers. It simplified the code a bit to have these > initialized like this. e.g. for the attenuator group of registers, > x11xxxxx are all mute values, so 0xe0 is setting these regs to that > first mute value to simplify things. I'll take another look at > eliminating this. As it is, it does not change the fact that the actual > reset value of 0xff is also mute from a user POV. If it is useful it definitely needs a comment explaining what's happening and that there's no practical change to the configuration. It would be nicer to be robust against the device getting a wider range of values in the register but that seems plausible. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-03-09 15:29 ` Mark Brown @ 2018-03-18 17:21 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-18 17:21 UTC (permalink / raw) To: Mark Brown Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel On Fri, Mar 09, 2018 at 03:29:12PM +0000, Mark Brown wrote: > On Fri, Mar 09, 2018 at 09:35:48AM -0500, Matt Porter wrote: > > On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote: > > > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > > > > > +static struct snd_kcontrol_new tda7419_controls[] = { > > > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), > > > > Should this be a DAPM route? > > > Ultimately yes. I initially took the path of ignoring DAPM support in > > interests of getting some clean done. Is it ok to merge DAPM support > > later or do you prefer just having it in the intitial driver? For > > routes, it'll include Main/Second source selects, the Rear Source > > switch, and Mix enable at least. > > You definitely shouldn't be implementing things that should be in DAPM > as non-DAPM controls. Ok, I addressed this by adding DAPM support in v2. > > > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); > > > > This looks like it's setting default volumes - just leave those at the > > > chip defaults and let userspace handle setting them, what works for one > > > board may be totally inappropriate on another board and using the chip > > > default means we've got some fixed thing we don't need to discuss. > > > This is actually setting the default/cache to the first mute value due > > to the assumption in my implementation of the tda7419-specific get/set > > for these registers. It simplified the code a bit to have these > > initialized like this. e.g. for the attenuator group of registers, > > x11xxxxx are all mute values, so 0xe0 is setting these regs to that > > first mute value to simplify things. I'll take another look at > > eliminating this. As it is, it does not change the fact that the actual > > reset value of 0xff is also mute from a user POV. > > If it is useful it definitely needs a comment explaining what's > happening and that there's no practical change to the configuration. It > would be nicer to be robust against the device getting a wider range of > values in the register but that seems plausible. I did some rework to make this unnecessary in v2. Thanks, Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver @ 2018-03-18 17:21 ` Matt Porter 0 siblings, 0 replies; 14+ messages in thread From: Matt Porter @ 2018-03-18 17:21 UTC (permalink / raw) To: Mark Brown Cc: Mark Rutland, devicetree, alsa-devel, linux-kernel, Takashi Iwai, Liam Girdwood, Rob Herring On Fri, Mar 09, 2018 at 03:29:12PM +0000, Mark Brown wrote: > On Fri, Mar 09, 2018 at 09:35:48AM -0500, Matt Porter wrote: > > On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote: > > > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote: > > > > > +static struct snd_kcontrol_new tda7419_controls[] = { > > > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel), > > > > Should this be a DAPM route? > > > Ultimately yes. I initially took the path of ignoring DAPM support in > > interests of getting some clean done. Is it ok to merge DAPM support > > later or do you prefer just having it in the intitial driver? For > > routes, it'll include Main/Second source selects, the Rear Source > > switch, and Mix enable at least. > > You definitely shouldn't be implementing things that should be in DAPM > as non-DAPM controls. Ok, I addressed this by adding DAPM support in v2. > > > > + regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0); > > > > This looks like it's setting default volumes - just leave those at the > > > chip defaults and let userspace handle setting them, what works for one > > > board may be totally inappropriate on another board and using the chip > > > default means we've got some fixed thing we don't need to discuss. > > > This is actually setting the default/cache to the first mute value due > > to the assumption in my implementation of the tda7419-specific get/set > > for these registers. It simplified the code a bit to have these > > initialized like this. e.g. for the attenuator group of registers, > > x11xxxxx are all mute values, so 0xe0 is setting these regs to that > > first mute value to simplify things. I'll take another look at > > eliminating this. As it is, it does not change the fact that the actual > > reset value of 0xff is also mute from a user POV. > > If it is useful it definitely needs a comment explaining what's > happening and that there's no practical change to the configuration. It > would be nicer to be robust against the device getting a wider range of > values in the register but that seems plausible. I did some rework to make this unnecessary in v2. Thanks, Matt ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter 2018-02-28 11:00 ` Mark Brown @ 2018-03-02 22:48 ` kbuild test robot 2018-03-02 22:48 ` [PATCH] ASoC: fix boolreturn.cocci warnings kbuild test robot 2 siblings, 0 replies; 14+ messages in thread From: kbuild test robot @ 2018-03-02 22:48 UTC (permalink / raw) To: Matt Porter Cc: kbuild-all, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel Hi Matt, I love your patch! Perhaps something to improve: [auto build test WARNING on asoc/for-next] [also build test WARNING on v4.16-rc3 next-20180302] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Matt-Porter/TDA7419-audio-processor-driver/20180303-042824 base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next coccinelle warnings: (new ones prefixed by >>) >> sound/soc/codecs/tda7419.c:151:9-10: WARNING: return of 0/1 in function 'tda7419_vol_is_stereo' with return type bool Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] ASoC: fix boolreturn.cocci warnings 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter 2018-02-28 11:00 ` Mark Brown 2018-03-02 22:48 ` kbuild test robot @ 2018-03-02 22:48 ` kbuild test robot 2 siblings, 0 replies; 14+ messages in thread From: kbuild test robot @ 2018-03-02 22:48 UTC (permalink / raw) To: Matt Porter Cc: kbuild-all, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, Rob Herring, Mark Rutland, alsa-devel, devicetree, linux-kernel From: Fengguang Wu <fengguang.wu@intel.com> sound/soc/codecs/tda7419.c:151:9-10: WARNING: return of 0/1 in function 'tda7419_vol_is_stereo' with return type bool Return statements in functions returning bool should use true/false instead of 1/0. Generated by: scripts/coccinelle/misc/boolreturn.cocci Fixes: d811df0fabec ("ASoC: add tda7419 audio processor driver") CC: Matt Porter <mporter@konsulko.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- tda7419.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) --- a/sound/soc/codecs/tda7419.c +++ b/sound/soc/codecs/tda7419.c @@ -148,9 +148,9 @@ struct tda7419_vol_control { static inline bool tda7419_vol_is_stereo(struct tda7419_vol_control *tvc) { if (tvc->reg == tvc->rreg) - return 0; + return false; - return 1; + return true; } static int tda7419_vol_info(struct snd_kcontrol *kcontrol, ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-03-18 17:22 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-27 22:51 [PATCH 0/2] TDA7419 audio processor driver Matt Porter 2018-02-27 22:51 ` [PATCH 1/2] ASoC: add tda7419 audio processor binding Matt Porter 2018-03-05 22:29 ` Rob Herring 2018-03-09 14:39 ` Matt Porter 2018-03-09 14:39 ` Matt Porter 2018-02-27 22:51 ` [PATCH 2/2] ASoC: add tda7419 audio processor driver Matt Porter 2018-02-28 11:00 ` Mark Brown 2018-03-09 14:35 ` Matt Porter 2018-03-09 14:35 ` Matt Porter 2018-03-09 15:29 ` Mark Brown 2018-03-18 17:21 ` Matt Porter 2018-03-18 17:21 ` Matt Porter 2018-03-02 22:48 ` kbuild test robot 2018-03-02 22:48 ` [PATCH] ASoC: fix boolreturn.cocci warnings kbuild test robot
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.