| Message ID | 1609936000-28378-1-git-send-email-srinivas.neeli@xilinx.com |
|---|---|
| Headers | show |
| Series | gpio-xilinx: Update on xilinx gpio driver | expand |
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > Add support for suspend and resume, pm runtime suspend and resume. > Added free and request calls. > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> (...) > +static int xgpio_request(struct gpio_chip *chip, unsigned int offset) > +{ > + int ret; > + > + ret = pm_runtime_get_sync(chip->parent); > + /* > + * If the device is already active pm_runtime_get() will return 1 on > + * success, but gpio_request still needs to return 0. > + */ > + return ret < 0 ? ret : 0; > +} That's clever. I think more GPIO drivers should be doing it like this, today I think most just ignore the return code. > +static int __maybe_unused xgpio_suspend(struct device *dev) > +static int __maybe_unused xgpio_resume(struct device *dev) Those look good. > /** > * xgpio_remove - Remove method for the GPIO device. > * @pdev: pointer to the platform device > @@ -289,7 +323,10 @@ static int xgpio_remove(struct platform_device *pdev) > { > struct xgpio_instance *gpio = platform_get_drvdata(pdev); > > - clk_disable_unprepare(gpio->clk); > + if (!pm_runtime_suspended(&pdev->dev)) > + clk_disable_unprepare(gpio->clk); > + > + pm_runtime_disable(&pdev->dev); This looks complex and racy. What if the device is resumed after you executed the first part of the statement. The normal sequence is: pm_runtime_get_sync(dev); pm_runtime_put_noidle(dev); pm_runtime_disable(dev); This will make sure the clock is enabled and pm runtime is disabled. After this you can unconditionally call clk_disable_unprepare(gpio->clk); It is what you are doing on the errorpath of probe(). Yours, Linus Walleij
On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > Add check to see if gpio-width property does not exceed 32. > If it exceeds then return -EINVAL. > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> Aha > @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) > if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0])) > chip->gpio_width[0] = 32; This xlnx,gpio-width seems very much like the standard ngpios property from Documentation/devicetree/bindings/gpio/gpio.txt but I guess not much to do about that now. :/ Do you think you can add support for both? > + if (chip->gpio_width[0] > 32) > + return -EINVAL; This looks OK. Yours, Linus Walleij
On 07. 01. 21 11:17, Linus Walleij wrote: > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > >> Add check to see if gpio-width property does not exceed 32. >> If it exceeds then return -EINVAL. >> >> Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > > Aha > >> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) >> if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0])) >> chip->gpio_width[0] = 32; > > This xlnx,gpio-width seems very much like the standard ngpios property > from Documentation/devicetree/bindings/gpio/gpio.txt > but I guess not much to do about that now. :/ > > Do you think you can add support for both? support for both is definitely possible but we need to handle also gpio width for second channel referenced by xlnx,gpio2-widht now. It means we could end up in situation which can be misleading for users where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we have 20 gpios. I think that it is better not to start to mess with ngpios property not to confuse people which are coming from other SOCs because ngpios can suggest all gpios assigned to this controller. And in second case where ngpios is total number of gpios and if xlnx,gpio2-width is defined you can find width for first bank. But it is questionable if this improve situation here. Please correct me if my logic is not correct. Definitely this should be done separately out of this patch. > >> + if (chip->gpio_width[0] > 32) >> + return -EINVAL; > > This looks OK. Does it mean ack for this patch? Thanks, Michal
On Thu, Jan 7, 2021 at 11:29 AM Michal Simek <michal.simek@xilinx.com> wrote: > On 07. 01. 21 11:17, Linus Walleij wrote: > > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > >> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) > >> if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0])) > >> chip->gpio_width[0] = 32; > > > > This xlnx,gpio-width seems very much like the standard ngpios property > > from Documentation/devicetree/bindings/gpio/gpio.txt > > but I guess not much to do about that now. :/ > > > > Do you think you can add support for both? > > support for both is definitely possible but we need to handle also gpio > width for second channel referenced by xlnx,gpio2-widht now. > > It means we could end up in situation which can be misleading for users > where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we > have 20 gpios. OK that is confusing. Let's not do that then. > I think that it is better not to start to mess with ngpios property not > to confuse people which are coming from other SOCs because ngpios can > suggest all gpios assigned to this controller. OK I agree. > >> + if (chip->gpio_width[0] > 32) > >> + return -EINVAL; > > > > This looks OK. > > Does it mean ack for this patch? Yeah after explanations this patch is fine: Acked-by: Linus Walleij <linus.walleij@linaro.org> It's just that this hardware with paired controllers is a bit weird so it will lead to discussions all the time because it's hard to understand. Yours, Linus Walleij
On 07. 01. 21 11:47, Linus Walleij wrote: > On Thu, Jan 7, 2021 at 11:29 AM Michal Simek <michal.simek@xilinx.com> wrote: >> On 07. 01. 21 11:17, Linus Walleij wrote: >>> On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> wrote: > >>>> @@ -591,6 +591,9 @@ static int xgpio_probe(struct platform_device *pdev) >>>> if (of_property_read_u32(np, "xlnx,gpio-width", &chip->gpio_width[0])) >>>> chip->gpio_width[0] = 32; >>> >>> This xlnx,gpio-width seems very much like the standard ngpios property >>> from Documentation/devicetree/bindings/gpio/gpio.txt >>> but I guess not much to do about that now. :/ >>> >>> Do you think you can add support for both? >> >> support for both is definitely possible but we need to handle also gpio >> width for second channel referenced by xlnx,gpio2-widht now. >> >> It means we could end up in situation which can be misleading for users >> where ngpios will be 10 and xlnx,gpio2-width another 10 and in total we >> have 20 gpios. > > OK that is confusing. Let's not do that then. > >> I think that it is better not to start to mess with ngpios property not >> to confuse people which are coming from other SOCs because ngpios can >> suggest all gpios assigned to this controller. > > OK I agree. > >>>> + if (chip->gpio_width[0] > 32) >>>> + return -EINVAL; >>> >>> This looks OK. >> >> Does it mean ack for this patch? > > Yeah after explanations this patch is fine: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > It's just that this hardware with paired controllers is a bit weird so it will > lead to discussions all the time because it's hard to understand. Maybe it should be described a little bit differently in DT. Just to have gpio node and every bank could be described as a child node where standard properties could be used and irq will be shared. Thanks, Michal
Hi Linus, > -----Original Message----- > From: Linus Walleij <linus.walleij@linaro.org> > Sent: Thursday, January 7, 2021 3:17 PM > To: Srinivas Neeli <sneeli@xilinx.com> > Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>; Michal Simek > <michals@xilinx.com>; Shubhrajyoti Datta <shubhraj@xilinx.com>; Srinivas > Goud <sgoud@xilinx.com>; Robert Hancock <hancock@sedsystems.ca>; > William Breathitt Gray <vilhelm.gray@gmail.com>; Syed Nayyar Waris > <syednwaris@gmail.com>; open list:GPIO SUBSYSTEM <linux- > gpio@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; > linux-kernel@vger.kernel.org; git <git@xilinx.com> > Subject: Re: [PATCH V4 4/5] gpio: gpio-xilinx: Add support for suspend and > resume > > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> > wrote: > > > Add support for suspend and resume, pm runtime suspend and resume. > > Added free and request calls. > > > > Signed-off-by: Srinivas Neeli <srinivas.neeli@xilinx.com> > (...) > > > +static int xgpio_request(struct gpio_chip *chip, unsigned int offset) > > +{ > > + int ret; > > + > > + ret = pm_runtime_get_sync(chip->parent); > > + /* > > + * If the device is already active pm_runtime_get() will return 1 on > > + * success, but gpio_request still needs to return 0. > > + */ > > + return ret < 0 ? ret : 0; > > +} > > That's clever. I think more GPIO drivers should be doing it like this, today I > think most just ignore the return code. > > > +static int __maybe_unused xgpio_suspend(struct device *dev) static > > +int __maybe_unused xgpio_resume(struct device *dev) > > Those look good. > > > > /** > > * xgpio_remove - Remove method for the GPIO device. > > * @pdev: pointer to the platform device @@ -289,7 +323,10 @@ static > > int xgpio_remove(struct platform_device *pdev) { > > struct xgpio_instance *gpio = platform_get_drvdata(pdev); > > > > - clk_disable_unprepare(gpio->clk); > > + if (!pm_runtime_suspended(&pdev->dev)) > > + clk_disable_unprepare(gpio->clk); > > + > > + pm_runtime_disable(&pdev->dev); > > This looks complex and racy. What if the device is resumed after you > executed the first part of the statement. Could you please explain more on this. What is the need to call pm_runtime_get_sync(); in remove API ? > > The normal sequence is: > > pm_runtime_get_sync(dev); > pm_runtime_put_noidle(dev); > pm_runtime_disable(dev); > > This will make sure the clock is enabled and pm runtime is disabled. > After this you can unconditionally call clk_disable_unprepare(gpio->clk); > > It is what you are doing on the errorpath of probe(). > > Yours, > Linus Walleij
On Fri, Jan 8, 2021 at 12:41 PM Srinivas Neeli <sneeli@xilinx.com> wrote: > > On Wed, Jan 6, 2021 at 1:27 PM Srinivas Neeli <srinivas.neeli@xilinx.com> > > wrote: > > > /** > > > * xgpio_remove - Remove method for the GPIO device. > > > * @pdev: pointer to the platform device @@ -289,7 +323,10 @@ static > > > int xgpio_remove(struct platform_device *pdev) { > > > struct xgpio_instance *gpio = platform_get_drvdata(pdev); > > > > > > - clk_disable_unprepare(gpio->clk); > > > + if (!pm_runtime_suspended(&pdev->dev)) > > > + clk_disable_unprepare(gpio->clk); > > > + > > > + pm_runtime_disable(&pdev->dev); > > > > This looks complex and racy. What if the device is resumed after you > > executed the first part of the statement. > > Could you please explain more on this. > What is the need to call pm_runtime_get_sync(); in remove API ? I explain that on the lines right below your comment ;D > > The normal sequence is: > > > > pm_runtime_get_sync(dev); > > pm_runtime_put_noidle(dev); > > pm_runtime_disable(dev); > > > > This will make sure the clock is enabled and pm runtime is disabled. > > After this you can unconditionally call clk_disable_unprepare(gpio->clk); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Yours, Linus Walleij