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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9bcc59d6-027a-4b66-af81-2f69c84c3efd@opensource.cirrus.com>
Date: Thu, 11 Dec 2025 10:19:49 +0000
From: Richard Fitzgerald <rf@...nsource.cirrus.com>
To: Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Brad Kelley <spambake@...dkelley.org>
Cc: david.rhodes@...rus.com, lgirdwood@...il.com, broonie@...nel.org,
        perex@...ex.cz, tiwai@...e.com, linux-sound@...r.kernel.org,
        patches@...nsource.cirrus.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Correct a typo which inverted the reset GPIO pin
 sequence.

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@...dkelley.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