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]

Date:   Sun, 9 Apr 2023 14:25:40 +0000
From:   "Sahin, Okan" <Okan.Sahin@...log.com>
To:     "andy.shevchenko@...il.com" <andy.shevchenko@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Michael Walle <michael@...le.cc>
CC:     Bartosz Golaszewski <brgl@...ev.pl>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 2/2] gpio: ds4520: Add ADI DS4520 Regulator Support

>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).
>Also this code lacks of proper locking and has style issues.
>
>--
>With Best Regards,
>Andy Shevchenko
>

Hi Linus,

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?

Hi Michael,

I tested driver for both writing and reading, it seems fine. Initially, this question
confused me too, but after examining other drivers with nvmem, my opinion is 
same as both Linus and Andy. Also, at this point I am not planning to add
nvmem support.

Hi Andy,

Could you give more detail related to locking and style issues?

Regards,
Okan Sahin