linux-kernel - Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 3 May 2023 10:37:45 +0200
From:   Edmund Berenson <edmund.berenson@...ix.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Michael Walle <michael@...le.cc>
Cc:     Lukasz Zemla <Lukasz.Zemla@...dward.com>,
        Bartosz Golaszewski <brgl@...ev.pl>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] gpio: max7317: Add gpio expander driver

On 4/4/23 16:05, Linus Walleij wrote:
> On Mon, Apr 3, 2023 at 1:41 PM Edmund Berenson
> <edmund.berenson@...ix.com> wrote:
> 
>> Add driver for maxim MAX7317 SPI-Interfaced 10 Port
>> GPIO Expander.
>>
>> v2: adjust driver to use regmap
>>
>> Co-developed-by: Lukasz Zemla <Lukasz.Zemla@...dward.com>
>> Signed-off-by: Lukasz Zemla <Lukasz.Zemla@...dward.com>
>> Signed-off-by: Edmund Berenson <edmund.berenson@...ix.com>
> 
> Notwithstanding the other comments from Bartosz, this seems like
> a driver that should be using the regmap GPIO helper library.
> git grep GPIO_REGMAP will show you examples of other drivers
> that use this and how it is used.
> 
> Yours,
> Linus Walleij

Hi,

thanks for the review and suggestion. I tried following your suggestion and use
GPIO_REGMAP to implement the driver.

Unfortunately I ran into two issues
1. reg_set_base == 0: for the devcie reg_set base is 0x0. In gpio-regmap there
are several tests for !reg_set_base. There doesn't seem a way to distinguish
between is set to 0 and is not set. :)

2. input/output direction: to set a gpio pin to input one has to write 0x1 to
the corresponding output register. The issue starts when I configure a port to
be an output, set output to 0x1, check the direction of the pin, doing so trough
sysfs the system will now assume the pin is an input and I can't set its values
anymore. Avoiding this I would like to track the direction of the pin separately
from the device register, which is atm done in the corresponding bespoke in/out
functions.

I could probably solve both of these issues trough the reg_mask_xlate function
but I believe this would introduce unneeded obscurity in the driver.

I do not believe there are any other easy obvious/better fixes for this. (or
maybe you prove me wrong :))
Would you be okay for this driver to stick with direct regmap usage? (obviously
fixing the review suggestions)

BR
Edmund

-- 
Edmund Berenson, emlix GmbH, http://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source