| Message ID | 20240706-pcie-kirin-dev_err_probe-v1-1-56df797fb8ee@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: kirin: cleanup (dev_err_probe() and scoped loop) | expand |
Le 06/07/2024 à 17:07, Javier Carrasco a écrit : > dev_err_probe() is used in some probe error paths, yet the > "dev_err() + return" pattern is used in some others. > > Use dev_err_probe() in all error paths with that construction. > > Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> > Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> > --- ... > - if (ret < 0) { > - dev_err(dev, "failed to parse devfn: %d\n", ret); > - return ret; > - } > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "failed to parse devfn: %d\n", ret); Hi, with dev_err_probe(), there is no more need to add 'ret' explicitly in the message. CJ
On 06/07/2024 21:47, Christophe JAILLET wrote: > Le 06/07/2024 à 17:07, Javier Carrasco a écrit : >> dev_err_probe() is used in some probe error paths, yet the >> "dev_err() + return" pattern is used in some others. >> >> Use dev_err_probe() in all error paths with that construction. >> >> Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> >> --- > > ... > >> - if (ret < 0) { >> - dev_err(dev, "failed to parse devfn: %d\n", ret); >> - return ret; >> - } >> + if (ret < 0) >> + return dev_err_probe(dev, ret, >> + "failed to parse devfn: %d\n", ret); > > Hi, > > with dev_err_probe(), there is no more need to add 'ret' explicitly in > the message. > > CJ You are right, thank you. I will remove that from the original message for v2. Best regards, Javier Carrasco
Hello, [...] > Use dev_err_probe() in all error paths with that construction. Thank you for this nice refactoring! Much appreciated. [...] > - if (ret > MAX_PCI_SLOTS) { > - dev_err(dev, "Too many GPIO clock requests!\n"); > - return -EINVAL; > - } > + if (ret > MAX_PCI_SLOTS) > + return dev_err_probe(dev, -EINVAL, > + "Too many GPIO clock requests!\n"); Something that would be nice to get consistent: adjust all the errors capitalisation to make everything consistent, as appropriate, so that it's either all lower-case or title case. A mix of both often looks a bit sloppy. Do you think this would be something you would be willing to clean up in this series too? Especially since we are touching this code now. > - if (!dev->of_node) { > - dev_err(dev, "NULL node\n"); > - return -EINVAL; > - } > + if (!dev->of_node) > + return dev_err_probe(dev, -EINVAL, "NULL node\n"); Perhaps -ENODEV would be more appropriate here? Also, the error message is not the best, as such, I wonder if we could make it better while we are at it, so to speak. Krzysztof
On 07/07/2024 08:53, Krzysztof Wilczyński wrote: > Hello, > > [...] >> Use dev_err_probe() in all error paths with that construction. > > Thank you for this nice refactoring! Much appreciated. > > [...] >> - if (ret > MAX_PCI_SLOTS) { >> - dev_err(dev, "Too many GPIO clock requests!\n"); >> - return -EINVAL; >> - } >> + if (ret > MAX_PCI_SLOTS) >> + return dev_err_probe(dev, -EINVAL, >> + "Too many GPIO clock requests!\n"); > > Something that would be nice to get consistent: adjust all the errors > capitalisation to make everything consistent, as appropriate, so that it's > either all lower-case or title case. A mix of both often looks a bit > sloppy. > > Do you think this would be something you would be willing to clean up in > this series too? Especially since we are touching this code now. > >> - if (!dev->of_node) { >> - dev_err(dev, "NULL node\n"); >> - return -EINVAL; >> - } >> + if (!dev->of_node) >> + return dev_err_probe(dev, -EINVAL, "NULL node\n"); > > Perhaps -ENODEV would be more appropriate here? Also, the error message is > not the best, as such, I wonder if we could make it better while we are at > it, so to speak. > > Krzysztof Sure, I will add that to v2. I have seen that typical error messages in other drivers for this error are "OF node not found", "Device node not found" and "This driver needs device tree". Given that "OF data missing" is used in this driver, I will go for the first one of the list, unless something different is preferred. Best regards, Javier Carrasco
diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c index 0a29136491b8..da8091f6b22b 100644 --- a/drivers/pci/controller/dwc/pcie-kirin.c +++ b/drivers/pci/controller/dwc/pcie-kirin.c @@ -216,10 +216,8 @@ static int hi3660_pcie_phy_start(struct hi3660_pcie_phy *phy) usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX); reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0); - if (reg_val & PIPE_CLK_STABLE) { - dev_err(dev, "PIPE clk is not stable\n"); - return -EINVAL; - } + if (reg_val & PIPE_CLK_STABLE) + return dev_err_probe(dev, -EINVAL, "PIPE clk is not stable\n"); return 0; } @@ -371,10 +369,9 @@ static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie, if (ret < 0) return 0; - if (ret > MAX_PCI_SLOTS) { - dev_err(dev, "Too many GPIO clock requests!\n"); - return -EINVAL; - } + if (ret > MAX_PCI_SLOTS) + return dev_err_probe(dev, -EINVAL, + "Too many GPIO clock requests!\n"); pcie->n_gpio_clkreq = ret; @@ -421,16 +418,14 @@ static int kirin_pcie_parse_port(struct kirin_pcie *pcie, } pcie->num_slots++; - if (pcie->num_slots > MAX_PCI_SLOTS) { - dev_err(dev, "Too many PCI slots!\n"); - return -EINVAL; - } + if (pcie->num_slots > MAX_PCI_SLOTS) + return dev_err_probe(dev, -EINVAL, + "Too many PCI slots!\n"); ret = of_pci_get_devfn(child); - if (ret < 0) { - dev_err(dev, "failed to parse devfn: %d\n", ret); - return ret; - } + if (ret < 0) + return dev_err_probe(dev, ret, + "failed to parse devfn: %d\n", ret); slot = PCI_SLOT(ret); @@ -729,16 +724,12 @@ static int kirin_pcie_probe(struct platform_device *pdev) struct dw_pcie *pci; int ret; - if (!dev->of_node) { - dev_err(dev, "NULL node\n"); - return -EINVAL; - } + if (!dev->of_node) + return dev_err_probe(dev, -EINVAL, "NULL node\n"); data = of_device_get_match_data(dev); - if (!data) { - dev_err(dev, "OF data missing\n"); - return -EINVAL; - } + if (!data) + return dev_err_probe(dev, -EINVAL, "OF data missing\n"); kirin_pcie = devm_kzalloc(dev, sizeof(struct kirin_pcie), GFP_KERNEL); if (!kirin_pcie)
dev_err_probe() is used in some probe error paths, yet the "dev_err() + return" pattern is used in some others. Use dev_err_probe() in all error paths with that construction. Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com> --- drivers/pci/controller/dwc/pcie-kirin.c | 39 +++++++++++++-------------------- 1 file changed, 15 insertions(+), 24 deletions(-)