Re: [PATCH] hte: tegra194: improve the GPIO-related comment

[Dipen Patel]

On 10/4/23 3:04 AM, Bartosz Golaszewski wrote:
> On Tue, Oct 3, 2023 at 6:42 PM Dipen Patel <dipenp@xxxxxxxxxx> wrote:
>>
>> On 10/3/23 1:58 AM, Bartosz Golaszewski wrote:
>>> On Mon, Oct 2, 2023 at 6:27 PM Dipen Patel <dipenp@xxxxxxxxxx> wrote:
>>>>
>>>> On 10/2/23 1:33 AM, Bartosz Golaszewski wrote:
>>>>> On Fri, Sep 29, 2023 at 11:38 PM Dipen Patel <dipenp@xxxxxxxxxx> wrote:
>>>>>>
>>>>>> On 9/11/23 2:44 AM, Bartosz Golaszewski wrote:
>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>>>>>>>
>>>>>>> Using any of the GPIO interfaces using the global numberspace is
>>>>>>> deprecated. Make it clear in the comment.
>>>>>>>
>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
>>>>>>> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> This was part of a wider series but since this is independent, I'm sending
>>>>>>> it separately.
>>>>>>>
>>>>>>>  drivers/hte/hte-tegra194.c | 13 ++++++++-----
>>>>>>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/hte/hte-tegra194.c b/drivers/hte/hte-tegra194.c
>>>>>>> index 6fe6897047ac..9fd3c00ff695 100644
>>>>>>> --- a/drivers/hte/hte-tegra194.c
>>>>>>> +++ b/drivers/hte/hte-tegra194.c
>>>>>>> @@ -407,12 +407,15 @@ static int tegra_hte_line_xlate(struct hte_chip *gc,
>>>>>>>               return -EINVAL;
>>>>>>>
>>>>>>>       /*
>>>>>>> +      * GPIO consumers can access GPIOs in two ways:
>>>>>>>        *
>>>>>>> -      * There are two paths GPIO consumers can take as follows:
>>>>>>> -      * 1) The consumer (gpiolib-cdev for example) which uses GPIO global
>>>>>>> -      * number which gets assigned run time.
>>>>>>> -      * 2) The consumer passing GPIO from the DT which is assigned
>>>>>>> -      * statically for example by using TEGRA194_AON_GPIO gpio DT binding.
>>>>>>> +      * 1) Using the global GPIO numberspace.
>>>>>>> +      *
>>>>>>> +      * This is the old, now DEPRECATED method and should not be used in
>>>>>>> +      * new code. TODO: Check if tegra is even concerned by this.
>>>>>> This use case is to do namespace mapping from gpio subsystem to hte. Few doubts:
>>>>>> 1. What does deprecate mean here? Does gpio subsys not use global space anymore?
>>>>>
>>>>> It does but we don't want to expose this to external users in any way
>>>>> anymore (and haven't to for years). This is what deprecated means.
>>>>> Users should deal with opaque GPIO descriptors not global GPIO
>>>>> numberspace.
>>>>>
>>>>>> 2. If yes, what GPIO number is set when it comes from gpiolib-cdev, as based on that I may have to
>>>>>> reflect in the mapping, tegra194_aon_gpio_map for example.
>>>>>
>>>>> Why DO you have to use a GPIO number though? If HTE needs just a
>>>>> number from some HTE numberspace (which in itself may be unnecessary)
>>>>> then why not just keep a local IDA for it? Do you have to know the
>>>>> GPIOs internal numbering scheme to make it work?
>>>>
>>>
>>> Dipen,
>>>
>>> Please set your mailer to wrap lines around at 80 characters as is
>>> customary on the mailing list.
>>
>> my email client misfired, will make sure. Thanks.
>>>
>>>> humm, overall, I just need to know which GPIO it is, for example, GPIO controller X Port A GPIO number 3
>>>> to do proper mapping.
>>>> Continuing from above example, the hte driver gets:
>>>> - GPIO Controller X from DT node
>>>> - the rest details in current code gets it from [1] and [2]
>>>>
>>>> If there is alternate method exists, I would like to explore. I think IDA will not help in this case as ID assigned
>>>> does not hold meaning in this context.
>>>>
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib-cdev.c?h=v6.6-rc3#n760
>>>
>>> Here: any reason why we have to translate the desc to the global GPIO
>>> numberspace? Can we just pass the descriptor pointer directly to the
>>> HTE subsystem?
>> Sure, if from GPIO descriptor with combination of any helper APIs from
>> the GPIO subsystem can help identify the GPIO pin, we can eliminate the need
>> to pass global number (I assume gpio desc
>> can be only accessed/manipulated using GPIO subsystem APIs)
>>
>>>
>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hte/hte-tegra194.c?h=v6.6-rc3#n421
>>>
>>> I still don't understand why you need to know the GPIO base? I'm not
>>> quite sure what the role of line_id is in this driver. Is it only to
>>> index the array?
>>>
>>> Please bear with me, I don't know this subsystem very well.
>>
>> Sure, no problem. Let me see if I am able to elaborate:
>>
>> 1. Map arrays' indexes are GPIO offsets so to avoid having
>> the extra field for the GPIO numbers.
>>
>> 2. The HTE driver needs to know exact GPIO to enable corresponding bits
>> in its registers. For example, hte register bit 3 would correspond to
>> GPIO 6 of GPIO controller X. If gpio descriptor is passed here, I think
>> I would need to do some conversions to identify the GPIO to enable
>> corresponding register bits. In the current scheme of things,
>> I though it was easier to identify passing the output of the desc_to_gpio* API.
>>
>> 3. Since GPIO global space is runtime, need base to calculate the offset
>> where offset does not change. So in the above example, gpio cdev would pass
>> 306 and HTE does simple conversion from the base, ie. 306 - 300 = 6.
>> Now 6 will serve as pin number as map array index to find the register.
>>
>> 4. Overall, I rely on base + global number to do namespace conversion
>> between gpio and hte subsys as far as gpio-cdev use case is concerned.
>>
> 
> Ok, so you *don't* need the global numbers, just controller-relative

Thats correct....

> offsets. This makes sense. This ties nicely into my plan for removing
> all accesses to gpio_chip except for GPIO providers.
> 
> Looking at the tegra dts I'm surprised that the GPIO controllers that
> use the HTE don't take the hte_lic or hte_aon phandles as arguments.
> How exactly do they resolve the HTE device to use for timestamping?
gpiochip added few callbacks during hte subsystem patch series to manipulate
gpio line that needs hte. So it does not need any handle as of now for the
tegra series of gpio controllers at least.

> 
> In any case, I think this commit is still correct.
Agreed.
> 
> Bart
> 
>>>
>>> Bart
>>>
>>>>
>>>>>
>>>>> Bart
>>>>>
>>>>>>> +      *
>>>>>>> +      * 2) Using GPIO descriptors that can be assigned to consumer devices
>>>>>>> +      * using device-tree, ACPI or lookup tables.
>>>>>>>        *
>>>>>>>        * The code below addresses both the consumer use cases and maps into
>>>>>>>        * HTE/GTE namespace.
>>>>>>
>>>>
>>
Reviewed-by: Dipen Patel <dipenp@xxxxxxxxxx>