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

On Wed, Oct 4, 2023 at 10:24=E2=80=AFPM Dipen Patel <[email protected]> wro=
te:
>
> On 10/4/23 3:04 AM, Bartosz Golaszewski wrote:
> > On Tue, Oct 3, 2023 at 6:42=E2=80=AFPM Dipen Patel <[email protected]> =
wrote:
> >>
> >> On 10/3/23 1:58 AM, Bartosz Golaszewski wrote:
> >>> On Mon, Oct 2, 2023 at 6:27=E2=80=AFPM Dipen Patel <[email protected]=
> wrote:
> >>>>
> >>>> On 10/2/23 1:33 AM, Bartosz Golaszewski wrote:
> >>>>> On Fri, Sep 29, 2023 at 11:38=E2=80=AFPM Dipen Patel <dipenp@nvidia=
.com> wrote:
> >>>>>>
> >>>>>> On 9/11/23 2:44 AM, Bartosz Golaszewski wrote:
> >>>>>>> From: Bartosz Golaszewski <[email protected]>
> >>>>>>>
> >>>>>>> Using any of the GPIO interfaces using the global numberspace is
> >>>>>>> deprecated. Make it clear in the comment.
> >>>>>>>
> >>>>>>> Signed-off-by: Bartosz Golaszewski <[email protected]=
g>
> >>>>>>> Acked-by: Linus Walleij <[email protected]>
> >>>>>>> ---
> >>>>>>> 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-tegra19=
4.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 GP=
IO global
> >>>>>>> -      * number which gets assigned run time.
> >>>>>>> -      * 2) The consumer passing GPIO from the DT which is assign=
ed
> >>>>>>> -      * statically for example by using TEGRA194_AON_GPIO gpio D=
T 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 ht=
e. 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 w=
ay
> >>>>> 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 unnecessar=
y)
> >>>>> 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, GP=
IO 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 thin=
k 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.g=
it/tree/drivers/gpio/gpiolib-cdev.c?h=3Dv6.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 th=
e 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.g=
it/tree/drivers/hte/hte-tegra194.c?h=3Dv6.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 bit=
s
> >> 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 thin=
k
> >> 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_g=
pio* API.
> >>
> >> 3. Since GPIO global space is runtime, need base to calculate the offs=
et
> >> 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 =3D 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 manipul=
ate
> gpio line that needs hte. So it does not need any handle as of now for th=
e
> 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 consum=
er 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 <[email protected]>

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

Bart