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: <a3ca3e705b5b8668cd511fc15681c75f@walle.cc>
Date:   Tue, 11 Apr 2023 15:42:39 +0200
From:   Michael Walle <michael@...le.cc>
To:     "Sahin, Okan" <Okan.Sahin@...log.com>
Cc:     andy.shevchenko@...il.com,
        Linus Walleij <linus.walleij@...aro.org>,
        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-09 16:25, schrieb Sahin, Okan:
>> Fri, Apr 07, 2023 at 03:48:25PM +0200, Linus Walleij kirjoitti:
>>> On Wed, Apr 5, 2023 at 3:57 PM Michael Walle <michael@...le.cc> 
>>> wrote:
>>> 
>>> > 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.
>>> 
>>> I'm pretty sure that should be in the .set_config() callback.
>>> 
>>> > 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).
>>> 
>>> That's a bit of a soft definition.
>>> 
>>> If the chip is *only* doing GPIO and nvram it can be a GPIO-only
>>> device I think.
>>> 
>>> The precedent is a ton of ethernet drivers with nvram for storing 
>>> e.g.
>>> the MAC address. We don't make all of those into MFDs, as the nvram 
>>> is
>>> closely tied to the one and only function of the block.
>> 
>> I agree with Linus. This should be part of the actual (main) driver 
>> for the chip as many
>> do (like USB to serial adapters that have GPIO capability).

You mean the gpio driver is calling nvmem_register()? Yeah I agree, that
should work.

> I think gpio_regmap is not suitable for this driver as Michael stated.
> https://www.analog.com/media/en/technical-documentation/data-sheets/ds4520.pdf
> Please check block diagram. There are two input registers that control
> gpio state
> so gpio_regmap does not look ok for this. Am I missing something?

You mean F8/F9? That will work as they are for different GPIOs. What
doesn't work with gpio-regmap is when you need to modify two different
registers for one GPIO. Have a look at gpio_regmap_get() and
gpio_regmap_set(). If the default gpio_regmap_simple_xlate() doesn't 
work
you can use your own .xlate() op.

> Also, at this point I am not planning to add nvmem support.

That is a pity, because that is the whole use case for this gpio 
expander,
no? "Programmable Replacement for Mechanical Jumpers and Switches"

-michael