| Message ID | 1495121559-1063-4-git-send-email-ckeepax@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 18, 2017 at 04:32:39PM +0100, Charles Keepax wrote: > + SOC_SINGLE_EXT("SYNC Audio", CS35L35_MULT_DEV_SYNCH2, 1, 1, 0, > + snd_soc_get_volsw, cs35l35_put_sync), > + SOC_SINGLE_EXT("SYNC VPBR", CS35L35_MULT_DEV_SYNCH2, 2, 1, 0, > + snd_soc_get_volsw, cs35l35_put_sync), > + SOC_SINGLE_EXT("SYNC OTW", CS35L35_MULT_DEV_SYNCH2, 3, 1, 0, > + snd_soc_get_volsw, cs35l35_put_sync), I can't tell how this works. It feels like this shouldn't just be being controlled from userspace but rather should be handled in some more standard fashion, or possibly as part of the platform integration but right now it's just some totally undocumented application managed controls.
On Mon, May 29, 2017 at 02:53:06PM +0100, Mark Brown wrote: > On Thu, May 18, 2017 at 04:32:39PM +0100, Charles Keepax wrote: > > > + SOC_SINGLE_EXT("SYNC Audio", CS35L35_MULT_DEV_SYNCH2, 1, 1, 0, > > + snd_soc_get_volsw, cs35l35_put_sync), > > + SOC_SINGLE_EXT("SYNC VPBR", CS35L35_MULT_DEV_SYNCH2, 2, 1, 0, > > + snd_soc_get_volsw, cs35l35_put_sync), > > + SOC_SINGLE_EXT("SYNC OTW", CS35L35_MULT_DEV_SYNCH2, 3, 1, 0, > > + snd_soc_get_volsw, cs35l35_put_sync), > > I can't tell how this works. It feels like this shouldn't just be being > controlled from userspace but rather should be handled in some more > standard fashion, or possibly as part of the platform integration but > right now it's just some totally undocumented application managed > controls. These activate the individual types of synchronisation between the two stereo amps over a proprietary single wire connection between the two amps. The audio option synchronises the group delay between the two amps. The VPBR links the brown out on the two amps and the OTW links the over temperature warning. The issue is really one of it being use-case specific whether the amps are being used independently or in a stereo configuration. You could have only a single amp in use in which case the sync features are best turned off. Or one might even have use-cases with both amps where they are being used independently but at the same time. I guess we could potentially do those as calls from the machine driver, although in some cases it might be hard to tell which use-case is being used. Alternatively, one could try to actually link the two amps in DAPM and control it that way although you probably still want some device tree stuff to say which things you want to sync and it might cause issues in any cases where you had both amps up but didn't want to sync. Thanks, Charles
On Tue, May 30, 2017 at 09:51:38AM +0100, Charles Keepax wrote: > On Mon, May 29, 2017 at 02:53:06PM +0100, Mark Brown wrote: > > On Thu, May 18, 2017 at 04:32:39PM +0100, Charles Keepax wrote: > > > > > + SOC_SINGLE_EXT("SYNC Audio", CS35L35_MULT_DEV_SYNCH2, 1, 1, 0, > > > + snd_soc_get_volsw, cs35l35_put_sync), > > > + SOC_SINGLE_EXT("SYNC VPBR", CS35L35_MULT_DEV_SYNCH2, 2, 1, 0, > > > + snd_soc_get_volsw, cs35l35_put_sync), > > > + SOC_SINGLE_EXT("SYNC OTW", CS35L35_MULT_DEV_SYNCH2, 3, 1, 0, > > > + snd_soc_get_volsw, cs35l35_put_sync), > > > > I can't tell how this works. It feels like this shouldn't just be being > > controlled from userspace but rather should be handled in some more > > standard fashion, or possibly as part of the platform integration but > > right now it's just some totally undocumented application managed > > controls. > > These activate the individual types of synchronisation between > the two stereo amps over a proprietary single wire connection > between the two amps. The audio option synchronises the group > delay between the two amps. The VPBR links the brown out on the > two amps and the OTW links the over temperature warning. > > The issue is really one of it being use-case specific whether the > amps are being used independently or in a stereo configuration. > You could have only a single amp in use in which case the sync > features are best turned off. Or one might even have use-cases > with both amps where they are being used independently but at the > same time. > > I guess we could potentially do those as calls from the machine > driver, although in some cases it might be hard to tell which > use-case is being used. Alternatively, one could try to actually > link the two amps in DAPM and control it that way although you > probably still want some device tree stuff to say which things > you want to sync and it might cause issues in any cases where you > had both amps up but didn't want to sync. Ok turns out there is one other corner case issue here, lets drop this patch for now and I will try to see if I can come up with better solution for it. Thanks, Charles
On Tue, May 30, 2017 at 09:51:38AM +0100, Charles Keepax wrote: > These activate the individual types of synchronisation between > the two stereo amps over a proprietary single wire connection > between the two amps. The audio option synchronises the group So we should at least have something that represents that single wire connection in DT since it might not be there and so that the kernel can make sure that both devices agree if they're synced or not and can disable the sync if it's been enabled in error (if one of the devices is off, you seemed to be saying that might cause problems). > delay between the two amps. The VPBR links the brown out on the > two amps and the OTW links the over temperature warning. It's not clear to me when it'd make sense to disable these from a system integration point of view. > The issue is really one of it being use-case specific whether the > amps are being used independently or in a stereo configuration. > You could have only a single amp in use in which case the sync > features are best turned off. Or one might even have use-cases > with both amps where they are being used independently but at the > same time. It feels like if this is going to be user controllable the UI needs to be pulled up a level to be something like an on/off switch for the sync rather than a series of magic _EXT controls on the individual CODECs. Even if it is magic _EXT controls having them being per device feels wrong.
diff --git a/sound/soc/codecs/cs35l35.c b/sound/soc/codecs/cs35l35.c index 375be49..a587269 100644 --- a/sound/soc/codecs/cs35l35.c +++ b/sound/soc/codecs/cs35l35.c @@ -310,11 +310,49 @@ static const struct snd_kcontrol_new cs35l35_aud_controls[] = { amp_gain_tlv), }; -static const struct snd_kcontrol_new cs35l35_adv_controls[] = { +static int cs35l35_put_sync(struct snd_kcontrol *kcontrol, + struct snd_ctl_elem_value *ucontrol) +{ + struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol); + struct snd_soc_dapm_context *dapm = snd_soc_codec_get_dapm(codec); + struct cs35l35_private *cs35l35 = snd_soc_codec_get_drvdata(codec); + unsigned int val; + int ret; + + snd_soc_dapm_mutex_lock(dapm); + + ret = regmap_read(cs35l35->regmap, CS35L35_PWRCTL1, &val); + if (ret) { + dev_err(cs35l35->dev, "Failed to check power state: %d\n", ret); + goto err; + } + + if (!(val & CS35L35_PDN_ALL)) { + dev_err(cs35l35->dev, "Can't change sync when active\n"); + ret = -EBUSY; + goto err; + } + + ret = snd_soc_put_volsw(kcontrol, ucontrol); + +err: + snd_soc_dapm_mutex_unlock(dapm); + + return ret; +} + +static const struct snd_kcontrol_new cs35l35_stereo_controls[] = { SOC_SINGLE_SX_TLV("Digital Advisory Volume", CS35L35_ADV_DIG_VOL, 0, 0x34, 0xE4, dig_vol_tlv), SOC_SINGLE_TLV("Analog Advisory Volume", CS35L35_AMP_GAIN_ADV_CTL, 0, 19, 0, amp_gain_tlv), + + SOC_SINGLE_EXT("SYNC Audio", CS35L35_MULT_DEV_SYNCH2, 1, 1, 0, + snd_soc_get_volsw, cs35l35_put_sync), + SOC_SINGLE_EXT("SYNC VPBR", CS35L35_MULT_DEV_SYNCH2, 2, 1, 0, + snd_soc_get_volsw, cs35l35_put_sync), + SOC_SINGLE_EXT("SYNC OTW", CS35L35_MULT_DEV_SYNCH2, 3, 1, 0, + snd_soc_get_volsw, cs35l35_put_sync), }; static const struct snd_soc_dapm_widget cs35l35_dapm_widgets[] = { @@ -872,8 +910,12 @@ static int cs35l35_codec_probe(struct snd_soc_codec *codec) regmap_update_bits(cs35l35->regmap, CS35L35_CLASS_H_CTL, CS35L35_CH_STEREO_MASK, 1 << CS35L35_CH_STEREO_SHIFT); - ret = snd_soc_add_codec_controls(codec, cs35l35_adv_controls, - ARRAY_SIZE(cs35l35_adv_controls)); + + regmap_update_bits(cs35l35->regmap, CS35L35_MULT_DEV_SYNCH2, + CS35L35_SYNC_EN_MASK, 1); + + ret = snd_soc_add_codec_controls(codec, cs35l35_stereo_controls, + ARRAY_SIZE(cs35l35_stereo_controls)); if (ret) return ret; } diff --git a/sound/soc/codecs/cs35l35.h b/sound/soc/codecs/cs35l35.h index 621bfef..7cec198 100644 --- a/sound/soc/codecs/cs35l35.h +++ b/sound/soc/codecs/cs35l35.h @@ -273,6 +273,15 @@ #define CS35L35_VMON_OVFL 0x08 #define CS35L35_IMON_OVFL 0x04 +/* Mulit-Device Sync */ +#define CS35L35_SYNC_EN_MASK 0x01 +#define CS35L35_SYNC_MS_MASK 0x20 +#define CS35L35_SYNC_MS_SHIFT 5 +#define CS35L35_SYNC_MS_CTRL_MASK 0x10 +#define CS35L35_SYNC_MS_CTRL_SHIFT 4 +#define CS35L35_SYNC_CLK_SEL_MASK 0x02 +#define CS35L35_SYNC_CLK_SEL_SHIFT 1 + #define CS35L35_FORMATS (SNDRV_PCM_FMTBIT_U8 | SNDRV_PCM_FMTBIT_S16_LE | \ SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE)
This patch adds multi-device synchronisation capabilities to the CS35L35 amp. In stereo mode you can select to have the devices communicate certain features to each other. Add options to allow the amps to synchronise the audio group delay, the brown out protection and the over temperature warning. Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com> --- sound/soc/codecs/cs35l35.c | 48 +++++++++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/cs35l35.h | 9 +++++++++ 2 files changed, 54 insertions(+), 3 deletions(-)