Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <071614c0-0f84-6087-47c6-385541c5733d@baylibre.com>
Date:   Mon, 22 May 2023 14:26:48 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2 1/2] gpio: tps65219: add GPIO support for TPS65219 PMIC
Content-Language: en-US
To:     Tony Lindgren <tony@atomide.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Lee Jones <lee@kernel.org>, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-omap@vger.kernel.org,
        Jonathan Cormier <jcormier@criticallink.com>
References: <20230511-tps65219-add-gpio-support-v2-0-60feb64d649a@baylibre.com>
 <20230511-tps65219-add-gpio-support-v2-1-60feb64d649a@baylibre.com>
 <CACRpkdbjjoOoGeaqv9yQ4fTKqxt5eLDBjZCnNAYQnng3L+n8TQ@mail.gmail.com>
 <2e5cae40-7040-a92f-3d11-ce68f1307049@baylibre.com>
 <20230517063303.GO14287@atomide.com>
From:   jerome Neanne <jneanne@baylibre.com>
In-Reply-To: <20230517063303.GO14287@atomide.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 17/05/2023 08:33, Tony Lindgren wrote:
> * jerome Neanne <jneanne@baylibre.com> [230512 07:13]:
>>
>>
>> On 11/05/2023 22:57, Linus Walleij wrote:
>>>> +       /* Documentation is stating that GPIO0 direction must not be changed in Linux:
>>>> +        * Table 8-34. MFP_1_CONFIG(3): MULTI_DEVICE_ENABLE,
>>>> +        * Should only be changed in INITIALIZE state (prior to ON Request).
>>>> +        * Set statically by NVM, changing direction in application can cause a hang.
>>>> +        * Below can be used for test purpose only:
>>>> +        */
>>>> +
>>>> +#if 0
>>>> +       int ret = regmap_update_bits(gpio->tps->regmap, TPS65219_REG_MFP_1_CONFIG,
>>>> +                                TPS65219_GPIO0_DIR_MASK, direction);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +#endif
>>>> +       dev_err(gpio->tps->dev,
>>>> +               "GPIO%d direction set by NVM, change to %u failed, not allowed by specification\n",
>>>> +                offset, direction);
>>>> +       return -EOPNOTSUPP;
>>>> +}
>>>
>>> Normally people would complain about #if 0 code.
>>>
>>> But this is a special case!
>>>
>>> I definitely want the code to be in there somehow.
>>>
>>> What about:
>>>
>>> if (IS_ENABLED(DEBUG))?
>>>
>>> If someone enables debug with an explicit -DDEBUG to the compiler
>>> this could be allowed.
>> I'm fine with your proposal. Will wait few days just in case anyone wants to
>> add any comment then go for this.
> 
> Just wondering.. Would it help for the driver probe to set gpio0 as a gpio
> hog after reading the configured value?
> 
> Maybe the multi device enable just means the pin may be shared with no
> specific hardware reason to not change it during the runtime?

Your point looks valid. But I think we can't simply add a regular 
"gpio-hog" as a property in the device tree because we don't have a one 
to one mapping for GPIO consumer (theoretically we can have more than 2 
PMICs).

I think your suggestion is to read the multi-device value through regmap 
then "kind of" hog gpio with devm_gpio_request_one
So that gpio0 is preserved from being requested by other user.
Is this correct understanding?

Practically the board I'm using for test currently only have one PMIC.
I'm reluctant to implement additional logic that does not look so 
"conventional" that I can't test.

If maintainers agree, I'll postpone the implementation of your proposal 
until a platform compatible with this implementation is available for 
testing. So that we can check what is most accurate in real life.

Side Note: wo this implementation use of the driver is less optimal in 
multi-device configuration but still usable.

Regards,
Jerome.