| Message ID | 20241206112843.98720-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: imx8m: Add remove function | expand |
On 24-12-06, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Unregister the cpufreq device and soc device in remove path, otherwise > there will be warning when do removing test: > sysfs: cannot create duplicate filename '/devices/platform/imx-cpufreq-dt' > CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.13.0-rc1-next-20241204 > Hardware name: NXP i.MX8MPlus EVK board (DT) > > Fixes: 9cc832d37799 ("soc: imx8m: Probe the SoC driver as platform driver") > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > drivers/soc/imx/soc-imx8m.c | 32 +++++++++++++++++++++++++++----- > 1 file changed, 27 insertions(+), 5 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index 8ac7658e3d52..8c368947d1e5 100644 > --- a/drivers/soc/imx/soc-imx8m.c > +++ b/drivers/soc/imx/soc-imx8m.c > @@ -33,6 +33,11 @@ struct imx8_soc_data { > int (*soc_revision)(u32 *socrev, u64 *socuid); > }; > > +struct imx8m_soc_priv { > + struct soc_device *soc_dev; > + struct platform_device *cpufreq_dev; > +}; > + > #ifdef CONFIG_HAVE_ARM_SMCCC > static u32 imx8mq_soc_revision_from_atf(void) > { > @@ -198,7 +203,7 @@ static int imx8m_soc_probe(struct platform_device *pdev) > const struct imx8_soc_data *data; > struct device *dev = &pdev->dev; > const struct of_device_id *id; > - struct soc_device *soc_dev; > + struct imx8m_soc_priv *priv; > u32 soc_rev = 0; > u64 soc_uid = 0; > int ret; > @@ -207,6 +212,10 @@ static int imx8m_soc_probe(struct platform_device *pdev) > if (!soc_dev_attr) > return -ENOMEM; > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > soc_dev_attr->family = "Freescale i.MX"; > > ret = of_property_read_string(of_root, "model", &soc_dev_attr->machine); > @@ -235,21 +244,34 @@ static int imx8m_soc_probe(struct platform_device *pdev) > if (!soc_dev_attr->serial_number) > return -ENOMEM; > > - soc_dev = soc_device_register(soc_dev_attr); > - if (IS_ERR(soc_dev)) > - return PTR_ERR(soc_dev); > + priv->soc_dev = soc_device_register(soc_dev_attr); > + if (IS_ERR(priv->soc_dev)) > + return PTR_ERR(priv->soc_dev); > > pr_info("SoC: %s revision %s\n", soc_dev_attr->soc_id, > soc_dev_attr->revision); > > if (IS_ENABLED(CONFIG_ARM_IMX_CPUFREQ_DT)) > - platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); > + priv->cpufreq_dev = platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); If CONFIG_ARM_IMX_CPUFREQ_DT is enabled, I asusme that platform_device_register_simple() shouldn't fail else it will be an error, right? Therefore I would like to add the 'if(!IS_ERR())' check here instead of the remove function. > + > + platform_set_drvdata(pdev, priv); > > return 0; > } > > +static void imx8m_soc_remove(struct platform_device *pdev) > +{ > + struct imx8m_soc_priv *priv = platform_get_drvdata(pdev); > + > + if (!IS_ERR(priv->cpufreq_dev)) With the above shifted, we only need to: if (priv->cpufreq_dev) Regards, Marco > + platform_device_unregister(priv->cpufreq_dev); > + > + soc_device_unregister(priv->soc_dev); > +} > + > static struct platform_driver imx8m_soc_driver = { > .probe = imx8m_soc_probe, > + .remove = imx8m_soc_remove, > .driver = { > .name = "imx8m-soc", > }, > -- > 2.37.1 > > >
> Subject: Re: [PATCH] soc: imx8m: Add remove function > > On 24-12-06, Peng Fan (OSS) wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > Unregister the cpufreq device and soc device in remove path, > otherwise > > there will be warning when do removing test: > > sysfs: cannot create duplicate filename '/devices/platform/imx- > cpufreq-dt' > > CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted > > 6.13.0-rc1-next-20241204 Hardware name: NXP i.MX8MPlus EVK > board (DT) > > > > Fixes: 9cc832d37799 ("soc: imx8m: Probe the SoC driver as platform > > driver") > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > drivers/soc/imx/soc-imx8m.c | 32 +++++++++++++++++++++++++++---- > - > > 1 file changed, 27 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc- > imx8m.c > > index 8ac7658e3d52..8c368947d1e5 100644 > > --- a/drivers/soc/imx/soc-imx8m.c > > +++ b/drivers/soc/imx/soc-imx8m.c > > @@ -33,6 +33,11 @@ struct imx8_soc_data { > > int (*soc_revision)(u32 *socrev, u64 *socuid); }; > > > > +struct imx8m_soc_priv { > > + struct soc_device *soc_dev; > > + struct platform_device *cpufreq_dev; }; > > + > > #ifdef CONFIG_HAVE_ARM_SMCCC > > static u32 imx8mq_soc_revision_from_atf(void) > > { > > @@ -198,7 +203,7 @@ static int imx8m_soc_probe(struct > platform_device *pdev) > > const struct imx8_soc_data *data; > > struct device *dev = &pdev->dev; > > const struct of_device_id *id; > > - struct soc_device *soc_dev; > > + struct imx8m_soc_priv *priv; > > u32 soc_rev = 0; > > u64 soc_uid = 0; > > int ret; > > @@ -207,6 +212,10 @@ static int imx8m_soc_probe(struct > platform_device *pdev) > > if (!soc_dev_attr) > > return -ENOMEM; > > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > soc_dev_attr->family = "Freescale i.MX"; > > > > ret = of_property_read_string(of_root, "model", > > &soc_dev_attr->machine); @@ -235,21 +244,34 @@ static int > imx8m_soc_probe(struct platform_device *pdev) > > if (!soc_dev_attr->serial_number) > > return -ENOMEM; > > > > - soc_dev = soc_device_register(soc_dev_attr); > > - if (IS_ERR(soc_dev)) > > - return PTR_ERR(soc_dev); > > + priv->soc_dev = soc_device_register(soc_dev_attr); > > + if (IS_ERR(priv->soc_dev)) > > + return PTR_ERR(priv->soc_dev); > > > > pr_info("SoC: %s revision %s\n", soc_dev_attr->soc_id, > > soc_dev_attr->revision); > > > > if (IS_ENABLED(CONFIG_ARM_IMX_CPUFREQ_DT)) > > - platform_device_register_simple("imx-cpufreq-dt", -1, > NULL, 0); > > + priv->cpufreq_dev = > > +platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); > > If CONFIG_ARM_IMX_CPUFREQ_DT is enabled, I asusme that > platform_device_register_simple() shouldn't fail else it will be an error, > right? Therefore I would like to add the 'if(!IS_ERR())' check here > instead of the remove function. You mean below? dev = platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); if (!IS_ERR(dev)) plat->cpufreq_dev = dev; else pr_err("Failed to register imx-cpufreq-dt: %d\n", ERR_PTR(dev))? But using !IS_ERR(plat->cpufreq_dev) in remove path looks simpler. Thanks, Peng > > > + > > + platform_set_drvdata(pdev, priv); > > > > return 0; > > } > > > > +static void imx8m_soc_remove(struct platform_device *pdev) { > > + struct imx8m_soc_priv *priv = platform_get_drvdata(pdev); > > + > > + if (!IS_ERR(priv->cpufreq_dev)) > > With the above shifted, we only need to: > if (priv->cpufreq_dev) > > Regards, > Marco > > > > + platform_device_unregister(priv->cpufreq_dev); > > + > > + soc_device_unregister(priv->soc_dev); > > +} > > + > > static struct platform_driver imx8m_soc_driver = { > > .probe = imx8m_soc_probe, > > + .remove = imx8m_soc_remove, > > .driver = { > > .name = "imx8m-soc", > > }, > > -- > > 2.37.1 > > > > > >
diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c index 8ac7658e3d52..8c368947d1e5 100644 --- a/drivers/soc/imx/soc-imx8m.c +++ b/drivers/soc/imx/soc-imx8m.c @@ -33,6 +33,11 @@ struct imx8_soc_data { int (*soc_revision)(u32 *socrev, u64 *socuid); }; +struct imx8m_soc_priv { + struct soc_device *soc_dev; + struct platform_device *cpufreq_dev; +}; + #ifdef CONFIG_HAVE_ARM_SMCCC static u32 imx8mq_soc_revision_from_atf(void) { @@ -198,7 +203,7 @@ static int imx8m_soc_probe(struct platform_device *pdev) const struct imx8_soc_data *data; struct device *dev = &pdev->dev; const struct of_device_id *id; - struct soc_device *soc_dev; + struct imx8m_soc_priv *priv; u32 soc_rev = 0; u64 soc_uid = 0; int ret; @@ -207,6 +212,10 @@ static int imx8m_soc_probe(struct platform_device *pdev) if (!soc_dev_attr) return -ENOMEM; + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + soc_dev_attr->family = "Freescale i.MX"; ret = of_property_read_string(of_root, "model", &soc_dev_attr->machine); @@ -235,21 +244,34 @@ static int imx8m_soc_probe(struct platform_device *pdev) if (!soc_dev_attr->serial_number) return -ENOMEM; - soc_dev = soc_device_register(soc_dev_attr); - if (IS_ERR(soc_dev)) - return PTR_ERR(soc_dev); + priv->soc_dev = soc_device_register(soc_dev_attr); + if (IS_ERR(priv->soc_dev)) + return PTR_ERR(priv->soc_dev); pr_info("SoC: %s revision %s\n", soc_dev_attr->soc_id, soc_dev_attr->revision); if (IS_ENABLED(CONFIG_ARM_IMX_CPUFREQ_DT)) - platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); + priv->cpufreq_dev = platform_device_register_simple("imx-cpufreq-dt", -1, NULL, 0); + + platform_set_drvdata(pdev, priv); return 0; } +static void imx8m_soc_remove(struct platform_device *pdev) +{ + struct imx8m_soc_priv *priv = platform_get_drvdata(pdev); + + if (!IS_ERR(priv->cpufreq_dev)) + platform_device_unregister(priv->cpufreq_dev); + + soc_device_unregister(priv->soc_dev); +} + static struct platform_driver imx8m_soc_driver = { .probe = imx8m_soc_probe, + .remove = imx8m_soc_remove, .driver = { .name = "imx8m-soc", },