Correct a typo which inverted the reset GPIO pin sequence.

[PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Brad Kelley 5 days, 3 hours ago

commit that had typo: 42d1178d223ba9498c1ed40a5fc243a43d35ec97 ASoC: cs4271: Convert to GPIO descriptors

The original commit changed a 1 to a 0 and a 0 to a 1. This inverted the reset sequence
resulting in the IC powering down and not initializing.  Correcting that allows the board to initialized.

Signed-off-by: Brad Kelley <spambake@bradkelley.org>
---
 sound/soc/codecs/cs4271.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
index 77dfc83a3c01..348f15c36d10 100644
--- a/sound/soc/codecs/cs4271.c
+++ b/sound/soc/codecs/cs4271.c
@@ -486,9 +486,9 @@ static int cs4271_reset(struct snd_soc_component *component)
 {
 	struct cs4271_private *cs4271 = snd_soc_component_get_drvdata(component);
 
-	gpiod_direction_output(cs4271->reset, 1);
+	gpiod_direction_output(cs4271->reset, 0);
 	mdelay(1);
-	gpiod_set_value(cs4271->reset, 0);
+	gpiod_set_value(cs4271->reset, 1);
 	mdelay(1);
 
 	return 0;
-- 
2.43.0

Re: [PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Charles Keepax 4 days, 23 hours ago

On Wed, Dec 10, 2025 at 09:16:28PM -0800, Brad Kelley wrote:
> commit that had typo: 42d1178d223ba9498c1ed40a5fc243a43d35ec97 ASoC: cs4271: Convert to GPIO descriptors
> 

This commit adds a GPIO_ACTIVE_LOW flag onto the GPIO lookups
which should cause the GPIO core to invert the sense of the
gpiod_set_value calls. Is your use-case using one of the in
kernel lookups or your own one? If it is your own one do you need
to add a GPIO_ACTIVE_LOW to that?

> The original commit changed a 1 to a 0 and a 0 to a 1. This inverted the reset sequence
> resulting in the IC powering down and not initializing.  Correcting that allows the board to initialized.
> 
> Signed-off-by: Brad Kelley <spambake@bradkelley.org>
> ---
>  sound/soc/codecs/cs4271.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
> index 77dfc83a3c01..348f15c36d10 100644
> --- a/sound/soc/codecs/cs4271.c
> +++ b/sound/soc/codecs/cs4271.c
> @@ -486,9 +486,9 @@ static int cs4271_reset(struct snd_soc_component *component)
>  {
>  	struct cs4271_private *cs4271 = snd_soc_component_get_drvdata(component);
>  
> -	gpiod_direction_output(cs4271->reset, 1);
> +	gpiod_direction_output(cs4271->reset, 0);

This does look like a bug however, the GPIO should clearly still be
an output. So keep this bit. Also could you change that commit in
the commit message to a Fixes tag:

Fixes: 42d1178d223b ("ASoC: cs4271: Convert to GPIO descriptors")

>  	mdelay(1);
> -	gpiod_set_value(cs4271->reset, 0);
> +	gpiod_set_value(cs4271->reset, 1);
>  	mdelay(1);
>  
>  	return 0;
> -- 
> 2.43.0
> 

Thanks,
Charles

Re: [PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Richard Fitzgerald 4 days, 22 hours ago

On 11/12/2025 9:39 am, Charles Keepax wrote:
> On Wed, Dec 10, 2025 at 09:16:28PM -0800, Brad Kelley wrote:
>> commit that had typo: 42d1178d223ba9498c1ed40a5fc243a43d35ec97 ASoC: cs4271: Convert to GPIO descriptors
>>
> 
> This commit adds a GPIO_ACTIVE_LOW flag onto the GPIO lookups
> which should cause the GPIO core to invert the sense of the
> gpiod_set_value calls. Is your use-case using one of the in
> kernel lookups or your own one? If it is your own one do you need
> to add a GPIO_ACTIVE_LOW to that?
> 
>> The original commit changed a 1 to a 0 and a 0 to a 1. This inverted the reset sequence
>> resulting in the IC powering down and not initializing.  Correcting that allows the board to initialized.
>>
>> Signed-off-by: Brad Kelley <spambake@bradkelley.org>
>> ---
>>   sound/soc/codecs/cs4271.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/codecs/cs4271.c b/sound/soc/codecs/cs4271.c
>> index 77dfc83a3c01..348f15c36d10 100644
>> --- a/sound/soc/codecs/cs4271.c
>> +++ b/sound/soc/codecs/cs4271.c
>> @@ -486,9 +486,9 @@ static int cs4271_reset(struct snd_soc_component *component)
>>   {
>>   	struct cs4271_private *cs4271 = snd_soc_component_get_drvdata(component);
>>   
>> -	gpiod_direction_output(cs4271->reset, 1);
>> +	gpiod_direction_output(cs4271->reset, 0);
> 
> This does look like a bug however, the GPIO should clearly still be
> an output. So keep this bit. Also could you change that commit in
> the commit message to a Fixes tag:
>
Actually this patch looks incorrect and will break things.
The 2nd argument to gpiod_direction_output() is the initial state of
the GPIO. The kerneldoc for the function says "The initial value of the
output must be specified as the logical value of the GPIO"

So the original code is correct: first we assert it (logical state 1)
then below it is deasserted (logical state 0).

The problem is that originally the code set the raw signal level
(0 to reset, 1 to not-reset) but now that it uses gpiod you must
add the ACTIVE_LOW flag to the gpio definition if its electrical
signal level is inverse of its logical level.

See the code in gpiod_direction_output_nonotify() in
drivers/gpio/gpiolib.c, which inverts the value if FLAG_ACTIVE_LOW
is set.

> Fixes: 42d1178d223b ("ASoC: cs4271: Convert to GPIO descriptors")
> 
>>   	mdelay(1);
>> -	gpiod_set_value(cs4271->reset, 0);
>> +	gpiod_set_value(cs4271->reset, 1);
>>   	mdelay(1);
>>   
>>   	return 0;
>> -- 
>> 2.43.0
>>
> 
> Thanks,
> Charles

Re: [PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Brad Kelley 3 days, 17 hours ago

On 12/11/2025 2:19 AM, Richard Fitzgerald wrote:
> Actually this patch looks incorrect and will break things.
> The 2nd argument to gpiod_direction_output() is the initial state of
> the GPIO. The kerneldoc for the function says "The initial value of the
> output must be specified as the logical value of the GPIO"
> 
> So the original code is correct: first we assert it (logical state 1)
> then below it is deasserted (logical state 0).
> 
> The problem is that originally the code set the raw signal level
> (0 to reset, 1 to not-reset) but now that it uses gpiod you must
> add the ACTIVE_LOW flag to the gpio definition if its electrical
> signal level is inverse of its logical level.
> 
> See the code in gpiod_direction_output_nonotify() in
> drivers/gpio/gpiolib.c, which inverts the value if FLAG_ACTIVE_LOW
> is set.

Thanks to both of you for the explanations and patience.  That all 
makes sense and I appreciate the guidance.  

This my first patch. I think I need to send a withdrawal email.

The problem for my use seems to be in the superaudioboard overlay 
which sets the GPIO_ACTIVE_HIGH flag.  A quick compile of the 
edited dts file fixes the problem.  I changed the line to this:
  reset-gpio = <&gpio 26 1>; /* Pin 26, active low */
but maybe there's an updated method.

I'll do some more research and testing and submit a new patch to 
the proper maintainers for that overlay.

Thanks again,

Brad

Re: [PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Charles Keepax 3 days, 16 hours ago

On Fri, Dec 12, 2025 at 07:44:21AM -0800, Brad Kelley wrote:
> On 12/11/2025 2:19 AM, Richard Fitzgerald wrote:
> > Actually this patch looks incorrect and will break things.
> > The 2nd argument to gpiod_direction_output() is the initial state of
> > the GPIO. The kerneldoc for the function says "The initial value of the
> > output must be specified as the logical value of the GPIO"
> > 
> > So the original code is correct: first we assert it (logical state 1)
> > then below it is deasserted (logical state 0).
> > 
> > The problem is that originally the code set the raw signal level
> > (0 to reset, 1 to not-reset) but now that it uses gpiod you must
> > add the ACTIVE_LOW flag to the gpio definition if its electrical
> > signal level is inverse of its logical level.
> > 
> > See the code in gpiod_direction_output_nonotify() in
> > drivers/gpio/gpiolib.c, which inverts the value if FLAG_ACTIVE_LOW
> > is set.
> 
> Thanks to both of you for the explanations and patience.  That all 
> makes sense and I appreciate the guidance.  

Absolutely no problem, welcome to the kernel community.

> This my first patch. I think I need to send a withdrawal email.

No need for any separate withdrawal.

> The problem for my use seems to be in the superaudioboard overlay 
> which sets the GPIO_ACTIVE_HIGH flag.  A quick compile of the 
> edited dts file fixes the problem.  I changed the line to this:
>   reset-gpio = <&gpio 26 1>; /* Pin 26, active low */
> but maybe there's an updated method.

Suspected that might be the problem, glad we could help you
figuring it out.

> I'll do some more research and testing and submit a new patch to 
> the proper maintainers for that overlay.

Aye that makes sense, if the DT is in the kernel the process
should be pretty similar just as you say finding the right
maintainers.

Thanks,
Charles

Re: [PATCH] Correct a typo which inverted the reset GPIO pin sequence.

Posted by Charles Keepax 4 days, 22 hours ago

On Thu, Dec 11, 2025 at 09:39:41AM +0000, Charles Keepax wrote:
> On Wed, Dec 10, 2025 at 09:16:28PM -0800, Brad Kelley wrote:
> > -	gpiod_direction_output(cs4271->reset, 1);
> > +	gpiod_direction_output(cs4271->reset, 0);
> 
> This does look like a bug however, the GPIO should clearly still be
> an output. So keep this bit. Also could you change that commit in
> the commit message to a Fixes tag:
> 
> Fixes: 42d1178d223b ("ASoC: cs4271: Convert to GPIO descriptors")
> 

Apologies must still need my morning coffee this is the initial
output value, not the direction. So should also be covered by the
GPIO_ACTIVE_LOW flag.

Thanks,
Charles