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

[Bartosz Golaszewski]

On Wed, Oct 4, 2023 at 10:24 PM Dipen Patel <dipenp@xxxxxxxxxx> wrote:
>
> 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>

I think you can just take it through your tree, GPIO does not depend
on this change in any way.

Bart