lkml.org 
[lkml]   [2021]   [Nov]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] net: ethernet: Add driver for Sunplus SP7021
    From
    Date
    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

    \
     
     \ /
      Last update: 2021-11-03 13:11    [W:4.647 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site