| Message ID | E1jIVU9-0005h4-QU@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PWM fan support on Clearfog gt8k | expand |
On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote: > Honour deferred probing for devm_clk_get() so that we can get the clock > for PWM. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpio/gpio-mvebu.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index fa5641615db6..ee13b11c5298 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) > } > > mvchip->clk = devm_clk_get(&pdev->dev, NULL); > + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) > + return -EPROBE_DEFER; > + > /* Not all SoCs require a clock.*/ > if (!IS_ERR(mvchip->clk)) > clk_prepare_enable(mvchip->clk); I'd say the following is the right thing to do here: mvchip->clk = devm_clk_get_optional(...); if (IS_ERR(mvchip->clk)) return ... Best regards Uwe
On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote: > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote: > > Honour deferred probing for devm_clk_get() so that we can get the clock > > for PWM. > > > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > > --- > > drivers/gpio/gpio-mvebu.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > index fa5641615db6..ee13b11c5298 100644 > > --- a/drivers/gpio/gpio-mvebu.c > > +++ b/drivers/gpio/gpio-mvebu.c > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) > > } > > > > mvchip->clk = devm_clk_get(&pdev->dev, NULL); > > + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) > > + return -EPROBE_DEFER; > > + > > /* Not all SoCs require a clock.*/ > > if (!IS_ERR(mvchip->clk)) > > clk_prepare_enable(mvchip->clk); > > I'd say the following is the right thing to do here: > > mvchip->clk = devm_clk_get_optional(...); > if (IS_ERR(mvchip->clk)) > return ... It's not that simple. The clock is required for Armada 370, and is optional for Armada 8040.
Hello Russell, On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote: > On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote: > > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote: > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > > index fa5641615db6..ee13b11c5298 100644 > > > --- a/drivers/gpio/gpio-mvebu.c > > > +++ b/drivers/gpio/gpio-mvebu.c > > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) > > > } > > > > > > mvchip->clk = devm_clk_get(&pdev->dev, NULL); > > > + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) > > > + return -EPROBE_DEFER; > > > + > > > /* Not all SoCs require a clock.*/ > > > if (!IS_ERR(mvchip->clk)) > > > clk_prepare_enable(mvchip->clk); > > > > I'd say the following is the right thing to do here: > > > > mvchip->clk = devm_clk_get_optional(...); > > if (IS_ERR(mvchip->clk)) > > return ... > > It's not that simple. The clock is required for Armada 370, and is > optional for Armada 8040. I'd say it is still the right approach here. On Armada 370 the dtb then has a clk and on Armada 8040 it doesn't. So if with devm_clk_get_optional() something goes wrong that's because the dtb is wrong. And in fact the handling is even better than with your suggested patch as every error (but EPROBE_DEFER) is ignored instead of passed to the caller with your (and the existing) approach. Best regards Uwe
On Sun, Mar 29, 2020 at 08:00:56PM +0200, Uwe Kleine-König wrote: > Hello Russell, > > On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote: > > On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote: > > > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote: > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > > > index fa5641615db6..ee13b11c5298 100644 > > > > --- a/drivers/gpio/gpio-mvebu.c > > > > +++ b/drivers/gpio/gpio-mvebu.c > > > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) > > > > } > > > > > > > > mvchip->clk = devm_clk_get(&pdev->dev, NULL); > > > > + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) > > > > + return -EPROBE_DEFER; > > > > + > > > > /* Not all SoCs require a clock.*/ > > > > if (!IS_ERR(mvchip->clk)) > > > > clk_prepare_enable(mvchip->clk); > > > > > > I'd say the following is the right thing to do here: > > > > > > mvchip->clk = devm_clk_get_optional(...); > > > if (IS_ERR(mvchip->clk)) > > > return ... > > > > It's not that simple. The clock is required for Armada 370, and is > > optional for Armada 8040. > > I'd say it is still the right approach here. On Armada 370 the dtb then > has a clk and on Armada 8040 it doesn't. So if with > devm_clk_get_optional() something goes wrong that's because the dtb is > wrong. And in fact the handling is even better than with your suggested > patch as every error (but EPROBE_DEFER) is ignored instead of passed to > the caller with your (and the existing) approach. Sort of. Every error is currently treated as "no clock", and only later does such an error become fatal in the driver _if_ PWM is configured into the kernel and we're running on Armada 370. If PWM is disabled in the kernel, or on some other SoC, then the driver doesn't care whether getting the clock reported any kind of error. Your proposal is to always treat any error getting the clock, irrespective of whether there is PWM or not, as a fatal error for the driver. That is an entirely seperate functional change.
Hello Russell, On Sun, Mar 29, 2020 at 07:22:36PM +0100, Russell King - ARM Linux admin wrote: > On Sun, Mar 29, 2020 at 08:00:56PM +0200, Uwe Kleine-König wrote: > > On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote: > > > On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote: > > > > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote: > > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > > > > > index fa5641615db6..ee13b11c5298 100644 > > > > > --- a/drivers/gpio/gpio-mvebu.c > > > > > +++ b/drivers/gpio/gpio-mvebu.c > > > > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) > > > > > } > > > > > > > > > > mvchip->clk = devm_clk_get(&pdev->dev, NULL); > > > > > + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) > > > > > + return -EPROBE_DEFER; > > > > > + > > > > > /* Not all SoCs require a clock.*/ > > > > > if (!IS_ERR(mvchip->clk)) > > > > > clk_prepare_enable(mvchip->clk); > > > > > > > > I'd say the following is the right thing to do here: > > > > > > > > mvchip->clk = devm_clk_get_optional(...); > > > > if (IS_ERR(mvchip->clk)) > > > > return ... > > > > > > It's not that simple. The clock is required for Armada 370, and is > > > optional for Armada 8040. > > > > I'd say it is still the right approach here. On Armada 370 the dtb then > > has a clk and on Armada 8040 it doesn't. So if with > > devm_clk_get_optional() something goes wrong that's because the dtb is > > wrong. And in fact the handling is even better than with your suggested > > patch as every error (but EPROBE_DEFER) is ignored instead of passed to > > the caller with your (and the existing) approach. > > Sort of. Every error is currently treated as "no clock", and only > later does such an error become fatal in the driver _if_ PWM is > configured into the kernel and we're running on Armada 370. If PWM > is disabled in the kernel, or on some other SoC, then the driver > doesn't care whether getting the clock reported any kind of error. > > Your proposal is to always treat any error getting the clock, > irrespective of whether there is PWM or not, as a fatal error for > the driver. Is this clock (assuming it's available) needed for GPIO operation? If not, I'd say the call to devm_clk_get should go into mvebu_pwm_probe(). And if yes, then use devm_clk_get_optional in mvebu_gpio_probe() and either request it once more in mvebu_pwm_probe() (without _optional) or test for mvchip->clk == NULL. (Or maybe just don't check and let the driver fail when clk_get_rate(mvchip->clk) returns zero.) > That is an entirely seperate functional change. This is still different to what you do, but it is (IMHO) cleaner and fixes the problem you want to solve en passant. Best regards Uwe
diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c index fa5641615db6..ee13b11c5298 100644 --- a/drivers/gpio/gpio-mvebu.c +++ b/drivers/gpio/gpio-mvebu.c @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev) } mvchip->clk = devm_clk_get(&pdev->dev, NULL); + if (mvchip->clk == ERR_PTR(-EPROBE_DEFER)) + return -EPROBE_DEFER; + /* Not all SoCs require a clock.*/ if (!IS_ERR(mvchip->clk)) clk_prepare_enable(mvchip->clk);
Honour deferred probing for devm_clk_get() so that we can get the clock for PWM. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/gpio/gpio-mvebu.c | 3 +++ 1 file changed, 3 insertions(+)