Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello Fabio,

Thank you for the review!

On Tue, 27 Feb 2018 19:30:11 -0300
Fabio Estevam  wrote:

> On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
>  wrote:
> 
> > +   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> > +   if (gpio_is_valid(pcm179x->reset)) {
> > +   ret = devm_gpio_request_one(dev, pcm179x->reset,
> > +   GPIOF_OUT_INIT_LOW,
> > +   "pcm179x reset");
> > +   if (ret) {
> > +   dev_err(dev,
> > +   "Failed to request GPIO %d as reset pin, 
> > error %d\n",
> > +   pcm179x->reset, ret);
> > +   return ret;
> > +   }
> > +
> > +   gpio_set_value(pcm179x->reset, 1);  
> 
> It would be better to use the gpiod API, which takes the GPIO polarity
> into account.
> 
> There may be systems that have an inverter connected to this pin, and
> this can be changed in dts via GPIO_ACTIVE_HIGH.

Oh, true, I should have used gpiod. Thanks for pointing me out.

> 
> Also, as the reset pin can be connected to an I2C expander, for
> example, so it is safer to use the cansleep variant:
> 
> gpiod_set_value_cansleep(pcm179x->reset, 0);

Sure, I will update it.

Thanks,

Mylène

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

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello Fabio,

Thank you for the review!

On Tue, 27 Feb 2018 19:30:11 -0300
Fabio Estevam  wrote:

> On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
>  wrote:
> 
> > +   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> > +   if (gpio_is_valid(pcm179x->reset)) {
> > +   ret = devm_gpio_request_one(dev, pcm179x->reset,
> > +   GPIOF_OUT_INIT_LOW,
> > +   "pcm179x reset");
> > +   if (ret) {
> > +   dev_err(dev,
> > +   "Failed to request GPIO %d as reset pin, 
> > error %d\n",
> > +   pcm179x->reset, ret);
> > +   return ret;
> > +   }
> > +
> > +   gpio_set_value(pcm179x->reset, 1);  
> 
> It would be better to use the gpiod API, which takes the GPIO polarity
> into account.
> 
> There may be systems that have an inverter connected to this pin, and
> this can be changed in dts via GPIO_ACTIVE_HIGH.

Oh, true, I should have used gpiod. Thanks for pointing me out.

> 
> Also, as the reset pin can be connected to an I2C expander, for
> example, so it is safer to use the cansleep variant:
> 
> gpiod_set_value_cansleep(pcm179x->reset, 0);

Sure, I will update it.

Thanks,

Mylène

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

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello,

On Tue, 27 Feb 2018 23:02:00 +0100
Thomas Petazzoni  wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> > Add a reset gpio to be able to reset this line at startup.
> > 
> > Signed-off-by: Mylène Josserand   
> 
> This needs an updated of the DT binding, since it's adding support for
> a new reset-gpios property.

True, I will add it in my new series.

> 
> Thanks!
> 
> Thomas

Thanks,

Mylène

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

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Hello,

On Tue, 27 Feb 2018 23:02:00 +0100
Thomas Petazzoni  wrote:

> Hello,
> 
> On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> > Add a reset gpio to be able to reset this line at startup.
> > 
> > Signed-off-by: Mylène Josserand   
> 
> This needs an updated of the DT binding, since it's adding support for
> a new reset-gpios property.

True, I will add it in my new series.

> 
> Thanks!
> 
> Thomas

Thanks,

Mylène

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

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Fabio Estevam]

On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
 wrote:

> +   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> +   if (gpio_is_valid(pcm179x->reset)) {
> +   ret = devm_gpio_request_one(dev, pcm179x->reset,
> +   GPIOF_OUT_INIT_LOW,
> +   "pcm179x reset");
> +   if (ret) {
> +   dev_err(dev,
> +   "Failed to request GPIO %d as reset pin, 
> error %d\n",
> +   pcm179x->reset, ret);
> +   return ret;
> +   }
> +
> +   gpio_set_value(pcm179x->reset, 1);

It would be better to use the gpiod API, which takes the GPIO polarity
into account.

There may be systems that have an inverter connected to this pin, and
this can be changed in dts via GPIO_ACTIVE_HIGH.

Also, as the reset pin can be connected to an I2C expander, for
example, so it is safer to use the cansleep variant:

gpiod_set_value_cansleep(pcm179x->reset, 0);

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Fabio Estevam]

On Tue, Feb 27, 2018 at 6:24 PM, Mylène Josserand
 wrote:

> +   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
> +   if (gpio_is_valid(pcm179x->reset)) {
> +   ret = devm_gpio_request_one(dev, pcm179x->reset,
> +   GPIOF_OUT_INIT_LOW,
> +   "pcm179x reset");
> +   if (ret) {
> +   dev_err(dev,
> +   "Failed to request GPIO %d as reset pin, 
> error %d\n",
> +   pcm179x->reset, ret);
> +   return ret;
> +   }
> +
> +   gpio_set_value(pcm179x->reset, 1);

It would be better to use the gpiod API, which takes the GPIO polarity
into account.

There may be systems that have an inverter connected to this pin, and
this can be changed in dts via GPIO_ACTIVE_HIGH.

Also, as the reset pin can be connected to an I2C expander, for
example, so it is safer to use the cansleep variant:

gpiod_set_value_cansleep(pcm179x->reset, 0);

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> Add a reset gpio to be able to reset this line at startup.
> 
> Signed-off-by: Mylène Josserand 

This needs an updated of the DT binding, since it's adding support for
a new reset-gpios property.

Thanks!

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

Re: [PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Thomas Petazzoni]

Hello,

On Tue, 27 Feb 2018 22:24:32 +0100, Mylène Josserand wrote:
> Add a reset gpio to be able to reset this line at startup.
> 
> Signed-off-by: Mylène Josserand 

This needs an updated of the DT binding, since it's adding support for
a new reset-gpios property.

Thanks!

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

[PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Add a reset gpio to be able to reset this line at startup.

Signed-off-by: Mylène Josserand 
---
 sound/soc/codecs/pcm179x.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 2285a51ff9e9..0242dfd67b53 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -22,6 +22,8 @@
 #include 
 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -106,6 +108,7 @@ struct pcm179x_private {
struct regmap *regmap;
unsigned int format;
unsigned int rate;
+   int reset;
 };
 
 static int pcm179x_set_dai_fmt(struct snd_soc_dai *codec_dai,
@@ -381,6 +384,8 @@ int pcm179x_common_init(struct device *dev, struct regmap 
*regmap,
enum pcm17xx_type type)
 {
struct pcm179x_private *pcm179x;
+   struct device_node *np = dev->of_node;
+   int ret;
 
pcm179x = devm_kzalloc(dev, sizeof(struct pcm179x_private),
GFP_KERNEL);
@@ -390,6 +395,21 @@ int pcm179x_common_init(struct device *dev, struct regmap 
*regmap,
pcm179x->regmap = regmap;
dev_set_drvdata(dev, pcm179x);
 
+   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
+   if (gpio_is_valid(pcm179x->reset)) {
+   ret = devm_gpio_request_one(dev, pcm179x->reset,
+   GPIOF_OUT_INIT_LOW,
+   "pcm179x reset");
+   if (ret) {
+   dev_err(dev,
+   "Failed to request GPIO %d as reset pin, error 
%d\n",
+   pcm179x->reset, ret);
+   return ret;
+   }
+
+   gpio_set_value(pcm179x->reset, 1);
+   }
+
if (type == PCM1789)
return devm_snd_soc_register_component(dev,
   
_component_dev_pcm1789,
-- 
2.11.0

[PATCH v1 3/4] ASoC: codecs: pcm179x: Add reset gpio

[Mylène Josserand]

Add a reset gpio to be able to reset this line at startup.

Signed-off-by: Mylène Josserand 
---
 sound/soc/codecs/pcm179x.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/sound/soc/codecs/pcm179x.c b/sound/soc/codecs/pcm179x.c
index 2285a51ff9e9..0242dfd67b53 100644
--- a/sound/soc/codecs/pcm179x.c
+++ b/sound/soc/codecs/pcm179x.c
@@ -22,6 +22,8 @@
 #include 
 
 #include 
+#include 
+#include 
 #include 
 #include 
 #include 
@@ -106,6 +108,7 @@ struct pcm179x_private {
struct regmap *regmap;
unsigned int format;
unsigned int rate;
+   int reset;
 };
 
 static int pcm179x_set_dai_fmt(struct snd_soc_dai *codec_dai,
@@ -381,6 +384,8 @@ int pcm179x_common_init(struct device *dev, struct regmap 
*regmap,
enum pcm17xx_type type)
 {
struct pcm179x_private *pcm179x;
+   struct device_node *np = dev->of_node;
+   int ret;
 
pcm179x = devm_kzalloc(dev, sizeof(struct pcm179x_private),
GFP_KERNEL);
@@ -390,6 +395,21 @@ int pcm179x_common_init(struct device *dev, struct regmap 
*regmap,
pcm179x->regmap = regmap;
dev_set_drvdata(dev, pcm179x);
 
+   pcm179x->reset = of_get_named_gpio(np, "reset-gpios", 0);
+   if (gpio_is_valid(pcm179x->reset)) {
+   ret = devm_gpio_request_one(dev, pcm179x->reset,
+   GPIOF_OUT_INIT_LOW,
+   "pcm179x reset");
+   if (ret) {
+   dev_err(dev,
+   "Failed to request GPIO %d as reset pin, error 
%d\n",
+   pcm179x->reset, ret);
+   return ret;
+   }
+
+   gpio_set_value(pcm179x->reset, 1);
+   }
+
if (type == PCM1789)
return devm_snd_soc_register_component(dev,
   
_component_dev_pcm1789,
-- 
2.11.0