[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bb2bce8b-4d99-1a15-3a34-055ee7637fe2@emlix.com>
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
Powered by blists - more mailing lists