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: <MN2PR03MB5168689A20C8D6C67F119BB7E7659@MN2PR03MB5168.namprd03.prod.outlook.com>
Date:   Wed, 26 Apr 2023 11:28:10 +0000
From:   "Sahin, Okan" <Okan.Sahin@...log.com>
To:     Michael Walle <michael@...le.cc>
CC:     "andy.shevchenko@...il.com" <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" <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

>Am 2023-04-24 17:39, schrieb Sahin, Okan:
>>> 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.
>>>
>>
>> Actually, I checked the functions that you suggested, but as far as I
>> understand
>> they might work if there would be one bit to set direction or value.
>> However,
>> this is not the case for ds4520. In other words, if I want to set the
>> gpio direction
>> as output, I need to set a corresponding bit for both F0 and F1
>> registers.
>
>I can't follow. F0/F1 is for the pull-up. That was actually my initial
>question and Linus said, that should probably be done in a seperate
>.set_config operation not together with a direction change.

I think I understand what you are trying to say so far. I did not have too much
experience related to gpio. I will set pull_up register in .set_config
However, I did not understand where its parameters come from.
set_config(struct gpio_chip *chip, unsigned int offset,
	      unsigned long config)
It might be trivial question, but Where does config come from?

At the end, I should rewrite the code using regmap_gpio, right? So if I rewrite 
code using regmap_gpio, how can I replace set_config(...)?

>
>> In the document, you can see block diagram. I do not know why, but
>> design is
>> not standard that’s why I think I can not use gpio-regmap.
>>
>>>> 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"
>>
>> I can set "SEE" bit as "0" in the Configuration Register to write
>> EEPROM so it might solve
>> issue.
>
>If you do that unconditionally, that might wear out the EEPROM,
>though.
>
>-michael

Hi Michael,

Thank you for your support.

Regards,
Okan Sahin