[alsa-devel] [PATCH 0/4] New feature and clean up for pcm1792a driver
From: Raphael Poggi poggi.raph@gmail.com
Hi,
This patch series support for the channel selection (stereo/left/right) based on device tree bindings. It also adds some registers defines and cleans up weird test on function exit.
Raphael Poggi (4): sound: codecs: pcm1792a: clean function exit sound: codecs: pcm1792a: add define for register dt-bindings: sound: add binding for pcm1792a sound: codecs: pcm1792a: introduce channel side
.../devicetree/bindings/sound/pcm1792a.txt | 5 + sound/soc/codecs/pcm1792a.c | 107 +++++++++++++++------ 2 files changed, 83 insertions(+), 29 deletions(-)
From: Raphael Poggi poggi.raph@gmail.com
Signed-off-by: Raphael Poggi poggi.raph@gmail.com --- sound/soc/codecs/pcm1792a.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/sound/soc/codecs/pcm1792a.c b/sound/soc/codecs/pcm1792a.c index 08bb486..cf42b24 100644 --- a/sound/soc/codecs/pcm1792a.c +++ b/sound/soc/codecs/pcm1792a.c @@ -91,14 +91,9 @@ static int pcm1792a_digital_mute(struct snd_soc_dai *dai, int mute) { struct snd_soc_codec *codec = dai->codec; struct pcm1792a_private *priv = snd_soc_codec_get_drvdata(codec); - int ret;
- ret = regmap_update_bits(priv->regmap, PCM1792A_SOFT_MUTE, + return regmap_update_bits(priv->regmap, PCM1792A_SOFT_MUTE, PCM1792A_MUTE_MASK, !!mute); - if (ret < 0) - return ret; - - return 0; }
static int pcm1792a_hw_params(struct snd_pcm_substream *substream, @@ -107,7 +102,8 @@ static int pcm1792a_hw_params(struct snd_pcm_substream *substream, { struct snd_soc_codec *codec = dai->codec; struct pcm1792a_private *priv = snd_soc_codec_get_drvdata(codec); - int val = 0, ret; + int val = 0; + int ret = 0;
priv->rate = params_rate(params);
@@ -147,10 +143,8 @@ static int pcm1792a_hw_params(struct snd_pcm_substream *substream,
ret = regmap_update_bits(priv->regmap, PCM1792A_FMT_CONTROL, PCM1792A_FMT_MASK | PCM1792A_ATLD_ENABLE, val); - if (ret < 0) - return ret;
- return 0; + return ret; }
static const struct snd_soc_dai_ops pcm1792a_dai_ops = {
Hi
On Fri, Nov 20, 2015 at 11:09 AM, Raphael Poggi poggi.raph@gmail.com wrote:
Ok, I have seen that almost all the codecs use return regmap_upda...
Please do the same as before
Michael
Hi,
I know that almost all the codec do as before, but what is the goal of using an extra condition when we can avoid it ?
Raphael
2015-11-21 10:31 GMT+01:00 Michael Trimarchi michael@amarulasolutions.com:
Hi
On Sat, Nov 21, 2015 at 05:16:53PM +0100, Raphaël Poggi wrote:
If you use top posting I can not follow. I said that I'm agree on the first one chage and I suggest to do the same for the next one.
Michael
From: Raphael Poggi poggi.raph@gmail.com
Use define for register instead of number, when it is possible. Doing this, make easier to read and use in the code.
Signed-off-by: Raphael Poggi poggi.raph@gmail.com --- sound/soc/codecs/pcm1792a.c | 47 +++++++++++++++++++++++++++------------------ 1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/sound/soc/codecs/pcm1792a.c b/sound/soc/codecs/pcm1792a.c index cf42b24..febaa48 100644 --- a/sound/soc/codecs/pcm1792a.c +++ b/sound/soc/codecs/pcm1792a.c @@ -33,27 +33,36 @@
#include "pcm1792a.h"
-#define PCM1792A_DAC_VOL_LEFT 0x10 -#define PCM1792A_DAC_VOL_RIGHT 0x11 -#define PCM1792A_FMT_CONTROL 0x12 -#define PCM1792A_MODE_CONTROL 0x13 -#define PCM1792A_SOFT_MUTE PCM1792A_FMT_CONTROL - -#define PCM1792A_FMT_MASK 0x70 -#define PCM1792A_FMT_SHIFT 4 -#define PCM1792A_MUTE_MASK 0x01 -#define PCM1792A_MUTE_SHIFT 0 -#define PCM1792A_ATLD_ENABLE (1 << 7) +#define PCM1792A_DAC_VOL_LEFT 0x10 +#define PCM1792A_DAC_VOL_RIGHT 0x11 +#define PCM1792A_FMT_CONTROL 0x12 +#define PCM1792A_MODE_CONTROL 0x13 +#define PCM1792A_ZERO_OUTPUT 0x15 +#define PCM1792A_SOFT_MUTE PCM1792A_FMT_CONTROL +#define PCM1792A_ZERO_FLAG 0x16 +#define PCM1792A_DEVICE_ID 0x17 + +#define PCM1792A_CHANNEL 0x14 +#define PCM1792A_CHANNEL_MONO_MASK 0x8 +#define PCM1792A_CHANNEL_MONO_SHIFT 0x8 +#define PCM1792A_CHANNEL_MONO_LEFT (0x0 << PCM1792A_CHANNEL_MONO_SHIFT) +#define PCM1792A_CHANNEL_MONO_RIGHT (0x1 << PCM1792A_CHANNEL_MONO_SHIFT) + +#define PCM1792A_FMT_MASK 0x70 +#define PCM1792A_FMT_SHIFT 4 +#define PCM1792A_MUTE_MASK 0x01 +#define PCM1792A_MUTE_SHIFT 0 +#define PCM1792A_ATLD_ENABLE (1 << 7)
static const struct reg_default pcm1792a_reg_defaults[] = { - { 0x10, 0xff }, - { 0x11, 0xff }, - { 0x12, 0x50 }, - { 0x13, 0x00 }, - { 0x14, 0x00 }, - { 0x15, 0x01 }, - { 0x16, 0x00 }, - { 0x17, 0x00 }, + { PCM1792A_DAC_VOL_LEFT, 0xff }, + { PCM1792A_DAC_VOL_RIGHT, 0xff }, + { PCM1792A_FMT_CONTROL, 0x50 }, + { PCM1792A_MODE_CONTROL, 0x00 }, + { PCM1792A_CHANNEL, 0x00 }, + { PCM1792A_ZERO_OUTPUT, 0x01 }, + { PCM1792A_ZERO_FLAG, 0x00 }, + { PCM1792A_DEVICE_ID, 0x00 }, };
static bool pcm1792a_accessible_reg(struct device *dev, unsigned int reg)
Hi
On Fri, Nov 20, 2015 at 11:09 AM, Raphael Poggi poggi.raph@gmail.com wrote:
Can you just add what you use? and give an order on those defines?
What is register, what is mask, what is shift?
This function should be update too
Michael
From: Raphael Poggi poggi.raph@gmail.com
Add a binding for specify channel selection.
Signed-off-by: Raphael Poggi poggi.raph@gmail.com --- Documentation/devicetree/bindings/sound/pcm1792a.txt | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/sound/pcm1792a.txt b/Documentation/devicetree/bindings/sound/pcm1792a.txt index 970ba1e..6e87a3a 100644 --- a/Documentation/devicetree/bindings/sound/pcm1792a.txt +++ b/Documentation/devicetree/bindings/sound/pcm1792a.txt @@ -9,10 +9,15 @@ Required properties: For required properties on SPI, please consult Documentation/devicetree/bindings/spi/spi-bus.txt
+Optional properties: +- ti,side: specifie the channel side + insertion, removal. + Examples:
codec_spi: 1792a@0 { compatible = "ti,pcm1792a"; spi-max-frequency = <600000>; + ti,side = <left> };
On Fri, Nov 20, 2015 at 11:09:16AM +0100, Raphael Poggi wrote:
typo
why the line wrap?
I don't understand what this property is/does. Both the name and description are a bit terse.
^ missing ;
Hi Raphael
On Fri, Nov 20, 2015 at 3:42 PM, Rob Herring robh@kernel.org wrote:
can you add me in the patch-set?
Michael
From: Raphael Poggi poggi.raph@gmail.com
Add possibility to choose the channel side using the device tree, and also modify it using alsa ctrl.
Signed-off-by: Raphael Poggi poggi.raph@gmail.com --- sound/soc/codecs/pcm1792a.c | 46 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/sound/soc/codecs/pcm1792a.c b/sound/soc/codecs/pcm1792a.c index febaa48..005b679 100644 --- a/sound/soc/codecs/pcm1792a.c +++ b/sound/soc/codecs/pcm1792a.c @@ -65,6 +65,12 @@ static const struct reg_default pcm1792a_reg_defaults[] = { { PCM1792A_DEVICE_ID, 0x00 }, };
+enum __pcm1792_side { + STEREO, + MONORAL_LEFT, + MONORAL_RIGHT +}; + static bool pcm1792a_accessible_reg(struct device *dev, unsigned int reg) { return reg >= 0x10 && reg <= 0x17; @@ -83,6 +89,7 @@ struct pcm1792a_private { struct regmap *regmap; unsigned int format; unsigned int rate; + unsigned int side; };
static int pcm1792a_set_dai_fmt(struct snd_soc_dai *codec_dai, @@ -153,6 +160,13 @@ static int pcm1792a_hw_params(struct snd_pcm_substream *substream, ret = regmap_update_bits(priv->regmap, PCM1792A_FMT_CONTROL, PCM1792A_FMT_MASK | PCM1792A_ATLD_ENABLE, val);
+ if (priv->side == MONORAL_LEFT) + val = PCM1792A_CHANNEL_MONO_LEFT; + else if (priv->side == MONORAL_RIGHT) + val = PCM1792A_CHANNEL_MONO_RIGHT; + + regmap_update_bits(priv->regmap, PCM1792A_CHANNEL, PCM1792A_CHANNEL_MONO_MASK, val); + return ret; }
@@ -222,10 +236,31 @@ static struct snd_soc_codec_driver soc_codec_dev_pcm1792a = { .num_dapm_routes = ARRAY_SIZE(pcm1792a_dapm_routes), };
+static int pcm1792a_of_init(struct spi_device *spi) +{ + int ret = 0; + struct pcm1792a_private *pcm1792a = dev_get_drvdata(&spi->dev); + const char *name; + struct device_node *np = spi->dev.of_node; + + name = of_get_property(np, "ti,side", NULL); + if (name) { + if (!strcmp(name, "left")) + pcm1792a->side = MONORAL_LEFT; + else + pcm1792a->side = MONORAL_RIGHT; + } + else + pcm1792a->side = STEREO; + + return ret; +} + static int pcm1792a_spi_probe(struct spi_device *spi) { struct pcm1792a_private *pcm1792a; int ret; + struct device_node *np = spi->dev.of_node;
pcm1792a = devm_kzalloc(&spi->dev, sizeof(struct pcm1792a_private), GFP_KERNEL); @@ -241,6 +276,17 @@ static int pcm1792a_spi_probe(struct spi_device *spi) return ret; }
+ if (np) { + ret = pcm1792a_of_init(spi); + if (ret) + return ret; + } + + if (pcm1792a->side == MONORAL_LEFT) + regmap_update_bits(pcm1792a->regmap, PCM1792A_CHANNEL, PCM1792A_CHANNEL_MONO_MASK, PCM1792A_CHANNEL_MONO_LEFT); + else if (pcm1792a->side == MONORAL_RIGHT) + regmap_update_bits(pcm1792a->regmap, PCM1792A_CHANNEL, PCM1792A_CHANNEL_MONO_MASK, PCM1792A_CHANNEL_MONO_RIGHT); + return snd_soc_register_codec(&spi->dev, &soc_codec_dev_pcm1792a, &pcm1792a_dai, 1); }
Hi
On Fri, Nov 20, 2015 at 11:09 AM, Raphael Poggi poggi.raph@gmail.com wrote:
Why this is a part of dts and not a part of mixer control?
Michael
Hi,
2015-11-21 10:37 GMT+01:00 Michael Trimarchi michael@amarulasolutions.com:
The "channel side" bindings should be used when the pcm7192a is used in monaural mode (which requires two DACs, both DACs operate in a balanced mode for one channel of audio input data). This is a hardware level design, it will never change at runtime. I feel that a device tree binding is more appropriate than a control.
Hi
On Tue, Nov 24, 2015 at 9:21 AM, Raphaël Poggi poggi.raph@gmail.com wrote:
Well I know exactly why is used for, but I don't really know if it's somenthing that should be in dts. Do I want to do channel inversion?
Michael
Hi,
2015-11-24 9:27 GMT+01:00 Michael Trimarchi michael@amarulasolutions.com:
I am not sure to understand. You mean "What if I want to do channel inversion", right ? If so, we have two differents "problems", one at hardware level design because monaural mode requires two DACs, and an another one at software level "channel inversion". I have always thought that hardware description should be in dts, that's why I implement monaural mode using dts.
Honestly, I don't know what is the best solution between "this is a hardware design, it should be in dts and shouldn't change" vs "this should be an alsa controler because we want extra features independent from hardware design"
Raphaël
Hi
On Tue, Nov 24, 2015 at 5:04 PM, Raphaël Poggi poggi.raph@gmail.com wrote:
If you put this in mixer control then you can avoid to have it in dts because you not suppose to boot playing music. Basically you can get the same result restore some alsa config that match your topology. Even if you have two mono amplifier you can have the situation that you want to invert channel left and right and this can be done easily just reconfigure one as mono left and the other as mono right or write your own sound card that do in one control, change both together. I suggest to wait some other opinions.
Michael
participants (3)
-
Michael Trimarchi
-
Raphael Poggi
-
Rob Herring