linux-kernel - Re: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

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]

Message-ID: <25e1fda4b6df2d10444d7eca3cd0e387@walle.cc>
Date:   Wed, 05 Apr 2023 15:57:05 +0200
From:   Michael Walle <michael@...le.cc>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     "Sahin, Okan" <Okan.Sahin@...log.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 v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

Am 2023-04-05 15:20, schrieb Linus Walleij:
> On Tue, Apr 4, 2023 at 4:36 PM Sahin, Okan <Okan.Sahin@...log.com> 
> wrote:
> 
>> >The driver is pretty straight-forward, but I think this can use the generic
>> >GPIO_REGMAP helpers in drivers/gpio/gpio-regmap.c check other drivers selecting
>> >this helper library for inspiration.
> (..)
>> Thank you for your contribution. Should I add select GPIO_REGMAP into 
>> Kconfig?
> 
> Yes but that is not all, you also need to make use of the library 
> helpers
> provided in include/linux/gpio/regmap.h.
> 
> Find examples of other drivers doing this by e.g.:
> git grep gpio_regmap_register
> 
> drivers/gpio/gpio-sl28cpld.c:   return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> drivers/gpio/gpio-tn48m.c:      return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
> drivers/pinctrl/bcm/pinctrl-bcm63xx.c:  return
> PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));
> 
> ^Look what these are doing

This driver is doing two register writes/reads for setting the
direction, that's something which isn't supported in GPIO_REGMAP.
OTOH I'm not sure the driver is doing it correctly, because it also
seems to switch the pullup resisters together with the direction.
I'm not sure that is correct. So there might be just one register
involved after all and the GPIO_REGMAP should work again.

Also, according to the datasheet this has some nv memory (to set the
initial state of the GPIOs [?]). So it should really be a multi-function
device. I'm not sure if this has to be considered right from the
beginning or if the device support can start with GPIO only and later
be transitioned to a full featured MFD (probably with nvmem support).

-michael