diff mbox series

[RFT,14/21] hte: tegra194: don't access struct gpio_chip

Message ID 20230905185309.131295-15-brgl@bgdev.pl (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series gpio: convert users to gpio_device_find() and remove gpiochip_find() | expand

Commit Message

Bartosz Golaszewski Sept. 5, 2023, 6:53 p.m. UTC
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Using struct gpio_chip is not safe as it will disappear if the
underlying driver is unbound for any reason. Switch to using reference
counted struct gpio_device and its dedicated accessors.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/hte/hte-tegra194.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

Comments

Andy Shevchenko Sept. 6, 2023, 2:47 p.m. UTC | #1
On Tue, Sep 05, 2023 at 08:53:02PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Using struct gpio_chip is not safe as it will disappear if the
> underlying driver is unbound for any reason. Switch to using reference
> counted struct gpio_device and its dedicated accessors.

...

> +	struct gpio_device *gdev __free(gpio_device_put) = NULL;

Using this requires cleanup.h to be included.
Does any of the included GPIO headers guarantee that inclusion implicitly?
Even though, it's a good practice to include headers of what we are using
independently if other (library) headers include them. I.o.w. we can rely
only on our headers (here HTE framework related) to guarantee any inclusions
implicitly.

This also applies to other users of the same construct.

...

>  static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>  {
>  	return chip->fwnode == of_node_to_fwnode(data);
>  }

Not sure how many users of this kind of match, but it might be useful to have
it by GPIO library

	gpio_device_find_by_fwnode()
Linus Walleij Sept. 7, 2023, 7:28 a.m. UTC | #2
On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Using struct gpio_chip is not safe as it will disappear if the
> underlying driver is unbound for any reason. Switch to using reference
> counted struct gpio_device and its dedicated accessors.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

As Andy points out add <linux/cleanup.h>, with that fixed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I think this can be merged into the gpio tree after leaving some
slack for the HTE maintainer to look at it, things look so much
better after this.

Yours,
Linus Walleij
Bartosz Golaszewski Oct. 4, 2023, noon UTC | #3
On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Using struct gpio_chip is not safe as it will disappear if the
> > underlying driver is unbound for any reason. Switch to using reference
> > counted struct gpio_device and its dedicated accessors.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> As Andy points out add <linux/cleanup.h>, with that fixed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
> I think this can be merged into the gpio tree after leaving some
> slack for the HTE maintainer to look at it, things look so much
> better after this.
>
> Yours,
> Linus Walleij

Dipen,

if you could give this patch a test and possibly ack it for me to take
it through the GPIO tree (or go the immutable tag from HTE route) then
it would be great. This is the last user of gpiochip_find() treewide,
so with it we could remove it entirely for v6.7.

Bart
Dipen Patel Oct. 4, 2023, 8:30 p.m. UTC | #4
On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>
>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> Using struct gpio_chip is not safe as it will disappear if the
>>> underlying driver is unbound for any reason. Switch to using reference
>>> counted struct gpio_device and its dedicated accessors.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>
>> As Andy points out add <linux/cleanup.h>, with that fixed:
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>> I think this can be merged into the gpio tree after leaving some
>> slack for the HTE maintainer to look at it, things look so much
>> better after this.
>>
>> Yours,
>> Linus Walleij
> 
> Dipen,
> 
> if you could give this patch a test and possibly ack it for me to take
> it through the GPIO tree (or go the immutable tag from HTE route) then
> it would be great. This is the last user of gpiochip_find() treewide,
> so with it we could remove it entirely for v6.7.

Progress so far for the RFT...

I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
some patches I needed to manually apply and correct. With all this, it failed
compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
compile. I thought I should let you know this part.

Now, I tried to test the hte and it seems to fail finding the gpio device,
roughly around this place [1]. I thought it would be your patch series so
tried to just use 6.6rc1 without your patches and it still failed at the
same place. I have to trace back now from which kernel version it broke.

> 
> Bart
Dipen Patel Oct. 4, 2023, 8:33 p.m. UTC | #5
On 10/4/23 1:30 PM, Dipen Patel wrote:
> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>
>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>
>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>> underlying driver is unbound for any reason. Switch to using reference
>>>> counted struct gpio_device and its dedicated accessors.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>
>>> I think this can be merged into the gpio tree after leaving some
>>> slack for the HTE maintainer to look at it, things look so much
>>> better after this.
>>>
>>> Yours,
>>> Linus Walleij
>>
>> Dipen,
>>
>> if you could give this patch a test and possibly ack it for me to take
>> it through the GPIO tree (or go the immutable tag from HTE route) then
>> it would be great. This is the last user of gpiochip_find() treewide,
>> so with it we could remove it entirely for v6.7.
> 
> Progress so far for the RFT...
> 
> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> some patches I needed to manually apply and correct. With all this, it failed
> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> compile. I thought I should let you know this part.
> 
> Now, I tried to test the hte and it seems to fail finding the gpio device,
> roughly around this place [1]. I thought it would be your patch series so
> tried to just use 6.6rc1 without your patches and it still failed at the
> same place. I have to trace back now from which kernel version it broke.

[1].
https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781

of course with your patches it would fail for the gdev instead of the chip.
> 
>>
>> Bart
>
Dipen Patel Oct. 4, 2023, 10:54 p.m. UTC | #6
On 10/4/23 1:33 PM, Dipen Patel wrote:
> On 10/4/23 1:30 PM, Dipen Patel wrote:
>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>
>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>
>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>
>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>
>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>
>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>
>>>> I think this can be merged into the gpio tree after leaving some
>>>> slack for the HTE maintainer to look at it, things look so much
>>>> better after this.
>>>>
>>>> Yours,
>>>> Linus Walleij
>>>
>>> Dipen,
>>>
>>> if you could give this patch a test and possibly ack it for me to take
>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>> it would be great. This is the last user of gpiochip_find() treewide,
>>> so with it we could remove it entirely for v6.7.
>>
>> Progress so far for the RFT...
>>
>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>> some patches I needed to manually apply and correct. With all this, it failed
>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>> compile. I thought I should let you know this part.
>>
>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>> roughly around this place [1]. I thought it would be your patch series so
>> tried to just use 6.6rc1 without your patches and it still failed at the
>> same place. I have to trace back now from which kernel version it broke.
> 
> [1].
> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> 
> of course with your patches it would fail for the gdev instead of the chip.

Small update:

I put some debugging prints in the gpio match function in the hte-tegra194.c as
below:

static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
 {
+       struct device_node *node = data;
+       struct fwnode_handle *fw = of_node_to_fwnode(data);
+       if (!fw || !chip->fwnode)
+               pr_err("dipen patel: fw is null\n");

-       pr_err("%s:%d\n", __func__, __LINE__);
+       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
__func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
fw), fw->dev->init_name);
        return chip->fwnode == of_node_to_fwnode(data);
 }

The output of the printfs looks like below:
[    3.955194] dipen patel: fw is null -----> this message started appearing
when I added !chip->fwnode test in the if condition line.

[    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
gpio@c2f0000, match?:0, fwnode name:(null)

I conclude that chip->fwnode is empty. Any idea in which conditions that node
would be empty?

>>
>>>
>>> Bart
>>
>
Dipen Patel Oct. 4, 2023, 11:51 p.m. UTC | #7
On 10/4/23 3:54 PM, Dipen Patel wrote:
> On 10/4/23 1:33 PM, Dipen Patel wrote:
>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>
>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>
>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>
>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>
>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>
>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>
>>>>> I think this can be merged into the gpio tree after leaving some
>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>> better after this.
>>>>>
>>>>> Yours,
>>>>> Linus Walleij
>>>>
>>>> Dipen,
>>>>
>>>> if you could give this patch a test and possibly ack it for me to take
>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>> so with it we could remove it entirely for v6.7.
>>>
>>> Progress so far for the RFT...
>>>
>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>> some patches I needed to manually apply and correct. With all this, it failed
>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>> compile. I thought I should let you know this part.
>>>
>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>> roughly around this place [1]. I thought it would be your patch series so
>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>> same place. I have to trace back now from which kernel version it broke.
>>
>> [1].
>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>
>> of course with your patches it would fail for the gdev instead of the chip.
> 
> Small update:
> 
> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> below:
> 
> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>  {
> +       struct device_node *node = data;
> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> +       if (!fw || !chip->fwnode)
> +               pr_err("dipen patel: fw is null\n");
> 
> -       pr_err("%s:%d\n", __func__, __LINE__);
> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> fw), fw->dev->init_name);
>         return chip->fwnode == of_node_to_fwnode(data);
>  }
> 
> The output of the printfs looks like below:
> [    3.955194] dipen patel: fw is null -----> this message started appearing
> when I added !chip->fwnode test in the if condition line.
> 
> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> gpio@c2f0000, match?:0, fwnode name:(null)
> 
> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> would be empty?

sorry for spamming, one last message before I sign off for the day....

Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
was able to verify your patch series.

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index d87dd06db40d..a56c159d7136 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
                offset += port->pins;
        }

+       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
+
        return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
 }

Now, few follow up questions:
1) is this the correct way of setting the chip fwnode in the gpio driver?
2) Or should I use something else in hte matching function instead of fwnode so
to avoid adding above line in the gpio driver?

> 
>>>
>>>>
>>>> Bart
>>>
>>
>
Bartosz Golaszewski Oct. 5, 2023, 1:48 p.m. UTC | #8
On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/4/23 3:54 PM, Dipen Patel wrote:
> > On 10/4/23 1:33 PM, Dipen Patel wrote:
> >> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>
> >>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>
> >>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>
> >>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>
> >>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>
> >>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>
> >>>>> I think this can be merged into the gpio tree after leaving some
> >>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>> better after this.
> >>>>>
> >>>>> Yours,
> >>>>> Linus Walleij
> >>>>
> >>>> Dipen,
> >>>>
> >>>> if you could give this patch a test and possibly ack it for me to take
> >>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>> so with it we could remove it entirely for v6.7.
> >>>
> >>> Progress so far for the RFT...
> >>>
> >>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>> some patches I needed to manually apply and correct. With all this, it failed
> >>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>> compile. I thought I should let you know this part.
> >>>
> >>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>> roughly around this place [1]. I thought it would be your patch series so
> >>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>> same place. I have to trace back now from which kernel version it broke.
> >>
> >> [1].
> >> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>
> >> of course with your patches it would fail for the gdev instead of the chip.
> >
> > Small update:
> >
> > I put some debugging prints in the gpio match function in the hte-tegra194.c as
> > below:
> >
> > static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >  {
> > +       struct device_node *node = data;
> > +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> > +       if (!fw || !chip->fwnode)
> > +               pr_err("dipen patel: fw is null\n");
> >
> > -       pr_err("%s:%d\n", __func__, __LINE__);
> > +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> > __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> > fw), fw->dev->init_name);
> >         return chip->fwnode == of_node_to_fwnode(data);
> >  }
> >
> > The output of the printfs looks like below:
> > [    3.955194] dipen patel: fw is null -----> this message started appearing
> > when I added !chip->fwnode test in the if condition line.
> >
> > [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> > gpio@c2f0000, match?:0, fwnode name:(null)
> >
> > I conclude that chip->fwnode is empty. Any idea in which conditions that node
> > would be empty?
>
> sorry for spamming, one last message before I sign off for the day....
>
> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> was able to verify your patch series.
>
> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> index d87dd06db40d..a56c159d7136 100644
> --- a/drivers/gpio/gpio-tegra186.c
> +++ b/drivers/gpio/gpio-tegra186.c
> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>                 offset += port->pins;
>         }
>
> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> +
>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>  }
>
> Now, few follow up questions:
> 1) is this the correct way of setting the chip fwnode in the gpio driver?

You shouldn't need this. This driver already does:

    gpio->gpio.parent = &pdev->dev;

so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
check why this doesn't happen?

Bart

> 2) Or should I use something else in hte matching function instead of fwnode so
> to avoid adding above line in the gpio driver?
>
> >
> >>>
> >>>>
> >>>> Bart
> >>>
> >>
> >
>
Dipen Patel Oct. 5, 2023, 6:12 p.m. UTC | #9
On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>
>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>
>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>
>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>
>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>
>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>
>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>> better after this.
>>>>>>>
>>>>>>> Yours,
>>>>>>> Linus Walleij
>>>>>>
>>>>>> Dipen,
>>>>>>
>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>> so with it we could remove it entirely for v6.7.
>>>>>
>>>>> Progress so far for the RFT...
>>>>>
>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>> compile. I thought I should let you know this part.
>>>>>
>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>
>>>> [1].
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>
>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>
>>> Small update:
>>>
>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>> below:
>>>
>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>  {
>>> +       struct device_node *node = data;
>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>> +       if (!fw || !chip->fwnode)
>>> +               pr_err("dipen patel: fw is null\n");
>>>
>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>> fw), fw->dev->init_name);
>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>  }
>>>
>>> The output of the printfs looks like below:
>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>> when I added !chip->fwnode test in the if condition line.
>>>
>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>
>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>> would be empty?
>>
>> sorry for spamming, one last message before I sign off for the day....
>>
>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>> was able to verify your patch series.
>>
>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>> index d87dd06db40d..a56c159d7136 100644
>> --- a/drivers/gpio/gpio-tegra186.c
>> +++ b/drivers/gpio/gpio-tegra186.c
>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>                 offset += port->pins;
>>         }
>>
>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +
>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>  }
>>
>> Now, few follow up questions:
>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> 
> You shouldn't need this. This driver already does:
> 
>     gpio->gpio.parent = &pdev->dev;
> 
> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> check why this doesn't happen?

I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
The only reference I see is here [1]. Does it mean I need to change my match
function from:

chip->fwnode == of_node_to_fwnode(data)

to:
dev_fwnode(chip->parent) == of_node_to_fwnode(data)?

[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767

> 
> Bart
> 
>> 2) Or should I use something else in hte matching function instead of fwnode so
>> to avoid adding above line in the gpio driver?
>>
>>>
>>>>>
>>>>>>
>>>>>> Bart
>>>>>
>>>>
>>>
>>
Bartosz Golaszewski Oct. 5, 2023, 7:05 p.m. UTC | #10
On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>
> >>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>
> >>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>
> >>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>
> >>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>
> >>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>> better after this.
> >>>>>>>
> >>>>>>> Yours,
> >>>>>>> Linus Walleij
> >>>>>>
> >>>>>> Dipen,
> >>>>>>
> >>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>> so with it we could remove it entirely for v6.7.
> >>>>>
> >>>>> Progress so far for the RFT...
> >>>>>
> >>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>> compile. I thought I should let you know this part.
> >>>>>
> >>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>
> >>>> [1].
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>
> >>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>
> >>> Small update:
> >>>
> >>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>> below:
> >>>
> >>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>  {
> >>> +       struct device_node *node = data;
> >>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>> +       if (!fw || !chip->fwnode)
> >>> +               pr_err("dipen patel: fw is null\n");
> >>>
> >>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>> fw), fw->dev->init_name);
> >>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>  }
> >>>
> >>> The output of the printfs looks like below:
> >>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>> when I added !chip->fwnode test in the if condition line.
> >>>
> >>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>
> >>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>> would be empty?
> >>
> >> sorry for spamming, one last message before I sign off for the day....
> >>
> >> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >> was able to verify your patch series.
> >>
> >> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >> index d87dd06db40d..a56c159d7136 100644
> >> --- a/drivers/gpio/gpio-tegra186.c
> >> +++ b/drivers/gpio/gpio-tegra186.c
> >> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>                 offset += port->pins;
> >>         }
> >>
> >> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >> +
> >>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>  }
> >>
> >> Now, few follow up questions:
> >> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >
> > You shouldn't need this. This driver already does:
> >
> >     gpio->gpio.parent = &pdev->dev;
> >
> > so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> > check why this doesn't happen?
>
> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> The only reference I see is here [1]. Does it mean I need to change my match
> function from:
>
> chip->fwnode == of_node_to_fwnode(data)
>
> to:
> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?

No! chip->fwnode is only used to let GPIOLIB know which fwnode to
assign to the GPIO device (struct gpio_device).

Bart

>
> [1]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>
> >
> > Bart
> >
> >> 2) Or should I use something else in hte matching function instead of fwnode so
> >> to avoid adding above line in the gpio driver?
> >>
> >>>
> >>>>>
> >>>>>>
> >>>>>> Bart
> >>>>>
> >>>>
> >>>
> >>
>
Dipen Patel Oct. 5, 2023, 7:43 p.m. UTC | #11
On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>
>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>
>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>
>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>
>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>
>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>> better after this.
>>>>>>>>>
>>>>>>>>> Yours,
>>>>>>>>> Linus Walleij
>>>>>>>>
>>>>>>>> Dipen,
>>>>>>>>
>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>
>>>>>>> Progress so far for the RFT...
>>>>>>>
>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>> compile. I thought I should let you know this part.
>>>>>>>
>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>
>>>>>> [1].
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>
>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>
>>>>> Small update:
>>>>>
>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>> below:
>>>>>
>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>  {
>>>>> +       struct device_node *node = data;
>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>> +       if (!fw || !chip->fwnode)
>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>
>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>> fw), fw->dev->init_name);
>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>  }
>>>>>
>>>>> The output of the printfs looks like below:
>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>
>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>
>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>> would be empty?
>>>>
>>>> sorry for spamming, one last message before I sign off for the day....
>>>>
>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>> was able to verify your patch series.
>>>>
>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>> index d87dd06db40d..a56c159d7136 100644
>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>                 offset += port->pins;
>>>>         }
>>>>
>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>> +
>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>  }
>>>>
>>>> Now, few follow up questions:
>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>
>>> You shouldn't need this. This driver already does:
>>>
>>>     gpio->gpio.parent = &pdev->dev;
>>>
>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>> check why this doesn't happen?
>>
>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>> The only reference I see is here [1]. Does it mean I need to change my match
>> function from:
>>
>> chip->fwnode == of_node_to_fwnode(data)
>>
>> to:
>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> 
> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> assign to the GPIO device (struct gpio_device).
What do you suggest I should use for the match as I do not see chip->fwnode
being set?

> 
> Bart
> 
>>
>> [1]:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>
>>>
>>> Bart
>>>
>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>> to avoid adding above line in the gpio driver?
>>>>
>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Bart
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
Bartosz Golaszewski Oct. 5, 2023, 7:47 p.m. UTC | #12
On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> >>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>>>
> >>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>>
> >>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>
> >>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>>>
> >>>>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>>>> better after this.
> >>>>>>>>>
> >>>>>>>>> Yours,
> >>>>>>>>> Linus Walleij
> >>>>>>>>
> >>>>>>>> Dipen,
> >>>>>>>>
> >>>>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>>>> so with it we could remove it entirely for v6.7.
> >>>>>>>
> >>>>>>> Progress so far for the RFT...
> >>>>>>>
> >>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>>>> compile. I thought I should let you know this part.
> >>>>>>>
> >>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>>>
> >>>>>> [1].
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>>>
> >>>>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>>>
> >>>>> Small update:
> >>>>>
> >>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>>>> below:
> >>>>>
> >>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>>>  {
> >>>>> +       struct device_node *node = data;
> >>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>>>> +       if (!fw || !chip->fwnode)
> >>>>> +               pr_err("dipen patel: fw is null\n");
> >>>>>
> >>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>>>> fw), fw->dev->init_name);
> >>>>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>>>  }
> >>>>>
> >>>>> The output of the printfs looks like below:
> >>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>>>> when I added !chip->fwnode test in the if condition line.
> >>>>>
> >>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>>>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>>>
> >>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>>>> would be empty?
> >>>>
> >>>> sorry for spamming, one last message before I sign off for the day....
> >>>>
> >>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >>>> was able to verify your patch series.
> >>>>
> >>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >>>> index d87dd06db40d..a56c159d7136 100644
> >>>> --- a/drivers/gpio/gpio-tegra186.c
> >>>> +++ b/drivers/gpio/gpio-tegra186.c
> >>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>>>                 offset += port->pins;
> >>>>         }
> >>>>
> >>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >>>> +
> >>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>>>  }
> >>>>
> >>>> Now, few follow up questions:
> >>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >>>
> >>> You shouldn't need this. This driver already does:
> >>>
> >>>     gpio->gpio.parent = &pdev->dev;
> >>>
> >>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> >>> check why this doesn't happen?
> >>
> >> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> >> The only reference I see is here [1]. Does it mean I need to change my match
> >> function from:
> >>
> >> chip->fwnode == of_node_to_fwnode(data)
> >>
> >> to:
> >> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> >
> > No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> > assign to the GPIO device (struct gpio_device).
> What do you suggest I should use for the match as I do not see chip->fwnode
> being set?
>

Andy, Linus,

Do you think it makes sense to make gpiochip_add_data_with_key()
assign the chip's fwnode if it's not set by the caller (and instead
taken from the parent device) for this particular use-case?

I think it's fine but wanted to run it by you.

Bart

> >
> > Bart
> >
> >>
> >> [1]:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
> >>
> >>>
> >>> Bart
> >>>
> >>>> 2) Or should I use something else in hte matching function instead of fwnode so
> >>>> to avoid adding above line in the gpio driver?
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Bart
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>
Bartosz Golaszewski Oct. 9, 2023, 6:48 a.m. UTC | #13
On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>
> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
> > On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
> >>
> >> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
> >>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
> >>>>
> >>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
> >>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
> >>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
> >>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
> >>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >>>>>>>>>
> >>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>>
> >>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
> >>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
> >>>>>>>>>> counted struct gpio_device and its dedicated accessors.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>>>>>>
> >>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
> >>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>>>>>>
> >>>>>>>>> I think this can be merged into the gpio tree after leaving some
> >>>>>>>>> slack for the HTE maintainer to look at it, things look so much
> >>>>>>>>> better after this.
> >>>>>>>>>
> >>>>>>>>> Yours,
> >>>>>>>>> Linus Walleij
> >>>>>>>>
> >>>>>>>> Dipen,
> >>>>>>>>
> >>>>>>>> if you could give this patch a test and possibly ack it for me to take
> >>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
> >>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
> >>>>>>>> so with it we could remove it entirely for v6.7.
> >>>>>>>
> >>>>>>> Progress so far for the RFT...
> >>>>>>>
> >>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
> >>>>>>> some patches I needed to manually apply and correct. With all this, it failed
> >>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
> >>>>>>> compile. I thought I should let you know this part.
> >>>>>>>
> >>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
> >>>>>>> roughly around this place [1]. I thought it would be your patch series so
> >>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
> >>>>>>> same place. I have to trace back now from which kernel version it broke.
> >>>>>>
> >>>>>> [1].
> >>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
> >>>>>>
> >>>>>> of course with your patches it would fail for the gdev instead of the chip.
> >>>>>
> >>>>> Small update:
> >>>>>
> >>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
> >>>>> below:
> >>>>>
> >>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
> >>>>>  {
> >>>>> +       struct device_node *node = data;
> >>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
> >>>>> +       if (!fw || !chip->fwnode)
> >>>>> +               pr_err("dipen patel: fw is null\n");
> >>>>>
> >>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
> >>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
> >>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
> >>>>> fw), fw->dev->init_name);
> >>>>>         return chip->fwnode == of_node_to_fwnode(data);
> >>>>>  }
> >>>>>
> >>>>> The output of the printfs looks like below:
> >>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
> >>>>> when I added !chip->fwnode test in the if condition line.
> >>>>>
> >>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
> >>>>> gpio@c2f0000, match?:0, fwnode name:(null)
> >>>>>
> >>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
> >>>>> would be empty?
> >>>>
> >>>> sorry for spamming, one last message before I sign off for the day....
> >>>>
> >>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
> >>>> was able to verify your patch series.
> >>>>
> >>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
> >>>> index d87dd06db40d..a56c159d7136 100644
> >>>> --- a/drivers/gpio/gpio-tegra186.c
> >>>> +++ b/drivers/gpio/gpio-tegra186.c
> >>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
> >>>>                 offset += port->pins;
> >>>>         }
> >>>>
> >>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
> >>>> +
> >>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
> >>>>  }
> >>>>
> >>>> Now, few follow up questions:
> >>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
> >>>
> >>> You shouldn't need this. This driver already does:
> >>>
> >>>     gpio->gpio.parent = &pdev->dev;
> >>>
> >>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
> >>> check why this doesn't happen?
> >>
> >> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
> >> The only reference I see is here [1]. Does it mean I need to change my match
> >> function from:
> >>
> >> chip->fwnode == of_node_to_fwnode(data)
> >>
> >> to:
> >> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
> >
> > No! chip->fwnode is only used to let GPIOLIB know which fwnode to
> > assign to the GPIO device (struct gpio_device).
> What do you suggest I should use for the match as I do not see chip->fwnode
> being set?
>

This is most likely going to be a longer discussion. I suggest that in
the meantime you just assign the gc->fwnode pointer explicitly from
the platform device in the tegra GPIO driver and use it in the lookup
function. Note that this is NOT wrong or a hack. It's just that most
devices don't need to be looked up using gpio_device_find().

Bart

> >
> > Bart
> >
> >>
> >> [1]:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
> >>
> >>>
> >>> Bart
> >>>
> >>>> 2) Or should I use something else in hte matching function instead of fwnode so
> >>>> to avoid adding above line in the gpio driver?
> >>>>
> >>>>>
> >>>>>>>
> >>>>>>>>
> >>>>>>>> Bart
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>
>
Dipen Patel Oct. 9, 2023, 4:34 p.m. UTC | #14
On 10/8/23 11:48 PM, Bartosz Golaszewski wrote:
> On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>
>> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
>>> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>
>>>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>>
>>>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>>>
>>>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>>>
>>>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>>>> better after this.
>>>>>>>>>>>
>>>>>>>>>>> Yours,
>>>>>>>>>>> Linus Walleij
>>>>>>>>>>
>>>>>>>>>> Dipen,
>>>>>>>>>>
>>>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>>>
>>>>>>>>> Progress so far for the RFT...
>>>>>>>>>
>>>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>>>> compile. I thought I should let you know this part.
>>>>>>>>>
>>>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>>>
>>>>>>>> [1].
>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>>>
>>>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>>>
>>>>>>> Small update:
>>>>>>>
>>>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>>>> below:
>>>>>>>
>>>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>>>  {
>>>>>>> +       struct device_node *node = data;
>>>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>>>> +       if (!fw || !chip->fwnode)
>>>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>>>
>>>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>>>> fw), fw->dev->init_name);
>>>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>>>  }
>>>>>>>
>>>>>>> The output of the printfs looks like below:
>>>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>>>
>>>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>>>
>>>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>>>> would be empty?
>>>>>>
>>>>>> sorry for spamming, one last message before I sign off for the day....
>>>>>>
>>>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>>>> was able to verify your patch series.
>>>>>>
>>>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>>>> index d87dd06db40d..a56c159d7136 100644
>>>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>>>                 offset += port->pins;
>>>>>>         }
>>>>>>
>>>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>>>> +
>>>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>>>  }
>>>>>>
>>>>>> Now, few follow up questions:
>>>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>>>
>>>>> You shouldn't need this. This driver already does:
>>>>>
>>>>>     gpio->gpio.parent = &pdev->dev;
>>>>>
>>>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>>>> check why this doesn't happen?
>>>>
>>>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>>>> The only reference I see is here [1]. Does it mean I need to change my match
>>>> function from:
>>>>
>>>> chip->fwnode == of_node_to_fwnode(data)
>>>>
>>>> to:
>>>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
>>>
>>> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
>>> assign to the GPIO device (struct gpio_device).
>> What do you suggest I should use for the match as I do not see chip->fwnode
>> being set?
>>
> 
> This is most likely going to be a longer discussion. I suggest that in
> the meantime you just assign the gc->fwnode pointer explicitly from
> the platform device in the tegra GPIO driver and use it in the lookup
> function. Note that this is NOT wrong or a hack. It's just that most
> devices don't need to be looked up using gpio_device_find().

Sure, at the same time, I am also find to use any other method/s.
> 
> Bart
> 
>>>
>>> Bart
>>>
>>>>
>>>> [1]:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>>>
>>>>>
>>>>> Bart
>>>>>
>>>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>>>> to avoid adding above line in the gpio driver?
>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Bart
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
Dipen Patel Oct. 9, 2023, 5:46 p.m. UTC | #15
On 10/9/23 9:34 AM, Dipen Patel wrote:
> On 10/8/23 11:48 PM, Bartosz Golaszewski wrote:
>> On Thu, Oct 5, 2023 at 9:43 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>
>>> On 10/5/23 12:05 PM, Bartosz Golaszewski wrote:
>>>> On Thu, Oct 5, 2023 at 8:12 PM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>
>>>>> On 10/5/23 6:48 AM, Bartosz Golaszewski wrote:
>>>>>> On Thu, Oct 5, 2023 at 1:52 AM Dipen Patel <dipenp@nvidia.com> wrote:
>>>>>>>
>>>>>>> On 10/4/23 3:54 PM, Dipen Patel wrote:
>>>>>>>> On 10/4/23 1:33 PM, Dipen Patel wrote:
>>>>>>>>> On 10/4/23 1:30 PM, Dipen Patel wrote:
>>>>>>>>>> On 10/4/23 5:00 AM, Bartosz Golaszewski wrote:
>>>>>>>>>>> On Thu, Sep 7, 2023 at 9:28 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Sep 5, 2023 at 8:53 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Using struct gpio_chip is not safe as it will disappear if the
>>>>>>>>>>>>> underlying driver is unbound for any reason. Switch to using reference
>>>>>>>>>>>>> counted struct gpio_device and its dedicated accessors.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> As Andy points out add <linux/cleanup.h>, with that fixed:
>>>>>>>>>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>>>>>>>>>
>>>>>>>>>>>> I think this can be merged into the gpio tree after leaving some
>>>>>>>>>>>> slack for the HTE maintainer to look at it, things look so much
>>>>>>>>>>>> better after this.
>>>>>>>>>>>>
>>>>>>>>>>>> Yours,
>>>>>>>>>>>> Linus Walleij
>>>>>>>>>>>
>>>>>>>>>>> Dipen,
>>>>>>>>>>>
>>>>>>>>>>> if you could give this patch a test and possibly ack it for me to take
>>>>>>>>>>> it through the GPIO tree (or go the immutable tag from HTE route) then
>>>>>>>>>>> it would be great. This is the last user of gpiochip_find() treewide,
>>>>>>>>>>> so with it we could remove it entirely for v6.7.
>>>>>>>>>>
>>>>>>>>>> Progress so far for the RFT...
>>>>>>>>>>
>>>>>>>>>> I tried applying the patch series on 6.6-rc1 and it did not apply cleanly,
>>>>>>>>>> some patches I needed to manually apply and correct. With all this, it failed
>>>>>>>>>> compilation at some spi/spi-bcm2835 driver. I disabled that and was able to
>>>>>>>>>> compile. I thought I should let you know this part.
>>>>>>>>>>
>>>>>>>>>> Now, I tried to test the hte and it seems to fail finding the gpio device,
>>>>>>>>>> roughly around this place [1]. I thought it would be your patch series so
>>>>>>>>>> tried to just use 6.6rc1 without your patches and it still failed at the
>>>>>>>>>> same place. I have to trace back now from which kernel version it broke.
>>>>>>>>>
>>>>>>>>> [1].
>>>>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/pateldipen1984/linux.git/tree/drivers/hte/hte-tegra194.c?h=for-next#n781
>>>>>>>>>
>>>>>>>>> of course with your patches it would fail for the gdev instead of the chip.
>>>>>>>>
>>>>>>>> Small update:
>>>>>>>>
>>>>>>>> I put some debugging prints in the gpio match function in the hte-tegra194.c as
>>>>>>>> below:
>>>>>>>>
>>>>>>>> static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
>>>>>>>>  {
>>>>>>>> +       struct device_node *node = data;
>>>>>>>> +       struct fwnode_handle *fw = of_node_to_fwnode(data);
>>>>>>>> +       if (!fw || !chip->fwnode)
>>>>>>>> +               pr_err("dipen patel: fw is null\n");
>>>>>>>>
>>>>>>>> -       pr_err("%s:%d\n", __func__, __LINE__);
>>>>>>>> +       pr_err("dipen patel, %s:%d: %s, %s, %s, match?:%d, fwnode name:%s\n",
>>>>>>>> __func__, __LINE__, chip->label, node->name, node->full_name, (chip->fwnode ==
>>>>>>>> fw), fw->dev->init_name);
>>>>>>>>         return chip->fwnode == of_node_to_fwnode(data);
>>>>>>>>  }
>>>>>>>>
>>>>>>>> The output of the printfs looks like below:
>>>>>>>> [    3.955194] dipen patel: fw is null -----> this message started appearing
>>>>>>>> when I added !chip->fwnode test in the if condition line.
>>>>>>>>
>>>>>>>> [    3.958864] dipen patel, tegra_gpiochip_match:689: tegra234-gpio, gpio,
>>>>>>>> gpio@c2f0000, match?:0, fwnode name:(null)
>>>>>>>>
>>>>>>>> I conclude that chip->fwnode is empty. Any idea in which conditions that node
>>>>>>>> would be empty?
>>>>>>>
>>>>>>> sorry for spamming, one last message before I sign off for the day....
>>>>>>>
>>>>>>> Seems, adding below in the tegra gpio driver resolved the issue I am facing, I
>>>>>>> was able to verify your patch series.
>>>>>>>
>>>>>>> diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
>>>>>>> index d87dd06db40d..a56c159d7136 100644
>>>>>>> --- a/drivers/gpio/gpio-tegra186.c
>>>>>>> +++ b/drivers/gpio/gpio-tegra186.c
>>>>>>> @@ -989,6 +989,8 @@ static int tegra186_gpio_probe(struct platform_device *pdev)
>>>>>>>                 offset += port->pins;
>>>>>>>         }
>>>>>>>
>>>>>>> +       gpio->gpio.fwnode = of_node_to_fwnode(pdev->dev.of_node);
>>>>>>> +
>>>>>>>         return devm_gpiochip_add_data(&pdev->dev, &gpio->gpio, gpio);
>>>>>>>  }
>>>>>>>
>>>>>>> Now, few follow up questions:
>>>>>>> 1) is this the correct way of setting the chip fwnode in the gpio driver?
>>>>>>
>>>>>> You shouldn't need this. This driver already does:
>>>>>>
>>>>>>     gpio->gpio.parent = &pdev->dev;
>>>>>>
>>>>>> so fwnode should be assigned in gpiochip_add_data_with_key(). Can you
>>>>>> check why this doesn't happen?
>>>>>
>>>>> I do not see anywhere chip->fwnode being set in the gpiochip_add_* function.
>>>>> The only reference I see is here [1]. Does it mean I need to change my match
>>>>> function from:
>>>>>
>>>>> chip->fwnode == of_node_to_fwnode(data)
>>>>>
>>>>> to:
>>>>> dev_fwnode(chip->parent) == of_node_to_fwnode(data)?
>>>>
>>>> No! chip->fwnode is only used to let GPIOLIB know which fwnode to
>>>> assign to the GPIO device (struct gpio_device).
>>> What do you suggest I should use for the match as I do not see chip->fwnode
>>> being set?
>>>
>>
>> This is most likely going to be a longer discussion. I suggest that in
>> the meantime you just assign the gc->fwnode pointer explicitly from
>> the platform device in the tegra GPIO driver and use it in the lookup
>> function. Note that this is NOT wrong or a hack. It's just that most
>> devices don't need to be looked up using gpio_device_find().
> 
> Sure, at the same time, I am also find to use any other method/s.

(Correction) I am also fine*

With patch
https://patchwork.ozlabs.org/project/linux-gpio/patch/20231009173858.723686-1-dipenp@nvidia.com/

Tested-by: Dipen Patel <dipenp@nvidia.com>

>>
>> Bart
>>
>>>>
>>>> Bart
>>>>
>>>>>
>>>>> [1]:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpiolib.c?h=v6.6-rc1#n767
>>>>>
>>>>>>
>>>>>> Bart
>>>>>>
>>>>>>> 2) Or should I use something else in hte matching function instead of fwnode so
>>>>>>> to avoid adding above line in the gpio driver?
>>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Bart
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>
diff mbox series

Patch

diff --git a/drivers/hte/hte-tegra194.c b/drivers/hte/hte-tegra194.c
index 9fd3c00ff695..d83ef30c9588 100644
--- a/drivers/hte/hte-tegra194.c
+++ b/drivers/hte/hte-tegra194.c
@@ -132,7 +132,7 @@  struct tegra_hte_soc {
 	const struct tegra_hte_data *prov_data;
 	struct tegra_hte_line_data *line_data;
 	struct hte_chip *chip;
-	struct gpio_chip *c;
+	struct gpio_device *gdev;
 	void __iomem *regs;
 };
 
@@ -421,7 +421,7 @@  static int tegra_hte_line_xlate(struct hte_chip *gc,
 	 * HTE/GTE namespace.
 	 */
 	if (gs->prov_data->type == HTE_TEGRA_TYPE_GPIO && !args) {
-		line_id = desc->attr.line_id - gs->c->base;
+		line_id = desc->attr.line_id - gpio_device_get_base(gs->gdev);
 		map = gs->prov_data->map;
 		map_sz = gs->prov_data->map_sz;
 	} else if (gs->prov_data->type == HTE_TEGRA_TYPE_GPIO && args) {
@@ -643,12 +643,15 @@  static irqreturn_t tegra_hte_isr(int irq, void *dev_id)
 static bool tegra_hte_match_from_linedata(const struct hte_chip *chip,
 					  const struct hte_ts_desc *hdesc)
 {
+	struct gpio_device *gdev __free(gpio_device_put) = NULL;
 	struct tegra_hte_soc *hte_dev = chip->data;
 
 	if (!hte_dev || (hte_dev->prov_data->type != HTE_TEGRA_TYPE_GPIO))
 		return false;
 
-	return hte_dev->c == gpiod_to_chip(hdesc->attr.line_data);
+	gdev = gpiod_to_device(hdesc->attr.line_data);
+
+	return hte_dev->gdev == gdev;
 }
 
 static const struct of_device_id tegra_hte_of_match[] = {
@@ -676,16 +679,18 @@  static void tegra_gte_disable(void *data)
 	tegra_hte_writel(gs, HTE_TECTRL, 0);
 }
 
-static int tegra_get_gpiochip_from_name(struct gpio_chip *chip, void *data)
-{
-	return !strcmp(chip->label, data);
-}
-
 static int tegra_gpiochip_match(struct gpio_chip *chip, void *data)
 {
 	return chip->fwnode == of_node_to_fwnode(data);
 }
 
+static void tegra_hte_put_gpio_device(void *data)
+{
+	struct gpio_device *gdev = data;
+
+	gpio_device_put(gdev);
+}
+
 static int tegra_hte_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -763,8 +768,8 @@  static int tegra_hte_probe(struct platform_device *pdev)
 
 		if (of_device_is_compatible(dev->of_node,
 					    "nvidia,tegra194-gte-aon")) {
-			hte_dev->c = gpiochip_find("tegra194-gpio-aon",
-						tegra_get_gpiochip_from_name);
+			hte_dev->gdev =
+				gpio_device_find_by_label("tegra194-gpio-aon");
 		} else {
 			gpio_ctrl = of_parse_phandle(dev->of_node,
 						     "nvidia,gpio-controller",
@@ -775,14 +780,19 @@  static int tegra_hte_probe(struct platform_device *pdev)
 				return -ENODEV;
 			}
 
-			hte_dev->c = gpiochip_find(gpio_ctrl,
-						   tegra_gpiochip_match);
+			hte_dev->gdev = gpio_device_find(gpio_ctrl,
+							 tegra_gpiochip_match);
 			of_node_put(gpio_ctrl);
 		}
 
-		if (!hte_dev->c)
+		if (!hte_dev->gdev)
 			return dev_err_probe(dev, -EPROBE_DEFER,
 					     "wait for gpio controller\n");
+
+		ret = devm_add_action_or_reset(dev, tegra_hte_put_gpio_device,
+					       hte_dev->gdev);
+		if (ret)
+			return ret;
 	}
 
 	hte_dev->chip = gc;