lkml.org 
[lkml]   [2023]   [Oct]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFT PATCH 14/21] hte: tegra194: don't access struct gpio_chip
    From
    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
    >>>>>>>
    >>>>>>
    >>>>>
    >>>>
    >>

    \
     
     \ /
      Last update: 2023-10-05 21:44    [W:3.519 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site