Messages in this thread | | | Subject | Re: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021 | From | Philipp Zabel <> | Date | Wed, 03 Nov 2021 13:10:06 +0100 |
| |
On Wed, 2021-11-03 at 19:02 +0800, Wells Lu wrote: [...] > diff --git a/drivers/net/ethernet/sunplus/l2sw_driver.c b/drivers/net/ethernet/sunplus/l2sw_driver.c > new file mode 100644 > index 0000000..3dfd0dd > --- /dev/null > +++ b/drivers/net/ethernet/sunplus/l2sw_driver.c > @@ -0,0 +1,779 @@ [...] > +static int l2sw_probe(struct platform_device *pdev) > +{ > + struct l2sw_common *comm; > + struct resource *r_mem; > + struct net_device *net_dev, *net_dev2; > + struct l2sw_mac *mac, *mac2; > + u32 mode; > + int ret = 0; > + int rc; > + > + if (platform_get_drvdata(pdev)) > + return -ENODEV; > + > + // Allocate memory for l2sw 'common' area. > + comm = kmalloc(sizeof(*comm), GFP_KERNEL);
I'd use devm_kzalloc() here for initialization and to simplify the cleanup path.
> + if (!comm) > + return -ENOMEM; > + pr_debug(" comm = %p\n", comm);
What is this useful for?
> + memset(comm, '\0', sizeof(struct l2sw_common));
Not needed with kzalloc, See above.
[...] > + // Get memory resoruce 0 from dts. > + r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (r_mem) { > + pr_debug(" res->name = \"%s\", r_mem->start = %pa\n", r_mem->name, &r_mem->start); > + if (l2sw_reg_base_set(devm_ioremap(&pdev->dev, r_mem->start, > + (r_mem->end - r_mem->start + 1))) != 0) { > + pr_err(" ioremap failed!\n"); > + ret = -ENOMEM; > + goto out_free_comm; > + } > + } else { > + pr_err(" No MEM resource 0 found!\n"); > + ret = -ENXIO; > + goto out_free_comm; > + } > + > + // Get memory resoruce 1 from dts. > + r_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1); > + if (r_mem) { > + pr_debug(" res->name = \"%s\", r_mem->start = %pa\n", r_mem->name, &r_mem->start); > + if (moon5_reg_base_set(devm_ioremap(&pdev->dev, r_mem->start, > + (r_mem->end - r_mem->start + 1))) != 0) { > + pr_err(" ioremap failed!\n"); > + ret = -ENOMEM; > + goto out_free_comm; > + } > + } else { > + pr_err(" No MEM resource 1 found!\n"); > + ret = -ENXIO; > + goto out_free_comm; > + }
Using devm_ioremap_resource() would simplify both a lot.
[...] > + comm->rstc = devm_reset_control_get(&pdev->dev, NULL);
Please use devm_reset_control_get_exclusive().
> + if (IS_ERR(comm->rstc)) { > + dev_err(&pdev->dev, "Failed to retrieve reset controller!\n"); > + ret = PTR_ERR(comm->rstc); > + goto out_free_comm; > + } > + > + // Enable clock. > + clk_prepare_enable(comm->clk); > + udelay(1); > + > + ret = reset_control_assert(comm->rstc);
No need to assign to ret if you ignore it anyway.
> + udelay(1); > + ret = reset_control_deassert(comm->rstc); > + if (ret) { > + dev_err(&pdev->dev, "Failed to deassert reset line (err = %d)!\n", ret); > + ret = -ENODEV; > + goto out_free_comm; > + } > + udelay(1);
regards Philipp
| |