On Fri, Mar 08, 2024 at 03:19:47PM +0530, Manivannan Sadhasivam wrote:

quoted

quoted

quoted

quoted

@@ -467,6 +467,13 @@ static int dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx,
 		return ret;
 	}
 
+	ret = dw_pcie_ep_init_registers(ep);
+	if (ret) {

Here you are using if (ret) to error check the return from
dw_pcie_ep_init_registers().

quoted

index c0c62533a3f1..8392894ed286 100644

--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c

@@ -1286,6 +1286,13 @@ static int ks_pcie_probe(struct platform_device *pdev)
 		ret = dw_pcie_ep_init(&pci->ep);
 		if (ret < 0)
 			goto err_get_sync;
+
+		ret = dw_pcie_ep_init_registers(&pci->ep);
+		if (ret < 0) {

Here you are using if (ret < 0) to error check the return from
dw_pcie_ep_init_registers(). Please be consistent.

I maintained the consistency w.r.t individual drivers. Please check them
individually.

If I maintain consistency w.r.t this patch, then the style will change within
the drivers.

Personally, I disagree with that.

All glue drivers should use the same way of checking dw_pcie_ep_init(),
depending on the kdoc of dw_pcie_ep_init().

If the kdoc for dw_pcie_ep_init() says returns 0 on success,
then I think that it is strictly more correct to do:

ret = dw_pcie_ep_init()
if (ret) {
	<error handling>
}

And if a glue driver doesn't look like that, then I think we should change
them. (Same reasoning for dw_pcie_ep_init_registers().)


If you read code that looks like:
ret = dw_pcie_ep_init()
if (ret < 0) {
	<error handling>
}

then you assume that is is a function with a kdoc that says it can return 0
or a positive value on success, e.g. a function that returns an index in an
array.

But if you read the same function from the individual drivers, it could present
a different opinion because the samantics is different than others.

Is there any glue driver where a positive result from dw_pcie_ep_init() is
considered valid?

I'm not opposed to keeping the API semantics consistent, but we have to take
account of the drivers style as well.

kdoc > "driver style"
IMO, but you are the maintainer, I just offered my 50 cents :)


Kind regards,
Niklas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel