Message ID | E1p1OIG-0098J4-EV@rmk-PC.armlinux.org.uk (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net-next,v2] net: sfp: clean up i2c-bus property parsing | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Clearly marked for net-next |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/subject_prefix | success | Link |
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/patch_count | success | Link |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
netdev/module_param | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/check_selftest | success | No net selftest shell script |
netdev/verify_fixes | success | No Fixes tag |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/checkpatch | fail | ERROR: do not use assignment in if condition |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
On Sun, Dec 04, 2022 at 12:40:25AM +0800, kernel test robot wrote: > Thank you for the patch! Perhaps something to improve: Sigh... this is another stupidity. > 73970055450eeb Russell King 2017-07-25 2701 if (pdev->dev.of_node) { > 73970055450eeb Russell King 2017-07-25 @2702 struct device_node *node = pdev->dev.of_node; "node" declared here... > 259c8618b0099b Russell King 2017-12-14 2703 const struct of_device_id *id; > 73970055450eeb Russell King 2017-07-25 2704 > 259c8618b0099b Russell King 2017-12-14 2705 id = of_match_node(sfp_of_match, node); ... and clearly used here, so the code looks to be correct. However, when CONFIG_OF is not set, of_match_node() does not make use of this argument: #define of_match_node(_matches, _node) NULL which results in otherwise correct code issuing a warning when CONFIG_OF is disabled... and sure enough, your configuration has: > # CONFIG_OF is not set This illustrates just how bad an idea it is to use compiler macros for this stuff - it actively hurts compile testing, because you have to test every damn combination of configuration options to get proper coverage, which is totally and utterly rediculous. of_match_node() and ACPI_HANDLE_FWNODE() should *both* be inline functions when the subsystem is disabled, so that incorrect arguments can be detected, and warnings about unused variables such as the one you're reporting here doesn't happen. While the issue lies firmly in the realms of the DT (and ACPI) headers, I will yet again respin this patch to sort this out - but really the correct solution is to fix the bloody headers so compile coverage actually works.
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c index 39fd1811375c..09c1d10f5b5d 100644 --- a/drivers/net/phy/sfp.c +++ b/drivers/net/phy/sfp.c @@ -2642,10 +2642,46 @@ static void sfp_cleanup(void *data) kfree(sfp); } +static int sfp_i2c_get(struct sfp *sfp) +{ + struct acpi_handle *acpi_handle; + struct fwnode_handle *h; + struct i2c_adapter *i2c; + struct device_node *np; + int err; + + h = fwnode_find_reference(dev_fwnode(sfp->dev), "i2c-bus", 0); + if (IS_ERR(h)) { + dev_err(sfp->dev, "missing 'i2c-bus' property\n"); + return -ENODEV; + } + + if (is_acpi_device_node(h)) { + acpi_handle = ACPI_HANDLE_FWNODE(h); + i2c = i2c_acpi_find_adapter_by_handle(acpi_handle); + } else if ((np = to_of_node(h)) != NULL) { + i2c = of_find_i2c_adapter_by_node(np); + } else { + err = -EINVAL; + goto put; + } + + if (!i2c) { + err = -EPROBE_DEFER; + goto put; + } + + err = sfp_i2c_configure(sfp, i2c); + if (err) + i2c_put_adapter(i2c); +put: + fwnode_handle_put(h); + return err; +} + static int sfp_probe(struct platform_device *pdev) { const struct sff_data *sff; - struct i2c_adapter *i2c; char *sfp_irq_name; struct sfp *sfp; int err, i; @@ -2665,49 +2701,19 @@ static int sfp_probe(struct platform_device *pdev) if (pdev->dev.of_node) { struct device_node *node = pdev->dev.of_node; const struct of_device_id *id; - struct device_node *np; id = of_match_node(sfp_of_match, node); if (WARN_ON(!id)) return -EINVAL; sff = sfp->type = id->data; - - np = of_parse_phandle(node, "i2c-bus", 0); - if (!np) { - dev_err(sfp->dev, "missing 'i2c-bus' property\n"); - return -ENODEV; - } - - i2c = of_find_i2c_adapter_by_node(np); - of_node_put(np); - } else if (has_acpi_companion(&pdev->dev)) { - struct acpi_device *adev = ACPI_COMPANION(&pdev->dev); - struct fwnode_handle *fw = acpi_fwnode_handle(adev); - struct fwnode_reference_args args; - struct acpi_handle *acpi_handle; - int ret; - - ret = acpi_node_get_property_reference(fw, "i2c-bus", 0, &args); - if (ret || !is_acpi_device_node(args.fwnode)) { - dev_err(&pdev->dev, "missing 'i2c-bus' property\n"); - return -ENODEV; - } - - acpi_handle = ACPI_HANDLE_FWNODE(args.fwnode); - i2c = i2c_acpi_find_adapter_by_handle(acpi_handle); - } else { + } else if (!has_acpi_companion(&pdev->dev)) { return -EINVAL; } - if (!i2c) - return -EPROBE_DEFER; - - err = sfp_i2c_configure(sfp, i2c); - if (err < 0) { - i2c_put_adapter(i2c); + err = sfp_i2c_get(sfp); + if (err) return err; - } for (i = 0; i < GPIO_MAX; i++) if (sff->gpios & BIT(i)) {
We currently have some complicated code in sfp_probe() which gets the I2C bus depending on whether the sfp node is DT or ACPI, and we use completely separate lookup functions. This could do with being in a separate function to make the code more readable, so move it to a new function, sfp_i2c_get(). We can also use fwnode_find_reference() to lookup the I2C bus fwnode before then decending into fwnode-type specific parsing. A future cleanup would be to move the fwnode-type specific parsing into the i2c layer, which is where it really should be. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- v2: actually send version with ACPI build error fixed (doesn't show up unless ACPI is enabled.) drivers/net/phy/sfp.c | 74 +++++++++++++++++++++++-------------------- 1 file changed, 40 insertions(+), 34 deletions(-)