Messages in this thread | | | Date | Thu, 2 Jun 2022 23:07:01 +0200 | Subject | Re: [PATCH v8 6/6] drivers: remoteproc: Add Xilinx r5 remoteproc driver | From | Christophe JAILLET <> |
| |
Hi,
should there be a v9, a nitpick below.
Le 02/06/2022 à 22:38, Tanmay Shah a écrit : > This driver enables r5f dual core Real time Processing Unit subsystem > available on Xilinx Zynq Ultrascale MPSoC Platform. RPU subsystem > (cluster) can be configured in different modes e.g. split mode in which > two r5f cores work independent of each other and lock-step mode in which > both r5f cores execute same code clock-for-clock and notify if the > result is different. > > The Xilinx r5 Remoteproc Driver boots the RPU cores via calls to the Xilinx > Platform Management Unit that handles the R5 configuration, memory access > and R5 lifecycle management. The interface to this manager is done in this > driver via zynqmp_pm_* function calls. > > Signed-off-by: Ben Levinsky <ben.levinsky-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> > Signed-off-by: Tanmay Shah <tanmay.shah-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> > --- >
[...]
> +static void zynqmp_r5_cluster_exit(void *data) > +{ > + struct platform_device *pdev = (struct platform_device *)data; > + struct zynqmp_r5_cluster *cluster; > + int i; > + > + cluster = (struct zynqmp_r5_cluster *)platform_get_drvdata(pdev); > + if (!cluster) > + return; > + > + for (i = 0; i < cluster->core_count; i++) { > + zynqmp_r5_core_exit(cluster->r5_cores[i]); > + cluster->r5_cores[i] = NULL; > + } > + > + kfree(cluster->r5_cores); > + kfree(cluster);
why not remove this kfree(cluster) here...
> + platform_set_drvdata(pdev, NULL); > +} > + > +/* > + * zynqmp_r5_remoteproc_probe() > + * > + * @pdev: domain platform device for R5 cluster > + * > + * called when driver is probed, for each R5 core specified in DT, > + * setup as needed to do remoteproc-related operations > + * > + * Return: 0 for success, negative value for failure. > + */ > +static int zynqmp_r5_remoteproc_probe(struct platform_device *pdev) > +{ > + int ret; > + struct zynqmp_r5_cluster *cluster; > + struct device *dev = &pdev->dev; > + > + cluster = kzalloc(sizeof(*cluster), GFP_KERNEL);
... devm_kzalloc() here...
> + if (!cluster) > + return -ENOMEM; > + > + cluster->dev = dev; > + > + ret = devm_of_platform_populate(dev); > + if (ret) { > + dev_err_probe(dev, ret, "failed to populate platform dev\n"); > + kfree(cluster); > + return ret;
and return dev_err_probe() here (without the kfree)? Would'nt it be cleaner?
just my 2c
CJ
> + } > + > + /* wire in so each core can be cleaned up at driver remove */ > + platform_set_drvdata(pdev, cluster); > + > + ret = zynqmp_r5_cluster_init(cluster); > + if (ret) { > + zynqmp_r5_cluster_exit(pdev); > + dev_err_probe(dev, ret, "Invalid r5f subsystem device tree\n"); > + return ret; > + } > + > + ret = devm_add_action_or_reset(dev, zynqmp_r5_cluster_exit, pdev); > + if (ret) > + return ret; > + > + return 0; > +} > +
[...]
| |