| Message ID | 20200902150348.14465-4-krzk@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | [01/10] clk: at91: Drop unused at91sam9g45_pcr_layout | expand |
Quoting Krzysztof Kozlowski (2020-09-02 08:03:42) > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c > index 38755a241ab7..a3cc53edcb11 100644 > --- a/drivers/clk/clk-gpio.c > +++ b/drivers/clk/clk-gpio.c > @@ -211,17 +210,10 @@ static int gpio_clk_driver_probe(struct platform_device *pdev) > > gpio_name = is_mux ? "select" : "enable"; > gpiod = devm_gpiod_get(dev, gpio_name, GPIOD_OUT_LOW); > - if (IS_ERR(gpiod)) { > - ret = PTR_ERR(gpiod); > - if (ret == -EPROBE_DEFER) > - pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n", > - node, __func__); > - else > - pr_err("%pOFn: %s: Can't get '%s' named GPIO property\n", > - node, __func__, > - gpio_name); > - return ret; > - } > + if (IS_ERR(gpiod)) > + return dev_err_probe(dev, PTR_ERR(gpiod), This is cool! I wonder if we could make it even more simplified with ret = dev_err_probe_ptr(dev, ptr, ...) if (ret) return ret; then we don't have to do the PTR_ERR() or IS_ERR() dance in all the drivers. It could already be changed here to look at the return value of dev_err_probe() so please do that at the least. It would also even be more super duper cool if we had a way to save some sort of cookie when the provider can't find it and is returning the -EPROBE_DEFER value. Maybe the provider could use device_set_deferred_probe_reason() on error and then if dev_err_probe() is called without any string it can print what is in the device's deferred probe reason? Or append to it whatever string is passed from the device driver? Sometimes the provider has more info like the DT property is malformed or the provider isn't probed yet which would probably help understand the deferred problem more.
On Thu, Sep 10, 2020 at 03:01:06PM -0700, Stephen Boyd wrote: > Quoting Krzysztof Kozlowski (2020-09-02 08:03:42) > > diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c > > index 38755a241ab7..a3cc53edcb11 100644 > > --- a/drivers/clk/clk-gpio.c > > +++ b/drivers/clk/clk-gpio.c > > @@ -211,17 +210,10 @@ static int gpio_clk_driver_probe(struct platform_device *pdev) > > > > gpio_name = is_mux ? "select" : "enable"; > > gpiod = devm_gpiod_get(dev, gpio_name, GPIOD_OUT_LOW); > > - if (IS_ERR(gpiod)) { > > - ret = PTR_ERR(gpiod); > > - if (ret == -EPROBE_DEFER) > > - pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n", > > - node, __func__); > > - else > > - pr_err("%pOFn: %s: Can't get '%s' named GPIO property\n", > > - node, __func__, > > - gpio_name); > > - return ret; > > - } > > + if (IS_ERR(gpiod)) > > + return dev_err_probe(dev, PTR_ERR(gpiod), > > This is cool! I wonder if we could make it even more simplified with > > ret = dev_err_probe_ptr(dev, ptr, ...) > if (ret) > return ret; > > then we don't have to do the PTR_ERR() or IS_ERR() dance in all the > drivers. It could already be changed here to look at the return value of > dev_err_probe() so please do that at the least. I think this could be achieved with Rob's work here: git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git dev_err-removal It would make this patchset obsolete. Up to you then if you plan to wait for Rob's work. > It would also even be more super duper cool if we had a way to save some > sort of cookie when the provider can't find it and is returning the > -EPROBE_DEFER value. Maybe the provider could use > device_set_deferred_probe_reason() on error and then if dev_err_probe() > is called without any string it can print what is in the device's > deferred probe reason? Or append to it whatever string is passed from > the device driver? Sometimes the provider has more info like the DT > property is malformed or the provider isn't probed yet which would > probably help understand the deferred problem more. Yes, good point. If all (or most) messages are moved to the providers, it would be easier to print also the defer reasons. Best regards, Krzysztof
diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c index 38755a241ab7..a3cc53edcb11 100644 --- a/drivers/clk/clk-gpio.c +++ b/drivers/clk/clk-gpio.c @@ -199,7 +199,6 @@ static int gpio_clk_driver_probe(struct platform_device *pdev) struct gpio_desc *gpiod; struct clk_hw *hw; bool is_mux; - int ret; is_mux = of_device_is_compatible(node, "gpio-mux-clock"); @@ -211,17 +210,10 @@ static int gpio_clk_driver_probe(struct platform_device *pdev) gpio_name = is_mux ? "select" : "enable"; gpiod = devm_gpiod_get(dev, gpio_name, GPIOD_OUT_LOW); - if (IS_ERR(gpiod)) { - ret = PTR_ERR(gpiod); - if (ret == -EPROBE_DEFER) - pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n", - node, __func__); - else - pr_err("%pOFn: %s: Can't get '%s' named GPIO property\n", - node, __func__, - gpio_name); - return ret; - } + if (IS_ERR(gpiod)) + return dev_err_probe(dev, PTR_ERR(gpiod), + "%pOFn: Can't get '%s' named GPIO property\n", + node, gpio_name); if (is_mux) hw = clk_hw_register_gpio_mux(dev, gpiod);
Common pattern of handling deferred probe can be simplified with dev_err_probe(). Less code and the error value gets printed. Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org> --- drivers/clk/clk-gpio.c | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-)