On 2016-11-09 14:38, Mark Brown wrote: > On Tue, Nov 08, 2016 at 05:20:57PM +0100, Peter Rosin wrote: > >> +++ b/sound/soc/axentia/Kconfig >> @@ -0,0 +1,10 @@ >> +config SND_SOC_AXENTIA_TSE850_PCM5142 >> + tristate "ASoC driver for the Axentia TSE-850" >> + depends on ARCH_AT91 && OF >> + select ATMEL_SSC >> + select SND_ATMEL_SOC >> + select SND_ATMEL_SOC_SSC_DMA >> + select SND_SOC_PCM512x_I2C >> + help >> + Say Y if you want to add support for the ASoC driver for the >> + Axentia TSE-850 with a PCM5142 codec. > > This just looks like a normal machine driver for an Atmel system which > would usually go in the atemel directory - why is a new directory being > created? I thought atmel in this context meant that Atmel made the board, not that the board was based on an Atmel cpu. I'll move it for v2. >> +static int tse850_get_mux2(struct snd_kcontrol *kctrl, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl); >> + struct snd_soc_card *card = dapm->card; >> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card); >> + int ret; >> + >> + ret = gpiod_get_value(tse850->loop2); >> + if (ret < 0) >> + return ret; > > We can't reliably read the value of output GPIOs (though in practice the > majority do support it) so it'd be better practice to use a state > variable to remember what we set. I'd also expect this to use the > _cansleep() GPIO calls as it's not in a context where sleeping would be > a problem. Ok, I'll add _cansleep and cached values for v2. >> +int tse850_get_ana(struct snd_kcontrol *kctrl, >> + struct snd_ctl_elem_value *ucontrol) >> +{ >> + struct snd_soc_dapm_context *dapm = snd_soc_dapm_kcontrol_dapm(kctrl); >> + struct snd_soc_card *card = dapm->card; >> + struct tse850_priv *tse850 = snd_soc_card_get_drvdata(card); >> + int ret; >> + >> + ret = regulator_get_voltage(tse850->ana); >> + if (ret < 0) >> + return ret; >> + >> + if (ret < 11000000) >> + ret = 11000000; >> + else if (ret > 20000000) >> + ret = 20000000; > > This needs some comments... Ok, I'll add some words... >> + struct snd_soc_pcm_runtime *rtd = substream->private_data; >> + struct device *dev = rtd->dev; >> + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; >> + int dir = substream->stream != SNDRV_PCM_STREAM_PLAYBACK; >> + int div_id = dir ? ATMEL_SSC_RCMR_PERIOD : ATMEL_SSC_TCMR_PERIOD; >> + int period = snd_soc_params_to_frame_size(params) / 2 - 1; > > Please write the logic out as normal if statements for legibility. It's > a bit concerning that we even need this function, it looks like pretty > basic stuff that I'd expect the CPU DAI to just be doing - why can't > this be the default behaviour of the CPU DAI? I don't know and obviously don't have all the relevant HW to test changes. Do you want me to attempt such a change anyway? Adding Cc: Nicolas Ferre >> +static int tse850_init(struct snd_soc_pcm_runtime *rtd) >> +{ >> + struct snd_soc_dapm_context *dapm = &rtd->card->dapm; >> + >> + return snd_soc_dapm_add_routes(dapm, tse850_intercon, >> + ARRAY_SIZE(tse850_intercon)); > > Set this up in the card data structure rather than open coding the call, > you can register DAPM routes there too. Right. Thanks for looking! Cheers, Peter -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html