Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-28 Thread Mylène Josserand 
Hello,

On Tue, 27 Feb 2018 22:56:29 +0100
Thomas Petazzoni  wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:
> 
> > diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> > index 795a0657c097..83a2e1508df8 100644
> > --- a/sound/soc/codecs/pcm179x-i2c.c
> > +++ b/sound/soc/codecs/pcm179x-i2c.c
> > @@ -26,10 +26,13 @@
> >  static int pcm179x_i2c_probe(struct i2c_client *client,
> >   const struct i2c_device_id *id)
> >  {
> > -   struct regmap *regmap;
> > +   struct regmap *regmap = NULL;  
> 
> I don't think this change is useful, since regmap is always initialized
> below anyway.

okay.

> 
> 
> > +   if (mute)
> > +   val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);  
> 
> That's not really useful with regmap_update_bits() which already does
> the masking, no?

Yep

> 
> > +   else
> > +   val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> > +   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> > +PCM1789_MUTE_MASK, val);  
> 
> Couldn't this be:
> 
>   if (mute)
>   val = 0;
>   else
>   val = PCM1789_MUTE_MASK;
> 
>   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
>PCM1789_MUTE_MASK, val);
> 

I will update my V2 with it.

> 
> > +static struct snd_soc_dai_driver pcm1789_dai = {
> > +   .name = "pcm1789-hifi",
> > +   .playback = {
> > +   .stream_name = "Playback",
> > +   .channels_min = 2,
> > +   .channels_max = 2,
> > +   .rates = SNDRV_PCM_RATE_CONTINUOUS,
> > +   .rate_min = 1,
> > +   .rate_max = 20,
> > +   .formats = PCM1792A_FORMATS, },  
> 
> Nit: the closing curly brace should be on a separate line.

Yep, thanks.

> 
> 
> > +   if (type == PCM1789)
> > +   return devm_snd_soc_register_component(dev,
> > +  
> > _component_dev_pcm1789,
> > +  _dai, 1);
> > +  
> 
> Perhaps a "else" here ?

Sure

> 
> > return devm_snd_soc_register_component(dev,
> > _component_dev_pcm179x, _dai, 1);  
> 
> Thanks!
> 
> Thomas

Thank you,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-28 Thread Mylène Josserand 
Hello,

On Tue, 27 Feb 2018 22:56:29 +0100
Thomas Petazzoni  wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:
> 
> > diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> > index 795a0657c097..83a2e1508df8 100644
> > --- a/sound/soc/codecs/pcm179x-i2c.c
> > +++ b/sound/soc/codecs/pcm179x-i2c.c
> > @@ -26,10 +26,13 @@
> >  static int pcm179x_i2c_probe(struct i2c_client *client,
> >   const struct i2c_device_id *id)
> >  {
> > -   struct regmap *regmap;
> > +   struct regmap *regmap = NULL;  
> 
> I don't think this change is useful, since regmap is always initialized
> below anyway.

okay.

> 
> 
> > +   if (mute)
> > +   val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);  
> 
> That's not really useful with regmap_update_bits() which already does
> the masking, no?

Yep

> 
> > +   else
> > +   val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> > +   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> > +PCM1789_MUTE_MASK, val);  
> 
> Couldn't this be:
> 
>   if (mute)
>   val = 0;
>   else
>   val = PCM1789_MUTE_MASK;
> 
>   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
>PCM1789_MUTE_MASK, val);
> 

I will update my V2 with it.

> 
> > +static struct snd_soc_dai_driver pcm1789_dai = {
> > +   .name = "pcm1789-hifi",
> > +   .playback = {
> > +   .stream_name = "Playback",
> > +   .channels_min = 2,
> > +   .channels_max = 2,
> > +   .rates = SNDRV_PCM_RATE_CONTINUOUS,
> > +   .rate_min = 1,
> > +   .rate_max = 20,
> > +   .formats = PCM1792A_FORMATS, },  
> 
> Nit: the closing curly brace should be on a separate line.

Yep, thanks.

> 
> 
> > +   if (type == PCM1789)
> > +   return devm_snd_soc_register_component(dev,
> > +  
> > _component_dev_pcm1789,
> > +  _dai, 1);
> > +  
> 
> Perhaps a "else" here ?

Sure

> 
> > return devm_snd_soc_register_component(dev,
> > _component_dev_pcm179x, _dai, 1);  
> 
> Thanks!
> 
> Thomas

Thank you,

Mylène

-- 
Mylène Josserand, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-27 Thread Thomas Petazzoni 
Hello,

On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:

> diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> index 795a0657c097..83a2e1508df8 100644
> --- a/sound/soc/codecs/pcm179x-i2c.c
> +++ b/sound/soc/codecs/pcm179x-i2c.c
> @@ -26,10 +26,13 @@
>  static int pcm179x_i2c_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
>  {
> - struct regmap *regmap;
> + struct regmap *regmap = NULL;

I don't think this change is useful, since regmap is always initialized
below anyway.


> + if (mute)
> + val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);

That's not really useful with regmap_update_bits() which already does
the masking, no?

> + else
> + val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> + ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> +  PCM1789_MUTE_MASK, val);

Couldn't this be:

if (mute)
val = 0;
else
val = PCM1789_MUTE_MASK;

ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
 PCM1789_MUTE_MASK, val);


> +static struct snd_soc_dai_driver pcm1789_dai = {
> + .name = "pcm1789-hifi",
> + .playback = {
> + .stream_name = "Playback",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_CONTINUOUS,
> + .rate_min = 1,
> + .rate_max = 20,
> + .formats = PCM1792A_FORMATS, },

Nit: the closing curly brace should be on a separate line.


> + if (type == PCM1789)
> + return devm_snd_soc_register_component(dev,
> +
> _component_dev_pcm1789,
> +_dai, 1);
> +

Perhaps a "else" here ?

>   return devm_snd_soc_register_component(dev,
>   _component_dev_pcm179x, _dai, 1);

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-27 Thread Thomas Petazzoni 
Hello,

On Tue, 27 Feb 2018 22:24:31 +0100, Mylène Josserand wrote:

> diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
> index 795a0657c097..83a2e1508df8 100644
> --- a/sound/soc/codecs/pcm179x-i2c.c
> +++ b/sound/soc/codecs/pcm179x-i2c.c
> @@ -26,10 +26,13 @@
>  static int pcm179x_i2c_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
>  {
> - struct regmap *regmap;
> + struct regmap *regmap = NULL;

I don't think this change is useful, since regmap is always initialized
below anyway.


> + if (mute)
> + val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);

That's not really useful with regmap_update_bits() which already does
the masking, no?

> + else
> + val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
> + ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
> +  PCM1789_MUTE_MASK, val);

Couldn't this be:

if (mute)
val = 0;
else
val = PCM1789_MUTE_MASK;

ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
 PCM1789_MUTE_MASK, val);


> +static struct snd_soc_dai_driver pcm1789_dai = {
> + .name = "pcm1789-hifi",
> + .playback = {
> + .stream_name = "Playback",
> + .channels_min = 2,
> + .channels_max = 2,
> + .rates = SNDRV_PCM_RATE_CONTINUOUS,
> + .rate_min = 1,
> + .rate_max = 20,
> + .formats = PCM1792A_FORMATS, },

Nit: the closing curly brace should be on a separate line.


> + if (type == PCM1789)
> + return devm_snd_soc_register_component(dev,
> +
> _component_dev_pcm1789,
> +_dai, 1);
> +

Perhaps a "else" here ?

>   return devm_snd_soc_register_component(dev,
>   _component_dev_pcm179x, _dai, 1);

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-27 Thread Mylène Josserand 
Add PCM1789 DAC support into pcm179x file.
This DAC is pretty much the same than PCM179x but some
registers are differents (such as mute registers split in
right/left).

One particularity about this DAC is that the clocks must be
always enabled. Also, an entire software reset is necessary
while starting to play a sound otherwise, the clocks are not
synchronized (so the DAC is not able to send data).

Signed-off-by: Mylène Josserand 
---
 sound/soc/codecs/pcm179x-i2c.c |   7 +-
 sound/soc/codecs/pcm179x.c | 164 +
 sound/soc/codecs/pcm179x.h |   1 +
 3 files changed, 170 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 795a0657c097..83a2e1508df8 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -26,10 +26,13 @@
 static int pcm179x_i2c_probe(struct i2c_client *client,
  const struct i2c_device_id *id)
 {
-   struct regmap *regmap;
+   struct regmap *regmap = NULL;
int ret;
 
-   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   if (id->driver_data == PCM1789)
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   else
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(regmap)) {
ret = PTR_ERR(regmap);
dev_err(>dev, "Failed to allocate regmap: %d\n", ret);
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 81cbf09319f6..2285a51ff9e9 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -43,6 +43,17 @@
 #define PCM179X_MUTE_SHIFT 0
 #define PCM179X_ATLD_ENABLE(1 << 7)
 
+#define PCM1789_FMT_CONTROL0x11
+#define PCM1789_FLT_CONTROL0x12
+#define PCM1789_REV_CONTROL0x13
+#define PCM1789_SOFT_MUTE  0x14
+#define PCM1789_DAC_VOL_LEFT   0x18
+#define PCM1789_DAC_VOL_RIGHT  0x19
+#define PCM1789_FMT_MASK   0x07
+#define PCM1789_MUTE_MASK  0x03
+#define PCM1789_MUTE_L_EN  BIT(0)
+#define PCM1789_MUTE_R_EN  BIT(1)
+
 static const struct reg_default pcm179x_reg_defaults[] = {
{ 0x10, 0xff },
{ 0x11, 0xff },
@@ -54,11 +65,25 @@ static const struct reg_default pcm179x_reg_defaults[] = {
{ 0x17, 0x00 },
 };
 
+static const struct reg_default pcm1789_reg_defaults[] = {
+   { PCM1789_FMT_CONTROL, 0x00 },
+   { PCM1789_FLT_CONTROL, 0x00 },
+   { PCM1789_REV_CONTROL, 0x00 },
+   { PCM1789_SOFT_MUTE, 0x00 },
+   { PCM1789_DAC_VOL_LEFT, 0xff },
+   { PCM1789_DAC_VOL_RIGHT, 0xff },
+};
+
 static bool pcm179x_accessible_reg(struct device *dev, unsigned int reg)
 {
return reg >= 0x10 && reg <= 0x17;
 }
 
+static bool pcm1789_accessible_reg(struct device *dev, unsigned int reg)
+{
+   return reg >= PCM1789_FMT_CONTROL && reg <= PCM1789_DAC_VOL_RIGHT;
+}
+
 static bool pcm179x_writeable_reg(struct device *dev, unsigned int reg)
 {
bool accessible;
@@ -68,6 +93,15 @@ static bool pcm179x_writeable_reg(struct device *dev, 
unsigned int reg)
return accessible && reg != 0x16 && reg != 0x17;
 }
 
+static bool pcm1789_writeable_reg(struct device *dev, unsigned int reg)
+{
+   bool accessible;
+
+   accessible = pcm1789_accessible_reg(dev, reg);
+
+   return accessible && reg != 0x16 && reg != 0x17;
+}
+
 struct pcm179x_private {
struct regmap *regmap;
unsigned int format;
@@ -99,6 +133,24 @@ static int pcm179x_digital_mute(struct snd_soc_dai *dai, 
int mute)
return 0;
 }
 
+static int pcm1789_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+   struct snd_soc_component *component = dai->component;
+   struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+   int ret, val;
+
+   if (mute)
+   val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);
+   else
+   val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
+   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
+PCM1789_MUTE_MASK, val);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
 static int pcm179x_hw_params(struct snd_pcm_substream *substream,
 struct snd_pcm_hw_params *params,
 struct snd_soc_dai *dai)
@@ -151,12 +203,76 @@ static int pcm179x_hw_params(struct snd_pcm_substream 
*substream,
return 0;
 }
 
+static int pcm1789_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params,
+struct snd_soc_dai *dai)
+{
+   struct snd_soc_component *component = dai->component;
+   struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+   int val = 0, ret;
+
+   priv->rate = params_rate(params);
+
+   switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+   case

[PATCH v1 2/4] ASoC: codecs: pcm179x: Add support for PCM1789

2018-02-27 Thread Mylène Josserand 
Add PCM1789 DAC support into pcm179x file.
This DAC is pretty much the same than PCM179x but some
registers are differents (such as mute registers split in
right/left).

One particularity about this DAC is that the clocks must be
always enabled. Also, an entire software reset is necessary
while starting to play a sound otherwise, the clocks are not
synchronized (so the DAC is not able to send data).

Signed-off-by: Mylène Josserand 
---
 sound/soc/codecs/pcm179x-i2c.c |   7 +-
 sound/soc/codecs/pcm179x.c | 164 +
 sound/soc/codecs/pcm179x.h |   1 +
 3 files changed, 170 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm179x-i2c.c b/sound/soc/codecs/pcm179x-i2c.c
index 795a0657c097..83a2e1508df8 100644
--- a/sound/soc/codecs/pcm179x-i2c.c
+++ b/sound/soc/codecs/pcm179x-i2c.c
@@ -26,10 +26,13 @@
 static int pcm179x_i2c_probe(struct i2c_client *client,
  const struct i2c_device_id *id)
 {
-   struct regmap *regmap;
+   struct regmap *regmap = NULL;
int ret;
 
-   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   if (id->driver_data == PCM1789)
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
+   else
+   regmap = devm_regmap_init_i2c(client, _regmap_config);
if (IS_ERR(regmap)) {
ret = PTR_ERR(regmap);
dev_err(>dev, "Failed to allocate regmap: %d\n", ret);
diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 81cbf09319f6..2285a51ff9e9 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -43,6 +43,17 @@
 #define PCM179X_MUTE_SHIFT 0
 #define PCM179X_ATLD_ENABLE(1 << 7)
 
+#define PCM1789_FMT_CONTROL0x11
+#define PCM1789_FLT_CONTROL0x12
+#define PCM1789_REV_CONTROL0x13
+#define PCM1789_SOFT_MUTE  0x14
+#define PCM1789_DAC_VOL_LEFT   0x18
+#define PCM1789_DAC_VOL_RIGHT  0x19
+#define PCM1789_FMT_MASK   0x07
+#define PCM1789_MUTE_MASK  0x03
+#define PCM1789_MUTE_L_EN  BIT(0)
+#define PCM1789_MUTE_R_EN  BIT(1)
+
 static const struct reg_default pcm179x_reg_defaults[] = {
{ 0x10, 0xff },
{ 0x11, 0xff },
@@ -54,11 +65,25 @@ static const struct reg_default pcm179x_reg_defaults[] = {
{ 0x17, 0x00 },
 };
 
+static const struct reg_default pcm1789_reg_defaults[] = {
+   { PCM1789_FMT_CONTROL, 0x00 },
+   { PCM1789_FLT_CONTROL, 0x00 },
+   { PCM1789_REV_CONTROL, 0x00 },
+   { PCM1789_SOFT_MUTE, 0x00 },
+   { PCM1789_DAC_VOL_LEFT, 0xff },
+   { PCM1789_DAC_VOL_RIGHT, 0xff },
+};
+
 static bool pcm179x_accessible_reg(struct device *dev, unsigned int reg)
 {
return reg >= 0x10 && reg <= 0x17;
 }
 
+static bool pcm1789_accessible_reg(struct device *dev, unsigned int reg)
+{
+   return reg >= PCM1789_FMT_CONTROL && reg <= PCM1789_DAC_VOL_RIGHT;
+}
+
 static bool pcm179x_writeable_reg(struct device *dev, unsigned int reg)
 {
bool accessible;
@@ -68,6 +93,15 @@ static bool pcm179x_writeable_reg(struct device *dev, 
unsigned int reg)
return accessible && reg != 0x16 && reg != 0x17;
 }
 
+static bool pcm1789_writeable_reg(struct device *dev, unsigned int reg)
+{
+   bool accessible;
+
+   accessible = pcm1789_accessible_reg(dev, reg);
+
+   return accessible && reg != 0x16 && reg != 0x17;
+}
+
 struct pcm179x_private {
struct regmap *regmap;
unsigned int format;
@@ -99,6 +133,24 @@ static int pcm179x_digital_mute(struct snd_soc_dai *dai, 
int mute)
return 0;
 }
 
+static int pcm1789_digital_mute(struct snd_soc_dai *dai, int mute)
+{
+   struct snd_soc_component *component = dai->component;
+   struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+   int ret, val;
+
+   if (mute)
+   val = ~(PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN);
+   else
+   val = PCM1789_MUTE_L_EN | PCM1789_MUTE_R_EN;
+   ret = regmap_update_bits(priv->regmap, PCM1789_SOFT_MUTE,
+PCM1789_MUTE_MASK, val);
+   if (ret < 0)
+   return ret;
+
+   return 0;
+}
+
 static int pcm179x_hw_params(struct snd_pcm_substream *substream,
 struct snd_pcm_hw_params *params,
 struct snd_soc_dai *dai)
@@ -151,12 +203,76 @@ static int pcm179x_hw_params(struct snd_pcm_substream 
*substream,
return 0;
 }
 
+static int pcm1789_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params,
+struct snd_soc_dai *dai)
+{
+   struct snd_soc_component *component = dai->component;
+   struct pcm179x_private *priv = snd_soc_component_get_drvdata(component);
+   int val = 0, ret;
+
+   priv->rate = params_rate(params);
+
+   switch (priv->format & SND_SOC_DAIFMT_FORMAT_MASK) {
+   case SND_SOC_DAIFMT_RIGHT_J:
+